mbox series

[v2,00/10] drm/i915: move DSC RC tables to drm_dsc_helper.c

Message ID 20230307134901.322560-1-dmitry.baryshkov@linaro.org
Headers show
Series drm/i915: move DSC RC tables to drm_dsc_helper.c | expand

Message

Dmitry Baryshkov March 7, 2023, 1:48 p.m. UTC 
Other platforms (msm) will benefit from sharing the DSC config setup
functions. This series moves parts of static DSC config data from the
i915 driver to the common helpers to be used by other drivers.

Note: the RC parameters were cross-checked against config files found in
DSC model 2021062, 20161212 (and 20150914). The first patch modifies
tables according to those config files, while preserving parameter
values using the code. I have not changed one of the values in the
pre-SCR config file as it clearly looks like a typo in the config file,
considering the table E in DSC 1.1 and in the DSC 1.1 SCR.

Chances since v1:
- Made drm_dsc_rc_buf_thresh static rather than exporting it
- Switched drm_dsc_rc_buf_thresh loop to use ARRAY_SIZE. Added
  BUILD_BUG_ON's to be sure that array sizes are correct
- Fixed rc_parameters_data indentation to be logical and tidy
- Fixed drm_dsc_setup_rc_params() kerneldoc
- Added a clause to drm_dsc_setup_rc_params() to verify bpp and bpc
  being set.
- Fixed range_bpg_offset programming in calculate_rc_params()
- Fixed bpp vs bpc bug in intel_dsc_compute_params()
- Added FIXME comment next to the customizations in
  intel_dsc_compute_params().

Dmitry Baryshkov (10):
  drm/i915/dsc: change DSC param tables to follow the DSC model
  drm/i915/dsc: move rc_buf_thresh values to common helper
  drm/i915/dsc: move DSC tables to DRM DSC helper
  drm/i915/dsc: stop using interim structure for calculated params
  drm/display/dsc: use flat array for rc_parameters lookup
  drm/display/dsc: split DSC 1.2 and DSC 1.1 (pre-SCR) parameters
  drm/display/dsc: include the rest of pre-SCR parameters
  drm/display/dsc: add YCbCr 4:2:2 and 4:2:0 RC parameters
  drm/display/dsc: add helper to set semi-const parameters
  drm/msm/dsi: use new helpers for DSC setup

 drivers/gpu/drm/display/drm_dsc_helper.c  | 1007 +++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_vdsc.c |  443 +--------
 drivers/gpu/drm/msm/dsi/dsi_host.c        |   61 +-
 include/drm/display/drm_dsc_helper.h      |   10 +
 4 files changed, 1072 insertions(+), 449 deletions(-)

Comments

Jani Nikula March 8, 2023, 10:14 a.m. UTC | #1 
On Tue, 07 Mar 2023, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> The array of rc_parameters contains a mixture of parameters from DSC 1.1
> and DSC 1.2 standards. Split these tow configuration arrays in
> preparation to adding more configuration data.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/display/drm_dsc_helper.c  | 127 ++++++++++++++++++----
>  drivers/gpu/drm/i915/display/intel_vdsc.c |  10 +-
>  include/drm/display/drm_dsc_helper.h      |   7 +-
>  3 files changed, 119 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> index acb93d4116e0..35b39f3109c4 100644
> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> @@ -324,11 +324,81 @@ struct rc_parameters_data {
>  
>  #define DSC_BPP(bpp)	((bpp) << 4)
>  
> +static const struct rc_parameters_data rc_parameters_pre_scr[] = {
> +	{
> +		.bpp = DSC_BPP(8), .bpc = 8,
> +		{ 512, 12, 6144, 3, 12, 11, 11, {
> +			{ 0, 4, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> +			{ 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> +			{ 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 }, { 5, 12, -12 },
> +			{ 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> +			}
> +		}
> +	},
> +	{
> +		.bpp = DSC_BPP(8), .bpc = 10,
> +		{ 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 },
> +			{ 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> +			}
> +		}
> +	},
> +	{
> +		.bpp = DSC_BPP(8), .bpc = 12,
> +		{ 512, 12, 6144, 11, 20, 19, 19, {
> +			{ 0, 12, 2 }, { 4, 12, 0 }, { 9, 13, 0 }, { 9, 14, -2 },
> +			{ 11, 15, -4 }, { 11, 15, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> +			{ 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> +			{ 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> +			{ 21, 23, -12 }
> +			}
> +		}
> +	},
> +	{
> +		.bpp = DSC_BPP(12), .bpc = 8,
> +		{ 341, 15, 2048, 3, 12, 11, 11, {
> +			{ 0, 2, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> +			{ 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> +			{ 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 },
> +			{ 5, 12, -12 }, { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> +			}
> +		}
> +	},
> +	{
> +		.bpp = DSC_BPP(12), .bpc = 10,
> +		{ 341, 15, 2048, 7, 16, 15, 15, {
> +			{ 0, 2, 2 }, { 2, 5, 0 }, { 3, 7, 0 }, { 4, 8, -2 },
> +			{ 6, 9, -4 }, { 7, 10, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
> +			{ 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
> +			{ 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> +			}
> +		}
> +	},
> +	{
> +		.bpp = DSC_BPP(12), .bpc = 12,
> +		{ 341, 15, 2048, 11, 20, 19, 19, {
> +			{ 0, 6, 2 }, { 4, 9, 0 }, { 7, 11, 0 }, { 8, 12, -2 },
> +			{ 10, 13, -4 }, { 11, 14, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> +			{ 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> +			{ 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> +			{ 21, 23, -12 }
> +			}
> +		}
> +	},
> +	{ /* sentinel */ }
> +};
> +
>  /*
>   * Selected Rate Control Related Parameter Recommended Values
>   * from DSC_v1.11 spec & C Model release: DSC_model_20161212
>   */

The comment is no longer accurate, is it?

There are various ways to determine the parameters to use. There's even
an application note "VESA DSC v1.2a Guidance on Deriving DSC Rate
Control Parameters" that lists the options. They are all valid and
should "provide visually lossless quality".

Would it be simplest to always use the C model parameters in the tables
here, referencing the zip file name with date above each table? That
could at least be consistent, and drivers could override parameters
using other methods if they desire. And it would be easiest to review.

I'm having a hard time finding time to review all this in a timely
fashion. Would be good to try to get other folks to review the rest,
it's really not very i915 specific anyway. In the mean time I think
patches 1-5 are okay to merge via drm-misc.

BR,
Jani.

> -static const struct rc_parameters_data rc_parameters[] = {
> +static const struct rc_parameters_data rc_parameters_1_2_444[] = {
>  	{
>  		.bpp = DSC_BPP(6), .bpc = 8,
>  		{ 768, 15, 6144, 3, 13, 11, 11, {
> @@ -388,22 +458,18 @@ static const struct rc_parameters_data rc_parameters[] = {
>  		{ 512, 12, 6144, 3, 12, 11, 11, {
>  			{ 0, 4, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
>  			{ 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> -			{ 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 }, { 5, 12, -12 },
> -			{ 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> +			{ 3, 9, -8 }, { 3, 10, -10 }, { 5, 10, -10 }, { 5, 11, -12 },
> +			{ 5, 11, -12 }, { 9, 12, -12 }, { 12, 13, -12 }
>  			}
>  		}
>  	},
>  	{
>  		.bpp = DSC_BPP(8), .bpc = 10,
>  		{ 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 },
> +			{ 0, 8, 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 },
> -			{ 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> +			{ 7, 13, -8 }, { 7, 14, -10 }, { 9, 14, -10 }, { 9, 15, -12 },
> +			{ 9, 15, -12 }, { 13, 16, -12 }, { 16, 17, -12 }
>  			}
>  		}
>  	},
> @@ -412,9 +478,9 @@ static const struct rc_parameters_data rc_parameters[] = {
>  		{ 512, 12, 6144, 11, 20, 19, 19, {
>  			{ 0, 12, 2 }, { 4, 12, 0 }, { 9, 13, 0 }, { 9, 14, -2 },
>  			{ 11, 15, -4 }, { 11, 15, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> -			{ 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> -			{ 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> -			{ 21, 23, -12 }
> +			{ 11, 17, -8 }, { 11, 18, -10 }, { 13, 18, -10 },
> +			{ 13, 19, -12 }, { 13, 19, -12 }, { 17, 20, -12 },
> +			{ 20, 21, -12 }
>  			}
>  		}
>  	},
> @@ -498,8 +564,8 @@ static const struct rc_parameters_data rc_parameters[] = {
>  		{ 341, 15, 2048, 3, 12, 11, 11, {
>  			{ 0, 2, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
>  			{ 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> -			{ 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 },
> -			{ 5, 12, -12 }, { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> +			{ 3, 8, -8 }, { 3, 9, -10 }, { 5, 9, -10 }, { 5, 9, -12 },
> +			{ 5, 9, -12 }, { 7, 10, -12 }, { 10, 11, -12 }
>  			}
>  		}
>  	},
> @@ -508,8 +574,8 @@ static const struct rc_parameters_data rc_parameters[] = {
>  		{ 341, 15, 2048, 7, 16, 15, 15, {
>  			{ 0, 2, 2 }, { 2, 5, 0 }, { 3, 7, 0 }, { 4, 8, -2 },
>  			{ 6, 9, -4 }, { 7, 10, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
> -			{ 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
> -			{ 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> +			{ 7, 12, -8 }, { 7, 13, -10 }, { 9, 13, -10 }, { 9, 13, -12 },
> +			{ 9, 13, -12 }, { 11, 14, -12 }, { 14, 15, -12 }
>  			}
>  		}
>  	},
> @@ -518,9 +584,9 @@ static const struct rc_parameters_data rc_parameters[] = {
>  		{ 341, 15, 2048, 11, 20, 19, 19, {
>  			{ 0, 6, 2 }, { 4, 9, 0 }, { 7, 11, 0 }, { 8, 12, -2 },
>  			{ 10, 13, -4 }, { 11, 14, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> -			{ 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> -			{ 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> -			{ 21, 23, -12 }
> +			{ 11, 16, -8 }, { 11, 17, -10 }, { 13, 17, -10 },
> +			{ 13, 17, -12 }, { 13, 17, -12 }, { 15, 18, -12 },
> +			{ 18, 19, -12 }
>  			}
>  		}
>  	},
> @@ -602,7 +668,8 @@ static const struct rc_parameters_data rc_parameters[] = {
>  	{ /* sentinel */ }
>  };
>  
> -static const struct rc_parameters *get_rc_params(u16 dsc_bpp,
> +static const struct rc_parameters *get_rc_params(const struct rc_parameters_data *rc_parameters,
> +						 u16 dsc_bpp,
>  						 u8 bits_per_component)
>  {
>  	int i;
> @@ -622,11 +689,13 @@ static const struct rc_parameters *get_rc_params(u16 dsc_bpp,
>   * function.
>   *
>   * @vdsc_cfg: DSC Configuration data partially filled by driver
> + * @kind: operating mode and standard to follow
>   *
>   * Return: 0 or -error code in case of an error
>   */
> -int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg)
> +int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum drm_dsc_params_kind kind)
>  {
> +	const struct rc_parameters_data *data;
>  	const struct rc_parameters *rc_params;
>  	int i;
>  
> @@ -634,7 +703,19 @@ int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg)
>  			 !vdsc_cfg->bits_per_component))
>  		return -EINVAL;
>  
> -	rc_params = get_rc_params(vdsc_cfg->bits_per_pixel,
> +	switch (kind) {
> +	case DRM_DSC_1_2_444:
> +		data = rc_parameters_1_2_444;
> +		break;
> +	case DRM_DSC_1_1_PRE_SCR:
> +		data = rc_parameters_pre_scr;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	rc_params = get_rc_params(data,
> +				  vdsc_cfg->bits_per_pixel,
>  				  vdsc_cfg->bits_per_component);
>  	if (!rc_params)
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index 20a4c2f343fe..a4d1d2ba71bb 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -157,7 +157,15 @@ int intel_dsc_compute_params(struct intel_crtc_state *pipe_config)
>  	if (DISPLAY_VER(dev_priv) >= 13) {
>  		calculate_rc_params(vdsc_cfg);
>  	} else {
> -		ret = drm_dsc_setup_rc_params(vdsc_cfg);
> +		if ((compressed_bpp == 8 ||
> +		     compressed_bpp == 12) &&
> +		    (vdsc_cfg->bits_per_component == 8 ||
> +		     vdsc_cfg->bits_per_component == 10 ||
> +		     vdsc_cfg->bits_per_component == 12))
> +			ret = drm_dsc_setup_rc_params(vdsc_cfg, DRM_DSC_1_1_PRE_SCR);
> +		else
> +			ret = drm_dsc_setup_rc_params(vdsc_cfg, DRM_DSC_1_2_444);
> +
>  		if (ret)
>  			return ret;
>  
> diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
> index 1681791f65a5..c634bb2935d3 100644
> --- a/include/drm/display/drm_dsc_helper.h
> +++ b/include/drm/display/drm_dsc_helper.h
> @@ -10,12 +10,17 @@
>  
>  #include <drm/display/drm_dsc.h>
>  
> +enum drm_dsc_params_kind {
> +	DRM_DSC_1_2_444,
> +	DRM_DSC_1_1_PRE_SCR, /* legacy params from DSC 1.1 */
> +};
> +
>  void drm_dsc_dp_pps_header_init(struct dp_sdp_header *pps_header);
>  int drm_dsc_dp_rc_buffer_size(u8 rc_buffer_block_size, u8 rc_buffer_size);
>  void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_sdp,
>  			      const struct drm_dsc_config *dsc_cfg);
>  void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg);
> -int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg);
> +int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum drm_dsc_params_kind kind);
>  int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>  
>  #endif /* _DRM_DSC_HELPER_H_ */
Dmitry Baryshkov March 8, 2023, 12:59 p.m. UTC | #2 
On Wed, 8 Mar 2023 at 12:14, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Tue, 07 Mar 2023, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> > The array of rc_parameters contains a mixture of parameters from DSC 1.1
> > and DSC 1.2 standards. Split these tow configuration arrays in
> > preparation to adding more configuration data.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/display/drm_dsc_helper.c  | 127 ++++++++++++++++++----
> >  drivers/gpu/drm/i915/display/intel_vdsc.c |  10 +-
> >  include/drm/display/drm_dsc_helper.h      |   7 +-
> >  3 files changed, 119 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> > index acb93d4116e0..35b39f3109c4 100644
> > --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> > @@ -324,11 +324,81 @@ struct rc_parameters_data {
> >
> >  #define DSC_BPP(bpp) ((bpp) << 4)
> >
> > +static const struct rc_parameters_data rc_parameters_pre_scr[] = {
> > +     {
> > +             .bpp = DSC_BPP(8), .bpc = 8,
> > +             { 512, 12, 6144, 3, 12, 11, 11, {
> > +                     { 0, 4, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> > +                     { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> > +                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 }, { 5, 12, -12 },
> > +                     { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> > +                     }
> > +             }
> > +     },
> > +     {
> > +             .bpp = DSC_BPP(8), .bpc = 10,
> > +             { 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 },
> > +                     { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> > +                     }
> > +             }
> > +     },
> > +     {
> > +             .bpp = DSC_BPP(8), .bpc = 12,
> > +             { 512, 12, 6144, 11, 20, 19, 19, {
> > +                     { 0, 12, 2 }, { 4, 12, 0 }, { 9, 13, 0 }, { 9, 14, -2 },
> > +                     { 11, 15, -4 }, { 11, 15, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> > +                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> > +                     { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> > +                     { 21, 23, -12 }
> > +                     }
> > +             }
> > +     },
> > +     {
> > +             .bpp = DSC_BPP(12), .bpc = 8,
> > +             { 341, 15, 2048, 3, 12, 11, 11, {
> > +                     { 0, 2, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> > +                     { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> > +                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 },
> > +                     { 5, 12, -12 }, { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> > +                     }
> > +             }
> > +     },
> > +     {
> > +             .bpp = DSC_BPP(12), .bpc = 10,
> > +             { 341, 15, 2048, 7, 16, 15, 15, {
> > +                     { 0, 2, 2 }, { 2, 5, 0 }, { 3, 7, 0 }, { 4, 8, -2 },
> > +                     { 6, 9, -4 }, { 7, 10, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
> > +                     { 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
> > +                     { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> > +                     }
> > +             }
> > +     },
> > +     {
> > +             .bpp = DSC_BPP(12), .bpc = 12,
> > +             { 341, 15, 2048, 11, 20, 19, 19, {
> > +                     { 0, 6, 2 }, { 4, 9, 0 }, { 7, 11, 0 }, { 8, 12, -2 },
> > +                     { 10, 13, -4 }, { 11, 14, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> > +                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> > +                     { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> > +                     { 21, 23, -12 }
> > +                     }
> > +             }
> > +     },
> > +     { /* sentinel */ }
> > +};
> > +
> >  /*
> >   * Selected Rate Control Related Parameter Recommended Values
> >   * from DSC_v1.11 spec & C Model release: DSC_model_20161212
> >   */
>
> The comment is no longer accurate, is it?

Ugh, yes. it is no longer DSC 1.1. I cross-checked, the rc*cfg files
are the same between 20161212 and 20210623

>
> There are various ways to determine the parameters to use. There's even
> an application note "VESA DSC v1.2a Guidance on Deriving DSC Rate
> Control Parameters" that lists the options. They are all valid and
> should "provide visually lossless quality".
>
> Would it be simplest to always use the C model parameters in the tables
> here, referencing the zip file name with date above each table? That
> could at least be consistent, and drivers could override parameters
> using other methods if they desire. And it would be easiest to review.

Do you mean the calculated RC parameters? As I mentioned in another
email, it is at least worth investigating. I haven't looked into that,
as my primary goal (driven by the forthcoming drm/msm needs) were the
.cfg tables. I targeted cleaning up the simplest path to reduce
duplication (see [1]). Anyway, with the proposed patches we have the
API, which tells nothing about the way it fills out the RC tables.
They can be based on top of the cfg files (method 2) or calculated
(methods 3, 4).

[1] https://patchwork.freedesktop.org/patch/524051/?series=114355&rev=1

>
> I'm having a hard time finding time to review all this in a timely
> fashion. Would be good to try to get other folks to review the rest,
> it's really not very i915 specific anyway. In the mean time I think
> patches 1-5 are okay to merge via drm-misc.

For reference, the tables here are a direct conversion of the rc*cfg
files found in the DSC model. If you wish, I can post the program that
I used to convert these files into C arrays. Will that help the
review?

>
> BR,
> Jani.
>
> > -static const struct rc_parameters_data rc_parameters[] = {
> > +static const struct rc_parameters_data rc_parameters_1_2_444[] = {
> >       {
> >               .bpp = DSC_BPP(6), .bpc = 8,
> >               { 768, 15, 6144, 3, 13, 11, 11, {
> > @@ -388,22 +458,18 @@ static const struct rc_parameters_data rc_parameters[] = {
> >               { 512, 12, 6144, 3, 12, 11, 11, {
> >                       { 0, 4, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> >                       { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> > -                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 }, { 5, 12, -12 },
> > -                     { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> > +                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 10, -10 }, { 5, 11, -12 },
> > +                     { 5, 11, -12 }, { 9, 12, -12 }, { 12, 13, -12 }
> >                       }
> >               }
> >       },
> >       {
> >               .bpp = DSC_BPP(8), .bpc = 10,
> >               { 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 },
> > +                     { 0, 8, 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 },
> > -                     { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> > +                     { 7, 13, -8 }, { 7, 14, -10 }, { 9, 14, -10 }, { 9, 15, -12 },
> > +                     { 9, 15, -12 }, { 13, 16, -12 }, { 16, 17, -12 }
> >                       }
> >               }
> >       },
> > @@ -412,9 +478,9 @@ static const struct rc_parameters_data rc_parameters[] = {
> >               { 512, 12, 6144, 11, 20, 19, 19, {
> >                       { 0, 12, 2 }, { 4, 12, 0 }, { 9, 13, 0 }, { 9, 14, -2 },
> >                       { 11, 15, -4 }, { 11, 15, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> > -                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> > -                     { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> > -                     { 21, 23, -12 }
> > +                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 18, -10 },
> > +                     { 13, 19, -12 }, { 13, 19, -12 }, { 17, 20, -12 },
> > +                     { 20, 21, -12 }
> >                       }
> >               }
> >       },
> > @@ -498,8 +564,8 @@ static const struct rc_parameters_data rc_parameters[] = {
> >               { 341, 15, 2048, 3, 12, 11, 11, {
> >                       { 0, 2, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> >                       { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> > -                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 },
> > -                     { 5, 12, -12 }, { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> > +                     { 3, 8, -8 }, { 3, 9, -10 }, { 5, 9, -10 }, { 5, 9, -12 },
> > +                     { 5, 9, -12 }, { 7, 10, -12 }, { 10, 11, -12 }
> >                       }
> >               }
> >       },
> > @@ -508,8 +574,8 @@ static const struct rc_parameters_data rc_parameters[] = {
> >               { 341, 15, 2048, 7, 16, 15, 15, {
> >                       { 0, 2, 2 }, { 2, 5, 0 }, { 3, 7, 0 }, { 4, 8, -2 },
> >                       { 6, 9, -4 }, { 7, 10, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
> > -                     { 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
> > -                     { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> > +                     { 7, 12, -8 }, { 7, 13, -10 }, { 9, 13, -10 }, { 9, 13, -12 },
> > +                     { 9, 13, -12 }, { 11, 14, -12 }, { 14, 15, -12 }
> >                       }
> >               }
> >       },
> > @@ -518,9 +584,9 @@ static const struct rc_parameters_data rc_parameters[] = {
> >               { 341, 15, 2048, 11, 20, 19, 19, {
> >                       { 0, 6, 2 }, { 4, 9, 0 }, { 7, 11, 0 }, { 8, 12, -2 },
> >                       { 10, 13, -4 }, { 11, 14, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> > -                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> > -                     { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> > -                     { 21, 23, -12 }
> > +                     { 11, 16, -8 }, { 11, 17, -10 }, { 13, 17, -10 },
> > +                     { 13, 17, -12 }, { 13, 17, -12 }, { 15, 18, -12 },
> > +                     { 18, 19, -12 }
> >                       }
> >               }
> >       },
> > @@ -602,7 +668,8 @@ static const struct rc_parameters_data rc_parameters[] = {
> >       { /* sentinel */ }
> >  };
> >
> > -static const struct rc_parameters *get_rc_params(u16 dsc_bpp,
> > +static const struct rc_parameters *get_rc_params(const struct rc_parameters_data *rc_parameters,
> > +                                              u16 dsc_bpp,
> >                                                u8 bits_per_component)
> >  {
> >       int i;
> > @@ -622,11 +689,13 @@ static const struct rc_parameters *get_rc_params(u16 dsc_bpp,
> >   * function.
> >   *
> >   * @vdsc_cfg: DSC Configuration data partially filled by driver
> > + * @kind: operating mode and standard to follow
> >   *
> >   * Return: 0 or -error code in case of an error
> >   */
> > -int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg)
> > +int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum drm_dsc_params_kind kind)
> >  {
> > +     const struct rc_parameters_data *data;
> >       const struct rc_parameters *rc_params;
> >       int i;
> >
> > @@ -634,7 +703,19 @@ int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg)
> >                        !vdsc_cfg->bits_per_component))
> >               return -EINVAL;
> >
> > -     rc_params = get_rc_params(vdsc_cfg->bits_per_pixel,
> > +     switch (kind) {
> > +     case DRM_DSC_1_2_444:
> > +             data = rc_parameters_1_2_444;
> > +             break;
> > +     case DRM_DSC_1_1_PRE_SCR:
> > +             data = rc_parameters_pre_scr;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     rc_params = get_rc_params(data,
> > +                               vdsc_cfg->bits_per_pixel,
> >                                 vdsc_cfg->bits_per_component);
> >       if (!rc_params)
> >               return -EINVAL;
> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > index 20a4c2f343fe..a4d1d2ba71bb 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > @@ -157,7 +157,15 @@ int intel_dsc_compute_params(struct intel_crtc_state *pipe_config)
> >       if (DISPLAY_VER(dev_priv) >= 13) {
> >               calculate_rc_params(vdsc_cfg);
> >       } else {
> > -             ret = drm_dsc_setup_rc_params(vdsc_cfg);
> > +             if ((compressed_bpp == 8 ||
> > +                  compressed_bpp == 12) &&
> > +                 (vdsc_cfg->bits_per_component == 8 ||
> > +                  vdsc_cfg->bits_per_component == 10 ||
> > +                  vdsc_cfg->bits_per_component == 12))
> > +                     ret = drm_dsc_setup_rc_params(vdsc_cfg, DRM_DSC_1_1_PRE_SCR);
> > +             else
> > +                     ret = drm_dsc_setup_rc_params(vdsc_cfg, DRM_DSC_1_2_444);
> > +
> >               if (ret)
> >                       return ret;
> >
> > diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
> > index 1681791f65a5..c634bb2935d3 100644
> > --- a/include/drm/display/drm_dsc_helper.h
> > +++ b/include/drm/display/drm_dsc_helper.h
> > @@ -10,12 +10,17 @@
> >
> >  #include <drm/display/drm_dsc.h>
> >
> > +enum drm_dsc_params_kind {
> > +     DRM_DSC_1_2_444,
> > +     DRM_DSC_1_1_PRE_SCR, /* legacy params from DSC 1.1 */
> > +};
> > +
> >  void drm_dsc_dp_pps_header_init(struct dp_sdp_header *pps_header);
> >  int drm_dsc_dp_rc_buffer_size(u8 rc_buffer_block_size, u8 rc_buffer_size);
> >  void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_sdp,
> >                             const struct drm_dsc_config *dsc_cfg);
> >  void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg);
> > -int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg);
> > +int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum drm_dsc_params_kind kind);
> >  int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
> >
> >  #endif /* _DRM_DSC_HELPER_H_ */
>
> --
> Jani Nikula, Intel Open Source Graphics Center
Jani Nikula March 8, 2023, 1:19 p.m. UTC | #3 
On Wed, 08 Mar 2023, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Wed, 8 Mar 2023 at 12:14, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>
>> On Tue, 07 Mar 2023, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>> > The array of rc_parameters contains a mixture of parameters from DSC 1.1
>> > and DSC 1.2 standards. Split these tow configuration arrays in
>> > preparation to adding more configuration data.
>> >
>> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> > ---
>> >  drivers/gpu/drm/display/drm_dsc_helper.c  | 127 ++++++++++++++++++----
>> >  drivers/gpu/drm/i915/display/intel_vdsc.c |  10 +-
>> >  include/drm/display/drm_dsc_helper.h      |   7 +-
>> >  3 files changed, 119 insertions(+), 25 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
>> > index acb93d4116e0..35b39f3109c4 100644
>> > --- a/drivers/gpu/drm/display/drm_dsc_helper.c
>> > +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
>> > @@ -324,11 +324,81 @@ struct rc_parameters_data {
>> >
>> >  #define DSC_BPP(bpp) ((bpp) << 4)
>> >
>> > +static const struct rc_parameters_data rc_parameters_pre_scr[] = {
>> > +     {
>> > +             .bpp = DSC_BPP(8), .bpc = 8,
>> > +             { 512, 12, 6144, 3, 12, 11, 11, {
>> > +                     { 0, 4, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
>> > +                     { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
>> > +                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 }, { 5, 12, -12 },
>> > +                     { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
>> > +                     }
>> > +             }
>> > +     },
>> > +     {
>> > +             .bpp = DSC_BPP(8), .bpc = 10,
>> > +             { 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 },
>> > +                     { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
>> > +                     }
>> > +             }
>> > +     },
>> > +     {
>> > +             .bpp = DSC_BPP(8), .bpc = 12,
>> > +             { 512, 12, 6144, 11, 20, 19, 19, {
>> > +                     { 0, 12, 2 }, { 4, 12, 0 }, { 9, 13, 0 }, { 9, 14, -2 },
>> > +                     { 11, 15, -4 }, { 11, 15, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
>> > +                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
>> > +                     { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
>> > +                     { 21, 23, -12 }
>> > +                     }
>> > +             }
>> > +     },
>> > +     {
>> > +             .bpp = DSC_BPP(12), .bpc = 8,
>> > +             { 341, 15, 2048, 3, 12, 11, 11, {
>> > +                     { 0, 2, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
>> > +                     { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
>> > +                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 },
>> > +                     { 5, 12, -12 }, { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
>> > +                     }
>> > +             }
>> > +     },
>> > +     {
>> > +             .bpp = DSC_BPP(12), .bpc = 10,
>> > +             { 341, 15, 2048, 7, 16, 15, 15, {
>> > +                     { 0, 2, 2 }, { 2, 5, 0 }, { 3, 7, 0 }, { 4, 8, -2 },
>> > +                     { 6, 9, -4 }, { 7, 10, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
>> > +                     { 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
>> > +                     { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
>> > +                     }
>> > +             }
>> > +     },
>> > +     {
>> > +             .bpp = DSC_BPP(12), .bpc = 12,
>> > +             { 341, 15, 2048, 11, 20, 19, 19, {
>> > +                     { 0, 6, 2 }, { 4, 9, 0 }, { 7, 11, 0 }, { 8, 12, -2 },
>> > +                     { 10, 13, -4 }, { 11, 14, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
>> > +                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
>> > +                     { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
>> > +                     { 21, 23, -12 }
>> > +                     }
>> > +             }
>> > +     },
>> > +     { /* sentinel */ }
>> > +};
>> > +
>> >  /*
>> >   * Selected Rate Control Related Parameter Recommended Values
>> >   * from DSC_v1.11 spec & C Model release: DSC_model_20161212
>> >   */
>>
>> The comment is no longer accurate, is it?
>
> Ugh, yes. it is no longer DSC 1.1. I cross-checked, the rc*cfg files
> are the same between 20161212 and 20210623
>
>>
>> There are various ways to determine the parameters to use. There's even
>> an application note "VESA DSC v1.2a Guidance on Deriving DSC Rate
>> Control Parameters" that lists the options. They are all valid and
>> should "provide visually lossless quality".
>>
>> Would it be simplest to always use the C model parameters in the tables
>> here, referencing the zip file name with date above each table? That
>> could at least be consistent, and drivers could override parameters
>> using other methods if they desire. And it would be easiest to review.
>
> Do you mean the calculated RC parameters?

No, I simply mean using the values from the .cfg files, and clearly
stating that in the source. Your reply below implies this is exactly
what you're doing. What we currently have in intel_vdsc.c isn't clear by
any means.

Moreover, I started checking some of the values against the *specs* and
those actually differ from the .cfg files. I just want to get rid of the
ambiguity.

> As I mentioned in another
> email, it is at least worth investigating. I haven't looked into that,
> as my primary goal (driven by the forthcoming drm/msm needs) were the
> .cfg tables. I targeted cleaning up the simplest path to reduce
> duplication (see [1]). Anyway, with the proposed patches we have the
> API, which tells nothing about the way it fills out the RC tables.
> They can be based on top of the cfg files (method 2) or calculated
> (methods 3, 4).
>
> [1] https://patchwork.freedesktop.org/patch/524051/?series=114355&rev=1
>
>>
>> I'm having a hard time finding time to review all this in a timely
>> fashion. Would be good to try to get other folks to review the rest,
>> it's really not very i915 specific anyway. In the mean time I think
>> patches 1-5 are okay to merge via drm-misc.
>
> For reference, the tables here are a direct conversion of the rc*cfg
> files found in the DSC model. If you wish, I can post the program that
> I used to convert these files into C arrays. Will that help the
> review?

Probably; it's a bit annoying to cross check as the values aren't in the
same order.

BR,
Jani.


>
>>
>> BR,
>> Jani.
>>
>> > -static const struct rc_parameters_data rc_parameters[] = {
>> > +static const struct rc_parameters_data rc_parameters_1_2_444[] = {
>> >       {
>> >               .bpp = DSC_BPP(6), .bpc = 8,
>> >               { 768, 15, 6144, 3, 13, 11, 11, {
>> > @@ -388,22 +458,18 @@ static const struct rc_parameters_data rc_parameters[] = {
>> >               { 512, 12, 6144, 3, 12, 11, 11, {
>> >                       { 0, 4, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
>> >                       { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
>> > -                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 }, { 5, 12, -12 },
>> > -                     { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
>> > +                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 10, -10 }, { 5, 11, -12 },
>> > +                     { 5, 11, -12 }, { 9, 12, -12 }, { 12, 13, -12 }
>> >                       }
>> >               }
>> >       },
>> >       {
>> >               .bpp = DSC_BPP(8), .bpc = 10,
>> >               { 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 },
>> > +                     { 0, 8, 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 },
>> > -                     { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
>> > +                     { 7, 13, -8 }, { 7, 14, -10 }, { 9, 14, -10 }, { 9, 15, -12 },
>> > +                     { 9, 15, -12 }, { 13, 16, -12 }, { 16, 17, -12 }
>> >                       }
>> >               }
>> >       },
>> > @@ -412,9 +478,9 @@ static const struct rc_parameters_data rc_parameters[] = {
>> >               { 512, 12, 6144, 11, 20, 19, 19, {
>> >                       { 0, 12, 2 }, { 4, 12, 0 }, { 9, 13, 0 }, { 9, 14, -2 },
>> >                       { 11, 15, -4 }, { 11, 15, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
>> > -                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
>> > -                     { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
>> > -                     { 21, 23, -12 }
>> > +                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 18, -10 },
>> > +                     { 13, 19, -12 }, { 13, 19, -12 }, { 17, 20, -12 },
>> > +                     { 20, 21, -12 }
>> >                       }
>> >               }
>> >       },
>> > @@ -498,8 +564,8 @@ static const struct rc_parameters_data rc_parameters[] = {
>> >               { 341, 15, 2048, 3, 12, 11, 11, {
>> >                       { 0, 2, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
>> >                       { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
>> > -                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 },
>> > -                     { 5, 12, -12 }, { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
>> > +                     { 3, 8, -8 }, { 3, 9, -10 }, { 5, 9, -10 }, { 5, 9, -12 },
>> > +                     { 5, 9, -12 }, { 7, 10, -12 }, { 10, 11, -12 }
>> >                       }
>> >               }
>> >       },
>> > @@ -508,8 +574,8 @@ static const struct rc_parameters_data rc_parameters[] = {
>> >               { 341, 15, 2048, 7, 16, 15, 15, {
>> >                       { 0, 2, 2 }, { 2, 5, 0 }, { 3, 7, 0 }, { 4, 8, -2 },
>> >                       { 6, 9, -4 }, { 7, 10, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
>> > -                     { 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
>> > -                     { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
>> > +                     { 7, 12, -8 }, { 7, 13, -10 }, { 9, 13, -10 }, { 9, 13, -12 },
>> > +                     { 9, 13, -12 }, { 11, 14, -12 }, { 14, 15, -12 }
>> >                       }
>> >               }
>> >       },
>> > @@ -518,9 +584,9 @@ static const struct rc_parameters_data rc_parameters[] = {
>> >               { 341, 15, 2048, 11, 20, 19, 19, {
>> >                       { 0, 6, 2 }, { 4, 9, 0 }, { 7, 11, 0 }, { 8, 12, -2 },
>> >                       { 10, 13, -4 }, { 11, 14, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
>> > -                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
>> > -                     { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
>> > -                     { 21, 23, -12 }
>> > +                     { 11, 16, -8 }, { 11, 17, -10 }, { 13, 17, -10 },
>> > +                     { 13, 17, -12 }, { 13, 17, -12 }, { 15, 18, -12 },
>> > +                     { 18, 19, -12 }
>> >                       }
>> >               }
>> >       },
>> > @@ -602,7 +668,8 @@ static const struct rc_parameters_data rc_parameters[] = {
>> >       { /* sentinel */ }
>> >  };
>> >
>> > -static const struct rc_parameters *get_rc_params(u16 dsc_bpp,
>> > +static const struct rc_parameters *get_rc_params(const struct rc_parameters_data *rc_parameters,
>> > +                                              u16 dsc_bpp,
>> >                                                u8 bits_per_component)
>> >  {
>> >       int i;
>> > @@ -622,11 +689,13 @@ static const struct rc_parameters *get_rc_params(u16 dsc_bpp,
>> >   * function.
>> >   *
>> >   * @vdsc_cfg: DSC Configuration data partially filled by driver
>> > + * @kind: operating mode and standard to follow
>> >   *
>> >   * Return: 0 or -error code in case of an error
>> >   */
>> > -int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg)
>> > +int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum drm_dsc_params_kind kind)
>> >  {
>> > +     const struct rc_parameters_data *data;
>> >       const struct rc_parameters *rc_params;
>> >       int i;
>> >
>> > @@ -634,7 +703,19 @@ int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg)
>> >                        !vdsc_cfg->bits_per_component))
>> >               return -EINVAL;
>> >
>> > -     rc_params = get_rc_params(vdsc_cfg->bits_per_pixel,
>> > +     switch (kind) {
>> > +     case DRM_DSC_1_2_444:
>> > +             data = rc_parameters_1_2_444;
>> > +             break;
>> > +     case DRM_DSC_1_1_PRE_SCR:
>> > +             data = rc_parameters_pre_scr;
>> > +             break;
>> > +     default:
>> > +             return -EINVAL;
>> > +     }
>> > +
>> > +     rc_params = get_rc_params(data,
>> > +                               vdsc_cfg->bits_per_pixel,
>> >                                 vdsc_cfg->bits_per_component);
>> >       if (!rc_params)
>> >               return -EINVAL;
>> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> > index 20a4c2f343fe..a4d1d2ba71bb 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> > @@ -157,7 +157,15 @@ int intel_dsc_compute_params(struct intel_crtc_state *pipe_config)
>> >       if (DISPLAY_VER(dev_priv) >= 13) {
>> >               calculate_rc_params(vdsc_cfg);
>> >       } else {
>> > -             ret = drm_dsc_setup_rc_params(vdsc_cfg);
>> > +             if ((compressed_bpp == 8 ||
>> > +                  compressed_bpp == 12) &&
>> > +                 (vdsc_cfg->bits_per_component == 8 ||
>> > +                  vdsc_cfg->bits_per_component == 10 ||
>> > +                  vdsc_cfg->bits_per_component == 12))
>> > +                     ret = drm_dsc_setup_rc_params(vdsc_cfg, DRM_DSC_1_1_PRE_SCR);
>> > +             else
>> > +                     ret = drm_dsc_setup_rc_params(vdsc_cfg, DRM_DSC_1_2_444);
>> > +
>> >               if (ret)
>> >                       return ret;
>> >
>> > diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
>> > index 1681791f65a5..c634bb2935d3 100644
>> > --- a/include/drm/display/drm_dsc_helper.h
>> > +++ b/include/drm/display/drm_dsc_helper.h
>> > @@ -10,12 +10,17 @@
>> >
>> >  #include <drm/display/drm_dsc.h>
>> >
>> > +enum drm_dsc_params_kind {
>> > +     DRM_DSC_1_2_444,
>> > +     DRM_DSC_1_1_PRE_SCR, /* legacy params from DSC 1.1 */
>> > +};
>> > +
>> >  void drm_dsc_dp_pps_header_init(struct dp_sdp_header *pps_header);
>> >  int drm_dsc_dp_rc_buffer_size(u8 rc_buffer_block_size, u8 rc_buffer_size);
>> >  void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_sdp,
>> >                             const struct drm_dsc_config *dsc_cfg);
>> >  void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg);
>> > -int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg);
>> > +int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum drm_dsc_params_kind kind);
>> >  int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>> >
>> >  #endif /* _DRM_DSC_HELPER_H_ */
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
Dmitry Baryshkov March 8, 2023, 4:50 p.m. UTC | #4 
On Wed, 8 Mar 2023 at 15:19, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Wed, 08 Mar 2023, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> > On Wed, 8 Mar 2023 at 12:14, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>
> >> On Tue, 07 Mar 2023, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> >> > The array of rc_parameters contains a mixture of parameters from DSC 1.1
> >> > and DSC 1.2 standards. Split these tow configuration arrays in
> >> > preparation to adding more configuration data.
> >> >
> >> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> > ---
> >> >  drivers/gpu/drm/display/drm_dsc_helper.c  | 127 ++++++++++++++++++----
> >> >  drivers/gpu/drm/i915/display/intel_vdsc.c |  10 +-
> >> >  include/drm/display/drm_dsc_helper.h      |   7 +-
> >> >  3 files changed, 119 insertions(+), 25 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> >> > index acb93d4116e0..35b39f3109c4 100644
> >> > --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> >> > +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> >> > @@ -324,11 +324,81 @@ struct rc_parameters_data {
> >> >
> >> >  #define DSC_BPP(bpp) ((bpp) << 4)
> >> >
> >> > +static const struct rc_parameters_data rc_parameters_pre_scr[] = {
> >> > +     {
> >> > +             .bpp = DSC_BPP(8), .bpc = 8,
> >> > +             { 512, 12, 6144, 3, 12, 11, 11, {
> >> > +                     { 0, 4, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> >> > +                     { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> >> > +                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 }, { 5, 12, -12 },
> >> > +                     { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> >> > +                     }
> >> > +             }
> >> > +     },
> >> > +     {
> >> > +             .bpp = DSC_BPP(8), .bpc = 10,
> >> > +             { 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 },
> >> > +                     { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> >> > +                     }
> >> > +             }
> >> > +     },
> >> > +     {
> >> > +             .bpp = DSC_BPP(8), .bpc = 12,
> >> > +             { 512, 12, 6144, 11, 20, 19, 19, {
> >> > +                     { 0, 12, 2 }, { 4, 12, 0 }, { 9, 13, 0 }, { 9, 14, -2 },
> >> > +                     { 11, 15, -4 }, { 11, 15, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> >> > +                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> >> > +                     { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> >> > +                     { 21, 23, -12 }
> >> > +                     }
> >> > +             }
> >> > +     },
> >> > +     {
> >> > +             .bpp = DSC_BPP(12), .bpc = 8,
> >> > +             { 341, 15, 2048, 3, 12, 11, 11, {
> >> > +                     { 0, 2, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> >> > +                     { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> >> > +                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 },
> >> > +                     { 5, 12, -12 }, { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> >> > +                     }
> >> > +             }
> >> > +     },
> >> > +     {
> >> > +             .bpp = DSC_BPP(12), .bpc = 10,
> >> > +             { 341, 15, 2048, 7, 16, 15, 15, {
> >> > +                     { 0, 2, 2 }, { 2, 5, 0 }, { 3, 7, 0 }, { 4, 8, -2 },
> >> > +                     { 6, 9, -4 }, { 7, 10, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
> >> > +                     { 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
> >> > +                     { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> >> > +                     }
> >> > +             }
> >> > +     },
> >> > +     {
> >> > +             .bpp = DSC_BPP(12), .bpc = 12,
> >> > +             { 341, 15, 2048, 11, 20, 19, 19, {
> >> > +                     { 0, 6, 2 }, { 4, 9, 0 }, { 7, 11, 0 }, { 8, 12, -2 },
> >> > +                     { 10, 13, -4 }, { 11, 14, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> >> > +                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> >> > +                     { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> >> > +                     { 21, 23, -12 }
> >> > +                     }
> >> > +             }
> >> > +     },
> >> > +     { /* sentinel */ }
> >> > +};
> >> > +
> >> >  /*
> >> >   * Selected Rate Control Related Parameter Recommended Values
> >> >   * from DSC_v1.11 spec & C Model release: DSC_model_20161212
> >> >   */
> >>
> >> The comment is no longer accurate, is it?
> >
> > Ugh, yes. it is no longer DSC 1.1. I cross-checked, the rc*cfg files
> > are the same between 20161212 and 20210623
> >
> >>
> >> There are various ways to determine the parameters to use. There's even
> >> an application note "VESA DSC v1.2a Guidance on Deriving DSC Rate
> >> Control Parameters" that lists the options. They are all valid and
> >> should "provide visually lossless quality".
> >>
> >> Would it be simplest to always use the C model parameters in the tables
> >> here, referencing the zip file name with date above each table? That
> >> could at least be consistent, and drivers could override parameters
> >> using other methods if they desire. And it would be easiest to review.
> >
> > Do you mean the calculated RC parameters?
>
> No, I simply mean using the values from the .cfg files, and clearly
> stating that in the source. Your reply below implies this is exactly
> what you're doing. What we currently have in intel_vdsc.c isn't clear by
> any means.

Yes, it took me a while to narrow down which entries correspond to
'legacy'/pre-SCR configs.

>
> Moreover, I started checking some of the values against the *specs* and
> those actually differ from the .cfg files. I just want to get rid of the
> ambiguity.

Ugh.

>
> > As I mentioned in another
> > email, it is at least worth investigating. I haven't looked into that,
> > as my primary goal (driven by the forthcoming drm/msm needs) were the
> > .cfg tables. I targeted cleaning up the simplest path to reduce
> > duplication (see [1]). Anyway, with the proposed patches we have the
> > API, which tells nothing about the way it fills out the RC tables.
> > They can be based on top of the cfg files (method 2) or calculated
> > (methods 3, 4).
> >
> > [1] https://patchwork.freedesktop.org/patch/524051/?series=114355&rev=1
> >
> >>
> >> I'm having a hard time finding time to review all this in a timely
> >> fashion. Would be good to try to get other folks to review the rest,
> >> it's really not very i915 specific anyway. In the mean time I think
> >> patches 1-5 are okay to merge via drm-misc.
> >
> > For reference, the tables here are a direct conversion of the rc*cfg
> > files found in the DSC model. If you wish, I can post the program that
> > I used to convert these files into C arrays. Will that help the
> > review?
>
> Probably; it's a bit annoying to cross check as the values aren't in the
> same order.

I pushed my code to
https://git.linaro.org/people/dmitry.baryshkov/dsc_model_process.git/

>
> BR,
> Jani.
>
>
> >
> >>
> >> BR,
> >> Jani.
> >>
> >> > -static const struct rc_parameters_data rc_parameters[] = {
> >> > +static const struct rc_parameters_data rc_parameters_1_2_444[] = {
> >> >       {
> >> >               .bpp = DSC_BPP(6), .bpc = 8,
> >> >               { 768, 15, 6144, 3, 13, 11, 11, {
> >> > @@ -388,22 +458,18 @@ static const struct rc_parameters_data rc_parameters[] = {
> >> >               { 512, 12, 6144, 3, 12, 11, 11, {
> >> >                       { 0, 4, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> >> >                       { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> >> > -                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 }, { 5, 12, -12 },
> >> > -                     { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> >> > +                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 10, -10 }, { 5, 11, -12 },
> >> > +                     { 5, 11, -12 }, { 9, 12, -12 }, { 12, 13, -12 }
> >> >                       }
> >> >               }
> >> >       },
> >> >       {
> >> >               .bpp = DSC_BPP(8), .bpc = 10,
> >> >               { 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 },
> >> > +                     { 0, 8, 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 },
> >> > -                     { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> >> > +                     { 7, 13, -8 }, { 7, 14, -10 }, { 9, 14, -10 }, { 9, 15, -12 },
> >> > +                     { 9, 15, -12 }, { 13, 16, -12 }, { 16, 17, -12 }
> >> >                       }
> >> >               }
> >> >       },
> >> > @@ -412,9 +478,9 @@ static const struct rc_parameters_data rc_parameters[] = {
> >> >               { 512, 12, 6144, 11, 20, 19, 19, {
> >> >                       { 0, 12, 2 }, { 4, 12, 0 }, { 9, 13, 0 }, { 9, 14, -2 },
> >> >                       { 11, 15, -4 }, { 11, 15, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> >> > -                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> >> > -                     { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> >> > -                     { 21, 23, -12 }
> >> > +                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 18, -10 },
> >> > +                     { 13, 19, -12 }, { 13, 19, -12 }, { 17, 20, -12 },
> >> > +                     { 20, 21, -12 }
> >> >                       }
> >> >               }
> >> >       },
> >> > @@ -498,8 +564,8 @@ static const struct rc_parameters_data rc_parameters[] = {
> >> >               { 341, 15, 2048, 3, 12, 11, 11, {
> >> >                       { 0, 2, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> >> >                       { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> >> > -                     { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 },
> >> > -                     { 5, 12, -12 }, { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> >> > +                     { 3, 8, -8 }, { 3, 9, -10 }, { 5, 9, -10 }, { 5, 9, -12 },
> >> > +                     { 5, 9, -12 }, { 7, 10, -12 }, { 10, 11, -12 }
> >> >                       }
> >> >               }
> >> >       },
> >> > @@ -508,8 +574,8 @@ static const struct rc_parameters_data rc_parameters[] = {
> >> >               { 341, 15, 2048, 7, 16, 15, 15, {
> >> >                       { 0, 2, 2 }, { 2, 5, 0 }, { 3, 7, 0 }, { 4, 8, -2 },
> >> >                       { 6, 9, -4 }, { 7, 10, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
> >> > -                     { 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
> >> > -                     { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> >> > +                     { 7, 12, -8 }, { 7, 13, -10 }, { 9, 13, -10 }, { 9, 13, -12 },
> >> > +                     { 9, 13, -12 }, { 11, 14, -12 }, { 14, 15, -12 }
> >> >                       }
> >> >               }
> >> >       },
> >> > @@ -518,9 +584,9 @@ static const struct rc_parameters_data rc_parameters[] = {
> >> >               { 341, 15, 2048, 11, 20, 19, 19, {
> >> >                       { 0, 6, 2 }, { 4, 9, 0 }, { 7, 11, 0 }, { 8, 12, -2 },
> >> >                       { 10, 13, -4 }, { 11, 14, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> >> > -                     { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> >> > -                     { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> >> > -                     { 21, 23, -12 }
> >> > +                     { 11, 16, -8 }, { 11, 17, -10 }, { 13, 17, -10 },
> >> > +                     { 13, 17, -12 }, { 13, 17, -12 }, { 15, 18, -12 },
> >> > +                     { 18, 19, -12 }
> >> >                       }
> >> >               }
> >> >       },
> >> > @@ -602,7 +668,8 @@ static const struct rc_parameters_data rc_parameters[] = {
> >> >       { /* sentinel */ }
> >> >  };
> >> >
> >> > -static const struct rc_parameters *get_rc_params(u16 dsc_bpp,
> >> > +static const struct rc_parameters *get_rc_params(const struct rc_parameters_data *rc_parameters,
> >> > +                                              u16 dsc_bpp,
> >> >                                                u8 bits_per_component)
> >> >  {
> >> >       int i;
> >> > @@ -622,11 +689,13 @@ static const struct rc_parameters *get_rc_params(u16 dsc_bpp,
> >> >   * function.
> >> >   *
> >> >   * @vdsc_cfg: DSC Configuration data partially filled by driver
> >> > + * @kind: operating mode and standard to follow
> >> >   *
> >> >   * Return: 0 or -error code in case of an error
> >> >   */
> >> > -int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg)
> >> > +int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum drm_dsc_params_kind kind)
> >> >  {
> >> > +     const struct rc_parameters_data *data;
> >> >       const struct rc_parameters *rc_params;
> >> >       int i;
> >> >
> >> > @@ -634,7 +703,19 @@ int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg)
> >> >                        !vdsc_cfg->bits_per_component))
> >> >               return -EINVAL;
> >> >
> >> > -     rc_params = get_rc_params(vdsc_cfg->bits_per_pixel,
> >> > +     switch (kind) {
> >> > +     case DRM_DSC_1_2_444:
> >> > +             data = rc_parameters_1_2_444;
> >> > +             break;
> >> > +     case DRM_DSC_1_1_PRE_SCR:
> >> > +             data = rc_parameters_pre_scr;
> >> > +             break;
> >> > +     default:
> >> > +             return -EINVAL;
> >> > +     }
> >> > +
> >> > +     rc_params = get_rc_params(data,
> >> > +                               vdsc_cfg->bits_per_pixel,
> >> >                                 vdsc_cfg->bits_per_component);
> >> >       if (!rc_params)
> >> >               return -EINVAL;
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> > index 20a4c2f343fe..a4d1d2ba71bb 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> > @@ -157,7 +157,15 @@ int intel_dsc_compute_params(struct intel_crtc_state *pipe_config)
> >> >       if (DISPLAY_VER(dev_priv) >= 13) {
> >> >               calculate_rc_params(vdsc_cfg);
> >> >       } else {
> >> > -             ret = drm_dsc_setup_rc_params(vdsc_cfg);
> >> > +             if ((compressed_bpp == 8 ||
> >> > +                  compressed_bpp == 12) &&
> >> > +                 (vdsc_cfg->bits_per_component == 8 ||
> >> > +                  vdsc_cfg->bits_per_component == 10 ||
> >> > +                  vdsc_cfg->bits_per_component == 12))
> >> > +                     ret = drm_dsc_setup_rc_params(vdsc_cfg, DRM_DSC_1_1_PRE_SCR);
> >> > +             else
> >> > +                     ret = drm_dsc_setup_rc_params(vdsc_cfg, DRM_DSC_1_2_444);
> >> > +
> >> >               if (ret)
> >> >                       return ret;
> >> >
> >> > diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
> >> > index 1681791f65a5..c634bb2935d3 100644
> >> > --- a/include/drm/display/drm_dsc_helper.h
> >> > +++ b/include/drm/display/drm_dsc_helper.h
> >> > @@ -10,12 +10,17 @@
> >> >
> >> >  #include <drm/display/drm_dsc.h>
> >> >
> >> > +enum drm_dsc_params_kind {
> >> > +     DRM_DSC_1_2_444,
> >> > +     DRM_DSC_1_1_PRE_SCR, /* legacy params from DSC 1.1 */
> >> > +};
> >> > +
> >> >  void drm_dsc_dp_pps_header_init(struct dp_sdp_header *pps_header);
> >> >  int drm_dsc_dp_rc_buffer_size(u8 rc_buffer_block_size, u8 rc_buffer_size);
> >> >  void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_sdp,
> >> >                             const struct drm_dsc_config *dsc_cfg);
> >> >  void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg);
> >> > -int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg);
> >> > +int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum drm_dsc_params_kind kind);
> >> >  int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
> >> >
> >> >  #endif /* _DRM_DSC_HELPER_H_ */
> >>
> >> --
> >> Jani Nikula, Intel Open Source Graphics Center
>
> --
> Jani Nikula, Intel Open Source Graphics Center