[PATCH 02/10] drm/i915/dsc: move rc_buf_thresh values to common helper

[Dmitry Baryshkov]

On Tue, 28 Feb 2023 at 14:25, Jani Nikula <jani.nikula at linux.intel.com> wrote:
>
> On Tue, 28 Feb 2023, Dmitry Baryshkov <dmitry.baryshkov at linaro.org> wrote:
> > The rc_buf_thresh values are common to all DSC implementations. Move
> > them to the common helper together with the code to propagage them to
> > the drm_dsc_config.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> > ---
> >  drivers/gpu/drm/display/drm_dsc_helper.c  | 37 +++++++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_vdsc.c | 24 +--------------
> >  include/drm/display/drm_dsc_helper.h      |  1 +
> >  3 files changed, 39 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> > index c869c6e51e2b..ab8679c158b5 100644
> > --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> > @@ -270,6 +270,43 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
> >  }
> >  EXPORT_SYMBOL(drm_dsc_pps_payload_pack);
> >
> > +/* From DSC_v1.11 spec, rc_parameter_Set syntax element typically constant */
> > +const u16 drm_dsc_rc_buf_thresh[] = {
> > +     896, 1792, 2688, 3584, 4480, 5376, 6272, 6720, 7168, 7616,
> > +     7744, 7872, 8000, 8064
> > +};
> > +EXPORT_SYMBOL(drm_dsc_rc_buf_thresh);
>
> This needs to be static, without exports.

Exported this to let other drivers use it, while skipping the
drm_dsc_set_rc_buf_thresh(). For example amdgpu driver sets buffer
thresholds on the interim structure, so the helper is not directly
applicable. See _do_calc_rc_params().

>
> > +
> > +/**
> > + * drm_dsc_set_rc_buf_thresh() - Set thresholds for the RC model
> > + * in accordance with the DSC 1.2 specification.
> > + *
> > + * @vdsc_cfg: DSC Configuration data partially filled by driver
> > + */
> > +void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg)
> > +{
> > +     int i = 0;
>
> Unnecessary initialization.

My bad.

>
> > +
> > +     for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
>
> Please use ARRAY_SIZE(). Maybe add BUILD_BUG_ON() for DSC_NUM_BUF_RANGES
> vs. ARRAY_SIZE(). (Yes, we should've used ARRAY_SIZE() in i915.)

Ack

>
> > +             /*
> > +              * six 0s are appended to the lsb of each threshold value
> > +              * internally in h/w.
> > +              * Only 8 bits are allowed for programming RcBufThreshold
> > +              */
> > +             vdsc_cfg->rc_buf_thresh[i] = drm_dsc_rc_buf_thresh[i] >> 6;
> > +     }
> > +
> > +     /*
> > +      * For 6bpp, RC Buffer threshold 12 and 13 need a different value
> > +      * as per C Model
> > +      */
> > +     if (vdsc_cfg->bits_per_pixel == 6 << 4) {
> > +             vdsc_cfg->rc_buf_thresh[12] = 7936 >> 6;
> > +             vdsc_cfg->rc_buf_thresh[13] = 8000 >> 6;
> > +     }
> > +}
> > +EXPORT_SYMBOL(drm_dsc_set_rc_buf_thresh);
> > +
> >  /**
> >   * drm_dsc_compute_rc_parameters() - Write rate control
> >   * parameters to the dsc configuration defined in
> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > index d080741fd0b3..b4faab4c8fb3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > @@ -36,12 +36,6 @@ enum COLUMN_INDEX_BPC {
> >       MAX_COLUMN_INDEX
> >  };
> >
> > -/* From DSC_v1.11 spec, rc_parameter_Set syntax element typically constant */
> > -static const u16 rc_buf_thresh[] = {
> > -     896, 1792, 2688, 3584, 4480, 5376, 6272, 6720, 7168, 7616,
> > -     7744, 7872, 8000, 8064
> > -};
> > -
> >  struct rc_parameters {
> >       u16 initial_xmit_delay;
> >       u8 first_line_bpg_offset;
> > @@ -474,23 +468,7 @@ int intel_dsc_compute_params(struct intel_crtc_state *pipe_config)
> >       vdsc_cfg->bits_per_pixel = compressed_bpp << 4;
> >       vdsc_cfg->bits_per_component = pipe_config->pipe_bpp / 3;
> >
> > -     for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
> > -             /*
> > -              * six 0s are appended to the lsb of each threshold value
> > -              * internally in h/w.
> > -              * Only 8 bits are allowed for programming RcBufThreshold
> > -              */
> > -             vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
> > -     }
> > -
> > -     /*
> > -      * For 6bpp, RC Buffer threshold 12 and 13 need a different value
> > -      * as per C Model
> > -      */
> > -     if (compressed_bpp == 6) {
> > -             vdsc_cfg->rc_buf_thresh[12] = 0x7C;
> > -             vdsc_cfg->rc_buf_thresh[13] = 0x7D;
> > -     }
> > +     drm_dsc_set_rc_buf_thresh(vdsc_cfg);
> >
> >       /*
> >        * From XE_LPD onwards we supports compression bpps in steps of 1
> > diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
> > index 8b41edbbabab..706ba1d34742 100644
> > --- a/include/drm/display/drm_dsc_helper.h
> > +++ b/include/drm/display/drm_dsc_helper.h
> > @@ -14,6 +14,7 @@ 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_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
> >
> >  #endif /* _DRM_DSC_HELPER_H_ */
>
> --
> Jani Nikula, Intel Open Source Graphics Center



-- 
With best wishes
Dmitry