| Message ID | 20161013194355.30669-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> == Summary == > > Series 13747v1 drm/edid: Only print the bad edid when aborting > https://patchwork.freedesktop.org/api/1.0/series/13747/revisions/1/mbox/ > > Test drv_module_reload_basic: > skip -> PASS (fi-skl-6770hq) > Test kms_busy: > Subgroup basic-flip-default-b: > pass -> DMESG-WARN (fi-skl-6700k) Same still: [ 427.065188] [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe B FIFO underrun I suppose this still? https://bugs.freedesktop.org/show_bug.cgi?id=98041 > Test kms_pipe_crc_basic: > Subgroup read-crc-pipe-b-frame-sequence: > dmesg-warn -> PASS (fi-ilk-650) > Test vgem_basic: > Subgroup unload: > skip -> PASS (fi-skl-6770hq) > skip -> PASS (fi-hsw-4770) > > fi-bdw-5557u total:246 pass:231 dwarn:0 dfail:0 fail:0 skip:15 > fi-bsw-n3050 total:246 pass:204 dwarn:0 dfail:0 fail:0 skip:42 > fi-bxt-t5700 total:246 pass:216 dwarn:0 dfail:0 fail:0 skip:30 > fi-byt-n2820 total:246 pass:210 dwarn:0 dfail:0 fail:1 skip:35 > fi-hsw-4770 total:246 pass:224 dwarn:0 dfail:0 fail:0 skip:22 > fi-hsw-4770r total:246 pass:224 dwarn:0 dfail:0 fail:0 skip:22 > fi-ilk-650 total:246 pass:184 dwarn:0 dfail:0 fail:2 skip:60 > fi-ivb-3520m total:246 pass:221 dwarn:0 dfail:0 fail:0 skip:25 > fi-ivb-3770 total:246 pass:221 dwarn:0 dfail:0 fail:0 skip:25 > fi-kbl-7200u total:246 pass:222 dwarn:0 dfail:0 fail:0 skip:24 > fi-skl-6260u total:246 pass:232 dwarn:0 dfail:0 fail:0 skip:14 > fi-skl-6700hq total:246 pass:223 dwarn:0 dfail:0 fail:0 skip:23 > fi-skl-6700k total:246 pass:220 dwarn:2 dfail:0 fail:0 skip:24 > fi-skl-6770hq total:246 pass:230 dwarn:1 dfail:0 fail:1 skip:14 > fi-snb-2520m total:246 pass:210 dwarn:0 dfail:0 fail:0 skip:36 > fi-snb-2600 total:246 pass:209 dwarn:0 dfail:0 fail:0 skip:37 > > Results at /archive/results/CI_IGT_test/Patchwork_2710/ > > dbcf6fbb541e70fac7db669631958eab2e4e0d9c drm-intel-nightly: 2016y-10m- > 13d-15h-31m-19s UTC integration manifest > 88e63c4 drm/edid: Only print the bad edid when aborting > Jani Saarinen Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
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 <chris@chris-wilson.co.uk> > --- > 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, > &connector->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); > } > > - if (valid_extensions != block[0x7e]) { > - block[EDID_LENGTH-1] += block[0x7e] - valid_extensions; > - block[0x7e] = valid_extensions; > - new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); > + if (valid_extensions != edid[0x7e]) { > + edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions; > + edid[0x7e] = valid_extensions; > + new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); > if (!new) > goto out; > - block = new; > + edid = new; > } > > - return (struct edid *)block; > + return (struct edid *)edid; > > carp: > - if (print_bad_edid) { > - dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n", > - connector->name, j); > - } > - connector->bad_edid_counter++; > - > + connector_add_bad_edid(connector, edid, 0); > out: > - kfree(block); > + kfree(edid); > return NULL; > } > EXPORT_SYMBOL_GPL(drm_do_get_edid); > -- > 2.9.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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
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 Wilson <chris@chris-wilson.co.uk> Can 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, > &connector->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); > } > > - if (valid_extensions != block[0x7e]) { > - block[EDID_LENGTH-1] += block[0x7e] - valid_extensions; > - block[0x7e] = valid_extensions; > - new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); > + if (valid_extensions != edid[0x7e]) { > + edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions; > + edid[0x7e] = valid_extensions; > + new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); > if (!new) > goto out; > - block = new; > + edid = new; > } > > - return (struct edid *)block; > + return (struct edid *)edid; > > carp: > - if (print_bad_edid) { > - dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n", > - connector->name, j); > - } > - connector->bad_edid_counter++; > - > + connector_add_bad_edid(connector, edid, 0); > out: > - kfree(block); > + kfree(edid); > return NULL; > } > EXPORT_SYMBOL_GPL(drm_do_get_edid); > -- > 2.9.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> == Summary == > > Series 13747v3 drm/edid: Only print the bad edid when aborting > https://patchwork.freedesktop.org/api/1.0/series/13747/revisions/3/mbox/ > > Test drv_module_reload_basic: > dmesg-warn -> PASS (fi-skl-6700hq) > Test gem_exec_suspend: > Subgroup basic-s3: > pass -> DMESG-WARN (fi-skl-6700hq) https://bugs.freedesktop.org/show_bug.cgi?id=98353 > Test kms_pipe_crc_basic: > Subgroup suspend-read-crc-pipe-a: > pass -> DMESG-WARN (fi-skl-6700hq) https://bugs.freedesktop.org/show_bug.cgi?id=98353 > Subgroup suspend-read-crc-pipe-b: > pass -> DMESG-WARN (fi-skl-6700hq) https://bugs.freedesktop.org/show_bug.cgi?id=98353 > Subgroup suspend-read-crc-pipe-c: > pass -> DMESG-WARN (fi-skl-6700hq) https://bugs.freedesktop.org/show_bug.cgi?id=98353 Waiting for Imre's patch to fix/rework these. > fi-skl-6700hq total:246 pass:219 dwarn:4 dfail:0 fail:0 skip:23 > > Results at /Patchwork_2798/ > > 4c02932868ae2c9912cfe09d4c877a141fccbe8f drm-intel-nightly: 2016y-10m- > 24d-11h-53m-56s UTC integration manifest > 868a463 drm/edid: Only print the bad edid when aborting > Jani Saarinen Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
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, &connector->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); } - if (valid_extensions != block[0x7e]) { - block[EDID_LENGTH-1] += block[0x7e] - valid_extensions; - block[0x7e] = valid_extensions; - new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); + if (valid_extensions != edid[0x7e]) { + edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions; + edid[0x7e] = valid_extensions; + new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) goto out; - block = new; + edid = new; } - return (struct edid *)block; + return (struct edid *)edid; carp: - if (print_bad_edid) { - dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n", - connector->name, j); - } - connector->bad_edid_counter++; - + connector_add_bad_edid(connector, edid, 0); out: - kfree(block); + kfree(edid); return NULL; } EXPORT_SYMBOL_GPL(drm_do_get_edid);
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 <chris@chris-wilson.co.uk> --- drivers/gpu/drm/drm_edid.c | 82 +++++++++++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 38 deletions(-)