diff mbox series

[12/15] drm/edid: use struct drm_edid for override/firmware EDID

Message ID de3f22db32feed871901408c52d0dc3a513bf686.1665496046.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/edid: EDID override refactoring and fixes | expand

Commit Message

Jani Nikula Oct. 11, 2022, 1:49 p.m. UTC 
There's a lot going on here, but the main thing is switching the
firmware EDID loader to use struct drm_edid. Unfortunately, it's
difficult to reasonably split to smaller pieces.

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

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

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c      | 32 ++++++------
 drivers/gpu/drm/drm_edid_load.c | 86 +++++++--------------------------
 include/drm/drm_edid.h          |  4 +-
 3 files changed, 36 insertions(+), 86 deletions(-)

Comments

Ville Syrjälä Oct. 19, 2022, 8:01 p.m. UTC | #1 
On Tue, Oct 11, 2022 at 04:49:46PM +0300, Jani Nikula wrote:
> There's a lot going on here, but the main thing is switching the
> firmware EDID loader to use struct drm_edid. Unfortunately, it's
> difficult to reasonably split to smaller pieces.
> 
> Convert the EDID loader to struct drm_edid. There's a functional change
> in validation; it no longer tries to fix errors or filter invalid
> blocks. It's stricter in this sense. Hopefully this will not be an
> issue.
> 
> As a by-product, this change also allows HF-EEODB extended EDIDs to be
> passed via override/firmware EDID.

Was pondering about that reading the earlier patch. Figured I'd keep
on reading, and voila here it is.

> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c      | 32 ++++++------
>  drivers/gpu/drm/drm_edid_load.c | 86 +++++++--------------------------
>  include/drm/drm_edid.h          |  4 +-
>  3 files changed, 36 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 2c66a901474d..935bdf4e6269 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2202,21 +2202,16 @@ static void connector_bad_edid(struct drm_connector *connector,
>  }
>  
>  /* Get override or firmware EDID */
> -static struct edid *drm_get_override_edid(struct drm_connector *connector,
> -					  size_t *alloc_size)
> +static const struct drm_edid *drm_edid_override_get(struct drm_connector *connector)
>  {
> -	struct edid *override = NULL;
> +	const struct drm_edid *override = NULL;
>  
>  	if (connector->edid_override)
> -		override = drm_edid_duplicate(connector->edid_override->edid);
> +		override = drm_edid_dup(connector->edid_override);
>  
>  	if (!override)
>  		override = drm_edid_load_firmware(connector);
>  
> -	/* FIXME: Get alloc size from deeper down the stack */
> -	if (!IS_ERR_OR_NULL(override) && alloc_size)
> -		*alloc_size = edid_size(override);
> -
>  	return IS_ERR(override) ? NULL : override;
>  }
>  
> @@ -2277,14 +2272,14 @@ int drm_edid_override_reset(struct drm_connector *connector)
>   */
>  int drm_edid_override_connector_update(struct drm_connector *connector)
>  {
> -	struct edid *override;
> +	const struct drm_edid *override;
>  	int num_modes = 0;
>  
> -	override = drm_get_override_edid(connector, NULL);
> +	override = drm_edid_override_get(connector);
>  	if (override) {
> -		drm_connector_update_edid_property(connector, override);
> -		num_modes = drm_add_edid_modes(connector, override);
> -		kfree(override);
> +		num_modes = drm_edid_connector_update(connector, override);
> +
> +		drm_edid_free(override);
>  
>  		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
>  			      connector->base.id, connector->name, num_modes);
> @@ -2335,12 +2330,19 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>  {
>  	enum edid_block_status status;
>  	int i, num_blocks, invalid_blocks = 0;
> +	const struct drm_edid *override;
>  	struct edid *edid, *new;
>  	size_t alloc_size = EDID_LENGTH;
>  
> -	edid = drm_get_override_edid(connector, &alloc_size);
> -	if (edid)
> +	override = drm_edid_override_get(connector);
> +	if (override) {
> +		alloc_size = override->size;
> +		edid = kmemdup(override->edid, alloc_size, GFP_KERNEL);
> +		drm_edid_free(override);
> +		if (!edid)
> +			return NULL;
>  		goto ok;
> +	}
>  
>  	edid = kmalloc(alloc_size, GFP_KERNEL);
>  	if (!edid)
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index bc6b96abd1b3..248f0685c33e 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -159,22 +159,12 @@ static const u8 generic_edid[GENERIC_EDIDS][128] = {
>  	},
>  };
>  
> -static int edid_size(const u8 *edid, int data_size)
> -{
> -	if (data_size < EDID_LENGTH)
> -		return 0;
> -
> -	return (edid[0x7e] + 1) * EDID_LENGTH;
> -}
> -
> -static void *edid_load(struct drm_connector *connector, const char *name)
> +static const struct drm_edid *edid_load(struct drm_connector *connector, const char *name)
>  {
>  	const struct firmware *fw = NULL;
>  	const u8 *fwdata;
> -	u8 *edid;
> +	const struct drm_edid *drm_edid;
>  	int fwsize, builtin;
> -	int i, valid_extensions = 0;
> -	bool print_bad_edid = !connector->bad_edid_counter || drm_debug_enabled(DRM_UT_KMS);
>  
>  	builtin = match_string(generic_edid_name, GENERIC_EDIDS, name);
>  	if (builtin >= 0) {
> @@ -203,69 +193,26 @@ static void *edid_load(struct drm_connector *connector, const char *name)
>  		fwsize = fw->size;
>  	}
>  
> -	if (edid_size(fwdata, fwsize) != fwsize) {
> -		DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
> -			  "(expected %d, got %d\n", name,
> -			  edid_size(fwdata, fwsize), (int)fwsize);
> -		edid = ERR_PTR(-EINVAL);
> -		goto out;
> -	}
> -
> -	edid = kmemdup(fwdata, fwsize, GFP_KERNEL);
> -	if (edid == NULL) {
> -		edid = ERR_PTR(-ENOMEM);
> -		goto out;
> -	}
> -
> -	if (!drm_edid_block_valid(edid, 0, print_bad_edid,
> -				  &connector->edid_corrupt)) {
> -		connector->bad_edid_counter++;
> -		DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
> -		    name);
> -		kfree(edid);
> -		edid = ERR_PTR(-EINVAL);
> -		goto out;
> -	}
> -
> -	for (i = 1; i <= edid[0x7e]; i++) {
> -		if (i != valid_extensions + 1)
> -			memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
> -			    edid + i * EDID_LENGTH, EDID_LENGTH);
> -		if (drm_edid_block_valid(edid + i * EDID_LENGTH, i,
> -					 print_bad_edid,
> -					 NULL))
> -			valid_extensions++;
> -	}
> -
> -	if (valid_extensions != edid[0x7e]) {
> -		u8 *new_edid;
> +	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Loaded %s firmware EDID \"%s\"\n",
> +		    connector->base.id, connector->name,
> +		    builtin >= 0 ? "built-in" : "external", name);
>  
> -		edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> -		DRM_INFO("Found %d valid extensions instead of %d in EDID data "
> -		    "\"%s\" for connector \"%s\"\n", valid_extensions,
> -		    edid[0x7e], name, connector->name);
> -		edid[0x7e] = valid_extensions;
> -
> -		new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
> -				    GFP_KERNEL);
> -		if (new_edid)
> -			edid = new_edid;
> +	drm_edid = drm_edid_alloc(fwdata, fwsize);
> +	if (!drm_edid_valid(drm_edid)) {
> +		drm_err(connector->dev, "Invalid firmware EDID \"%s\"\n", name);
> +		drm_edid_free(drm_edid);
> +		drm_edid = ERR_PTR(-EINVAL);
>  	}

Lovely diffstat ratio there.

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

>  
> -	DRM_INFO("Got %s EDID base block and %d extension%s from "
> -	    "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
> -	    "external", valid_extensions, valid_extensions == 1 ? "" : "s",
> -	    name, connector->name);
> -
> -out:
>  	release_firmware(fw);
> -	return edid;
> +
> +	return drm_edid;
>  }
>  
> -struct edid *drm_edid_load_firmware(struct drm_connector *connector)
> +const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector)
>  {
>  	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
> -	struct edid *edid;
> +	const struct drm_edid *drm_edid;
>  
>  	if (edid_firmware[0] == '\0')
>  		return ERR_PTR(-ENOENT);
> @@ -308,8 +255,9 @@ struct edid *drm_edid_load_firmware(struct drm_connector *connector)
>  	if (*last == '\n')
>  		*last = '\0';
>  
> -	edid = edid_load(connector, edidname);
> +	drm_edid = edid_load(connector, edidname);
> +
>  	kfree(fwstr);
>  
> -	return edid;
> +	return drm_edid;
>  }
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index dc7467d25cd8..8138613f4e4e 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -388,11 +388,11 @@ int drm_av_sync_delay(struct drm_connector *connector,
>  		      const struct drm_display_mode *mode);
>  
>  #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
> -struct edid *drm_edid_load_firmware(struct drm_connector *connector);
> +const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector);
>  int __drm_set_edid_firmware_path(const char *path);
>  int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
>  #else
> -static inline struct edid *
> +static inline const struct drm_edid *
>  drm_edid_load_firmware(struct drm_connector *connector)
>  {
>  	return ERR_PTR(-ENOENT);
> -- 
> 2.34.1
diff mbox series

Patch

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