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/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?


Regards,

Ankit

>
>> case 2:
>> case 4:
>> 	break;
>> default:
>> 	bad;
>> }
>>
>> should do for validation.
>>