Re: [Intel-gfx] [PATCH 2/2] drm/i915: Implement UHBR bandwidth check

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

On Wed, Feb 01, 2023 at 01:00:59PM +0200, Lisovskiy, Stanislav wrote:
> On Tue, Jan 31, 2023 at 05:41:45PM +0200, Ville Syrjälä wrote:
> > On Tue, Jan 31, 2023 at 05:20:44PM +0200, Lisovskiy, Stanislav wrote:
> > > On Tue, Jan 31, 2023 at 05:00:30PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Jan 16, 2023 at 01:19:37PM +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
> > > > > 
> > > > > v3: - Changed commit subject(Rodrigo Vivi)
> > > > >     - Fixed the error message if check fails(Rodrigo Vivi)
> > > > > 
> > > > > v4: - Move UHBR check to new helper function
> > > > >     - Now both for non-DSC/DSC we use that new check as
> > > > >       one of the constraints, when figuring out output bpp
> > > > >       to be used(Ville Syrjälä)
> > > > > 
> > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 13 ++++++++++++-
> > > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > index e3e7c305fece..b95051fed23d 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > @@ -47,8 +47,19 @@
> > > > >  
> > > > >  static int intel_dp_mst_check_constraints(struct drm_i915_private *i915, int bpp,
> > > > >  					  const struct drm_display_mode *adjusted_mode,
> > > > > -					  struct intel_crtc_state *crtc_state)
> > > > > +					  struct intel_crtc_state *pipe_config)
> > > > >  {
> > > > > +	if (intel_dp_is_uhbr(pipe_config)) {
> > > > > +		int output_bpp = bpp;
> > > > > +
> > > > > +		if (output_bpp * adjusted_mode->crtc_clock >=
> > > > > +		    pipe_config->port_clock * 72) {
> > > > 
> > > > This seems to be some DSC specific constraint, but this code appears to
> > > > apply it also to uncompresed output.
> > > 
> > > Was thinking about that. Looking at the initial HSD, also to the DSC page
> > > in the BSpec, I have a strong feeling that this applies to any output bpp,
> > > regardless if its compressed or not.
> > > So decided to make this check more generic. I think this is a just general
> > > Link BW limitation which just happened to be mentioned on DSC page.
> > > I will clarify that.
> > > 
> > > > 
> > > > Also DDICLK != port_clock, so this looks to be off by quite a lot.
> > > 
> > > Any hints, what should I use instead?..
> > 
> > DDICLK looks to be the symbol clock more or less (for 32bit symbols),
> > so presumably you want a /32, with maybe an extra factor of 10^n in
> > there, or not.
> > 
> > That magic 72 is also strange. Maybe a 2*36, but dunno what that would
> > really be either. The spec could really use some better explanations.
> 
> There is another check mentioned right above in the same place, which 
> I believe is related(btw, we probably need that one as well, if not added):
> 
> Output bpp < Number of lanes * DDICLK frequency * Bits per lane / Pixel clock
> 
> and bits per lane have to be:
> 
> DisplayPort 8b/10b bits per lane = 8
> DisplayPort 2 128b/132b bits per lane = 32
> 
> The check we are talking here is:
> 
> Output bpp * Pixel clock < DDICLK frequency * 72 bits
> 
> Which means
> 
> Output bpp < (DDICLK frequency * 72 bits) / Pixel clock
> 
> So I guess that means that to get DDICLK I need to divide port_clock by bits_per_lane.
> 
> And 72 bits is a probably max symbol width which can be used due to some HW
> restrictions.

It would help if we knew what "DPT" was. It might be some max bits per
clock limit of the DSC encoder, or something. Looks like it's getting
doubled again in future projects.

-- 
Ville Syrjälä
Intel