Re: [Intel-gfx] [PATCH 2/2] drm/i915: Don't rely that 2 VDSC engines are always enough for pixel rate

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

On Mon, Jul 10, 2023 at 11:37:56AM +0530, Nautiyal, Ankit K wrote:
> 
> On 7/4/2023 6:47 PM, Stanislav Lisovskiy wrote:
> > We are currently having FIFO underruns happening for kms_dsc test case,
> > problem is that, we check if curreny cdclk is >= pixel rate only if
> > there is a single VDSC engine enabled(i.e dsc_split=false) however if
> > we happen to have 2 VDSC engines enabled, we just kinda rely that this
> > would be automatically enough.
> > However pixel rate can be even >= than VDSC clock(cdclk) * 2, so in that
> > case even with 2 VDSC engines enabled, we still need to tweak it up.
> > So lets compare pixel rate with cdclk * slice count(VDSC engine count) and
> 
> Since we are not using slice count, we can just mention VDSC engine count.

Agree, thanks for spotting.

> 
> 
> > check if it still requires bumping up.
> > Previously we had to bump up CDCLK many times for similar reasons.
> > 
> > v2: - Use new intel_dsc_get_num_vdsc_instances to determine number of VDSC
> >        engines, instead of slice count(Ankit Nautiyal)
> > v3: - s/u8/int/ (Jani Nikula)
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_cdclk.c | 13 +++++++++++--
> >   1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 4207863b7b2a..bfa1c5d589ba 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -37,6 +37,7 @@
> >   #include "intel_pci_config.h"
> >   #include "intel_pcode.h"
> >   #include "intel_psr.h"
> > +#include "intel_vdsc.h"
> >   #include "vlv_sideband.h"
> >   /**
> > @@ -2607,9 +2608,17 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> >   	 * When we decide to use only one VDSC engine, since
> >   	 * each VDSC operates with 1 ppc throughput, pixel clock
> >   	 * cannot be higher than the VDSC clock (cdclk)
> > +	 * If there 2 VDSC engines, then pixel clock can't be higher than
> > +	 * VDSC clock(cdclk) * 2. However even that can still be not enough.
> > +	 * Slice count reflects amount of VDSC engines,
> As mentioned above, we can remove slice_count, as we are using VDSC engine
> count.
> > +	 * so lets use that to determine, if need still need to tweak CDCLK higher.
> 
> 
> >   	 */
> > -	if (crtc_state->dsc.compression_enable && !crtc_state->dsc.dsc_split)
> > -		min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate);
> > +	if (crtc_state->dsc.compression_enable) {
> > +		int num_vdsc_instances = intel_dsc_get_num_vdsc_instances(crtc_state);
> > +
> > +		min_cdclk = max_t(int, min_cdclk,
> > +			          crtc_state->pixel_rate / num_vdsc_instances);
> 
> I was wondering if we should use DIV_ROUND_UP(crtc_state->pixel_rate /
> num_vdsc_instances), since min_cdclk should be more than this value.
> 
> Though practically Pixel rate in Khz / num of vdsc instances, wont need to
> roundup, so perhaps we might not require this. I leave it up to you.

Yep was thinking about that too. Practically DIV_ROUND_UP(pixel_rate, num_vdsc_instances)
means pixel_rate + (2 - 1) / 2 here, so we might get + 1 Khz here. Considering that
values in cdclk_table differ by 1000's of kHz, when we are looking for value which is >=
this is quite unlikely to be a problem.
For this to be a problem we need to have CDCLK to be lets say 345601, then once we divide by
2, we get 172800, which is 345600, if multiplied by 2. 

However we still might want to add that, just to be on safe side.

Stan

> 
> 
> With the above changes in documentation, this is:
> 
> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> 
> 
> > +	}
> >   	/*
> >   	 * HACK. Currently for TGL/DG2 platforms we calculate