[PATCH] drm: Print bad EDID notices only once

[PATCH] drm: Print bad EDID notices only once

[Andi Kleen]

I have an old monitor that reports a zero EDID block, which results in a
warning message. This happens on every screen save cycle, and maybe in
some other situations, and over time the whole kernel log gets filled
with these redundant messages. Printing it only once should be
sufficient.

Mark all the bad EDID notices as _once.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 855beafb76ff..d6b47bdcd5d7 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1916,10 +1916,10 @@ static void edid_block_status_print(enum edid_block_status status,
 		pr_debug("EDID block %d pointer is NULL\n", block_num);
 		break;
 	case EDID_BLOCK_ZERO:
-		pr_notice("EDID block %d is all zeroes\n", block_num);
+		pr_notice_once("EDID block %d is all zeroes\n", block_num);
 		break;
 	case EDID_BLOCK_HEADER_CORRUPT:
-		pr_notice("EDID has corrupt header\n");
+		pr_notice_once("EDID has corrupt header\n");
 		break;
 	case EDID_BLOCK_HEADER_REPAIR:
 		pr_debug("EDID corrupt header needs repair\n");
@@ -1933,13 +1933,13 @@ static void edid_block_status_print(enum edid_block_status status,
 				 block_num, edid_block_tag(block),
 				 edid_block_compute_checksum(block));
 		} else {
-			pr_notice("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d\n",
+			pr_notice_once("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d\n",
 				  block_num, edid_block_tag(block),
 				  edid_block_compute_checksum(block));
 		}
 		break;
 	case EDID_BLOCK_VERSION:
-		pr_notice("EDID has major version %d, instead of 1\n",
+		pr_notice_once("EDID has major version %d, instead of 1\n",
 			  block->version);
 		break;
 	default:
-- 
2.46.1

Re: [PATCH] drm: Print bad EDID notices only once

[Hamza Mahfooz]

On 9/26/24 09:32, Andi Kleen wrote:
> I have an old monitor that reports a zero EDID block, which results in a
> warning message. This happens on every screen save cycle, and maybe in
> some other situations, and over time the whole kernel log gets filled
> with these redundant messages. Printing it only once should be
> sufficient.
> 
> Mark all the bad EDID notices as _once.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>   drivers/gpu/drm/drm_edid.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 855beafb76ff..d6b47bdcd5d7 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1916,10 +1916,10 @@ static void edid_block_status_print(enum edid_block_status status,
>   		pr_debug("EDID block %d pointer is NULL\n", block_num);
>   		break;
>   	case EDID_BLOCK_ZERO:
> -		pr_notice("EDID block %d is all zeroes\n", block_num);
> +		pr_notice_once("EDID block %d is all zeroes\n", block_num);

It may be a good opportunity to switch these over to drm_notice_once()
instead.

Hamza

>   		break;
>   	case EDID_BLOCK_HEADER_CORRUPT:
> -		pr_notice("EDID has corrupt header\n");
> +		pr_notice_once("EDID has corrupt header\n");
>   		break;
>   	case EDID_BLOCK_HEADER_REPAIR:
>   		pr_debug("EDID corrupt header needs repair\n");
> @@ -1933,13 +1933,13 @@ static void edid_block_status_print(enum edid_block_status status,
>   				 block_num, edid_block_tag(block),
>   				 edid_block_compute_checksum(block));
>   		} else {
> -			pr_notice("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d\n",
> +			pr_notice_once("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d\n",
>   				  block_num, edid_block_tag(block),
>   				  edid_block_compute_checksum(block));
>   		}
>   		break;
>   	case EDID_BLOCK_VERSION:
> -		pr_notice("EDID has major version %d, instead of 1\n",
> +		pr_notice_once("EDID has major version %d, instead of 1\n",
>   			  block->version);
>   		break;
>   	default:
-- 
Hamza

Re: [PATCH] drm: Print bad EDID notices only once

[Andi Kleen]

> It may be a good opportunity to switch these over to drm_notice_once()
> instead.

I looked at it, but the callers to several levels don't have the
drm pointer that would be needed for that. It would require changing
them, and then all the drivers which call into the generic EDID code.
And even there the callers don't necessarily have
the pointer, so it would need more changes in drivers.

Maybe that's a good idea but I don't want to turn this into a gigantic
patch.

-Andi

Re: [PATCH] drm: Print bad EDID notices only once

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

Hi,

On Thu, Sep 26, 2024 at 06:32:53AM GMT, Andi Kleen wrote:
> I have an old monitor that reports a zero EDID block, which results in a
> warning message. This happens on every screen save cycle, and maybe in
> some other situations, and over time the whole kernel log gets filled
> with these redundant messages. Printing it only once should be
> sufficient.
> 
> Mark all the bad EDID notices as _once.

Is it?

I mean, it probably is if you connect and reconnect the same display,
but if it's two different then the second definitely has value.

Maxime

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

Re: [PATCH] drm: Print bad EDID notices only once

[Andi Kleen]

On Thu, Sep 26, 2024 at 03:51:09PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Thu, Sep 26, 2024 at 06:32:53AM GMT, Andi Kleen wrote:
> > I have an old monitor that reports a zero EDID block, which results in a
> > warning message. This happens on every screen save cycle, and maybe in
> > some other situations, and over time the whole kernel log gets filled
> > with these redundant messages. Printing it only once should be
> > sufficient.
> > 
> > Mark all the bad EDID notices as _once.
> 
> Is it?
> 
> I mean, it probably is if you connect and reconnect the same display,
> but if it's two different then the second definitely has value.

If the first display had a good (or differently corrupted) EDID block the
second bad one would still be printed. The case where you don't see anything
is if you connect two different displays with the same kind of EDID corruption.

I'm not sure if that is actually actionable in any way. I certainly
have no idea what I would do with it.

But I think what you're suggesting is to include some identifier per
monitor, but I'm not aware of anything that would work here because
if the EDID block is bad there is nothing that is intelligible to
the user. Including the device like it was suggested earlier also
wouldn't help because it is tied to the connector, not the monitor.

In the end it's a trade off between a dubious benefit versus
a significant down side (flooding logs).

-Andi

Re: [PATCH] drm: Print bad EDID notices only once

[Jani Nikula]

On Thu, 26 Sep 2024, Andi Kleen <ak@linux.intel.com> wrote:
> I have an old monitor that reports a zero EDID block, which results in a
> warning message. This happens on every screen save cycle, and maybe in
> some other situations, and over time the whole kernel log gets filled
> with these redundant messages. Printing it only once should be
> sufficient.
>
> Mark all the bad EDID notices as _once.

I'm afraid this is too big of a hammer. If it was possible to make this
once per display, fine, but this silences all same type warnings for all
EDID blocks for all subsequently plugged in displays.

For example, you try to plug in a display, get errors, try another
display because of that, and you no longer see warnings for the other
display.

But I hear you. I'll try to think of alternatives.

BR,
Jani.


>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 855beafb76ff..d6b47bdcd5d7 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1916,10 +1916,10 @@ static void edid_block_status_print(enum edid_block_status status,
>  		pr_debug("EDID block %d pointer is NULL\n", block_num);
>  		break;
>  	case EDID_BLOCK_ZERO:
> -		pr_notice("EDID block %d is all zeroes\n", block_num);
> +		pr_notice_once("EDID block %d is all zeroes\n", block_num);
>  		break;
>  	case EDID_BLOCK_HEADER_CORRUPT:
> -		pr_notice("EDID has corrupt header\n");
> +		pr_notice_once("EDID has corrupt header\n");
>  		break;
>  	case EDID_BLOCK_HEADER_REPAIR:
>  		pr_debug("EDID corrupt header needs repair\n");
> @@ -1933,13 +1933,13 @@ static void edid_block_status_print(enum edid_block_status status,
>  				 block_num, edid_block_tag(block),
>  				 edid_block_compute_checksum(block));
>  		} else {
> -			pr_notice("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d\n",
> +			pr_notice_once("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d\n",
>  				  block_num, edid_block_tag(block),
>  				  edid_block_compute_checksum(block));
>  		}
>  		break;
>  	case EDID_BLOCK_VERSION:
> -		pr_notice("EDID has major version %d, instead of 1\n",
> +		pr_notice_once("EDID has major version %d, instead of 1\n",
>  			  block->version);
>  		break;
>  	default:

-- 
Jani Nikula, Intel

Re: [PATCH] drm: Print bad EDID notices only once

[Ville Syrjälä]

On Thu, Sep 26, 2024 at 04:59:00PM +0300, Jani Nikula wrote:
> On Thu, 26 Sep 2024, Andi Kleen <ak@linux.intel.com> wrote:
> > I have an old monitor that reports a zero EDID block, which results in a
> > warning message. This happens on every screen save cycle, and maybe in
> > some other situations, and over time the whole kernel log gets filled
> > with these redundant messages. Printing it only once should be
> > sufficient.
> >
> > Mark all the bad EDID notices as _once.
> 
> I'm afraid this is too big of a hammer. If it was possible to make this
> once per display, fine, but this silences all same type warnings for all
> EDID blocks for all subsequently plugged in displays.
> 
> For example, you try to plug in a display, get errors, try another
> display because of that, and you no longer see warnings for the other
> display.
> 
> But I hear you. I'll try to think of alternatives.

We already have a bad_edid_counter on the connector. Presumable using
that, or adding something similar to handle other cases should be doable.

> 
> BR,
> Jani.
> 
> 
> >
> > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 855beafb76ff..d6b47bdcd5d7 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -1916,10 +1916,10 @@ static void edid_block_status_print(enum edid_block_status status,
> >  		pr_debug("EDID block %d pointer is NULL\n", block_num);
> >  		break;
> >  	case EDID_BLOCK_ZERO:
> > -		pr_notice("EDID block %d is all zeroes\n", block_num);
> > +		pr_notice_once("EDID block %d is all zeroes\n", block_num);
> >  		break;
> >  	case EDID_BLOCK_HEADER_CORRUPT:
> > -		pr_notice("EDID has corrupt header\n");
> > +		pr_notice_once("EDID has corrupt header\n");
> >  		break;
> >  	case EDID_BLOCK_HEADER_REPAIR:
> >  		pr_debug("EDID corrupt header needs repair\n");
> > @@ -1933,13 +1933,13 @@ static void edid_block_status_print(enum edid_block_status status,
> >  				 block_num, edid_block_tag(block),
> >  				 edid_block_compute_checksum(block));
> >  		} else {
> > -			pr_notice("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d\n",
> > +			pr_notice_once("EDID block %d (tag 0x%02x) checksum is invalid, remainder is %d\n",
> >  				  block_num, edid_block_tag(block),
> >  				  edid_block_compute_checksum(block));
> >  		}
> >  		break;
> >  	case EDID_BLOCK_VERSION:
> > -		pr_notice("EDID has major version %d, instead of 1\n",
> > +		pr_notice_once("EDID has major version %d, instead of 1\n",
> >  			  block->version);
> >  		break;
> >  	default:
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel