Re: [PATCH 01/14] drm/i915/display: Modify debugfs for joiner to force n pipes

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

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@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;

         switch (connector->force_joined_pipes) {
         case 1:
                 num_joined_pipes = connector->force_joined_pipes;
                 break;
         case 2:
                 if (intel_dp_has_joiner(intel_dp))
                         num_joined_pipes = connector->force_joined_pipes;
                 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.
>>>>