Re: [PATCH 01/10] drm/i915/dsc: change DSC param tables to follow the DSC model

Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> · Tue, 28 Feb 2023 18:10:16 +0200

On 28/02/2023 17:56, Jani Nikula wrote:
On Tue, 28 Feb 2023, Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote:
After cross-checking DSC models (20150914, 20161212, 20210623) change
values in rc_parameters tables to follow config files present inside
the DSC model. Handle two places, where i915 tables diverged from the
model, by patching the rc values in the code.

Note: I left one case uncorrected, 8bpp/10bpc/range_max_qp[0], because
the table in the VESA DSC 1.1 sets it to 4.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
  drivers/gpu/drm/i915/display/intel_vdsc.c | 18 ++++++++++++++++--
  1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
index 207b2a648d32..d080741fd0b3 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
@@ -86,7 +86,7 @@ static const struct rc_parameters rc_parameters[][MAX_COLUMN_INDEX] = {
  		}
  	},
  	/* 6BPP/14BPC */
-	{ 768, 15, 6144, 15, 25, 23, 27, {
+	{ 768, 15, 6144, 15, 25, 23, 23, {
  		{ 0, 16, 0 }, { 7, 18, -2 }, { 15, 20, -2 }, { 16, 20, -4 },
  		{ 17, 21, -6 }, { 17, 21, -6 }, { 18, 21, -6 }, { 18, 22, -8 },
  		{ 19, 23, -8 }, { 20, 24, -10 }, { 21, 24, -10 },
@@ -115,6 +115,10 @@ static const struct rc_parameters rc_parameters[][MAX_COLUMN_INDEX] = {
  	},
  	/* 8BPP/10BPC */
  	{ 512, 12, 6144, 7, 16, 15, 15, {
+		/*
+		 * DSC model/pre-SCR-cfg has 8 for range_max_qp[0], however
+		 * VESA DSC 1.1 Table E-5 sets it to 4.
+		 */
  		{ 0, 4, 2 }, { 4, 8, 0 }, { 5, 9, 0 }, { 5, 10, -2 },
  		{ 7, 11, -4 }, { 7, 11, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
  		{ 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
@@ -132,7 +136,7 @@ static const struct rc_parameters rc_parameters[][MAX_COLUMN_INDEX] = {
  	},
  	/* 8BPP/14BPC */
  	{ 512, 12, 6144, 15, 24, 23, 23, {
-		{ 0, 12, 0 }, { 5, 13, 0 }, { 11, 15, 0 }, { 12, 17, -2 },
+		{ 0, 12, 2 }, { 5, 13, 0 }, { 11, 15, 0 }, { 12, 17, -2 },
  		{ 15, 19, -4 }, { 15, 19, -6 }, { 15, 19, -8 }, { 15, 20, -8 },
  		{ 15, 21, -8 }, { 15, 22, -10 }, { 17, 22, -10 },
  		{ 17, 23, -12 }, { 17, 23, -12 }, { 21, 24, -12 },
@@ -529,6 +533,16 @@ int intel_dsc_compute_params(struct intel_crtc_state *pipe_config)
  			DSC_RANGE_BPG_OFFSET_MASK;
  	}
  
+	if (DISPLAY_VER(dev_priv) < 13) {
+		if (compressed_bpp == 6 &&
+		    vdsc_cfg->bits_per_component == 8)
+			vdsc_cfg->rc_quant_incr_limit1 = 23;
+
+		if (compressed_bpp == 8 &&
+		    vdsc_cfg->bits_per_component == 14)
+			vdsc_cfg->rc_range_params[0].range_bpg_offset = 0;
+	}
+

I wonder if we shouldn't just use the updated values...

I also wondered about this, so I wanted to get a double check from 
somebody having better knowledge of this part, if it is a typo in the 
original patch or a typo in the cfg files.

E.g. the pre_scr_cfg_files_for_reference/rc_10bpc_8bpp.cfg has 8 as 
RX_MAXQP[0], which (for me) looks like a typo in the CFG file itself, 
rather than being a typo in the driver.

On the other hand, these two issues belong to the 'current' CFG files, 
so they, most probably, received more attention from anybody working 
with the standard and with the model.

I can change this patch to become a fix for the tables (dropping the if 
clause), if you can confirm that these values are typos in the driver.


Maybe add a FIXME comment above the block to consider removing it?

Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>


  	/*
  	 * BitsPerComponent value determines mux_word_size:
  	 * When BitsPerComponent is less than or 10bpc, muxWordSize will be equal to


--
With best wishes
Dmitry







