Re: [Intel-gfx] [PATCH] drm/i915: Implement workaround for DP2 UHBR bandwidth check

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Tue, Jan 10, 2023 at 02:12:17PM -0500, Rodrigo Vivi wrote:
> On Tue, Jan 10, 2023 at 02:33:38PM +0200, Stanislav Lisovskiy wrote:
> > According to spec, we should check if output_bpp * pixel_rate is less
> > than DDI clock * 72, if UHBR is used.
> > 
> > HSDES: 1406899791
> > BSPEC: 49259
> > 
> > v2: - Removed wrong comment(Rodrigo Vivi)
> >     - Added HSDES to the commit msg(Rodrigo Vivi)
> >     - Moved UHBR check to the MST specific code
> 
> I'm afraid you forgot to remove the "workaround" from the patch subject.

Ah, right, my bad!

> 
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 8b0e4defa3f1..1f1f7f5f6501 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -339,10 +339,19 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  		ret = intel_dp_dsc_compute_config(intel_dp, pipe_config,
> >  						  conn_state, &limits,
> >  						  pipe_config->dp_m_n.tu, false);
> > -	}
> > +		if (ret < 0)
> > +			return ret;
> >  
> > -	if (ret)
> > -		return ret;
> > +		if (intel_dp_is_uhbr(pipe_config)) {
> > +			int output_bpp = pipe_config->dsc.compressed_bpp;
> > +
> > +			if (output_bpp * adjusted_mode->crtc_clock >=
> > +			    pipe_config->port_clock * 72) {
> > +				drm_dbg_kms(&dev_priv->drm, "DP2 UHBR check failed\n");
> 
> I'm wondering if I misunderstood your recent reply....  but I believe you told this
> has nothing to do with DP2.0 so why we have DP2 in the msg still?
> 
> I believe that or:
> 1. We are sure this case is only happening on DP2.0 because it is impossible
> 2. or because we are adding a DP2.0 check
> 3. or we don't mention DP2.0

I think we should mention UHBR only, because it is basically more like bandwidth
limitation. It might be that it can happen only on DP2.0, but still I think
it is more correct to link it to UHBR.
I mean that limitation is most likely still valid even with DP1.4, but it simply
becomes relevant once we start using ultra high bit rate, so that in theory we
can exceed that bw limitation.

> 
> With the subject and the comment fixed:
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Yes, thank you!

> 
> > +				return -EINVAL;
> > +			}
> > +		}
> > +	}
> >  
> >  	ret = intel_dp_mst_update_slots(encoder, pipe_config, conn_state);
> >  	if (ret)
> > -- 
> > 2.37.3
> >