drm: facilitate driver unification around edid read and override

Add a replacement for drm_get_edid that handles override and firmware EDID at the low level, and handles EDID property update, ELD update, and adds modes. This allows for a more transparent EDID override, and makes it possible to unify the drivers better. It also allows reusing the EDID cached in the property, and only probing the DDC.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_connector.c    | 60 ++++++++++++++++++++++++++++++++++++++

 drivers/gpu/drm/drm_probe_helper.c |  7 +++++
 include/drm/drm_connector.h        |  6 ++++
 3 files changed, 73 insertions(+)

drivers/gpu/drm/drm_connector.c +30 -0

index 3115db2ae6b1..b9636c98dbf3 100644
@@ -1086,6 +1086,66 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
 
+/**
+ * drm_mode_connector_get_edid - do it all replacement for drm_get_edid()
+ * @connector: connector to probe
+ * @adapter: I2C adapter to use for DDC
+ * @edid_out: if non-NULL, get EDID here, must be kfree'd by caller
+ * @no_cache: false to allow using cached EDID, true to force read
+ *
+ * drm_get_edid() on steroids. Handle debugfs override edid, firmware edid, and
+ * caching at the low level. Add modes, update ELD and EDID property. Using this
+ * prevents override/firmware edid usage at the higher level.
+ *
+ * @return: Number of modes from drm_add_edid_modes().
+ */
+int drm_mode_connector_get_edid(struct drm_connector *connector,
+				struct i2c_adapter *adapter,
+				struct edid **edid_out,
+				bool no_cache)
+{
+	struct drm_property_blob *prop = connector->edid_blob_ptr;
+	struct edid *edid = NULL;
+	int count = 0;
+
+	/*
+	 * If a driver uses this function on a connector, override/firmware edid
+	 * will only be used at this level.
+	 */
+	connector->low_level_override = true;
+
+	if ((connector->override_edid || !no_cache) && prop && prop->data)
+		edid = drm_edid_duplicate((struct edid *) prop->data);

drivers/gpu/drm/drm_connector.c +20 -0

+			drm_get_displayid(connector, edid);
+		else
+			edid = NULL;
+	} else {
+		edid = drm_get_edid(connector, adapter);
+	}
+
+	count = drm_add_edid_modes(connector, edid);
+	drm_edid_to_eld(connector, edid);
+	drm_mode_connector_update_edid_property(connector, edid);
+
+	if (edid_out)
+		*edid_out = edid;
+	else
+		kfree(edid);
+
+	return count;
+}
+EXPORT_SYMBOL(drm_mode_connector_get_edid);
+
 int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
 				    struct drm_property *property,
 				    uint64_t value)

drivers/gpu/drm/drm_probe_helper.c +7 -0

index 431349673846..20a175a775d6 100644
@@ -167,6 +167,13 @@ static bool bypass_edid(struct drm_connector *connector, int *count)
 {
 	struct edid *edid;
 
+	/*
+	 * If a driver uses drm_connector_get_edid() on a connector,
+	 * override/firmware edid will only be used at that level.
+	 */
+	if (connector->low_level_override)
+		return false;
+
 	if (connector->override_edid) {
 		edid = (struct edid *) connector->edid_blob_ptr->data;
 

include/drm/drm_connector.h +6 -0

index 6e352a0b5c81..0c85ddc422de 100644
@@ -25,6 +25,7 @@
 
 #include <linux/list.h>
 #include <linux/ctype.h>
+#include <linux/i2c.h>
 #include <drm/drm_mode_object.h>
 
 #include <uapi/drm/drm_mode.h>

@@ -727,6 +728,7 @@ struct drm_connector {
 	/* forced on connector */
 	struct drm_cmdline_mode cmdline_mode;
 	enum drm_connector_force force;
+	bool low_level_override;
 	bool override_edid;
 
 #define DRM_CONNECTOR_MAX_ENCODER 3

@@ -837,6 +839,10 @@ int drm_mode_connector_set_path_property(struct drm_connector *connector,
 int drm_mode_connector_set_tile_property(struct drm_connector *connector);
 int drm_mode_connector_update_edid_property(struct drm_connector *connector,
 					    const struct edid *edid);
+int drm_mode_connector_get_edid(struct drm_connector *connector,
+				struct i2c_adapter *adapter,
+				struct edid **edid_out,
+				bool no_cache);
 
 /**
  * struct drm_tile_group - Tile group metadata

Signature · 6 lines

2.1.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

I'm not sure how this all should look like.

Quoted 1 line · Daniel Vetter

I'm not sure how this all should look like.

This we definitely agree on, and hence the RFC. :)

I'm pretty sure it should *not* look like it currently does.

Hi all -

This series aims at three goals:

1) Most drivers do similar things around drm_get_edid (namely convert edid to eld, add modes, and update edid blob property). Add a helper for the drivers, to reduce code and unify.

2) Move override and firmware EDID handling to a lower level, in the helper. This makes them more transparent to the rest of the stack, avoiding special casing. For example, any drm_detect_hdmi_monitor calls typically use the EDID from the display, not the override EDID.

3) Make EDID caching easier and unified across drivers. Currently, plenty of drivers have their own hacks for caching EDID reads. This could be made more transparent and unified at the helper level.

All of this is opt-in, using the helper from patch 6/7. This is mostly because converting everything at one go is pretty much impossible. The main wrinkle is having to leave override/firmware EDID handling in two places for now, but this could be fixed once enough drivers have switched to using the common helper.

BR,
Jani.

Jani Nikula (7):
drm: reset ELD if NULL edid is passed to drm_edid_to_eld
drm: move edid property update and add modes out of edid firmware
loader
drm: abstract override and firmware edid
drm: export load edid firmware
drm: make drm_get_displayid() available outside of drm_edid.c
drm: add new drm_mode_connector_get_edid to do drm_get_edid and
friends
drm/i915: replace intel_ddc_get_modes with drm_mode_connector_get_edid

 drivers/gpu/drm/drm_connector.c    | 60 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_edid.c         | 10 +++----
 drivers/gpu/drm/drm_edid_load.c    | 18 ++++--------
 drivers/gpu/drm/drm_probe_helper.c | 45 +++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h   |  1 -
 drivers/gpu/drm/i915/intel_dvo.c   |  5 ++--
 drivers/gpu/drm/i915/intel_modes.c | 23 ---------------
 drivers/gpu/drm/i915/intel_sdvo.c  |  2 +-
 include/drm/drm_connector.h        |  6 ++++
 include/drm/drm_edid.h             |  8 +++--
 10 files changed, 120 insertions(+), 58 deletions(-)

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 7 lines · Jani Nikula

Hi all -

This series aims at three goals:

1) Most drivers do similar things around drm_get_edid (namely convert edid to eld, add modes, and update edid blob property). Add a helper for the drivers, to reduce code and unify.

I like.

Quoted 4 lines

2) Move override and firmware EDID handling to a lower level, in the helper. This makes them more transparent to the rest of the stack, avoiding special casing. For example, any drm_detect_hdmi_monitor calls typically use the EDID from the display, not the override EDID.

I replied to that patch, I think we need to rethink the helper callbacks to make this work. The general direction make sense to me though.

Quoted 3 lines

3) Make EDID caching easier and unified across drivers. Currently, plenty of drivers have their own hacks for caching EDID reads. This could be made more transparent and unified at the helper level.

Caching is Real Hard, and I don't think something generic will work:
- On DP, hpd works and will reliable tell you when you need to invalidate stuff. Except when you have a downstream port to something else (hdmi, vga, whatever). I think this is best solved by making the shared helpers for DP a lot better.

- HDMI is supposed to work, except it's not. You can't rely on hpd, any caching needs to have a time limit. I guess we could share some code here.

- Everything else is much worse, and caching is a no-go. At most a time-based cache that invalidates and re-probes.

- Built-in panels are special.

- One issue with all this is that currently the hpd helpers we have (not the stuff in i915) don't even forward which hpd signalled which connector.

- Another fun is handling invalidations in general. System suspend/resume is real fun that way ...

4) Fix the locking (well, entire lack thereof) between the probe paths (protected by mode_config.mutex), and the atomic modeset paths (protected by mode_config.connection_mutex).

Yes that's feature creep but I think any redesign of the probe code should have a answer to that too.

Quoted 6 lines

All of this is opt-in, using the helper from patch 6/7. This is mostly because converting everything at one go is pretty much impossible. The main wrinkle is having to leave override/firmware EDID handling in two places for now, but this could be fixed once enough drivers have switched to using the common helper.

I feel like we'd need a bit more conversion when merging, to make sure it all makes sense. Ending up with 2 not really useful ways to handle probing in the helpers would be worse than what we have now.
-Daniel

Quoted 35 lines

BR,
Jani.

Jani Nikula (7):
drm: reset ELD if NULL edid is passed to drm_edid_to_eld
drm: move edid property update and add modes out of edid firmware
loader
drm: abstract override and firmware edid
drm: export load edid firmware
drm: make drm_get_displayid() available outside of drm_edid.c
drm: add new drm_mode_connector_get_edid to do drm_get_edid and
friends
drm/i915: replace intel_ddc_get_modes with drm_mode_connector_get_edid

 drivers/gpu/drm/drm_connector.c    | 60 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_edid.c         | 10 +++----
 drivers/gpu/drm/drm_edid_load.c    | 18 ++++--------
 drivers/gpu/drm/drm_probe_helper.c | 45 +++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h   |  1 -
 drivers/gpu/drm/i915/intel_dvo.c   |  5 ++--
 drivers/gpu/drm/i915/intel_modes.c | 23 ---------------
 drivers/gpu/drm/i915/intel_sdvo.c  |  2 +-
 include/drm/drm_connector.h        |  6 ++++
 include/drm/drm_edid.h             |  8 +++--
 10 files changed, 120 insertions(+), 58 deletions(-)

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 7 lines · Jani Nikula

Hi all -

This series aims at three goals:

1) Most drivers do similar things around drm_get_edid (namely convert edid to eld, add modes, and update edid blob property). Add a helper for the drivers, to reduce code and unify.

I like.

Quoted 4 lines

2) Move override and firmware EDID handling to a lower level, in the helper. This makes them more transparent to the rest of the stack, avoiding special casing. For example, any drm_detect_hdmi_monitor calls typically use the EDID from the display, not the override EDID.

I replied to that patch, I think we need to rethink the helper callbacks to make this work. The general direction make sense to me though.

Quoted 3 lines

3) Make EDID caching easier and unified across drivers. Currently, plenty of drivers have their own hacks for caching EDID reads. This could be made more transparent and unified at the helper level.

Caching is Real Hard, and I don't think something generic will work:
- On DP, hpd works and will reliable tell you when you need to invalidate stuff. Except when you have a downstream port to something else (hdmi, vga, whatever). I think this is best solved by making the shared helpers for DP a lot better.

- HDMI is supposed to work, except it's not. You can't rely on hpd, any caching needs to have a time limit. I guess we could share some code here.

- Everything else is much worse, and caching is a no-go. At most a time-based cache that invalidates and re-probes.

- Built-in panels are special.

- One issue with all this is that currently the hpd helpers we have (not the stuff in i915) don't even forward which hpd signalled which connector.

- Another fun is handling invalidations in general. System suspend/resume is real fun that way ...

4) Fix the locking (well, entire lack thereof) between the probe paths (protected by mode_config.mutex), and the atomic modeset paths (protected by mode_config.connection_mutex).

Yes that's feature creep but I think any redesign of the probe code should have a answer to that too.

Quoted 5 lines

All of this is opt-in, using the helper from patch 6/7. This is mostly because converting everything at one go is pretty much impossible. The main wrinkle is having to leave override/firmware EDID handling in two places for now, but this could be fixed once enough drivers have switched to using the common helper.

I feel like we'd need a bit more conversion when merging, to make sure it all makes sense. Ending up with 2 not really useful ways to handle probing in the helpers would be worse than what we have now.

-Daniel

Quoted 35 lines

BR,
Jani.

Jani Nikula (7):
drm: reset ELD if NULL edid is passed to drm_edid_to_eld
drm: move edid property update and add modes out of edid firmware
loader
drm: abstract override and firmware edid
drm: export load edid firmware
drm: make drm_get_displayid() available outside of drm_edid.c
drm: add new drm_mode_connector_get_edid to do drm_get_edid and
friends
drm/i915: replace intel_ddc_get_modes with drm_mode_connector_get_edid

 drivers/gpu/drm/drm_connector.c    | 60 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_edid.c         | 10 +++----
 drivers/gpu/drm/drm_edid_load.c    | 18 ++++--------
 drivers/gpu/drm/drm_probe_helper.c | 45 +++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h   |  1 -
 drivers/gpu/drm/i915/intel_dvo.c   |  5 ++--
 drivers/gpu/drm/i915/intel_modes.c | 23 ---------------
 drivers/gpu/drm/i915/intel_sdvo.c  |  2 +-
 include/drm/drm_connector.h        |  6 ++++
 include/drm/drm_edid.h             |  8 +++--
 10 files changed, 120 insertions(+), 58 deletions(-)

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 36 lines · Daniel Vetter

Quoted 7 lines · Jani Nikula

Hi all -

This series aims at three goals:

1) Most drivers do similar things around drm_get_edid (namely convert edid to eld, add modes, and update edid blob property). Add a helper for the drivers, to reduce code and unify.

I like.

Quoted 4 lines

2) Move override and firmware EDID handling to a lower level, in the helper. This makes them more transparent to the rest of the stack, avoiding special casing. For example, any drm_detect_hdmi_monitor calls typically use the EDID from the display, not the override EDID.

I replied to that patch, I think we need to rethink the helper callbacks to make this work. The general direction make sense to me though.

Quoted 3 lines

3) Make EDID caching easier and unified across drivers. Currently, plenty of drivers have their own hacks for caching EDID reads. This could be made more transparent and unified at the helper level.

Caching is Real Hard, and I don't think something generic will work:
- On DP, hpd works and will reliable tell you when you need to invalidate stuff. Except when you have a downstream port to something else (hdmi, vga, whatever). I think this is best solved by making the shared helpers for DP a lot better.

- HDMI is supposed to work, except it's not. You can't rely on hpd, any caching needs to have a time limit. I guess we could share some code here.

- Everything else is much worse, and caching is a no-go. At most a time-based cache that invalidates and re-probes.

Quoted 8 lines

- Built-in panels are special.

- One issue with all this is that currently the hpd helpers we have (not the stuff in i915) don't even forward which hpd signalled which connector.

- Another fun is handling invalidations in general. System suspend/resume is real fun that way ...

So I don't disagree. But looking at all the code that uses drm_get_edid(), there's quite a bit of EDID caching around. I don't think it's all that great as it is. Mostly I'm just saying, perhaps the EDID property should be the place to store the EDID if drivers want to use a cached value? Why keep several copies around, making the invalidation even more complicated? And really, updating of the EDID property seems to be bonkers all over the place too. Perhaps we should just have helpers for the drivers to make the caching easier?

Quoted 13 lines

4) Fix the locking (well, entire lack thereof) between the probe paths (protected by mode_config.mutex), and the atomic modeset paths (protected by mode_config.connection_mutex).

Yes that's feature creep but I think any redesign of the probe code should have a answer to that too.

Quoted 5 lines

All of this is opt-in, using the helper from patch 6/7. This is mostly because converting everything at one go is pretty much impossible. The main wrinkle is having to leave override/firmware EDID handling in two places for now, but this could be fixed once enough drivers have switched to using the common helper.

Quoted 3 lines

I feel like we'd need a bit more conversion when merging, to make sure it all makes sense. Ending up with 2 not really useful ways to handle probing in the helpers would be worse than what we have now.

Sure, I just threw in something for the discussion. Unfortunately it doesn't look easy to convert everything in one go, at least it requires digging into the internals of too many drivers for any single person to easily handle.

Perhaps one outcome of the discussion should be a better idea what the aim for the override/firmware EDID really is. Currently, you can use it to use specific modes and timings, but that's about it. You can use it to test a specific part of the EDID parser, but not all. Everything else still gets used from the display EDID. So should we try to make them complete and transparent replacements of the EDID or not?