Re: [Intel-gfx] [PATCH] drm/i915: Fail if DSC compression requirement is less than platform supports

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

On Mon, Jun 26, 2023 at 03:18:52PM +0300, Luca Coelho wrote:
> On Mon, 2023-06-26 at 11:28 +0300, Stanislav Lisovskiy wrote:
> > Currently we just clamp that value to the highest supported one, however that
> > means, we are not able to fit this into our available bandwidth range, so we
> > might see glitches or FIFO underruns.
> > While choosing less compressed bpp than min bpp required to handle the mode is
> > harmless and might even save some bandwidth, choosing higher compressed bpp than
> > min bpp required to handle the required mode config, can cause issues.
> > So in that case lets just conclude that even with DSC, we are not able to comply
> > with bandwidth requirements and fail.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 9f40da20e88d..8696a1f02805 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -713,8 +713,17 @@ u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 p
> >  
> >  		/*
> >  		 * According to BSpec, 27 is the max DSC output bpp,
> > -		 * 8 is the min DSC output bpp
> > +		 * 8 is the min DSC output bpp.
> > +		 * While we can still clamp higher bpp values to 27, saving bandwidth,
> > +		 * if it is required to oompress up to bpp < 8, means we can't do
> 
> Small typo, "compress".

Thanks for spotting

> 
> 
> > +		 * that and probably means we can't fit the required mode, even with
> > +		 * DSC enabled.
> >  		 */
> > +		if (bits_per_pixel < 8) {
> > +			drm_dbg_kms(&i915->drm, "Unsupported BPP %u, min 8\n",
> > +				    bits_per_pixel);
> > +			return 0;
> > +		}
> >  		bits_per_pixel = clamp_t(u32, bits_per_pixel, 8, 27);
> 
> I guess you don't need to clamp anymore but could use min_t(u32,
> bits_per_pixel, 27) now, right? Actually, you don't even need to type
> it, so min(bits_per_pixel, 27) should suffice.

Yep, was thinking about that, was willing to leave clamp just as an additional
check "just in case", but probably you are right. 

Stan

> 
> 
> >  	} else {
> >  		/* Find the nearest match in the array of known BPPs from VESA */
> 
> --
> Cheers,
> Luca.