| Message ID | 20240926133253.2623342-1-ak@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Print bad EDID notices only once | expand |
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:
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
> 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
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:
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
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
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:
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(-)