[PATCH v2 0/4] drm: handle override/firmware edid at the lowest level

[PATCH v2 0/4] drm: handle override/firmware edid at the lowest level

[Jani Nikula]

v2 of http://mid.mail-archive.com/cover.1487241304.git.jani.nikula@intel.com

BR,
Jani.


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

[PATCH v2 1/4] drm: move edid property update and add modes out of edid firmware loader

[Jani Nikula]

Make the firmware loader more generic and generally useful.

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 93381454bdf7..358957118ca9 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -311,7 +311,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 43fb0ac5eb9c..a55eea4afb61 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -331,11 +331,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

[PATCH v2 2/4] drm: do not debug log about missing CEA extensions on NULL edid

[Jani Nikula]

Make the drm_edid_to_eld() function useful for resetting, not just
setting, the ELD and HDMI VSDB data, without debug warnings about
missing CEA extensions.

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 24e7b282f16c..4bb50e0e7110 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3437,6 +3437,9 @@ void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
 	connector->video_latency[1] = 0;
 	connector->audio_latency[1] = 0;
 
+	if (!edid)
+		return;
+
 	cea = drm_find_cea_extension(edid);
 	if (!cea) {
 		DRM_DEBUG_KMS("ELD: no CEA Extension found\n");
-- 
2.1.4

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

[PATCH v2 3/4] drm/edid: respect connector force for drm_get_edid ddc probe

[Jani Nikula]

Skip DDC probe for forced connector status. Don't try to read the EDID
if the connector is forced off. Skipping probe for forced on connectors
will make more sense when drm_do_get_edid() will handle override and
firmware EDIDs.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 4bb50e0e7110..e1743ab276dc 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1428,7 +1428,10 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 {
 	struct edid *edid;
 
-	if (!drm_probe_ddc(adapter))
+	if (connector->force == DRM_FORCE_OFF)
+		return NULL;
+
+	if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter))
 		return NULL;
 
 	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
-- 
2.1.4

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

[PATCH v2 4/4] drm: handle override edid and firmware EDID at drm_do_get_edid() level

[Jani Nikula]

Handle debugfs override edid and firmware edid at the low level to
transparently and completely replace the real edid. Previously, we
practically only used the modes from the override EDID, and none of the
other data. This also prevents actual EDID reads when the EDID is to be
overridden, but retains the DDC probe.

Move firmware EDID loading from helper to core, as the functionality
moves to lower level as well. This will result in a change of module
parameter from drm_kms_helper.edid_firmware to drm.edid_firmware, which
arguably makes more sense anyway.

FIXME: validate override edid, deduplicate firmware edid validation.

v2: move firmware loading to core

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/Kconfig            |  2 +-
 drivers/gpu/drm/Makefile           |  2 +-
 drivers/gpu/drm/drm_edid.c         | 15 +++++++++++++++
 drivers/gpu/drm/drm_probe_helper.c | 19 +------------------
 4 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 90bc65d07a35..f983ef60299c 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -101,7 +101,7 @@ config DRM_FBDEV_EMULATION
 
 config DRM_LOAD_EDID_FIRMWARE
 	bool "Allow to specify an EDID data set instead of probing for it"
-	depends on DRM_KMS_HELPER
+	depends on DRM
 	help
 	  Say Y here, if you want to use EDID data to be loaded from the
 	  /lib/firmware directory or one of the provided built-in
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 92de3991fa56..a10ac095608f 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -27,13 +27,13 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
 drm-$(CONFIG_OF) += drm_of.o
 drm-$(CONFIG_AGP) += drm_agpsupport.o
 drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
+drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 
 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
 		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
 		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
 		drm_simple_kms_helper.o drm_modeset_helper.o
 
-drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
 drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e1743ab276dc..4007998d5ce3 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1309,6 +1309,10 @@ static void connector_bad_edid(struct drm_connector *connector,
  * level, drivers must make all reasonable efforts to expose it as an I2C
  * adapter and use drm_get_edid() instead of abusing this function.
  *
+ * The EDID may be overridden using debugfs override_edid or firmare EDID
+ * (drm_load_edid_firmware()), 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.
  */
 struct edid *drm_do_get_edid(struct drm_connector *connector,
@@ -1318,6 +1322,17 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 {
 	int i, j = 0, valid_extensions = 0;
 	u8 *edid, *new;
+	struct edid *override = NULL;
+
+	if (connector->override_edid)
+		override = drm_edid_duplicate((const struct edid *)
+					      connector->edid_blob_ptr->data);
+
+	if (!override)
+		override = drm_load_edid_firmware(connector);
+
+	if (!IS_ERR_OR_NULL(override))
+		return override;
 
 	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
 		return NULL;
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 358957118ca9..871326cbc465 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -199,8 +199,6 @@ drm_connector_detect(struct drm_connector *connector, bool force)
  *    drm_mode_probed_add(). New modes start their life with status as OK.
  *    Modes are added from a single source using the following priority order.
  *
- *    - debugfs 'override_edid' (used for testing only)
- *    - firmware EDID (drm_load_edid_firmware())
  *    - &drm_connector_helper_funcs.get_modes vfunc
  *    - if the connector status is connector_status_connected, standard
  *      VESA DMT modes up to 1024x768 are automatically added
@@ -305,22 +303,7 @@ 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);
-	}
+	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

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

Re: [PATCH v2 0/4] drm: handle override/firmware edid at the lowest level

[Ville Syrjälä]

On Fri, Feb 17, 2017 at 05:20:50PM +0200, Jani Nikula wrote:
> v2 of http://mid.mail-archive.com/cover.1487241304.git.jani.nikula@intel.com

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for drm: handle override/firmware edid at the lowest level (rev2)

[Patchwork]

== Series Details ==

Series: drm: handle override/firmware edid at the lowest level (rev2)
URL   : https://patchwork.freedesktop.org/series/19764/
State : success

== Summary ==

Series 19764v2 drm: handle override/firmware edid at the lowest level
https://patchwork.freedesktop.org/api/1.0/series/19764/revisions/2/mbox/

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:224  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:223  dwarn:0   dfail:0   fail:0   skip:29 

02022d17a5787709617b7897de3906970e2b0721 drm-tip: 2017y-02m-17d-15h-15m-45s UTC integration manifest
5e3f195 drm: handle override edid and firmware EDID at drm_do_get_edid() level
2c81969 drm/edid: respect connector force for drm_get_edid ddc probe
bc9819b drm: do not debug log about missing CEA extensions on NULL edid
cf94d8b drm: move edid property update and add modes out of edid firmware loader

== Logs ==

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

Re: [PATCH v2 0/4] drm: handle override/firmware edid at the lowest level

[Jani Nikula]

On Fri, 17 Feb 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 17, 2017 at 05:20:50PM +0200, Jani Nikula wrote:
>> v2 of http://mid.mail-archive.com/cover.1487241304.git.jani.nikula@intel.com
>
> lgtm. For the series
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks for the review. I pushed patches 1-3 as they're not all that
controversial, and it'll give us more coverage on the change in patch 3.

I would like to have more eyeballs and acks on patch 4 before pushing.

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: [PATCH v2 4/4] drm: handle override edid and firmware EDID at drm_do_get_edid() level

[Daniel Vetter]

On Fri, Feb 17, 2017 at 05:20:54PM +0200, Jani Nikula wrote:
> Handle debugfs override edid and firmware edid at the low level to
> transparently and completely replace the real edid. Previously, we
> practically only used the modes from the override EDID, and none of the
> other data. This also prevents actual EDID reads when the EDID is to be
> overridden, but retains the DDC probe.
> 
> Move firmware EDID loading from helper to core, as the functionality
> moves to lower level as well. This will result in a change of module
> parameter from drm_kms_helper.edid_firmware to drm.edid_firmware, which
> arguably makes more sense anyway.
> 
> FIXME: validate override edid, deduplicate firmware edid validation.
> 
> v2: move firmware loading to core
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/Kconfig            |  2 +-
>  drivers/gpu/drm/Makefile           |  2 +-
>  drivers/gpu/drm/drm_edid.c         | 15 +++++++++++++++
>  drivers/gpu/drm/drm_probe_helper.c | 19 +------------------
>  4 files changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 90bc65d07a35..f983ef60299c 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -101,7 +101,7 @@ config DRM_FBDEV_EMULATION
>  
>  config DRM_LOAD_EDID_FIRMWARE
>  	bool "Allow to specify an EDID data set instead of probing for it"
> -	depends on DRM_KMS_HELPER
> +	depends on DRM
>  	help
>  	  Say Y here, if you want to use EDID data to be loaded from the
>  	  /lib/firmware directory or one of the provided built-in
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 92de3991fa56..a10ac095608f 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -27,13 +27,13 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
>  drm-$(CONFIG_OF) += drm_of.o
>  drm-$(CONFIG_AGP) += drm_agpsupport.o
>  drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
> +drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  
>  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>  		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
>  		drm_simple_kms_helper.o drm_modeset_helper.o
>  
> -drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e1743ab276dc..4007998d5ce3 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1309,6 +1309,10 @@ static void connector_bad_edid(struct drm_connector *connector,
>   * level, drivers must make all reasonable efforts to expose it as an I2C
>   * adapter and use drm_get_edid() instead of abusing this function.
>   *
> + * The EDID may be overridden using debugfs override_edid or firmare EDID
> + * (drm_load_edid_firmware()), 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.
>   */
>  struct edid *drm_do_get_edid(struct drm_connector *connector,
> @@ -1318,6 +1322,17 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  {
>  	int i, j = 0, valid_extensions = 0;
>  	u8 *edid, *new;
> +	struct edid *override = NULL;
> +
> +	if (connector->override_edid)
> +		override = drm_edid_duplicate((const struct edid *)
> +					      connector->edid_blob_ptr->data);
> +
> +	if (!override)
> +		override = drm_load_edid_firmware(connector);
> +
> +	if (!IS_ERR_OR_NULL(override))
> +		return override;
>  
>  	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
>  		return NULL;
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 358957118ca9..871326cbc465 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -199,8 +199,6 @@ drm_connector_detect(struct drm_connector *connector, bool force)
>   *    drm_mode_probed_add(). New modes start their life with status as OK.
>   *    Modes are added from a single source using the following priority order.
>   *
> - *    - debugfs 'override_edid' (used for testing only)
> - *    - firmware EDID (drm_load_edid_firmware())
>   *    - &drm_connector_helper_funcs.get_modes vfunc
>   *    - if the connector status is connector_status_connected, standard
>   *      VESA DMT modes up to 1024x768 are automatically added
> @@ -305,22 +303,7 @@ 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);
> -	}
> +	count = (*connector_funcs->get_modes)(connector);

What happens for drivers which cache the edid in their probe function, and
in their get_modes function only parse the already retrived edid? This
becomes fun in combination with the output forcing, where the connector
status forcing prevents ->detect from being run ...

I still think we can't handle override edid without also taking connector
forcing into account.
-Daniel

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

-- 
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: [PATCH v2 4/4] drm: handle override edid and firmware EDID at drm_do_get_edid() level

[Ville Syrjälä]

On Sun, Feb 26, 2017 at 10:22:42PM +0100, Daniel Vetter wrote:
> On Fri, Feb 17, 2017 at 05:20:54PM +0200, Jani Nikula wrote:
> > Handle debugfs override edid and firmware edid at the low level to
> > transparently and completely replace the real edid. Previously, we
> > practically only used the modes from the override EDID, and none of the
> > other data. This also prevents actual EDID reads when the EDID is to be
> > overridden, but retains the DDC probe.
> > 
> > Move firmware EDID loading from helper to core, as the functionality
> > moves to lower level as well. This will result in a change of module
> > parameter from drm_kms_helper.edid_firmware to drm.edid_firmware, which
> > arguably makes more sense anyway.
> > 
> > FIXME: validate override edid, deduplicate firmware edid validation.
> > 
> > v2: move firmware loading to core
> > 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/Kconfig            |  2 +-
> >  drivers/gpu/drm/Makefile           |  2 +-
> >  drivers/gpu/drm/drm_edid.c         | 15 +++++++++++++++
> >  drivers/gpu/drm/drm_probe_helper.c | 19 +------------------
> >  4 files changed, 18 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 90bc65d07a35..f983ef60299c 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -101,7 +101,7 @@ config DRM_FBDEV_EMULATION
> >  
> >  config DRM_LOAD_EDID_FIRMWARE
> >  	bool "Allow to specify an EDID data set instead of probing for it"
> > -	depends on DRM_KMS_HELPER
> > +	depends on DRM
> >  	help
> >  	  Say Y here, if you want to use EDID data to be loaded from the
> >  	  /lib/firmware directory or one of the provided built-in
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 92de3991fa56..a10ac095608f 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -27,13 +27,13 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
> >  drm-$(CONFIG_OF) += drm_of.o
> >  drm-$(CONFIG_AGP) += drm_agpsupport.o
> >  drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
> > +drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >  
> >  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> >  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> >  		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> >  		drm_simple_kms_helper.o drm_modeset_helper.o
> >  
> > -drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> >  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> >  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index e1743ab276dc..4007998d5ce3 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -1309,6 +1309,10 @@ static void connector_bad_edid(struct drm_connector *connector,
> >   * level, drivers must make all reasonable efforts to expose it as an I2C
> >   * adapter and use drm_get_edid() instead of abusing this function.
> >   *
> > + * The EDID may be overridden using debugfs override_edid or firmare EDID
> > + * (drm_load_edid_firmware()), 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.
> >   */
> >  struct edid *drm_do_get_edid(struct drm_connector *connector,
> > @@ -1318,6 +1322,17 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
> >  {
> >  	int i, j = 0, valid_extensions = 0;
> >  	u8 *edid, *new;
> > +	struct edid *override = NULL;
> > +
> > +	if (connector->override_edid)
> > +		override = drm_edid_duplicate((const struct edid *)
> > +					      connector->edid_blob_ptr->data);
> > +
> > +	if (!override)
> > +		override = drm_load_edid_firmware(connector);
> > +
> > +	if (!IS_ERR_OR_NULL(override))
> > +		return override;
> >  
> >  	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
> >  		return NULL;
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index 358957118ca9..871326cbc465 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -199,8 +199,6 @@ drm_connector_detect(struct drm_connector *connector, bool force)
> >   *    drm_mode_probed_add(). New modes start their life with status as OK.
> >   *    Modes are added from a single source using the following priority order.
> >   *
> > - *    - debugfs 'override_edid' (used for testing only)
> > - *    - firmware EDID (drm_load_edid_firmware())
> >   *    - &drm_connector_helper_funcs.get_modes vfunc
> >   *    - if the connector status is connector_status_connected, standard
> >   *      VESA DMT modes up to 1024x768 are automatically added
> > @@ -305,22 +303,7 @@ 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);
> > -	}
> > +	count = (*connector_funcs->get_modes)(connector);
> 
> What happens for drivers which cache the edid in their probe function, and
> in their get_modes function only parse the already retrived edid?

Hmm. Good question. I guess currently they'll just keep using that
initial EDID (which may or may not come from the firmware EDID loader).
but overriding at runtime likely won't work.

For i915 that only happens for eDP/LVDS/DSI and such. And for those we
also parse the panel fixed mode from the EDID at probe time. So there
isn't much you could gain from runtime override unless we change the
entire implementation to not hang on to the fixed mode either.

> This
> becomes fun in combination with the output forcing, where the connector
> status forcing prevents ->detect from being run ...

We do have the ->force() hook for those cases.

> I still think we can't handle override edid without also taking connector
> forcing into account.

After these patches the two are somewhat linked. The force status
will affect whether drm_get_edid() will even return anything (doesn't
matter if there's an override EDID or not), but the presence of the
override EDID won't force the connector status. To me that seems
like a perfectly sensible approach.

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm: handle override edid and firmware EDID at drm_do_get_edid() level

[Daniel Vetter]

On Mon, Feb 27, 2017 at 05:09:44PM +0200, Ville Syrjälä wrote:
> On Sun, Feb 26, 2017 at 10:22:42PM +0100, Daniel Vetter wrote:
> > On Fri, Feb 17, 2017 at 05:20:54PM +0200, Jani Nikula wrote:
> > > Handle debugfs override edid and firmware edid at the low level to
> > > transparently and completely replace the real edid. Previously, we
> > > practically only used the modes from the override EDID, and none of the
> > > other data. This also prevents actual EDID reads when the EDID is to be
> > > overridden, but retains the DDC probe.
> > > 
> > > Move firmware EDID loading from helper to core, as the functionality
> > > moves to lower level as well. This will result in a change of module
> > > parameter from drm_kms_helper.edid_firmware to drm.edid_firmware, which
> > > arguably makes more sense anyway.
> > > 
> > > FIXME: validate override edid, deduplicate firmware edid validation.
> > > 
> > > v2: move firmware loading to core
> > > 
> > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > ---
> > >  drivers/gpu/drm/Kconfig            |  2 +-
> > >  drivers/gpu/drm/Makefile           |  2 +-
> > >  drivers/gpu/drm/drm_edid.c         | 15 +++++++++++++++
> > >  drivers/gpu/drm/drm_probe_helper.c | 19 +------------------
> > >  4 files changed, 18 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > index 90bc65d07a35..f983ef60299c 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -101,7 +101,7 @@ config DRM_FBDEV_EMULATION
> > >  
> > >  config DRM_LOAD_EDID_FIRMWARE
> > >  	bool "Allow to specify an EDID data set instead of probing for it"
> > > -	depends on DRM_KMS_HELPER
> > > +	depends on DRM
> > >  	help
> > >  	  Say Y here, if you want to use EDID data to be loaded from the
> > >  	  /lib/firmware directory or one of the provided built-in
> > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > index 92de3991fa56..a10ac095608f 100644
> > > --- a/drivers/gpu/drm/Makefile
> > > +++ b/drivers/gpu/drm/Makefile
> > > @@ -27,13 +27,13 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
> > >  drm-$(CONFIG_OF) += drm_of.o
> > >  drm-$(CONFIG_AGP) += drm_agpsupport.o
> > >  drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
> > > +drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > >  
> > >  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> > >  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> > >  		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> > >  		drm_simple_kms_helper.o drm_modeset_helper.o
> > >  
> > > -drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > >  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> > >  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> > >  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index e1743ab276dc..4007998d5ce3 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -1309,6 +1309,10 @@ static void connector_bad_edid(struct drm_connector *connector,
> > >   * level, drivers must make all reasonable efforts to expose it as an I2C
> > >   * adapter and use drm_get_edid() instead of abusing this function.
> > >   *
> > > + * The EDID may be overridden using debugfs override_edid or firmare EDID
> > > + * (drm_load_edid_firmware()), 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.
> > >   */
> > >  struct edid *drm_do_get_edid(struct drm_connector *connector,
> > > @@ -1318,6 +1322,17 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
> > >  {
> > >  	int i, j = 0, valid_extensions = 0;
> > >  	u8 *edid, *new;
> > > +	struct edid *override = NULL;
> > > +
> > > +	if (connector->override_edid)
> > > +		override = drm_edid_duplicate((const struct edid *)
> > > +					      connector->edid_blob_ptr->data);
> > > +
> > > +	if (!override)
> > > +		override = drm_load_edid_firmware(connector);
> > > +
> > > +	if (!IS_ERR_OR_NULL(override))
> > > +		return override;
> > >  
> > >  	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
> > >  		return NULL;
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > index 358957118ca9..871326cbc465 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -199,8 +199,6 @@ drm_connector_detect(struct drm_connector *connector, bool force)
> > >   *    drm_mode_probed_add(). New modes start their life with status as OK.
> > >   *    Modes are added from a single source using the following priority order.
> > >   *
> > > - *    - debugfs 'override_edid' (used for testing only)
> > > - *    - firmware EDID (drm_load_edid_firmware())
> > >   *    - &drm_connector_helper_funcs.get_modes vfunc
> > >   *    - if the connector status is connector_status_connected, standard
> > >   *      VESA DMT modes up to 1024x768 are automatically added
> > > @@ -305,22 +303,7 @@ 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);
> > > -	}
> > > +	count = (*connector_funcs->get_modes)(connector);
> > 
> > What happens for drivers which cache the edid in their probe function, and
> > in their get_modes function only parse the already retrived edid?
> 
> Hmm. Good question. I guess currently they'll just keep using that
> initial EDID (which may or may not come from the firmware EDID loader).
> but overriding at runtime likely won't work.
> 
> For i915 that only happens for eDP/LVDS/DSI and such. And for those we
> also parse the panel fixed mode from the EDID at probe time. So there
> isn't much you could gain from runtime override unless we change the
> entire implementation to not hang on to the fixed mode either.
> 
> > This
> > becomes fun in combination with the output forcing, where the connector
> > status forcing prevents ->detect from being run ...
> 
> We do have the ->force() hook for those cases.

Yes, that's the problem I'm seeing. Atm our igts both force the connector
state and override the edid, which means they'd be defunct. At least I
think they'd fail ...

> > I still think we can't handle override edid without also taking connector
> > forcing into account.
> 
> After these patches the two are somewhat linked. The force status
> will affect whether drm_get_edid() will even return anything (doesn't
> matter if there's an override EDID or not), but the presence of the
> override EDID won't force the connector status. To me that seems
> like a perfectly sensible approach.

How does the EDID override affect the status? If there's an issue with hpd
on e.g. DP, it still won't. I'm not even sure this would affect all the
places where we realy on ddc detection.
-Daniel
-- 
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: [PATCH v2 4/4] drm: handle override edid and firmware EDID at drm_do_get_edid() level

[Ville Syrjälä]

On Mon, Feb 27, 2017 at 05:19:21PM +0100, Daniel Vetter wrote:
> On Mon, Feb 27, 2017 at 05:09:44PM +0200, Ville Syrjälä wrote:
> > On Sun, Feb 26, 2017 at 10:22:42PM +0100, Daniel Vetter wrote:
> > > On Fri, Feb 17, 2017 at 05:20:54PM +0200, Jani Nikula wrote:
> > > > Handle debugfs override edid and firmware edid at the low level to
> > > > transparently and completely replace the real edid. Previously, we
> > > > practically only used the modes from the override EDID, and none of the
> > > > other data. This also prevents actual EDID reads when the EDID is to be
> > > > overridden, but retains the DDC probe.
> > > > 
> > > > Move firmware EDID loading from helper to core, as the functionality
> > > > moves to lower level as well. This will result in a change of module
> > > > parameter from drm_kms_helper.edid_firmware to drm.edid_firmware, which
> > > > arguably makes more sense anyway.
> > > > 
> > > > FIXME: validate override edid, deduplicate firmware edid validation.
> > > > 
> > > > v2: move firmware loading to core
> > > > 
> > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/Kconfig            |  2 +-
> > > >  drivers/gpu/drm/Makefile           |  2 +-
> > > >  drivers/gpu/drm/drm_edid.c         | 15 +++++++++++++++
> > > >  drivers/gpu/drm/drm_probe_helper.c | 19 +------------------
> > > >  4 files changed, 18 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > > index 90bc65d07a35..f983ef60299c 100644
> > > > --- a/drivers/gpu/drm/Kconfig
> > > > +++ b/drivers/gpu/drm/Kconfig
> > > > @@ -101,7 +101,7 @@ config DRM_FBDEV_EMULATION
> > > >  
> > > >  config DRM_LOAD_EDID_FIRMWARE
> > > >  	bool "Allow to specify an EDID data set instead of probing for it"
> > > > -	depends on DRM_KMS_HELPER
> > > > +	depends on DRM
> > > >  	help
> > > >  	  Say Y here, if you want to use EDID data to be loaded from the
> > > >  	  /lib/firmware directory or one of the provided built-in
> > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > > index 92de3991fa56..a10ac095608f 100644
> > > > --- a/drivers/gpu/drm/Makefile
> > > > +++ b/drivers/gpu/drm/Makefile
> > > > @@ -27,13 +27,13 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
> > > >  drm-$(CONFIG_OF) += drm_of.o
> > > >  drm-$(CONFIG_AGP) += drm_agpsupport.o
> > > >  drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
> > > > +drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > > >  
> > > >  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> > > >  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> > > >  		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> > > >  		drm_simple_kms_helper.o drm_modeset_helper.o
> > > >  
> > > > -drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > > >  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> > > >  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> > > >  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > index e1743ab276dc..4007998d5ce3 100644
> > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > @@ -1309,6 +1309,10 @@ static void connector_bad_edid(struct drm_connector *connector,
> > > >   * level, drivers must make all reasonable efforts to expose it as an I2C
> > > >   * adapter and use drm_get_edid() instead of abusing this function.
> > > >   *
> > > > + * The EDID may be overridden using debugfs override_edid or firmare EDID
> > > > + * (drm_load_edid_firmware()), 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.
> > > >   */
> > > >  struct edid *drm_do_get_edid(struct drm_connector *connector,
> > > > @@ -1318,6 +1322,17 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
> > > >  {
> > > >  	int i, j = 0, valid_extensions = 0;
> > > >  	u8 *edid, *new;
> > > > +	struct edid *override = NULL;
> > > > +
> > > > +	if (connector->override_edid)
> > > > +		override = drm_edid_duplicate((const struct edid *)
> > > > +					      connector->edid_blob_ptr->data);
> > > > +
> > > > +	if (!override)
> > > > +		override = drm_load_edid_firmware(connector);
> > > > +
> > > > +	if (!IS_ERR_OR_NULL(override))
> > > > +		return override;
> > > >  
> > > >  	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
> > > >  		return NULL;
> > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > > index 358957118ca9..871326cbc465 100644
> > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > @@ -199,8 +199,6 @@ drm_connector_detect(struct drm_connector *connector, bool force)
> > > >   *    drm_mode_probed_add(). New modes start their life with status as OK.
> > > >   *    Modes are added from a single source using the following priority order.
> > > >   *
> > > > - *    - debugfs 'override_edid' (used for testing only)
> > > > - *    - firmware EDID (drm_load_edid_firmware())
> > > >   *    - &drm_connector_helper_funcs.get_modes vfunc
> > > >   *    - if the connector status is connector_status_connected, standard
> > > >   *      VESA DMT modes up to 1024x768 are automatically added
> > > > @@ -305,22 +303,7 @@ 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);
> > > > -	}
> > > > +	count = (*connector_funcs->get_modes)(connector);
> > > 
> > > What happens for drivers which cache the edid in their probe function, and
> > > in their get_modes function only parse the already retrived edid?
> > 
> > Hmm. Good question. I guess currently they'll just keep using that
> > initial EDID (which may or may not come from the firmware EDID loader).
> > but overriding at runtime likely won't work.
> > 
> > For i915 that only happens for eDP/LVDS/DSI and such. And for those we
> > also parse the panel fixed mode from the EDID at probe time. So there
> > isn't much you could gain from runtime override unless we change the
> > entire implementation to not hang on to the fixed mode either.
> > 
> > > This
> > > becomes fun in combination with the output forcing, where the connector
> > > status forcing prevents ->detect from being run ...
> > 
> > We do have the ->force() hook for those cases.
> 
> Yes, that's the problem I'm seeing. Atm our igts both force the connector
> state and override the edid, which means they'd be defunct. At least I
> think they'd fail ...

They should work just fine.

> 
> > > I still think we can't handle override edid without also taking connector
> > > forcing into account.
> > 
> > After these patches the two are somewhat linked. The force status
> > will affect whether drm_get_edid() will even return anything (doesn't
> > matter if there's an override EDID or not), but the presence of the
> > override EDID won't force the connector status. To me that seems
> > like a perfectly sensible approach.
> 
> How does the EDID override affect the status?

It doesn't.

> If there's an issue with hpd
> on e.g. DP, it still won't. I'm not even sure this would affect all the
> places where we realy on ddc detection.

I think the lack of DPCD read in ->force() is the only real problem
I see in the DP code.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx