| Message ID | 20190830181652.5b58727b@endymion (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/edid: don't log errors on absent CEA SAD blocks | expand |
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.
--- 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) {
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(-)