[PATCH 0/3] drm/edid: don't log errors on absent CEA SAD blocks

[PATCH 0/3] drm/edid: don't log errors on absent CEA SAD blocks

[Jean Delvare]

Hi all,

This is my attempt to fix bug fdo#107825:
https://bugs.freedesktop.org/show_bug.cgi?id=107825

[PATCH 1/3] drm/amd: be quiet when no SAD block is found
[PATCH 2/3] drm/radeon: be quiet when no SAD block is found
[PATCH 3/3] drm/edid: no CEA extension is not an error

-- 
Jean Delvare
SUSE L3 Support
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/3] drm/amd: be quiet when no SAD block is found

[Jean Delvare]

It is fine for displays without audio functionality to not provide
any SAD block in their EDID. Do not log an error in that case,
just return quietly.

This fixes half of bug fdo#107825:
https://bugs.freedesktop.org/show_bug.cgi?id=107825

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c                    |    4 ++--
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c                    |    4 ++--
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c                     |    4 ++--
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c                     |    4 ++--
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c |    7 +++----
 5 files changed, 11 insertions(+), 12 deletions(-)

--- linux-5.2.orig/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c	2019-07-08 00:41:56.000000000 +0200
+++ linux-5.2/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c	2019-08-30 14:28:46.081682223 +0200
@@ -1345,10 +1345,10 @@ static void dce_v10_0_audio_write_sad_re
 	}
 
 	sad_count = drm_edid_to_sad(amdgpu_connector_edid(connector), &sads);
-	if (sad_count <= 0) {
+	if (sad_count < 0)
 		DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
+	if (sad_count <= 0)
 		return;
-	}
 	BUG_ON(!sads);
 
 	for (i = 0; i < ARRAY_SIZE(eld_reg_to_type); i++) {
--- linux-5.2.orig/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c	2019-07-08 00:41:56.000000000 +0200
+++ linux-5.2/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c	2019-08-30 14:29:27.276205310 +0200
@@ -1371,10 +1371,10 @@ static void dce_v11_0_audio_write_sad_re
 	}
 
 	sad_count = drm_edid_to_sad(amdgpu_connector_edid(connector), &sads);
-	if (sad_count <= 0) {
+	if (sad_count < 0)
 		DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
+	if (sad_count <= 0)
 		return;
-	}
 	BUG_ON(!sads);
 
 	for (i = 0; i < ARRAY_SIZE(eld_reg_to_type); i++) {
--- linux-5.2.orig/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c	2019-07-08 00:41:56.000000000 +0200
+++ linux-5.2/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c	2019-08-30 17:58:53.613953458 +0200
@@ -1248,10 +1248,10 @@ static void dce_v6_0_audio_write_sad_reg
 	}
 
 	sad_count = drm_edid_to_sad(amdgpu_connector_edid(connector), &sads);
-	if (sad_count <= 0) {
+	if (sad_count < 0)
 		DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
+	if (sad_count <= 0)
 		return;
-	}
 
 	for (i = 0; i < ARRAY_SIZE(eld_reg_to_type); i++) {
 		u32 tmp = 0;
--- linux-5.2.orig/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c	2019-07-08 00:41:56.000000000 +0200
+++ linux-5.2/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c	2019-08-30 14:29:01.948883708 +0200
@@ -1298,10 +1298,10 @@ static void dce_v8_0_audio_write_sad_reg
 	}
 
 	sad_count = drm_edid_to_sad(amdgpu_connector_edid(connector), &sads);
-	if (sad_count <= 0) {
+	if (sad_count < 0)
 		DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
+	if (sad_count <= 0)
 		return;
-	}
 	BUG_ON(!sads);
 
 	for (i = 0; i < ARRAY_SIZE(eld_reg_to_type); i++) {
--- linux-5.2.orig/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c	2019-07-08 00:41:56.000000000 +0200
+++ linux-5.2/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c	2019-08-30 14:31:03.086421910 +0200
@@ -98,11 +98,10 @@ enum dc_edid_status dm_helpers_parse_edi
 			(struct edid *) edid->raw_edid);
 
 	sad_count = drm_edid_to_sad((struct edid *) edid->raw_edid, &sads);
-	if (sad_count <= 0) {
-		DRM_INFO("SADs count is: %d, don't need to read it\n",
-				sad_count);
+	if (sad_count < 0)
+		DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
+	if (sad_count <= 0)
 		return result;
-	}
 
 	edid_caps->audio_mode_count = sad_count < DC_MAX_AUDIO_DESC_COUNT ? sad_count : DC_MAX_AUDIO_DESC_COUNT;
 	for (i = 0; i < edid_caps->audio_mode_count; ++i) {

-- 
Jean Delvare
SUSE L3 Support
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/3] drm/edid: don't log errors on absent CEA SAD blocks

[Jean Delvare]

It is fine for displays without audio functionality to not provide
any SAD block in their EDID. Do not log an error in that case,
just return quietly.

Inspired by a similar fix to the amdgpu driver in the context of bug
fdo#107825:
https://bugs.freedesktop.org/show_bug.cgi?id=107825

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/radeon/radeon_audio.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- linux-5.2.orig/drivers/gpu/drm/radeon/radeon_audio.c	2019-08-30 18:04:15.125056697 +0200
+++ linux-5.2/drivers/gpu/drm/radeon/radeon_audio.c	2019-08-30 18:04:35.078311347 +0200
@@ -367,10 +367,10 @@ static void radeon_audio_write_sad_regs(
 		return;
 
 	sad_count = drm_edid_to_sad(radeon_connector_edid(connector), &sads);
-	if (sad_count <= 0) {
+	if (sad_count < 0)
 		DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
+	if (sad_count <= 0)
 		return;
-	}
 	BUG_ON(!sads);
 
 	if (radeon_encoder->audio && radeon_encoder->audio->write_sad_regs)

-- 
Jean Delvare
SUSE L3 Support
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/radeon: be quiet when no SAD block is found

[Jean Delvare]

Oops, sorry, I messed up the subject line of that one, which should
really read "drm/radeon: be quiet when no SAD block is found".
-- 
Jean Delvare
SUSE L3 Support
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/3] drm/edid: no CEA extension is not an error

[Jean Delvare]

It is fine for displays without audio functionality to not implement
CEA extension in their EDID. Do not return an error in that case,
instead return 0 as if there was a CEA extension with no audio or
speaker block.

This fixes half of bug fdo#107825:
https://bugs.freedesktop.org/show_bug.cgi?id=107825

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/drm_edid.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- linux-5.2.orig/drivers/gpu/drm/drm_edid.c	2019-08-30 17:57:38.199990995 +0200
+++ linux-5.2/drivers/gpu/drm/drm_edid.c	2019-08-30 18:04:36.840333834 +0200
@@ -4130,7 +4130,7 @@ int drm_edid_to_sad(struct edid *edid, s
 	cea = drm_find_cea_extension(edid);
 	if (!cea) {
 		DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
-		return -ENOENT;
+		return 0;
 	}
 
 	if (cea_revision(cea) < 3) {
@@ -4191,7 +4191,7 @@ int drm_edid_to_speaker_allocation(struc
 	cea = drm_find_cea_extension(edid);
 	if (!cea) {
 		DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
-		return -ENOENT;
+		return 0;
 	}
 
 	if (cea_revision(cea) < 3) {

-- 
Jean Delvare
SUSE L3 Support
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/edid: no CEA extension is not an error

[Ville Syrjälä]

On Fri, Aug 30, 2019 at 06:16:52PM +0200, Jean Delvare wrote:
> It is fine for displays without audio functionality to not implement
> CEA extension in their EDID. Do not return an error in that case,
> instead return 0 as if there was a CEA extension with no audio or
> speaker block.
> 
> This fixes half of bug fdo#107825:
> https://bugs.freedesktop.org/show_bug.cgi?id=107825
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_edid.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- linux-5.2.orig/drivers/gpu/drm/drm_edid.c	2019-08-30 17:57:38.199990995 +0200
> +++ linux-5.2/drivers/gpu/drm/drm_edid.c	2019-08-30 18:04:36.840333834 +0200
> @@ -4130,7 +4130,7 @@ int drm_edid_to_sad(struct edid *edid, s
>  	cea = drm_find_cea_extension(edid);
>  	if (!cea) {
>  		DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
> -		return -ENOENT;
> +		return 0;
>  	}

Seems reasonable. Maybe the cea_revision<3 branches should alse return 0?

Either way
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	if (cea_revision(cea) < 3) {
> @@ -4191,7 +4191,7 @@ int drm_edid_to_speaker_allocation(struc
>  	cea = drm_find_cea_extension(edid);
>  	if (!cea) {
>  		DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
> -		return -ENOENT;
> +		return 0;
>  	}
>  
>  	if (cea_revision(cea) < 3) {
> 
> -- 
> Jean Delvare
> SUSE L3 Support
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/edid: no CEA extension is not an error

[Jean Delvare]

On Mon, 2 Sep 2019 14:46:51 +0300, Ville Syrjälä wrote:
> On Fri, Aug 30, 2019 at 06:16:52PM +0200, Jean Delvare wrote:
> > It is fine for displays without audio functionality to not implement
> > CEA extension in their EDID. Do not return an error in that case,
> > instead return 0 as if there was a CEA extension with no audio or
> > speaker block.
> > 
> > This fixes half of bug fdo#107825:
> > https://bugs.freedesktop.org/show_bug.cgi?id=107825
> > 
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_edid.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > --- linux-5.2.orig/drivers/gpu/drm/drm_edid.c	2019-08-30 17:57:38.199990995 +0200
> > +++ linux-5.2/drivers/gpu/drm/drm_edid.c	2019-08-30 18:04:36.840333834 +0200
> > @@ -4130,7 +4130,7 @@ int drm_edid_to_sad(struct edid *edid, s
> >  	cea = drm_find_cea_extension(edid);
> >  	if (!cea) {
> >  		DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
> > -		return -ENOENT;
> > +		return 0;
> >  	}  
> 
> Seems reasonable. Maybe the cea_revision<3 branches should alse return 0?

I wasn't sure about that one, as I'm not familiar with this CEA
extension thing.

If revision < 3 means the data is invalid then returning an error is
fine. If on the other hand revision < 3 simply means that the block
types we are looking for were not defined back then yes returning 0
instead would be better.

I'll do whatever developers more familiar with this topic think is
better.

> Either way
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks,
-- 
Jean Delvare
SUSE L3 Support
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/edid: no CEA extension is not an error

[Ville Syrjälä]

On Mon, Sep 02, 2019 at 01:55:21PM +0200, Jean Delvare wrote:
> On Mon, 2 Sep 2019 14:46:51 +0300, Ville Syrjälä wrote:
> > On Fri, Aug 30, 2019 at 06:16:52PM +0200, Jean Delvare wrote:
> > > It is fine for displays without audio functionality to not implement
> > > CEA extension in their EDID. Do not return an error in that case,
> > > instead return 0 as if there was a CEA extension with no audio or
> > > speaker block.
> > > 
> > > This fixes half of bug fdo#107825:
> > > https://bugs.freedesktop.org/show_bug.cgi?id=107825
> > > 
> > > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > > Cc: Sean Paul <sean@poorly.run>
> > > Cc: David Airlie <airlied@linux.ie>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/drm_edid.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > --- linux-5.2.orig/drivers/gpu/drm/drm_edid.c	2019-08-30 17:57:38.199990995 +0200
> > > +++ linux-5.2/drivers/gpu/drm/drm_edid.c	2019-08-30 18:04:36.840333834 +0200
> > > @@ -4130,7 +4130,7 @@ int drm_edid_to_sad(struct edid *edid, s
> > >  	cea = drm_find_cea_extension(edid);
> > >  	if (!cea) {
> > >  		DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
> > > -		return -ENOENT;
> > > +		return 0;
> > >  	}  
> > 
> > Seems reasonable. Maybe the cea_revision<3 branches should alse return 0?
> 
> I wasn't sure about that one, as I'm not familiar with this CEA
> extension thing.
> 
> If revision < 3 means the data is invalid then returning an error is
> fine. If on the other hand revision < 3 simply means that the block
> types we are looking for were not defined back then yes returning 0
> instead would be better.

That is indeed the case. A quick read through the code showed that
we're not 100% consistent in checing for that though. I just send
a few patches to fix that up.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel