On 5/14/2023 2:25 PM, Marijn Suijten wrote:
On 2023-05-12 14:32:14, Jessica Zhang wrote:
Introduce MSM-specific DSC helper methods, as some calculations are
common between DP and DSC.
Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
---
drivers/gpu/drm/msm/msm_dsc_helper.h | 65 ++++++++++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_dsc_helper.h b/drivers/gpu/drm/msm/msm_dsc_helper.h
new file mode 100644
index 000000000000..0d2a097b428d
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_dsc_helper.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved
+ */
+
+#ifndef MSM_DSC_HELPER_H_
+#define MSM_DSC_HELPER_H_
+
+#include <linux/bug.h>
+#include <linux/math.h>
+#include <drm/display/drm_dsc_helper.h>
+
+/*
+ * Helper methods for MSM specific DSC calculations that are common between timing engine,
+ * DSI, and DP.
+ */
Isn't this more common to have directly below the copyright statement,
above the includes?
Hi Marijn,
Acked.
+
+/**
+ * msm_dsc_get_bpp_int() - get bits per pixel integer value
+ * @dsc: Pointer to drm dsc config struct
+ * Returns: BPP integer value
+ */
+static inline int msm_dsc_get_bpp_int(struct drm_dsc_config *dsc)
Const, as requested elsewhere. But this function is not used anywhere
in any of the series (because we replaced the usages with more sensible
member accesses like slice_chunk_size).
Acked.
I would prefer to keep this helper so that we have a way to easily get
BPP information from the DRM DSC config in the future, but if you'd
prefer we add this helper then, I'm also ok with that.
+{
+ WARN_ON_ONCE(dsc->bits_per_pixel & 0xf);
+ return dsc->bits_per_pixel >> 4;
+}
+
+/**
+ * msm_dsc_get_slice_per_intf() - get number of slices per interface
+ * @dsc: Pointer to drm dsc config struct
+ * @intf_width: interface width
Width of the interface (to query), *in pixels*
Acked.
+ * Returns: Integer representing the slice per interface
the *number of slices* per interface.
Also, the returned value applies specifically to *the given interface*
(width).
Acked.
+ */
+static inline int msm_dsc_get_slice_per_intf(struct drm_dsc_config *dsc, int intf_width)
Const pointer.
Also: sliceS_per_intf? It's pluiral in the docs too.
Should the argument and return value be u32, to match the uses? Same
for everything below.
Acked.
+{
+ return DIV_ROUND_UP(intf_width, dsc->slice_width);
+}
+
+/**
+ * msm_dsc_get_bytes_per_line() - Calculate bytes per line
Calculate -> (lowecase) get
(to match all the other helpers in this file)
Acked.
+ * @dsc: Pointer to drm dsc config struct
+ * Returns: Integer value representing pclk per interface
+ *
+ * Note: This value will then be passed along to DSI and DP for some more
+ * calculations. This is because DSI and DP divide the pclk_per_intf value
+ * by different values depending on if widebus is enabled.
Can you elaborate what this "note" is trying to tell users of this
function? That they should not use bytes_per_line raw? That it doesn't
actually represent bytes_per_line if the extra calculations mentioned
here are not applied?
The latter -- this method is used for calculating the pclk for DSI and
DP. While it does get the raw bytes_per_line, there are some extra
calculations needed before it can be set as the pclk_rate. These "extra
calculations" are different between DP and DSI.
For more context, please refer to the earlier revisions of this patch
and the usage of the helper in dsi_host.c
+ */
+static inline int msm_dsc_get_bytes_per_line(struct drm_dsc_config *dsc)
const, return u32.
Acked.
+{
+ return dsc->slice_count * dsc->slice_chunk_size;
This is a u8 times a u16. Could it overflow a u16 and should we hence
cast one of the expressions to u32 first?
Acked.
+}
+
+/**
+ * msm_dsc_get_bytes_per_intf() - get total bytes per interface
+ * @dsc: Pointer to drm dsc config struct
+ * @intf_width: interface width
+ * Returns: u32 value representing bytes per interface
Nit: no need to repeat the type, I think? Just "number of bytes per
interface" is more concise.
Acked.
+ */
+static inline u32 msm_dsc_get_bytes_per_intf(struct drm_dsc_config *dsc, int intf_width)
And one more const.
Acked.
Not sure that this helper is useful though: it is
only used where msm_dsc_get_slice_per_intf() was already called, so it
makes more sense to the reader to just multiply slice_per_intf by
slice_chunk_size than to defer to an opaque helper.
I would prefer to keep this as a helper as this math is common between
DP and DSI, and I believe the name of the helper accurately reflects
what is being calculated.
If there's any confusion with the name of the method, I am open to
suggestions.
Thanks,
Jessica Zhang
- Marijn
+{
+ return dsc->slice_chunk_size * msm_dsc_get_slice_per_intf(dsc, intf_width);
+}
+
+#endif /* MSM_DSC_HELPER_H_ */
--
2.40.1
