On 28/02/2023 14:49, Jani Nikula wrote:
On Tue, 28 Feb 2023, Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote:
On Tue, 28 Feb 2023 at 14:25, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
On Tue, 28 Feb 2023, Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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.
Yes, I usually do not export data, exactly for these reasons. I could
have argued here that the data is constant here, etc. etc.
However let's stop caring about other drivers. I'll drop the export for v2.
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
--
With best wishes
Dmitry
