[PATCH] drm/edid: Allow comma separated edid binaries. (v2)

[PATCH] drm/edid: Allow comma separated edid binaries. (v2)

[Bob Paauwe]

Allow comma separated filenames in the edid_firmware parameter.

For example:

edid_firmware=eDP-1:edid/1280x480.bin,DP-2:edid/1920x1080.bin

v2: Use strsep() to simplify parsing of comma seperated string. (Matt)
    Move initial bail before strdup. (Matt)

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
---
 drivers/gpu/drm/drm_edid_load.c | 44 ++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index c5605fe..93b9275 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -264,27 +264,53 @@ out:
 int drm_load_edid_firmware(struct drm_connector *connector)
 {
 	const char *connector_name = connector->name;
-	char *edidname = edid_firmware, *last, *colon;
+	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
 	int ret;
 	struct edid *edid;
 
-	if (*edidname == '\0')
+	if (edid_firmware[0] == '\0')
 		return 0;
 
-	colon = strchr(edidname, ':');
-	if (colon != NULL) {
-		if (strncmp(connector_name, edidname, colon - edidname))
-			return 0;
-		edidname = colon + 1;
-		if (*edidname == '\0')
-			return 0;
+	/*
+	 * If there are multiple edid files specified and separated
+	 * by commas, search through the list looking for one that
+	 * matches the connector.
+	 *
+	 * If there's one or more that don't't specify a connector, keep
+	 * the last one found one as a fallback.
+	 */
+	fwstr = kstrdup(edid_firmware, GFP_KERNEL);
+	edidstr = fwstr;
+
+	while ((edidname = strsep(&edidstr, ","))) {
+		colon = strchr(edidname, ':');
+		if (colon != NULL) {
+			if (strncmp(connector_name, edidname, colon-edidname))
+				continue;
+			edidname = colon + 1;
+			break;
+		} else {
+			if (*edidname != '\0') /* corner case: multiple ',' */
+				fallback = edidname;
+		}
+
 	}
 
+	if (fallback == NULL && edidname == NULL) {
+		kfree(fwstr);
+		return 0;
+	}
+
+	if (edidname == NULL && fallback)
+		edidname = fallback;
+
 	last = edidname + strlen(edidname) - 1;
 	if (*last == '\n')
 		*last = '\0';
 
 	edid = edid_load(connector, edidname, connector_name);
+	kfree(fwstr);
+
 	if (IS_ERR_OR_NULL(edid))
 		return 0;
 
-- 
2.1.0

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

Re: [PATCH] drm/edid: Allow comma separated edid binaries. (v2)

[Jani Nikula]

On Wed, 26 Aug 2015, Bob Paauwe <bob.j.paauwe@intel.com> wrote:
> Allow comma separated filenames in the edid_firmware parameter.
>
> For example:
>
> edid_firmware=eDP-1:edid/1280x480.bin,DP-2:edid/1920x1080.bin
>
> v2: Use strsep() to simplify parsing of comma seperated string. (Matt)
>     Move initial bail before strdup. (Matt)
>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> ---
>  drivers/gpu/drm/drm_edid_load.c | 44 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index c5605fe..93b9275 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -264,27 +264,53 @@ out:
>  int drm_load_edid_firmware(struct drm_connector *connector)
>  {
>  	const char *connector_name = connector->name;
> -	char *edidname = edid_firmware, *last, *colon;
> +	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
>  	int ret;
>  	struct edid *edid;
>  
> -	if (*edidname == '\0')
> +	if (edid_firmware[0] == '\0')
>  		return 0;
>  
> -	colon = strchr(edidname, ':');
> -	if (colon != NULL) {
> -		if (strncmp(connector_name, edidname, colon - edidname))
> -			return 0;
> -		edidname = colon + 1;
> -		if (*edidname == '\0')
> -			return 0;
> +	/*
> +	 * If there are multiple edid files specified and separated
> +	 * by commas, search through the list looking for one that
> +	 * matches the connector.
> +	 *
> +	 * If there's one or more that don't't specify a connector, keep
> +	 * the last one found one as a fallback.
> +	 */
> +	fwstr = kstrdup(edid_firmware, GFP_KERNEL);
> +	edidstr = fwstr;
> +
> +	while ((edidname = strsep(&edidstr, ","))) {
> +		colon = strchr(edidname, ':');
> +		if (colon != NULL) {
> +			if (strncmp(connector_name, edidname, colon-edidname))
> +				continue;
> +			edidname = colon + 1;
> +			break;
> +		} else {
> +			if (*edidname != '\0') /* corner case: multiple ',' */
> +				fallback = edidname;
> +		}
> +
>  	}
>  
> +	if (fallback == NULL && edidname == NULL) {
> +		kfree(fwstr);
> +		return 0;
> +	}
> +
> +	if (edidname == NULL && fallback)
> +		edidname = fallback;
> +

I guess I might write that as either:

	if (!edidname)
        	edidname = fallback;

	if (!edidname) {
		kfree(fwstr);
                return 0;
	}

or:

	if (!edidname) {
        	if (!fallback) {
                	kfree(fwstr);
                        return 0;
                }
                edidname = fallback;
	}

to make it faster to read, but meh. Up to you.

This will need an update to Documentation/kernel-parameters.txt
though. With that updated, this is

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  	last = edidname + strlen(edidname) - 1;
>  	if (*last == '\n')
>  		*last = '\0';
>  
>  	edid = edid_load(connector, edidname, connector_name);
> +	kfree(fwstr);
> +
>  	if (IS_ERR_OR_NULL(edid))
>  		return 0;
>  
> -- 
> 2.1.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/edid: Allow comma separated edid binaries. (v3)

[Bob Paauwe]

Allow comma separated filenames in the edid_firmware parameter.

For example:

edid_firmware=eDP-1:edid/1280x480.bin,DP-2:edid/1920x1080.bin

v2: Use strsep() to simplify parsing of comma seperated string. (Matt)
    Move initial bail before strdup. (Matt)
v3: Changed conditionals after while loop to make more readable (Jani)
    Updated kernel-parameters.txt to reflect changes (Jani)

Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
---
 Documentation/kernel-parameters.txt | 15 +++++++------
 drivers/gpu/drm/drm_edid_load.c     | 42 ++++++++++++++++++++++++++++++-------
 2 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index cd03a0f..a0cab10 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -915,11 +915,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			The filter can be disabled or changed to another
 			driver later using sysfs.
 
-	drm_kms_helper.edid_firmware=[<connector>:]<file>
-			Broken monitors, graphic adapters and KVMs may
-			send no or incorrect EDID data sets. This parameter
-			allows to specify an EDID data set in the
-			/lib/firmware directory that is used instead.
+	drm_kms_helper.edid_firmware=[<connector>:]<file>[,[<connector>:]<file>]
+			Broken monitors, graphic adapters, KVMs and EDIDless
+			panels may send no or incorrect EDID data sets.
+			This parameter allows to specify an EDID data sets
+			in the /lib/firmware directory that are used instead.
 			Generic built-in EDID data sets are used, if one of
 			edid/1024x768.bin, edid/1280x1024.bin,
 			edid/1680x1050.bin, or edid/1920x1080.bin is given
@@ -928,7 +928,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			available in Documentation/EDID/HOWTO.txt. An EDID
 			data set will only be used for a particular connector,
 			if its name and a colon are prepended to the EDID
-			name.
+			name. Each connector may use a unique EDID data
+			set by separating the files with a comma.  An EDID
+			data set with no connector name will be used for
+			any connectors not explicitly specified.
 
 	dscc4.setup=	[NET]
 
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index c5605fe..a203e154 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -264,20 +264,44 @@ out:
 int drm_load_edid_firmware(struct drm_connector *connector)
 {
 	const char *connector_name = connector->name;
-	char *edidname = edid_firmware, *last, *colon;
+	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
 	int ret;
 	struct edid *edid;
 
-	if (*edidname == '\0')
+	if (edid_firmware[0] == '\0')
 		return 0;
 
-	colon = strchr(edidname, ':');
-	if (colon != NULL) {
-		if (strncmp(connector_name, edidname, colon - edidname))
-			return 0;
-		edidname = colon + 1;
-		if (*edidname == '\0')
+	/*
+	 * If there are multiple edid files specified and separated
+	 * by commas, search through the list looking for one that
+	 * matches the connector.
+	 *
+	 * If there's one or more that don't't specify a connector, keep
+	 * the last one found one as a fallback.
+	 */
+	fwstr = kstrdup(edid_firmware, GFP_KERNEL);
+	edidstr = fwstr;
+
+	while ((edidname = strsep(&edidstr, ","))) {
+		colon = strchr(edidname, ':');
+		if (colon != NULL) {
+			if (strncmp(connector_name, edidname, colon-edidname))
+				continue;
+			edidname = colon + 1;
+			break;
+		} else {
+			if (*edidname != '\0') /* corner case: multiple ',' */
+				fallback = edidname;
+		}
+
+	}
+
+	if (!edidname) {
+		if (!fallback) {
+			kfree(fwstr);
 			return 0;
+		}
+		edidname = fallback;
 	}
 
 	last = edidname + strlen(edidname) - 1;
@@ -285,6 +309,8 @@ int drm_load_edid_firmware(struct drm_connector *connector)
 		*last = '\0';
 
 	edid = edid_load(connector, edidname, connector_name);
+	kfree(fwstr);
+
 	if (IS_ERR_OR_NULL(edid))
 		return 0;
 
-- 
2.1.0

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

Re: [PATCH] drm/edid: Allow comma separated edid binaries. (v3)

[Jani Nikula]

On Thu, 27 Aug 2015, Bob Paauwe <bob.j.paauwe@intel.com> wrote:
> Allow comma separated filenames in the edid_firmware parameter.
>
> For example:
>
> edid_firmware=eDP-1:edid/1280x480.bin,DP-2:edid/1920x1080.bin
>
> v2: Use strsep() to simplify parsing of comma seperated string. (Matt)
>     Move initial bail before strdup. (Matt)
> v3: Changed conditionals after while loop to make more readable (Jani)
>     Updated kernel-parameters.txt to reflect changes (Jani)
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Yup.

> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> ---
>  Documentation/kernel-parameters.txt | 15 +++++++------
>  drivers/gpu/drm/drm_edid_load.c     | 42 ++++++++++++++++++++++++++++++-------
>  2 files changed, 43 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index cd03a0f..a0cab10 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -915,11 +915,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			The filter can be disabled or changed to another
>  			driver later using sysfs.
>  
> -	drm_kms_helper.edid_firmware=[<connector>:]<file>
> -			Broken monitors, graphic adapters and KVMs may
> -			send no or incorrect EDID data sets. This parameter
> -			allows to specify an EDID data set in the
> -			/lib/firmware directory that is used instead.
> +	drm_kms_helper.edid_firmware=[<connector>:]<file>[,[<connector>:]<file>]
> +			Broken monitors, graphic adapters, KVMs and EDIDless
> +			panels may send no or incorrect EDID data sets.
> +			This parameter allows to specify an EDID data sets
> +			in the /lib/firmware directory that are used instead.
>  			Generic built-in EDID data sets are used, if one of
>  			edid/1024x768.bin, edid/1280x1024.bin,
>  			edid/1680x1050.bin, or edid/1920x1080.bin is given
> @@ -928,7 +928,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			available in Documentation/EDID/HOWTO.txt. An EDID
>  			data set will only be used for a particular connector,
>  			if its name and a colon are prepended to the EDID
> -			name.
> +			name. Each connector may use a unique EDID data
> +			set by separating the files with a comma.  An EDID
> +			data set with no connector name will be used for
> +			any connectors not explicitly specified.
>  
>  	dscc4.setup=	[NET]
>  
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index c5605fe..a203e154 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -264,20 +264,44 @@ out:
>  int drm_load_edid_firmware(struct drm_connector *connector)
>  {
>  	const char *connector_name = connector->name;
> -	char *edidname = edid_firmware, *last, *colon;
> +	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
>  	int ret;
>  	struct edid *edid;
>  
> -	if (*edidname == '\0')
> +	if (edid_firmware[0] == '\0')
>  		return 0;
>  
> -	colon = strchr(edidname, ':');
> -	if (colon != NULL) {
> -		if (strncmp(connector_name, edidname, colon - edidname))
> -			return 0;
> -		edidname = colon + 1;
> -		if (*edidname == '\0')
> +	/*
> +	 * If there are multiple edid files specified and separated
> +	 * by commas, search through the list looking for one that
> +	 * matches the connector.
> +	 *
> +	 * If there's one or more that don't't specify a connector, keep
> +	 * the last one found one as a fallback.
> +	 */
> +	fwstr = kstrdup(edid_firmware, GFP_KERNEL);
> +	edidstr = fwstr;
> +
> +	while ((edidname = strsep(&edidstr, ","))) {
> +		colon = strchr(edidname, ':');
> +		if (colon != NULL) {
> +			if (strncmp(connector_name, edidname, colon-edidname))
> +				continue;
> +			edidname = colon + 1;
> +			break;
> +		} else {
> +			if (*edidname != '\0') /* corner case: multiple ',' */
> +				fallback = edidname;
> +		}
> +
> +	}
> +
> +	if (!edidname) {
> +		if (!fallback) {
> +			kfree(fwstr);
>  			return 0;
> +		}
> +		edidname = fallback;
>  	}
>  
>  	last = edidname + strlen(edidname) - 1;
> @@ -285,6 +309,8 @@ int drm_load_edid_firmware(struct drm_connector *connector)
>  		*last = '\0';
>  
>  	edid = edid_load(connector, edidname, connector_name);
> +	kfree(fwstr);
> +
>  	if (IS_ERR_OR_NULL(edid))
>  		return 0;
>  
> -- 
> 2.1.0
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/edid: Allow comma separated edid binaries. (v3)

[Daniel Vetter]

On Thu, Aug 27, 2015 at 10:04:13AM -0700, Bob Paauwe wrote:
> Allow comma separated filenames in the edid_firmware parameter.
> 
> For example:
> 
> edid_firmware=eDP-1:edid/1280x480.bin,DP-2:edid/1920x1080.bin
> 
> v2: Use strsep() to simplify parsing of comma seperated string. (Matt)
>     Move initial bail before strdup. (Matt)
> v3: Changed conditionals after while loop to make more readable (Jani)
>     Updated kernel-parameters.txt to reflect changes (Jani)
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> ---
>  Documentation/kernel-parameters.txt | 15 +++++++------
>  drivers/gpu/drm/drm_edid_load.c     | 42 ++++++++++++++++++++++++++++++-------
>  2 files changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index cd03a0f..a0cab10 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -915,11 +915,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			The filter can be disabled or changed to another
>  			driver later using sysfs.
>  
> -	drm_kms_helper.edid_firmware=[<connector>:]<file>
> -			Broken monitors, graphic adapters and KVMs may
> -			send no or incorrect EDID data sets. This parameter
> -			allows to specify an EDID data set in the
> -			/lib/firmware directory that is used instead.
> +	drm_kms_helper.edid_firmware=[<connector>:]<file>[,[<connector>:]<file>]
> +			Broken monitors, graphic adapters, KVMs and EDIDless
> +			panels may send no or incorrect EDID data sets.
> +			This parameter allows to specify an EDID data sets
> +			in the /lib/firmware directory that are used instead.
>  			Generic built-in EDID data sets are used, if one of
>  			edid/1024x768.bin, edid/1280x1024.bin,
>  			edid/1680x1050.bin, or edid/1920x1080.bin is given
> @@ -928,7 +928,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			available in Documentation/EDID/HOWTO.txt. An EDID
>  			data set will only be used for a particular connector,
>  			if its name and a colon are prepended to the EDID
> -			name.
> +			name. Each connector may use a unique EDID data
> +			set by separating the files with a comma.  An EDID
> +			data set with no connector name will be used for
> +			any connectors not explicitly specified.
>  
>  	dscc4.setup=	[NET]
>  
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index c5605fe..a203e154 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -264,20 +264,44 @@ out:
>  int drm_load_edid_firmware(struct drm_connector *connector)
>  {
>  	const char *connector_name = connector->name;
> -	char *edidname = edid_firmware, *last, *colon;
> +	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
>  	int ret;
>  	struct edid *edid;
>  
> -	if (*edidname == '\0')
> +	if (edid_firmware[0] == '\0')
>  		return 0;
>  
> -	colon = strchr(edidname, ':');
> -	if (colon != NULL) {
> -		if (strncmp(connector_name, edidname, colon - edidname))
> -			return 0;
> -		edidname = colon + 1;
> -		if (*edidname == '\0')
> +	/*
> +	 * If there are multiple edid files specified and separated
> +	 * by commas, search through the list looking for one that
> +	 * matches the connector.
> +	 *
> +	 * If there's one or more that don't't specify a connector, keep
> +	 * the last one found one as a fallback.
> +	 */
> +	fwstr = kstrdup(edid_firmware, GFP_KERNEL);
> +	edidstr = fwstr;
> +
> +	while ((edidname = strsep(&edidstr, ","))) {
> +		colon = strchr(edidname, ':');
> +		if (colon != NULL) {
> +			if (strncmp(connector_name, edidname, colon-edidname))
> +				continue;
> +			edidname = colon + 1;
> +			break;
> +		} else {
> +			if (*edidname != '\0') /* corner case: multiple ',' */
> +				fallback = edidname;
> +		}

I flattened the control flow here and done a few other checkpatch
appeasements. Merged to drm-misc, thanks.
-Daniel

> +
> +	}
> +
> +	if (!edidname) {
> +		if (!fallback) {
> +			kfree(fwstr);
>  			return 0;
> +		}
> +		edidname = fallback;
>  	}
>  
>  	last = edidname + strlen(edidname) - 1;
> @@ -285,6 +309,8 @@ int drm_load_edid_firmware(struct drm_connector *connector)
>  		*last = '\0';
>  
>  	edid = edid_load(connector, edidname, connector_name);
> +	kfree(fwstr);
> +
>  	if (IS_ERR_OR_NULL(edid))
>  		return 0;
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/dri-devel