Re: [PATCH v10 4/8] drm/msm: Add MSM-specific DSC helper methods

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux