[RFC PATCH 0/7] drm: facilitate driver unification around edid read and override

[RFC PATCH 0/7] drm: facilitate driver unification around edid read and override

[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.

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(-)

-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 1/7] drm: reset ELD if NULL edid is passed to drm_edid_to_eld

[Jani Nikula]

Make the function useful for resetting the ELD.

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

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 67d6a73731d8..fead6622e4b5 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3424,6 +3424,9 @@ void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
 
 	memset(eld, 0, sizeof(connector->eld));
 
+	if (!edid)
+		return;
+
 	connector->latency_present[0] = false;
 	connector->latency_present[1] = false;
 	connector->video_latency[0] = 0;
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 2/7] drm: move edid property update and add modes out of edid firmware loader

[Jani Nikula]

Make the firmware loader more generic and generally useful. In
particular, make it possible for the caller to distinguish between
firmware being there but without modes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid_load.c    | 17 ++++-------------
 drivers/gpu/drm/drm_probe_helper.c |  8 +++++++-
 include/drm/drm_edid.h             |  7 ++++---
 3 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 622f788bff46..1c0495acf341 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -256,15 +256,14 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 	return edid;
 }
 
-int drm_load_edid_firmware(struct drm_connector *connector)
+struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 {
 	const char *connector_name = connector->name;
 	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
-	int ret;
 	struct edid *edid;
 
 	if (edid_firmware[0] == '\0')
-		return 0;
+		return ERR_PTR(-ENOENT);
 
 	/*
 	 * If there are multiple edid files specified and separated
@@ -293,7 +292,7 @@ int drm_load_edid_firmware(struct drm_connector *connector)
 	if (!edidname) {
 		if (!fallback) {
 			kfree(fwstr);
-			return 0;
+			return ERR_PTR(-ENOENT);
 		}
 		edidname = fallback;
 	}
@@ -305,13 +304,5 @@ int drm_load_edid_firmware(struct drm_connector *connector)
 	edid = edid_load(connector, edidname, connector_name);
 	kfree(fwstr);
 
-	if (IS_ERR_OR_NULL(edid))
-		return 0;
-
-	drm_mode_connector_update_edid_property(connector, edid);
-	ret = drm_add_edid_modes(connector, edid);
-	drm_edid_to_eld(connector, edid);
-	kfree(edid);
-
-	return ret;
+	return edid;
 }
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 7cff91e7497f..fe65af8f48b9 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -298,7 +298,13 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		count = drm_add_edid_modes(connector, edid);
 		drm_edid_to_eld(connector, edid);
 	} else {
-		count = drm_load_edid_firmware(connector);
+		struct edid *edid = drm_load_edid_firmware(connector);
+		if (!IS_ERR_OR_NULL(edid)) {
+			drm_mode_connector_update_edid_property(connector, edid);
+			count = drm_add_edid_modes(connector, edid);
+			drm_edid_to_eld(connector, edid);
+			kfree(edid);
+		}
 		if (count == 0)
 			count = (*connector_funcs->get_modes)(connector);
 	}
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 38eabf65f19d..99c812ca5d20 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -332,11 +332,12 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
-int drm_load_edid_firmware(struct drm_connector *connector);
+struct edid *drm_load_edid_firmware(struct drm_connector *connector);
 #else
-static inline int drm_load_edid_firmware(struct drm_connector *connector)
+static inline struct edid *
+drm_load_edid_firmware(struct drm_connector *connector)
 {
-	return 0;
+	return ERR_PTR(-ENOENT);
 }
 #endif
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC PATCH 3/7] drm: abstract override and firmware edid

[Jani Nikula]

The functional change is that if firmware edid is present, it'll still
bypass get_modes even if number of modes is 0. This is in line with
override edid handling.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_probe_helper.c | 44 ++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index fe65af8f48b9..431349673846 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -163,6 +163,32 @@ drm_connector_detect(struct drm_connector *connector, bool force)
 		connector_status_connected;
 }
 
+static bool bypass_edid(struct drm_connector *connector, int *count)
+{
+	struct edid *edid;
+
+	if (connector->override_edid) {
+		edid = (struct edid *) connector->edid_blob_ptr->data;
+
+		*count = drm_add_edid_modes(connector, edid);
+		drm_edid_to_eld(connector, edid);
+
+		return true;
+	}
+
+	edid = drm_load_edid_firmware(connector);
+	if (!IS_ERR_OR_NULL(edid)) {
+		drm_mode_connector_update_edid_property(connector, edid);
+		*count = drm_add_edid_modes(connector, edid);
+		drm_edid_to_eld(connector, edid);
+		kfree(edid);
+
+		return true;
+	}
+
+	return false;
+}
+
 /**
  * drm_helper_probe_single_connector_modes - get complete set of display modes
  * @connector: connector to probe
@@ -292,22 +318,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		goto prune;
 	}
 
-	if (connector->override_edid) {
-		struct edid *edid = (struct edid *) connector->edid_blob_ptr->data;
-
-		count = drm_add_edid_modes(connector, edid);
-		drm_edid_to_eld(connector, edid);
-	} else {
-		struct edid *edid = drm_load_edid_firmware(connector);
-		if (!IS_ERR_OR_NULL(edid)) {
-			drm_mode_connector_update_edid_property(connector, edid);
-			count = drm_add_edid_modes(connector, edid);
-			drm_edid_to_eld(connector, edid);
-			kfree(edid);
-		}
-		if (count == 0)
-			count = (*connector_funcs->get_modes)(connector);
-	}
+	if (!bypass_edid(connector, &count))
+		count = (*connector_funcs->get_modes)(connector);
 
 	if (count == 0 && connector->status == connector_status_connected)
 		count = drm_add_modes_noedid(connector, 1024, 768);
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 4/7] drm: export load edid firmware

[Jani Nikula]

Will be needed in drm in followup work.

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

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 1c0495acf341..7857864f7878 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -306,3 +306,4 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 
 	return edid;
 }
+EXPORT_SYMBOL(drm_load_edid_firmware);
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 5/7] drm: make drm_get_displayid() available outside of drm_edid.c

[Jani Nikula]

Will be needed in followup work.

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

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index fead6622e4b5..56c5065bb314 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1060,9 +1060,6 @@ module_param_named(edid_fixup, edid_fixup, int, 0400);
 MODULE_PARM_DESC(edid_fixup,
 		 "Minimum number of valid EDID header bytes (0-8, default 6)");
 
-static void drm_get_displayid(struct drm_connector *connector,
-			      struct edid *edid);
-
 static int drm_edid_block_checksum(const u8 *raw_edid)
 {
 	int i;
@@ -4444,8 +4441,8 @@ static int drm_parse_display_id(struct drm_connector *connector,
 	return 0;
 }
 
-static void drm_get_displayid(struct drm_connector *connector,
-			      struct edid *edid)
+void drm_get_displayid(struct drm_connector *connector,
+		       struct edid *edid)
 {
 	void *displayid = NULL;
 	int ret;
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 99c812ca5d20..5d6c44fdb47f 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -435,6 +435,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter);
 struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
 				     struct i2c_adapter *adapter);
+void drm_get_displayid(struct drm_connector *connector, struct edid *edid);
 struct edid *drm_edid_duplicate(const struct edid *edid);
 int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
 
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 6/7] drm: add new drm_mode_connector_get_edid to do drm_get_edid and friends

[Jani Nikula]

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 <jani.nikula@intel.com>
---
 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(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 3115db2ae6b1..b9636c98dbf3 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -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);
+
+	if (!edid) {
+		edid = drm_load_edid_firmware(connector);
+		if (IS_ERR(edid))
+			edid = NULL;
+	}
+
+	if (edid) {
+		/* Require successful probe for override/cached/firmware EDID */
+		if (drm_probe_ddc(adapter))
+			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)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 431349673846..20a175a775d6 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -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;
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 6e352a0b5c81..0c85ddc422de 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -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
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 6/7] drm: add new drm_mode_connector_get_edid to do drm_get_edid and friends

[Daniel Vetter]

On Tue, Dec 27, 2016 at 06:21:53PM +0200, Jani Nikula wrote:
> 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 <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_connector.c    | 60 ++++++++++++++++++++++++++++++++++++++

It's a helper, but placed in drm.ko core. Moving it into drm_kms_helper.ko
should remove the need for some of the experts, and that way we should
also be able to unload modules again.

>  drivers/gpu/drm/drm_probe_helper.c |  7 +++++
>  include/drm/drm_connector.h        |  6 ++++
>  3 files changed, 73 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 3115db2ae6b1..b9636c98dbf3 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -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);
> +
> +	if (!edid) {
> +		edid = drm_load_edid_firmware(connector);
> +		if (IS_ERR(edid))
> +			edid = NULL;
> +	}
> +
> +	if (edid) {
> +		/* Require successful probe for override/cached/firmware EDID */
> +		if (drm_probe_ddc(adapter))

This doesn't take into account hpd or connector status overwriting. Not
sure how to solve that, but probably we need to push those down a few
layers, too.

There's also the annoying split of ->detect vs. ->get_modes, and I think
if you want to clean up this entire mess, then unifying it all would be
really good. With caching and everything there's no real need to split
things into parts.

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

> +			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)
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 431349673846..20a175a775d6 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -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;
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 6e352a0b5c81..0c85ddc422de 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -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
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 6/7] drm: add new drm_mode_connector_get_edid to do drm_get_edid and friends

[Jani Nikula]

On Tue, 27 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> 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.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 6/7] drm: add new drm_mode_connector_get_edid to do drm_get_edid and friends

[Daniel Vetter]

On Wed, Dec 28, 2016 at 10:39:17AM +0200, Jani Nikula wrote:
> On Tue, 27 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> > 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.

Yup, agreed on that. Semi-random other idea: What about introducing a new
->probe helper callback on connectors which would essentially entai all
the things you want to change? I.e. what's currently done with ->detect
plus connector status forcing, and the ->get_modes vs. edid override
stuff?

Step 1 would be to have a default implementation that does exactly the
same thing. Plus reworking the hdp and poll helpers to prefer ->probe over
just calling ->detect. Although for hpd I'm not sure whether using the
same callback is a good idea really, at least in i915 we have a separate
->hpd callback for DP. And it needs it.

And then we can try to figure out how to re-split responsibilities between
drivers and helpers (and maybe try to stuff parts of it into specialized
helpers, e.g. for DP).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC PATCH 7/7] drm/i915: replace intel_ddc_get_modes with drm_mode_connector_get_edid

[Jani Nikula]

Don't do caching for now to minimize changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 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 +-
 4 files changed, 4 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 025e4c8b3e63..f467eaacd9c7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1578,7 +1578,6 @@ bool intel_is_dual_link_lvds(struct drm_device *dev);
 /* intel_modes.c */
 int intel_connector_update_modes(struct drm_connector *connector,
 				 struct edid *edid);
-int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
 void intel_attach_force_audio_property(struct drm_connector *connector);
 void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
 void intel_attach_aspect_ratio_property(struct drm_connector *connector);
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 50da89dcb92b..2bab6012bf0f 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -319,8 +319,9 @@ static int intel_dvo_get_modes(struct drm_connector *connector)
 	 * (TV-out, for example), but for now with just TMDS and LVDS,
 	 * that's not the case.
 	 */
-	intel_ddc_get_modes(connector,
-			    intel_gmbus_get_adapter(dev_priv, GMBUS_PIN_DPC));
+	drm_mode_connector_get_edid(connector,
+				    intel_gmbus_get_adapter(dev_priv, GMBUS_PIN_DPC),
+				    NULL, true);
 	if (!list_empty(&connector->probed_modes))
 		return 1;
 
diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
index 951e834dd274..4aafc0a5b953 100644
--- a/drivers/gpu/drm/i915/intel_modes.c
+++ b/drivers/gpu/drm/i915/intel_modes.c
@@ -47,29 +47,6 @@ int intel_connector_update_modes(struct drm_connector *connector,
 	return ret;
 }
 
-/**
- * intel_ddc_get_modes - get modelist from monitor
- * @connector: DRM connector device to use
- * @adapter: i2c adapter
- *
- * Fetch the EDID information from @connector using the DDC bus.
- */
-int intel_ddc_get_modes(struct drm_connector *connector,
-			struct i2c_adapter *adapter)
-{
-	struct edid *edid;
-	int ret;
-
-	edid = drm_get_edid(connector, adapter);
-	if (!edid)
-		return 0;
-
-	ret = intel_connector_update_modes(connector, edid);
-	kfree(edid);
-
-	return ret;
-}
-
 static const struct drm_prop_enum_list force_audio_names[] = {
 	{ HDMI_AUDIO_OFF_DVI, "force-dvi" },
 	{ HDMI_AUDIO_OFF, "off" },
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 2ad13903a054..b63971b1de56 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1948,7 +1948,7 @@ static void intel_sdvo_get_lvds_modes(struct drm_connector *connector)
 	 * Assume that the preferred modes are
 	 * arranged in priority order.
 	 */
-	intel_ddc_get_modes(connector, &intel_sdvo->ddc);
+	drm_mode_connector_get_edid(connector, &intel_sdvo->ddc, NULL, true);
 
 	list_for_each_entry(newmode, &connector->probed_modes, head) {
 		if (newmode->type & DRM_MODE_TYPE_PREFERRED) {
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm: facilitate driver unification around edid read and override

[Patchwork]

== Series Details ==

Series: drm: facilitate driver unification around edid read and override
URL   : https://patchwork.freedesktop.org/series/17226/
State : success

== Summary ==

Series 17226v1 drm: facilitate driver unification around edid read and override
https://patchwork.freedesktop.org/api/1.0/series/17226/revisions/1/mbox/

Test kms_force_connector_basic:
        Subgroup force-connector-state:
                dmesg-warn -> PASS       (fi-ivb-3770)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:82   pass:69   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

0d15e733fab5dd48154dda2bebdacd1d87069d08 drm-tip: 2016y-12m-27d-15h-47m-15s UTC integration manifest
5e99c43 drm/i915: replace intel_ddc_get_modes with drm_mode_connector_get_edid
c59cae8 drm: add new drm_mode_connector_get_edid to do drm_get_edid and friends
b1730b7 drm: make drm_get_displayid() available outside of drm_edid.c
4469ca5 drm: export load edid firmware
02bfed6 drm: abstract override and firmware edid
5143e80 drm: move edid property update and add modes out of edid firmware loader
943256e drm: reset ELD if NULL edid is passed to drm_edid_to_eld

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3395/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 0/7] drm: facilitate driver unification around edid read and override

[Daniel Vetter]

On Tue, Dec 27, 2016 at 06:21:47PM +0200, Jani Nikula wrote:
> 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.

> 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.

> 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.

> 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

> 
> 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(-)
> 
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH 0/7] drm: facilitate driver unification around edid read and override

[Daniel Vetter]

On Tue, Dec 27, 2016 at 07:41:47PM +0100, Daniel Vetter wrote:
> On Tue, Dec 27, 2016 at 06:21:47PM +0200, Jani Nikula wrote:
> > 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.
> 
> > 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.
> 
> > 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.

5) Also integrate the connector status lifetime counter thing I discussed
with Chris, so that we can start to catch&report changes besides
connector->status (which is atm the only thing the probe helpers bother to
track). E.g. if you have hdmi repeaters, they indicate downstream changes
in a changed edid. Currently we fail to send out a hdp event for that.
-Daniel

> 
> > 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
> 
> > 
> > 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(-)
> > 
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/7] drm: facilitate driver unification around edid read and override

[Jani Nikula]

On Tue, 27 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Dec 27, 2016 at 06:21:47PM +0200, Jani Nikula wrote:
>> 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.
>
>> 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.
>
>> 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 ...

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?

> 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.
>
>> 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.

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?

BR,
Jani.



> -Daniel
>
>> 
>> 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(-)
>> 
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 0/7] drm: facilitate driver unification around edid read and override

[Daniel Vetter]

On Wed, Dec 28, 2016 at 11:23:42AM +0200, Jani Nikula wrote:
> On Tue, 27 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Dec 27, 2016 at 06:21:47PM +0200, Jani Nikula wrote:
> >> 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.
> >
> >> 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.
> >
> >> 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 ...
> 
> 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?

I think a big part for all that caching is the split between ->detect and
->get_modes. That simply seems to cause pain all over. Caching the EDID
beyond that is where it gets really tricky, and afaik (haven't re-read all
the drivers) i915 is the only one that tries to do that somewhat. Or at
least did (not sure again, since detect/get_modes + hotplug makes a fine
mess).

> > 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.
> >
> >> 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.
> 
> 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?

Very much +1 on that goal, since it would allow us to test fun stuff like
audio, screen bpp limits and kinds of fun. I want this since years. I'm
just not sure how to do it properly, since indeed this entire area has a
lot of "organically evolved" taste to it ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx