Re: [Freedreno] [PATCH v5 6/8] drm/display/dsc: split DSC 1.2 and DSC 1.1 (pre-SCR) parameters

[Dmitry Baryshkov]

On 16/05/2023 21:46, Kandpal, Suraj 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.


Hi ,
Needed to add some more comments apart from the previous ones already given


[skipped]


diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
b/drivers/gpu/drm/i915/display/intel_vdsc.c
index d4340b18c18d..bd9116d2cd76 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
@@ -226,7 +226,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);
+

I do not think this kind of assignment works as you will also be adding
DRM_DSC_1_2_422 and DRM_DSC_1_2_420 in further patches and AFAICS
There is no where in patch 8 that you have accounted for when 422 or 420 will 
be used.
Maybe you can add an if case inside the else block to check 
pipe_config->output_format
to pass the rc_param_data in patch 8

I don't think this is necessary for now. The driver doesn't support YUV 
422. The YUV 420 is supported only for DISPLAY_VER(dev_priv) >= 14, 
however these helpers are only used for DISPLAY_VER(dev_priv) < 13.

I did not move RC calculation to drm_dsc_helpers.c (yet ?), which is 
used for DISPLAY_VER >= 13.


Regards,
Suraj Kandpal
                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_ */
--
2.39.2


--
With best wishes
Dmitry