> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Sent: Wednesday, May 17, 2023 5:33 AM
> To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; David Airlie
> <airlied@xxxxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>; Jani Nikula
> <jani.nikula@xxxxxxxxxxxxxxx>; Joonas Lahtinen
> <joonas.lahtinen@xxxxxxxxxxxxxxx>; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>;
> Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>; Rob Clark
> <robdclark@xxxxxxxxx>; Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>;
> Sean Paul <sean@xxxxxxxxxx>; Marijn Suijten
> <marijn.suijten@xxxxxxxxxxxxxx>
> Cc: linux-arm-msm@xxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> freedreno@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Ville
> Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 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
