dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes
@ 2022-10-24 12:33 Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 01/16] drm/i915/hdmi: do dual mode detect only if connected Jani Nikula
                   ` (16 more replies)
  0 siblings, 17 replies; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

v2 of drm/edid: EDID override refactoring and fixes

Address review comments, add patch 15.

BR,
Jani.


Jani Nikula (16):
  drm/i915/hdmi: do dual mode detect only if connected
  drm/i915/hdmi: stop using connector->override_edid
  drm/amd/display: stop using connector->override_edid
  drm/edid: debug log EDID override set/reset
  drm/edid: abstract debugfs override EDID show better
  drm/edid: rename drm_add_override_edid_modes() to
    drm_edid_override_connector_update()
  drm/edid: split drm_edid block count helper
  drm/edid: add function for checking drm_edid validity
  drm/edid: detach debugfs EDID override from EDID property update
  drm/edid/firmware: drop redundant connector_name variable/parameter
  drm/edid/firmware: rename drm_load_edid_firmware() to
    drm_edid_load_firmware()
  drm/edid: use struct drm_edid for override/firmware EDID
  drm/edid: move edid load declarations to internal header
  drm/edid/firmware: convert to drm device specific logging
  drm/edid: add [CONNECTOR:%d:%s] to debug logging
  drm/edid: convert to device specific logging

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   3 -
 drivers/gpu/drm/drm_connector.c               |   1 +
 drivers/gpu/drm/drm_crtc_internal.h           |  15 +-
 drivers/gpu/drm/drm_debugfs.c                 |   8 +-
 drivers/gpu/drm/drm_edid.c                    | 346 +++++++++++-------
 drivers/gpu/drm/drm_edid_load.c               | 109 ++----
 drivers/gpu/drm/drm_probe_helper.c            |   2 +-
 drivers/gpu/drm/i915/display/intel_hdmi.c     |  20 +-
 include/drm/drm_connector.h                   |  16 +-
 include/drm/drm_edid.h                        |  10 +-
 10 files changed, 283 insertions(+), 247 deletions(-)

-- 
2.34.1


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

* [PATCH v2 01/16] drm/i915/hdmi: do dual mode detect only if connected
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 02/16] drm/i915/hdmi: stop using connector->override_edid Jani Nikula
                   ` (15 subsequent siblings)
  16 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

For normal connector detect, there's really no point in trying dual mode
detect if the connector is disconnected. We can simplify the detect
sequence by skipping it. Since intel_hdmi_dp_dual_mode_detect() is only
called when EDID is present, we can drop the has_edid parameter.

The functional effect is speeding up disconnected connector detection
ever so slightly, and, combined with firmware EDID, also stop logging
about assuming dual mode adaptor.

It's a bit subtle, but this will also skip dual mode detect if the
connector is force connected and a) there's no EDID of any kind, normal
or override/firmware or b) there's EDID but it does not indicate
digital. These are corner cases no matter what, and arguably forcing
should not be limited by dual mode detect.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_hdmi.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 93519fb23d9d..a332eaac86cd 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2355,7 +2355,7 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
 }
 
 static void
-intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid)
+intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_hdmi *hdmi = intel_attached_hdmi(to_intel_connector(connector));
@@ -2371,16 +2371,13 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid)
 	 * CONFIG1 pin, but no such luck on our hardware.
 	 *
 	 * The only method left to us is to check the VBT to see
-	 * if the port is a dual mode capable DP port. But let's
-	 * only do that when we sucesfully read the EDID, to avoid
-	 * confusing log messages about DP dual mode adaptors when
-	 * there's nothing connected to the port.
+	 * if the port is a dual mode capable DP port.
 	 */
 	if (type == DRM_DP_DUAL_MODE_UNKNOWN) {
 		/* An overridden EDID imply that we want this port for testing.
 		 * Make sure not to set limits for that port.
 		 */
-		if (has_edid && !connector->override_edid &&
+		if (!connector->override_edid &&
 		    intel_bios_is_port_dp_dual_mode(dev_priv, port)) {
 			drm_dbg_kms(&dev_priv->drm,
 				    "Assuming DP dual mode adaptor presence based on VBT\n");
@@ -2435,18 +2432,18 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 		intel_gmbus_force_bit(i2c, false);
 	}
 
-	intel_hdmi_dp_dual_mode_detect(connector, edid != NULL);
-
-	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref);
-
 	to_intel_connector(connector)->detect_edid = edid;
 	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
 		intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
 		intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
 
+		intel_hdmi_dp_dual_mode_detect(connector);
+
 		connected = true;
 	}
 
+	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref);
+
 	cec_notifier_set_phys_addr_from_edid(intel_hdmi->cec_notifier, edid);
 
 	return connected;
-- 
2.34.1


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

* [PATCH v2 02/16] drm/i915/hdmi: stop using connector->override_edid
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 01/16] drm/i915/hdmi: do dual mode detect only if connected Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 15:13   ` Ville Syrjälä
  2022-10-24 12:33 ` [PATCH v2 03/16] drm/amd/display: " Jani Nikula
                   ` (14 subsequent siblings)
  16 siblings, 1 reply; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

The connector->override_edid flag is strictly for EDID override debugfs
management, and drivers have no business using it.

The check for override_edid was added in commit 301906290553 ("drm/i915:
Ignore TMDS clock limit for DP++ when EDID override is set") to
facilitate mode list cross-checking against modes in override EDID when
the connector in question isn't even connected. The dual mode detect
fallback would do VBT based limiting in this case.

Instead of override EDID, check for connector forcing in the fallback.

v2: Simply use !connector->force (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_hdmi.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index a332eaac86cd..02f8374ea51f 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2374,10 +2374,7 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
 	 * if the port is a dual mode capable DP port.
 	 */
 	if (type == DRM_DP_DUAL_MODE_UNKNOWN) {
-		/* An overridden EDID imply that we want this port for testing.
-		 * Make sure not to set limits for that port.
-		 */
-		if (!connector->override_edid &&
+		if (!connector->force &&
 		    intel_bios_is_port_dp_dual_mode(dev_priv, port)) {
 			drm_dbg_kms(&dev_priv->drm,
 				    "Assuming DP dual mode adaptor presence based on VBT\n");
-- 
2.34.1


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

* [PATCH v2 03/16] drm/amd/display: stop using connector->override_edid
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 01/16] drm/i915/hdmi: do dual mode detect only if connected Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 02/16] drm/i915/hdmi: stop using connector->override_edid Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 04/16] drm/edid: debug log EDID override set/reset Jani Nikula
                   ` (13 subsequent siblings)
  16 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel
  Cc: jani.nikula, intel-gfx, Xinhui Pan, amd-gfx, Alex Deucher,
	Christian König

The connector->override_edid flag is strictly for EDID override debugfs
management, and drivers have no business using it.

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Xinhui Pan <Xinhui.Pan@amd.com>
Cc: amd-gfx@lists.freedesktop.org
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Acked-by: Alex Deucher <alexdeucher@gmail.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ---
 1 file changed, 3 deletions(-)

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 0db2a88cd4d7..3c072754738d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6108,7 +6108,6 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
 				aconnector->base.name);
 
 		aconnector->base.force = DRM_FORCE_OFF;
-		aconnector->base.override_edid = false;
 		return;
 	}
 
@@ -6143,8 +6142,6 @@ static void handle_edid_mgmt(struct amdgpu_dm_connector *aconnector)
 		link->verified_link_cap.link_rate = LINK_RATE_HIGH2;
 	}
 
-
-	aconnector->base.override_edid = true;
 	create_eml_sink(aconnector);
 }
 
-- 
2.34.1


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

* [PATCH v2 04/16] drm/edid: debug log EDID override set/reset
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (2 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 03/16] drm/amd/display: " Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 05/16] drm/edid: abstract debugfs override EDID show better Jani Nikula
                   ` (12 subsequent siblings)
  16 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

It's useful debugging information to know if and when an override EDID
was set or reset.

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 47465b9765f1..a863cffa2dc5 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2194,6 +2194,9 @@ int drm_edid_override_set(struct drm_connector *connector, const void *edid,
 
 	connector->override_edid = false;
 
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override set\n",
+		    connector->base.id, connector->name);
+
 	ret = drm_connector_update_edid_property(connector, edid);
 	if (!ret)
 		connector->override_edid = true;
@@ -2206,6 +2209,9 @@ int drm_edid_override_reset(struct drm_connector *connector)
 {
 	connector->override_edid = false;
 
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override reset\n",
+		    connector->base.id, connector->name);
+
 	return drm_connector_update_edid_property(connector, NULL);
 }
 
-- 
2.34.1


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

* [PATCH v2 05/16] drm/edid: abstract debugfs override EDID show better
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (3 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 04/16] drm/edid: debug log EDID override set/reset Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 06/16] drm/edid: rename drm_add_override_edid_modes() to drm_edid_override_connector_update() Jani Nikula
                   ` (11 subsequent siblings)
  16 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Add a function to dump the override EDID in debugfs. This hides the
override EDID management better in drm_edid.c.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc_internal.h |  4 +++-
 drivers/gpu/drm/drm_debugfs.c       |  8 +-------
 drivers/gpu/drm/drm_edid.c          | 11 +++++++++++
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 56041b604881..fb8a68d90940 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -56,9 +56,10 @@ struct drm_plane;
 struct drm_plane_state;
 struct drm_property;
 struct edid;
+struct fwnode_handle;
 struct kref;
+struct seq_file;
 struct work_struct;
-struct fwnode_handle;
 
 /* drm_crtc.c */
 int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
@@ -286,5 +287,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 
 /* drm_edid.c */
 void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
+int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m);
 int drm_edid_override_set(struct drm_connector *connector, const void *edid, size_t size);
 int drm_edid_override_reset(struct drm_connector *connector);
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 01ee3febb813..ee445f4605ba 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -328,13 +328,7 @@ static ssize_t connector_write(struct file *file, const char __user *ubuf,
 
 static int edid_show(struct seq_file *m, void *data)
 {
-	struct drm_connector *connector = m->private;
-	struct drm_property_blob *edid = connector->edid_blob_ptr;
-
-	if (connector->override_edid && edid)
-		seq_write(m, edid->data, edid->length);
-
-	return 0;
+	return drm_edid_override_show(m->private, m);
 }
 
 static int edid_open(struct inode *inode, struct file *file)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index a863cffa2dc5..1ada36e0d826 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2183,6 +2183,17 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
 	return IS_ERR(override) ? NULL : override;
 }
 
+/* For debugfs edid_override implementation */
+int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m)
+{
+	struct drm_property_blob *edid = connector->edid_blob_ptr;
+
+	if (connector->override_edid && edid)
+		seq_write(m, edid->data, edid->length);
+
+	return 0;
+}
+
 /* For debugfs edid_override implementation */
 int drm_edid_override_set(struct drm_connector *connector, const void *edid,
 			  size_t size)
-- 
2.34.1


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

* [PATCH v2 06/16] drm/edid: rename drm_add_override_edid_modes() to drm_edid_override_connector_update()
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (4 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 05/16] drm/edid: abstract debugfs override EDID show better Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 07/16] drm/edid: split drm_edid block count helper Jani Nikula
                   ` (10 subsequent siblings)
  16 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Follow the naming of both EDID override functions as well as
drm_edid_connector_update(). This also matches better what the function
does; a combination of EDID property update and add modes. Indeed it
should later be converted to call drm_edid_connector_update().

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         | 6 +++---
 drivers/gpu/drm/drm_probe_helper.c | 2 +-
 include/drm/drm_edid.h             | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 1ada36e0d826..8baa46dc2537 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2227,7 +2227,7 @@ int drm_edid_override_reset(struct drm_connector *connector)
 }
 
 /**
- * drm_add_override_edid_modes - add modes from override/firmware EDID
+ * drm_edid_override_connector_update - add modes from override/firmware EDID
  * @connector: connector we're probing
  *
  * Add modes from the override/firmware EDID, if available. Only to be used from
@@ -2237,7 +2237,7 @@ int drm_edid_override_reset(struct drm_connector *connector)
  *
  * Return: The number of modes added or 0 if we couldn't find any.
  */
-int drm_add_override_edid_modes(struct drm_connector *connector)
+int drm_edid_override_connector_update(struct drm_connector *connector)
 {
 	struct edid *override;
 	int num_modes = 0;
@@ -2254,7 +2254,7 @@ int drm_add_override_edid_modes(struct drm_connector *connector)
 
 	return num_modes;
 }
-EXPORT_SYMBOL(drm_add_override_edid_modes);
+EXPORT_SYMBOL(drm_edid_override_connector_update);
 
 typedef int read_block_fn(void *context, u8 *buf, unsigned int block, size_t len);
 
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 69b0b2b9cc1c..2fc21df709bc 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -367,7 +367,7 @@ static int drm_helper_probe_get_modes(struct drm_connector *connector)
 	 * override/firmware EDID.
 	 */
 	if (count == 0 && connector->status == connector_status_connected)
-		count = drm_add_override_edid_modes(connector);
+		count = drm_edid_override_connector_update(connector);
 
 	return count;
 }
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 429735b91f63..05380681a4fb 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -577,7 +577,7 @@ struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
 				     struct i2c_adapter *adapter);
 struct edid *drm_edid_duplicate(const struct edid *edid);
 int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
-int drm_add_override_edid_modes(struct drm_connector *connector);
+int drm_edid_override_connector_update(struct drm_connector *connector);
 
 u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
 bool drm_detect_hdmi_monitor(const struct edid *edid);
-- 
2.34.1


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

* [PATCH v2 07/16] drm/edid: split drm_edid block count helper
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (5 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 06/16] drm/edid: rename drm_add_override_edid_modes() to drm_edid_override_connector_update() Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 08/16] drm/edid: add function for checking drm_edid validity Jani Nikula
                   ` (9 subsequent siblings)
  16 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Split the drm_edid block count helper to a base version that reports the
block count indicated by EDID contents, and another on top that limits
the block count based on size allocated for the EDID.

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 | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 8baa46dc2537..616c1cdc7507 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1613,7 +1613,8 @@ static const void *edid_extension_block_data(const struct edid *edid, int index)
 	return edid_block_data(edid, index + 1);
 }
 
-static int drm_edid_block_count(const struct drm_edid *drm_edid)
+/* EDID block count indicated in EDID, may exceed allocated size */
+static int __drm_edid_block_count(const struct drm_edid *drm_edid)
 {
 	int num_blocks;
 
@@ -1633,12 +1634,18 @@ static int drm_edid_block_count(const struct drm_edid *drm_edid)
 			num_blocks = eeodb;
 	}
 
-	/* Limit by allocated size */
-	num_blocks = min(num_blocks, (int)drm_edid->size / EDID_LENGTH);
-
 	return num_blocks;
 }
 
+/* EDID block count, limited by allocated size */
+static int drm_edid_block_count(const struct drm_edid *drm_edid)
+{
+	/* Limit by allocated size */
+	return min(__drm_edid_block_count(drm_edid),
+		   (int)drm_edid->size / EDID_LENGTH);
+}
+
+/* EDID extension block count, limited by allocated size */
 static int drm_edid_extension_block_count(const struct drm_edid *drm_edid)
 {
 	return drm_edid_block_count(drm_edid) - 1;
-- 
2.34.1


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

* [PATCH v2 08/16] drm/edid: add function for checking drm_edid validity
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (6 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 07/16] drm/edid: split drm_edid block count helper Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 09/16] drm/edid: detach debugfs EDID override from EDID property update Jani Nikula
                   ` (8 subsequent siblings)
  16 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

We've lacked a function for immutable validity check on drm_edid. Add
one.

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 | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_edid.h     |  1 +
 2 files changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 616c1cdc7507..c3cf942186b7 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2040,6 +2040,36 @@ bool drm_edid_is_valid(struct edid *edid)
 }
 EXPORT_SYMBOL(drm_edid_is_valid);
 
+/**
+ * drm_edid_valid - sanity check EDID data
+ * @drm_edid: EDID data
+ *
+ * Sanity check an EDID. Cross check block count against allocated size and
+ * checksum the blocks.
+ *
+ * Return: True if the EDID data is valid, false otherwise.
+ */
+bool drm_edid_valid(const struct drm_edid *drm_edid)
+{
+	int i;
+
+	if (!drm_edid)
+		return false;
+
+	if (edid_size_by_blocks(__drm_edid_block_count(drm_edid)) != drm_edid->size)
+		return false;
+
+	for (i = 0; i < drm_edid_block_count(drm_edid); i++) {
+		const void *block = drm_edid_block_data(drm_edid, i);
+
+		if (!edid_block_valid(block, i == 0))
+			return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(drm_edid_valid);
+
 static struct edid *edid_filter_invalid_blocks(struct edid *edid,
 					       size_t *alloc_size)
 {
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 05380681a4fb..a2e25e7e6ee5 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -606,6 +606,7 @@ drm_display_mode_from_cea_vic(struct drm_device *dev,
 const struct drm_edid *drm_edid_alloc(const void *edid, size_t size);
 const struct drm_edid *drm_edid_dup(const struct drm_edid *drm_edid);
 void drm_edid_free(const struct drm_edid *drm_edid);
+bool drm_edid_valid(const struct drm_edid *drm_edid);
 const struct edid *drm_edid_raw(const struct drm_edid *drm_edid);
 const struct drm_edid *drm_edid_read(struct drm_connector *connector);
 const struct drm_edid *drm_edid_read_ddc(struct drm_connector *connector,
-- 
2.34.1


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

* [PATCH v2 09/16] drm/edid: detach debugfs EDID override from EDID property update
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (7 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 08/16] drm/edid: add function for checking drm_edid validity Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 17:53   ` Ville Syrjälä
  2022-10-24 12:33 ` [PATCH v2 10/16] drm/edid/firmware: drop redundant connector_name variable/parameter Jani Nikula
                   ` (7 subsequent siblings)
  16 siblings, 1 reply; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Having the EDID override debugfs directly update the EDID property is
problematic. The update is partial only. The driver has no way of
knowing it's been updated. Mode list is not updated. It's an
inconsistent state.

Detach debugfs EDID override from the property update completely. Only
set and reset a separate override EDID copy from debugfs, and have it
take effect only at detect (via EDID read). The copy is at
connector->edid_override, protected by connector->edid_override_mutex.

This also brings override EDID closer to firmware EDID in behaviour.

Add validation of the override EDID which we completely lacked.

Note that IGT already forces a detect whenever tests update the override
EDID.

v2: Add locking (Ville)

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

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 61c29ce74b03..f25674c0d41e 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -274,6 +274,7 @@ static int __drm_connector_init(struct drm_device *dev,
 	INIT_LIST_HEAD(&connector->probed_modes);
 	INIT_LIST_HEAD(&connector->modes);
 	mutex_init(&connector->mutex);
+	mutex_init(&connector->edid_override_mutex);
 	connector->edid_blob_ptr = NULL;
 	connector->epoch_counter = 0;
 	connector->tile_blob_ptr = NULL;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index c3cf942186b7..b8d07e4d6215 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2207,8 +2207,12 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
 {
 	struct edid *override = NULL;
 
-	if (connector->override_edid)
-		override = drm_edid_duplicate(connector->edid_blob_ptr->data);
+	mutex_lock(&connector->edid_override_mutex);
+
+	if (connector->edid_override)
+		override = drm_edid_duplicate(connector->edid_override->edid);
+
+	mutex_unlock(&connector->edid_override_mutex);
 
 	if (!override)
 		override = drm_load_edid_firmware(connector);
@@ -2223,10 +2227,15 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
 /* For debugfs edid_override implementation */
 int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m)
 {
-	struct drm_property_blob *edid = connector->edid_blob_ptr;
+	const struct drm_edid *drm_edid;
 
-	if (connector->override_edid && edid)
-		seq_write(m, edid->data, edid->length);
+	mutex_lock(&connector->edid_override_mutex);
+
+	drm_edid = connector->edid_override;
+	if (drm_edid)
+		seq_write(m, drm_edid->edid, drm_edid->size);
+
+	mutex_unlock(&connector->edid_override_mutex);
 
 	return 0;
 }
@@ -2235,32 +2244,43 @@ int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m)
 int drm_edid_override_set(struct drm_connector *connector, const void *edid,
 			  size_t size)
 {
-	int ret;
+	const struct drm_edid *drm_edid;
 
-	if (size < EDID_LENGTH || edid_size(edid) > size)
+	drm_edid = drm_edid_alloc(edid, size);
+	if (!drm_edid_valid(drm_edid)) {
+		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override invalid\n",
+			    connector->base.id, connector->name);
+		drm_edid_free(drm_edid);
 		return -EINVAL;
-
-	connector->override_edid = false;
+	}
 
 	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override set\n",
 		    connector->base.id, connector->name);
 
-	ret = drm_connector_update_edid_property(connector, edid);
-	if (!ret)
-		connector->override_edid = true;
+	mutex_lock(&connector->edid_override_mutex);
 
-	return ret;
+	drm_edid_free(connector->edid_override);
+	connector->edid_override = drm_edid;
+
+	mutex_unlock(&connector->edid_override_mutex);
+
+	return 0;
 }
 
 /* For debugfs edid_override implementation */
 int drm_edid_override_reset(struct drm_connector *connector)
 {
-	connector->override_edid = false;
-
 	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override reset\n",
 		    connector->base.id, connector->name);
 
-	return drm_connector_update_edid_property(connector, NULL);
+	mutex_lock(&connector->edid_override_mutex);
+
+	drm_edid_free(connector->edid_override);
+	connector->edid_override = NULL;
+
+	mutex_unlock(&connector->edid_override_mutex);
+
+	return 0;
 }
 
 /**
@@ -6634,23 +6654,6 @@ int drm_edid_connector_update(struct drm_connector *connector,
 {
 	int count;
 
-	/*
-	 * FIXME: Reconcile the differences in override_edid handling between
-	 * this and drm_connector_update_edid_property().
-	 *
-	 * If override_edid is set, and the EDID passed in here originates from
-	 * drm_edid_read() and friends, it will be the override EDID, and there
-	 * are no issues. drm_connector_update_edid_property() ignoring requests
-	 * to set the EDID dates back to a time when override EDID was not
-	 * handled at the low level EDID read.
-	 *
-	 * The only way the EDID passed in here can be different from the
-	 * override EDID is when a driver passes in an EDID that does *not*
-	 * originate from drm_edid_read() and friends, or passes in a stale
-	 * cached version. This, in turn, is a question of when an override EDID
-	 * set via debugfs should take effect.
-	 */
-
 	count = _drm_edid_connector_update(connector, drm_edid);
 
 	_drm_update_tile_info(connector, drm_edid);
@@ -6665,10 +6668,6 @@ EXPORT_SYMBOL(drm_edid_connector_update);
 static int _drm_connector_update_edid_property(struct drm_connector *connector,
 					       const struct drm_edid *drm_edid)
 {
-	/* ignore requests to set edid when overridden */
-	if (connector->override_edid)
-		return 0;
-
 	/*
 	 * Set the display info, using edid if available, otherwise resetting
 	 * the values to defaults. This duplicates the work done in
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index b1b2df48d42c..e641a4725f99 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1550,12 +1550,20 @@ struct drm_connector {
 	struct drm_cmdline_mode cmdline_mode;
 	/** @force: a DRM_FORCE_<foo> state for forced mode sets */
 	enum drm_connector_force force;
+
+	/**
+	 * @edid_override: Override EDID set via debugfs.
+	 *
+	 * Do not modify or access outside of the drm_edid_override_* family of
+	 * functions.
+	 */
+	const struct drm_edid *edid_override;
+
 	/**
-	 * @override_edid: has the EDID been overwritten through debugfs for
-	 * testing? Do not modify outside of drm_edid_override_set() and
-	 * drm_edid_override_reset().
+	 * @edid_override_mutex: Protect access to edid_override.
 	 */
-	bool override_edid;
+	struct mutex edid_override_mutex;
+
 	/** @epoch_counter: used to detect any other changes in connector, besides status */
 	u64 epoch_counter;
 
-- 
2.34.1


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

* [PATCH v2 10/16] drm/edid/firmware: drop redundant connector_name variable/parameter
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (8 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 09/16] drm/edid: detach debugfs EDID override from EDID property update Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 11/16] drm/edid/firmware: rename drm_load_edid_firmware() to drm_edid_load_firmware() Jani Nikula
                   ` (6 subsequent siblings)
  16 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Stop passing around something that's readily available in
connector->name.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid_load.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 37d8ba3ddb46..13cdbfb991eb 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -167,8 +167,7 @@ static int edid_size(const u8 *edid, int data_size)
 	return (edid[0x7e] + 1) * EDID_LENGTH;
 }
 
-static void *edid_load(struct drm_connector *connector, const char *name,
-			const char *connector_name)
+static void *edid_load(struct drm_connector *connector, const char *name)
 {
 	const struct firmware *fw = NULL;
 	const u8 *fwdata;
@@ -185,10 +184,10 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		struct platform_device *pdev;
 		int err;
 
-		pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
+		pdev = platform_device_register_simple(connector->name, -1, NULL, 0);
 		if (IS_ERR(pdev)) {
 			DRM_ERROR("Failed to register EDID firmware platform device "
-				  "for connector \"%s\"\n", connector_name);
+				  "for connector \"%s\"\n", connector->name);
 			return ERR_CAST(pdev);
 		}
 
@@ -244,7 +243,7 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
 		DRM_INFO("Found %d valid extensions instead of %d in EDID data "
 		    "\"%s\" for connector \"%s\"\n", valid_extensions,
-		    edid[0x7e], name, connector_name);
+		    edid[0x7e], name, connector->name);
 		edid[0x7e] = valid_extensions;
 
 		new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
@@ -256,7 +255,7 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 	DRM_INFO("Got %s EDID base block and %d extension%s from "
 	    "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
 	    "external", valid_extensions, valid_extensions == 1 ? "" : "s",
-	    name, connector_name);
+	    name, connector->name);
 
 out:
 	release_firmware(fw);
@@ -265,7 +264,6 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 
 struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 {
-	const char *connector_name = connector->name;
 	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
 	struct edid *edid;
 
@@ -288,7 +286,7 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 	while ((edidname = strsep(&edidstr, ","))) {
 		colon = strchr(edidname, ':');
 		if (colon != NULL) {
-			if (strncmp(connector_name, edidname, colon - edidname))
+			if (strncmp(connector->name, edidname, colon - edidname))
 				continue;
 			edidname = colon + 1;
 			break;
@@ -310,7 +308,7 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 	if (*last == '\n')
 		*last = '\0';
 
-	edid = edid_load(connector, edidname, connector_name);
+	edid = edid_load(connector, edidname);
 	kfree(fwstr);
 
 	return edid;
-- 
2.34.1


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

* [PATCH v2 11/16] drm/edid/firmware: rename drm_load_edid_firmware() to drm_edid_load_firmware()
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (9 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 10/16] drm/edid/firmware: drop redundant connector_name variable/parameter Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 12/16] drm/edid: use struct drm_edid for override/firmware EDID Jani Nikula
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Follow the usual naming convention by file name.

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      | 10 +++++-----
 drivers/gpu/drm/drm_edid_load.c |  2 +-
 include/drm/drm_edid.h          |  4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index b8d07e4d6215..5e5e1463e45f 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2215,7 +2215,7 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
 	mutex_unlock(&connector->edid_override_mutex);
 
 	if (!override)
-		override = drm_load_edid_firmware(connector);
+		override = drm_edid_load_firmware(connector);
 
 	/* FIXME: Get alloc size from deeper down the stack */
 	if (!IS_ERR_OR_NULL(override) && alloc_size)
@@ -2462,7 +2462,7 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
  * adapter and use drm_get_edid() instead of abusing this function.
  *
  * The EDID may be overridden using debugfs override_edid or firmware EDID
- * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
+ * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
  * order. Having either of them bypasses actual EDID reads.
  *
  * Return: Pointer to valid EDID or NULL if we couldn't find any.
@@ -2640,7 +2640,7 @@ EXPORT_SYMBOL(drm_get_edid);
  * this function.
  *
  * The EDID may be overridden using debugfs override_edid or firmware EDID
- * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
+ * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
  * order. Having either of them bypasses actual EDID reads.
  *
  * The returned pointer must be freed using drm_edid_free().
@@ -2678,7 +2678,7 @@ EXPORT_SYMBOL(drm_edid_read_custom);
  * Read EDID using the given I2C adapter.
  *
  * The EDID may be overridden using debugfs override_edid or firmware EDID
- * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
+ * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
  * order. Having either of them bypasses actual EDID reads.
  *
  * Prefer initializing connector->ddc with drm_connector_init_with_ddc() and
@@ -2714,7 +2714,7 @@ EXPORT_SYMBOL(drm_edid_read_ddc);
  * Read EDID using the connector's I2C adapter.
  *
  * The EDID may be overridden using debugfs override_edid or firmware EDID
- * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
+ * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
  * order. Having either of them bypasses actual EDID reads.
  *
  * The returned pointer must be freed using drm_edid_free().
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 13cdbfb991eb..bc6b96abd1b3 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -262,7 +262,7 @@ static void *edid_load(struct drm_connector *connector, const char *name)
 	return edid;
 }
 
-struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+struct edid *drm_edid_load_firmware(struct drm_connector *connector)
 {
 	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
 	struct edid *edid;
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index a2e25e7e6ee5..dc7467d25cd8 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -388,12 +388,12 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
-struct edid *drm_load_edid_firmware(struct drm_connector *connector);
+struct edid *drm_edid_load_firmware(struct drm_connector *connector);
 int __drm_set_edid_firmware_path(const char *path);
 int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
 #else
 static inline struct edid *
-drm_load_edid_firmware(struct drm_connector *connector)
+drm_edid_load_firmware(struct drm_connector *connector)
 {
 	return ERR_PTR(-ENOENT);
 }
-- 
2.34.1


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

* [PATCH v2 12/16] drm/edid: use struct drm_edid for override/firmware EDID
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (10 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 11/16] drm/edid/firmware: rename drm_load_edid_firmware() to drm_edid_load_firmware() Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 13/16] drm/edid: move edid load declarations to internal header Jani Nikula
                   ` (4 subsequent siblings)
  16 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

There's a lot going on here, but the main thing is switching the
firmware EDID loader to use struct drm_edid. Unfortunately, it's
difficult to reasonably split to smaller pieces.

Convert the EDID loader to struct drm_edid. There's a functional change
in validation; it no longer tries to fix errors or filter invalid
blocks. It's stricter in this sense. Hopefully this will not be an
issue.

As a by-product, this change also allows HF-EEODB extended EDIDs to be
passed via override/firmware EDID.

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      | 32 ++++++------
 drivers/gpu/drm/drm_edid_load.c | 86 +++++++--------------------------
 include/drm/drm_edid.h          |  4 +-
 3 files changed, 36 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 5e5e1463e45f..9078d1fb6d00 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2202,25 +2202,20 @@ static void connector_bad_edid(struct drm_connector *connector,
 }
 
 /* Get override or firmware EDID */
-static struct edid *drm_get_override_edid(struct drm_connector *connector,
-					  size_t *alloc_size)
+static const struct drm_edid *drm_edid_override_get(struct drm_connector *connector)
 {
-	struct edid *override = NULL;
+	const struct drm_edid *override = NULL;
 
 	mutex_lock(&connector->edid_override_mutex);
 
 	if (connector->edid_override)
-		override = drm_edid_duplicate(connector->edid_override->edid);
+		override = drm_edid_dup(connector->edid_override);
 
 	mutex_unlock(&connector->edid_override_mutex);
 
 	if (!override)
 		override = drm_edid_load_firmware(connector);
 
-	/* FIXME: Get alloc size from deeper down the stack */
-	if (!IS_ERR_OR_NULL(override) && alloc_size)
-		*alloc_size = edid_size(override);
-
 	return IS_ERR(override) ? NULL : override;
 }
 
@@ -2296,14 +2291,14 @@ int drm_edid_override_reset(struct drm_connector *connector)
  */
 int drm_edid_override_connector_update(struct drm_connector *connector)
 {
-	struct edid *override;
+	const struct drm_edid *override;
 	int num_modes = 0;
 
-	override = drm_get_override_edid(connector, NULL);
+	override = drm_edid_override_get(connector);
 	if (override) {
-		drm_connector_update_edid_property(connector, override);
-		num_modes = drm_add_edid_modes(connector, override);
-		kfree(override);
+		num_modes = drm_edid_connector_update(connector, override);
+
+		drm_edid_free(override);
 
 		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
 			      connector->base.id, connector->name, num_modes);
@@ -2354,12 +2349,19 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
 {
 	enum edid_block_status status;
 	int i, num_blocks, invalid_blocks = 0;
+	const struct drm_edid *override;
 	struct edid *edid, *new;
 	size_t alloc_size = EDID_LENGTH;
 
-	edid = drm_get_override_edid(connector, &alloc_size);
-	if (edid)
+	override = drm_edid_override_get(connector);
+	if (override) {
+		alloc_size = override->size;
+		edid = kmemdup(override->edid, alloc_size, GFP_KERNEL);
+		drm_edid_free(override);
+		if (!edid)
+			return NULL;
 		goto ok;
+	}
 
 	edid = kmalloc(alloc_size, GFP_KERNEL);
 	if (!edid)
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index bc6b96abd1b3..248f0685c33e 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -159,22 +159,12 @@ static const u8 generic_edid[GENERIC_EDIDS][128] = {
 	},
 };
 
-static int edid_size(const u8 *edid, int data_size)
-{
-	if (data_size < EDID_LENGTH)
-		return 0;
-
-	return (edid[0x7e] + 1) * EDID_LENGTH;
-}
-
-static void *edid_load(struct drm_connector *connector, const char *name)
+static const struct drm_edid *edid_load(struct drm_connector *connector, const char *name)
 {
 	const struct firmware *fw = NULL;
 	const u8 *fwdata;
-	u8 *edid;
+	const struct drm_edid *drm_edid;
 	int fwsize, builtin;
-	int i, valid_extensions = 0;
-	bool print_bad_edid = !connector->bad_edid_counter || drm_debug_enabled(DRM_UT_KMS);
 
 	builtin = match_string(generic_edid_name, GENERIC_EDIDS, name);
 	if (builtin >= 0) {
@@ -203,69 +193,26 @@ static void *edid_load(struct drm_connector *connector, const char *name)
 		fwsize = fw->size;
 	}
 
-	if (edid_size(fwdata, fwsize) != fwsize) {
-		DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
-			  "(expected %d, got %d\n", name,
-			  edid_size(fwdata, fwsize), (int)fwsize);
-		edid = ERR_PTR(-EINVAL);
-		goto out;
-	}
-
-	edid = kmemdup(fwdata, fwsize, GFP_KERNEL);
-	if (edid == NULL) {
-		edid = ERR_PTR(-ENOMEM);
-		goto out;
-	}
-
-	if (!drm_edid_block_valid(edid, 0, print_bad_edid,
-				  &connector->edid_corrupt)) {
-		connector->bad_edid_counter++;
-		DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
-		    name);
-		kfree(edid);
-		edid = ERR_PTR(-EINVAL);
-		goto out;
-	}
-
-	for (i = 1; i <= edid[0x7e]; i++) {
-		if (i != valid_extensions + 1)
-			memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
-			    edid + i * EDID_LENGTH, EDID_LENGTH);
-		if (drm_edid_block_valid(edid + i * EDID_LENGTH, i,
-					 print_bad_edid,
-					 NULL))
-			valid_extensions++;
-	}
-
-	if (valid_extensions != edid[0x7e]) {
-		u8 *new_edid;
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Loaded %s firmware EDID \"%s\"\n",
+		    connector->base.id, connector->name,
+		    builtin >= 0 ? "built-in" : "external", name);
 
-		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
-		DRM_INFO("Found %d valid extensions instead of %d in EDID data "
-		    "\"%s\" for connector \"%s\"\n", valid_extensions,
-		    edid[0x7e], name, connector->name);
-		edid[0x7e] = valid_extensions;
-
-		new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
-				    GFP_KERNEL);
-		if (new_edid)
-			edid = new_edid;
+	drm_edid = drm_edid_alloc(fwdata, fwsize);
+	if (!drm_edid_valid(drm_edid)) {
+		drm_err(connector->dev, "Invalid firmware EDID \"%s\"\n", name);
+		drm_edid_free(drm_edid);
+		drm_edid = ERR_PTR(-EINVAL);
 	}
 
-	DRM_INFO("Got %s EDID base block and %d extension%s from "
-	    "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
-	    "external", valid_extensions, valid_extensions == 1 ? "" : "s",
-	    name, connector->name);
-
-out:
 	release_firmware(fw);
-	return edid;
+
+	return drm_edid;
 }
 
-struct edid *drm_edid_load_firmware(struct drm_connector *connector)
+const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector)
 {
 	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
-	struct edid *edid;
+	const struct drm_edid *drm_edid;
 
 	if (edid_firmware[0] == '\0')
 		return ERR_PTR(-ENOENT);
@@ -308,8 +255,9 @@ struct edid *drm_edid_load_firmware(struct drm_connector *connector)
 	if (*last == '\n')
 		*last = '\0';
 
-	edid = edid_load(connector, edidname);
+	drm_edid = edid_load(connector, edidname);
+
 	kfree(fwstr);
 
-	return edid;
+	return drm_edid;
 }
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index dc7467d25cd8..8138613f4e4e 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -388,11 +388,11 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
-struct edid *drm_edid_load_firmware(struct drm_connector *connector);
+const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector);
 int __drm_set_edid_firmware_path(const char *path);
 int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
 #else
-static inline struct edid *
+static inline const struct drm_edid *
 drm_edid_load_firmware(struct drm_connector *connector)
 {
 	return ERR_PTR(-ENOENT);
-- 
2.34.1


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

* [PATCH v2 13/16] drm/edid: move edid load declarations to internal header
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (11 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 12/16] drm/edid: use struct drm_edid for override/firmware EDID Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 12:33 ` [PATCH v2 14/16] drm/edid/firmware: convert to drm device specific logging Jani Nikula
                   ` (3 subsequent siblings)
  16 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

The EDID loader is internal to drm, not for drivers.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc_internal.h | 11 +++++++++++
 drivers/gpu/drm/drm_edid_load.c     |  5 +++--
 include/drm/drm_edid.h              |  7 -------
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index fb8a68d90940..501a10edd0e1 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -290,3 +290,14 @@ void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
 int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m);
 int drm_edid_override_set(struct drm_connector *connector, const void *edid, size_t size);
 int drm_edid_override_reset(struct drm_connector *connector);
+
+/* drm_edid_load.c */
+#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
+const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector);
+#else
+static inline const struct drm_edid *
+drm_edid_load_firmware(struct drm_connector *connector)
+{
+	return ERR_PTR(-ENOENT);
+}
+#endif
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 248f0685c33e..882caaa6e663 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -11,12 +11,13 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
-#include <drm/drm_crtc.h>
-#include <drm/drm_crtc_helper.h>
+#include <drm/drm_connector.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_print.h>
 
+#include "drm_crtc_internal.h"
+
 static char edid_firmware[PATH_MAX];
 module_param_string(edid_firmware, edid_firmware, sizeof(edid_firmware), 0644);
 MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 8138613f4e4e..372963600f1d 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -388,15 +388,8 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
-const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector);
 int __drm_set_edid_firmware_path(const char *path);
 int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
-#else
-static inline const struct drm_edid *
-drm_edid_load_firmware(struct drm_connector *connector)
-{
-	return ERR_PTR(-ENOENT);
-}
 #endif
 
 bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2);
-- 
2.34.1


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

* [PATCH v2 14/16] drm/edid/firmware: convert to drm device specific logging
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (12 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 13/16] drm/edid: move edid load declarations to internal header Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 16:00   ` Ville Syrjälä
  2022-10-24 12:33 ` [PATCH v2 15/16] drm/edid: add [CONNECTOR:%d:%s] to debug logging Jani Nikula
                   ` (2 subsequent siblings)
  16 siblings, 1 reply; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Conform to device specific logging.

v2: Include [CONNECTOR:%d:%s] (Ville)

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid_load.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 882caaa6e663..ef4ab59d6935 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -177,16 +177,20 @@ static const struct drm_edid *edid_load(struct drm_connector *connector, const c
 
 		pdev = platform_device_register_simple(connector->name, -1, NULL, 0);
 		if (IS_ERR(pdev)) {
-			DRM_ERROR("Failed to register EDID firmware platform device "
-				  "for connector \"%s\"\n", connector->name);
+			drm_err(connector->dev,
+				"[CONNECTOR:%d:%s] Failed to register EDID firmware platform device for connector \"%s\"\n",
+				connector->base.id, connector->name,
+				connector->name);
 			return ERR_CAST(pdev);
 		}
 
 		err = request_firmware(&fw, name, &pdev->dev);
 		platform_device_unregister(pdev);
 		if (err) {
-			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
-				  name, err);
+			drm_err(connector->dev,
+				"[CONNECTOR:%d:%s] Requesting EDID firmware \"%s\" failed (err=%d)\n",
+				connector->base.id, connector->name,
+				name, err);
 			return ERR_PTR(err);
 		}
 
-- 
2.34.1


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

* [PATCH v2 15/16] drm/edid: add [CONNECTOR:%d:%s] to debug logging
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (13 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 14/16] drm/edid/firmware: convert to drm device specific logging Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 13:27   ` Simon Ser
  2022-10-24 12:33 ` [PATCH v2 16/16] drm/edid: convert to device specific logging Jani Nikula
  2022-10-26  8:33 ` [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
  16 siblings, 1 reply; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Reference the connector using [CONNECTOR:%d:%s] in existing device based
debug logging.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 9078d1fb6d00..3be98ac2e711 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2196,7 +2196,8 @@ static void connector_bad_edid(struct drm_connector *connector,
 	if (connector->bad_edid_counter++ && !drm_debug_enabled(DRM_UT_KMS))
 		return;
 
-	drm_dbg_kms(connector->dev, "%s: EDID is invalid:\n", connector->name);
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID is invalid:\n",
+		    connector->base.id, connector->name);
 	for (i = 0; i < num_blocks; i++)
 		edid_block_dump(KERN_DEBUG, edid + i, i);
 }
@@ -6031,7 +6032,8 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
 	}
 
 	drm_dbg_kms(connector->dev,
-		    "HF-VSDB: max TMDS clock: %d KHz, HDMI 2.1 support: %s, DSC 1.2 support: %s\n",
+		    "[CONNECTOR:%d:%s] HF-VSDB: max TMDS clock: %d KHz, HDMI 2.1 support: %s, DSC 1.2 support: %s\n",
+		    connector->base.id, connector->name,
 		    max_tmds_clock, str_yes_no(max_frl_rate), str_yes_no(dsc_support));
 }
 
@@ -6131,8 +6133,9 @@ static void drm_parse_microsoft_vsdb(struct drm_connector *connector,
 	if (version == 1 || version == 2 || (version == 3 && !desktop_usage))
 		info->non_desktop = true;
 
-	drm_dbg_kms(connector->dev, "HMD or specialized display VSDB version %u: 0x%02x\n",
-		    version, db[5]);
+	drm_dbg_kms(connector->dev,
+		    "[CONNECTOR:%d:%s] HMD or specialized display VSDB version %u: 0x%02x\n",
+		    connector->base.id, connector->name, version, db[5]);
 }
 
 static void drm_parse_cea_ext(struct drm_connector *connector,
@@ -6253,8 +6256,9 @@ static void drm_parse_vesa_mso_data(struct drm_connector *connector,
 	struct drm_display_info *info = &connector->display_info;
 
 	if (block->num_bytes < 3) {
-		drm_dbg_kms(connector->dev, "Unexpected vendor block size %u\n",
-			    block->num_bytes);
+		drm_dbg_kms(connector->dev,
+			    "[CONNECTOR:%d:%s] Unexpected vendor block size %u\n",
+			    connector->base.id, connector->name, block->num_bytes);
 		return;
 	}
 
@@ -6262,13 +6266,16 @@ static void drm_parse_vesa_mso_data(struct drm_connector *connector,
 		return;
 
 	if (sizeof(*vesa) != sizeof(*block) + block->num_bytes) {
-		drm_dbg_kms(connector->dev, "Unexpected VESA vendor block size\n");
+		drm_dbg_kms(connector->dev,
+			    "[CONNECTOR:%d:%s] Unexpected VESA vendor block size\n",
+			    connector->base.id, connector->name);
 		return;
 	}
 
 	switch (FIELD_GET(DISPLAYID_VESA_MSO_MODE, vesa->mso)) {
 	default:
-		drm_dbg_kms(connector->dev, "Reserved MSO mode value\n");
+		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Reserved MSO mode value\n",
+			    connector->base.id, connector->name);
 		fallthrough;
 	case 0:
 		info->mso_stream_count = 0;
@@ -6288,12 +6295,16 @@ static void drm_parse_vesa_mso_data(struct drm_connector *connector,
 
 	info->mso_pixel_overlap = FIELD_GET(DISPLAYID_VESA_MSO_OVERLAP, vesa->mso);
 	if (info->mso_pixel_overlap > 8) {
-		drm_dbg_kms(connector->dev, "Reserved MSO pixel overlap value %u\n",
+		drm_dbg_kms(connector->dev,
+			    "[CONNECTOR:%d:%s] Reserved MSO pixel overlap value %u\n",
+			    connector->base.id, connector->name,
 			    info->mso_pixel_overlap);
 		info->mso_pixel_overlap = 8;
 	}
 
-	drm_dbg_kms(connector->dev, "MSO stream count %u, pixel overlap %u\n",
+	drm_dbg_kms(connector->dev,
+		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
+		    connector->base.id, connector->name,
 		    info->mso_stream_count, info->mso_pixel_overlap);
 }
 
@@ -6421,7 +6432,8 @@ static u32 update_display_info(struct drm_connector *connector,
 
 out:
 	if (quirks & EDID_QUIRK_NON_DESKTOP) {
-		drm_dbg_kms(connector->dev, "Non-desktop display%s\n",
+		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Non-desktop display%s\n",
+			    connector->base.id, connector->name,
 			    info->non_desktop ? " (redundant quirk)" : "");
 		info->non_desktop = true;
 	}
@@ -6732,8 +6744,8 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
 	struct drm_edid drm_edid;
 
 	if (edid && !drm_edid_is_valid(edid)) {
-		drm_warn(connector->dev, "%s: EDID invalid.\n",
-			 connector->name);
+		drm_warn(connector->dev, "[CONNECTOR:%d:%s] EDID invalid.\n",
+			 connector->base.id, connector->name);
 		edid = NULL;
 	}
 
-- 
2.34.1


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

* [PATCH v2 16/16] drm/edid: convert to device specific logging
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (14 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 15/16] drm/edid: add [CONNECTOR:%d:%s] to debug logging Jani Nikula
@ 2022-10-24 12:33 ` Jani Nikula
  2022-10-24 16:02   ` Ville Syrjälä
  2022-10-26  8:33 ` [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
  16 siblings, 1 reply; 23+ messages in thread
From: Jani Nikula @ 2022-10-24 12:33 UTC (permalink / raw)
  To: dri-devel; +Cc: jani.nikula, intel-gfx

Convert to drm_kms_dbg/drm_err where possible, and reference the
connector using [CONNECTOR:%d:%s]. Pass connectors around a bit more to
enable this. Where this is not possible, unify the rest of the debugs to
DRM_DEBUG_KMS.

Rewrite tile debug logging to one line while at it.

v2:
- Use [CONNECTOR:%d:%s] throughout (Ville)
- Tile debug logging revamp
- Pass connector around a bit more

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 137 +++++++++++++++++++++----------------
 1 file changed, 77 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3be98ac2e711..b2d61c05f559 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1979,7 +1979,7 @@ bool drm_edid_block_valid(u8 *_block, int block_num, bool print_bad_edid,
 
 	status = edid_block_check(block, is_base_block);
 	if (status == EDID_BLOCK_HEADER_REPAIR) {
-		DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
+		DRM_DEBUG_KMS("Fixing EDID header, your hardware may be failing\n");
 		edid_header_fix(block);
 
 		/* Retry with fixed header, update status if that worked. */
@@ -2301,8 +2301,9 @@ int drm_edid_override_connector_update(struct drm_connector *connector)
 
 		drm_edid_free(override);
 
-		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
-			      connector->base.id, connector->name, num_modes);
+		drm_dbg_kms(connector->dev,
+			    "[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
+			    connector->base.id, connector->name, num_modes);
 	}
 
 	return num_modes;
@@ -3388,11 +3389,12 @@ drm_mode_do_interlace_quirk(struct drm_display_mode *mode,
  * timing block contains enough info for us to create and return a new struct
  * drm_display_mode.
  */
-static struct drm_display_mode *drm_mode_detailed(struct drm_device *dev,
+static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connector,
 						  const struct drm_edid *drm_edid,
 						  const struct detailed_timing *timing,
 						  u32 quirks)
 {
+	struct drm_device *dev = connector->dev;
 	struct drm_display_mode *mode;
 	const struct detailed_pixel_timing *pt = &timing->data.pixel_data;
 	unsigned hactive = (pt->hactive_hblank_hi & 0xf0) << 4 | pt->hactive_lo;
@@ -3409,17 +3411,19 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_device *dev,
 		return NULL;
 
 	if (pt->misc & DRM_EDID_PT_STEREO) {
-		DRM_DEBUG_KMS("stereo mode not supported\n");
+		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Stereo mode not supported\n",
+			    connector->base.id, connector->name);
 		return NULL;
 	}
 	if (!(pt->misc & DRM_EDID_PT_SEPARATE_SYNC)) {
-		DRM_DEBUG_KMS("composite sync not supported\n");
+		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Composite sync not supported\n",
+			    connector->base.id, connector->name);
 	}
 
 	/* it is incorrect if hsync/vsync width is zero */
 	if (!hsync_pulse_width || !vsync_pulse_width) {
-		DRM_DEBUG_KMS("Incorrect Detailed timing. "
-				"Wrong Hsync/Vsync pulse width\n");
+		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Incorrect Detailed timing. Wrong Hsync/Vsync pulse width\n",
+			    connector->base.id, connector->name);
 		return NULL;
 	}
 
@@ -3976,7 +3980,8 @@ add_cvt_modes(struct drm_connector *connector, const struct drm_edid *drm_edid)
 	return closure.modes;
 }
 
-static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode);
+static void fixup_detailed_cea_mode_clock(struct drm_connector *connector,
+					  struct drm_display_mode *mode);
 
 static void
 do_detailed_mode(const struct detailed_timing *timing, void *c)
@@ -3987,7 +3992,7 @@ do_detailed_mode(const struct detailed_timing *timing, void *c)
 	if (!is_detailed_timing_descriptor(timing))
 		return;
 
-	newmode = drm_mode_detailed(closure->connector->dev,
+	newmode = drm_mode_detailed(closure->connector,
 				    closure->drm_edid, timing,
 				    closure->quirks);
 	if (!newmode)
@@ -4001,7 +4006,7 @@ do_detailed_mode(const struct detailed_timing *timing, void *c)
 	 * so fix up anything that looks like CEA/HDMI mode, but the clock
 	 * is just slightly off.
 	 */
-	fixup_detailed_cea_mode_clock(newmode);
+	fixup_detailed_cea_mode_clock(closure->connector, newmode);
 
 	drm_mode_probed_add(closure->connector, newmode);
 	closure->modes++;
@@ -4663,7 +4668,8 @@ static int add_hdmi_mode(struct drm_connector *connector, u8 vic)
 	struct drm_display_mode *newmode;
 
 	if (!drm_valid_hdmi_vic(vic)) {
-		DRM_ERROR("Unknown HDMI VIC: %d\n", vic);
+		drm_err(connector->dev, "[CONNECTOR:%d:%s] Unknown HDMI VIC: %d\n",
+			connector->base.id, connector->name, vic);
 		return 0;
 	}
 
@@ -5270,7 +5276,8 @@ static int add_cea_modes(struct drm_connector *connector,
 	return modes;
 }
 
-static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode)
+static void fixup_detailed_cea_mode_clock(struct drm_connector *connector,
+					  struct drm_display_mode *mode)
 {
 	const struct drm_display_mode *cea_mode;
 	int clock1, clock2, clock;
@@ -5308,8 +5315,10 @@ static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode)
 	if (mode->clock == clock)
 		return;
 
-	DRM_DEBUG("detailed mode matches %s VIC %d, adjusting clock %d -> %d\n",
-		  type, vic, mode->clock, clock);
+	drm_dbg_kms(connector->dev,
+		    "[CONNECTOR:%d:%s] detailed mode matches %s VIC %d, adjusting clock %d -> %d\n",
+		    connector->base.id, connector->name,
+		    type, vic, mode->clock, clock);
 	mode->clock = clock;
 }
 
@@ -5417,15 +5426,12 @@ drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
 	if (len >= 12)
 		connector->audio_latency[1] = db[12];
 
-	DRM_DEBUG_KMS("HDMI: latency present %d %d, "
-		      "video latency %d %d, "
-		      "audio latency %d %d\n",
-		      connector->latency_present[0],
-		      connector->latency_present[1],
-		      connector->video_latency[0],
-		      connector->video_latency[1],
-		      connector->audio_latency[0],
-		      connector->audio_latency[1]);
+	drm_dbg_kms(connector->dev,
+		    "[CONNECTOR:%d:%s] HDMI: latency present %d %d, video latency %d %d, audio latency %d %d\n",
+		    connector->base.id, connector->name,
+		    connector->latency_present[0], connector->latency_present[1],
+		    connector->video_latency[0], connector->video_latency[1],
+		    connector->audio_latency[0], connector->audio_latency[1]);
 }
 
 static void
@@ -5523,7 +5529,9 @@ static void drm_edid_to_eld(struct drm_connector *connector,
 		return;
 
 	mnl = get_monitor_name(drm_edid, &eld[DRM_ELD_MONITOR_NAME_STRING]);
-	DRM_DEBUG_KMS("ELD monitor %s\n", &eld[DRM_ELD_MONITOR_NAME_STRING]);
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] ELD monitor %s\n",
+		    connector->base.id, connector->name,
+		    &eld[DRM_ELD_MONITOR_NAME_STRING]);
 
 	eld[DRM_ELD_CEA_EDID_VER_MNL] = info->cea_rev << DRM_ELD_CEA_EDID_VER_SHIFT;
 	eld[DRM_ELD_CEA_EDID_VER_MNL] |= mnl;
@@ -5577,8 +5585,9 @@ static void drm_edid_to_eld(struct drm_connector *connector,
 	eld[DRM_ELD_BASELINE_ELD_LEN] =
 		DIV_ROUND_UP(drm_eld_calc_baseline_block_size(eld), 4);
 
-	DRM_DEBUG_KMS("ELD size %d, SAD count %d\n",
-		      drm_eld_size(eld), total_sad_count);
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] ELD size %d, SAD count %d\n",
+		    connector->base.id, connector->name,
+		    drm_eld_size(eld), total_sad_count);
 }
 
 static int _drm_edid_to_sad(const struct drm_edid *drm_edid,
@@ -5849,7 +5858,8 @@ static void drm_parse_vcdb(struct drm_connector *connector, const u8 *db)
 {
 	struct drm_display_info *info = &connector->display_info;
 
-	DRM_DEBUG_KMS("CEA VCDB 0x%02x\n", db[2]);
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] CEA VCDB 0x%02x\n",
+		    connector->base.id, connector->name, db[2]);
 
 	if (db[2] & EDID_CEA_VCDB_QS)
 		info->rgb_quant_range_selectable = true;
@@ -6052,39 +6062,39 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
 	if (hdmi[6] & DRM_EDID_HDMI_DC_30) {
 		dc_bpc = 10;
 		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_30;
-		DRM_DEBUG("%s: HDMI sink does deep color 30.\n",
-			  connector->name);
+		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does deep color 30.\n",
+			    connector->base.id, connector->name);
 	}
 
 	if (hdmi[6] & DRM_EDID_HDMI_DC_36) {
 		dc_bpc = 12;
 		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_36;
-		DRM_DEBUG("%s: HDMI sink does deep color 36.\n",
-			  connector->name);
+		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does deep color 36.\n",
+			    connector->base.id, connector->name);
 	}
 
 	if (hdmi[6] & DRM_EDID_HDMI_DC_48) {
 		dc_bpc = 16;
 		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_48;
-		DRM_DEBUG("%s: HDMI sink does deep color 48.\n",
-			  connector->name);
+		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does deep color 48.\n",
+			    connector->base.id, connector->name);
 	}
 
 	if (dc_bpc == 0) {
-		DRM_DEBUG("%s: No deep color support on this HDMI sink.\n",
-			  connector->name);
+		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] No deep color support on this HDMI sink.\n",
+			    connector->base.id, connector->name);
 		return;
 	}
 
-	DRM_DEBUG("%s: Assigning HDMI sink color depth as %d bpc.\n",
-		  connector->name, dc_bpc);
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Assigning HDMI sink color depth as %d bpc.\n",
+		    connector->base.id, connector->name, dc_bpc);
 	info->bpc = dc_bpc;
 
 	/* YCRCB444 is optional according to spec. */
 	if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) {
 		info->edid_hdmi_ycbcr444_dc_modes = info->edid_hdmi_rgb444_dc_modes;
-		DRM_DEBUG("%s: HDMI sink does YCRCB444 in deep color.\n",
-			  connector->name);
+		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does YCRCB444 in deep color.\n",
+			    connector->base.id, connector->name);
 	}
 
 	/*
@@ -6092,8 +6102,8 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
 	 * then deep color 36 bit must be supported.
 	 */
 	if (!(hdmi[6] & DRM_EDID_HDMI_DC_36)) {
-		DRM_DEBUG("%s: HDMI sink should do DC_36, but does not!\n",
-			  connector->name);
+		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink should do DC_36, but does not!\n",
+			    connector->base.id, connector->name);
 	}
 }
 
@@ -6110,10 +6120,9 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
 	if (len >= 7)
 		info->max_tmds_clock = db[7] * 5000;
 
-	DRM_DEBUG_KMS("HDMI: DVI dual %d, "
-		      "max TMDS clock %d kHz\n",
-		      info->dvi_dual,
-		      info->max_tmds_clock);
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI: DVI dual %d, max TMDS clock %d kHz\n",
+		    connector->base.id, connector->name,
+		    info->dvi_dual, info->max_tmds_clock);
 
 	drm_parse_hdmi_deep_color_info(connector, db);
 }
@@ -6156,8 +6165,10 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
 			info->cea_rev = edid_ext[1];
 
 		if (info->cea_rev != edid_ext[1])
-			DRM_DEBUG_KMS("CEA extension version mismatch %u != %u\n",
-				      info->cea_rev, edid_ext[1]);
+			drm_dbg_kms(connector->dev,
+				    "[CONNECTOR:%d:%s] CEA extension version mismatch %u != %u\n",
+				    connector->base.id, connector->name,
+				    info->cea_rev, edid_ext[1]);
 
 		/* The existence of a CTA extension should imply RGB support */
 		info->color_formats = DRM_COLOR_FORMAT_RGB444;
@@ -6243,9 +6254,10 @@ static void drm_get_monitor_range(struct drm_connector *connector,
 
 	drm_for_each_detailed_block(drm_edid, get_monitor_range, &closure);
 
-	DRM_DEBUG_KMS("Supported Monitor Refresh rate range is %d Hz - %d Hz\n",
-		      info->monitor_range.min_vfreq,
-		      info->monitor_range.max_vfreq);
+	drm_dbg_kms(connector->dev,
+		    "[CONNECTOR:%d:%s] Supported Monitor Refresh rate range is %d Hz - %d Hz\n",
+		    connector->base.id, connector->name,
+		    info->monitor_range.min_vfreq, info->monitor_range.max_vfreq);
 }
 
 static void drm_parse_vesa_mso_data(struct drm_connector *connector,
@@ -6387,8 +6399,9 @@ static u32 update_display_info(struct drm_connector *connector,
 	if (info->bpc == 0 && edid->revision == 3 &&
 	    edid->input & DRM_EDID_DIGITAL_DFP_1_X) {
 		info->bpc = 8;
-		DRM_DEBUG("%s: Assigning DFP sink color depth as %d bpc.\n",
-			  connector->name, info->bpc);
+		drm_dbg_kms(connector->dev,
+			    "[CONNECTOR:%d:%s] Assigning DFP sink color depth as %d bpc.\n",
+			    connector->base.id, connector->name, info->bpc);
 	}
 
 	/* Only defined for 1.4 with digital displays */
@@ -6420,8 +6433,9 @@ static u32 update_display_info(struct drm_connector *connector,
 		break;
 	}
 
-	DRM_DEBUG("%s: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
-			  connector->name, info->bpc);
+	drm_dbg_kms(connector->dev,
+		    "[CONNECTOR:%d:%s] Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
+		    connector->base.id, connector->name, info->bpc);
 
 	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
 		info->color_formats |= DRM_COLOR_FORMAT_YCBCR444;
@@ -7121,11 +7135,14 @@ static void drm_parse_tiled_block(struct drm_connector *connector,
 	connector->tile_h_size = w + 1;
 	connector->tile_v_size = h + 1;
 
-	DRM_DEBUG_KMS("tile cap 0x%x\n", tile->tile_cap);
-	DRM_DEBUG_KMS("tile_size %d x %d\n", w + 1, h + 1);
-	DRM_DEBUG_KMS("topo num tiles %dx%d, location %dx%d\n",
-		      num_h_tile + 1, num_v_tile + 1, tile_h_loc, tile_v_loc);
-	DRM_DEBUG_KMS("vend %c%c%c\n", tile->topology_id[0], tile->topology_id[1], tile->topology_id[2]);
+	drm_dbg_kms(connector->dev,
+		    "[CONNECTOR:%d:%s] tile cap 0x%x, size %dx%d, num tiles %dx%d, location %dx%d, vend %c%c%c",
+		    connector->base.id, connector->name,
+		    tile->tile_cap,
+		    connector->tile_h_size, connector->tile_v_size,
+		    connector->num_h_tile, connector->num_v_tile,
+		    connector->tile_h_loc, connector->tile_v_loc,
+		    tile->topology_id[0], tile->topology_id[1], tile->topology_id[2]);
 
 	tg = drm_mode_get_tile_group(connector->dev, tile->topology_id);
 	if (!tg)
-- 
2.34.1


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

* Re: [PATCH v2 15/16] drm/edid: add [CONNECTOR:%d:%s] to debug logging
  2022-10-24 12:33 ` [PATCH v2 15/16] drm/edid: add [CONNECTOR:%d:%s] to debug logging Jani Nikula
@ 2022-10-24 13:27   ` Simon Ser
  0 siblings, 0 replies; 23+ messages in thread
From: Simon Ser @ 2022-10-24 13:27 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, dri-devel

Reviewed-by: Simon Ser <contact@emersion.fr>

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

* Re: [PATCH v2 02/16] drm/i915/hdmi: stop using connector->override_edid
  2022-10-24 12:33 ` [PATCH v2 02/16] drm/i915/hdmi: stop using connector->override_edid Jani Nikula
@ 2022-10-24 15:13   ` Ville Syrjälä
  0 siblings, 0 replies; 23+ messages in thread
From: Ville Syrjälä @ 2022-10-24 15:13 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, dri-devel

On Mon, Oct 24, 2022 at 03:33:30PM +0300, Jani Nikula wrote:
> The connector->override_edid flag is strictly for EDID override debugfs
> management, and drivers have no business using it.
> 
> The check for override_edid was added in commit 301906290553 ("drm/i915:
> Ignore TMDS clock limit for DP++ when EDID override is set") to
> facilitate mode list cross-checking against modes in override EDID when
> the connector in question isn't even connected. The dual mode detect
> fallback would do VBT based limiting in this case.
> 
> Instead of override EDID, check for connector forcing in the fallback.
> 
> v2: Simply use !connector->force (Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

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

> ---
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index a332eaac86cd..02f8374ea51f 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2374,10 +2374,7 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
>  	 * if the port is a dual mode capable DP port.
>  	 */
>  	if (type == DRM_DP_DUAL_MODE_UNKNOWN) {
> -		/* An overridden EDID imply that we want this port for testing.
> -		 * Make sure not to set limits for that port.
> -		 */
> -		if (!connector->override_edid &&
> +		if (!connector->force &&
>  		    intel_bios_is_port_dp_dual_mode(dev_priv, port)) {
>  			drm_dbg_kms(&dev_priv->drm,
>  				    "Assuming DP dual mode adaptor presence based on VBT\n");
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

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

* Re: [PATCH v2 14/16] drm/edid/firmware: convert to drm device specific logging
  2022-10-24 12:33 ` [PATCH v2 14/16] drm/edid/firmware: convert to drm device specific logging Jani Nikula
@ 2022-10-24 16:00   ` Ville Syrjälä
  0 siblings, 0 replies; 23+ messages in thread
From: Ville Syrjälä @ 2022-10-24 16:00 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, dri-devel

On Mon, Oct 24, 2022 at 03:33:42PM +0300, Jani Nikula wrote:
> Conform to device specific logging.
> 
> v2: Include [CONNECTOR:%d:%s] (Ville)
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

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

> ---
>  drivers/gpu/drm/drm_edid_load.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 882caaa6e663..ef4ab59d6935 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -177,16 +177,20 @@ static const struct drm_edid *edid_load(struct drm_connector *connector, const c
>  
>  		pdev = platform_device_register_simple(connector->name, -1, NULL, 0);
>  		if (IS_ERR(pdev)) {
> -			DRM_ERROR("Failed to register EDID firmware platform device "
> -				  "for connector \"%s\"\n", connector->name);
> +			drm_err(connector->dev,
> +				"[CONNECTOR:%d:%s] Failed to register EDID firmware platform device for connector \"%s\"\n",
> +				connector->base.id, connector->name,
> +				connector->name);
>  			return ERR_CAST(pdev);
>  		}
>  
>  		err = request_firmware(&fw, name, &pdev->dev);
>  		platform_device_unregister(pdev);
>  		if (err) {
> -			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
> -				  name, err);
> +			drm_err(connector->dev,
> +				"[CONNECTOR:%d:%s] Requesting EDID firmware \"%s\" failed (err=%d)\n",
> +				connector->base.id, connector->name,
> +				name, err);
>  			return ERR_PTR(err);
>  		}
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

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

* Re: [PATCH v2 16/16] drm/edid: convert to device specific logging
  2022-10-24 12:33 ` [PATCH v2 16/16] drm/edid: convert to device specific logging Jani Nikula
@ 2022-10-24 16:02   ` Ville Syrjälä
  0 siblings, 0 replies; 23+ messages in thread
From: Ville Syrjälä @ 2022-10-24 16:02 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, dri-devel

On Mon, Oct 24, 2022 at 03:33:44PM +0300, Jani Nikula wrote:
> Convert to drm_kms_dbg/drm_err where possible, and reference the
> connector using [CONNECTOR:%d:%s]. Pass connectors around a bit more to
> enable this. Where this is not possible, unify the rest of the debugs to
> DRM_DEBUG_KMS.
> 
> Rewrite tile debug logging to one line while at it.
> 
> v2:
> - Use [CONNECTOR:%d:%s] throughout (Ville)
> - Tile debug logging revamp
> - Pass connector around a bit more
> 
> 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 | 137 +++++++++++++++++++++----------------
>  1 file changed, 77 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3be98ac2e711..b2d61c05f559 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1979,7 +1979,7 @@ bool drm_edid_block_valid(u8 *_block, int block_num, bool print_bad_edid,
>  
>  	status = edid_block_check(block, is_base_block);
>  	if (status == EDID_BLOCK_HEADER_REPAIR) {
> -		DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
> +		DRM_DEBUG_KMS("Fixing EDID header, your hardware may be failing\n");
>  		edid_header_fix(block);
>  
>  		/* Retry with fixed header, update status if that worked. */
> @@ -2301,8 +2301,9 @@ int drm_edid_override_connector_update(struct drm_connector *connector)
>  
>  		drm_edid_free(override);
>  
> -		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
> -			      connector->base.id, connector->name, num_modes);
> +		drm_dbg_kms(connector->dev,
> +			    "[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
> +			    connector->base.id, connector->name, num_modes);
>  	}
>  
>  	return num_modes;
> @@ -3388,11 +3389,12 @@ drm_mode_do_interlace_quirk(struct drm_display_mode *mode,
>   * timing block contains enough info for us to create and return a new struct
>   * drm_display_mode.
>   */
> -static struct drm_display_mode *drm_mode_detailed(struct drm_device *dev,
> +static struct drm_display_mode *drm_mode_detailed(struct drm_connector *connector,
>  						  const struct drm_edid *drm_edid,
>  						  const struct detailed_timing *timing,
>  						  u32 quirks)
>  {
> +	struct drm_device *dev = connector->dev;
>  	struct drm_display_mode *mode;
>  	const struct detailed_pixel_timing *pt = &timing->data.pixel_data;
>  	unsigned hactive = (pt->hactive_hblank_hi & 0xf0) << 4 | pt->hactive_lo;
> @@ -3409,17 +3411,19 @@ static struct drm_display_mode *drm_mode_detailed(struct drm_device *dev,
>  		return NULL;
>  
>  	if (pt->misc & DRM_EDID_PT_STEREO) {
> -		DRM_DEBUG_KMS("stereo mode not supported\n");
> +		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Stereo mode not supported\n",
> +			    connector->base.id, connector->name);
>  		return NULL;
>  	}
>  	if (!(pt->misc & DRM_EDID_PT_SEPARATE_SYNC)) {
> -		DRM_DEBUG_KMS("composite sync not supported\n");
> +		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Composite sync not supported\n",
> +			    connector->base.id, connector->name);
>  	}
>  
>  	/* it is incorrect if hsync/vsync width is zero */
>  	if (!hsync_pulse_width || !vsync_pulse_width) {
> -		DRM_DEBUG_KMS("Incorrect Detailed timing. "
> -				"Wrong Hsync/Vsync pulse width\n");
> +		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Incorrect Detailed timing. Wrong Hsync/Vsync pulse width\n",
> +			    connector->base.id, connector->name);
>  		return NULL;
>  	}
>  
> @@ -3976,7 +3980,8 @@ add_cvt_modes(struct drm_connector *connector, const struct drm_edid *drm_edid)
>  	return closure.modes;
>  }
>  
> -static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode);
> +static void fixup_detailed_cea_mode_clock(struct drm_connector *connector,
> +					  struct drm_display_mode *mode);
>  
>  static void
>  do_detailed_mode(const struct detailed_timing *timing, void *c)
> @@ -3987,7 +3992,7 @@ do_detailed_mode(const struct detailed_timing *timing, void *c)
>  	if (!is_detailed_timing_descriptor(timing))
>  		return;
>  
> -	newmode = drm_mode_detailed(closure->connector->dev,
> +	newmode = drm_mode_detailed(closure->connector,
>  				    closure->drm_edid, timing,
>  				    closure->quirks);
>  	if (!newmode)
> @@ -4001,7 +4006,7 @@ do_detailed_mode(const struct detailed_timing *timing, void *c)
>  	 * so fix up anything that looks like CEA/HDMI mode, but the clock
>  	 * is just slightly off.
>  	 */
> -	fixup_detailed_cea_mode_clock(newmode);
> +	fixup_detailed_cea_mode_clock(closure->connector, newmode);
>  
>  	drm_mode_probed_add(closure->connector, newmode);
>  	closure->modes++;
> @@ -4663,7 +4668,8 @@ static int add_hdmi_mode(struct drm_connector *connector, u8 vic)
>  	struct drm_display_mode *newmode;
>  
>  	if (!drm_valid_hdmi_vic(vic)) {
> -		DRM_ERROR("Unknown HDMI VIC: %d\n", vic);
> +		drm_err(connector->dev, "[CONNECTOR:%d:%s] Unknown HDMI VIC: %d\n",
> +			connector->base.id, connector->name, vic);
>  		return 0;
>  	}
>  
> @@ -5270,7 +5276,8 @@ static int add_cea_modes(struct drm_connector *connector,
>  	return modes;
>  }
>  
> -static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode)
> +static void fixup_detailed_cea_mode_clock(struct drm_connector *connector,
> +					  struct drm_display_mode *mode)
>  {
>  	const struct drm_display_mode *cea_mode;
>  	int clock1, clock2, clock;
> @@ -5308,8 +5315,10 @@ static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode)
>  	if (mode->clock == clock)
>  		return;
>  
> -	DRM_DEBUG("detailed mode matches %s VIC %d, adjusting clock %d -> %d\n",
> -		  type, vic, mode->clock, clock);
> +	drm_dbg_kms(connector->dev,
> +		    "[CONNECTOR:%d:%s] detailed mode matches %s VIC %d, adjusting clock %d -> %d\n",
> +		    connector->base.id, connector->name,
> +		    type, vic, mode->clock, clock);
>  	mode->clock = clock;
>  }
>  
> @@ -5417,15 +5426,12 @@ drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
>  	if (len >= 12)
>  		connector->audio_latency[1] = db[12];
>  
> -	DRM_DEBUG_KMS("HDMI: latency present %d %d, "
> -		      "video latency %d %d, "
> -		      "audio latency %d %d\n",
> -		      connector->latency_present[0],
> -		      connector->latency_present[1],
> -		      connector->video_latency[0],
> -		      connector->video_latency[1],
> -		      connector->audio_latency[0],
> -		      connector->audio_latency[1]);
> +	drm_dbg_kms(connector->dev,
> +		    "[CONNECTOR:%d:%s] HDMI: latency present %d %d, video latency %d %d, audio latency %d %d\n",
> +		    connector->base.id, connector->name,
> +		    connector->latency_present[0], connector->latency_present[1],
> +		    connector->video_latency[0], connector->video_latency[1],
> +		    connector->audio_latency[0], connector->audio_latency[1]);
>  }
>  
>  static void
> @@ -5523,7 +5529,9 @@ static void drm_edid_to_eld(struct drm_connector *connector,
>  		return;
>  
>  	mnl = get_monitor_name(drm_edid, &eld[DRM_ELD_MONITOR_NAME_STRING]);
> -	DRM_DEBUG_KMS("ELD monitor %s\n", &eld[DRM_ELD_MONITOR_NAME_STRING]);
> +	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] ELD monitor %s\n",
> +		    connector->base.id, connector->name,
> +		    &eld[DRM_ELD_MONITOR_NAME_STRING]);
>  
>  	eld[DRM_ELD_CEA_EDID_VER_MNL] = info->cea_rev << DRM_ELD_CEA_EDID_VER_SHIFT;
>  	eld[DRM_ELD_CEA_EDID_VER_MNL] |= mnl;
> @@ -5577,8 +5585,9 @@ static void drm_edid_to_eld(struct drm_connector *connector,
>  	eld[DRM_ELD_BASELINE_ELD_LEN] =
>  		DIV_ROUND_UP(drm_eld_calc_baseline_block_size(eld), 4);
>  
> -	DRM_DEBUG_KMS("ELD size %d, SAD count %d\n",
> -		      drm_eld_size(eld), total_sad_count);
> +	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] ELD size %d, SAD count %d\n",
> +		    connector->base.id, connector->name,
> +		    drm_eld_size(eld), total_sad_count);
>  }
>  
>  static int _drm_edid_to_sad(const struct drm_edid *drm_edid,
> @@ -5849,7 +5858,8 @@ static void drm_parse_vcdb(struct drm_connector *connector, const u8 *db)
>  {
>  	struct drm_display_info *info = &connector->display_info;
>  
> -	DRM_DEBUG_KMS("CEA VCDB 0x%02x\n", db[2]);
> +	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] CEA VCDB 0x%02x\n",
> +		    connector->base.id, connector->name, db[2]);
>  
>  	if (db[2] & EDID_CEA_VCDB_QS)
>  		info->rgb_quant_range_selectable = true;
> @@ -6052,39 +6062,39 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>  	if (hdmi[6] & DRM_EDID_HDMI_DC_30) {
>  		dc_bpc = 10;
>  		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_30;
> -		DRM_DEBUG("%s: HDMI sink does deep color 30.\n",
> -			  connector->name);
> +		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does deep color 30.\n",
> +			    connector->base.id, connector->name);
>  	}
>  
>  	if (hdmi[6] & DRM_EDID_HDMI_DC_36) {
>  		dc_bpc = 12;
>  		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_36;
> -		DRM_DEBUG("%s: HDMI sink does deep color 36.\n",
> -			  connector->name);
> +		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does deep color 36.\n",
> +			    connector->base.id, connector->name);
>  	}
>  
>  	if (hdmi[6] & DRM_EDID_HDMI_DC_48) {
>  		dc_bpc = 16;
>  		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_48;
> -		DRM_DEBUG("%s: HDMI sink does deep color 48.\n",
> -			  connector->name);
> +		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does deep color 48.\n",
> +			    connector->base.id, connector->name);
>  	}
>  
>  	if (dc_bpc == 0) {
> -		DRM_DEBUG("%s: No deep color support on this HDMI sink.\n",
> -			  connector->name);
> +		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] No deep color support on this HDMI sink.\n",
> +			    connector->base.id, connector->name);
>  		return;
>  	}
>  
> -	DRM_DEBUG("%s: Assigning HDMI sink color depth as %d bpc.\n",
> -		  connector->name, dc_bpc);
> +	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Assigning HDMI sink color depth as %d bpc.\n",
> +		    connector->base.id, connector->name, dc_bpc);
>  	info->bpc = dc_bpc;
>  
>  	/* YCRCB444 is optional according to spec. */
>  	if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) {
>  		info->edid_hdmi_ycbcr444_dc_modes = info->edid_hdmi_rgb444_dc_modes;
> -		DRM_DEBUG("%s: HDMI sink does YCRCB444 in deep color.\n",
> -			  connector->name);
> +		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink does YCRCB444 in deep color.\n",
> +			    connector->base.id, connector->name);
>  	}
>  
>  	/*
> @@ -6092,8 +6102,8 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>  	 * then deep color 36 bit must be supported.
>  	 */
>  	if (!(hdmi[6] & DRM_EDID_HDMI_DC_36)) {
> -		DRM_DEBUG("%s: HDMI sink should do DC_36, but does not!\n",
> -			  connector->name);
> +		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI sink should do DC_36, but does not!\n",
> +			    connector->base.id, connector->name);
>  	}
>  }
>  
> @@ -6110,10 +6120,9 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
>  	if (len >= 7)
>  		info->max_tmds_clock = db[7] * 5000;
>  
> -	DRM_DEBUG_KMS("HDMI: DVI dual %d, "
> -		      "max TMDS clock %d kHz\n",
> -		      info->dvi_dual,
> -		      info->max_tmds_clock);
> +	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] HDMI: DVI dual %d, max TMDS clock %d kHz\n",
> +		    connector->base.id, connector->name,
> +		    info->dvi_dual, info->max_tmds_clock);
>  
>  	drm_parse_hdmi_deep_color_info(connector, db);
>  }
> @@ -6156,8 +6165,10 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  			info->cea_rev = edid_ext[1];
>  
>  		if (info->cea_rev != edid_ext[1])
> -			DRM_DEBUG_KMS("CEA extension version mismatch %u != %u\n",
> -				      info->cea_rev, edid_ext[1]);
> +			drm_dbg_kms(connector->dev,
> +				    "[CONNECTOR:%d:%s] CEA extension version mismatch %u != %u\n",
> +				    connector->base.id, connector->name,
> +				    info->cea_rev, edid_ext[1]);
>  
>  		/* The existence of a CTA extension should imply RGB support */
>  		info->color_formats = DRM_COLOR_FORMAT_RGB444;
> @@ -6243,9 +6254,10 @@ static void drm_get_monitor_range(struct drm_connector *connector,
>  
>  	drm_for_each_detailed_block(drm_edid, get_monitor_range, &closure);
>  
> -	DRM_DEBUG_KMS("Supported Monitor Refresh rate range is %d Hz - %d Hz\n",
> -		      info->monitor_range.min_vfreq,
> -		      info->monitor_range.max_vfreq);
> +	drm_dbg_kms(connector->dev,
> +		    "[CONNECTOR:%d:%s] Supported Monitor Refresh rate range is %d Hz - %d Hz\n",
> +		    connector->base.id, connector->name,
> +		    info->monitor_range.min_vfreq, info->monitor_range.max_vfreq);
>  }
>  
>  static void drm_parse_vesa_mso_data(struct drm_connector *connector,
> @@ -6387,8 +6399,9 @@ static u32 update_display_info(struct drm_connector *connector,
>  	if (info->bpc == 0 && edid->revision == 3 &&
>  	    edid->input & DRM_EDID_DIGITAL_DFP_1_X) {
>  		info->bpc = 8;
> -		DRM_DEBUG("%s: Assigning DFP sink color depth as %d bpc.\n",
> -			  connector->name, info->bpc);
> +		drm_dbg_kms(connector->dev,
> +			    "[CONNECTOR:%d:%s] Assigning DFP sink color depth as %d bpc.\n",
> +			    connector->base.id, connector->name, info->bpc);
>  	}
>  
>  	/* Only defined for 1.4 with digital displays */
> @@ -6420,8 +6433,9 @@ static u32 update_display_info(struct drm_connector *connector,
>  		break;
>  	}
>  
> -	DRM_DEBUG("%s: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
> -			  connector->name, info->bpc);
> +	drm_dbg_kms(connector->dev,
> +		    "[CONNECTOR:%d:%s] Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
> +		    connector->base.id, connector->name, info->bpc);
>  
>  	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
>  		info->color_formats |= DRM_COLOR_FORMAT_YCBCR444;
> @@ -7121,11 +7135,14 @@ static void drm_parse_tiled_block(struct drm_connector *connector,
>  	connector->tile_h_size = w + 1;
>  	connector->tile_v_size = h + 1;
>  
> -	DRM_DEBUG_KMS("tile cap 0x%x\n", tile->tile_cap);
> -	DRM_DEBUG_KMS("tile_size %d x %d\n", w + 1, h + 1);
> -	DRM_DEBUG_KMS("topo num tiles %dx%d, location %dx%d\n",
> -		      num_h_tile + 1, num_v_tile + 1, tile_h_loc, tile_v_loc);
> -	DRM_DEBUG_KMS("vend %c%c%c\n", tile->topology_id[0], tile->topology_id[1], tile->topology_id[2]);
> +	drm_dbg_kms(connector->dev,
> +		    "[CONNECTOR:%d:%s] tile cap 0x%x, size %dx%d, num tiles %dx%d, location %dx%d, vend %c%c%c",
> +		    connector->base.id, connector->name,
> +		    tile->tile_cap,
> +		    connector->tile_h_size, connector->tile_v_size,
> +		    connector->num_h_tile, connector->num_v_tile,
> +		    connector->tile_h_loc, connector->tile_v_loc,
> +		    tile->topology_id[0], tile->topology_id[1], tile->topology_id[2]);
>  
>  	tg = drm_mode_get_tile_group(connector->dev, tile->topology_id);
>  	if (!tg)
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

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

* Re: [PATCH v2 09/16] drm/edid: detach debugfs EDID override from EDID property update
  2022-10-24 12:33 ` [PATCH v2 09/16] drm/edid: detach debugfs EDID override from EDID property update Jani Nikula
@ 2022-10-24 17:53   ` Ville Syrjälä
  0 siblings, 0 replies; 23+ messages in thread
From: Ville Syrjälä @ 2022-10-24 17:53 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, dri-devel

On Mon, Oct 24, 2022 at 03:33:37PM +0300, Jani Nikula wrote:
> Having the EDID override debugfs directly update the EDID property is
> problematic. The update is partial only. The driver has no way of
> knowing it's been updated. Mode list is not updated. It's an
> inconsistent state.
> 
> Detach debugfs EDID override from the property update completely. Only
> set and reset a separate override EDID copy from debugfs, and have it
> take effect only at detect (via EDID read). The copy is at
> connector->edid_override, protected by connector->edid_override_mutex.
> 
> This also brings override EDID closer to firmware EDID in behaviour.
> 
> Add validation of the override EDID which we completely lacked.
> 
> Note that IGT already forces a detect whenever tests update the override
> EDID.
> 
> v2: Add locking (Ville)
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_connector.c |  1 +
>  drivers/gpu/drm/drm_edid.c      | 73 ++++++++++++++++-----------------
>  include/drm/drm_connector.h     | 16 ++++++--
>  3 files changed, 49 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 61c29ce74b03..f25674c0d41e 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -274,6 +274,7 @@ static int __drm_connector_init(struct drm_device *dev,
>  	INIT_LIST_HEAD(&connector->probed_modes);
>  	INIT_LIST_HEAD(&connector->modes);
>  	mutex_init(&connector->mutex);
> +	mutex_init(&connector->edid_override_mutex);
>  	connector->edid_blob_ptr = NULL;
>  	connector->epoch_counter = 0;
>  	connector->tile_blob_ptr = NULL;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index c3cf942186b7..b8d07e4d6215 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2207,8 +2207,12 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
>  {
>  	struct edid *override = NULL;
>  
> -	if (connector->override_edid)
> -		override = drm_edid_duplicate(connector->edid_blob_ptr->data);
> +	mutex_lock(&connector->edid_override_mutex);
> +
> +	if (connector->edid_override)
> +		override = drm_edid_duplicate(connector->edid_override->edid);
> +
> +	mutex_unlock(&connector->edid_override_mutex);

I was first thinking we'd use connection_mutex or something for this,
but given no one should anymore access this thing directly a dedicated
mutex seems like a fine choice. At least it's clear what it protects.

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

>  
>  	if (!override)
>  		override = drm_load_edid_firmware(connector);
> @@ -2223,10 +2227,15 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector,
>  /* For debugfs edid_override implementation */
>  int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m)
>  {
> -	struct drm_property_blob *edid = connector->edid_blob_ptr;
> +	const struct drm_edid *drm_edid;
>  
> -	if (connector->override_edid && edid)
> -		seq_write(m, edid->data, edid->length);
> +	mutex_lock(&connector->edid_override_mutex);
> +
> +	drm_edid = connector->edid_override;
> +	if (drm_edid)
> +		seq_write(m, drm_edid->edid, drm_edid->size);
> +
> +	mutex_unlock(&connector->edid_override_mutex);
>  
>  	return 0;
>  }
> @@ -2235,32 +2244,43 @@ int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m)
>  int drm_edid_override_set(struct drm_connector *connector, const void *edid,
>  			  size_t size)
>  {
> -	int ret;
> +	const struct drm_edid *drm_edid;
>  
> -	if (size < EDID_LENGTH || edid_size(edid) > size)
> +	drm_edid = drm_edid_alloc(edid, size);
> +	if (!drm_edid_valid(drm_edid)) {
> +		drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override invalid\n",
> +			    connector->base.id, connector->name);
> +		drm_edid_free(drm_edid);
>  		return -EINVAL;
> -
> -	connector->override_edid = false;
> +	}
>  
>  	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override set\n",
>  		    connector->base.id, connector->name);
>  
> -	ret = drm_connector_update_edid_property(connector, edid);
> -	if (!ret)
> -		connector->override_edid = true;
> +	mutex_lock(&connector->edid_override_mutex);
>  
> -	return ret;
> +	drm_edid_free(connector->edid_override);
> +	connector->edid_override = drm_edid;
> +
> +	mutex_unlock(&connector->edid_override_mutex);
> +
> +	return 0;
>  }
>  
>  /* For debugfs edid_override implementation */
>  int drm_edid_override_reset(struct drm_connector *connector)
>  {
> -	connector->override_edid = false;
> -
>  	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] EDID override reset\n",
>  		    connector->base.id, connector->name);
>  
> -	return drm_connector_update_edid_property(connector, NULL);
> +	mutex_lock(&connector->edid_override_mutex);
> +
> +	drm_edid_free(connector->edid_override);
> +	connector->edid_override = NULL;
> +
> +	mutex_unlock(&connector->edid_override_mutex);
> +
> +	return 0;
>  }
>  
>  /**
> @@ -6634,23 +6654,6 @@ int drm_edid_connector_update(struct drm_connector *connector,
>  {
>  	int count;
>  
> -	/*
> -	 * FIXME: Reconcile the differences in override_edid handling between
> -	 * this and drm_connector_update_edid_property().
> -	 *
> -	 * If override_edid is set, and the EDID passed in here originates from
> -	 * drm_edid_read() and friends, it will be the override EDID, and there
> -	 * are no issues. drm_connector_update_edid_property() ignoring requests
> -	 * to set the EDID dates back to a time when override EDID was not
> -	 * handled at the low level EDID read.
> -	 *
> -	 * The only way the EDID passed in here can be different from the
> -	 * override EDID is when a driver passes in an EDID that does *not*
> -	 * originate from drm_edid_read() and friends, or passes in a stale
> -	 * cached version. This, in turn, is a question of when an override EDID
> -	 * set via debugfs should take effect.
> -	 */
> -
>  	count = _drm_edid_connector_update(connector, drm_edid);
>  
>  	_drm_update_tile_info(connector, drm_edid);
> @@ -6665,10 +6668,6 @@ EXPORT_SYMBOL(drm_edid_connector_update);
>  static int _drm_connector_update_edid_property(struct drm_connector *connector,
>  					       const struct drm_edid *drm_edid)
>  {
> -	/* ignore requests to set edid when overridden */
> -	if (connector->override_edid)
> -		return 0;
> -
>  	/*
>  	 * Set the display info, using edid if available, otherwise resetting
>  	 * the values to defaults. This duplicates the work done in
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index b1b2df48d42c..e641a4725f99 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1550,12 +1550,20 @@ struct drm_connector {
>  	struct drm_cmdline_mode cmdline_mode;
>  	/** @force: a DRM_FORCE_<foo> state for forced mode sets */
>  	enum drm_connector_force force;
> +
> +	/**
> +	 * @edid_override: Override EDID set via debugfs.
> +	 *
> +	 * Do not modify or access outside of the drm_edid_override_* family of
> +	 * functions.
> +	 */
> +	const struct drm_edid *edid_override;
> +
>  	/**
> -	 * @override_edid: has the EDID been overwritten through debugfs for
> -	 * testing? Do not modify outside of drm_edid_override_set() and
> -	 * drm_edid_override_reset().
> +	 * @edid_override_mutex: Protect access to edid_override.
>  	 */
> -	bool override_edid;
> +	struct mutex edid_override_mutex;
> +
>  	/** @epoch_counter: used to detect any other changes in connector, besides status */
>  	u64 epoch_counter;
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

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

* Re: [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes
  2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
                   ` (15 preceding siblings ...)
  2022-10-24 12:33 ` [PATCH v2 16/16] drm/edid: convert to device specific logging Jani Nikula
@ 2022-10-26  8:33 ` Jani Nikula
  16 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2022-10-26  8:33 UTC (permalink / raw)
  To: dri-devel; +Cc: intel-gfx

On Mon, 24 Oct 2022, Jani Nikula <jani.nikula@intel.com> wrote:
> v2 of drm/edid: EDID override refactoring and fixes
>
> Address review comments, add patch 15.

Thanks for the reviews, pushed the series to drm-misc-next.

BR,
Jani.


>
> BR,
> Jani.
>
>
> Jani Nikula (16):
>   drm/i915/hdmi: do dual mode detect only if connected
>   drm/i915/hdmi: stop using connector->override_edid
>   drm/amd/display: stop using connector->override_edid
>   drm/edid: debug log EDID override set/reset
>   drm/edid: abstract debugfs override EDID show better
>   drm/edid: rename drm_add_override_edid_modes() to
>     drm_edid_override_connector_update()
>   drm/edid: split drm_edid block count helper
>   drm/edid: add function for checking drm_edid validity
>   drm/edid: detach debugfs EDID override from EDID property update
>   drm/edid/firmware: drop redundant connector_name variable/parameter
>   drm/edid/firmware: rename drm_load_edid_firmware() to
>     drm_edid_load_firmware()
>   drm/edid: use struct drm_edid for override/firmware EDID
>   drm/edid: move edid load declarations to internal header
>   drm/edid/firmware: convert to drm device specific logging
>   drm/edid: add [CONNECTOR:%d:%s] to debug logging
>   drm/edid: convert to device specific logging
>
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   3 -
>  drivers/gpu/drm/drm_connector.c               |   1 +
>  drivers/gpu/drm/drm_crtc_internal.h           |  15 +-
>  drivers/gpu/drm/drm_debugfs.c                 |   8 +-
>  drivers/gpu/drm/drm_edid.c                    | 346 +++++++++++-------
>  drivers/gpu/drm/drm_edid_load.c               | 109 ++----
>  drivers/gpu/drm/drm_probe_helper.c            |   2 +-
>  drivers/gpu/drm/i915/display/intel_hdmi.c     |  20 +-
>  include/drm/drm_connector.h                   |  16 +-
>  include/drm/drm_edid.h                        |  10 +-
>  10 files changed, 283 insertions(+), 247 deletions(-)

-- 
Jani Nikula, Intel Open Source Graphics Center

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

end of thread, other threads:[~2022-10-26  8:33 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-24 12:33 [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula
2022-10-24 12:33 ` [PATCH v2 01/16] drm/i915/hdmi: do dual mode detect only if connected Jani Nikula
2022-10-24 12:33 ` [PATCH v2 02/16] drm/i915/hdmi: stop using connector->override_edid Jani Nikula
2022-10-24 15:13   ` Ville Syrjälä
2022-10-24 12:33 ` [PATCH v2 03/16] drm/amd/display: " Jani Nikula
2022-10-24 12:33 ` [PATCH v2 04/16] drm/edid: debug log EDID override set/reset Jani Nikula
2022-10-24 12:33 ` [PATCH v2 05/16] drm/edid: abstract debugfs override EDID show better Jani Nikula
2022-10-24 12:33 ` [PATCH v2 06/16] drm/edid: rename drm_add_override_edid_modes() to drm_edid_override_connector_update() Jani Nikula
2022-10-24 12:33 ` [PATCH v2 07/16] drm/edid: split drm_edid block count helper Jani Nikula
2022-10-24 12:33 ` [PATCH v2 08/16] drm/edid: add function for checking drm_edid validity Jani Nikula
2022-10-24 12:33 ` [PATCH v2 09/16] drm/edid: detach debugfs EDID override from EDID property update Jani Nikula
2022-10-24 17:53   ` Ville Syrjälä
2022-10-24 12:33 ` [PATCH v2 10/16] drm/edid/firmware: drop redundant connector_name variable/parameter Jani Nikula
2022-10-24 12:33 ` [PATCH v2 11/16] drm/edid/firmware: rename drm_load_edid_firmware() to drm_edid_load_firmware() Jani Nikula
2022-10-24 12:33 ` [PATCH v2 12/16] drm/edid: use struct drm_edid for override/firmware EDID Jani Nikula
2022-10-24 12:33 ` [PATCH v2 13/16] drm/edid: move edid load declarations to internal header Jani Nikula
2022-10-24 12:33 ` [PATCH v2 14/16] drm/edid/firmware: convert to drm device specific logging Jani Nikula
2022-10-24 16:00   ` Ville Syrjälä
2022-10-24 12:33 ` [PATCH v2 15/16] drm/edid: add [CONNECTOR:%d:%s] to debug logging Jani Nikula
2022-10-24 13:27   ` Simon Ser
2022-10-24 12:33 ` [PATCH v2 16/16] drm/edid: convert to device specific logging Jani Nikula
2022-10-24 16:02   ` Ville Syrjälä
2022-10-26  8:33 ` [PATCH v2 00/16] drm/edid: EDID override refactoring and fixes Jani Nikula

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).