[PATCH 01/14] drm/i915/display: Modify debugfs for joiner to force n pipes
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Tue Sep 10 14:10:04 UTC 2024
On 9/10/2024 5:16 PM, Ville Syrjälä wrote:
> On Tue, Sep 10, 2024 at 11:12:30AM +0530, Nautiyal, Ankit K wrote:
>> On 9/9/2024 7:10 PM, Ville Syrjälä wrote:
>>> On Mon, Sep 09, 2024 at 11:10:16AM +0530, Nautiyal, Ankit K wrote:
>>>> On 9/6/2024 8:24 PM, Ville Syrjälä wrote:
>>>>> On Fri, Sep 06, 2024 at 05:46:11PM +0300, Ville Syrjälä wrote:
>>>>>> On Fri, Sep 06, 2024 at 06:27:54PM +0530, Ankit Nautiyal wrote:
>>>>>>> At the moment, the debugfs for joiner allows only to force enable/disable
>>>>>>> pipe joiner for 2 pipes. Modify it to force join 'n' number of pipes,
>>>>>>> where n is a valid pipe joiner configuration.
>>>>>>> This will help in case of ultra joiner where 4 pipes are joined.
>>>>>>>
>>>>>>> v2:
>>>>>>> -Fix commit message to state that only valid joiner config can be
>>>>>>> forced. (Suraj)
>>>>>>> -Rename the identifiers to have INTEL_BIG/NONE_JOINER_PIPES. (Suraj)
>>>>>>>
>>>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>>>>>>> ---
>>>>>>> .../drm/i915/display/intel_display_debugfs.c | 71 ++++++++++++++++++-
>>>>>>> .../drm/i915/display/intel_display_types.h | 8 ++-
>>>>>>> drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
>>>>>>> 3 files changed, 77 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>>>>>>> index 830b9eb60976..0ef573afd8a1 100644
>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>>>>>>> @@ -1504,6 +1504,73 @@ static int intel_crtc_pipe_show(struct seq_file *m, void *unused)
>>>>>>> }
>>>>>>> DEFINE_SHOW_ATTRIBUTE(intel_crtc_pipe);
>>>>>>>
>>>>>>> +static int i915_joiner_show(struct seq_file *m, void *data)
>>>>>>> +{
>>>>>>> + struct intel_connector *connector = m->private;
>>>>>>> + struct drm_i915_private *i915 = to_i915(connector->base.dev);
>>>>>>> + int ret;
>>>>>>> +
>>>>>>> + ret = drm_modeset_lock_single_interruptible(&i915->drm.mode_config.connection_mutex);
>>>>>>> + if (ret)
>>>>>>> + return ret;
>>>>>> What does that lock do for us?
>>>>>>
>>>>>>> +
>>>>>>> + seq_printf(m, "Force_joined_pipes: %d\n", connector->force_joined_pipes);
>>>>>> This should just be thae bare number. Adding other junk in there just
>>>>>> complicates matters if anyone has to parse this.
>>>>>>
>>>>>>> +
>>>>>>> + drm_modeset_unlock(&i915->drm.mode_config.connection_mutex);
>>>>>>> +
>>>>>>> + return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static ssize_t i915_joiner_write(struct file *file,
>>>>>>> + const char __user *ubuf,
>>>>>>> + size_t len, loff_t *offp)
>>>>>>> +{
>>>>>>> + struct seq_file *m = file->private_data;
>>>>>>> + struct intel_connector *connector = m->private;
>>>>>>> + struct drm_i915_private *i915 = to_i915(connector->base.dev);
>>>>>>> + int force_join_pipes = 0;
>>>>>>> + int ret;
>>>>>>> +
>>>>>>> + if (len == 0)
>>>>>>> + return 0;
>>>>>>> +
>>>>>>> + drm_dbg(&i915->drm,
>>>>>>> + "Copied %zu bytes from user to force joiner\n", len);
>>>>>> Leftover debug junk.
>>>>>>
>>>>>>> +
>>>>>>> + ret = kstrtoint_from_user(ubuf, len, 0, &force_join_pipes);
>>>>>>> + if (ret < 0)
>>>>>>> + return ret;
>>>>>>> +
>>>>>>> + drm_dbg(&i915->drm, "Got %d for force joining pipes\n", force_join_pipes);
>>>>>> More.
>>>>>>
>>>>>>> +
>>>>>>> + if (force_join_pipes < INTEL_NONE_JOINER_PIPES ||
>>>>>>> + force_join_pipes >= INTEL_INVALID_JOINER_PIPES) {
>>>>>>> + drm_dbg(&i915->drm, "Ignoring Invalid num of pipes %d for force joining\n",
>>>>>>> + force_join_pipes);
>>>>>>> + connector->force_joined_pipes = INTEL_NONE_JOINER_PIPES;
>>>>>>> + } else {
>>>>>>> + connector->force_joined_pipes = force_join_pipes;
>>>>>>> + }
>>>>>> I think just something like
>>>>>> switch (num_pipes) {
>>>>>> case 0: /* or should 1 be the default? */
>>>>> I suppose both 0 and 1 should be accepted. 0==not forced, 1==forced to
>>>>> exactly one pipe (ie. no joiner despite what the automagic logic
>>>>> is saying).
>>>> I understand 0 as not forced. I didnt get the meaning of forcing to one
>>>> pipe.
>>>>
>>>> Does this mean, disable joiner? (Perhaps do not use joiner even for the
>>>> cases where driver thinks joiner is required)
>>>>
>>>> How should we handle the case in driver, where it is 1?
>>> Whatever code that determines how many pipes will should:
>>> 1) if the override is non-zero just use it
>>> 2) otherwise determine the number by using whatever
>>> logic is appropriate
>>
>> Alright, If I get correctly the driver logic will be something like:
>>
>> int intel_dp_compute_joiner_pipes(struct intel_dp *intel_dp,
>> struct intel_connector *connector,
>> int hdisplay, int clock)
>> {
>> int num_joined_pipes = 0;
> This variable looks redundant. You can just directly return
> the correct number from the switch statement.
Yeah I was inititally going with that, but changed later. Will remove this.
>
>> switch (connector->force_joined_pipes) {
>> case 1:
>> num_joined_pipes = connector->force_joined_pipes;
> This would now return 1, which is probably a value we never
> want to return from here. Either that or we want to never
> return 0 (which this code would do in some of the other
> cases). Not sure which way is better tbh.
Currently I have coded to not allow 0, so we would return 1, 2, or 4
from here.
But I am open to what ever makes semantics intuitive, and handling easier.
>
>> break;
>> case 2:
>> if (intel_dp_has_joiner(intel_dp))
>> num_joined_pipes = connector->force_joined_pipes;
> Hmm. We might want to make the debugfs knob already reject the
> !has_joiner case so that the user won't even be allowed to
> pick a completely unsupported value.
Alright, will have this checked in the function where we parse.
Thanks,
Ankit
>
>> break;
>> default:
>> MISSING_CASE(connector->force_joined_pipes);
>> fallthrough;
>> case 0:
>> if (intel_dp_has_joiner(intel_dp) &&
>> intel_dp_needs_bigjoiner(intel_dp, connector,
>> hdisplay, clock))
>> num_joined_pipes = 2;
>> }
>>
>> return num_joined_pipes;
>> }
>>
>> With a value of 1 we are kind of forcing to not use joiner.
>>
>> Currently for testing sent this to trybot:
>> https://patchwork.freedesktop.org/patch/613627/?series=138444&rev=1
>>
>> Regards,
>>
>> Ankit
>>
>>>> Regards,
>>>>
>>>> Ankit
>>>>
>>>>>> case 2:
>>>>>> case 4:
>>>>>> break;
>>>>>> default:
>>>>>> bad;
>>>>>> }
>>>>>>
>>>>>> should do for validation.
>>>>>>
More information about the Intel-gfx
mailing list