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

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

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

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

-- 
Ville Syrjälä
Intel