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

Submitted by Jean Delvare on Aug. 30, 2019, 4:16 p.m.

Details

Message ID 20190830181652.5b58727b@endymion
State New
Headers show
Series "drm/edid: don't log errors on absent CEA SAD blocks" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Jean Delvare Aug. 30, 2019, 4:16 p.m.
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(-)

Patch hide | download patch | download mbox

--- 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) {

Comments

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