Re: [Intel-gfx] [PATCH] drm/edid: Only print the bad edid when aborting
On Thu, Oct 13, 2016 at 08:43:55PM +0100, Chris Wilson wrote: > Currently, if drm.debug is enabled, we get a DRM_ERROR message on the > intermediate edid reads. This causes transient failures in CI which > flags up the sporadic EDID read failures, which are recovered by > rereading the EDID automatically. This patch combines the reporting done > by drm_do_get_edid() itself with the bad block printing from > get_edid_block(), into a single warning associated with the connector > once all attempts to retrieve the EDID fail. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=98228 > Signed-off-by: Chris WilsonCan I haz a version of this patch without the s/block/edid/? It confuses me this early. If you want it, please split out. -Daniel > --- > drivers/gpu/drm/drm_edid.c | 82 > +- > 1 file changed, 44 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index ec77bd3e1f08..51dd10c65b53 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -1260,6 +1260,26 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned > int block, size_t len) > return ret == xfers ? 0 : -1; > } > > +static void connector_add_bad_edid(struct drm_connector *connector, > +u8 *block, int num) > +{ > + if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS)) > + return; > + > + if (drm_edid_is_zero(block, EDID_LENGTH)) { > + dev_warn(connector->dev->dev, > + "%s: EDID block %d is all zeroes.\n", > + connector->name, num); > + } else { > + dev_warn(connector->dev->dev, > + "%s: EDID block %d invalid:\n", > + connector->name, num); > + print_hex_dump(KERN_WARNING, > +" \t", DUMP_PREFIX_NONE, 16, 1, > +block, EDID_LENGTH, false); > + } > +} > + > /** > * drm_do_get_edid - get EDID data using a custom EDID block read function > * @connector: connector we're probing > @@ -1282,20 +1302,19 @@ struct edid *drm_do_get_edid(struct drm_connector > *connector, > void *data) > { > int i, j = 0, valid_extensions = 0; > - u8 *block, *new; > - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & > DRM_UT_KMS); > + u8 *edid, *new; > > - if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) > + if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) > return NULL; > > /* base block fetch */ > for (i = 0; i < 4; i++) { > - if (get_edid_block(data, block, 0, EDID_LENGTH)) > + if (get_edid_block(data, edid, 0, EDID_LENGTH)) > goto out; > - if (drm_edid_block_valid(block, 0, print_bad_edid, > + if (drm_edid_block_valid(edid, 0, false, >>edid_corrupt)) > break; > - if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) { > + if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { > connector->null_edid_counter++; > goto carp; > } > @@ -1304,58 +1323,45 @@ struct edid *drm_do_get_edid(struct drm_connector > *connector, > goto carp; > > /* if there's no extensions, we're done */ > - if (block[0x7e] == 0) > - return (struct edid *)block; > + if (edid[0x7e] == 0) > + return (struct edid *)edid; > > - new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL); > + new = krealloc(edid, (edid[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL); > if (!new) > goto out; > - block = new; > + edid = new; > + > + for (j = 1; j <= edid[0x7e]; j++) { > + u8 *block = edid + (valid_extensions + 1) * EDID_LENGTH; > > - for (j = 1; j <= block[0x7e]; j++) { > for (i = 0; i < 4; i++) { > - if (get_edid_block(data, > - block + (valid_extensions + 1) * EDID_LENGTH, > - j, EDID_LENGTH)) > + if (get_edid_block(data, block, j, EDID_LENGTH)) > goto out; > - if (drm_edid_block_valid(block + (valid_extensions + 1) > - * EDID_LENGTH, j, > - print_bad_edid, > - NULL)) { > + if (drm_edid_block_valid(block, j, false, NULL)) { > valid_extensions++; > break; > } > } > > - if (i == 4 && print_bad_edid) { > - dev_warn(connector->dev->dev, > -
Re: [Intel-gfx] [PATCH] drm/edid: Only print the bad edid when aborting
On Fri, Oct 14, 2016 at 01:46:37PM +0300, Ville Syrjälä wrote: > On Thu, Oct 13, 2016 at 08:43:55PM +0100, Chris Wilson wrote: > > Currently, if drm.debug is enabled, we get a DRM_ERROR message on the > > intermediate edid reads. This causes transient failures in CI which > > flags up the sporadic EDID read failures, which are recovered by > > rereading the EDID automatically. This patch combines the reporting done > > by drm_do_get_edid() itself with the bad block printing from > > get_edid_block(), into a single warning associated with the connector > > once all attempts to retrieve the EDID fail. > > One question is why have been getting more of these corrupt EDID reads > recently. Due to your gmbus change, or the live status revert perhaps? The recent reports are from a new Ironlake setup aiui. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/edid: Only print the bad edid when aborting
On Thu, Oct 13, 2016 at 08:43:55PM +0100, Chris Wilson wrote: > Currently, if drm.debug is enabled, we get a DRM_ERROR message on the > intermediate edid reads. This causes transient failures in CI which > flags up the sporadic EDID read failures, which are recovered by > rereading the EDID automatically. This patch combines the reporting done > by drm_do_get_edid() itself with the bad block printing from > get_edid_block(), into a single warning associated with the connector > once all attempts to retrieve the EDID fail. One question is why have been getting more of these corrupt EDID reads recently. Due to your gmbus change, or the live status revert perhaps? > > References: https://bugs.freedesktop.org/show_bug.cgi?id=98228 > Signed-off-by: Chris Wilson> --- > drivers/gpu/drm/drm_edid.c | 82 > +- > 1 file changed, 44 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index ec77bd3e1f08..51dd10c65b53 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -1260,6 +1260,26 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned > int block, size_t len) > return ret == xfers ? 0 : -1; > } > > +static void connector_add_bad_edid(struct drm_connector *connector, > +u8 *block, int num) > +{ > + if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS)) > + return; > + > + if (drm_edid_is_zero(block, EDID_LENGTH)) { > + dev_warn(connector->dev->dev, > + "%s: EDID block %d is all zeroes.\n", > + connector->name, num); > + } else { > + dev_warn(connector->dev->dev, > + "%s: EDID block %d invalid:\n", > + connector->name, num); > + print_hex_dump(KERN_WARNING, > +" \t", DUMP_PREFIX_NONE, 16, 1, > +block, EDID_LENGTH, false); > + } > +} > + > /** > * drm_do_get_edid - get EDID data using a custom EDID block read function > * @connector: connector we're probing > @@ -1282,20 +1302,19 @@ struct edid *drm_do_get_edid(struct drm_connector > *connector, > void *data) > { > int i, j = 0, valid_extensions = 0; > - u8 *block, *new; > - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & > DRM_UT_KMS); > + u8 *edid, *new; > > - if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) > + if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) > return NULL; > > /* base block fetch */ > for (i = 0; i < 4; i++) { > - if (get_edid_block(data, block, 0, EDID_LENGTH)) > + if (get_edid_block(data, edid, 0, EDID_LENGTH)) > goto out; > - if (drm_edid_block_valid(block, 0, print_bad_edid, > + if (drm_edid_block_valid(edid, 0, false, >>edid_corrupt)) > break; > - if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) { > + if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { > connector->null_edid_counter++; > goto carp; > } > @@ -1304,58 +1323,45 @@ struct edid *drm_do_get_edid(struct drm_connector > *connector, > goto carp; > > /* if there's no extensions, we're done */ > - if (block[0x7e] == 0) > - return (struct edid *)block; > + if (edid[0x7e] == 0) > + return (struct edid *)edid; > > - new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL); > + new = krealloc(edid, (edid[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL); > if (!new) > goto out; > - block = new; > + edid = new; > + > + for (j = 1; j <= edid[0x7e]; j++) { > + u8 *block = edid + (valid_extensions + 1) * EDID_LENGTH; > > - for (j = 1; j <= block[0x7e]; j++) { > for (i = 0; i < 4; i++) { > - if (get_edid_block(data, > - block + (valid_extensions + 1) * EDID_LENGTH, > - j, EDID_LENGTH)) > + if (get_edid_block(data, block, j, EDID_LENGTH)) > goto out; > - if (drm_edid_block_valid(block + (valid_extensions + 1) > - * EDID_LENGTH, j, > - print_bad_edid, > - NULL)) { > + if (drm_edid_block_valid(block, j, false, NULL)) { > valid_extensions++; > break; > } > } > > - if (i == 4 && print_bad_edid) { > -
[Intel-gfx] [PATCH] drm/edid: Only print the bad edid when aborting
Currently, if drm.debug is enabled, we get a DRM_ERROR message on the intermediate edid reads. This causes transient failures in CI which flags up the sporadic EDID read failures, which are recovered by rereading the EDID automatically. This patch combines the reporting done by drm_do_get_edid() itself with the bad block printing from get_edid_block(), into a single warning associated with the connector once all attempts to retrieve the EDID fail. References: https://bugs.freedesktop.org/show_bug.cgi?id=98228 Signed-off-by: Chris Wilson--- drivers/gpu/drm/drm_edid.c | 82 +- 1 file changed, 44 insertions(+), 38 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index ec77bd3e1f08..51dd10c65b53 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1260,6 +1260,26 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len) return ret == xfers ? 0 : -1; } +static void connector_add_bad_edid(struct drm_connector *connector, + u8 *block, int num) +{ + if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS)) + return; + + if (drm_edid_is_zero(block, EDID_LENGTH)) { + dev_warn(connector->dev->dev, +"%s: EDID block %d is all zeroes.\n", +connector->name, num); + } else { + dev_warn(connector->dev->dev, +"%s: EDID block %d invalid:\n", +connector->name, num); + print_hex_dump(KERN_WARNING, + " \t", DUMP_PREFIX_NONE, 16, 1, + block, EDID_LENGTH, false); + } +} + /** * drm_do_get_edid - get EDID data using a custom EDID block read function * @connector: connector we're probing @@ -1282,20 +1302,19 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, void *data) { int i, j = 0, valid_extensions = 0; - u8 *block, *new; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + u8 *edid, *new; - if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) + if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) return NULL; /* base block fetch */ for (i = 0; i < 4; i++) { - if (get_edid_block(data, block, 0, EDID_LENGTH)) + if (get_edid_block(data, edid, 0, EDID_LENGTH)) goto out; - if (drm_edid_block_valid(block, 0, print_bad_edid, + if (drm_edid_block_valid(edid, 0, false, >edid_corrupt)) break; - if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) { + if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { connector->null_edid_counter++; goto carp; } @@ -1304,58 +1323,45 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, goto carp; /* if there's no extensions, we're done */ - if (block[0x7e] == 0) - return (struct edid *)block; + if (edid[0x7e] == 0) + return (struct edid *)edid; - new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL); + new = krealloc(edid, (edid[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) goto out; - block = new; + edid = new; + + for (j = 1; j <= edid[0x7e]; j++) { + u8 *block = edid + (valid_extensions + 1) * EDID_LENGTH; - for (j = 1; j <= block[0x7e]; j++) { for (i = 0; i < 4; i++) { - if (get_edid_block(data, - block + (valid_extensions + 1) * EDID_LENGTH, - j, EDID_LENGTH)) + if (get_edid_block(data, block, j, EDID_LENGTH)) goto out; - if (drm_edid_block_valid(block + (valid_extensions + 1) -* EDID_LENGTH, j, -print_bad_edid, -NULL)) { + if (drm_edid_block_valid(block, j, false, NULL)) { valid_extensions++; break; } } - if (i == 4 && print_bad_edid) { - dev_warn(connector->dev->dev, -"%s: Ignoring invalid EDID block %d.\n", -connector->name, j); - - connector->bad_edid_counter++; - } + if (i == 4) + connector_add_bad_edid(connector, block, j);