[PATCH 0/6] drm, cec and edid updates

[PATCH 0/6] drm, cec and edid updates

[Jani Nikula]

Avoid accessing the raw edid directly. Pre-parse the source physical
address during normal EDID parsing and use that for CEC.

Jani Nikula (6):
  drm/edid: add drm_edid_is_digital()
  drm/i915/display: use drm_edid_is_digital()
  drm/edid: parse source physical address
  drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid
  drm/i915/cec: switch to setting physical address directly
  media: cec: core: add note about *_from_edid() function usage in drm

 drivers/gpu/drm/display/drm_dp_cec.c      | 22 +++++++++++++++++++---
 drivers/gpu/drm/drm_edid.c                | 22 ++++++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_crt.c  | 11 ++++-------
 drivers/gpu/drm/i915/display/intel_dp.c   |  7 ++-----
 drivers/gpu/drm/i915/display/intel_hdmi.c |  8 +++-----
 drivers/gpu/drm/i915/display/intel_sdvo.c |  7 ++-----
 drivers/media/cec/core/cec-adap.c         |  4 ++++
 drivers/media/cec/core/cec-notifier.c     |  4 ++++
 include/drm/display/drm_dp_helper.h       |  6 ++++++
 include/drm/drm_connector.h               |  8 ++++++++
 include/drm/drm_edid.h                    |  1 +
 11 files changed, 73 insertions(+), 27 deletions(-)

-- 
2.39.2

[PATCH 1/6] drm/edid: add drm_edid_is_digital()

[Jani Nikula]

Checking edid->input & DRM_EDID_INPUT_DIGITAL is common enough to
deserve a helper that also lets us abstract the raw EDID a bit better.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 17 +++++++++++++++--
 include/drm/drm_edid.h     |  1 +
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 340da8257b51..1dbb15439468 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3110,7 +3110,7 @@ drm_monitor_supports_rb(const struct drm_edid *drm_edid)
 		return ret;
 	}
 
-	return ((drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL) != 0);
+	return drm_edid_is_digital(drm_edid);
 }
 
 static void
@@ -6519,7 +6519,7 @@ static void update_display_info(struct drm_connector *connector,
 	if (edid->revision < 3)
 		goto out;
 
-	if (!(edid->input & DRM_EDID_INPUT_DIGITAL))
+	if (!drm_edid_is_digital(drm_edid))
 		goto out;
 
 	info->color_formats |= DRM_COLOR_FORMAT_RGB444;
@@ -7335,3 +7335,16 @@ static void _drm_update_tile_info(struct drm_connector *connector,
 		connector->tile_group = NULL;
 	}
 }
+
+/**
+ * drm_edid_is_digital - is digital?
+ * @drm_edid: The EDID
+ *
+ * Return true if input is digital.
+ */
+bool drm_edid_is_digital(const struct drm_edid *drm_edid)
+{
+	return drm_edid && drm_edid->edid &&
+		drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
+}
+EXPORT_SYMBOL(drm_edid_is_digital);
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 48e93f909ef6..882d2638708e 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -612,6 +612,7 @@ const struct drm_edid *drm_edid_read_switcheroo(struct drm_connector *connector,
 int drm_edid_connector_update(struct drm_connector *connector,
 			      const struct drm_edid *edid);
 int drm_edid_connector_add_modes(struct drm_connector *connector);
+bool drm_edid_is_digital(const struct drm_edid *drm_edid);
 
 const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
 				  int ext_id, int *ext_index);
-- 
2.39.2

[PATCH 2/6] drm/i915/display: use drm_edid_is_digital()

[Jani Nikula]

Reduce the use of struct edid and drm_edid_raw().

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_crt.c  | 11 ++++-------
 drivers/gpu/drm/i915/display/intel_hdmi.c |  9 ++++-----
 drivers/gpu/drm/i915/display/intel_sdvo.c |  7 ++-----
 3 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
index f66340b4caf0..310670bb6c25 100644
--- a/drivers/gpu/drm/i915/display/intel_crt.c
+++ b/drivers/gpu/drm/i915/display/intel_crt.c
@@ -657,21 +657,18 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
 	drm_edid = intel_crt_get_edid(connector, i2c);
 
 	if (drm_edid) {
-		const struct edid *edid = drm_edid_raw(drm_edid);
-		bool is_digital = edid->input & DRM_EDID_INPUT_DIGITAL;
-
 		/*
 		 * This may be a DVI-I connector with a shared DDC
 		 * link between analog and digital outputs, so we
 		 * have to check the EDID input spec of the attached device.
 		 */
-		if (!is_digital) {
+		if (drm_edid_is_digital(drm_edid)) {
 			drm_dbg_kms(&dev_priv->drm,
-				    "CRT detected via DDC:0x50 [EDID]\n");
-			ret = true;
+				    "CRT not detected via DDC:0x50 [EDID reports a digital panel]\n");
 		} else {
 			drm_dbg_kms(&dev_priv->drm,
-				    "CRT not detected via DDC:0x50 [EDID reports a digital panel]\n");
+				    "CRT detected via DDC:0x50 [EDID]\n");
+			ret = true;
 		}
 	} else {
 		drm_dbg_kms(&dev_priv->drm,
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 9442bf43550e..aa9915098dda 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2452,7 +2452,6 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(to_intel_connector(connector));
 	intel_wakeref_t wakeref;
 	const struct drm_edid *drm_edid;
-	const struct edid *edid;
 	bool connected = false;
 	struct i2c_adapter *i2c;
 
@@ -2475,9 +2474,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 
 	to_intel_connector(connector)->detect_edid = drm_edid;
 
-	/* FIXME: Get rid of drm_edid_raw() */
-	edid = drm_edid_raw(drm_edid);
-	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
+	if (drm_edid_is_digital(drm_edid)) {
 		intel_hdmi_dp_dual_mode_detect(connector);
 
 		connected = true;
@@ -2485,7 +2482,9 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref);
 
-	cec_notifier_set_phys_addr_from_edid(intel_hdmi->cec_notifier, edid);
+	/* FIXME: Get rid of drm_edid_raw() */
+	cec_notifier_set_phys_addr_from_edid(intel_hdmi->cec_notifier,
+					     drm_edid_raw(drm_edid));
 
 	return connected;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
index 7d25a64698e2..917771e19e38 100644
--- a/drivers/gpu/drm/i915/display/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
@@ -2094,10 +2094,8 @@ intel_sdvo_tmds_sink_detect(struct drm_connector *connector)
 
 	status = connector_status_unknown;
 	if (drm_edid) {
-		const struct edid *edid = drm_edid_raw(drm_edid);
-
 		/* DDC bus is shared, match EDID to connector type */
-		if (edid && edid->input & DRM_EDID_INPUT_DIGITAL)
+		if (drm_edid_is_digital(drm_edid))
 			status = connector_status_connected;
 		else
 			status = connector_status_disconnected;
@@ -2111,8 +2109,7 @@ static bool
 intel_sdvo_connector_matches_edid(struct intel_sdvo_connector *sdvo,
 				  const struct drm_edid *drm_edid)
 {
-	const struct edid *edid = drm_edid_raw(drm_edid);
-	bool monitor_is_digital = !!(edid->input & DRM_EDID_INPUT_DIGITAL);
+	bool monitor_is_digital = drm_edid_is_digital(drm_edid);
 	bool connector_is_digital = !!IS_DIGITAL(sdvo);
 
 	DRM_DEBUG_KMS("connector_is_digital? %d, monitor_is_digital? %d\n",
-- 
2.39.2

[PATCH 3/6] drm/edid: parse source physical address

[Jani Nikula]

CEC needs the source physical address. Parsing it is trivial with the
existing EDID CEA DB infrastructure.

Default to CEC_PHYS_ADDR_INVALID (0xffff) instead of 0 to cater for
easier CEC usage.

Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: linux-media@vger.kernel.org
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 5 +++++
 include/drm/drm_connector.h | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 1dbb15439468..39dd3f694544 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -29,6 +29,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/cec.h>
 #include <linux/hdmi.h>
 #include <linux/i2c.h>
 #include <linux/kernel.h>
@@ -6192,6 +6193,8 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
 
 	info->is_hdmi = true;
 
+	info->source_physical_address = (db[4] << 8) | db[5];
+
 	if (len >= 6)
 		info->dvi_dual = db[6] & 1;
 	if (len >= 7)
@@ -6470,6 +6473,8 @@ static void drm_reset_display_info(struct drm_connector *connector)
 	info->vics_len = 0;
 
 	info->quirks = 0;
+
+	info->source_physical_address = CEC_PHYS_ADDR_INVALID;
 }
 
 static void update_displayid_info(struct drm_connector *connector,
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index d300fde6c1a4..40a5e7acf2fa 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -816,6 +816,14 @@ struct drm_display_info {
 	 * @quirks: EDID based quirks. Internal to EDID parsing.
 	 */
 	u32 quirks;
+
+	/**
+	 * @source_physical_address: Source Physical Address from HDMI
+	 * Vendor-Specific Data Block, for CEC usage.
+	 *
+	 * Defaults to CEC_PHYS_ADDR_INVALID (0xffff).
+	 */
+	u16 source_physical_address;
 };
 
 int drm_display_info_set_bus_formats(struct drm_display_info *info,
-- 
2.39.2

[PATCH 4/6] drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid

[Jani Nikula]

Connectors have source physical address available in display
info. There's no need to parse the EDID again for this. Add
drm_dp_cec_attach() to do this.

Seems like the set_edid/unset_edid naming is a bit specific now that
there's no need to pass the EDID at all, so aim for attach/detach going
forward.

Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: linux-media@vger.kernel.org
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/display/drm_dp_cec.c | 22 +++++++++++++++++++---
 include/drm/display/drm_dp_helper.h  |  6 ++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_cec.c b/drivers/gpu/drm/display/drm_dp_cec.c
index ae39dc794190..da7a7d357446 100644
--- a/drivers/gpu/drm/display/drm_dp_cec.c
+++ b/drivers/gpu/drm/display/drm_dp_cec.c
@@ -297,7 +297,7 @@ static void drm_dp_cec_unregister_work(struct work_struct *work)
  * were unchanged and just update the CEC physical address. Otherwise
  * unregister the old CEC adapter and create a new one.
  */
-void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
+void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address)
 {
 	struct drm_connector *connector = aux->cec.connector;
 	u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
@@ -339,7 +339,7 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
 		if (aux->cec.adap->capabilities == cec_caps &&
 		    aux->cec.adap->available_log_addrs == num_las) {
 			/* Unchanged, so just set the phys addr */
-			cec_s_phys_addr_from_edid(aux->cec.adap, edid);
+			cec_s_phys_addr(adap, source_physical_address, false);
 			goto unlock;
 		}
 		/*
@@ -370,11 +370,27 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
 		 * from drm_dp_cec_register_connector() edid == NULL, so in
 		 * that case the phys addr is just invalidated.
 		 */
-		cec_s_phys_addr_from_edid(aux->cec.adap, edid);
+		cec_s_phys_addr(adap, source_physical_address, false);
 	}
 unlock:
 	mutex_unlock(&aux->cec.lock);
 }
+EXPORT_SYMBOL(drm_dp_cec_attach);
+
+/*
+ * Note: Prefer calling drm_dp_cec_attach() with
+ * connector->display_info.source_physical_address if possible.
+ */
+void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
+{
+	u16 source_physical_address = CEC_PHYS_ADDR_INVALID;
+
+	if (edid && edid->extensions)
+		pa = cec_get_edid_phys_addr((const u8 *)edid,
+					    EDID_LENGTH * (edid->extensions + 1), NULL);
+
+	drm_dp_cec_attach(aux, source_physical_address);
+}
 EXPORT_SYMBOL(drm_dp_cec_set_edid);
 
 /*
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index 86f24a759268..3369104e2d25 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -699,6 +699,7 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux);
 void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
 				   struct drm_connector *connector);
 void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux);
+void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address);
 void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid);
 void drm_dp_cec_unset_edid(struct drm_dp_aux *aux);
 #else
@@ -716,6 +717,11 @@ static inline void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux)
 {
 }
 
+static inline void drm_dp_cec_attach(struct drm_dp_aux *aux,
+				     u16 source_physical_address)
+{
+}
+
 static inline void drm_dp_cec_set_edid(struct drm_dp_aux *aux,
 				       const struct edid *edid)
 {
-- 
2.39.2

[PATCH 5/6] drm/i915/cec: switch to setting physical address directly

[Jani Nikula]

Avoid parsing the EDID again for source physical address. Also gets rids
of a few remaining raw EDID usages.

Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: linux-media@vger.kernel.org
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c   | 7 ++-----
 drivers/gpu/drm/i915/display/intel_hdmi.c | 5 ++---
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 7067ee3a4bd3..c4b8e0e74c15 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5198,7 +5198,6 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 	struct intel_connector *connector = intel_dp->attached_connector;
 	const struct drm_edid *drm_edid;
-	const struct edid *edid;
 	bool vrr_capable;
 
 	intel_dp_unset_edid(intel_dp);
@@ -5216,10 +5215,8 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
 	intel_dp_update_dfp(intel_dp, drm_edid);
 	intel_dp_update_420(intel_dp);
 
-	/* FIXME: Get rid of drm_edid_raw() */
-	edid = drm_edid_raw(drm_edid);
-
-	drm_dp_cec_set_edid(&intel_dp->aux, edid);
+	drm_dp_cec_attach(&intel_dp->aux,
+			  connector->base.display_info.source_physical_address);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index aa9915098dda..5d6255ee8b54 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2482,9 +2482,8 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref);
 
-	/* FIXME: Get rid of drm_edid_raw() */
-	cec_notifier_set_phys_addr_from_edid(intel_hdmi->cec_notifier,
-					     drm_edid_raw(drm_edid));
+	cec_notifier_set_phys_addr(intel_hdmi->cec_notifier,
+				   connector->display_info.source_physical_address);
 
 	return connected;
 }
-- 
2.39.2

[PATCH 6/6] media: cec: core: add note about *_from_edid() function usage in drm

[Jani Nikula]

In the drm subsystem, the source physical address is, in most cases,
available without having to parse the EDID again. Add notes about
preferring to use the pre-parsed address instead.

Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: linux-media@vger.kernel.org
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/media/cec/core/cec-adap.c     | 4 ++++
 drivers/media/cec/core/cec-notifier.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
index 241b1621b197..2c627ed611ed 100644
--- a/drivers/media/cec/core/cec-adap.c
+++ b/drivers/media/cec/core/cec-adap.c
@@ -1688,6 +1688,10 @@ void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
 }
 EXPORT_SYMBOL_GPL(cec_s_phys_addr);
 
+/*
+ * Note: In the drm subsystem, prefer calling cec_s_phys_addr() with
+ * connector->display_info.source_physical_address if possible.
+ */
 void cec_s_phys_addr_from_edid(struct cec_adapter *adap,
 			       const struct edid *edid)
 {
diff --git a/drivers/media/cec/core/cec-notifier.c b/drivers/media/cec/core/cec-notifier.c
index 389dc664b211..13f043b3025b 100644
--- a/drivers/media/cec/core/cec-notifier.c
+++ b/drivers/media/cec/core/cec-notifier.c
@@ -195,6 +195,10 @@ void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa)
 }
 EXPORT_SYMBOL_GPL(cec_notifier_set_phys_addr);
 
+/*
+ * Note: In the drm subsystem, prefer calling cec_notifier_set_phys_addr() with
+ * connector->display_info.source_physical_address if possible.
+ */
 void cec_notifier_set_phys_addr_from_edid(struct cec_notifier *n,
 					  const struct edid *edid)
 {
-- 
2.39.2

Re: [Intel-gfx] [PATCH 4/6] drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid

[kernel test robot]

Hi Jani,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm/drm-next next-20230825]
[cannot apply to drm-intel/for-linux-next-fixes drm-misc/drm-misc-next linus/master v6.5-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jani-Nikula/drm-edid-add-drm_edid_is_digital/20230824-220926
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:    https://lore.kernel.org/r/f8ed9b38fd2ebcd8344a1889a6c0f288969454ea.1692884619.git.jani.nikula%40intel.com
patch subject: [Intel-gfx] [PATCH 4/6] drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid
config: nios2-randconfig-r036-20230824 (https://download.01.org/0day-ci/archive/20230825/202308251937.G5ETiCfF-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230825/202308251937.G5ETiCfF-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/202308251937.G5ETiCfF-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpu/drm/display/drm_dp_cec.c: In function 'drm_dp_cec_attach':
>> drivers/gpu/drm/display/drm_dp_cec.c:342:41: error: 'adap' undeclared (first use in this function)
     342 |                         cec_s_phys_addr(adap, source_physical_address, false);
         |                                         ^~~~
   drivers/gpu/drm/display/drm_dp_cec.c:342:41: note: each undeclared identifier is reported only once for each function it appears in
   drivers/gpu/drm/display/drm_dp_cec.c: In function 'drm_dp_cec_set_edid':
>> drivers/gpu/drm/display/drm_dp_cec.c:388:25: error: invalid use of undefined type 'const struct edid'
     388 |         if (edid && edid->extensions)
         |                         ^~
>> drivers/gpu/drm/display/drm_dp_cec.c:389:17: error: 'pa' undeclared (first use in this function)
     389 |                 pa = cec_get_edid_phys_addr((const u8 *)edid,
         |                 ^~
>> drivers/gpu/drm/display/drm_dp_cec.c:390:45: error: 'EDID_LENGTH' undeclared (first use in this function)
     390 |                                             EDID_LENGTH * (edid->extensions + 1), NULL);
         |                                             ^~~~~~~~~~~
   drivers/gpu/drm/display/drm_dp_cec.c:390:64: error: invalid use of undefined type 'const struct edid'
     390 |                                             EDID_LENGTH * (edid->extensions + 1), NULL);
         |                                                                ^~


vim +/adap +342 drivers/gpu/drm/display/drm_dp_cec.c

   293	
   294	/*
   295	 * A new EDID is set. If there is no CEC adapter, then create one. If
   296	 * there was a CEC adapter, then check if the CEC adapter properties
   297	 * were unchanged and just update the CEC physical address. Otherwise
   298	 * unregister the old CEC adapter and create a new one.
   299	 */
   300	void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address)
   301	{
   302		struct drm_connector *connector = aux->cec.connector;
   303		u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
   304			       CEC_CAP_CONNECTOR_INFO;
   305		struct cec_connector_info conn_info;
   306		unsigned int num_las = 1;
   307		u8 cap;
   308	
   309		/* No transfer function was set, so not a DP connector */
   310		if (!aux->transfer)
   311			return;
   312	
   313	#ifndef CONFIG_MEDIA_CEC_RC
   314		/*
   315		 * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
   316		 * cec_allocate_adapter() if CONFIG_MEDIA_CEC_RC is undefined.
   317		 *
   318		 * Do this here as well to ensure the tests against cec_caps are
   319		 * correct.
   320		 */
   321		cec_caps &= ~CEC_CAP_RC;
   322	#endif
   323		cancel_delayed_work_sync(&aux->cec.unregister_work);
   324	
   325		mutex_lock(&aux->cec.lock);
   326		if (!drm_dp_cec_cap(aux, &cap)) {
   327			/* CEC is not supported, unregister any existing adapter */
   328			cec_unregister_adapter(aux->cec.adap);
   329			aux->cec.adap = NULL;
   330			goto unlock;
   331		}
   332	
   333		if (cap & DP_CEC_SNOOPING_CAPABLE)
   334			cec_caps |= CEC_CAP_MONITOR_ALL;
   335		if (cap & DP_CEC_MULTIPLE_LA_CAPABLE)
   336			num_las = CEC_MAX_LOG_ADDRS;
   337	
   338		if (aux->cec.adap) {
   339			if (aux->cec.adap->capabilities == cec_caps &&
   340			    aux->cec.adap->available_log_addrs == num_las) {
   341				/* Unchanged, so just set the phys addr */
 > 342				cec_s_phys_addr(adap, source_physical_address, false);
   343				goto unlock;
   344			}
   345			/*
   346			 * The capabilities changed, so unregister the old
   347			 * adapter first.
   348			 */
   349			cec_unregister_adapter(aux->cec.adap);
   350		}
   351	
   352		/* Create a new adapter */
   353		aux->cec.adap = cec_allocate_adapter(&drm_dp_cec_adap_ops,
   354						     aux, connector->name, cec_caps,
   355						     num_las);
   356		if (IS_ERR(aux->cec.adap)) {
   357			aux->cec.adap = NULL;
   358			goto unlock;
   359		}
   360	
   361		cec_fill_conn_info_from_drm(&conn_info, connector);
   362		cec_s_conn_info(aux->cec.adap, &conn_info);
   363	
   364		if (cec_register_adapter(aux->cec.adap, connector->dev->dev)) {
   365			cec_delete_adapter(aux->cec.adap);
   366			aux->cec.adap = NULL;
   367		} else {
   368			/*
   369			 * Update the phys addr for the new CEC adapter. When called
   370			 * from drm_dp_cec_register_connector() edid == NULL, so in
   371			 * that case the phys addr is just invalidated.
   372			 */
   373			cec_s_phys_addr(adap, source_physical_address, false);
   374		}
   375	unlock:
   376		mutex_unlock(&aux->cec.lock);
   377	}
   378	EXPORT_SYMBOL(drm_dp_cec_attach);
   379	
   380	/*
   381	 * Note: Prefer calling drm_dp_cec_attach() with
   382	 * connector->display_info.source_physical_address if possible.
   383	 */
   384	void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
   385	{
   386		u16 source_physical_address = CEC_PHYS_ADDR_INVALID;
   387	
 > 388		if (edid && edid->extensions)
 > 389			pa = cec_get_edid_phys_addr((const u8 *)edid,
 > 390						    EDID_LENGTH * (edid->extensions + 1), NULL);
   391	
   392		drm_dp_cec_attach(aux, source_physical_address);
   393	}
   394	EXPORT_SYMBOL(drm_dp_cec_set_edid);
   395	

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

Re: [Intel-gfx] [PATCH 4/6] drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid

[kernel test robot]

Hi Jani,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm/drm-next next-20230825]
[cannot apply to drm-intel/for-linux-next-fixes drm-misc/drm-misc-next linus/master v6.5-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jani-Nikula/drm-edid-add-drm_edid_is_digital/20230824-220926
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:    https://lore.kernel.org/r/f8ed9b38fd2ebcd8344a1889a6c0f288969454ea.1692884619.git.jani.nikula%40intel.com
patch subject: [Intel-gfx] [PATCH 4/6] drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid
config: i386-buildonly-randconfig-001-20230824 (https://download.01.org/0day-ci/archive/20230825/202308251944.9tpk0wma-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230825/202308251944.9tpk0wma-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/202308251944.9tpk0wma-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpu/drm/display/drm_dp_cec.c: In function 'drm_dp_cec_attach':
   drivers/gpu/drm/display/drm_dp_cec.c:342:20: error: 'adap' undeclared (first use in this function)
     342 |    cec_s_phys_addr(adap, source_physical_address, false);
         |                    ^~~~
   drivers/gpu/drm/display/drm_dp_cec.c:342:20: note: each undeclared identifier is reported only once for each function it appears in
   drivers/gpu/drm/display/drm_dp_cec.c: In function 'drm_dp_cec_set_edid':
>> drivers/gpu/drm/display/drm_dp_cec.c:388:18: error: dereferencing pointer to incomplete type 'const struct edid'
     388 |  if (edid && edid->extensions)
         |                  ^~
   drivers/gpu/drm/display/drm_dp_cec.c:389:3: error: 'pa' undeclared (first use in this function)
     389 |   pa = cec_get_edid_phys_addr((const u8 *)edid,
         |   ^~
   drivers/gpu/drm/display/drm_dp_cec.c:390:10: error: 'EDID_LENGTH' undeclared (first use in this function)
     390 |          EDID_LENGTH * (edid->extensions + 1), NULL);
         |          ^~~~~~~~~~~


vim +388 drivers/gpu/drm/display/drm_dp_cec.c

   379	
   380	/*
   381	 * Note: Prefer calling drm_dp_cec_attach() with
   382	 * connector->display_info.source_physical_address if possible.
   383	 */
   384	void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
   385	{
   386		u16 source_physical_address = CEC_PHYS_ADDR_INVALID;
   387	
 > 388		if (edid && edid->extensions)
   389			pa = cec_get_edid_phys_addr((const u8 *)edid,
   390						    EDID_LENGTH * (edid->extensions + 1), NULL);
   391	
   392		drm_dp_cec_attach(aux, source_physical_address);
   393	}
   394	EXPORT_SYMBOL(drm_dp_cec_set_edid);
   395	

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

[PATCH v2] drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid

[Jani Nikula]

Connectors have source physical address available in display
info. There's no need to parse the EDID again for this. Add
drm_dp_cec_attach() to do this.

Seems like the set_edid/unset_edid naming is a bit specific now that
there's no need to pass the EDID at all, so aim for attach/detach going
forward.

v2: Fix the embarrashing build failures

Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: linux-media@vger.kernel.org
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/display/drm_dp_cec.c | 23 ++++++++++++++++++++---
 include/drm/display/drm_dp_helper.h  |  6 ++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_cec.c b/drivers/gpu/drm/display/drm_dp_cec.c
index ae39dc794190..007ceb281d00 100644
--- a/drivers/gpu/drm/display/drm_dp_cec.c
+++ b/drivers/gpu/drm/display/drm_dp_cec.c
@@ -14,6 +14,7 @@
 #include <drm/display/drm_dp_helper.h>
 #include <drm/drm_connector.h>
 #include <drm/drm_device.h>
+#include <drm/drm_edid.h>
 
 /*
  * Unfortunately it turns out that we have a chicken-and-egg situation
@@ -297,7 +298,7 @@ static void drm_dp_cec_unregister_work(struct work_struct *work)
  * were unchanged and just update the CEC physical address. Otherwise
  * unregister the old CEC adapter and create a new one.
  */
-void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
+void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address)
 {
 	struct drm_connector *connector = aux->cec.connector;
 	u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
@@ -339,7 +340,7 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
 		if (aux->cec.adap->capabilities == cec_caps &&
 		    aux->cec.adap->available_log_addrs == num_las) {
 			/* Unchanged, so just set the phys addr */
-			cec_s_phys_addr_from_edid(aux->cec.adap, edid);
+			cec_s_phys_addr(aux->cec.adap, source_physical_address, false);
 			goto unlock;
 		}
 		/*
@@ -370,11 +371,27 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
 		 * from drm_dp_cec_register_connector() edid == NULL, so in
 		 * that case the phys addr is just invalidated.
 		 */
-		cec_s_phys_addr_from_edid(aux->cec.adap, edid);
+		cec_s_phys_addr(aux->cec.adap, source_physical_address, false);
 	}
 unlock:
 	mutex_unlock(&aux->cec.lock);
 }
+EXPORT_SYMBOL(drm_dp_cec_attach);
+
+/*
+ * Note: Prefer calling drm_dp_cec_attach() with
+ * connector->display_info.source_physical_address if possible.
+ */
+void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
+{
+	u16 pa = CEC_PHYS_ADDR_INVALID;
+
+	if (edid && edid->extensions)
+		pa = cec_get_edid_phys_addr((const u8 *)edid,
+					    EDID_LENGTH * (edid->extensions + 1), NULL);
+
+	drm_dp_cec_attach(aux, pa);
+}
 EXPORT_SYMBOL(drm_dp_cec_set_edid);
 
 /*
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index 86f24a759268..3369104e2d25 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -699,6 +699,7 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux);
 void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
 				   struct drm_connector *connector);
 void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux);
+void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address);
 void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid);
 void drm_dp_cec_unset_edid(struct drm_dp_aux *aux);
 #else
@@ -716,6 +717,11 @@ static inline void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux)
 {
 }
 
+static inline void drm_dp_cec_attach(struct drm_dp_aux *aux,
+				     u16 source_physical_address)
+{
+}
+
 static inline void drm_dp_cec_set_edid(struct drm_dp_aux *aux,
 				       const struct edid *edid)
 {
-- 
2.39.2

Re: [PATCH 3/6] drm/edid: parse source physical address

[Hans Verkuil]

On 24/08/2023 15:46, Jani Nikula wrote:
> CEC needs the source physical address. Parsing it is trivial with the
> existing EDID CEA DB infrastructure.
> 
> Default to CEC_PHYS_ADDR_INVALID (0xffff) instead of 0 to cater for
> easier CEC usage.
> 
> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Regards,

	Hans

> ---
>  drivers/gpu/drm/drm_edid.c  | 5 +++++
>  include/drm/drm_connector.h | 8 ++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 1dbb15439468..39dd3f694544 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -29,6 +29,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/cec.h>
>  #include <linux/hdmi.h>
>  #include <linux/i2c.h>
>  #include <linux/kernel.h>
> @@ -6192,6 +6193,8 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
>  
>  	info->is_hdmi = true;
>  
> +	info->source_physical_address = (db[4] << 8) | db[5];
> +
>  	if (len >= 6)
>  		info->dvi_dual = db[6] & 1;
>  	if (len >= 7)
> @@ -6470,6 +6473,8 @@ static void drm_reset_display_info(struct drm_connector *connector)
>  	info->vics_len = 0;
>  
>  	info->quirks = 0;
> +
> +	info->source_physical_address = CEC_PHYS_ADDR_INVALID;
>  }
>  
>  static void update_displayid_info(struct drm_connector *connector,
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index d300fde6c1a4..40a5e7acf2fa 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -816,6 +816,14 @@ struct drm_display_info {
>  	 * @quirks: EDID based quirks. Internal to EDID parsing.
>  	 */
>  	u32 quirks;
> +
> +	/**
> +	 * @source_physical_address: Source Physical Address from HDMI
> +	 * Vendor-Specific Data Block, for CEC usage.
> +	 *
> +	 * Defaults to CEC_PHYS_ADDR_INVALID (0xffff).
> +	 */
> +	u16 source_physical_address;
>  };
>  
>  int drm_display_info_set_bus_formats(struct drm_display_info *info,

Re: [PATCH 4/6] drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid

[Hans Verkuil]

On 24/08/2023 15:46, Jani Nikula wrote:
> Connectors have source physical address available in display
> info. There's no need to parse the EDID again for this. Add
> drm_dp_cec_attach() to do this.
> 
> Seems like the set_edid/unset_edid naming is a bit specific now that
> there's no need to pass the EDID at all, so aim for attach/detach going
> forward.
> 
> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_cec.c | 22 +++++++++++++++++++---
>  include/drm/display/drm_dp_helper.h  |  6 ++++++
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_cec.c b/drivers/gpu/drm/display/drm_dp_cec.c
> index ae39dc794190..da7a7d357446 100644
> --- a/drivers/gpu/drm/display/drm_dp_cec.c
> +++ b/drivers/gpu/drm/display/drm_dp_cec.c
> @@ -297,7 +297,7 @@ static void drm_dp_cec_unregister_work(struct work_struct *work)
>   * were unchanged and just update the CEC physical address. Otherwise
>   * unregister the old CEC adapter and create a new one.
>   */
> -void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> +void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address)
>  {
>  	struct drm_connector *connector = aux->cec.connector;
>  	u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
> @@ -339,7 +339,7 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
>  		if (aux->cec.adap->capabilities == cec_caps &&
>  		    aux->cec.adap->available_log_addrs == num_las) {
>  			/* Unchanged, so just set the phys addr */
> -			cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> +			cec_s_phys_addr(adap, source_physical_address, false);

As the kernel test robot indicated, this does not compile, this should
be aux->cec.adap.

>  			goto unlock;
>  		}
>  		/*
> @@ -370,11 +370,27 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
>  		 * from drm_dp_cec_register_connector() edid == NULL, so in
>  		 * that case the phys addr is just invalidated.
>  		 */
> -		cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> +		cec_s_phys_addr(adap, source_physical_address, false);
>  	}
>  unlock:
>  	mutex_unlock(&aux->cec.lock);
>  }
> +EXPORT_SYMBOL(drm_dp_cec_attach);
> +
> +/*
> + * Note: Prefer calling drm_dp_cec_attach() with
> + * connector->display_info.source_physical_address if possible.
> + */
> +void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> +{
> +	u16 source_physical_address = CEC_PHYS_ADDR_INVALID;
> +
> +	if (edid && edid->extensions)

And this source needs to include <drm/drm_edid.h>, also as found by
the kernel test robot.

Regards,

	Hans

> +		pa = cec_get_edid_phys_addr((const u8 *)edid,
> +					    EDID_LENGTH * (edid->extensions + 1), NULL);
> +
> +	drm_dp_cec_attach(aux, source_physical_address);
> +}
>  EXPORT_SYMBOL(drm_dp_cec_set_edid);
>  
>  /*
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index 86f24a759268..3369104e2d25 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -699,6 +699,7 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux);
>  void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
>  				   struct drm_connector *connector);
>  void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux);
> +void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address);
>  void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid);
>  void drm_dp_cec_unset_edid(struct drm_dp_aux *aux);
>  #else
> @@ -716,6 +717,11 @@ static inline void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux)
>  {
>  }
>  
> +static inline void drm_dp_cec_attach(struct drm_dp_aux *aux,
> +				     u16 source_physical_address)
> +{
> +}
> +
>  static inline void drm_dp_cec_set_edid(struct drm_dp_aux *aux,
>  				       const struct edid *edid)
>  {

Re: [PATCH 5/6] drm/i915/cec: switch to setting physical address directly

[Hans Verkuil]

On 24/08/2023 15:46, Jani Nikula wrote:
> Avoid parsing the EDID again for source physical address. Also gets rids
> of a few remaining raw EDID usages.
> 
> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Regards,

	Hans

> ---
>  drivers/gpu/drm/i915/display/intel_dp.c   | 7 ++-----
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 5 ++---
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 7067ee3a4bd3..c4b8e0e74c15 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5198,7 +5198,6 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  	struct intel_connector *connector = intel_dp->attached_connector;
>  	const struct drm_edid *drm_edid;
> -	const struct edid *edid;
>  	bool vrr_capable;
>  
>  	intel_dp_unset_edid(intel_dp);
> @@ -5216,10 +5215,8 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
>  	intel_dp_update_dfp(intel_dp, drm_edid);
>  	intel_dp_update_420(intel_dp);
>  
> -	/* FIXME: Get rid of drm_edid_raw() */
> -	edid = drm_edid_raw(drm_edid);
> -
> -	drm_dp_cec_set_edid(&intel_dp->aux, edid);
> +	drm_dp_cec_attach(&intel_dp->aux,
> +			  connector->base.display_info.source_physical_address);
>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index aa9915098dda..5d6255ee8b54 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2482,9 +2482,8 @@ intel_hdmi_set_edid(struct drm_connector *connector)
>  
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref);
>  
> -	/* FIXME: Get rid of drm_edid_raw() */
> -	cec_notifier_set_phys_addr_from_edid(intel_hdmi->cec_notifier,
> -					     drm_edid_raw(drm_edid));
> +	cec_notifier_set_phys_addr(intel_hdmi->cec_notifier,
> +				   connector->display_info.source_physical_address);
>  
>  	return connected;
>  }

Re: [PATCH 6/6] media: cec: core: add note about *_from_edid() function usage in drm

[Hans Verkuil]

On 24/08/2023 15:46, Jani Nikula wrote:
> In the drm subsystem, the source physical address is, in most cases,
> available without having to parse the EDID again. Add notes about
> preferring to use the pre-parsed address instead.
> 
> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/media/cec/core/cec-adap.c     | 4 ++++
>  drivers/media/cec/core/cec-notifier.c | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
> index 241b1621b197..2c627ed611ed 100644
> --- a/drivers/media/cec/core/cec-adap.c
> +++ b/drivers/media/cec/core/cec-adap.c
> @@ -1688,6 +1688,10 @@ void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
>  }
>  EXPORT_SYMBOL_GPL(cec_s_phys_addr);
>  
> +/*
> + * Note: In the drm subsystem, prefer calling cec_s_phys_addr() with
> + * connector->display_info.source_physical_address if possible.
> + */

I would rephrase this:

/*
 * Note: in the drm subsystem, prefer calling (if possible):
 *
 * cec_s_phys_addr(adap, connector->display_info.source_physical_address, false);
 */

I think it is important to indicate that the last argument should be 'false'.

>  void cec_s_phys_addr_from_edid(struct cec_adapter *adap,
>  			       const struct edid *edid)
>  {
> diff --git a/drivers/media/cec/core/cec-notifier.c b/drivers/media/cec/core/cec-notifier.c
> index 389dc664b211..13f043b3025b 100644
> --- a/drivers/media/cec/core/cec-notifier.c
> +++ b/drivers/media/cec/core/cec-notifier.c
> @@ -195,6 +195,10 @@ void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa)
>  }
>  EXPORT_SYMBOL_GPL(cec_notifier_set_phys_addr);
>  
> +/*
> + * Note: In the drm subsystem, prefer calling cec_notifier_set_phys_addr() with
> + * connector->display_info.source_physical_address if possible.
> + */

This comment is fine, there is no similar last argument here. But perhaps
it is good to use a similar format as above. Up to you.

>  void cec_notifier_set_phys_addr_from_edid(struct cec_notifier *n,
>  					  const struct edid *edid)
>  {

Regards,

	Hans

Re: [PATCH 4/6] drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid

[Jani Nikula]

On Wed, 30 Aug 2023, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> On 24/08/2023 15:46, Jani Nikula wrote:
>> Connectors have source physical address available in display
>> info. There's no need to parse the EDID again for this. Add
>> drm_dp_cec_attach() to do this.
>> 
>> Seems like the set_edid/unset_edid naming is a bit specific now that
>> there's no need to pass the EDID at all, so aim for attach/detach going
>> forward.
>> 
>> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Cc: linux-media@vger.kernel.org
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/display/drm_dp_cec.c | 22 +++++++++++++++++++---
>>  include/drm/display/drm_dp_helper.h  |  6 ++++++
>>  2 files changed, 25 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/display/drm_dp_cec.c b/drivers/gpu/drm/display/drm_dp_cec.c
>> index ae39dc794190..da7a7d357446 100644
>> --- a/drivers/gpu/drm/display/drm_dp_cec.c
>> +++ b/drivers/gpu/drm/display/drm_dp_cec.c
>> @@ -297,7 +297,7 @@ static void drm_dp_cec_unregister_work(struct work_struct *work)
>>   * were unchanged and just update the CEC physical address. Otherwise
>>   * unregister the old CEC adapter and create a new one.
>>   */
>> -void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
>> +void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address)
>>  {
>>  	struct drm_connector *connector = aux->cec.connector;
>>  	u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
>> @@ -339,7 +339,7 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
>>  		if (aux->cec.adap->capabilities == cec_caps &&
>>  		    aux->cec.adap->available_log_addrs == num_las) {
>>  			/* Unchanged, so just set the phys addr */
>> -			cec_s_phys_addr_from_edid(aux->cec.adap, edid);
>> +			cec_s_phys_addr(adap, source_physical_address, false);
>
> As the kernel test robot indicated, this does not compile, this should
> be aux->cec.adap.
>
>>  			goto unlock;
>>  		}
>>  		/*
>> @@ -370,11 +370,27 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
>>  		 * from drm_dp_cec_register_connector() edid == NULL, so in
>>  		 * that case the phys addr is just invalidated.
>>  		 */
>> -		cec_s_phys_addr_from_edid(aux->cec.adap, edid);
>> +		cec_s_phys_addr(adap, source_physical_address, false);
>>  	}
>>  unlock:
>>  	mutex_unlock(&aux->cec.lock);
>>  }
>> +EXPORT_SYMBOL(drm_dp_cec_attach);
>> +
>> +/*
>> + * Note: Prefer calling drm_dp_cec_attach() with
>> + * connector->display_info.source_physical_address if possible.
>> + */
>> +void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
>> +{
>> +	u16 source_physical_address = CEC_PHYS_ADDR_INVALID;
>> +
>> +	if (edid && edid->extensions)
>
> And this source needs to include <drm/drm_edid.h>, also as found by
> the kernel test robot.

Yes, very embarrassing, I sent the v2 in reply [1].

BR,
Jani.

[1] https://patchwork.freedesktop.org/patch/msgid/20230825130120.1250089-1-jani.nikula@intel.com


>
> Regards,
>
> 	Hans
>
>> +		pa = cec_get_edid_phys_addr((const u8 *)edid,
>> +					    EDID_LENGTH * (edid->extensions + 1), NULL);
>> +
>> +	drm_dp_cec_attach(aux, source_physical_address);
>> +}
>>  EXPORT_SYMBOL(drm_dp_cec_set_edid);
>>  
>>  /*
>> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
>> index 86f24a759268..3369104e2d25 100644
>> --- a/include/drm/display/drm_dp_helper.h
>> +++ b/include/drm/display/drm_dp_helper.h
>> @@ -699,6 +699,7 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux);
>>  void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
>>  				   struct drm_connector *connector);
>>  void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux);
>> +void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address);
>>  void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid);
>>  void drm_dp_cec_unset_edid(struct drm_dp_aux *aux);
>>  #else
>> @@ -716,6 +717,11 @@ static inline void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux)
>>  {
>>  }
>>  
>> +static inline void drm_dp_cec_attach(struct drm_dp_aux *aux,
>> +				     u16 source_physical_address)
>> +{
>> +}
>> +
>>  static inline void drm_dp_cec_set_edid(struct drm_dp_aux *aux,
>>  				       const struct edid *edid)
>>  {
>

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2] drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid

[Hans Verkuil]

Hi Jani,

Sorry, I missed the v2.

On 25/08/2023 15:01, Jani Nikula wrote:
> Connectors have source physical address available in display
> info. There's no need to parse the EDID again for this. Add
> drm_dp_cec_attach() to do this.
> 
> Seems like the set_edid/unset_edid naming is a bit specific now that
> there's no need to pass the EDID at all, so aim for attach/detach going
> forward.
> 
> v2: Fix the embarrashing build failures
> 
> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Regards,

	Hans

> ---
>  drivers/gpu/drm/display/drm_dp_cec.c | 23 ++++++++++++++++++++---
>  include/drm/display/drm_dp_helper.h  |  6 ++++++
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_cec.c b/drivers/gpu/drm/display/drm_dp_cec.c
> index ae39dc794190..007ceb281d00 100644
> --- a/drivers/gpu/drm/display/drm_dp_cec.c
> +++ b/drivers/gpu/drm/display/drm_dp_cec.c
> @@ -14,6 +14,7 @@
>  #include <drm/display/drm_dp_helper.h>
>  #include <drm/drm_connector.h>
>  #include <drm/drm_device.h>
> +#include <drm/drm_edid.h>
>  
>  /*
>   * Unfortunately it turns out that we have a chicken-and-egg situation
> @@ -297,7 +298,7 @@ static void drm_dp_cec_unregister_work(struct work_struct *work)
>   * were unchanged and just update the CEC physical address. Otherwise
>   * unregister the old CEC adapter and create a new one.
>   */
> -void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> +void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address)
>  {
>  	struct drm_connector *connector = aux->cec.connector;
>  	u32 cec_caps = CEC_CAP_DEFAULTS | CEC_CAP_NEEDS_HPD |
> @@ -339,7 +340,7 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
>  		if (aux->cec.adap->capabilities == cec_caps &&
>  		    aux->cec.adap->available_log_addrs == num_las) {
>  			/* Unchanged, so just set the phys addr */
> -			cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> +			cec_s_phys_addr(aux->cec.adap, source_physical_address, false);
>  			goto unlock;
>  		}
>  		/*
> @@ -370,11 +371,27 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
>  		 * from drm_dp_cec_register_connector() edid == NULL, so in
>  		 * that case the phys addr is just invalidated.
>  		 */
> -		cec_s_phys_addr_from_edid(aux->cec.adap, edid);
> +		cec_s_phys_addr(aux->cec.adap, source_physical_address, false);
>  	}
>  unlock:
>  	mutex_unlock(&aux->cec.lock);
>  }
> +EXPORT_SYMBOL(drm_dp_cec_attach);
> +
> +/*
> + * Note: Prefer calling drm_dp_cec_attach() with
> + * connector->display_info.source_physical_address if possible.
> + */
> +void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
> +{
> +	u16 pa = CEC_PHYS_ADDR_INVALID;
> +
> +	if (edid && edid->extensions)
> +		pa = cec_get_edid_phys_addr((const u8 *)edid,
> +					    EDID_LENGTH * (edid->extensions + 1), NULL);
> +
> +	drm_dp_cec_attach(aux, pa);
> +}
>  EXPORT_SYMBOL(drm_dp_cec_set_edid);
>  
>  /*
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index 86f24a759268..3369104e2d25 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -699,6 +699,7 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux);
>  void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
>  				   struct drm_connector *connector);
>  void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux);
> +void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address);
>  void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid);
>  void drm_dp_cec_unset_edid(struct drm_dp_aux *aux);
>  #else
> @@ -716,6 +717,11 @@ static inline void drm_dp_cec_unregister_connector(struct drm_dp_aux *aux)
>  {
>  }
>  
> +static inline void drm_dp_cec_attach(struct drm_dp_aux *aux,
> +				     u16 source_physical_address)
> +{
> +}
> +
>  static inline void drm_dp_cec_set_edid(struct drm_dp_aux *aux,
>  				       const struct edid *edid)
>  {

[PATCH v2] media: cec: core: add note about *_from_edid() function usage in drm

[Jani Nikula]

In the drm subsystem, the source physical address is, in most cases,
available without having to parse the EDID again. Add notes about
preferring to use the pre-parsed address instead.

Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: linux-media@vger.kernel.org
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

v2: rephrase comments, in particular indicate cec_s_phys_addr() should
be false (Hans)
---
 drivers/media/cec/core/cec-adap.c     | 5 +++++
 drivers/media/cec/core/cec-notifier.c | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
index 241b1621b197..1109af525c35 100644
--- a/drivers/media/cec/core/cec-adap.c
+++ b/drivers/media/cec/core/cec-adap.c
@@ -1688,6 +1688,11 @@ void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
 }
 EXPORT_SYMBOL_GPL(cec_s_phys_addr);
 
+/*
+ * Note: In the drm subsystem, prefer calling (if possible):
+ *
+ * cec_s_phys_addr(adap, connector->display_info.source_physical_address, false);
+ */
 void cec_s_phys_addr_from_edid(struct cec_adapter *adap,
 			       const struct edid *edid)
 {
diff --git a/drivers/media/cec/core/cec-notifier.c b/drivers/media/cec/core/cec-notifier.c
index 389dc664b211..d600be0f7b67 100644
--- a/drivers/media/cec/core/cec-notifier.c
+++ b/drivers/media/cec/core/cec-notifier.c
@@ -195,6 +195,11 @@ void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa)
 }
 EXPORT_SYMBOL_GPL(cec_notifier_set_phys_addr);
 
+/*
+ * Note: In the drm subsystem, prefer calling (if possible):
+ *
+ * cec_notifier_set_phys_addr(n, connector->display_info.source_physical_address);
+ */
 void cec_notifier_set_phys_addr_from_edid(struct cec_notifier *n,
 					  const struct edid *edid)
 {
-- 
2.39.2

Re: [PATCH 6/6] media: cec: core: add note about *_from_edid() function usage in drm

[Jani Nikula]

On Wed, 30 Aug 2023, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> On 24/08/2023 15:46, Jani Nikula wrote:
>> In the drm subsystem, the source physical address is, in most cases,
>> available without having to parse the EDID again. Add notes about
>> preferring to use the pre-parsed address instead.
>> 
>> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Cc: linux-media@vger.kernel.org
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/media/cec/core/cec-adap.c     | 4 ++++
>>  drivers/media/cec/core/cec-notifier.c | 4 ++++
>>  2 files changed, 8 insertions(+)
>> 
>> diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
>> index 241b1621b197..2c627ed611ed 100644
>> --- a/drivers/media/cec/core/cec-adap.c
>> +++ b/drivers/media/cec/core/cec-adap.c
>> @@ -1688,6 +1688,10 @@ void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
>>  }
>>  EXPORT_SYMBOL_GPL(cec_s_phys_addr);
>>  
>> +/*
>> + * Note: In the drm subsystem, prefer calling cec_s_phys_addr() with
>> + * connector->display_info.source_physical_address if possible.
>> + */
>
> I would rephrase this:
>
> /*
>  * Note: in the drm subsystem, prefer calling (if possible):
>  *
>  * cec_s_phys_addr(adap, connector->display_info.source_physical_address, false);
>  */
>
> I think it is important to indicate that the last argument should be 'false'.

Agreed.

>
>>  void cec_s_phys_addr_from_edid(struct cec_adapter *adap,
>>  			       const struct edid *edid)
>>  {
>> diff --git a/drivers/media/cec/core/cec-notifier.c b/drivers/media/cec/core/cec-notifier.c
>> index 389dc664b211..13f043b3025b 100644
>> --- a/drivers/media/cec/core/cec-notifier.c
>> +++ b/drivers/media/cec/core/cec-notifier.c
>> @@ -195,6 +195,10 @@ void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa)
>>  }
>>  EXPORT_SYMBOL_GPL(cec_notifier_set_phys_addr);
>>  
>> +/*
>> + * Note: In the drm subsystem, prefer calling cec_notifier_set_phys_addr() with
>> + * connector->display_info.source_physical_address if possible.
>> + */
>
> This comment is fine, there is no similar last argument here. But perhaps
> it is good to use a similar format as above. Up to you.

Thanks, rephrased both in v2.

BR,
Jani.


>
>>  void cec_notifier_set_phys_addr_from_edid(struct cec_notifier *n,
>>  					  const struct edid *edid)
>>  {
>
> Regards,
>
> 	Hans

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2] media: cec: core: add note about *_from_edid() function usage in drm

[Hans Verkuil]

On 31/08/2023 12:51, Jani Nikula wrote:
> In the drm subsystem, the source physical address is, in most cases,
> available without having to parse the EDID again. Add notes about
> preferring to use the pre-parsed address instead.
> 
> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Thanks!

	Hans

> 
> ---
> 
> v2: rephrase comments, in particular indicate cec_s_phys_addr() should
> be false (Hans)
> ---
>  drivers/media/cec/core/cec-adap.c     | 5 +++++
>  drivers/media/cec/core/cec-notifier.c | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
> index 241b1621b197..1109af525c35 100644
> --- a/drivers/media/cec/core/cec-adap.c
> +++ b/drivers/media/cec/core/cec-adap.c
> @@ -1688,6 +1688,11 @@ void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
>  }
>  EXPORT_SYMBOL_GPL(cec_s_phys_addr);
>  
> +/*
> + * Note: In the drm subsystem, prefer calling (if possible):
> + *
> + * cec_s_phys_addr(adap, connector->display_info.source_physical_address, false);
> + */
>  void cec_s_phys_addr_from_edid(struct cec_adapter *adap,
>  			       const struct edid *edid)
>  {
> diff --git a/drivers/media/cec/core/cec-notifier.c b/drivers/media/cec/core/cec-notifier.c
> index 389dc664b211..d600be0f7b67 100644
> --- a/drivers/media/cec/core/cec-notifier.c
> +++ b/drivers/media/cec/core/cec-notifier.c
> @@ -195,6 +195,11 @@ void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa)
>  }
>  EXPORT_SYMBOL_GPL(cec_notifier_set_phys_addr);
>  
> +/*
> + * Note: In the drm subsystem, prefer calling (if possible):
> + *
> + * cec_notifier_set_phys_addr(n, connector->display_info.source_physical_address);
> + */
>  void cec_notifier_set_phys_addr_from_edid(struct cec_notifier *n,
>  					  const struct edid *edid)
>  {

Re: [PATCH 1/6] drm/edid: add drm_edid_is_digital()

[Ville Syrjälä]

On Thu, Aug 24, 2023 at 04:46:02PM +0300, Jani Nikula wrote:
> Checking edid->input & DRM_EDID_INPUT_DIGITAL is common enough to
> deserve a helper that also lets us abstract the raw EDID a bit better.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_edid.c | 17 +++++++++++++++--
>  include/drm/drm_edid.h     |  1 +
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 340da8257b51..1dbb15439468 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3110,7 +3110,7 @@ drm_monitor_supports_rb(const struct drm_edid *drm_edid)
>  		return ret;
>  	}
>  
> -	return ((drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL) != 0);
> +	return drm_edid_is_digital(drm_edid);
>  }
>  
>  static void
> @@ -6519,7 +6519,7 @@ static void update_display_info(struct drm_connector *connector,
>  	if (edid->revision < 3)
>  		goto out;
>  
> -	if (!(edid->input & DRM_EDID_INPUT_DIGITAL))
> +	if (!drm_edid_is_digital(drm_edid))
>  		goto out;
>  
>  	info->color_formats |= DRM_COLOR_FORMAT_RGB444;
> @@ -7335,3 +7335,16 @@ static void _drm_update_tile_info(struct drm_connector *connector,
>  		connector->tile_group = NULL;
>  	}
>  }
> +
> +/**
> + * drm_edid_is_digital - is digital?
> + * @drm_edid: The EDID
> + *
> + * Return true if input is digital.
> + */
> +bool drm_edid_is_digital(const struct drm_edid *drm_edid)
> +{
> +	return drm_edid && drm_edid->edid &&
> +		drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
> +}
> +EXPORT_SYMBOL(drm_edid_is_digital);
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 48e93f909ef6..882d2638708e 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -612,6 +612,7 @@ const struct drm_edid *drm_edid_read_switcheroo(struct drm_connector *connector,
>  int drm_edid_connector_update(struct drm_connector *connector,
>  			      const struct drm_edid *edid);
>  int drm_edid_connector_add_modes(struct drm_connector *connector);
> +bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>  
>  const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>  				  int ext_id, int *ext_index);
> -- 
> 2.39.2

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/6] drm/i915/display: use drm_edid_is_digital()

[Ville Syrjälä]

On Thu, Aug 24, 2023 at 04:46:03PM +0300, Jani Nikula wrote:
> Reduce the use of struct edid and drm_edid_raw().
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_crt.c  | 11 ++++-------
>  drivers/gpu/drm/i915/display/intel_hdmi.c |  9 ++++-----
>  drivers/gpu/drm/i915/display/intel_sdvo.c |  7 ++-----
>  3 files changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
> index f66340b4caf0..310670bb6c25 100644
> --- a/drivers/gpu/drm/i915/display/intel_crt.c
> +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> @@ -657,21 +657,18 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
>  	drm_edid = intel_crt_get_edid(connector, i2c);
>  
>  	if (drm_edid) {
> -		const struct edid *edid = drm_edid_raw(drm_edid);
> -		bool is_digital = edid->input & DRM_EDID_INPUT_DIGITAL;
> -
>  		/*
>  		 * This may be a DVI-I connector with a shared DDC
>  		 * link between analog and digital outputs, so we
>  		 * have to check the EDID input spec of the attached device.
>  		 */
> -		if (!is_digital) {
> +		if (drm_edid_is_digital(drm_edid)) {
>  			drm_dbg_kms(&dev_priv->drm,
> -				    "CRT detected via DDC:0x50 [EDID]\n");
> -			ret = true;
> +				    "CRT not detected via DDC:0x50 [EDID reports a digital panel]\n");
>  		} else {
>  			drm_dbg_kms(&dev_priv->drm,
> -				    "CRT not detected via DDC:0x50 [EDID reports a digital panel]\n");
> +				    "CRT detected via DDC:0x50 [EDID]\n");
> +			ret = true;

Inverting the check made the diff a bit confusing, but looks
correct in the end.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  		}
>  	} else {
>  		drm_dbg_kms(&dev_priv->drm,
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 9442bf43550e..aa9915098dda 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2452,7 +2452,6 @@ intel_hdmi_set_edid(struct drm_connector *connector)
>  	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(to_intel_connector(connector));
>  	intel_wakeref_t wakeref;
>  	const struct drm_edid *drm_edid;
> -	const struct edid *edid;
>  	bool connected = false;
>  	struct i2c_adapter *i2c;
>  
> @@ -2475,9 +2474,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
>  
>  	to_intel_connector(connector)->detect_edid = drm_edid;
>  
> -	/* FIXME: Get rid of drm_edid_raw() */
> -	edid = drm_edid_raw(drm_edid);
> -	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> +	if (drm_edid_is_digital(drm_edid)) {
>  		intel_hdmi_dp_dual_mode_detect(connector);
>  
>  		connected = true;
> @@ -2485,7 +2482,9 @@ intel_hdmi_set_edid(struct drm_connector *connector)
>  
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref);
>  
> -	cec_notifier_set_phys_addr_from_edid(intel_hdmi->cec_notifier, edid);
> +	/* FIXME: Get rid of drm_edid_raw() */
> +	cec_notifier_set_phys_addr_from_edid(intel_hdmi->cec_notifier,
> +					     drm_edid_raw(drm_edid));
>  
>  	return connected;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 7d25a64698e2..917771e19e38 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -2094,10 +2094,8 @@ intel_sdvo_tmds_sink_detect(struct drm_connector *connector)
>  
>  	status = connector_status_unknown;
>  	if (drm_edid) {
> -		const struct edid *edid = drm_edid_raw(drm_edid);
> -
>  		/* DDC bus is shared, match EDID to connector type */
> -		if (edid && edid->input & DRM_EDID_INPUT_DIGITAL)
> +		if (drm_edid_is_digital(drm_edid))
>  			status = connector_status_connected;
>  		else
>  			status = connector_status_disconnected;
> @@ -2111,8 +2109,7 @@ static bool
>  intel_sdvo_connector_matches_edid(struct intel_sdvo_connector *sdvo,
>  				  const struct drm_edid *drm_edid)
>  {
> -	const struct edid *edid = drm_edid_raw(drm_edid);
> -	bool monitor_is_digital = !!(edid->input & DRM_EDID_INPUT_DIGITAL);
> +	bool monitor_is_digital = drm_edid_is_digital(drm_edid);
>  	bool connector_is_digital = !!IS_DIGITAL(sdvo);
>  
>  	DRM_DEBUG_KMS("connector_is_digital? %d, monitor_is_digital? %d\n",
> -- 
> 2.39.2

-- 
Ville Syrjälä
Intel

Re: [PATCH 0/6] drm, cec and edid updates

[Jani Nikula]

On Thu, 24 Aug 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> Avoid accessing the raw edid directly. Pre-parse the source physical
> address during normal EDID parsing and use that for CEC.
>
> Jani Nikula (6):
>   drm/edid: add drm_edid_is_digital()
>   drm/i915/display: use drm_edid_is_digital()
>   drm/edid: parse source physical address
>   drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid
>   drm/i915/cec: switch to setting physical address directly

Maarten, Maxime, Thomas, ack for merging patches 1, 3 and 4 via via
drm-intel?

>   media: cec: core: add note about *_from_edid() function usage in drm

Hans, while there's no build dependency here, I think it would make
sense to merge this together with patches 3 and 4. Ack for merging via
drm-intel?

Thanks,
Jani.


>
>  drivers/gpu/drm/display/drm_dp_cec.c      | 22 +++++++++++++++++++---
>  drivers/gpu/drm/drm_edid.c                | 22 ++++++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_crt.c  | 11 ++++-------
>  drivers/gpu/drm/i915/display/intel_dp.c   |  7 ++-----
>  drivers/gpu/drm/i915/display/intel_hdmi.c |  8 +++-----
>  drivers/gpu/drm/i915/display/intel_sdvo.c |  7 ++-----
>  drivers/media/cec/core/cec-adap.c         |  4 ++++
>  drivers/media/cec/core/cec-notifier.c     |  4 ++++
>  include/drm/display/drm_dp_helper.h       |  6 ++++++
>  include/drm/drm_connector.h               |  8 ++++++++
>  include/drm/drm_edid.h                    |  1 +
>  11 files changed, 73 insertions(+), 27 deletions(-)

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 0/6] drm, cec and edid updates

[Hans Verkuil]

On 31/08/2023 20:51, Jani Nikula wrote:
> On Thu, 24 Aug 2023, Jani Nikula <jani.nikula@intel.com> wrote:
>> Avoid accessing the raw edid directly. Pre-parse the source physical
>> address during normal EDID parsing and use that for CEC.
>>
>> Jani Nikula (6):
>>   drm/edid: add drm_edid_is_digital()
>>   drm/i915/display: use drm_edid_is_digital()
>>   drm/edid: parse source physical address
>>   drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid
>>   drm/i915/cec: switch to setting physical address directly
> 
> Maarten, Maxime, Thomas, ack for merging patches 1, 3 and 4 via via
> drm-intel?
> 
>>   media: cec: core: add note about *_from_edid() function usage in drm
> 
> Hans, while there's no build dependency here, I think it would make
> sense to merge this together with patches 3 and 4. Ack for merging via
> drm-intel?

That's fine, it makes sense to do that.

If you need it, for this series:

Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Regards,

	Hans

> 
> Thanks,
> Jani.
> 
> 
>>
>>  drivers/gpu/drm/display/drm_dp_cec.c      | 22 +++++++++++++++++++---
>>  drivers/gpu/drm/drm_edid.c                | 22 ++++++++++++++++++++--
>>  drivers/gpu/drm/i915/display/intel_crt.c  | 11 ++++-------
>>  drivers/gpu/drm/i915/display/intel_dp.c   |  7 ++-----
>>  drivers/gpu/drm/i915/display/intel_hdmi.c |  8 +++-----
>>  drivers/gpu/drm/i915/display/intel_sdvo.c |  7 ++-----
>>  drivers/media/cec/core/cec-adap.c         |  4 ++++
>>  drivers/media/cec/core/cec-notifier.c     |  4 ++++
>>  include/drm/display/drm_dp_helper.h       |  6 ++++++
>>  include/drm/drm_connector.h               |  8 ++++++++
>>  include/drm/drm_edid.h                    |  1 +
>>  11 files changed, 73 insertions(+), 27 deletions(-)
> 

Re: [PATCH 0/6] drm, cec and edid updates

[Maxime Ripard]

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

On Thu, Aug 31, 2023 at 09:51:24PM +0300, Jani Nikula wrote:
> On Thu, 24 Aug 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> > Avoid accessing the raw edid directly. Pre-parse the source physical
> > address during normal EDID parsing and use that for CEC.
> >
> > Jani Nikula (6):
> >   drm/edid: add drm_edid_is_digital()
> >   drm/i915/display: use drm_edid_is_digital()
> >   drm/edid: parse source physical address
> >   drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid
> >   drm/i915/cec: switch to setting physical address directly
> 
> Maarten, Maxime, Thomas, ack for merging patches 1, 3 and 4 via via
> drm-intel?

Acked-by: Maxime Ripard <mripard@kernel.org>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/6] drm, cec and edid updates

[Jani Nikula]

On Fri, 01 Sep 2023, Maxime Ripard <mripard@kernel.org> wrote:
> On Thu, Aug 31, 2023 at 09:51:24PM +0300, Jani Nikula wrote:
>> On Thu, 24 Aug 2023, Jani Nikula <jani.nikula@intel.com> wrote:
>> > Avoid accessing the raw edid directly. Pre-parse the source physical
>> > address during normal EDID parsing and use that for CEC.
>> >
>> > Jani Nikula (6):
>> >   drm/edid: add drm_edid_is_digital()
>> >   drm/i915/display: use drm_edid_is_digital()
>> >   drm/edid: parse source physical address
>> >   drm/cec: add drm_dp_cec_attach() as the non-edid version of set edid
>> >   drm/i915/cec: switch to setting physical address directly
>> 
>> Maarten, Maxime, Thomas, ack for merging patches 1, 3 and 4 via via
>> drm-intel?
>
> Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks for all the reviews and acks, pushed the series to
drm-intel-next.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 1/6] drm/edid: add drm_edid_is_digital()

[Alex Deucher]

On Thu, Aug 24, 2023 at 9:46 AM Jani Nikula <jani.nikula@intel.com> wrote:
>
> Checking edid->input & DRM_EDID_INPUT_DIGITAL is common enough to
> deserve a helper that also lets us abstract the raw EDID a bit better.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Seems to be a few additional users of this that could be converted:

drivers/gpu/drm/i915/display/intel_sdvo.c:        if (edid &&
edid->input & DRM_EDID_INPUT_DIGITAL)
drivers/gpu/drm/i915/display/intel_sdvo.c:    bool monitor_is_digital
= !!(edid->input & DRM_EDID_INPUT_DIGITAL);
drivers/gpu/drm/i915/display/intel_crt.c:        bool is_digital =
edid->input & DRM_EDID_INPUT_DIGITAL;
drivers/gpu/drm/i915/display/intel_hdmi.c:    if (edid && edid->input
& DRM_EDID_INPUT_DIGITAL) {
drivers/gpu/drm/gma500/psb_intel_sdvo.c:        if (edid->input &
DRM_EDID_INPUT_DIGITAL) {
drivers/gpu/drm/gma500/psb_intel_sdvo.c:            if (edid->input &
DRM_EDID_INPUT_DIGITAL)
drivers/gpu/drm/gma500/psb_intel_sdvo.c:        bool
monitor_is_digital = !!(edid->input & DRM_EDID_INPUT_DIGITAL);
drivers/gpu/drm/gma500/psb_intel_sdvo.c:    if (edid != NULL &&
edid->input & DRM_EDID_INPUT_DIGITAL)
drivers/gpu/drm/gma500/cdv_intel_hdmi.c:        if (edid->input &
DRM_EDID_INPUT_DIGITAL) {
drivers/gpu/drm/display/drm_dp_helper.c:        edid->input &
DRM_EDID_INPUT_DIGITAL &&
drivers/gpu/drm/nouveau/nouveau_connector.c:            if
(nv_connector->edid->input & DRM_EDID_INPUT_DIGITAL)
drivers/gpu/drm/radeon/radeon_connectors.c:
!!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
drivers/gpu/drm/radeon/radeon_connectors.c:
!!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c:
!!(amdgpu_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c:
!!(amdgpu_connector->edid->input & DRM_EDID_INPUT_DIGITAL);




> ---
>  drivers/gpu/drm/drm_edid.c | 17 +++++++++++++++--
>  include/drm/drm_edid.h     |  1 +
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 340da8257b51..1dbb15439468 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3110,7 +3110,7 @@ drm_monitor_supports_rb(const struct drm_edid *drm_edid)
>                 return ret;
>         }
>
> -       return ((drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL) != 0);
> +       return drm_edid_is_digital(drm_edid);
>  }
>
>  static void
> @@ -6519,7 +6519,7 @@ static void update_display_info(struct drm_connector *connector,
>         if (edid->revision < 3)
>                 goto out;
>
> -       if (!(edid->input & DRM_EDID_INPUT_DIGITAL))
> +       if (!drm_edid_is_digital(drm_edid))
>                 goto out;
>
>         info->color_formats |= DRM_COLOR_FORMAT_RGB444;
> @@ -7335,3 +7335,16 @@ static void _drm_update_tile_info(struct drm_connector *connector,
>                 connector->tile_group = NULL;
>         }
>  }
> +
> +/**
> + * drm_edid_is_digital - is digital?
> + * @drm_edid: The EDID
> + *
> + * Return true if input is digital.
> + */
> +bool drm_edid_is_digital(const struct drm_edid *drm_edid)
> +{
> +       return drm_edid && drm_edid->edid &&
> +               drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
> +}
> +EXPORT_SYMBOL(drm_edid_is_digital);
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 48e93f909ef6..882d2638708e 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -612,6 +612,7 @@ const struct drm_edid *drm_edid_read_switcheroo(struct drm_connector *connector,
>  int drm_edid_connector_update(struct drm_connector *connector,
>                               const struct drm_edid *edid);
>  int drm_edid_connector_add_modes(struct drm_connector *connector);
> +bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>
>  const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>                                   int ext_id, int *ext_index);
> --
> 2.39.2
>

Re: [PATCH 1/6] drm/edid: add drm_edid_is_digital()

[Jani Nikula]

On Fri, 01 Sep 2023, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Thu, Aug 24, 2023 at 9:46 AM Jani Nikula <jani.nikula@intel.com> wrote:
>>
>> Checking edid->input & DRM_EDID_INPUT_DIGITAL is common enough to
>> deserve a helper that also lets us abstract the raw EDID a bit better.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Thanks; I'm afraid this got merged already.

> Seems to be a few additional users of this that could be converted:
>
> drivers/gpu/drm/i915/display/intel_sdvo.c:        if (edid &&
> edid->input & DRM_EDID_INPUT_DIGITAL)
> drivers/gpu/drm/i915/display/intel_sdvo.c:    bool monitor_is_digital
> = !!(edid->input & DRM_EDID_INPUT_DIGITAL);
> drivers/gpu/drm/i915/display/intel_crt.c:        bool is_digital =
> edid->input & DRM_EDID_INPUT_DIGITAL;
> drivers/gpu/drm/i915/display/intel_hdmi.c:    if (edid && edid->input
> & DRM_EDID_INPUT_DIGITAL) {

The next patch takes care of these.

> drivers/gpu/drm/gma500/psb_intel_sdvo.c:        if (edid->input &
> DRM_EDID_INPUT_DIGITAL) {
> drivers/gpu/drm/gma500/psb_intel_sdvo.c:            if (edid->input &
> DRM_EDID_INPUT_DIGITAL)
> drivers/gpu/drm/gma500/psb_intel_sdvo.c:        bool
> monitor_is_digital = !!(edid->input & DRM_EDID_INPUT_DIGITAL);
> drivers/gpu/drm/gma500/psb_intel_sdvo.c:    if (edid != NULL &&
> edid->input & DRM_EDID_INPUT_DIGITAL)
> drivers/gpu/drm/gma500/cdv_intel_hdmi.c:        if (edid->input &
> DRM_EDID_INPUT_DIGITAL) {
> drivers/gpu/drm/display/drm_dp_helper.c:        edid->input &
> DRM_EDID_INPUT_DIGITAL &&
> drivers/gpu/drm/nouveau/nouveau_connector.c:            if
> (nv_connector->edid->input & DRM_EDID_INPUT_DIGITAL)
> drivers/gpu/drm/radeon/radeon_connectors.c:
> !!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
> drivers/gpu/drm/radeon/radeon_connectors.c:
> !!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
> drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c:
> !!(amdgpu_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
> drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c:
> !!(amdgpu_connector->edid->input & DRM_EDID_INPUT_DIGITAL);

drm_edid_is_digital() operates on struct drm_edid.

The drivers would first need to be converted to use struct drm_edid
instead of struct edid, and I'm not really taking that on.

IMO adding helpers to operate on struct edid would be
counter-productive.

BR,
Jani.

>
>
>
>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 17 +++++++++++++++--
>>  include/drm/drm_edid.h     |  1 +
>>  2 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 340da8257b51..1dbb15439468 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -3110,7 +3110,7 @@ drm_monitor_supports_rb(const struct drm_edid *drm_edid)
>>                 return ret;
>>         }
>>
>> -       return ((drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL) != 0);
>> +       return drm_edid_is_digital(drm_edid);
>>  }
>>
>>  static void
>> @@ -6519,7 +6519,7 @@ static void update_display_info(struct drm_connector *connector,
>>         if (edid->revision < 3)
>>                 goto out;
>>
>> -       if (!(edid->input & DRM_EDID_INPUT_DIGITAL))
>> +       if (!drm_edid_is_digital(drm_edid))
>>                 goto out;
>>
>>         info->color_formats |= DRM_COLOR_FORMAT_RGB444;
>> @@ -7335,3 +7335,16 @@ static void _drm_update_tile_info(struct drm_connector *connector,
>>                 connector->tile_group = NULL;
>>         }
>>  }
>> +
>> +/**
>> + * drm_edid_is_digital - is digital?
>> + * @drm_edid: The EDID
>> + *
>> + * Return true if input is digital.
>> + */
>> +bool drm_edid_is_digital(const struct drm_edid *drm_edid)
>> +{
>> +       return drm_edid && drm_edid->edid &&
>> +               drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
>> +}
>> +EXPORT_SYMBOL(drm_edid_is_digital);
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 48e93f909ef6..882d2638708e 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -612,6 +612,7 @@ const struct drm_edid *drm_edid_read_switcheroo(struct drm_connector *connector,
>>  int drm_edid_connector_update(struct drm_connector *connector,
>>                               const struct drm_edid *edid);
>>  int drm_edid_connector_add_modes(struct drm_connector *connector);
>> +bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>>
>>  const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>>                                   int ext_id, int *ext_index);
>> --
>> 2.39.2
>>

-- 
Jani Nikula, Intel Open Source Graphics Center