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

[Imre Deak <imre.deak@intel.com>]

On Tue, Jun 09, 2020 at 11:56:56AM -0700, Manasi Navare wrote:
> On Tue, Jun 09, 2020 at 09:08:52PM +0300, Imre Deak wrote:
> > On Tue, Jun 09, 2020 at 08:59:23PM +0300, Ville Syrjälä wrote:
> > > 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) {
> 
> even after we force dsc en from IGT through this debugfs node, the
> actual enabling checks if dsc supported, so may be since we currently
> dont support on MST, can we add some condition in this function:
> intel_dsc_source_support() to not support on MST encoders?

I think the final API is not yeat clear for MST, so since it's not used
either I agree that it's better to remove it.

> Also, currently since we dont even call intel_dp_dsc_compute_config in
> MST codepath, how does it even make a difference if we set
> force_dsc_en through this debugfs or not for MST?

The difference is that intel_attached_dp() can't be used on MST
connectors.

> 
> Manasi
> 
> > > > > 
> > > > > 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.
> > 
> > It's used to force DSC by writing to it before commit, so yes then it
> > wouldn't work anyway.
> > 
> > > > 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.
> > 
> > Ok, will use that.
> > 
> > > 
> > > > 
> > > > > 
> > > > > > +		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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx