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@xxxxxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> ---
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
