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

[Kandpal, Suraj]

> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> Sent: Wednesday, May 17, 2023 5:33 AM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>; David Airlie
> <airlied at gmail.com>; Daniel Vetter <daniel at ffwll.ch>; Jani Nikula
> <jani.nikula at linux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen at linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>;
> Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>; Rob Clark
> <robdclark at gmail.com>; Abhinav Kumar <quic_abhinavk at quicinc.com>;
> Sean Paul <sean at poorly.run>; Marijn Suijten
> <marijn.suijten at somainline.org>
> Cc: linux-arm-msm at vger.kernel.org; intel-gfx at lists.freedesktop.org;
> freedreno at lists.freedesktop.org; dri-devel at lists.freedesktop.org; Ville
> Syrjälä <ville.syrjala at linux.intel.com>
> Subject: Re: [Freedreno] [PATCH v5 6/8] drm/display/dsc: split DSC 1.2 and
> DSC 1.1 (pre-SCR) parameters
> 
> 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.

Hmm. I see I'll work on it once this patch series is merged

Regards,
Suraj Kandpal
> 
> >
> > 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