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

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

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