[Intel-gfx] [PATCH 6/9] drm/i915/dsc/mtl: Add support for fractional bpp

[Kandpal, Suraj]

> Subject: RE: [Intel-gfx] [PATCH 6/9] drm/i915/dsc/mtl: Add support for fractional
> bpp
> 
> > Subject: [Intel-gfx] [PATCH 6/9] drm/i915/dsc/mtl: Add support for
> > fractional bpp
> >
> > From: Vandita Kulkarni <vandita.kulkarni at intel.com>
> >
> > Consider the fractional bpp while reading the qp values.
> >
> > v2: Use helpers for fractional, integral bits of bits_per_pixel.
> >
> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni at intel.com>
> > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_qp_tables.c    |  3 ---
> >  drivers/gpu/drm/i915/display/intel_vdsc.c     | 23 +++++++++++++++----
> >  2 files changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_qp_tables.c
> > b/drivers/gpu/drm/i915/display/intel_qp_tables.c
> > index 543cdc46aa1d..600c815e37e4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_qp_tables.c
> > +++ b/drivers/gpu/drm/i915/display/intel_qp_tables.c
> > @@ -34,9 +34,6 @@
> >   * These qp tables are as per the C model
> >   * and it has the rows pointing to bpps which increment
> >   * in steps of 0.5
> > - * We do not support fractional bpps as of today,
> > - * hence we would skip the fractional bpps during
> > - * our references for qp calclulations.
> >   */
> >  static const u8
> >
> rc_range_minqp444_8bpc[DSC_NUM_BUF_RANGES][RC_RANGE_QP444_8BPC
> > _MAX_NUM_BPP] = {
> >  	{ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > 0, diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > index 2dc6ea82c024..4bd570fb0ab2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > @@ -77,8 +77,9 @@ intel_vdsc_set_min_max_qp(struct drm_dsc_config
> > *vdsc_cfg, int buf,  static void  calculate_rc_params(struct
> > drm_dsc_config
> > *vdsc_cfg)  {
> > +	int fractional_bits = dsc_fractional_compressed_bpp(vdsc_cfg-
> > >bits_per_pixel);
> > +	int bpp = dsc_integral_compressed_bpp(vdsc_cfg->bits_per_pixel);
> >  	int bpc = vdsc_cfg->bits_per_component;
> > -	int bpp = vdsc_cfg->bits_per_pixel >> 4;
> >  	int qp_bpc_modifier = (bpc - 8) * 2;
> >  	int uncompressed_bpg_rate;
> >  	int first_line_bpg_offset;
> > @@ -148,7 +149,13 @@ calculate_rc_params(struct drm_dsc_config
> *vdsc_cfg)
> >  		static const s8 ofs_und8[] = {
> >  			10, 8, 6, 4, 2, 0, -2, -4, -6, -8, -10, -10, -12, -12, -12
> >  		};
> > -
> > +	/*
> > +	 * For 420 format since bits_per_pixel (bpp) is set to target bpp * 2,
> > +	 * QP table values for target bpp 4.0 to 4.4375 (rounded to 4.0) are
> > +	 * actually for bpp 8 to 8.875 (rounded to 4.0 * 2 i.e 8).
> > +	 * Similarly values for target bpp 4.5 to 4.8375 (rounded to 4.5)
> > +	 * are for bpp 9 to 9.875 (rounded to 4.5 * 2 i.e 9), and so on.
> > +	 */
> >  		bpp_i  = bpp - 8;
> >  		for (buf_i = 0; buf_i < DSC_NUM_BUF_RANGES; buf_i++) {
> >  			u8 range_bpg_offset;
> > @@ -191,7 +198,14 @@ calculate_rc_params(struct drm_dsc_config
> *vdsc_cfg)
> >  			10, 8, 6, 4, 2, 0, -2, -4, -6, -8, -10, -10, -12, -12, -12
> >  		};
> >
> > -		bpp_i  = (2 * (bpp - 6));
> > +		/*
> > +		 * QP table rows have values in increment of 0.5.
> > +		 * So 6.0 bpp to 6.4375 will have index 0, 6.5 to 6.9375 will
> > have index 1,
> > +		 * and so on.
> > +		 * 0.5 represented as 0x8 in U6.4 format.
> > +		 */
> > +		bpp_i  = ((bpp - 6) + (fractional_bits < 0x8 ? 0 : 1));
> 
> Can we have a the 0x8 as a #define rather than a direct comparison to 0x8
> 

Also maybe we can have a function/macro which takes in the integral and fractional
part and spits out the closest integer
something like DIV_ROUND_CLOSEST but that won't directly work with this case
but I do think we can repurpose it for our use case.

> Also isn't what was previously present doing the same thing Sure fractional bits
> weren't taken into consideration but they would still fall in the same Index.
> Anyways the bpp taken is the integer part so I thing no change is required to
> the formula.

Also scratch this comment I had another looks and seems logical to me what we are doing.

Regards,
Suraj Kandpal

> 
> > +
> >  		for (buf_i = 0; buf_i < DSC_NUM_BUF_RANGES; buf_i++) {
> >  			u8 range_bpg_offset;
> >
> > @@ -279,8 +293,7 @@ int intel_dsc_compute_params(struct
> > intel_crtc_state
> > *pipe_config)
> >  	/* Gen 11 does not support VBR */
> >  	vdsc_cfg->vbr_enable = false;
> >
> > -	/* Gen 11 only supports integral values of bpp */
> > -	vdsc_cfg->bits_per_pixel = compressed_bpp << 4;
> > +	vdsc_cfg->bits_per_pixel = pipe_config->dsc.compressed_bpp;
> >
> >  	/*
> >  	 * According to DSC 1.2 specs in Section 4.1 if native_420 is set
> > --
> > 2.40.1