[PATCH v2] drm: Print bad EDID notices less often

[PATCH v2] drm: Print bad EDID notices less often

[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.

Make most of these prints conditional on bad_edid_count like other verbose EDID
messages.

Signed-off-by: Andi Kleen <ak@linux.intel.com>

---

v2: Use bad_edid_count instead of _once.
---
 drivers/gpu/drm/drm_edid.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 855beafb76ff..52c8f08152fd 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1902,10 +1902,17 @@ static bool edid_block_valid(const void *block, bool base)
 				       edid_block_tag(block));
 }
 
-static void edid_block_status_print(enum edid_block_status status,
+static void edid_block_status_print(struct drm_connector *connector,
+				    enum edid_block_status status,
 				    const struct edid *block,
 				    int block_num)
 {
+	if (status != EDID_BLOCK_OK &&
+		connector &&
+		!connector->bad_edid_counter++ &&
+		!drm_debug_enabled(DRM_UT_KMS))
+		return;
+
 	switch (status) {
 	case EDID_BLOCK_OK:
 		break;
@@ -2004,7 +2011,7 @@ static bool drm_edid_block_valid(void *_block, int block_num, bool print_bad_edi
 			*edid_corrupt = true;
 	}
 
-	edid_block_status_print(status, block, block_num);
+	edid_block_status_print(NULL, status, block, block_num);
 
 	/* Determine whether we can use this block with this status. */
 	valid = edid_block_status_valid(status, edid_block_tag(block));
@@ -2375,7 +2382,7 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
 
 	status = edid_block_read(edid, 0, read_block, context);
 
-	edid_block_status_print(status, edid, 0);
+	edid_block_status_print(connector, status, edid, 0);
 
 	if (status == EDID_BLOCK_READ_FAIL)
 		goto fail;
@@ -2409,7 +2416,7 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
 
 		status = edid_block_read(block, i, read_block, context);
 
-		edid_block_status_print(status, block, i);
+		edid_block_status_print(connector, status, block, i);
 
 		if (!edid_block_status_valid(status, edid_block_tag(block))) {
 			if (status == EDID_BLOCK_READ_FAIL)
@@ -2842,7 +2849,7 @@ const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter)
 
 	status = edid_block_read(base_block, 0, drm_do_probe_ddc_edid, adapter);
 
-	edid_block_status_print(status, base_block, 0);
+	edid_block_status_print(NULL, status, base_block, 0);
 
 	if (!edid_block_status_valid(status, edid_block_tag(base_block))) {
 		edid_block_dump(KERN_NOTICE, base_block, 0);
-- 
2.46.1

Re: [PATCH v2] drm: Print bad EDID notices less often

[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.
>
> Make most of these prints conditional on bad_edid_count like other verbose EDID
> messages.
>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
>
> ---
>
> v2: Use bad_edid_count instead of _once.
> ---
>  drivers/gpu/drm/drm_edid.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 855beafb76ff..52c8f08152fd 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1902,10 +1902,17 @@ static bool edid_block_valid(const void *block, bool base)
>  				       edid_block_tag(block));
>  }
>  
> -static void edid_block_status_print(enum edid_block_status status,
> +static void edid_block_status_print(struct drm_connector *connector,
> +				    enum edid_block_status status,
>  				    const struct edid *block,
>  				    int block_num)
>  {
> +	if (status != EDID_BLOCK_OK &&
> +		connector &&
> +		!connector->bad_edid_counter++ &&

The status print has no business changing anything. Besides, this
function gets called per EDID block, not per EDID.

> +		!drm_debug_enabled(DRM_UT_KMS))

If we did that, we might just as well change the pr_* prints to
drm_dbg_kms() and be done with it.

BR,
Jani.

> +		return;
> +
>  	switch (status) {
>  	case EDID_BLOCK_OK:
>  		break;
> @@ -2004,7 +2011,7 @@ static bool drm_edid_block_valid(void *_block, int block_num, bool print_bad_edi
>  			*edid_corrupt = true;
>  	}
>  
> -	edid_block_status_print(status, block, block_num);
> +	edid_block_status_print(NULL, status, block, block_num);
>  
>  	/* Determine whether we can use this block with this status. */
>  	valid = edid_block_status_valid(status, edid_block_tag(block));
> @@ -2375,7 +2382,7 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>  
>  	status = edid_block_read(edid, 0, read_block, context);
>  
> -	edid_block_status_print(status, edid, 0);
> +	edid_block_status_print(connector, status, edid, 0);
>  
>  	if (status == EDID_BLOCK_READ_FAIL)
>  		goto fail;
> @@ -2409,7 +2416,7 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>  
>  		status = edid_block_read(block, i, read_block, context);
>  
> -		edid_block_status_print(status, block, i);
> +		edid_block_status_print(connector, status, block, i);
>  
>  		if (!edid_block_status_valid(status, edid_block_tag(block))) {
>  			if (status == EDID_BLOCK_READ_FAIL)
> @@ -2842,7 +2849,7 @@ const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter)
>  
>  	status = edid_block_read(base_block, 0, drm_do_probe_ddc_edid, adapter);
>  
> -	edid_block_status_print(status, base_block, 0);
> +	edid_block_status_print(NULL, status, base_block, 0);
>  
>  	if (!edid_block_status_valid(status, edid_block_tag(base_block))) {
>  		edid_block_dump(KERN_NOTICE, base_block, 0);

-- 
Jani Nikula, Intel

Re: [PATCH v2] drm: Print bad EDID notices less often

[Andi Kleen]

> > -static void edid_block_status_print(enum edid_block_status status,
> > +static void edid_block_status_print(struct drm_connector *connector,
> > +				    enum edid_block_status status,
> >  				    const struct edid *block,
> >  				    int block_num)
> >  {
> > +	if (status != EDID_BLOCK_OK &&
> > +		connector &&
> > +		!connector->bad_edid_counter++ &&
> 
> The status print has no business changing anything. Besides, this
> function gets called per EDID block, not per EDID.

In this case it doesn't matter because the counter only
prints once per life time of a connector structure, 
or if it wraps.

I suppose this can be moved into a wrapper.

> 
> > +		!drm_debug_enabled(DRM_UT_KMS))
> 
> If we did that, we might just as well change the pr_* prints to
> drm_dbg_kms() and be done with it.

But that would prevent it always unless debug is set?

I was trying to print it according to counter first. This matches
the existing usage.

-Andi