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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

> 
>          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.

>                  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.

>                  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.
> >>>>

-- 
Ville Syrjälä
Intel