Intel-GFX Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH 0/6] drm/edid: split out drm_eld.[ch], add some SAD helpers
@ 2023-09-07  9:28 Jani Nikula
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 1/6] drm/edid: split out drm_eld.h from drm_edid.h Jani Nikula
                   ` (8 more replies)
  0 siblings, 9 replies; 21+ messages in thread
From: Jani Nikula @ 2023-09-07  9:28 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Split out drm_eld.[ch] from drm_edid.h, add some helpers to convert and
modify SADs.

This should make it easier and more robust to implement things like [1],
with ELD parsing details hidden in drm_eld.[ch].

	for (i = 0; i < drm_eld_sad_count(eld); i++) {
		struct cea_sad sad;

		drm_eld_sad_get(eld, i, &sad);
		/* do stuff with sad */
		drm_eld_sad_set(eld, i, &sad);
	}

struct cea_sad provides an easier way to manipulate CTA SADs.

** UNTESTED ***

Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>

[1] https://patchwork.freedesktop.org/patch/msgid/20230821160004.2821445-4-mitulkumar.ajitkumar.golani@intel.com


Jani Nikula (6):
  drm/edid: split out drm_eld.h from drm_edid.h
  drm/eld: replace uint8_t with u8
  drm/edid: include drm_eld.h only where required
  drm/edid: use a temp variable for sads to drop one level of
    dereferences
  drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
  drm/eld: add helpers to modify the SADs of an ELD

 Documentation/gpu/drm-kms-helpers.rst         |   6 +
 drivers/gpu/drm/Makefile                      |   1 +
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   1 +
 drivers/gpu/drm/drm_edid.c                    |  42 +++--
 drivers/gpu/drm/drm_eld.c                     |  55 ++++++
 drivers/gpu/drm/drm_internal.h                |   6 +
 drivers/gpu/drm/i915/display/intel_audio.c    |   1 +
 .../drm/i915/display/intel_crtc_state_dump.c  |   1 +
 drivers/gpu/drm/i915/display/intel_sdvo.c     |   1 +
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |   1 +
 drivers/gpu/drm/radeon/radeon_audio.c         |   1 +
 drivers/gpu/drm/tegra/hdmi.c                  |   1 +
 drivers/gpu/drm/tegra/sor.c                   |   1 +
 include/drm/drm_edid.h                        | 148 ----------------
 include/drm/drm_eld.h                         | 164 ++++++++++++++++++
 sound/core/pcm_drm_eld.c                      |   1 +
 sound/soc/codecs/hdac_hdmi.c                  |   1 +
 sound/soc/codecs/hdmi-codec.c                 |   1 +
 sound/x86/intel_hdmi_audio.c                  |   1 +
 19 files changed, 274 insertions(+), 160 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_eld.c
 create mode 100644 include/drm/drm_eld.h

-- 
2.39.2


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Intel-gfx] [PATCH 1/6] drm/edid: split out drm_eld.h from drm_edid.h
  2023-09-07  9:28 [Intel-gfx] [PATCH 0/6] drm/edid: split out drm_eld.[ch], add some SAD helpers Jani Nikula
@ 2023-09-07  9:28 ` Jani Nikula
  2023-09-25 15:52   ` Golani, Mitulkumar Ajitkumar
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 2/6] drm/eld: replace uint8_t with u8 Jani Nikula
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Jani Nikula @ 2023-09-07  9:28 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

The drm_edid.[ch] files are starting to be a bit crowded, and with plans
to add more ELD related functionality, it's perhaps cleanest to split
the ELD code out to a header of its own.

Include drm_eld.h from drm_edid.h for starters, and leave it to
follow-up work to only include drm_eld.h where needed.

Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 Documentation/gpu/drm-kms-helpers.rst |   3 +
 include/drm/drm_edid.h                | 149 +-----------------------
 include/drm/drm_eld.h                 | 159 ++++++++++++++++++++++++++
 3 files changed, 163 insertions(+), 148 deletions(-)
 create mode 100644 include/drm/drm_eld.h

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index b8ab05e42dbb..f0f93aa62545 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -363,6 +363,9 @@ EDID Helper Functions Reference
 .. kernel-doc:: drivers/gpu/drm/drm_edid.c
    :export:
 
+.. kernel-doc:: include/drm/drm_eld.h
+   :internal:
+
 SCDC Helper Functions Reference
 ===============================
 
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 882d2638708e..1ff52f57ab9c 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -25,6 +25,7 @@
 
 #include <linux/types.h>
 #include <linux/hdmi.h>
+#include <drm/drm_eld.h> /* FIXME: remove this, include directly where needed */
 #include <drm/drm_mode.h>
 
 struct drm_device;
@@ -269,64 +270,6 @@ struct detailed_timing {
 #define DRM_EDID_DSC_MAX_SLICES			0xf
 #define DRM_EDID_DSC_TOTAL_CHUNK_KBYTES		0x3f
 
-/* ELD Header Block */
-#define DRM_ELD_HEADER_BLOCK_SIZE	4
-
-#define DRM_ELD_VER			0
-# define DRM_ELD_VER_SHIFT		3
-# define DRM_ELD_VER_MASK		(0x1f << 3)
-# define DRM_ELD_VER_CEA861D		(2 << 3) /* supports 861D or below */
-# define DRM_ELD_VER_CANNED		(0x1f << 3)
-
-#define DRM_ELD_BASELINE_ELD_LEN	2	/* in dwords! */
-
-/* ELD Baseline Block for ELD_Ver == 2 */
-#define DRM_ELD_CEA_EDID_VER_MNL	4
-# define DRM_ELD_CEA_EDID_VER_SHIFT	5
-# define DRM_ELD_CEA_EDID_VER_MASK	(7 << 5)
-# define DRM_ELD_CEA_EDID_VER_NONE	(0 << 5)
-# define DRM_ELD_CEA_EDID_VER_CEA861	(1 << 5)
-# define DRM_ELD_CEA_EDID_VER_CEA861A	(2 << 5)
-# define DRM_ELD_CEA_EDID_VER_CEA861BCD	(3 << 5)
-# define DRM_ELD_MNL_SHIFT		0
-# define DRM_ELD_MNL_MASK		(0x1f << 0)
-
-#define DRM_ELD_SAD_COUNT_CONN_TYPE	5
-# define DRM_ELD_SAD_COUNT_SHIFT	4
-# define DRM_ELD_SAD_COUNT_MASK		(0xf << 4)
-# define DRM_ELD_CONN_TYPE_SHIFT	2
-# define DRM_ELD_CONN_TYPE_MASK		(3 << 2)
-# define DRM_ELD_CONN_TYPE_HDMI		(0 << 2)
-# define DRM_ELD_CONN_TYPE_DP		(1 << 2)
-# define DRM_ELD_SUPPORTS_AI		(1 << 1)
-# define DRM_ELD_SUPPORTS_HDCP		(1 << 0)
-
-#define DRM_ELD_AUD_SYNCH_DELAY		6	/* in units of 2 ms */
-# define DRM_ELD_AUD_SYNCH_DELAY_MAX	0xfa	/* 500 ms */
-
-#define DRM_ELD_SPEAKER			7
-# define DRM_ELD_SPEAKER_MASK		0x7f
-# define DRM_ELD_SPEAKER_RLRC		(1 << 6)
-# define DRM_ELD_SPEAKER_FLRC		(1 << 5)
-# define DRM_ELD_SPEAKER_RC		(1 << 4)
-# define DRM_ELD_SPEAKER_RLR		(1 << 3)
-# define DRM_ELD_SPEAKER_FC		(1 << 2)
-# define DRM_ELD_SPEAKER_LFE		(1 << 1)
-# define DRM_ELD_SPEAKER_FLR		(1 << 0)
-
-#define DRM_ELD_PORT_ID			8	/* offsets 8..15 inclusive */
-# define DRM_ELD_PORT_ID_LEN		8
-
-#define DRM_ELD_MANUFACTURER_NAME0	16
-#define DRM_ELD_MANUFACTURER_NAME1	17
-
-#define DRM_ELD_PRODUCT_CODE0		18
-#define DRM_ELD_PRODUCT_CODE1		19
-
-#define DRM_ELD_MONITOR_NAME_STRING	20	/* offsets 20..(20+mnl-1) inclusive */
-
-#define DRM_ELD_CEA_SAD(mnl, sad)	(20 + (mnl) + 3 * (sad))
-
 struct edid {
 	u8 header[8];
 	/* Vendor & product info */
@@ -409,96 +352,6 @@ drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
 				   const struct drm_display_mode *mode,
 				   enum hdmi_quantization_range rgb_quant_range);
 
-/**
- * drm_eld_mnl - Get ELD monitor name length in bytes.
- * @eld: pointer to an eld memory structure with mnl set
- */
-static inline int drm_eld_mnl(const uint8_t *eld)
-{
-	return (eld[DRM_ELD_CEA_EDID_VER_MNL] & DRM_ELD_MNL_MASK) >> DRM_ELD_MNL_SHIFT;
-}
-
-/**
- * drm_eld_sad - Get ELD SAD structures.
- * @eld: pointer to an eld memory structure with sad_count set
- */
-static inline const uint8_t *drm_eld_sad(const uint8_t *eld)
-{
-	unsigned int ver, mnl;
-
-	ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >> DRM_ELD_VER_SHIFT;
-	if (ver != 2 && ver != 31)
-		return NULL;
-
-	mnl = drm_eld_mnl(eld);
-	if (mnl > 16)
-		return NULL;
-
-	return eld + DRM_ELD_CEA_SAD(mnl, 0);
-}
-
-/**
- * drm_eld_sad_count - Get ELD SAD count.
- * @eld: pointer to an eld memory structure with sad_count set
- */
-static inline int drm_eld_sad_count(const uint8_t *eld)
-{
-	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_SAD_COUNT_MASK) >>
-		DRM_ELD_SAD_COUNT_SHIFT;
-}
-
-/**
- * drm_eld_calc_baseline_block_size - Calculate baseline block size in bytes
- * @eld: pointer to an eld memory structure with mnl and sad_count set
- *
- * This is a helper for determining the payload size of the baseline block, in
- * bytes, for e.g. setting the Baseline_ELD_Len field in the ELD header block.
- */
-static inline int drm_eld_calc_baseline_block_size(const uint8_t *eld)
-{
-	return DRM_ELD_MONITOR_NAME_STRING - DRM_ELD_HEADER_BLOCK_SIZE +
-		drm_eld_mnl(eld) + drm_eld_sad_count(eld) * 3;
-}
-
-/**
- * drm_eld_size - Get ELD size in bytes
- * @eld: pointer to a complete eld memory structure
- *
- * The returned value does not include the vendor block. It's vendor specific,
- * and comprises of the remaining bytes in the ELD memory buffer after
- * drm_eld_size() bytes of header and baseline block.
- *
- * The returned value is guaranteed to be a multiple of 4.
- */
-static inline int drm_eld_size(const uint8_t *eld)
-{
-	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
-}
-
-/**
- * drm_eld_get_spk_alloc - Get speaker allocation
- * @eld: pointer to an ELD memory structure
- *
- * The returned value is the speakers mask. User has to use %DRM_ELD_SPEAKER
- * field definitions to identify speakers.
- */
-static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
-{
-	return eld[DRM_ELD_SPEAKER] & DRM_ELD_SPEAKER_MASK;
-}
-
-/**
- * drm_eld_get_conn_type - Get device type hdmi/dp connected
- * @eld: pointer to an ELD memory structure
- *
- * The caller need to use %DRM_ELD_CONN_TYPE_HDMI or %DRM_ELD_CONN_TYPE_DP to
- * identify the display type connected.
- */
-static inline u8 drm_eld_get_conn_type(const uint8_t *eld)
-{
-	return eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK;
-}
-
 /**
  * drm_edid_decode_mfg_id - Decode the manufacturer ID
  * @mfg_id: The manufacturer ID
diff --git a/include/drm/drm_eld.h b/include/drm/drm_eld.h
new file mode 100644
index 000000000000..9bde89bd96ea
--- /dev/null
+++ b/include/drm/drm_eld.h
@@ -0,0 +1,159 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#ifndef __DRM_ELD_H__
+#define __DRM_ELD_H__
+
+#include <linux/types.h>
+
+/* ELD Header Block */
+#define DRM_ELD_HEADER_BLOCK_SIZE	4
+
+#define DRM_ELD_VER			0
+# define DRM_ELD_VER_SHIFT		3
+# define DRM_ELD_VER_MASK		(0x1f << 3)
+# define DRM_ELD_VER_CEA861D		(2 << 3) /* supports 861D or below */
+# define DRM_ELD_VER_CANNED		(0x1f << 3)
+
+#define DRM_ELD_BASELINE_ELD_LEN	2	/* in dwords! */
+
+/* ELD Baseline Block for ELD_Ver == 2 */
+#define DRM_ELD_CEA_EDID_VER_MNL	4
+# define DRM_ELD_CEA_EDID_VER_SHIFT	5
+# define DRM_ELD_CEA_EDID_VER_MASK	(7 << 5)
+# define DRM_ELD_CEA_EDID_VER_NONE	(0 << 5)
+# define DRM_ELD_CEA_EDID_VER_CEA861	(1 << 5)
+# define DRM_ELD_CEA_EDID_VER_CEA861A	(2 << 5)
+# define DRM_ELD_CEA_EDID_VER_CEA861BCD	(3 << 5)
+# define DRM_ELD_MNL_SHIFT		0
+# define DRM_ELD_MNL_MASK		(0x1f << 0)
+
+#define DRM_ELD_SAD_COUNT_CONN_TYPE	5
+# define DRM_ELD_SAD_COUNT_SHIFT	4
+# define DRM_ELD_SAD_COUNT_MASK		(0xf << 4)
+# define DRM_ELD_CONN_TYPE_SHIFT	2
+# define DRM_ELD_CONN_TYPE_MASK		(3 << 2)
+# define DRM_ELD_CONN_TYPE_HDMI		(0 << 2)
+# define DRM_ELD_CONN_TYPE_DP		(1 << 2)
+# define DRM_ELD_SUPPORTS_AI		(1 << 1)
+# define DRM_ELD_SUPPORTS_HDCP		(1 << 0)
+
+#define DRM_ELD_AUD_SYNCH_DELAY		6	/* in units of 2 ms */
+# define DRM_ELD_AUD_SYNCH_DELAY_MAX	0xfa	/* 500 ms */
+
+#define DRM_ELD_SPEAKER			7
+# define DRM_ELD_SPEAKER_MASK		0x7f
+# define DRM_ELD_SPEAKER_RLRC		(1 << 6)
+# define DRM_ELD_SPEAKER_FLRC		(1 << 5)
+# define DRM_ELD_SPEAKER_RC		(1 << 4)
+# define DRM_ELD_SPEAKER_RLR		(1 << 3)
+# define DRM_ELD_SPEAKER_FC		(1 << 2)
+# define DRM_ELD_SPEAKER_LFE		(1 << 1)
+# define DRM_ELD_SPEAKER_FLR		(1 << 0)
+
+#define DRM_ELD_PORT_ID			8	/* offsets 8..15 inclusive */
+# define DRM_ELD_PORT_ID_LEN		8
+
+#define DRM_ELD_MANUFACTURER_NAME0	16
+#define DRM_ELD_MANUFACTURER_NAME1	17
+
+#define DRM_ELD_PRODUCT_CODE0		18
+#define DRM_ELD_PRODUCT_CODE1		19
+
+#define DRM_ELD_MONITOR_NAME_STRING	20	/* offsets 20..(20+mnl-1) inclusive */
+
+#define DRM_ELD_CEA_SAD(mnl, sad)	(20 + (mnl) + 3 * (sad))
+
+/**
+ * drm_eld_mnl - Get ELD monitor name length in bytes.
+ * @eld: pointer to an eld memory structure with mnl set
+ */
+static inline int drm_eld_mnl(const uint8_t *eld)
+{
+	return (eld[DRM_ELD_CEA_EDID_VER_MNL] & DRM_ELD_MNL_MASK) >> DRM_ELD_MNL_SHIFT;
+}
+
+/**
+ * drm_eld_sad - Get ELD SAD structures.
+ * @eld: pointer to an eld memory structure with sad_count set
+ */
+static inline const uint8_t *drm_eld_sad(const uint8_t *eld)
+{
+	unsigned int ver, mnl;
+
+	ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >> DRM_ELD_VER_SHIFT;
+	if (ver != 2 && ver != 31)
+		return NULL;
+
+	mnl = drm_eld_mnl(eld);
+	if (mnl > 16)
+		return NULL;
+
+	return eld + DRM_ELD_CEA_SAD(mnl, 0);
+}
+
+/**
+ * drm_eld_sad_count - Get ELD SAD count.
+ * @eld: pointer to an eld memory structure with sad_count set
+ */
+static inline int drm_eld_sad_count(const uint8_t *eld)
+{
+	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_SAD_COUNT_MASK) >>
+		DRM_ELD_SAD_COUNT_SHIFT;
+}
+
+/**
+ * drm_eld_calc_baseline_block_size - Calculate baseline block size in bytes
+ * @eld: pointer to an eld memory structure with mnl and sad_count set
+ *
+ * This is a helper for determining the payload size of the baseline block, in
+ * bytes, for e.g. setting the Baseline_ELD_Len field in the ELD header block.
+ */
+static inline int drm_eld_calc_baseline_block_size(const uint8_t *eld)
+{
+	return DRM_ELD_MONITOR_NAME_STRING - DRM_ELD_HEADER_BLOCK_SIZE +
+		drm_eld_mnl(eld) + drm_eld_sad_count(eld) * 3;
+}
+
+/**
+ * drm_eld_size - Get ELD size in bytes
+ * @eld: pointer to a complete eld memory structure
+ *
+ * The returned value does not include the vendor block. It's vendor specific,
+ * and comprises of the remaining bytes in the ELD memory buffer after
+ * drm_eld_size() bytes of header and baseline block.
+ *
+ * The returned value is guaranteed to be a multiple of 4.
+ */
+static inline int drm_eld_size(const uint8_t *eld)
+{
+	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
+}
+
+/**
+ * drm_eld_get_spk_alloc - Get speaker allocation
+ * @eld: pointer to an ELD memory structure
+ *
+ * The returned value is the speakers mask. User has to use %DRM_ELD_SPEAKER
+ * field definitions to identify speakers.
+ */
+static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
+{
+	return eld[DRM_ELD_SPEAKER] & DRM_ELD_SPEAKER_MASK;
+}
+
+/**
+ * drm_eld_get_conn_type - Get device type hdmi/dp connected
+ * @eld: pointer to an ELD memory structure
+ *
+ * The caller need to use %DRM_ELD_CONN_TYPE_HDMI or %DRM_ELD_CONN_TYPE_DP to
+ * identify the display type connected.
+ */
+static inline u8 drm_eld_get_conn_type(const uint8_t *eld)
+{
+	return eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK;
+}
+
+#endif /* __DRM_ELD_H__ */
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Intel-gfx] [PATCH 2/6] drm/eld: replace uint8_t with u8
  2023-09-07  9:28 [Intel-gfx] [PATCH 0/6] drm/edid: split out drm_eld.[ch], add some SAD helpers Jani Nikula
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 1/6] drm/edid: split out drm_eld.h from drm_edid.h Jani Nikula
@ 2023-09-07  9:28 ` Jani Nikula
  2023-09-08  5:25   ` Borah, Chaitanya Kumar
  2023-09-25 16:32   ` Golani, Mitulkumar Ajitkumar
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 3/6] drm/edid: include drm_eld.h only where required Jani Nikula
                   ` (6 subsequent siblings)
  8 siblings, 2 replies; 21+ messages in thread
From: Jani Nikula @ 2023-09-07  9:28 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Unify on kernel types.

Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 include/drm/drm_eld.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/drm/drm_eld.h b/include/drm/drm_eld.h
index 9bde89bd96ea..7b674256b9aa 100644
--- a/include/drm/drm_eld.h
+++ b/include/drm/drm_eld.h
@@ -70,7 +70,7 @@
  * drm_eld_mnl - Get ELD monitor name length in bytes.
  * @eld: pointer to an eld memory structure with mnl set
  */
-static inline int drm_eld_mnl(const uint8_t *eld)
+static inline int drm_eld_mnl(const u8 *eld)
 {
 	return (eld[DRM_ELD_CEA_EDID_VER_MNL] & DRM_ELD_MNL_MASK) >> DRM_ELD_MNL_SHIFT;
 }
@@ -79,7 +79,7 @@ static inline int drm_eld_mnl(const uint8_t *eld)
  * drm_eld_sad - Get ELD SAD structures.
  * @eld: pointer to an eld memory structure with sad_count set
  */
-static inline const uint8_t *drm_eld_sad(const uint8_t *eld)
+static inline const u8 *drm_eld_sad(const u8 *eld)
 {
 	unsigned int ver, mnl;
 
@@ -98,7 +98,7 @@ static inline const uint8_t *drm_eld_sad(const uint8_t *eld)
  * drm_eld_sad_count - Get ELD SAD count.
  * @eld: pointer to an eld memory structure with sad_count set
  */
-static inline int drm_eld_sad_count(const uint8_t *eld)
+static inline int drm_eld_sad_count(const u8 *eld)
 {
 	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_SAD_COUNT_MASK) >>
 		DRM_ELD_SAD_COUNT_SHIFT;
@@ -111,7 +111,7 @@ static inline int drm_eld_sad_count(const uint8_t *eld)
  * This is a helper for determining the payload size of the baseline block, in
  * bytes, for e.g. setting the Baseline_ELD_Len field in the ELD header block.
  */
-static inline int drm_eld_calc_baseline_block_size(const uint8_t *eld)
+static inline int drm_eld_calc_baseline_block_size(const u8 *eld)
 {
 	return DRM_ELD_MONITOR_NAME_STRING - DRM_ELD_HEADER_BLOCK_SIZE +
 		drm_eld_mnl(eld) + drm_eld_sad_count(eld) * 3;
@@ -127,7 +127,7 @@ static inline int drm_eld_calc_baseline_block_size(const uint8_t *eld)
  *
  * The returned value is guaranteed to be a multiple of 4.
  */
-static inline int drm_eld_size(const uint8_t *eld)
+static inline int drm_eld_size(const u8 *eld)
 {
 	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
 }
@@ -139,7 +139,7 @@ static inline int drm_eld_size(const uint8_t *eld)
  * The returned value is the speakers mask. User has to use %DRM_ELD_SPEAKER
  * field definitions to identify speakers.
  */
-static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
+static inline u8 drm_eld_get_spk_alloc(const u8 *eld)
 {
 	return eld[DRM_ELD_SPEAKER] & DRM_ELD_SPEAKER_MASK;
 }
@@ -151,7 +151,7 @@ static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
  * The caller need to use %DRM_ELD_CONN_TYPE_HDMI or %DRM_ELD_CONN_TYPE_DP to
  * identify the display type connected.
  */
-static inline u8 drm_eld_get_conn_type(const uint8_t *eld)
+static inline u8 drm_eld_get_conn_type(const u8 *eld)
 {
 	return eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK;
 }
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Intel-gfx] [PATCH 3/6] drm/edid: include drm_eld.h only where required
  2023-09-07  9:28 [Intel-gfx] [PATCH 0/6] drm/edid: split out drm_eld.[ch], add some SAD helpers Jani Nikula
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 1/6] drm/edid: split out drm_eld.h from drm_edid.h Jani Nikula
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 2/6] drm/eld: replace uint8_t with u8 Jani Nikula
@ 2023-09-07  9:28 ` Jani Nikula
  2023-09-25 16:59   ` Golani, Mitulkumar Ajitkumar
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 4/6] drm/edid: use a temp variable for sads to drop one level of dereferences Jani Nikula
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Jani Nikula @ 2023-09-07  9:28 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Reduce the dependencies on drm_eld.h. Some files might be able to drop
the dependency on drm_edid.h too with the direct inclusion of drm_eld.h.

Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c    | 1 +
 drivers/gpu/drm/drm_edid.c                           | 1 +
 drivers/gpu/drm/i915/display/intel_audio.c           | 1 +
 drivers/gpu/drm/i915/display/intel_crtc_state_dump.c | 1 +
 drivers/gpu/drm/i915/display/intel_sdvo.c            | 1 +
 drivers/gpu/drm/nouveau/dispnv50/disp.c              | 1 +
 drivers/gpu/drm/radeon/radeon_audio.c                | 1 +
 drivers/gpu/drm/tegra/hdmi.c                         | 1 +
 drivers/gpu/drm/tegra/sor.c                          | 1 +
 include/drm/drm_edid.h                               | 1 -
 sound/core/pcm_drm_eld.c                             | 1 +
 sound/soc/codecs/hdac_hdmi.c                         | 1 +
 sound/soc/codecs/hdmi-codec.c                        | 1 +
 sound/x86/intel_hdmi_audio.c                         | 1 +
 14 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 268cb99a4c4b..fe7e307ae7f9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -86,6 +86,7 @@
 #include <drm/drm_blend.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_eld.h>
 #include <drm/drm_vblank.h>
 #include <drm/drm_audio_component.h>
 #include <drm/drm_gem_atomic_helper.h>
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 39dd3f694544..2025970816c9 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -41,6 +41,7 @@
 #include <drm/drm_displayid.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_eld.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_print.h>
 
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
index 19605264a35c..39f5b698e08a 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -25,6 +25,7 @@
 #include <linux/kernel.h>
 
 #include <drm/drm_edid.h>
+#include <drm/drm_eld.h>
 #include <drm/i915_component.h>
 
 #include "i915_drv.h"
diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
index 8d4640d0fd34..fcddd6d81768 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
@@ -4,6 +4,7 @@
  */
 
 #include <drm/drm_edid.h>
+#include <drm/drm_eld.h>
 
 #include "i915_drv.h"
 #include "intel_crtc_state_dump.h"
diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
index 135a2527fd1b..6abae283998e 100644
--- a/drivers/gpu/drm/i915/display/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
@@ -35,6 +35,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_eld.h>
 
 #include "i915_drv.h"
 #include "i915_reg.h"
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 4e7c9c353c51..9332aa633867 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -38,6 +38,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_eld.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
diff --git a/drivers/gpu/drm/radeon/radeon_audio.c b/drivers/gpu/drm/radeon/radeon_audio.c
index d6ccaf24ee0c..279bf130a18c 100644
--- a/drivers/gpu/drm/radeon/radeon_audio.c
+++ b/drivers/gpu/drm/radeon/radeon_audio.c
@@ -26,6 +26,7 @@
 #include <linux/component.h>
 
 #include <drm/drm_crtc.h>
+#include <drm/drm_eld.h>
 #include "dce6_afmt.h"
 #include "evergreen_hdmi.h"
 #include "radeon.h"
diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
index 0ba3ca3ac509..a1fcee665023 100644
--- a/drivers/gpu/drm/tegra/hdmi.c
+++ b/drivers/gpu/drm/tegra/hdmi.c
@@ -24,6 +24,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_debugfs.h>
+#include <drm/drm_eld.h>
 #include <drm/drm_file.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_probe_helper.h>
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index d5a3d3f4fece..83341576630d 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -20,6 +20,7 @@
 #include <drm/display/drm_scdc_helper.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_debugfs.h>
+#include <drm/drm_eld.h>
 #include <drm/drm_file.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_simple_kms_helper.h>
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 1ff52f57ab9c..e98aa6818700 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -25,7 +25,6 @@
 
 #include <linux/types.h>
 #include <linux/hdmi.h>
-#include <drm/drm_eld.h> /* FIXME: remove this, include directly where needed */
 #include <drm/drm_mode.h>
 
 struct drm_device;
diff --git a/sound/core/pcm_drm_eld.c b/sound/core/pcm_drm_eld.c
index 07075071972d..1cdca4d4fc9c 100644
--- a/sound/core/pcm_drm_eld.c
+++ b/sound/core/pcm_drm_eld.c
@@ -6,6 +6,7 @@
 #include <linux/export.h>
 #include <linux/hdmi.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_eld.h>
 #include <sound/pcm.h>
 #include <sound/pcm_drm_eld.h>
 
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 8b6b76029694..d1b53fc1efb6 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -16,6 +16,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/hdmi.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_eld.h>
 #include <sound/pcm_params.h>
 #include <sound/jack.h>
 #include <sound/soc.h>
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index d21f69f05342..9b01d060f7cc 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -17,6 +17,7 @@
 #include <sound/pcm_iec958.h>
 
 #include <drm/drm_crtc.h> /* This is only to get MAX_ELD_BYTES */
+#include <drm/drm_eld.h>
 
 #define HDMI_CODEC_CHMAP_IDX_UNKNOWN  -1
 
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c
index ab95fb34a635..02f5a7f9b728 100644
--- a/sound/x86/intel_hdmi_audio.c
+++ b/sound/x86/intel_hdmi_audio.c
@@ -30,6 +30,7 @@
 #include <sound/control.h>
 #include <sound/jack.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_eld.h>
 #include <drm/intel_lpe_audio.h>
 #include "intel_hdmi_audio.h"
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Intel-gfx] [PATCH 4/6] drm/edid: use a temp variable for sads to drop one level of dereferences
  2023-09-07  9:28 [Intel-gfx] [PATCH 0/6] drm/edid: split out drm_eld.[ch], add some SAD helpers Jani Nikula
                   ` (2 preceding siblings ...)
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 3/6] drm/edid: include drm_eld.h only where required Jani Nikula
@ 2023-09-07  9:28 ` Jani Nikula
  2023-09-25 17:46   ` Golani, Mitulkumar Ajitkumar
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad Jani Nikula
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Jani Nikula @ 2023-09-07  9:28 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

It's arguably easier on the eyes, and drops a set of parenthesis too.

Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 2025970816c9..fcdc2c314cde 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5583,7 +5583,7 @@ static void drm_edid_to_eld(struct drm_connector *connector,
 }
 
 static int _drm_edid_to_sad(const struct drm_edid *drm_edid,
-			    struct cea_sad **sads)
+			    struct cea_sad **psads)
 {
 	const struct cea_db *db;
 	struct cea_db_iter iter;
@@ -5592,19 +5592,21 @@ static int _drm_edid_to_sad(const struct drm_edid *drm_edid,
 	cea_db_iter_edid_begin(drm_edid, &iter);
 	cea_db_iter_for_each(db, &iter) {
 		if (cea_db_tag(db) == CTA_DB_AUDIO) {
+			struct cea_sad *sads;
 			int j;
 
 			count = cea_db_payload_len(db) / 3; /* SAD is 3B */
-			*sads = kcalloc(count, sizeof(**sads), GFP_KERNEL);
-			if (!*sads)
+			sads = kcalloc(count, sizeof(*sads), GFP_KERNEL);
+			*psads = sads;
+			if (!sads)
 				return -ENOMEM;
 			for (j = 0; j < count; j++) {
 				const u8 *sad = &db->data[j * 3];
 
-				(*sads)[j].format = (sad[0] & 0x78) >> 3;
-				(*sads)[j].channels = sad[0] & 0x7;
-				(*sads)[j].freq = sad[1] & 0x7F;
-				(*sads)[j].byte2 = sad[2];
+				sads[j].format = (sad[0] & 0x78) >> 3;
+				sads[j].channels = sad[0] & 0x7;
+				sads[j].freq = sad[1] & 0x7F;
+				sads[j].byte2 = sad[2];
 			}
 			break;
 		}
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Intel-gfx] [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
  2023-09-07  9:28 [Intel-gfx] [PATCH 0/6] drm/edid: split out drm_eld.[ch], add some SAD helpers Jani Nikula
                   ` (3 preceding siblings ...)
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 4/6] drm/edid: use a temp variable for sads to drop one level of dereferences Jani Nikula
@ 2023-09-07  9:28 ` Jani Nikula
  2023-09-07 11:49   ` kernel test robot
                     ` (2 more replies)
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 6/6] drm/eld: add helpers to modify the SADs of an ELD Jani Nikula
                   ` (3 subsequent siblings)
  8 siblings, 3 replies; 21+ messages in thread
From: Jani Nikula @ 2023-09-07  9:28 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Add helpers to pack/unpack SADs. Both ways and non-static, as follow-up
work needs them.

Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c     | 33 ++++++++++++++++++++++++---------
 drivers/gpu/drm/drm_internal.h |  6 ++++++
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index fcdc2c314cde..1260e2688bd7 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5499,6 +5499,27 @@ static void clear_eld(struct drm_connector *connector)
 	connector->audio_latency[1] = 0;
 }
 
+/*
+ * Get 3-byte SAD buf from struct cea_sad.
+ */
+void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad)
+{
+	sad[0] = cta_sad->format << 3 | cta_sad->channels;
+	sad[1] = cta_sad->freq;
+	sad[2] = cta_sad->byte2;
+}
+
+/*
+ * Set struct cea_sad from 3-byte SAD buf.
+ */
+void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad)
+{
+	cta_sad->format = (sad[0] & 0x78) >> 3;
+	cta_sad->channels = sad[0] & 0x07;
+	cta_sad->freq = sad[1] & 0x7f;
+	cta_sad->byte2 = sad[2];
+}
+
 /*
  * drm_edid_to_eld - build ELD from EDID
  * @connector: connector corresponding to the HDMI/DP sink
@@ -5593,21 +5614,15 @@ static int _drm_edid_to_sad(const struct drm_edid *drm_edid,
 	cea_db_iter_for_each(db, &iter) {
 		if (cea_db_tag(db) == CTA_DB_AUDIO) {
 			struct cea_sad *sads;
-			int j;
+			int i;
 
 			count = cea_db_payload_len(db) / 3; /* SAD is 3B */
 			sads = kcalloc(count, sizeof(*sads), GFP_KERNEL);
 			*psads = sads;
 			if (!sads)
 				return -ENOMEM;
-			for (j = 0; j < count; j++) {
-				const u8 *sad = &db->data[j * 3];
-
-				sads[j].format = (sad[0] & 0x78) >> 3;
-				sads[j].channels = sad[0] & 0x7;
-				sads[j].freq = sad[1] & 0x7F;
-				sads[j].byte2 = sad[2];
-			}
+			for (i = 0; i < count; i++)
+				drm_edid_cta_sad_set(&sads[i], &db->data[i * 3]);
 			break;
 		}
 	}
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index ab9a472f4a47..5b7c661da459 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -22,6 +22,7 @@
  */
 
 #include <linux/kthread.h>
+#include <linux/types.h>
 
 #include <drm/drm_ioctl.h>
 #include <drm/drm_vblank.h>
@@ -31,6 +32,7 @@
 
 #define DRM_IF_VERSION(maj, min) (maj << 16 | min)
 
+struct cea_sad;
 struct dentry;
 struct dma_buf;
 struct iosys_map;
@@ -265,3 +267,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
 void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
 				const struct drm_framebuffer *fb);
 void drm_framebuffer_debugfs_init(struct drm_device *dev);
+
+/* drm_edid.c */
+void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad);
+void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad);
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Intel-gfx] [PATCH 6/6] drm/eld: add helpers to modify the SADs of an ELD
  2023-09-07  9:28 [Intel-gfx] [PATCH 0/6] drm/edid: split out drm_eld.[ch], add some SAD helpers Jani Nikula
                   ` (4 preceding siblings ...)
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad Jani Nikula
@ 2023-09-07  9:28 ` Jani Nikula
  2023-09-26 18:04   ` Golani, Mitulkumar Ajitkumar
  2023-09-07 12:14 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/edid: split out drm_eld.[ch], add some SAD helpers Patchwork
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Jani Nikula @ 2023-09-07  9:28 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Occasionally it's necessary for drivers to modify the SADs of an ELD,
but it's not so cool to have drivers poke at the ELD buffer directly.

Using the helpers to translate between 3-byte SAD and struct cea_sad,
add ELD helpers to get/set the SADs from/to an ELD.

Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 Documentation/gpu/drm-kms-helpers.rst |  3 ++
 drivers/gpu/drm/Makefile              |  1 +
 drivers/gpu/drm/drm_eld.c             | 55 +++++++++++++++++++++++++++
 include/drm/drm_eld.h                 |  5 +++
 4 files changed, 64 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_eld.c

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index f0f93aa62545..df91b7cd992e 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -366,6 +366,9 @@ EDID Helper Functions Reference
 .. kernel-doc:: include/drm/drm_eld.h
    :internal:
 
+.. kernel-doc:: drivers/gpu/drm/drm_eld.c
+   :export:
+
 SCDC Helper Functions Reference
 ===============================
 
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 215e78e79125..632e74d823e8 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -22,6 +22,7 @@ drm-y := \
 	drm_drv.o \
 	drm_dumb_buffers.o \
 	drm_edid.o \
+	drm_eld.o \
 	drm_encoder.o \
 	drm_file.o \
 	drm_fourcc.o \
diff --git a/drivers/gpu/drm/drm_eld.c b/drivers/gpu/drm/drm_eld.c
new file mode 100644
index 000000000000..34e0d71c3550
--- /dev/null
+++ b/drivers/gpu/drm/drm_eld.c
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#include <drm/drm_edid.h>
+#include <drm/drm_eld.h>
+
+#include "drm_internal.h"
+
+/**
+ * drm_eld_sad_get - get SAD from ELD to struct cea_sad
+ * @eld: ELD buffer
+ * @i: SAD number
+ * @cta_sad: destination struct cea_sad
+ *
+ * @return: 0 on success, or negative on errors
+ */
+int drm_eld_sad_get(const u8 *eld, int i, struct cea_sad *cta_sad)
+{
+	const u8 *sad;
+
+	if (i >= drm_eld_sad_count(eld))
+		return -EINVAL;
+
+	sad = eld + DRM_ELD_CEA_SAD(drm_eld_mnl(eld), i);
+
+	drm_edid_cta_sad_set(cta_sad, sad);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_eld_sad_get);
+
+/**
+ * drm_eld_sad_set - set SAD to ELD from struct cea_sad
+ * @eld: ELD buffer
+ * @i: SAD number
+ * @cta_sad: source struct cea_sad
+ *
+ * @return: 0 on success, or negative on errors
+ */
+int drm_eld_sad_set(u8 *eld, int i, const struct cea_sad *cta_sad)
+{
+	u8 *sad;
+
+	if (i >= drm_eld_sad_count(eld))
+		return -EINVAL;
+
+	sad = eld + DRM_ELD_CEA_SAD(drm_eld_mnl(eld), i);
+
+	drm_edid_cta_sad_get(cta_sad, sad);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_eld_sad_set);
diff --git a/include/drm/drm_eld.h b/include/drm/drm_eld.h
index 7b674256b9aa..5b320157684c 100644
--- a/include/drm/drm_eld.h
+++ b/include/drm/drm_eld.h
@@ -8,6 +8,8 @@
 
 #include <linux/types.h>
 
+struct cea_sad;
+
 /* ELD Header Block */
 #define DRM_ELD_HEADER_BLOCK_SIZE	4
 
@@ -75,6 +77,9 @@ static inline int drm_eld_mnl(const u8 *eld)
 	return (eld[DRM_ELD_CEA_EDID_VER_MNL] & DRM_ELD_MNL_MASK) >> DRM_ELD_MNL_SHIFT;
 }
 
+int drm_eld_sad_get(const u8 *eld, int i, struct cea_sad *cta_sad);
+int drm_eld_sad_set(u8 *eld, int i, const struct cea_sad *cta_sad);
+
 /**
  * drm_eld_sad - Get ELD SAD structures.
  * @eld: pointer to an eld memory structure with sad_count set
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [Intel-gfx] [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad Jani Nikula
@ 2023-09-07 11:49   ` kernel test robot
  2023-09-07 13:37   ` kernel test robot
  2023-09-26 18:13   ` Golani, Mitulkumar Ajitkumar
  2 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2023-09-07 11:49 UTC (permalink / raw)
  To: Jani Nikula, dri-devel; +Cc: jani.nikula, intel-gfx, oe-kbuild-all

Hi Jani,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-tip/drm-tip]

url:    https://github.com/intel-lab-lkp/linux/commits/Jani-Nikula/drm-edid-split-out-drm_eld-h-from-drm_edid-h/20230907-173011
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link:    https://lore.kernel.org/r/eba53a0904126fb904a5190750002695350f44eb.1694078430.git.jani.nikula%40intel.com
patch subject: [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
config: arm-defconfig (https://download.01.org/0day-ci/archive/20230907/202309071934.AzntzCVc-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230907/202309071934.AzntzCVc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309071934.AzntzCVc-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/drm_edid.c:5505:6: warning: no previous prototype for 'drm_edid_cta_sad_get' [-Wmissing-prototypes]
    5505 | void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad)
         |      ^~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/drm_edid.c:5515:6: warning: no previous prototype for 'drm_edid_cta_sad_set' [-Wmissing-prototypes]
    5515 | void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad)
         |      ^~~~~~~~~~~~~~~~~~~~


vim +/drm_edid_cta_sad_get +5505 drivers/gpu/drm/drm_edid.c

  5501	
  5502	/*
  5503	 * Get 3-byte SAD buf from struct cea_sad.
  5504	 */
> 5505	void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad)
  5506	{
  5507		sad[0] = cta_sad->format << 3 | cta_sad->channels;
  5508		sad[1] = cta_sad->freq;
  5509		sad[2] = cta_sad->byte2;
  5510	}
  5511	
  5512	/*
  5513	 * Set struct cea_sad from 3-byte SAD buf.
  5514	 */
> 5515	void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad)
  5516	{
  5517		cta_sad->format = (sad[0] & 0x78) >> 3;
  5518		cta_sad->channels = sad[0] & 0x07;
  5519		cta_sad->freq = sad[1] & 0x7f;
  5520		cta_sad->byte2 = sad[2];
  5521	}
  5522	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/edid: split out drm_eld.[ch], add some SAD helpers
  2023-09-07  9:28 [Intel-gfx] [PATCH 0/6] drm/edid: split out drm_eld.[ch], add some SAD helpers Jani Nikula
                   ` (5 preceding siblings ...)
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 6/6] drm/eld: add helpers to modify the SADs of an ELD Jani Nikula
@ 2023-09-07 12:14 ` Patchwork
  2023-09-07 12:14 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
  2023-09-07 12:31 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
  8 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2023-09-07 12:14 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

== Series Details ==

Series: drm/edid: split out drm_eld.[ch], add some SAD helpers
URL   : https://patchwork.freedesktop.org/series/123384/
State : warning

== Summary ==

Error: dim checkpatch failed
940b009582a3 drm/edid: split out drm_eld.h from drm_edid.h
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
-:205: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#205: 
new file mode 100644

total: 0 errors, 1 warnings, 0 checks, 335 lines checked
bfb13c1d78fa drm/eld: replace uint8_t with u8
1458d514001c drm/edid: include drm_eld.h only where required
814b0bb13735 drm/edid: use a temp variable for sads to drop one level of dereferences
078b4dda5ca5 drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
0c4ecf16ece2 drm/eld: add helpers to modify the SADs of an ELD
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
-:42: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#42: 
new file mode 100644

total: 0 errors, 1 warnings, 0 checks, 88 lines checked



^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/edid: split out drm_eld.[ch], add some SAD helpers
  2023-09-07  9:28 [Intel-gfx] [PATCH 0/6] drm/edid: split out drm_eld.[ch], add some SAD helpers Jani Nikula
                   ` (6 preceding siblings ...)
  2023-09-07 12:14 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/edid: split out drm_eld.[ch], add some SAD helpers Patchwork
@ 2023-09-07 12:14 ` Patchwork
  2023-09-07 12:31 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
  8 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2023-09-07 12:14 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

== Series Details ==

Series: drm/edid: split out drm_eld.[ch], add some SAD helpers
URL   : https://patchwork.freedesktop.org/series/123384/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.



^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/edid: split out drm_eld.[ch], add some SAD helpers
  2023-09-07  9:28 [Intel-gfx] [PATCH 0/6] drm/edid: split out drm_eld.[ch], add some SAD helpers Jani Nikula
                   ` (7 preceding siblings ...)
  2023-09-07 12:14 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
@ 2023-09-07 12:31 ` Patchwork
  8 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2023-09-07 12:31 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 4296 bytes --]

== Series Details ==

Series: drm/edid: split out drm_eld.[ch], add some SAD helpers
URL   : https://patchwork.freedesktop.org/series/123384/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13606 -> Patchwork_123384v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_123384v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_123384v1, please notify your bug team (lgci.bug.filing@intel.com) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123384v1/index.html

Participating hosts (39 -> 30)
------------------------------

  Additional (1): fi-hsw-4770 
  Missing    (10): fi-kbl-7567u fi-tgl-1115g4 fi-cfl-guc fi-apl-guc fi-snb-2520m fi-kbl-guc fi-kbl-x1275 bat-dg2-11 fi-bsw-nick bat-mtlp-6 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_123384v1:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_suspend@basic-s0@smem:
    - bat-dg2-9:          [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13606/bat-dg2-9/igt@gem_exec_suspend@basic-s0@smem.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123384v1/bat-dg2-9/igt@gem_exec_suspend@basic-s0@smem.html

  
Known issues
------------

  Here are the changes found in Patchwork_123384v1 that come from known issues:

### CI changes ###

#### Issues hit ####

  * boot:
    - fi-hsw-4770:        NOTRUN -> [FAIL][3] ([i915#8293])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123384v1/fi-hsw-4770/boot.html

  

### IGT changes ###

#### Issues hit ####

  * igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-a-dp-5:
    - bat-adlp-11:        [PASS][4] -> [ABORT][5] ([i915#8668])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13606/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-a-dp-5.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123384v1/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-a-dp-5.html

  * igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-edp-1:
    - bat-rplp-1:         [PASS][6] -> [ABORT][7] ([i915#8442] / [i915#8668])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13606/bat-rplp-1/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-edp-1.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123384v1/bat-rplp-1/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-edp-1.html

  
#### Possible fixes ####

  * igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-b-dp-6:
    - bat-adlp-11:        [ABORT][8] ([i915#8668]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13606/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-b-dp-6.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123384v1/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-b-dp-6.html

  
  [i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293
  [i915#8442]: https://gitlab.freedesktop.org/drm/intel/issues/8442
  [i915#8668]: https://gitlab.freedesktop.org/drm/intel/issues/8668


Build changes
-------------

  * Linux: CI_DRM_13606 -> Patchwork_123384v1

  CI-20190529: 20190529
  CI_DRM_13606: 0723fc45231c46bbdaef60144e5af32323d27538 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7474: 9d91cf2c6e7bb64d60c2030d1535e40ca0ad53ee @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_123384v1: 0723fc45231c46bbdaef60144e5af32323d27538 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

dc6384398098 drm/eld: add helpers to modify the SADs of an ELD
9cbc38dd7951 drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
6db20d05f1cc drm/edid: use a temp variable for sads to drop one level of dereferences
bdb2982e84f2 drm/edid: include drm_eld.h only where required
d499add4e943 drm/eld: replace uint8_t with u8
6e51d25293cf drm/edid: split out drm_eld.h from drm_edid.h

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123384v1/index.html

[-- Attachment #2: Type: text/html, Size: 5189 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-gfx] [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad Jani Nikula
  2023-09-07 11:49   ` kernel test robot
@ 2023-09-07 13:37   ` kernel test robot
  2023-09-26 18:13   ` Golani, Mitulkumar Ajitkumar
  2 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2023-09-07 13:37 UTC (permalink / raw)
  To: Jani Nikula, dri-devel; +Cc: jani.nikula, intel-gfx, oe-kbuild-all

Hi Jani,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-tip/drm-tip]

url:    https://github.com/intel-lab-lkp/linux/commits/Jani-Nikula/drm-edid-split-out-drm_eld-h-from-drm_edid-h/20230907-173011
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link:    https://lore.kernel.org/r/eba53a0904126fb904a5190750002695350f44eb.1694078430.git.jani.nikula%40intel.com
patch subject: [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
config: i386-randconfig-013-20230907 (https://download.01.org/0day-ci/archive/20230907/202309072156.iD0ETpd1-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230907/202309072156.iD0ETpd1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309072156.iD0ETpd1-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/drm_edid.c:5505:6: warning: no previous declaration for 'drm_edid_cta_sad_get' [-Wmissing-declarations]
    void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad)
         ^~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/drm_edid.c:5515:6: warning: no previous declaration for 'drm_edid_cta_sad_set' [-Wmissing-declarations]
    void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad)
         ^~~~~~~~~~~~~~~~~~~~


vim +/drm_edid_cta_sad_get +5505 drivers/gpu/drm/drm_edid.c

  5501	
  5502	/*
  5503	 * Get 3-byte SAD buf from struct cea_sad.
  5504	 */
> 5505	void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad)
  5506	{
  5507		sad[0] = cta_sad->format << 3 | cta_sad->channels;
  5508		sad[1] = cta_sad->freq;
  5509		sad[2] = cta_sad->byte2;
  5510	}
  5511	
  5512	/*
  5513	 * Set struct cea_sad from 3-byte SAD buf.
  5514	 */
> 5515	void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad)
  5516	{
  5517		cta_sad->format = (sad[0] & 0x78) >> 3;
  5518		cta_sad->channels = sad[0] & 0x07;
  5519		cta_sad->freq = sad[1] & 0x7f;
  5520		cta_sad->byte2 = sad[2];
  5521	}
  5522	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-gfx] [PATCH 2/6] drm/eld: replace uint8_t with u8
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 2/6] drm/eld: replace uint8_t with u8 Jani Nikula
@ 2023-09-08  5:25   ` Borah, Chaitanya Kumar
  2023-09-25 16:32   ` Golani, Mitulkumar Ajitkumar
  1 sibling, 0 replies; 21+ messages in thread
From: Borah, Chaitanya Kumar @ 2023-09-08  5:25 UTC (permalink / raw)
  To: Nikula, Jani, dri-devel@lists.freedesktop.org
  Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org



> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Jani
> Nikula
> Sent: Thursday, September 7, 2023 2:58 PM
> To: dri-devel@lists.freedesktop.org
> Cc: Nikula, Jani <jani.nikula@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 2/6] drm/eld: replace uint8_t with u8
>
> Unify on kernel types.
>
> Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

LGTM

Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>

> ---
>  include/drm/drm_eld.h | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/include/drm/drm_eld.h b/include/drm/drm_eld.h index
> 9bde89bd96ea..7b674256b9aa 100644
> --- a/include/drm/drm_eld.h
> +++ b/include/drm/drm_eld.h
> @@ -70,7 +70,7 @@
>   * drm_eld_mnl - Get ELD monitor name length in bytes.
>   * @eld: pointer to an eld memory structure with mnl set
>   */
> -static inline int drm_eld_mnl(const uint8_t *eld)
> +static inline int drm_eld_mnl(const u8 *eld)
>  {
>       return (eld[DRM_ELD_CEA_EDID_VER_MNL] &
> DRM_ELD_MNL_MASK) >> DRM_ELD_MNL_SHIFT;  } @@ -79,7 +79,7 @@
> static inline int drm_eld_mnl(const uint8_t *eld)
>   * drm_eld_sad - Get ELD SAD structures.
>   * @eld: pointer to an eld memory structure with sad_count set
>   */
> -static inline const uint8_t *drm_eld_sad(const uint8_t *eld)
> +static inline const u8 *drm_eld_sad(const u8 *eld)
>  {
>       unsigned int ver, mnl;
>
> @@ -98,7 +98,7 @@ static inline const uint8_t *drm_eld_sad(const uint8_t
> *eld)
>   * drm_eld_sad_count - Get ELD SAD count.
>   * @eld: pointer to an eld memory structure with sad_count set
>   */
> -static inline int drm_eld_sad_count(const uint8_t *eld)
> +static inline int drm_eld_sad_count(const u8 *eld)
>  {
>       return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] &
> DRM_ELD_SAD_COUNT_MASK) >>
>               DRM_ELD_SAD_COUNT_SHIFT;
> @@ -111,7 +111,7 @@ static inline int drm_eld_sad_count(const uint8_t *eld)
>   * This is a helper for determining the payload size of the baseline block, in
>   * bytes, for e.g. setting the Baseline_ELD_Len field in the ELD header block.
>   */
> -static inline int drm_eld_calc_baseline_block_size(const uint8_t *eld)
> +static inline int drm_eld_calc_baseline_block_size(const u8 *eld)
>  {
>       return DRM_ELD_MONITOR_NAME_STRING -
> DRM_ELD_HEADER_BLOCK_SIZE +
>               drm_eld_mnl(eld) + drm_eld_sad_count(eld) * 3; @@ -127,7
> +127,7 @@ static inline int drm_eld_calc_baseline_block_size(const uint8_t
> *eld)
>   *
>   * The returned value is guaranteed to be a multiple of 4.
>   */
> -static inline int drm_eld_size(const uint8_t *eld)
> +static inline int drm_eld_size(const u8 *eld)
>  {
>       return DRM_ELD_HEADER_BLOCK_SIZE +
> eld[DRM_ELD_BASELINE_ELD_LEN] * 4;  } @@ -139,7 +139,7 @@ static inline
> int drm_eld_size(const uint8_t *eld)
>   * The returned value is the speakers mask. User has to use
> %DRM_ELD_SPEAKER
>   * field definitions to identify speakers.
>   */
> -static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
> +static inline u8 drm_eld_get_spk_alloc(const u8 *eld)
>  {
>       return eld[DRM_ELD_SPEAKER] & DRM_ELD_SPEAKER_MASK;  } @@ -
> 151,7 +151,7 @@ static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
>   * The caller need to use %DRM_ELD_CONN_TYPE_HDMI or
> %DRM_ELD_CONN_TYPE_DP to
>   * identify the display type connected.
>   */
> -static inline u8 drm_eld_get_conn_type(const uint8_t *eld)
> +static inline u8 drm_eld_get_conn_type(const u8 *eld)
>  {
>       return eld[DRM_ELD_SAD_COUNT_CONN_TYPE] &
> DRM_ELD_CONN_TYPE_MASK;  }
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-gfx] [PATCH 1/6] drm/edid: split out drm_eld.h from drm_edid.h
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 1/6] drm/edid: split out drm_eld.h from drm_edid.h Jani Nikula
@ 2023-09-25 15:52   ` Golani, Mitulkumar Ajitkumar
  0 siblings, 0 replies; 21+ messages in thread
From: Golani, Mitulkumar Ajitkumar @ 2023-09-25 15:52 UTC (permalink / raw)
  To: Nikula, Jani, dri-devel@lists.freedesktop.org
  Cc: intel-gfx@lists.freedesktop.org

> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Thursday, September 7, 2023 2:58 PM
> To: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> Subject: [PATCH 1/6] drm/edid: split out drm_eld.h from drm_edid.h
> 
> The drm_edid.[ch] files are starting to be a bit crowded, and with plans to
> add more ELD related functionality, it's perhaps cleanest to split the ELD code
> out to a header of its own.
> 
> Include drm_eld.h from drm_edid.h for starters, and leave it to follow-up
> work to only include drm_eld.h where needed.
> 
> Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  Documentation/gpu/drm-kms-helpers.rst |   3 +
>  include/drm/drm_edid.h                | 149 +-----------------------
>  include/drm/drm_eld.h                 | 159 ++++++++++++++++++++++++++
>  3 files changed, 163 insertions(+), 148 deletions(-)  create mode 100644
> include/drm/drm_eld.h
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst
> b/Documentation/gpu/drm-kms-helpers.rst
> index b8ab05e42dbb..f0f93aa62545 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -363,6 +363,9 @@ EDID Helper Functions Reference  .. kernel-doc::
> drivers/gpu/drm/drm_edid.c
>     :export:
> 
> +.. kernel-doc:: include/drm/drm_eld.h
> +   :internal:
> +
>  SCDC Helper Functions Reference
>  ===============================
> 
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index
> 882d2638708e..1ff52f57ab9c 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -25,6 +25,7 @@
> 
>  #include <linux/types.h>
>  #include <linux/hdmi.h>
> +#include <drm/drm_eld.h> /* FIXME: remove this, include directly where
> +needed */
>  #include <drm/drm_mode.h>
> 
>  struct drm_device;
> @@ -269,64 +270,6 @@ struct detailed_timing {
>  #define DRM_EDID_DSC_MAX_SLICES			0xf
>  #define DRM_EDID_DSC_TOTAL_CHUNK_KBYTES		0x3f
> 
> -/* ELD Header Block */
> -#define DRM_ELD_HEADER_BLOCK_SIZE	4
> -
> -#define DRM_ELD_VER			0
> -# define DRM_ELD_VER_SHIFT		3
> -# define DRM_ELD_VER_MASK		(0x1f << 3)
> -# define DRM_ELD_VER_CEA861D		(2 << 3) /* supports 861D or
> below */
> -# define DRM_ELD_VER_CANNED		(0x1f << 3)
> -
> -#define DRM_ELD_BASELINE_ELD_LEN	2	/* in dwords! */
> -
> -/* ELD Baseline Block for ELD_Ver == 2 */
> -#define DRM_ELD_CEA_EDID_VER_MNL	4
> -# define DRM_ELD_CEA_EDID_VER_SHIFT	5
> -# define DRM_ELD_CEA_EDID_VER_MASK	(7 << 5)
> -# define DRM_ELD_CEA_EDID_VER_NONE	(0 << 5)
> -# define DRM_ELD_CEA_EDID_VER_CEA861	(1 << 5)
> -# define DRM_ELD_CEA_EDID_VER_CEA861A	(2 << 5)
> -# define DRM_ELD_CEA_EDID_VER_CEA861BCD	(3 << 5)
> -# define DRM_ELD_MNL_SHIFT		0
> -# define DRM_ELD_MNL_MASK		(0x1f << 0)
> -
> -#define DRM_ELD_SAD_COUNT_CONN_TYPE	5
> -# define DRM_ELD_SAD_COUNT_SHIFT	4
> -# define DRM_ELD_SAD_COUNT_MASK		(0xf << 4)
> -# define DRM_ELD_CONN_TYPE_SHIFT	2
> -# define DRM_ELD_CONN_TYPE_MASK		(3 << 2)
> -# define DRM_ELD_CONN_TYPE_HDMI		(0 << 2)
> -# define DRM_ELD_CONN_TYPE_DP		(1 << 2)
> -# define DRM_ELD_SUPPORTS_AI		(1 << 1)
> -# define DRM_ELD_SUPPORTS_HDCP		(1 << 0)
> -
> -#define DRM_ELD_AUD_SYNCH_DELAY		6	/* in units of
> 2 ms */
> -# define DRM_ELD_AUD_SYNCH_DELAY_MAX	0xfa	/* 500 ms */
> -
> -#define DRM_ELD_SPEAKER			7
> -# define DRM_ELD_SPEAKER_MASK		0x7f
> -# define DRM_ELD_SPEAKER_RLRC		(1 << 6)
> -# define DRM_ELD_SPEAKER_FLRC		(1 << 5)
> -# define DRM_ELD_SPEAKER_RC		(1 << 4)
> -# define DRM_ELD_SPEAKER_RLR		(1 << 3)
> -# define DRM_ELD_SPEAKER_FC		(1 << 2)
> -# define DRM_ELD_SPEAKER_LFE		(1 << 1)
> -# define DRM_ELD_SPEAKER_FLR		(1 << 0)
> -
> -#define DRM_ELD_PORT_ID			8	/* offsets 8..15
> inclusive */
> -# define DRM_ELD_PORT_ID_LEN		8
> -
> -#define DRM_ELD_MANUFACTURER_NAME0	16
> -#define DRM_ELD_MANUFACTURER_NAME1	17
> -
> -#define DRM_ELD_PRODUCT_CODE0		18
> -#define DRM_ELD_PRODUCT_CODE1		19
> -
> -#define DRM_ELD_MONITOR_NAME_STRING	20	/* offsets
> 20..(20+mnl-1) inclusive */
> -
> -#define DRM_ELD_CEA_SAD(mnl, sad)	(20 + (mnl) + 3 * (sad))
> -
>  struct edid {
>  	u8 header[8];
>  	/* Vendor & product info */
> @@ -409,96 +352,6 @@ drm_hdmi_avi_infoframe_quant_range(struct
> hdmi_avi_infoframe *frame,
>  				   const struct drm_display_mode *mode,
>  				   enum hdmi_quantization_range
> rgb_quant_range);
> 
> -/**
> - * drm_eld_mnl - Get ELD monitor name length in bytes.
> - * @eld: pointer to an eld memory structure with mnl set
> - */
> -static inline int drm_eld_mnl(const uint8_t *eld) -{
> -	return (eld[DRM_ELD_CEA_EDID_VER_MNL] &
> DRM_ELD_MNL_MASK) >> DRM_ELD_MNL_SHIFT;
> -}
> -
> -/**
> - * drm_eld_sad - Get ELD SAD structures.
> - * @eld: pointer to an eld memory structure with sad_count set
> - */
> -static inline const uint8_t *drm_eld_sad(const uint8_t *eld) -{
> -	unsigned int ver, mnl;
> -
> -	ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >>
> DRM_ELD_VER_SHIFT;
> -	if (ver != 2 && ver != 31)
> -		return NULL;
> -
> -	mnl = drm_eld_mnl(eld);
> -	if (mnl > 16)
> -		return NULL;
> -
> -	return eld + DRM_ELD_CEA_SAD(mnl, 0);
> -}
> -
> -/**
> - * drm_eld_sad_count - Get ELD SAD count.
> - * @eld: pointer to an eld memory structure with sad_count set
> - */
> -static inline int drm_eld_sad_count(const uint8_t *eld) -{
> -	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] &
> DRM_ELD_SAD_COUNT_MASK) >>
> -		DRM_ELD_SAD_COUNT_SHIFT;
> -}
> -
> -/**
> - * drm_eld_calc_baseline_block_size - Calculate baseline block size in bytes
> - * @eld: pointer to an eld memory structure with mnl and sad_count set
> - *
> - * This is a helper for determining the payload size of the baseline block, in
> - * bytes, for e.g. setting the Baseline_ELD_Len field in the ELD header block.
> - */
> -static inline int drm_eld_calc_baseline_block_size(const uint8_t *eld) -{
> -	return DRM_ELD_MONITOR_NAME_STRING -
> DRM_ELD_HEADER_BLOCK_SIZE +
> -		drm_eld_mnl(eld) + drm_eld_sad_count(eld) * 3;
> -}
> -
> -/**
> - * drm_eld_size - Get ELD size in bytes
> - * @eld: pointer to a complete eld memory structure
> - *
> - * The returned value does not include the vendor block. It's vendor specific,
> - * and comprises of the remaining bytes in the ELD memory buffer after
> - * drm_eld_size() bytes of header and baseline block.
> - *
> - * The returned value is guaranteed to be a multiple of 4.
> - */
> -static inline int drm_eld_size(const uint8_t *eld) -{
> -	return DRM_ELD_HEADER_BLOCK_SIZE +
> eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
> -}
> -
> -/**
> - * drm_eld_get_spk_alloc - Get speaker allocation
> - * @eld: pointer to an ELD memory structure
> - *
> - * The returned value is the speakers mask. User has to use
> %DRM_ELD_SPEAKER
> - * field definitions to identify speakers.
> - */
> -static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld) -{
> -	return eld[DRM_ELD_SPEAKER] & DRM_ELD_SPEAKER_MASK;
> -}
> -
> -/**
> - * drm_eld_get_conn_type - Get device type hdmi/dp connected
> - * @eld: pointer to an ELD memory structure
> - *
> - * The caller need to use %DRM_ELD_CONN_TYPE_HDMI or
> %DRM_ELD_CONN_TYPE_DP to
> - * identify the display type connected.
> - */
> -static inline u8 drm_eld_get_conn_type(const uint8_t *eld) -{
> -	return eld[DRM_ELD_SAD_COUNT_CONN_TYPE] &
> DRM_ELD_CONN_TYPE_MASK;
> -}
> -
>  /**
>   * drm_edid_decode_mfg_id - Decode the manufacturer ID
>   * @mfg_id: The manufacturer ID
> diff --git a/include/drm/drm_eld.h b/include/drm/drm_eld.h new file mode
> 100644 index 000000000000..9bde89bd96ea
> --- /dev/null
> +++ b/include/drm/drm_eld.h
> @@ -0,0 +1,159 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef __DRM_ELD_H__
> +#define __DRM_ELD_H__
> +
> +#include <linux/types.h>
> +
> +/* ELD Header Block */
> +#define DRM_ELD_HEADER_BLOCK_SIZE	4
> +
> +#define DRM_ELD_VER			0
> +# define DRM_ELD_VER_SHIFT		3
> +# define DRM_ELD_VER_MASK		(0x1f << 3)
> +# define DRM_ELD_VER_CEA861D		(2 << 3) /* supports 861D or
> below */
> +# define DRM_ELD_VER_CANNED		(0x1f << 3)
> +
> +#define DRM_ELD_BASELINE_ELD_LEN	2	/* in dwords! */
> +
> +/* ELD Baseline Block for ELD_Ver == 2 */
> +#define DRM_ELD_CEA_EDID_VER_MNL	4
> +# define DRM_ELD_CEA_EDID_VER_SHIFT	5
> +# define DRM_ELD_CEA_EDID_VER_MASK	(7 << 5)
> +# define DRM_ELD_CEA_EDID_VER_NONE	(0 << 5)
> +# define DRM_ELD_CEA_EDID_VER_CEA861	(1 << 5)
> +# define DRM_ELD_CEA_EDID_VER_CEA861A	(2 << 5)
> +# define DRM_ELD_CEA_EDID_VER_CEA861BCD	(3 << 5)
> +# define DRM_ELD_MNL_SHIFT		0
> +# define DRM_ELD_MNL_MASK		(0x1f << 0)
> +
> +#define DRM_ELD_SAD_COUNT_CONN_TYPE	5
> +# define DRM_ELD_SAD_COUNT_SHIFT	4
> +# define DRM_ELD_SAD_COUNT_MASK		(0xf << 4)
> +# define DRM_ELD_CONN_TYPE_SHIFT	2
> +# define DRM_ELD_CONN_TYPE_MASK		(3 << 2)
> +# define DRM_ELD_CONN_TYPE_HDMI		(0 << 2)
> +# define DRM_ELD_CONN_TYPE_DP		(1 << 2)
> +# define DRM_ELD_SUPPORTS_AI		(1 << 1)
> +# define DRM_ELD_SUPPORTS_HDCP		(1 << 0)
> +
> +#define DRM_ELD_AUD_SYNCH_DELAY		6	/* in units of
> 2 ms */
> +# define DRM_ELD_AUD_SYNCH_DELAY_MAX	0xfa	/* 500 ms */
> +
> +#define DRM_ELD_SPEAKER			7
> +# define DRM_ELD_SPEAKER_MASK		0x7f
> +# define DRM_ELD_SPEAKER_RLRC		(1 << 6)
> +# define DRM_ELD_SPEAKER_FLRC		(1 << 5)
> +# define DRM_ELD_SPEAKER_RC		(1 << 4)
> +# define DRM_ELD_SPEAKER_RLR		(1 << 3)
> +# define DRM_ELD_SPEAKER_FC		(1 << 2)
> +# define DRM_ELD_SPEAKER_LFE		(1 << 1)
> +# define DRM_ELD_SPEAKER_FLR		(1 << 0)
> +
> +#define DRM_ELD_PORT_ID			8	/* offsets 8..15
> inclusive */
> +# define DRM_ELD_PORT_ID_LEN		8
> +
> +#define DRM_ELD_MANUFACTURER_NAME0	16
> +#define DRM_ELD_MANUFACTURER_NAME1	17
> +
> +#define DRM_ELD_PRODUCT_CODE0		18
> +#define DRM_ELD_PRODUCT_CODE1		19
> +
> +#define DRM_ELD_MONITOR_NAME_STRING	20	/* offsets
> 20..(20+mnl-1) inclusive */
> +
> +#define DRM_ELD_CEA_SAD(mnl, sad)	(20 + (mnl) + 3 * (sad))
> +
> +/**
> + * drm_eld_mnl - Get ELD monitor name length in bytes.
> + * @eld: pointer to an eld memory structure with mnl set  */ static
> +inline int drm_eld_mnl(const uint8_t *eld) {
> +	return (eld[DRM_ELD_CEA_EDID_VER_MNL] &
> DRM_ELD_MNL_MASK) >>
> +DRM_ELD_MNL_SHIFT; }
> +
> +/**
> + * drm_eld_sad - Get ELD SAD structures.
> + * @eld: pointer to an eld memory structure with sad_count set  */
> +static inline const uint8_t *drm_eld_sad(const uint8_t *eld) {
> +	unsigned int ver, mnl;
> +
> +	ver = (eld[DRM_ELD_VER] & DRM_ELD_VER_MASK) >>
> DRM_ELD_VER_SHIFT;
> +	if (ver != 2 && ver != 31)
> +		return NULL;
> +
> +	mnl = drm_eld_mnl(eld);
> +	if (mnl > 16)
> +		return NULL;
> +
> +	return eld + DRM_ELD_CEA_SAD(mnl, 0);
> +}
> +
> +/**
> + * drm_eld_sad_count - Get ELD SAD count.
> + * @eld: pointer to an eld memory structure with sad_count set  */
> +static inline int drm_eld_sad_count(const uint8_t *eld) {
> +	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] &
> DRM_ELD_SAD_COUNT_MASK) >>
> +		DRM_ELD_SAD_COUNT_SHIFT;
> +}
> +
> +/**
> + * drm_eld_calc_baseline_block_size - Calculate baseline block size in
> +bytes
> + * @eld: pointer to an eld memory structure with mnl and sad_count set
> + *
> + * This is a helper for determining the payload size of the baseline
> +block, in
> + * bytes, for e.g. setting the Baseline_ELD_Len field in the ELD header block.
> + */
> +static inline int drm_eld_calc_baseline_block_size(const uint8_t *eld)
> +{
> +	return DRM_ELD_MONITOR_NAME_STRING -
> DRM_ELD_HEADER_BLOCK_SIZE +
> +		drm_eld_mnl(eld) + drm_eld_sad_count(eld) * 3; }
> +
> +/**
> + * drm_eld_size - Get ELD size in bytes
> + * @eld: pointer to a complete eld memory structure
> + *
> + * The returned value does not include the vendor block. It's vendor
> +specific,
> + * and comprises of the remaining bytes in the ELD memory buffer after
> + * drm_eld_size() bytes of header and baseline block.
> + *
> + * The returned value is guaranteed to be a multiple of 4.
> + */
> +static inline int drm_eld_size(const uint8_t *eld) {
> +	return DRM_ELD_HEADER_BLOCK_SIZE +
> eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
> +}
> +
> +/**
> + * drm_eld_get_spk_alloc - Get speaker allocation
> + * @eld: pointer to an ELD memory structure
> + *
> + * The returned value is the speakers mask. User has to use
> +%DRM_ELD_SPEAKER
> + * field definitions to identify speakers.
> + */
> +static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld) {
> +	return eld[DRM_ELD_SPEAKER] & DRM_ELD_SPEAKER_MASK; }
> +
> +/**
> + * drm_eld_get_conn_type - Get device type hdmi/dp connected
> + * @eld: pointer to an ELD memory structure
> + *
> + * The caller need to use %DRM_ELD_CONN_TYPE_HDMI or
> +%DRM_ELD_CONN_TYPE_DP to
> + * identify the display type connected.
> + */
> +static inline u8 drm_eld_get_conn_type(const uint8_t *eld) {
> +	return eld[DRM_ELD_SAD_COUNT_CONN_TYPE] &
> DRM_ELD_CONN_TYPE_MASK; }
> +
> +#endif /* __DRM_ELD_H__ */


The commit splits drm_eld.h from drm_edid.h and commit message is also aligned to that.

Changes LGTM.
Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-gfx] [PATCH 2/6] drm/eld: replace uint8_t with u8
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 2/6] drm/eld: replace uint8_t with u8 Jani Nikula
  2023-09-08  5:25   ` Borah, Chaitanya Kumar
@ 2023-09-25 16:32   ` Golani, Mitulkumar Ajitkumar
  1 sibling, 0 replies; 21+ messages in thread
From: Golani, Mitulkumar Ajitkumar @ 2023-09-25 16:32 UTC (permalink / raw)
  To: Nikula, Jani, dri-devel@lists.freedesktop.org
  Cc: intel-gfx@lists.freedesktop.org



> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Thursday, September 7, 2023 2:58 PM
> To: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> Subject: [PATCH 2/6] drm/eld: replace uint8_t with u8
> 
> Unify on kernel types.
> 
> Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  include/drm/drm_eld.h | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/include/drm/drm_eld.h b/include/drm/drm_eld.h index
> 9bde89bd96ea..7b674256b9aa 100644
> --- a/include/drm/drm_eld.h
> +++ b/include/drm/drm_eld.h
> @@ -70,7 +70,7 @@
>   * drm_eld_mnl - Get ELD monitor name length in bytes.
>   * @eld: pointer to an eld memory structure with mnl set
>   */
> -static inline int drm_eld_mnl(const uint8_t *eld)
> +static inline int drm_eld_mnl(const u8 *eld)
>  {
>  	return (eld[DRM_ELD_CEA_EDID_VER_MNL] &
> DRM_ELD_MNL_MASK) >> DRM_ELD_MNL_SHIFT;  } @@ -79,7 +79,7 @@
> static inline int drm_eld_mnl(const uint8_t *eld)
>   * drm_eld_sad - Get ELD SAD structures.
>   * @eld: pointer to an eld memory structure with sad_count set
>   */
> -static inline const uint8_t *drm_eld_sad(const uint8_t *eld)
> +static inline const u8 *drm_eld_sad(const u8 *eld)
>  {
>  	unsigned int ver, mnl;
> 
> @@ -98,7 +98,7 @@ static inline const uint8_t *drm_eld_sad(const uint8_t
> *eld)
>   * drm_eld_sad_count - Get ELD SAD count.
>   * @eld: pointer to an eld memory structure with sad_count set
>   */
> -static inline int drm_eld_sad_count(const uint8_t *eld)
> +static inline int drm_eld_sad_count(const u8 *eld)
>  {
>  	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] &
> DRM_ELD_SAD_COUNT_MASK) >>
>  		DRM_ELD_SAD_COUNT_SHIFT;
> @@ -111,7 +111,7 @@ static inline int drm_eld_sad_count(const uint8_t
> *eld)
>   * This is a helper for determining the payload size of the baseline block, in
>   * bytes, for e.g. setting the Baseline_ELD_Len field in the ELD header block.
>   */
> -static inline int drm_eld_calc_baseline_block_size(const uint8_t *eld)
> +static inline int drm_eld_calc_baseline_block_size(const u8 *eld)
>  {
>  	return DRM_ELD_MONITOR_NAME_STRING -
> DRM_ELD_HEADER_BLOCK_SIZE +
>  		drm_eld_mnl(eld) + drm_eld_sad_count(eld) * 3; @@ -127,7
> +127,7 @@ static inline int drm_eld_calc_baseline_block_size(const uint8_t
> *eld)
>   *
>   * The returned value is guaranteed to be a multiple of 4.
>   */
> -static inline int drm_eld_size(const uint8_t *eld)
> +static inline int drm_eld_size(const u8 *eld)
>  {
>  	return DRM_ELD_HEADER_BLOCK_SIZE +
> eld[DRM_ELD_BASELINE_ELD_LEN] * 4;  } @@ -139,7 +139,7 @@ static inline
> int drm_eld_size(const uint8_t *eld)
>   * The returned value is the speakers mask. User has to use
> %DRM_ELD_SPEAKER
>   * field definitions to identify speakers.
>   */
> -static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
> +static inline u8 drm_eld_get_spk_alloc(const u8 *eld)
>  {
>  	return eld[DRM_ELD_SPEAKER] & DRM_ELD_SPEAKER_MASK;  } @@ -
> 151,7 +151,7 @@ static inline u8 drm_eld_get_spk_alloc(const uint8_t *eld)
>   * The caller need to use %DRM_ELD_CONN_TYPE_HDMI or
> %DRM_ELD_CONN_TYPE_DP to
>   * identify the display type connected.
>   */
> -static inline u8 drm_eld_get_conn_type(const uint8_t *eld)
> +static inline u8 drm_eld_get_conn_type(const u8 *eld)

Changes LGTM
Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>

>  {
>  	return eld[DRM_ELD_SAD_COUNT_CONN_TYPE] &
> DRM_ELD_CONN_TYPE_MASK;  }
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-gfx] [PATCH 3/6] drm/edid: include drm_eld.h only where required
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 3/6] drm/edid: include drm_eld.h only where required Jani Nikula
@ 2023-09-25 16:59   ` Golani, Mitulkumar Ajitkumar
  0 siblings, 0 replies; 21+ messages in thread
From: Golani, Mitulkumar Ajitkumar @ 2023-09-25 16:59 UTC (permalink / raw)
  To: Nikula, Jani, dri-devel@lists.freedesktop.org
  Cc: intel-gfx@lists.freedesktop.org

> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Thursday, September 7, 2023 2:58 PM
> To: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> Subject: [PATCH 3/6] drm/edid: include drm_eld.h only where required
> 
> Reduce the dependencies on drm_eld.h. Some files might be able to drop the
> dependency on drm_edid.h too with the direct inclusion of drm_eld.h.
> 
> Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c    | 1 +
>  drivers/gpu/drm/drm_edid.c                           | 1 +
>  drivers/gpu/drm/i915/display/intel_audio.c           | 1 +
>  drivers/gpu/drm/i915/display/intel_crtc_state_dump.c | 1 +
>  drivers/gpu/drm/i915/display/intel_sdvo.c            | 1 +
>  drivers/gpu/drm/nouveau/dispnv50/disp.c              | 1 +
>  drivers/gpu/drm/radeon/radeon_audio.c                | 1 +
>  drivers/gpu/drm/tegra/hdmi.c                         | 1 +
>  drivers/gpu/drm/tegra/sor.c                          | 1 +
>  include/drm/drm_edid.h                               | 1 -
>  sound/core/pcm_drm_eld.c                             | 1 +
>  sound/soc/codecs/hdac_hdmi.c                         | 1 +
>  sound/soc/codecs/hdmi-codec.c                        | 1 +
>  sound/x86/intel_hdmi_audio.c                         | 1 +
>  14 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 268cb99a4c4b..fe7e307ae7f9 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -86,6 +86,7 @@
>  #include <drm/drm_blend.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_eld.h>
>  #include <drm/drm_vblank.h>
>  #include <drm/drm_audio_component.h>
>  #include <drm/drm_gem_atomic_helper.h>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index
> 39dd3f694544..2025970816c9 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -41,6 +41,7 @@
>  #include <drm/drm_displayid.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_eld.h>
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_print.h>
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c
> b/drivers/gpu/drm/i915/display/intel_audio.c
> index 19605264a35c..39f5b698e08a 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -25,6 +25,7 @@
>  #include <linux/kernel.h>
> 
>  #include <drm/drm_edid.h>
> +#include <drm/drm_eld.h>
>  #include <drm/i915_component.h>
> 
>  #include "i915_drv.h"
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> index 8d4640d0fd34..fcddd6d81768 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> @@ -4,6 +4,7 @@
>   */
> 
>  #include <drm/drm_edid.h>
> +#include <drm/drm_eld.h>
> 
>  #include "i915_drv.h"
>  #include "intel_crtc_state_dump.h"
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 135a2527fd1b..6abae283998e 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -35,6 +35,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_eld.h>
> 
>  #include "i915_drv.h"
>  #include "i915_reg.h"
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 4e7c9c353c51..9332aa633867 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -38,6 +38,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_eld.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_vblank.h>
> diff --git a/drivers/gpu/drm/radeon/radeon_audio.c
> b/drivers/gpu/drm/radeon/radeon_audio.c
> index d6ccaf24ee0c..279bf130a18c 100644
> --- a/drivers/gpu/drm/radeon/radeon_audio.c
> +++ b/drivers/gpu/drm/radeon/radeon_audio.c
> @@ -26,6 +26,7 @@
>  #include <linux/component.h>
> 
>  #include <drm/drm_crtc.h>
> +#include <drm/drm_eld.h>
>  #include "dce6_afmt.h"
>  #include "evergreen_hdmi.h"
>  #include "radeon.h"
> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
> index 0ba3ca3ac509..a1fcee665023 100644
> --- a/drivers/gpu/drm/tegra/hdmi.c
> +++ b/drivers/gpu/drm/tegra/hdmi.c
> @@ -24,6 +24,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_debugfs.h>
> +#include <drm/drm_eld.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_probe_helper.h>
> diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c index
> d5a3d3f4fece..83341576630d 100644
> --- a/drivers/gpu/drm/tegra/sor.c
> +++ b/drivers/gpu/drm/tegra/sor.c
> @@ -20,6 +20,7 @@
>  #include <drm/display/drm_scdc_helper.h>  #include
> <drm/drm_atomic_helper.h>  #include <drm/drm_debugfs.h>
> +#include <drm/drm_eld.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_simple_kms_helper.h>
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index
> 1ff52f57ab9c..e98aa6818700 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -25,7 +25,6 @@
> 
>  #include <linux/types.h>
>  #include <linux/hdmi.h>
> -#include <drm/drm_eld.h> /* FIXME: remove this, include directly where
> needed */  #include <drm/drm_mode.h>
> 
>  struct drm_device;
> diff --git a/sound/core/pcm_drm_eld.c b/sound/core/pcm_drm_eld.c index
> 07075071972d..1cdca4d4fc9c 100644
> --- a/sound/core/pcm_drm_eld.c
> +++ b/sound/core/pcm_drm_eld.c
> @@ -6,6 +6,7 @@
>  #include <linux/export.h>
>  #include <linux/hdmi.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_eld.h>
>  #include <sound/pcm.h>
>  #include <sound/pcm_drm_eld.h>
> 
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 8b6b76029694..d1b53fc1efb6 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -16,6 +16,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/hdmi.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_eld.h>
>  #include <sound/pcm_params.h>
>  #include <sound/jack.h>
>  #include <sound/soc.h>
> diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-
> codec.c index d21f69f05342..9b01d060f7cc 100644
> --- a/sound/soc/codecs/hdmi-codec.c
> +++ b/sound/soc/codecs/hdmi-codec.c
> @@ -17,6 +17,7 @@
>  #include <sound/pcm_iec958.h>
> 
>  #include <drm/drm_crtc.h> /* This is only to get MAX_ELD_BYTES */
> +#include <drm/drm_eld.h>
> 
>  #define HDMI_CODEC_CHMAP_IDX_UNKNOWN  -1
> 
> diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c
> index ab95fb34a635..02f5a7f9b728 100644
> --- a/sound/x86/intel_hdmi_audio.c
> +++ b/sound/x86/intel_hdmi_audio.c
> @@ -30,6 +30,7 @@
>  #include <sound/control.h>
>  #include <sound/jack.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_eld.h>

Changes LGTM.
Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>

>  #include <drm/intel_lpe_audio.h>
>  #include "intel_hdmi_audio.h"
> 
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-gfx] [PATCH 4/6] drm/edid: use a temp variable for sads to drop one level of dereferences
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 4/6] drm/edid: use a temp variable for sads to drop one level of dereferences Jani Nikula
@ 2023-09-25 17:46   ` Golani, Mitulkumar Ajitkumar
  2023-09-26  7:44     ` Jani Nikula
  0 siblings, 1 reply; 21+ messages in thread
From: Golani, Mitulkumar Ajitkumar @ 2023-09-25 17:46 UTC (permalink / raw)
  To: Nikula, Jani, dri-devel@lists.freedesktop.org
  Cc: intel-gfx@lists.freedesktop.org

Hi Jani,

added comments in-line.

> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Thursday, September 7, 2023 2:58 PM
> To: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> Subject: [PATCH 4/6] drm/edid: use a temp variable for sads to drop one level
> of dereferences
> 
> It's arguably easier on the eyes, and drops a set of parenthesis too.

Please consider providing a bit more context in the commit message for better clarity.

> 
> Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index
> 2025970816c9..fcdc2c314cde 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5583,7 +5583,7 @@ static void drm_edid_to_eld(struct drm_connector
> *connector,  }
> 
>  static int _drm_edid_to_sad(const struct drm_edid *drm_edid,
> -			    struct cea_sad **sads)
> +			    struct cea_sad **psads)
>  {
>  	const struct cea_db *db;
>  	struct cea_db_iter iter;
> @@ -5592,19 +5592,21 @@ static int _drm_edid_to_sad(const struct
> drm_edid *drm_edid,
>  	cea_db_iter_edid_begin(drm_edid, &iter);
>  	cea_db_iter_for_each(db, &iter) {
>  		if (cea_db_tag(db) == CTA_DB_AUDIO) {
> +			struct cea_sad *sads;
>  			int j;
> 
>  			count = cea_db_payload_len(db) / 3; /* SAD is 3B */
> -			*sads = kcalloc(count, sizeof(**sads), GFP_KERNEL);
> -			if (!*sads)
> +			sads = kcalloc(count, sizeof(*sads), GFP_KERNEL);
> +			*psads = sads;
> +			if (!sads)
>  				return -ENOMEM;
>  			for (j = 0; j < count; j++) {
>  				const u8 *sad = &db->data[j * 3];
> 
> -				(*sads)[j].format = (sad[0] & 0x78) >> 3;
> -				(*sads)[j].channels = sad[0] & 0x7;
> -				(*sads)[j].freq = sad[1] & 0x7F;
> -				(*sads)[j].byte2 = sad[2];
> +				sads[j].format = (sad[0] & 0x78) >> 3;
> +				sads[j].channels = sad[0] & 0x7;
> +				sads[j].freq = sad[1] & 0x7F;
> +				sads[j].byte2 = sad[2];

Thanks for the code update. I noticed the use of magic values in this section, which can make the code less clear 
and harder to maintain. Would it be possible to define constants or use descriptive names instead of these magic 
values?

Regards,
Mitul
>  			}
>  			break;
>  		}
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-gfx] [PATCH 4/6] drm/edid: use a temp variable for sads to drop one level of dereferences
  2023-09-25 17:46   ` Golani, Mitulkumar Ajitkumar
@ 2023-09-26  7:44     ` Jani Nikula
  2023-09-26 16:14       ` Golani, Mitulkumar Ajitkumar
  0 siblings, 1 reply; 21+ messages in thread
From: Jani Nikula @ 2023-09-26  7:44 UTC (permalink / raw)
  To: Golani, Mitulkumar Ajitkumar, dri-devel@lists.freedesktop.org
  Cc: intel-gfx@lists.freedesktop.org

On Mon, 25 Sep 2023, "Golani, Mitulkumar Ajitkumar" <mitulkumar.ajitkumar.golani@intel.com> wrote:
> Hi Jani,
>
> added comments in-line.
>
>> -----Original Message-----
>> From: Nikula, Jani <jani.nikula@intel.com>
>> Sent: Thursday, September 7, 2023 2:58 PM
>> To: dri-devel@lists.freedesktop.org
>> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
>> Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
>> Subject: [PATCH 4/6] drm/edid: use a temp variable for sads to drop one level
>> of dereferences
>>
>> It's arguably easier on the eyes, and drops a set of parenthesis too.
>
> Please consider providing a bit more context in the commit message for better clarity.
>
>>
>> Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index
>> 2025970816c9..fcdc2c314cde 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -5583,7 +5583,7 @@ static void drm_edid_to_eld(struct drm_connector
>> *connector,  }
>>
>>  static int _drm_edid_to_sad(const struct drm_edid *drm_edid,
>> -                         struct cea_sad **sads)
>> +                         struct cea_sad **psads)
>>  {
>>       const struct cea_db *db;
>>       struct cea_db_iter iter;
>> @@ -5592,19 +5592,21 @@ static int _drm_edid_to_sad(const struct
>> drm_edid *drm_edid,
>>       cea_db_iter_edid_begin(drm_edid, &iter);
>>       cea_db_iter_for_each(db, &iter) {
>>               if (cea_db_tag(db) == CTA_DB_AUDIO) {
>> +                     struct cea_sad *sads;
>>                       int j;
>>
>>                       count = cea_db_payload_len(db) / 3; /* SAD is 3B */
>> -                     *sads = kcalloc(count, sizeof(**sads), GFP_KERNEL);
>> -                     if (!*sads)
>> +                     sads = kcalloc(count, sizeof(*sads), GFP_KERNEL);
>> +                     *psads = sads;
>> +                     if (!sads)
>>                               return -ENOMEM;
>>                       for (j = 0; j < count; j++) {
>>                               const u8 *sad = &db->data[j * 3];
>>
>> -                             (*sads)[j].format = (sad[0] & 0x78) >> 3;
>> -                             (*sads)[j].channels = sad[0] & 0x7;
>> -                             (*sads)[j].freq = sad[1] & 0x7F;
>> -                             (*sads)[j].byte2 = sad[2];
>> +                             sads[j].format = (sad[0] & 0x78) >> 3;
>> +                             sads[j].channels = sad[0] & 0x7;
>> +                             sads[j].freq = sad[1] & 0x7F;
>> +                             sads[j].byte2 = sad[2];
>
> Thanks for the code update. I noticed the use of magic values in this section, which can make the code less clear
> and harder to maintain. Would it be possible to define constants or use descriptive names instead of these magic
> values?

Yes, but that would be for a separate patch. The magic values aren't
added here.

BR,
Jani.

>
> Regards,
> Mitul
>>                       }
>>                       break;
>>               }
>> --
>> 2.39.2
>

-- 
Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-gfx] [PATCH 4/6] drm/edid: use a temp variable for sads to drop one level of dereferences
  2023-09-26  7:44     ` Jani Nikula
@ 2023-09-26 16:14       ` Golani, Mitulkumar Ajitkumar
  0 siblings, 0 replies; 21+ messages in thread
From: Golani, Mitulkumar Ajitkumar @ 2023-09-26 16:14 UTC (permalink / raw)
  To: Nikula, Jani, dri-devel@lists.freedesktop.org
  Cc: intel-gfx@lists.freedesktop.org



> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: 26 September 2023 13:14
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>;
> dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org
> Subject: RE: [PATCH 4/6] drm/edid: use a temp variable for sads to drop one
> level of dereferences
> 
> On Mon, 25 Sep 2023, "Golani, Mitulkumar Ajitkumar"
> <mitulkumar.ajitkumar.golani@intel.com> wrote:
> > Hi Jani,
> >
> > added comments in-line.
> >
> >> -----Original Message-----
> >> From: Nikula, Jani <jani.nikula@intel.com>
> >> Sent: Thursday, September 7, 2023 2:58 PM
> >> To: dri-devel@lists.freedesktop.org
> >> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani
> >> <jani.nikula@intel.com>; Golani, Mitulkumar Ajitkumar
> >> <mitulkumar.ajitkumar.golani@intel.com>
> >> Subject: [PATCH 4/6] drm/edid: use a temp variable for sads to drop
> >> one level of dereferences
> >>
> >> It's arguably easier on the eyes, and drops a set of parenthesis too.
> >
> > Please consider providing a bit more context in the commit message for
> better clarity.
> >
> >>
> >> Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_edid.c | 16 +++++++++-------
> >>  1 file changed, 9 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index 2025970816c9..fcdc2c314cde 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -5583,7 +5583,7 @@ static void drm_edid_to_eld(struct
> >> drm_connector *connector,  }
> >>
> >>  static int _drm_edid_to_sad(const struct drm_edid *drm_edid,
> >> -                         struct cea_sad **sads)
> >> +                         struct cea_sad **psads)
> >>  {
> >>       const struct cea_db *db;
> >>       struct cea_db_iter iter;
> >> @@ -5592,19 +5592,21 @@ static int _drm_edid_to_sad(const struct
> >> drm_edid *drm_edid,
> >>       cea_db_iter_edid_begin(drm_edid, &iter);
> >>       cea_db_iter_for_each(db, &iter) {
> >>               if (cea_db_tag(db) == CTA_DB_AUDIO) {
> >> +                     struct cea_sad *sads;
> >>                       int j;
> >>
> >>                       count = cea_db_payload_len(db) / 3; /* SAD is 3B */
> >> -                     *sads = kcalloc(count, sizeof(**sads), GFP_KERNEL);
> >> -                     if (!*sads)
> >> +                     sads = kcalloc(count, sizeof(*sads), GFP_KERNEL);
> >> +                     *psads = sads;
> >> +                     if (!sads)
> >>                               return -ENOMEM;
> >>                       for (j = 0; j < count; j++) {
> >>                               const u8 *sad = &db->data[j * 3];
> >>
> >> -                             (*sads)[j].format = (sad[0] & 0x78) >> 3;
> >> -                             (*sads)[j].channels = sad[0] & 0x7;
> >> -                             (*sads)[j].freq = sad[1] & 0x7F;
> >> -                             (*sads)[j].byte2 = sad[2];
> >> +                             sads[j].format = (sad[0] & 0x78) >> 3;
> >> +                             sads[j].channels = sad[0] & 0x7;
> >> +                             sads[j].freq = sad[1] & 0x7F;
> >> +                             sads[j].byte2 = sad[2];
> >
> > Thanks for the code update. I noticed the use of magic values in this
> > section, which can make the code less clear and harder to maintain.
> > Would it be possible to define constants or use descriptive names instead
> of these magic values?
> 
> Yes, but that would be for a separate patch. The magic values aren't added
> here.
 
Sure. Other changes looks good to me.
Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> 
> BR,
> Jani.
> 
> >
> > Regards,
> > Mitul
> >>                       }
> >>                       break;
> >>               }
> >> --
> >> 2.39.2
> >
> 
> --
> Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-gfx] [PATCH 6/6] drm/eld: add helpers to modify the SADs of an ELD
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 6/6] drm/eld: add helpers to modify the SADs of an ELD Jani Nikula
@ 2023-09-26 18:04   ` Golani, Mitulkumar Ajitkumar
  0 siblings, 0 replies; 21+ messages in thread
From: Golani, Mitulkumar Ajitkumar @ 2023-09-26 18:04 UTC (permalink / raw)
  To: Nikula, Jani, dri-devel@lists.freedesktop.org
  Cc: intel-gfx@lists.freedesktop.org

> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: 07 September 2023 14:58
> To: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> Subject: [PATCH 6/6] drm/eld: add helpers to modify the SADs of an ELD
> 
> Occasionally it's necessary for drivers to modify the SADs of an ELD, but it's
> not so cool to have drivers poke at the ELD buffer directly.
> 
> Using the helpers to translate between 3-byte SAD and struct cea_sad, add
> ELD helpers to get/set the SADs from/to an ELD.
> 
> Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  Documentation/gpu/drm-kms-helpers.rst |  3 ++
>  drivers/gpu/drm/Makefile              |  1 +
>  drivers/gpu/drm/drm_eld.c             | 55 +++++++++++++++++++++++++++
>  include/drm/drm_eld.h                 |  5 +++
>  4 files changed, 64 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_eld.c
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst
> b/Documentation/gpu/drm-kms-helpers.rst
> index f0f93aa62545..df91b7cd992e 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -366,6 +366,9 @@ EDID Helper Functions Reference  .. kernel-doc::
> include/drm/drm_eld.h
>     :internal:
> 
> +.. kernel-doc:: drivers/gpu/drm/drm_eld.c
> +   :export:
> +
>  SCDC Helper Functions Reference
>  ===============================
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index
> 215e78e79125..632e74d823e8 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -22,6 +22,7 @@ drm-y := \
>  	drm_drv.o \
>  	drm_dumb_buffers.o \
>  	drm_edid.o \
> +	drm_eld.o \
>  	drm_encoder.o \
>  	drm_file.o \
>  	drm_fourcc.o \
> diff --git a/drivers/gpu/drm/drm_eld.c b/drivers/gpu/drm/drm_eld.c new
> file mode 100644 index 000000000000..34e0d71c3550
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_eld.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include <drm/drm_edid.h>
> +#include <drm/drm_eld.h>
> +
> +#include "drm_internal.h"
> +
> +/**
> + * drm_eld_sad_get - get SAD from ELD to struct cea_sad
> + * @eld: ELD buffer
> + * @i: SAD number
> + * @cta_sad: destination struct cea_sad
> + *
> + * @return: 0 on success, or negative on errors  */ int
> +drm_eld_sad_get(const u8 *eld, int i, struct cea_sad *cta_sad) {

Could we use a more descriptive variable name than 'i' for better code readability in the functions drm_eld_sad_get and drm_eld_sad_set?
possibly something like, `sad_number` or `index` ?

Regards,
Mitul
> +	const u8 *sad;
> +
> +	if (i >= drm_eld_sad_count(eld))
> +		return -EINVAL;
> +
> +	sad = eld + DRM_ELD_CEA_SAD(drm_eld_mnl(eld), i);
> +
> +	drm_edid_cta_sad_set(cta_sad, sad);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_eld_sad_get);
> +
> +/**
> + * drm_eld_sad_set - set SAD to ELD from struct cea_sad
> + * @eld: ELD buffer
> + * @i: SAD number
> + * @cta_sad: source struct cea_sad
> + *
> + * @return: 0 on success, or negative on errors  */ int
> +drm_eld_sad_set(u8 *eld, int i, const struct cea_sad *cta_sad) {
> +	u8 *sad;
> +
> +	if (i >= drm_eld_sad_count(eld))
> +		return -EINVAL;
> +
> +	sad = eld + DRM_ELD_CEA_SAD(drm_eld_mnl(eld), i);
> +
> +	drm_edid_cta_sad_get(cta_sad, sad);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_eld_sad_set);
> diff --git a/include/drm/drm_eld.h b/include/drm/drm_eld.h index
> 7b674256b9aa..5b320157684c 100644
> --- a/include/drm/drm_eld.h
> +++ b/include/drm/drm_eld.h
> @@ -8,6 +8,8 @@
> 
>  #include <linux/types.h>
> 
> +struct cea_sad;
> +
>  /* ELD Header Block */
>  #define DRM_ELD_HEADER_BLOCK_SIZE	4
> 
> @@ -75,6 +77,9 @@ static inline int drm_eld_mnl(const u8 *eld)
>  	return (eld[DRM_ELD_CEA_EDID_VER_MNL] &
> DRM_ELD_MNL_MASK) >> DRM_ELD_MNL_SHIFT;  }
> 
> +int drm_eld_sad_get(const u8 *eld, int i, struct cea_sad *cta_sad); int
> +drm_eld_sad_set(u8 *eld, int i, const struct cea_sad *cta_sad);
> +
>  /**
>   * drm_eld_sad - Get ELD SAD structures.
>   * @eld: pointer to an eld memory structure with sad_count set
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-gfx] [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
  2023-09-07  9:28 ` [Intel-gfx] [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad Jani Nikula
  2023-09-07 11:49   ` kernel test robot
  2023-09-07 13:37   ` kernel test robot
@ 2023-09-26 18:13   ` Golani, Mitulkumar Ajitkumar
  2 siblings, 0 replies; 21+ messages in thread
From: Golani, Mitulkumar Ajitkumar @ 2023-09-26 18:13 UTC (permalink / raw)
  To: Nikula, Jani, dri-devel@lists.freedesktop.org
  Cc: intel-gfx@lists.freedesktop.org

> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: 07 September 2023 14:58
> To: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> Subject: [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad
> from/to 3-byte sad
> 
> Add helpers to pack/unpack SADs. Both ways and non-static, as follow-up
> work needs them.
> 
> Cc: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c     | 33 ++++++++++++++++++++++++---------
>  drivers/gpu/drm/drm_internal.h |  6 ++++++
>  2 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index fcdc2c314cde..1260e2688bd7 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5499,6 +5499,27 @@ static void clear_eld(struct drm_connector
> *connector)
>  	connector->audio_latency[1] = 0;
>  }
> 
> +/*
> + * Get 3-byte SAD buf from struct cea_sad.
> + */

Just a small nit-pick: 'SAD buffer'.

Otherwise the change looks good to me.
Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>

> +void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad) {
> +	sad[0] = cta_sad->format << 3 | cta_sad->channels;
> +	sad[1] = cta_sad->freq;
> +	sad[2] = cta_sad->byte2;
> +}
> +
> +/*
> + * Set struct cea_sad from 3-byte SAD buf.
> + */
> +void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad) {
> +	cta_sad->format = (sad[0] & 0x78) >> 3;
> +	cta_sad->channels = sad[0] & 0x07;
> +	cta_sad->freq = sad[1] & 0x7f;
> +	cta_sad->byte2 = sad[2];
> +}
> +
>  /*
>   * drm_edid_to_eld - build ELD from EDID
>   * @connector: connector corresponding to the HDMI/DP sink @@ -5593,21
> +5614,15 @@ static int _drm_edid_to_sad(const struct drm_edid
> *drm_edid,
>  	cea_db_iter_for_each(db, &iter) {
>  		if (cea_db_tag(db) == CTA_DB_AUDIO) {
>  			struct cea_sad *sads;
> -			int j;
> +			int i;
> 
>  			count = cea_db_payload_len(db) / 3; /* SAD is 3B */
>  			sads = kcalloc(count, sizeof(*sads), GFP_KERNEL);
>  			*psads = sads;
>  			if (!sads)
>  				return -ENOMEM;
> -			for (j = 0; j < count; j++) {
> -				const u8 *sad = &db->data[j * 3];
> -
> -				sads[j].format = (sad[0] & 0x78) >> 3;
> -				sads[j].channels = sad[0] & 0x7;
> -				sads[j].freq = sad[1] & 0x7F;
> -				sads[j].byte2 = sad[2];
> -			}
> +			for (i = 0; i < count; i++)
> +				drm_edid_cta_sad_set(&sads[i], &db->data[i
> * 3]);
>  			break;
>  		}
>  	}
> diff --git a/drivers/gpu/drm/drm_internal.h
> b/drivers/gpu/drm/drm_internal.h index ab9a472f4a47..5b7c661da459
> 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -22,6 +22,7 @@
>   */
> 
>  #include <linux/kthread.h>
> +#include <linux/types.h>
> 
>  #include <drm/drm_ioctl.h>
>  #include <drm/drm_vblank.h>
> @@ -31,6 +32,7 @@
> 
>  #define DRM_IF_VERSION(maj, min) (maj << 16 | min)
> 
> +struct cea_sad;
>  struct dentry;
>  struct dma_buf;
>  struct iosys_map;
> @@ -265,3 +267,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev,
> void *data,  void drm_framebuffer_print_info(struct drm_printer *p,
> unsigned int indent,
>  				const struct drm_framebuffer *fb);
>  void drm_framebuffer_debugfs_init(struct drm_device *dev);
> +
> +/* drm_edid.c */
> +void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad); void
> +drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad);
> --
> 2.39.2


^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2023-09-26 18:13 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-07  9:28 [Intel-gfx] [PATCH 0/6] drm/edid: split out drm_eld.[ch], add some SAD helpers Jani Nikula
2023-09-07  9:28 ` [Intel-gfx] [PATCH 1/6] drm/edid: split out drm_eld.h from drm_edid.h Jani Nikula
2023-09-25 15:52   ` Golani, Mitulkumar Ajitkumar
2023-09-07  9:28 ` [Intel-gfx] [PATCH 2/6] drm/eld: replace uint8_t with u8 Jani Nikula
2023-09-08  5:25   ` Borah, Chaitanya Kumar
2023-09-25 16:32   ` Golani, Mitulkumar Ajitkumar
2023-09-07  9:28 ` [Intel-gfx] [PATCH 3/6] drm/edid: include drm_eld.h only where required Jani Nikula
2023-09-25 16:59   ` Golani, Mitulkumar Ajitkumar
2023-09-07  9:28 ` [Intel-gfx] [PATCH 4/6] drm/edid: use a temp variable for sads to drop one level of dereferences Jani Nikula
2023-09-25 17:46   ` Golani, Mitulkumar Ajitkumar
2023-09-26  7:44     ` Jani Nikula
2023-09-26 16:14       ` Golani, Mitulkumar Ajitkumar
2023-09-07  9:28 ` [Intel-gfx] [PATCH 5/6] drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad Jani Nikula
2023-09-07 11:49   ` kernel test robot
2023-09-07 13:37   ` kernel test robot
2023-09-26 18:13   ` Golani, Mitulkumar Ajitkumar
2023-09-07  9:28 ` [Intel-gfx] [PATCH 6/6] drm/eld: add helpers to modify the SADs of an ELD Jani Nikula
2023-09-26 18:04   ` Golani, Mitulkumar Ajitkumar
2023-09-07 12:14 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/edid: split out drm_eld.[ch], add some SAD helpers Patchwork
2023-09-07 12:14 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2023-09-07 12:31 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox