* [PATCH] drm: Try loading builtin EDIDs first
@ 2013-10-04 9:21 Chris Wilson
2013-10-04 9:50 ` Jani Nikula
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2013-10-04 9:21 UTC (permalink / raw)
To: dri-devel; +Cc: Dave Airlie
If the firmware is builtin and userspace is not yet running, we can
stall the boot process for a minute whilst the firmware loader times
out probing for a non-existent connector EDID. This is contrary to the
expectations of providing a builtin EDID!
In the process, we can rearrange the code to make the error handling
more resilient and prevent gcc warning about unitialised variables along
the error paths.
v2: Load builtins first, fix gcc second (Jani) and cosmetics (Ville).
v3: Verify that we do not read beyond the end of the fwdata (Ville)
v4: Restore the lost parenthesis (Jani)
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Reviewed-by: Jani Nikula <jani.nikula@linux.intel.com>
---
drivers/gpu/drm/drm_edid_load.c | 108 ++++++++++++++++++++--------------------
1 file changed, 54 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 271b42bbfb72..75f204bbb888 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -32,7 +32,7 @@ MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
"from built-in data or /lib/firmware instead. ");
#define GENERIC_EDIDS 5
-static char *generic_edid_name[GENERIC_EDIDS] = {
+static const char *generic_edid_name[GENERIC_EDIDS] = {
"edid/1024x768.bin",
"edid/1280x1024.bin",
"edid/1600x1200.bin",
@@ -40,7 +40,7 @@ static char *generic_edid_name[GENERIC_EDIDS] = {
"edid/1920x1080.bin",
};
-static u8 generic_edid[GENERIC_EDIDS][128] = {
+static const u8 generic_edid[GENERIC_EDIDS][128] = {
{
0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00,
0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
@@ -133,63 +133,68 @@ static 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 u8 *edid_load(struct drm_connector *connector, const char *name,
const char *connector_name)
{
- const struct firmware *fw;
- struct platform_device *pdev;
- u8 *fwdata = NULL, *edid, *new_edid;
- int fwsize, expected;
- int builtin = 0, err = 0;
+ const struct firmware *fw = NULL;
+ const u8 *fwdata;
+ u8 *edid;
+ int fwsize, builtin;
int i, valid_extensions = 0;
bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
- pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
- if (IS_ERR(pdev)) {
- DRM_ERROR("Failed to register EDID firmware platform device "
- "for connector \"%s\"\n", connector_name);
- err = -EINVAL;
- goto out;
- }
-
- err = request_firmware(&fw, name, &pdev->dev);
- platform_device_unregister(pdev);
-
- if (err) {
- i = 0;
- while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i]))
- i++;
- if (i < GENERIC_EDIDS) {
- err = 0;
- builtin = 1;
+ builtin = 0;
+ for (i = 0; i < GENERIC_EDIDS; i++) {
+ if (strcmp(name, generic_edid_name[i]) == 0) {
fwdata = generic_edid[i];
fwsize = sizeof(generic_edid[i]);
+ builtin = 1;
+ break;
}
}
+ if (!builtin) {
+ struct platform_device *pdev;
+ int err;
- if (err) {
- DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
- name, err);
- goto out;
- }
+ pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
+ if (IS_ERR(pdev)) {
+ DRM_ERROR("Failed to register EDID firmware platform device "
+ "for connector \"%s\"\n", connector_name);
+ return ERR_CAST(pdev);
+ }
+
+ err = request_firmware(&fw, name, &pdev->dev);
+ platform_device_unregister(pdev);
+ if (err) {
+ DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
+ name, err);
+ return ERR_PTR(err);
+ }
- if (fwdata == NULL) {
- fwdata = (u8 *) fw->data;
+ fwdata = fw->data;
fwsize = fw->size;
}
- expected = (fwdata[0x7e] + 1) * EDID_LENGTH;
- if (expected != fwsize) {
+ if (edid_size(fwdata, fwsize) != fwsize) {
DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
- "(expected %d, got %d)\n", name, expected, (int) fwsize);
- err = -EINVAL;
- goto relfw_out;
+ "(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) {
- err = -ENOMEM;
- goto relfw_out;
+ edid = ERR_PTR(-ENOMEM);
+ goto out;
}
if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
@@ -197,8 +202,8 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
name);
kfree(edid);
- err = -EINVAL;
- goto relfw_out;
+ edid = ERR_PTR(-EINVAL);
+ goto out;
}
for (i = 1; i <= edid[0x7e]; i++) {
@@ -210,19 +215,18 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
}
if (valid_extensions != edid[0x7e]) {
+ u8 *new_edid;
+
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 == NULL) {
- err = -ENOMEM;
- kfree(edid);
- goto relfw_out;
- }
- edid = new_edid;
+ GFP_KERNEL);
+ if (new_edid)
+ edid = new_edid;
}
DRM_INFO("Got %s EDID base block and %d extension%s from "
@@ -230,13 +234,9 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
"external", valid_extensions, valid_extensions == 1 ? "" : "s",
name, connector_name);
-relfw_out:
- release_firmware(fw);
-
out:
- if (err)
- return ERR_PTR(err);
-
+ if (fw)
+ release_firmware(fw);
return edid;
}
--
1.8.4.rc3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] drm: Try loading builtin EDIDs first
2013-10-04 9:21 [PATCH] drm: Try loading builtin EDIDs first Chris Wilson
@ 2013-10-04 9:50 ` Jani Nikula
0 siblings, 0 replies; 8+ messages in thread
From: Jani Nikula @ 2013-10-04 9:50 UTC (permalink / raw)
To: Chris Wilson, dri-devel; +Cc: Dave Airlie
On Fri, 04 Oct 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> If the firmware is builtin and userspace is not yet running, we can
> stall the boot process for a minute whilst the firmware loader times
> out probing for a non-existent connector EDID. This is contrary to the
> expectations of providing a builtin EDID!
>
> In the process, we can rearrange the code to make the error handling
> more resilient and prevent gcc warning about unitialised variables along
> the error paths.
>
> v2: Load builtins first, fix gcc second (Jani) and cosmetics (Ville).
> v3: Verify that we do not read beyond the end of the fwdata (Ville)
> v4: Restore the lost parenthesis (Jani)
Thanks, looks good,
Jani.
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Reviewed-by: Jani Nikula <jani.nikula@linux.intel.com>
> ---
> drivers/gpu/drm/drm_edid_load.c | 108 ++++++++++++++++++++--------------------
> 1 file changed, 54 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 271b42bbfb72..75f204bbb888 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -32,7 +32,7 @@ MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
> "from built-in data or /lib/firmware instead. ");
>
> #define GENERIC_EDIDS 5
> -static char *generic_edid_name[GENERIC_EDIDS] = {
> +static const char *generic_edid_name[GENERIC_EDIDS] = {
> "edid/1024x768.bin",
> "edid/1280x1024.bin",
> "edid/1600x1200.bin",
> @@ -40,7 +40,7 @@ static char *generic_edid_name[GENERIC_EDIDS] = {
> "edid/1920x1080.bin",
> };
>
> -static u8 generic_edid[GENERIC_EDIDS][128] = {
> +static const u8 generic_edid[GENERIC_EDIDS][128] = {
> {
> 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00,
> 0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> @@ -133,63 +133,68 @@ static 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 u8 *edid_load(struct drm_connector *connector, const char *name,
> const char *connector_name)
> {
> - const struct firmware *fw;
> - struct platform_device *pdev;
> - u8 *fwdata = NULL, *edid, *new_edid;
> - int fwsize, expected;
> - int builtin = 0, err = 0;
> + const struct firmware *fw = NULL;
> + const u8 *fwdata;
> + u8 *edid;
> + int fwsize, builtin;
> int i, valid_extensions = 0;
> bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
>
> - pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> - if (IS_ERR(pdev)) {
> - DRM_ERROR("Failed to register EDID firmware platform device "
> - "for connector \"%s\"\n", connector_name);
> - err = -EINVAL;
> - goto out;
> - }
> -
> - err = request_firmware(&fw, name, &pdev->dev);
> - platform_device_unregister(pdev);
> -
> - if (err) {
> - i = 0;
> - while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i]))
> - i++;
> - if (i < GENERIC_EDIDS) {
> - err = 0;
> - builtin = 1;
> + builtin = 0;
> + for (i = 0; i < GENERIC_EDIDS; i++) {
> + if (strcmp(name, generic_edid_name[i]) == 0) {
> fwdata = generic_edid[i];
> fwsize = sizeof(generic_edid[i]);
> + builtin = 1;
> + break;
> }
> }
> + if (!builtin) {
> + struct platform_device *pdev;
> + int err;
>
> - if (err) {
> - DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
> - name, err);
> - goto out;
> - }
> + pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> + if (IS_ERR(pdev)) {
> + DRM_ERROR("Failed to register EDID firmware platform device "
> + "for connector \"%s\"\n", connector_name);
> + return ERR_CAST(pdev);
> + }
> +
> + err = request_firmware(&fw, name, &pdev->dev);
> + platform_device_unregister(pdev);
> + if (err) {
> + DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
> + name, err);
> + return ERR_PTR(err);
> + }
>
> - if (fwdata == NULL) {
> - fwdata = (u8 *) fw->data;
> + fwdata = fw->data;
> fwsize = fw->size;
> }
>
> - expected = (fwdata[0x7e] + 1) * EDID_LENGTH;
> - if (expected != fwsize) {
> + if (edid_size(fwdata, fwsize) != fwsize) {
> DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
> - "(expected %d, got %d)\n", name, expected, (int) fwsize);
> - err = -EINVAL;
> - goto relfw_out;
> + "(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) {
> - err = -ENOMEM;
> - goto relfw_out;
> + edid = ERR_PTR(-ENOMEM);
> + goto out;
> }
>
> if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
> @@ -197,8 +202,8 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
> DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
> name);
> kfree(edid);
> - err = -EINVAL;
> - goto relfw_out;
> + edid = ERR_PTR(-EINVAL);
> + goto out;
> }
>
> for (i = 1; i <= edid[0x7e]; i++) {
> @@ -210,19 +215,18 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
> }
>
> if (valid_extensions != edid[0x7e]) {
> + u8 *new_edid;
> +
> 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 == NULL) {
> - err = -ENOMEM;
> - kfree(edid);
> - goto relfw_out;
> - }
> - edid = new_edid;
> + GFP_KERNEL);
> + if (new_edid)
> + edid = new_edid;
> }
>
> DRM_INFO("Got %s EDID base block and %d extension%s from "
> @@ -230,13 +234,9 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
> "external", valid_extensions, valid_extensions == 1 ? "" : "s",
> name, connector_name);
>
> -relfw_out:
> - release_firmware(fw);
> -
> out:
> - if (err)
> - return ERR_PTR(err);
> -
> + if (fw)
> + release_firmware(fw);
> return edid;
> }
>
> --
> 1.8.4.rc3
>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: Prevent use of uninitialised values whilst loading edid firmware
@ 2013-10-02 7:52 Jani Nikula
2013-10-02 9:07 ` [PATCH] drm: Try loading builtin EDIDs first Chris Wilson
0 siblings, 1 reply; 8+ messages in thread
From: Jani Nikula @ 2013-10-02 7:52 UTC (permalink / raw)
To: Chris Wilson, Ville Syrjälä; +Cc: dri-devel
On Tue, 01 Oct 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Oct 01, 2013 at 06:49:42PM +0300, Ville Syrjälä wrote:
>> On Tue, Oct 01, 2013 at 02:06:13PM +0100, Chris Wilson wrote:
>> > CC drivers/gpu/drm/drm_edid_load.o
>> > drivers/gpu/drm/drm_edid_load.c: In function ‘drm_load_edid_firmware’: include/linux/err.h:39:17: warning: ‘edid’ may be used uninitialised in this function [-Wuninitialized]
>> > drivers/gpu/drm/drm_edid_load.c:141:22: note: ‘edid’ was declared here
>> >
>> > In the process, we can make the error handling more resilient.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > ---
>> > drivers/gpu/drm/drm_edid_load.c | 75 +++++++++++++++++----------------------
>> > 1 file changed, 32 insertions(+), 43 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
>> > index 271b42b..4b57a4c 100644
>> > --- a/drivers/gpu/drm/drm_edid_load.c
>> > +++ b/drivers/gpu/drm/drm_edid_load.c
>> > @@ -136,59 +136,51 @@ static u8 generic_edid[GENERIC_EDIDS][128] = {
>> > static u8 *edid_load(struct drm_connector *connector, const char *name,
>> > const char *connector_name)
>> > {
>> > - const struct firmware *fw;
>> > + const struct firmware *fw = NULL;
>> > struct platform_device *pdev;
>> > - u8 *fwdata = NULL, *edid, *new_edid;
>> > - int fwsize, expected;
>> > - int builtin = 0, err = 0;
>> > + u8 *fwdata, *edid;
>>
>> Orthogonal issue, but fwdata, generic_edid and generic_edid_names could
>> all be const.
>>
>> > + int fwsize, expected, err, builtin;
>> > int i, valid_extensions = 0;
>> > bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
>> >
>> > pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
>> > - if (IS_ERR(pdev)) {
>> > - DRM_ERROR("Failed to register EDID firmware platform device "
>> > - "for connector \"%s\"\n", connector_name);
>> > - err = -EINVAL;
>> > - goto out;
>> > - }
>> > -
>> > - err = request_firmware(&fw, name, &pdev->dev);
>> > - platform_device_unregister(pdev);
>> > + if (!IS_ERR(pdev)) {
>> > + err = request_firmware(&fw, name, &pdev->dev);
>> > + platform_device_unregister(pdev);
>> > + } else
>> > + err = PTR_ERR(pdev);
>> >
>> > - if (err) {
>> > + if (err == 0) {
>> > + fwdata = (u8 *)fw->data;
>> > + fwsize = fw->size;
>> > + builtin = 0;
>> > + } else {
>> > i = 0;
>> > while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i]))
>> > i++;
>> > - if (i < GENERIC_EDIDS) {
>> > - err = 0;
>> > - builtin = 1;
>> > - fwdata = generic_edid[i];
>> > - fwsize = sizeof(generic_edid[i]);
>> > + if (i >= GENERIC_EDIDS) {
>> > + DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>> > + name, err);
>> > + edid = ERR_PTR(err);
>> > + goto out;
>>
>> Due to the 'if (fw)' check in the cleanup code, you could eliminate
>> the out label.
>>
>> > }
>> > - }
>> >
>> > - if (err) {
>> > - DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>> > - name, err);
>> > - goto out;
>> > - }
>> > -
>> > - if (fwdata == NULL) {
>> > - fwdata = (u8 *) fw->data;
>> > - fwsize = fw->size;
>> > + fwdata = generic_edid[i];
>> > + fwsize = sizeof(generic_edid[i]);
>> > + builtin = 1;
>> > }
>> >
>> > expected = (fwdata[0x7e] + 1) * EDID_LENGTH;
>>
>> Not your bug, but we're missing a check for fwsize > 0x7e.
>>
>> Can't spot any real bugs, so w/ or w/o the out label idea:
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Might as well spend the time to fix up the little warts whilst we are
> here, so expect a v2 shortly.
One thing that bugs me about the current code is that with
CONFIG_FW_LOADER_USER_HELPER=y, if the firmware isn't builtin and the
kernel can't load the firmware directly from the filesystem, it will
take a full minute to timeout if userspace/udev isn't ready yet. This
usually happens when the user is trying to use the DRM builtin EDIDs,
and a user has reported this happening.
request_firmware() tries to load the firmware in order:
1) kernel builtin - fw_get_builtin_firmware()
2) kernel direct load - fw_get_filesystem_firmware()
3) usermode helper - fw_load_from_user_helper()
Given the above order, I don't think it would be unreasonable to move
the DRM builtin EDID loading to the top of the list, especially since 3)
is prone to take a long time in early boot. An alternative would be to
use request_firmware_nowait(), but that seems like it could get messy.
I don't know if that fits in with what you're doing, or whether you'd
like to do that, but it's something to think about while at it.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH] drm: Try loading builtin EDIDs first
2013-10-02 7:52 [PATCH] drm: Prevent use of uninitialised values whilst loading edid firmware Jani Nikula
@ 2013-10-02 9:07 ` Chris Wilson
2013-10-02 9:22 ` Ville Syrjälä
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2013-10-02 9:07 UTC (permalink / raw)
To: dri-devel
If the firmware is not builtin and userspace is not yet running, we can
stall the boot process for a minute whilst the firmware loader times
out. This is contrary to expectations of providing a builtin EDID!
In the process, we can rearrange the code to make the error handling
more resilient and prevent gcc warning about unitialised variables along
the error paths.
v2: Load builtins first, fix gcc second (Jani) and cosmetics (Ville).
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
---
drivers/gpu/drm/drm_edid_load.c | 117 +++++++++++++++++++++------------------
1 file changed, 63 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 271b42b..9aed4d9 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -32,7 +32,7 @@ MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
"from built-in data or /lib/firmware instead. ");
#define GENERIC_EDIDS 5
-static char *generic_edid_name[GENERIC_EDIDS] = {
+static const char *generic_edid_name[GENERIC_EDIDS] = {
"edid/1024x768.bin",
"edid/1280x1024.bin",
"edid/1600x1200.bin",
@@ -40,7 +40,7 @@ static char *generic_edid_name[GENERIC_EDIDS] = {
"edid/1920x1080.bin",
};
-static u8 generic_edid[GENERIC_EDIDS][128] = {
+static const u8 generic_edid[GENERIC_EDIDS][128] = {
{
0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00,
0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
@@ -133,63 +133,77 @@ static u8 generic_edid[GENERIC_EDIDS][128] = {
},
};
+static int edid_size(const u8 *edid)
+{
+ return (edid[0x7e] + 1) * EDID_LENGTH;
+}
+
+static bool edid_check_size(const u8 *data, int data_size)
+{
+ if (data[0x7e] > 0x7e)
+ return false;
+
+ if (edid_size(data) != data_size)
+ return false;
+
+ return true;
+}
+
static u8 *edid_load(struct drm_connector *connector, const char *name,
const char *connector_name)
{
- const struct firmware *fw;
- struct platform_device *pdev;
- u8 *fwdata = NULL, *edid, *new_edid;
- int fwsize, expected;
- int builtin = 0, err = 0;
+ const struct firmware *fw = NULL;
+ const u8 *fwdata;
+ u8 *edid;
+ int fwsize, builtin;
int i, valid_extensions = 0;
bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
- pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
- if (IS_ERR(pdev)) {
- DRM_ERROR("Failed to register EDID firmware platform device "
- "for connector \"%s\"\n", connector_name);
- err = -EINVAL;
- goto out;
- }
-
- err = request_firmware(&fw, name, &pdev->dev);
- platform_device_unregister(pdev);
-
- if (err) {
- i = 0;
- while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i]))
- i++;
- if (i < GENERIC_EDIDS) {
- err = 0;
- builtin = 1;
+ builtin = 0;
+ for (i = 0; i < GENERIC_EDIDS; i++) {
+ if (strcmp(name, generic_edid_name[i]) == 0) {
fwdata = generic_edid[i];
fwsize = sizeof(generic_edid[i]);
+ builtin = 1;
+ break;
}
}
+ if (!builtin) {
+ struct platform_device *pdev;
+ int err;
- if (err) {
- DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
- name, err);
- goto out;
- }
+ pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
+ if (IS_ERR(pdev)) {
+ DRM_ERROR("Failed to register EDID firmware platform device "
+ "for connector \"%s\"\n", connector_name);
+ return ERR_CAST(pdev);
+ }
- if (fwdata == NULL) {
- fwdata = (u8 *) fw->data;
+ err = request_firmware(&fw, name, &pdev->dev);
+ platform_device_unregister(pdev);
+ if (err) {
+ DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
+ name, err);
+ return ERR_PTR(err);
+ }
+
+ fwdata = (const u8 *)fw->data;
fwsize = fw->size;
}
- expected = (fwdata[0x7e] + 1) * EDID_LENGTH;
- if (expected != fwsize) {
+ if (!edid_check_size(fwdata, fwsize)) {
DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
- "(expected %d, got %d)\n", name, expected, (int) fwsize);
- err = -EINVAL;
- goto relfw_out;
+ "(expected %d, got %d, limit %d)\n", name,
+ edid_size(fwdata), (int)fwsize,
+ 0x7f*EDID_LENGTH);
+ edid = ERR_PTR(-EINVAL);
+ goto out;
}
edid = kmemdup(fwdata, fwsize, GFP_KERNEL);
if (edid == NULL) {
- err = -ENOMEM;
- goto relfw_out;
+ edid = ERR_PTR(-ENOMEM);
+ goto out;
}
if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
@@ -197,8 +211,8 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
name);
kfree(edid);
- err = -EINVAL;
- goto relfw_out;
+ edid = ERR_PTR(-EINVAL);
+ goto out;
}
for (i = 1; i <= edid[0x7e]; i++) {
@@ -210,19 +224,18 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
}
if (valid_extensions != edid[0x7e]) {
+ u8 *new_edid;
+
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 == NULL) {
- err = -ENOMEM;
- kfree(edid);
- goto relfw_out;
- }
- edid = new_edid;
+ GFP_KERNEL);
+ if (new_edid)
+ edid = new_edid;
}
DRM_INFO("Got %s EDID base block and %d extension%s from "
@@ -230,13 +243,9 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
"external", valid_extensions, valid_extensions == 1 ? "" : "s",
name, connector_name);
-relfw_out:
- release_firmware(fw);
-
out:
- if (err)
- return ERR_PTR(err);
-
+ if (fw)
+ release_firmware(fw);
return edid;
}
--
1.7.9.5
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] drm: Try loading builtin EDIDs first
2013-10-02 9:07 ` [PATCH] drm: Try loading builtin EDIDs first Chris Wilson
@ 2013-10-02 9:22 ` Ville Syrjälä
2013-10-02 9:32 ` Chris Wilson
0 siblings, 1 reply; 8+ messages in thread
From: Ville Syrjälä @ 2013-10-02 9:22 UTC (permalink / raw)
To: Chris Wilson; +Cc: dri-devel
On Wed, Oct 02, 2013 at 10:07:39AM +0100, Chris Wilson wrote:
> If the firmware is not builtin and userspace is not yet running, we can
> stall the boot process for a minute whilst the firmware loader times
> out. This is contrary to expectations of providing a builtin EDID!
>
> In the process, we can rearrange the code to make the error handling
> more resilient and prevent gcc warning about unitialised variables along
> the error paths.
>
> v2: Load builtins first, fix gcc second (Jani) and cosmetics (Ville).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> ---
> drivers/gpu/drm/drm_edid_load.c | 117 +++++++++++++++++++++------------------
> 1 file changed, 63 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 271b42b..9aed4d9 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -32,7 +32,7 @@ MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
> "from built-in data or /lib/firmware instead. ");
>
> #define GENERIC_EDIDS 5
> -static char *generic_edid_name[GENERIC_EDIDS] = {
> +static const char *generic_edid_name[GENERIC_EDIDS] = {
> "edid/1024x768.bin",
> "edid/1280x1024.bin",
> "edid/1600x1200.bin",
> @@ -40,7 +40,7 @@ static char *generic_edid_name[GENERIC_EDIDS] = {
> "edid/1920x1080.bin",
> };
>
> -static u8 generic_edid[GENERIC_EDIDS][128] = {
> +static const u8 generic_edid[GENERIC_EDIDS][128] = {
> {
> 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00,
> 0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> @@ -133,63 +133,77 @@ static u8 generic_edid[GENERIC_EDIDS][128] = {
> },
> };
>
> +static int edid_size(const u8 *edid)
> +{
> + return (edid[0x7e] + 1) * EDID_LENGTH;
> +}
> +
> +static bool edid_check_size(const u8 *data, int data_size)
> +{
> + if (data[0x7e] > 0x7e)
> + return false;
That should be 'if (data_size <= 0x7e) return false;' no?
Or maybe just 'data_size < EDID_LENGTH' since we anyway want a
multiple of EDID_LENGTH.
> +
> + if (edid_size(data) != data_size)
> + return false;
> +
> + return true;
> +}
> +
> static u8 *edid_load(struct drm_connector *connector, const char *name,
> const char *connector_name)
> {
> - const struct firmware *fw;
> - struct platform_device *pdev;
> - u8 *fwdata = NULL, *edid, *new_edid;
> - int fwsize, expected;
> - int builtin = 0, err = 0;
> + const struct firmware *fw = NULL;
> + const u8 *fwdata;
> + u8 *edid;
> + int fwsize, builtin;
> int i, valid_extensions = 0;
> bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
>
> - pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> - if (IS_ERR(pdev)) {
> - DRM_ERROR("Failed to register EDID firmware platform device "
> - "for connector \"%s\"\n", connector_name);
> - err = -EINVAL;
> - goto out;
> - }
> -
> - err = request_firmware(&fw, name, &pdev->dev);
> - platform_device_unregister(pdev);
> -
> - if (err) {
> - i = 0;
> - while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i]))
> - i++;
> - if (i < GENERIC_EDIDS) {
> - err = 0;
> - builtin = 1;
> + builtin = 0;
> + for (i = 0; i < GENERIC_EDIDS; i++) {
> + if (strcmp(name, generic_edid_name[i]) == 0) {
> fwdata = generic_edid[i];
> fwsize = sizeof(generic_edid[i]);
> + builtin = 1;
> + break;
> }
> }
> + if (!builtin) {
> + struct platform_device *pdev;
> + int err;
>
> - if (err) {
> - DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
> - name, err);
> - goto out;
> - }
> + pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> + if (IS_ERR(pdev)) {
> + DRM_ERROR("Failed to register EDID firmware platform device "
> + "for connector \"%s\"\n", connector_name);
> + return ERR_CAST(pdev);
> + }
>
> - if (fwdata == NULL) {
> - fwdata = (u8 *) fw->data;
> + err = request_firmware(&fw, name, &pdev->dev);
> + platform_device_unregister(pdev);
> + if (err) {
> + DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
> + name, err);
> + return ERR_PTR(err);
> + }
> +
> + fwdata = (const u8 *)fw->data;
No need to cast now that fwdata is const.
> fwsize = fw->size;
> }
>
> - expected = (fwdata[0x7e] + 1) * EDID_LENGTH;
> - if (expected != fwsize) {
> + if (!edid_check_size(fwdata, fwsize)) {
> DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
> - "(expected %d, got %d)\n", name, expected, (int) fwsize);
> - err = -EINVAL;
> - goto relfw_out;
> + "(expected %d, got %d, limit %d)\n", name,
> + edid_size(fwdata), (int)fwsize,
> + 0x7f*EDID_LENGTH);
> + edid = ERR_PTR(-EINVAL);
> + goto out;
> }
>
> edid = kmemdup(fwdata, fwsize, GFP_KERNEL);
> if (edid == NULL) {
> - err = -ENOMEM;
> - goto relfw_out;
> + edid = ERR_PTR(-ENOMEM);
> + goto out;
> }
>
> if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
> @@ -197,8 +211,8 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
> DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
> name);
> kfree(edid);
> - err = -EINVAL;
> - goto relfw_out;
> + edid = ERR_PTR(-EINVAL);
> + goto out;
> }
>
> for (i = 1; i <= edid[0x7e]; i++) {
> @@ -210,19 +224,18 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
> }
>
> if (valid_extensions != edid[0x7e]) {
> + u8 *new_edid;
> +
> 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 == NULL) {
> - err = -ENOMEM;
> - kfree(edid);
> - goto relfw_out;
> - }
> - edid = new_edid;
> + GFP_KERNEL);
> + if (new_edid)
> + edid = new_edid;
> }
>
> DRM_INFO("Got %s EDID base block and %d extension%s from "
> @@ -230,13 +243,9 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
> "external", valid_extensions, valid_extensions == 1 ? "" : "s",
> name, connector_name);
>
> -relfw_out:
> - release_firmware(fw);
> -
> out:
> - if (err)
> - return ERR_PTR(err);
> -
> + if (fw)
> + release_firmware(fw);
> return edid;
> }
>
> --
> 1.7.9.5
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm: Try loading builtin EDIDs first
2013-10-02 9:22 ` Ville Syrjälä
@ 2013-10-02 9:32 ` Chris Wilson
2013-10-02 9:56 ` Ville Syrjälä
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2013-10-02 9:32 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: dri-devel
On Wed, Oct 02, 2013 at 12:22:07PM +0300, Ville Syrjälä wrote:
> > +static bool edid_check_size(const u8 *data, int data_size)
> > +{
> > + if (data[0x7e] > 0x7e)
> > + return false;
>
> That should be 'if (data_size <= 0x7e) return false;' no?
>
> Or maybe just 'data_size < EDID_LENGTH' since we anyway want a
> multiple of EDID_LENGTH.
Hmm, I'm missing the point here then. If edid_size() only returns a
non-zero mulitple of EDID_LENGTH, data_size must also be a non-zero
multiple of EDID_LENGTH for it to pass.
If you want to simply give me your ideal check_size()... :)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm: Try loading builtin EDIDs first
2013-10-02 9:32 ` Chris Wilson
@ 2013-10-02 9:56 ` Ville Syrjälä
2013-10-02 10:12 ` Chris Wilson
0 siblings, 1 reply; 8+ messages in thread
From: Ville Syrjälä @ 2013-10-02 9:56 UTC (permalink / raw)
To: Chris Wilson, dri-devel, Jani Nikula
On Wed, Oct 02, 2013 at 10:32:35AM +0100, Chris Wilson wrote:
> On Wed, Oct 02, 2013 at 12:22:07PM +0300, Ville Syrjälä wrote:
> > > +static bool edid_check_size(const u8 *data, int data_size)
> > > +{
> > > + if (data[0x7e] > 0x7e)
> > > + return false;
> >
> > That should be 'if (data_size <= 0x7e) return false;' no?
> >
> > Or maybe just 'data_size < EDID_LENGTH' since we anyway want a
> > multiple of EDID_LENGTH.
>
> Hmm, I'm missing the point here then. If edid_size() only returns a
> non-zero mulitple of EDID_LENGTH, data_size must also be a non-zero
> multiple of EDID_LENGTH for it to pass.
The point is to check that the edid[0x7e] access in fact lands inside
our data and not somewhere else.
>
> If you want to simply give me your ideal check_size()... :)
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH] drm: Try loading builtin EDIDs first
2013-10-02 9:56 ` Ville Syrjälä
@ 2013-10-02 10:12 ` Chris Wilson
2013-10-02 11:12 ` Jani Nikula
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2013-10-02 10:12 UTC (permalink / raw)
To: dri-devel
If the firmware is not builtin and userspace is not yet running, we can
stall the boot process for a minute whilst the firmware loader times
out. This is contrary to expectations of providing a builtin EDID!
In the process, we can rearrange the code to make the error handling
more resilient and prevent gcc warning about unitialised variables along
the error paths.
v2: Load builtins first, fix gcc second (Jani) and cosmetics (Ville).
v3: Verify that we do not read beyond the end of the fwdata (Ville)
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
---
drivers/gpu/drm/drm_edid_load.c | 108 +++++++++++++++++++--------------------
1 file changed, 54 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 271b42b..9081172 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -32,7 +32,7 @@ MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
"from built-in data or /lib/firmware instead. ");
#define GENERIC_EDIDS 5
-static char *generic_edid_name[GENERIC_EDIDS] = {
+static const char *generic_edid_name[GENERIC_EDIDS] = {
"edid/1024x768.bin",
"edid/1280x1024.bin",
"edid/1600x1200.bin",
@@ -40,7 +40,7 @@ static char *generic_edid_name[GENERIC_EDIDS] = {
"edid/1920x1080.bin",
};
-static u8 generic_edid[GENERIC_EDIDS][128] = {
+static const u8 generic_edid[GENERIC_EDIDS][128] = {
{
0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00,
0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
@@ -133,63 +133,68 @@ static 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 u8 *edid_load(struct drm_connector *connector, const char *name,
const char *connector_name)
{
- const struct firmware *fw;
- struct platform_device *pdev;
- u8 *fwdata = NULL, *edid, *new_edid;
- int fwsize, expected;
- int builtin = 0, err = 0;
+ const struct firmware *fw = NULL;
+ const u8 *fwdata;
+ u8 *edid;
+ int fwsize, builtin;
int i, valid_extensions = 0;
bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
- pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
- if (IS_ERR(pdev)) {
- DRM_ERROR("Failed to register EDID firmware platform device "
- "for connector \"%s\"\n", connector_name);
- err = -EINVAL;
- goto out;
- }
-
- err = request_firmware(&fw, name, &pdev->dev);
- platform_device_unregister(pdev);
-
- if (err) {
- i = 0;
- while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i]))
- i++;
- if (i < GENERIC_EDIDS) {
- err = 0;
- builtin = 1;
+ builtin = 0;
+ for (i = 0; i < GENERIC_EDIDS; i++) {
+ if (strcmp(name, generic_edid_name[i]) == 0) {
fwdata = generic_edid[i];
fwsize = sizeof(generic_edid[i]);
+ builtin = 1;
+ break;
}
}
+ if (!builtin) {
+ struct platform_device *pdev;
+ int err;
- if (err) {
- DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
- name, err);
- goto out;
- }
+ pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
+ if (IS_ERR(pdev)) {
+ DRM_ERROR("Failed to register EDID firmware platform device "
+ "for connector \"%s\"\n", connector_name);
+ return ERR_CAST(pdev);
+ }
+
+ err = request_firmware(&fw, name, &pdev->dev);
+ platform_device_unregister(pdev);
+ if (err) {
+ DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
+ name, err);
+ return ERR_PTR(err);
+ }
- if (fwdata == NULL) {
- fwdata = (u8 *) fw->data;
+ fwdata = fw->data;
fwsize = fw->size;
}
- expected = (fwdata[0x7e] + 1) * EDID_LENGTH;
- if (expected != fwsize) {
+ if (edid_size(fwdata, fwsize) != fwsize) {
DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
- "(expected %d, got %d)\n", name, expected, (int) fwsize);
- err = -EINVAL;
- goto relfw_out;
+ "(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) {
- err = -ENOMEM;
- goto relfw_out;
+ edid = ERR_PTR(-ENOMEM);
+ goto out;
}
if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
@@ -197,8 +202,8 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
name);
kfree(edid);
- err = -EINVAL;
- goto relfw_out;
+ edid = ERR_PTR(-EINVAL);
+ goto out;
}
for (i = 1; i <= edid[0x7e]; i++) {
@@ -210,19 +215,18 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
}
if (valid_extensions != edid[0x7e]) {
+ u8 *new_edid;
+
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 == NULL) {
- err = -ENOMEM;
- kfree(edid);
- goto relfw_out;
- }
- edid = new_edid;
+ GFP_KERNEL);
+ if (new_edid)
+ edid = new_edid;
}
DRM_INFO("Got %s EDID base block and %d extension%s from "
@@ -230,13 +234,9 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
"external", valid_extensions, valid_extensions == 1 ? "" : "s",
name, connector_name);
-relfw_out:
- release_firmware(fw);
-
out:
- if (err)
- return ERR_PTR(err);
-
+ if (fw)
+ release_firmware(fw);
return edid;
}
--
1.7.9.5
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] drm: Try loading builtin EDIDs first
2013-10-02 10:12 ` Chris Wilson
@ 2013-10-02 11:12 ` Jani Nikula
0 siblings, 0 replies; 8+ messages in thread
From: Jani Nikula @ 2013-10-02 11:12 UTC (permalink / raw)
To: Chris Wilson, dri-devel
On Wed, 02 Oct 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> If the firmware is not builtin and userspace is not yet running, we can
> stall the boot process for a minute whilst the firmware loader times
> out. This is contrary to expectations of providing a builtin EDID!
>
> In the process, we can rearrange the code to make the error handling
> more resilient and prevent gcc warning about unitialised variables along
> the error paths.
>
> v2: Load builtins first, fix gcc second (Jani) and cosmetics (Ville).
> v3: Verify that we do not read beyond the end of the fwdata (Ville)
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> ---
> drivers/gpu/drm/drm_edid_load.c | 108 +++++++++++++++++++--------------------
> 1 file changed, 54 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 271b42b..9081172 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -32,7 +32,7 @@ MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
> "from built-in data or /lib/firmware instead. ");
>
> #define GENERIC_EDIDS 5
> -static char *generic_edid_name[GENERIC_EDIDS] = {
> +static const char *generic_edid_name[GENERIC_EDIDS] = {
> "edid/1024x768.bin",
> "edid/1280x1024.bin",
> "edid/1600x1200.bin",
> @@ -40,7 +40,7 @@ static char *generic_edid_name[GENERIC_EDIDS] = {
> "edid/1920x1080.bin",
> };
>
> -static u8 generic_edid[GENERIC_EDIDS][128] = {
> +static const u8 generic_edid[GENERIC_EDIDS][128] = {
> {
> 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00,
> 0x31, 0xd8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> @@ -133,63 +133,68 @@ static 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 u8 *edid_load(struct drm_connector *connector, const char *name,
> const char *connector_name)
> {
> - const struct firmware *fw;
> - struct platform_device *pdev;
> - u8 *fwdata = NULL, *edid, *new_edid;
> - int fwsize, expected;
> - int builtin = 0, err = 0;
> + const struct firmware *fw = NULL;
> + const u8 *fwdata;
> + u8 *edid;
> + int fwsize, builtin;
> int i, valid_extensions = 0;
> bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
>
> - pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> - if (IS_ERR(pdev)) {
> - DRM_ERROR("Failed to register EDID firmware platform device "
> - "for connector \"%s\"\n", connector_name);
> - err = -EINVAL;
> - goto out;
> - }
> -
> - err = request_firmware(&fw, name, &pdev->dev);
> - platform_device_unregister(pdev);
> -
> - if (err) {
> - i = 0;
> - while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i]))
> - i++;
> - if (i < GENERIC_EDIDS) {
> - err = 0;
> - builtin = 1;
> + builtin = 0;
> + for (i = 0; i < GENERIC_EDIDS; i++) {
> + if (strcmp(name, generic_edid_name[i]) == 0) {
> fwdata = generic_edid[i];
> fwsize = sizeof(generic_edid[i]);
> + builtin = 1;
> + break;
> }
> }
> + if (!builtin) {
> + struct platform_device *pdev;
> + int err;
>
> - if (err) {
> - DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
> - name, err);
> - goto out;
> - }
> + pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> + if (IS_ERR(pdev)) {
> + DRM_ERROR("Failed to register EDID firmware platform device "
> + "for connector \"%s\"\n", connector_name);
> + return ERR_CAST(pdev);
> + }
> +
> + err = request_firmware(&fw, name, &pdev->dev);
> + platform_device_unregister(pdev);
> + if (err) {
> + DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
> + name, err);
> + return ERR_PTR(err);
> + }
>
> - if (fwdata == NULL) {
> - fwdata = (u8 *) fw->data;
> + fwdata = fw->data;
> fwsize = fw->size;
> }
>
> - expected = (fwdata[0x7e] + 1) * EDID_LENGTH;
> - if (expected != fwsize) {
> + if (edid_size(fwdata, fwsize) != fwsize) {
> DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
> - "(expected %d, got %d)\n", name, expected, (int) fwsize);
> - err = -EINVAL;
> - goto relfw_out;
> + "(expected %d, got %d\n", name,
Obligatory nitpick, closing brace missing... ;)
Good stuff.
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> + edid_size(fwdata, fwsize), (int)fwsize);
> + edid = ERR_PTR(-EINVAL);
> + goto out;
> }
>
> edid = kmemdup(fwdata, fwsize, GFP_KERNEL);
> if (edid == NULL) {
> - err = -ENOMEM;
> - goto relfw_out;
> + edid = ERR_PTR(-ENOMEM);
> + goto out;
> }
>
> if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
> @@ -197,8 +202,8 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
> DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
> name);
> kfree(edid);
> - err = -EINVAL;
> - goto relfw_out;
> + edid = ERR_PTR(-EINVAL);
> + goto out;
> }
>
> for (i = 1; i <= edid[0x7e]; i++) {
> @@ -210,19 +215,18 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
> }
>
> if (valid_extensions != edid[0x7e]) {
> + u8 *new_edid;
> +
> 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 == NULL) {
> - err = -ENOMEM;
> - kfree(edid);
> - goto relfw_out;
> - }
> - edid = new_edid;
> + GFP_KERNEL);
> + if (new_edid)
> + edid = new_edid;
> }
>
> DRM_INFO("Got %s EDID base block and %d extension%s from "
> @@ -230,13 +234,9 @@ static u8 *edid_load(struct drm_connector *connector, const char *name,
> "external", valid_extensions, valid_extensions == 1 ? "" : "s",
> name, connector_name);
>
> -relfw_out:
> - release_firmware(fw);
> -
> out:
> - if (err)
> - return ERR_PTR(err);
> -
> + if (fw)
> + release_firmware(fw);
> return edid;
> }
>
> --
> 1.7.9.5
>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-10-04 9:48 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-04 9:21 [PATCH] drm: Try loading builtin EDIDs first Chris Wilson
2013-10-04 9:50 ` Jani Nikula
-- strict thread matches above, loose matches on Subject: below --
2013-10-02 7:52 [PATCH] drm: Prevent use of uninitialised values whilst loading edid firmware Jani Nikula
2013-10-02 9:07 ` [PATCH] drm: Try loading builtin EDIDs first Chris Wilson
2013-10-02 9:22 ` Ville Syrjälä
2013-10-02 9:32 ` Chris Wilson
2013-10-02 9:56 ` Ville Syrjälä
2013-10-02 10:12 ` Chris Wilson
2013-10-02 11:12 ` Jani Nikula
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).