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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 28 Feb 2023, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Tue, 28 Feb 2023 at 14:25, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>
>> On Tue, 28 Feb 2023, Dmitry Baryshkov <dmitry.baryshkov@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@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().

Regardless, I'm still saying don't do that.

Data is not an interface.

If you make it easy to just use the data, nobody will ever fix their
drivers to use proper interfaces, and you'll lock yourself to a
particular representation of the data even though it's supposed to be a
hidden implementation detail.


BR,
Jani.


>
>>
>> > +
>> > +/**
>> > + * 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

-- 
Jani Nikula, Intel Open Source Graphics Center