diff mbox series

[6/8] drm/edid: Add a FIXME about DispID CEA data block revision

Message ID 20200124200231.10517-6-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/8] drm/edid: Check the number of detailed timing descriptors in the CEA ext block | expand

Commit Message

Ville Syrjälä Jan. 24, 2020, 8:02 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I don't understand what the DispID CEA data block revision
means. The spec doesn't say. I guess some DispID must have
a value of >= 3 in there or else we generally wouldn't
even parse the CEA data blocks. Or does all this code
actually not do anything?

Cc: Andres Rodriguez <andresx7@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Alex Deucher Jan. 27, 2020, 10:32 p.m. UTC | #1 
On Fri, Jan 24, 2020 at 3:02 PM Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
>
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> I don't understand what the DispID CEA data block revision
> means. The spec doesn't say. I guess some DispID must have
> a value of >= 3 in there or else we generally wouldn't
> even parse the CEA data blocks. Or does all this code
> actually not do anything?
>
> Cc: Andres Rodriguez <andresx7@gmail.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 0369a54e3d32..fd9b724067a7 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3977,6 +3977,13 @@ cea_db_tag(const u8 *db)
>  static int
>  cea_revision(const u8 *cea)
>  {
> +       /*
> +        * FIXME is this correct for the DispID variant?
> +        * The DispID spec doesn't really specify whether
> +        * this is the revision of the CEA extension or
> +        * the DispID CEA data block. And the only value
> +        * given as an example is 0.
> +        */

Same comment as the previous patch regarding the comment formatting.

Alex

>         return cea[1];
>  }
>
> --
> 2.24.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Shankar, Uma Feb. 3, 2020, 8:15 p.m. UTC | #2 
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> Sent: Saturday, January 25, 2020 1:32 AM
> To: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org; Andres Rodriguez <andresx7@gmail.com>
> Subject: [Intel-gfx] [PATCH 6/8] drm/edid: Add a FIXME about DispID CEA data block
> revision
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I don't understand what the DispID CEA data block revision means. The spec doesn't
> say. I guess some DispID must have a value of >= 3 in there or else we generally
> wouldn't even parse the CEA data blocks. Or does all this code actually not do
> anything?

This signifies the CTA extension revision (byte 1 of the block). As per the spec, seems like
Version 1 is legacy and 2 is deprecated. So version >=3 is checked here.
Refer section 7.3 of CTA-861-G

> Cc: Andres Rodriguez <andresx7@gmail.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index
> 0369a54e3d32..fd9b724067a7 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3977,6 +3977,13 @@ cea_db_tag(const u8 *db)  static int  cea_revision(const
> u8 *cea)  {
> +	/*
> +	 * FIXME is this correct for the DispID variant?
> +	 * The DispID spec doesn't really specify whether
> +	 * this is the revision of the CEA extension or
> +	 * the DispID CEA data block. And the only value
> +	 * given as an example is 0.
> +	 */
>  	return cea[1];
>  }
> 
> --
> 2.24.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Feb. 4, 2020, 1:32 p.m. UTC | #3 
On Mon, Feb 03, 2020 at 08:15:51PM +0000, Shankar, Uma wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> > Sent: Saturday, January 25, 2020 1:32 AM
> > To: dri-devel@lists.freedesktop.org
> > Cc: intel-gfx@lists.freedesktop.org; Andres Rodriguez <andresx7@gmail.com>
> > Subject: [Intel-gfx] [PATCH 6/8] drm/edid: Add a FIXME about DispID CEA data block
> > revision
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > I don't understand what the DispID CEA data block revision means. The spec doesn't
> > say. I guess some DispID must have a value of >= 3 in there or else we generally
> > wouldn't even parse the CEA data blocks. Or does all this code actually not do
> > anything?
> 
> This signifies the CTA extension revision (byte 1 of the block). As per the spec, seems like
> Version 1 is legacy and 2 is deprecated. So version >=3 is checked here.
> Refer section 7.3 of CTA-861-G

The confusion is about the revision field in the DispID CTA
block, not in the CTA extension block.

> 
> > Cc: Andres Rodriguez <andresx7@gmail.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index
> > 0369a54e3d32..fd9b724067a7 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -3977,6 +3977,13 @@ cea_db_tag(const u8 *db)  static int  cea_revision(const
> > u8 *cea)  {
> > +	/*
> > +	 * FIXME is this correct for the DispID variant?
> > +	 * The DispID spec doesn't really specify whether
> > +	 * this is the revision of the CEA extension or
> > +	 * the DispID CEA data block. And the only value
> > +	 * given as an example is 0.
> > +	 */
> >  	return cea[1];
> >  }
> > 
> > --
> > 2.24.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Shankar, Uma Feb. 4, 2020, 2:57 p.m. UTC | #4 
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Tuesday, February 4, 2020 7:02 PM
> To: Shankar, Uma <uma.shankar@intel.com>
> Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Andres
> Rodriguez <andresx7@gmail.com>
> Subject: Re: [Intel-gfx] [PATCH 6/8] drm/edid: Add a FIXME about DispID CEA data
> block revision
> 
> On Mon, Feb 03, 2020 at 08:15:51PM +0000, Shankar, Uma wrote:
> >
> >
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf
> > > Of Ville Syrjala
> > > Sent: Saturday, January 25, 2020 1:32 AM
> > > To: dri-devel@lists.freedesktop.org
> > > Cc: intel-gfx@lists.freedesktop.org; Andres Rodriguez
> > > <andresx7@gmail.com>
> > > Subject: [Intel-gfx] [PATCH 6/8] drm/edid: Add a FIXME about DispID
> > > CEA data block revision
> > >
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > I don't understand what the DispID CEA data block revision means.
> > > The spec doesn't say. I guess some DispID must have a value of >= 3
> > > in there or else we generally wouldn't even parse the CEA data
> > > blocks. Or does all this code actually not do anything?
> >
> > This signifies the CTA extension revision (byte 1 of the block). As
> > per the spec, seems like Version 1 is legacy and 2 is deprecated. So version >=3 is
> checked here.
> > Refer section 7.3 of CTA-861-G
> 
> The confusion is about the revision field in the DispID CTA block, not in the CTA
> extension block.

Oh ok, got the ambiguity here. Not sure if we actually get >3 here as value for the block revision,
totally unclear from spec, default being 0. Good to have this comment till we get some clarity on
its significance. Thanks for the clarification.

Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> >
> > > Cc: Andres Rodriguez <andresx7@gmail.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_edid.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index
> > > 0369a54e3d32..fd9b724067a7 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -3977,6 +3977,13 @@ cea_db_tag(const u8 *db)  static int
> > > cea_revision(const
> > > u8 *cea)  {
> > > +	/*
> > > +	 * FIXME is this correct for the DispID variant?
> > > +	 * The DispID spec doesn't really specify whether
> > > +	 * this is the revision of the CEA extension or
> > > +	 * the DispID CEA data block. And the only value
> > > +	 * given as an example is 0.
> > > +	 */
> > >  	return cea[1];
> > >  }
> > >
> > > --
> > > 2.24.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 0369a54e3d32..fd9b724067a7 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3977,6 +3977,13 @@  cea_db_tag(const u8 *db)
 static int
 cea_revision(const u8 *cea)
 {
+	/*
+	 * FIXME is this correct for the DispID variant?
+	 * The DispID spec doesn't really specify whether
+	 * this is the revision of the CEA extension or
+	 * the DispID CEA data block. And the only value
+	 * given as an example is 0.
+	 */
 	return cea[1];
 }