[PATCH] drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE

[PATCH] drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE

[Chris Wilson]

The warnings from parsing the EDID are not driver errors, but the
"normal but significant" conditions from the external device. As such,
they do not need the ferocity of an *ERROR*, but can use the less harsh
DRM_NOTE instead.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_edid.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 5a3b34a88ac3..24e7b282f16c 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1140,7 +1140,7 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
 			DRM_DEBUG("Assuming a KVM switch modified the CEA block but left the original checksum\n");
 		} else {
 			if (print_bad_edid)
-				DRM_ERROR("EDID checksum is invalid, remainder is %d\n", csum);
+				DRM_NOTE("EDID checksum is invalid, remainder is %d\n", csum);
 
 			goto bad;
 		}
@@ -1150,7 +1150,7 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
 	switch (raw_edid[0]) {
 	case 0: /* base */
 		if (edid->version != 1) {
-			DRM_ERROR("EDID has major version %d, instead of 1\n", edid->version);
+			DRM_NOTE("EDID has major version %d, instead of 1\n", edid->version);
 			goto bad;
 		}
 
@@ -1167,11 +1167,12 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
 bad:
 	if (print_bad_edid) {
 		if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) {
-			printk(KERN_ERR "EDID block is all zeroes\n");
+			printk(KERN_NOTICE "EDID block is all zeroes\n");
 		} else {
-			printk(KERN_ERR "Raw EDID:\n");
-			print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1,
-			       raw_edid, EDID_LENGTH, false);
+			printk(KERN_NOTICE "Raw EDID:\n");
+			print_hex_dump(KERN_NOTICE,
+				       " \t", DUMP_PREFIX_NONE, 16, 1,
+				       raw_edid, EDID_LENGTH, false);
 		}
 	}
 	return false;
@@ -4002,7 +4003,7 @@ static int validate_displayid(u8 *displayid, int length, int idx)
 		csum += displayid[i];
 	}
 	if (csum) {
-		DRM_ERROR("DisplayID checksum invalid, remainder is %d\n", csum);
+		DRM_NOTE("DisplayID checksum invalid, remainder is %d\n", csum);
 		return -EINVAL;
 	}
 	return 0;
-- 
2.11.0

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

✗ Fi.CI.BAT: failure for drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE

[Patchwork]

== Series Details ==

Series: drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE
URL   : https://patchwork.freedesktop.org/series/19477/
State : failure

== Summary ==

Series 19477v1 drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE
https://patchwork.freedesktop.org/api/1.0/series/19477/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> FAIL       (fi-skl-6770hq)

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:241  dwarn:0   dfail:0   fail:1   skip:10 
fi-snb-2520m     total:252  pass:224  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:223  dwarn:0   dfail:0   fail:0   skip:29 

4dbd7c0fbb78579ff491ef1184f78087055c5aa5 drm-tip: 2017y-02m-10d-21h-45m-14s UTC integration manifest
7741f88 drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3776/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 825 bytes --]

On Fri, Feb 10, 2017 at 07:59:13PM +0000, Chris Wilson wrote:
> The warnings from parsing the EDID are not driver errors, but the
> "normal but significant" conditions from the external device. As such,
> they do not need the ferocity of an *ERROR*, but can use the less harsh
> DRM_NOTE instead.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_edid.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)

The below are all conditions that happen when the EDID is bad. I'm not
sure that really qualifies as "normal".

From a quick look through the code we don't always trigger an error from
the below failure paths at higher levels, so decreasing the level here
has the potential to let this kind of exceptional condition go
unnoticed.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE

[Chris Wilson]

On Mon, Feb 13, 2017 at 08:41:10AM +0100, Thierry Reding wrote:
> On Fri, Feb 10, 2017 at 07:59:13PM +0000, Chris Wilson wrote:
> > The warnings from parsing the EDID are not driver errors, but the
> > "normal but significant" conditions from the external device. As such,
> > they do not need the ferocity of an *ERROR*, but can use the less harsh
> > DRM_NOTE instead.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/drm_edid.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> The below are all conditions that happen when the EDID is bad. I'm not
> sure that really qualifies as "normal".

Often it is - a bad EDID on the monitor will always be bad. The
challenge is distinguishing that from silent data corruption during the
read - a reported read failure are trivial.
 
> From a quick look through the code we don't always trigger an error from
> the below failure paths at higher levels, so decreasing the level here
> has the potential to let this kind of exceptional condition go
> unnoticed.

The messages are not gone, they are higher than the default loglevel,
but now below the level at which they are printed to a terminal. The
bad EDID is either expected or recoverable, and definitely not fatal
so I don't think an *ERROR* is justified.
-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

✓ Fi.CI.BAT: success for drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE

[Patchwork]

== Series Details ==

Series: drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE
URL   : https://patchwork.freedesktop.org/series/19477/
State : success

== Summary ==

Series 19477v1 drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE
https://patchwork.freedesktop.org/api/1.0/series/19477/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-skl-6260u) FDO#99750
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                incomplete -> PASS       (fi-byt-n2820) FDO#99740

FDO#99750 https://bugs.freedesktop.org/show_bug.cgi?id=99750
FDO#99740 https://bugs.freedesktop.org/show_bug.cgi?id=99740

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:224  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:223  dwarn:0   dfail:0   fail:0   skip:29 

91e2366ecab8b191f15f4457bde3b4e15f5e152d drm-tip: 2017y-02m-13d-09h-00m-05s UTC integration manifest
6e4723e drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3788/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE

[Sean Paul]

On Mon, Feb 13, 2017 at 3:59 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Feb 13, 2017 at 08:41:10AM +0100, Thierry Reding wrote:
>> On Fri, Feb 10, 2017 at 07:59:13PM +0000, Chris Wilson wrote:
>> > The warnings from parsing the EDID are not driver errors, but the
>> > "normal but significant" conditions from the external device. As such,
>> > they do not need the ferocity of an *ERROR*, but can use the less harsh
>> > DRM_NOTE instead.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > ---
>> >  drivers/gpu/drm/drm_edid.c | 15 ++++++++-------
>> >  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> The below are all conditions that happen when the EDID is bad. I'm not
>> sure that really qualifies as "normal".
>
> Often it is - a bad EDID on the monitor will always be bad. The
> challenge is distinguishing that from silent data corruption during the
> read - a reported read failure are trivial.
>
>> From a quick look through the code we don't always trigger an error from
>> the below failure paths at higher levels, so decreasing the level here
>> has the potential to let this kind of exceptional condition go
>> unnoticed.
>
> The messages are not gone, they are higher than the default loglevel,
> but now below the level at which they are printed to a terminal. The
> bad EDID is either expected or recoverable, and definitely not fatal
> so I don't think an *ERROR* is justified.

I tend to agree.

The description for the KERN_NOTICE level is "normal but significant
condition". I might argue that the presence of these EDID messages
represents a normal *or* significant condition (depending on why the
EDID is bad), but I don't think it's unreasonable to expect people to
check their logs if the display/mode is not working properly.

Sean



> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE

[Daniel Vetter]

On Mon, Feb 13, 2017 at 12:17:27PM -0500, Sean Paul wrote:
> On Mon, Feb 13, 2017 at 3:59 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Mon, Feb 13, 2017 at 08:41:10AM +0100, Thierry Reding wrote:
> >> On Fri, Feb 10, 2017 at 07:59:13PM +0000, Chris Wilson wrote:
> >> > The warnings from parsing the EDID are not driver errors, but the
> >> > "normal but significant" conditions from the external device. As such,
> >> > they do not need the ferocity of an *ERROR*, but can use the less harsh
> >> > DRM_NOTE instead.
> >> >
> >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> > ---
> >> >  drivers/gpu/drm/drm_edid.c | 15 ++++++++-------
> >> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >>
> >> The below are all conditions that happen when the EDID is bad. I'm not
> >> sure that really qualifies as "normal".
> >
> > Often it is - a bad EDID on the monitor will always be bad. The
> > challenge is distinguishing that from silent data corruption during the
> > read - a reported read failure are trivial.
> >
> >> From a quick look through the code we don't always trigger an error from
> >> the below failure paths at higher levels, so decreasing the level here
> >> has the potential to let this kind of exceptional condition go
> >> unnoticed.
> >
> > The messages are not gone, they are higher than the default loglevel,
> > but now below the level at which they are printed to a terminal. The
> > bad EDID is either expected or recoverable, and definitely not fatal
> > so I don't think an *ERROR* is justified.
> 
> I tend to agree.
> 
> The description for the KERN_NOTICE level is "normal but significant
> condition". I might argue that the presence of these EDID messages
> represents a normal *or* significant condition (depending on why the
> EDID is bad), but I don't think it's unreasonable to expect people to
> check their logs if the display/mode is not working properly.

So for cases where we know that there is shit hw out there (specifically
kvm switches that mangle the cea block without adjusting the edid) we
already tune down the error to debug level. So in principle totally agree
with tuning down anything that happens because it's outside of our control
to info or debug, but do we still need this patch after the cea one has
landed? Our CI at least seems happy ...

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE

[Chris Wilson]

On Tue, Feb 14, 2017 at 10:36:09PM +0100, Daniel Vetter wrote:
> On Mon, Feb 13, 2017 at 12:17:27PM -0500, Sean Paul wrote:
> > On Mon, Feb 13, 2017 at 3:59 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Mon, Feb 13, 2017 at 08:41:10AM +0100, Thierry Reding wrote:
> > >> On Fri, Feb 10, 2017 at 07:59:13PM +0000, Chris Wilson wrote:
> > >> > The warnings from parsing the EDID are not driver errors, but the
> > >> > "normal but significant" conditions from the external device. As such,
> > >> > they do not need the ferocity of an *ERROR*, but can use the less harsh
> > >> > DRM_NOTE instead.
> > >> >
> > >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > >> > ---
> > >> >  drivers/gpu/drm/drm_edid.c | 15 ++++++++-------
> > >> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > >>
> > >> The below are all conditions that happen when the EDID is bad. I'm not
> > >> sure that really qualifies as "normal".
> > >
> > > Often it is - a bad EDID on the monitor will always be bad. The
> > > challenge is distinguishing that from silent data corruption during the
> > > read - a reported read failure are trivial.
> > >
> > >> From a quick look through the code we don't always trigger an error from
> > >> the below failure paths at higher levels, so decreasing the level here
> > >> has the potential to let this kind of exceptional condition go
> > >> unnoticed.
> > >
> > > The messages are not gone, they are higher than the default loglevel,
> > > but now below the level at which they are printed to a terminal. The
> > > bad EDID is either expected or recoverable, and definitely not fatal
> > > so I don't think an *ERROR* is justified.
> > 
> > I tend to agree.
> > 
> > The description for the KERN_NOTICE level is "normal but significant
> > condition". I might argue that the presence of these EDID messages
> > represents a normal *or* significant condition (depending on why the
> > EDID is bad), but I don't think it's unreasonable to expect people to
> > check their logs if the display/mode is not working properly.
> 
> So for cases where we know that there is shit hw out there (specifically
> kvm switches that mangle the cea block without adjusting the edid) we
> already tune down the error to debug level. So in principle totally agree
> with tuning down anything that happens because it's outside of our control
> to info or debug, but do we still need this patch after the cea one has
> landed? Our CI at least seems happy ...

Yes. The one machine with a dodgy EDID also happens to have a dodgy
BIOS. This reduces the number of consistent errors to 1, but since an
unrelated error still remains, CI doesn't detect the improvement.
https://intel-gfx-ci.01.org/CI/CI_DRM_2198/fi-skl-6700k/igt@drv_module_reload@basic-reload.html
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Reduce EDID warnings from DRM_ERROR to DRM_NOTE

[Daniel Vetter]

On Tue, Feb 14, 2017 at 09:43:45PM +0000, Chris Wilson wrote:
> On Tue, Feb 14, 2017 at 10:36:09PM +0100, Daniel Vetter wrote:
> > On Mon, Feb 13, 2017 at 12:17:27PM -0500, Sean Paul wrote:
> > > On Mon, Feb 13, 2017 at 3:59 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > On Mon, Feb 13, 2017 at 08:41:10AM +0100, Thierry Reding wrote:
> > > >> On Fri, Feb 10, 2017 at 07:59:13PM +0000, Chris Wilson wrote:
> > > >> > The warnings from parsing the EDID are not driver errors, but the
> > > >> > "normal but significant" conditions from the external device. As such,
> > > >> > they do not need the ferocity of an *ERROR*, but can use the less harsh
> > > >> > DRM_NOTE instead.
> > > >> >
> > > >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > >> > ---
> > > >> >  drivers/gpu/drm/drm_edid.c | 15 ++++++++-------
> > > >> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > > >>
> > > >> The below are all conditions that happen when the EDID is bad. I'm not
> > > >> sure that really qualifies as "normal".
> > > >
> > > > Often it is - a bad EDID on the monitor will always be bad. The
> > > > challenge is distinguishing that from silent data corruption during the
> > > > read - a reported read failure are trivial.
> > > >
> > > >> From a quick look through the code we don't always trigger an error from
> > > >> the below failure paths at higher levels, so decreasing the level here
> > > >> has the potential to let this kind of exceptional condition go
> > > >> unnoticed.
> > > >
> > > > The messages are not gone, they are higher than the default loglevel,
> > > > but now below the level at which they are printed to a terminal. The
> > > > bad EDID is either expected or recoverable, and definitely not fatal
> > > > so I don't think an *ERROR* is justified.
> > > 
> > > I tend to agree.
> > > 
> > > The description for the KERN_NOTICE level is "normal but significant
> > > condition". I might argue that the presence of these EDID messages
> > > represents a normal *or* significant condition (depending on why the
> > > EDID is bad), but I don't think it's unreasonable to expect people to
> > > check their logs if the display/mode is not working properly.
> > 
> > So for cases where we know that there is shit hw out there (specifically
> > kvm switches that mangle the cea block without adjusting the edid) we
> > already tune down the error to debug level. So in principle totally agree
> > with tuning down anything that happens because it's outside of our control
> > to info or debug, but do we still need this patch after the cea one has
> > landed? Our CI at least seems happy ...
> 
> Yes. The one machine with a dodgy EDID also happens to have a dodgy
> BIOS. This reduces the number of consistent errors to 1, but since an
> unrelated error still remains, CI doesn't detect the improvement.
> https://intel-gfx-ci.01.org/CI/CI_DRM_2198/fi-skl-6700k/igt@drv_module_reload@basic-reload.html

Ok, count my convinced, I pushed the patch to drm-misc-next.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx