Re: [Intel-gfx] [PATCH] drm/i915: Fix the i915_dsc_fec_support debugfs file for DP MST connectors

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

On Tue, Jun 09, 2020 at 08:40:35PM +0300, Imre Deak wrote:
> On Tue, Jun 09, 2020 at 06:05:25PM +0300, Ville Syrjälä wrote:
> > On Mon, Jun 08, 2020 at 09:10:23PM +0300, Imre Deak wrote:
> > > DSC is not supported on DP MST streams so just return -EINVAL when
> > > reading/writing the i915_dsc_fec_support debugfs file for such
> > > connectors.
> > > 
> > > This also fixes an OOPS, caused by the encoder->digport cast, which is
> > > not valid for MST encoders.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_debugfs.c  | 36 +++++++++++++++----
> > >  1 file changed, 29 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > index 2b640d8ab9d2..ebca8e488d03 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > @@ -2094,6 +2094,8 @@ static int i915_dsc_fec_support_show(struct seq_file *m, void *data)
> > >  	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> > >  
> > >  	do {
> > > +		struct intel_encoder *encoder;
> > > +
> > >  		try_again = false;
> > >  		ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
> > >  				       &ctx);
> > > @@ -2120,8 +2122,17 @@ static int i915_dsc_fec_support_show(struct seq_file *m, void *data)
> > >  		} else if (ret) {
> > >  			break;
> > >  		}
> > > -		intel_dp = intel_attached_dp(to_intel_connector(connector));
> > > +
> > > +		encoder = intel_attached_encoder(to_intel_connector(connector));
> > > +		/* TODO: Add DSC support for MST streams */
> > > +		if (encoder->type == INTEL_OUTPUT_DP_MST) {
> > > +			ret = -EINVAL;
> > > +			break;
> > > +		}
> > > +
> > > +		intel_dp = &enc_to_dig_port(encoder)->dp;
> > >  		crtc_state = to_intel_crtc_state(crtc->state);
> > > +
> > >  		seq_printf(m, "DSC_Enabled: %s\n",
> > >  			   yesno(crtc_state->dsc.compression_enable));
> > >  		seq_printf(m, "DSC_Sink_Support: %s\n",
> > > @@ -2147,9 +2158,8 @@ static ssize_t i915_dsc_fec_support_write(struct file *file,
> > >  	int ret;
> > >  	struct drm_connector *connector =
> > >  		((struct seq_file *)file->private_data)->private;
> > > -	struct intel_encoder *encoder = intel_attached_encoder(to_intel_connector(connector));
> > > -	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > > -	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > +	struct drm_i915_private *i915 = to_i915(connector->dev);
> > > +	struct intel_encoder *encoder;
> > >  
> > >  	if (len == 0)
> > >  		return 0;
> > > @@ -2163,10 +2173,22 @@ static ssize_t i915_dsc_fec_support_write(struct file *file,
> > >  
> > >  	drm_dbg(&i915->drm, "Got %s for DSC Enable\n",
> > >  		(dsc_enable) ? "true" : "false");
> > > -	intel_dp->force_dsc_en = dsc_enable;
> > >  
> > > -	*offp += len;
> > > -	return len;
> > > +	drm_modeset_lock(&i915->drm.mode_config.connection_mutex, NULL);
> > > +
> > > +	encoder = intel_attached_encoder(to_intel_connector(connector));
> > > +	/* TODO: Add DSC support for MST streams */
> > > +	if (encoder->type == INTEL_OUTPUT_DP_MST) {
> > 
> > The attached encoder can be NULL for MST.
> 
> Yes, I also sent v2 with that fixed.
> 
> > Can't we just not add this debugfs file for MST connectors?
> 
> Won't we have per MST connector DSC at one point? In that case we'd need
> something like this anyway.

The problem is that it doesn't work until the encoder has been attached.
So the behaviour of the interface would be rather inconsistent when
applied to MST connectors. No idea what current tests are doing with
this stuff so can't say whether it would be a problem in practice.

> If this is never needed then yes, better not
> to add it. I can't use connector_type which is the same for MST and SST
> connectors so is it ok to differentiate based on a
> intel_attached_encoder() == NULL check?

IIRC some other places use connector.mst_port to tell the two
apart.

> 
> > 
> > > +		ret = -EINVAL;
> > > +	} else {
> > > +		enc_to_intel_dp(encoder)->force_dsc_en = dsc_enable;
> > > +		*offp += len;
> > > +		ret = len;
> > > +	}
> > > +
> > > +	drm_modeset_unlock(&i915->drm.mode_config.connection_mutex);
> > > +
> > > +	return ret;
> > >  }
> > >  
> > >  static int i915_dsc_fec_support_open(struct inode *inode,
> > > -- 
> > > 2.23.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx