[PATCH] drm: fix HDR static metadata type field numbering

[PATCH] drm: fix HDR static metadata type field numbering

[Laurentiu Palcu]

According to CTA-861 specification, HDR static metadata data block allows a
sink to indicate which HDR metadata types it supports by setting the SM_0 to
SM_7 bits. Currently, only Static Metadata Type 1 is supported and this is
indicated by setting the SM_0 bit to 1.

However, the connector->hdr_sink_metadata.hdmi_type1.metadata_type is always
0, because hdr_metadata_type() in drm_edid.c checks the wrong bit.

This patch corrects the HDMI_STATIC_METADATA_TYPE1 bit position.

Signed-off-by: Laurentiu Palcu <laurentiu.palcu@nxp.com>
---
 include/linux/hdmi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index 9918a6c..216e25e 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -155,7 +155,7 @@ enum hdmi_content_type {
 };
 
 enum hdmi_metadata_type {
-	HDMI_STATIC_METADATA_TYPE1 = 1,
+	HDMI_STATIC_METADATA_TYPE1 = 0,
 };
 
 enum hdmi_eotf {
-- 
2.7.4

Re: [PATCH] drm: fix HDR static metadata type field numbering

[Ville Syrjälä]

On Wed, Nov 27, 2019 at 02:42:35PM +0000, Laurentiu Palcu wrote:
> According to CTA-861 specification, HDR static metadata data block allows a
> sink to indicate which HDR metadata types it supports by setting the SM_0 to
> SM_7 bits. Currently, only Static Metadata Type 1 is supported and this is
> indicated by setting the SM_0 bit to 1.
> 
> However, the connector->hdr_sink_metadata.hdmi_type1.metadata_type is always
> 0, because hdr_metadata_type() in drm_edid.c checks the wrong bit.
> 
> This patch corrects the HDMI_STATIC_METADATA_TYPE1 bit position.

Was confused for a while why this has even been workning, but I guess 
that's due to userspace populating the metadata infoframe blob correctly
even if we misreported the metadata types in the parsed EDID metadata
blob.

Hmm. Actually on further inspection this all seems to be dead code. The
only thing we seem to use from the parsed EDID metadata stuff is
eotf bitmask. We check that in drm_hdmi_infoframe_set_hdr_metadata()
but we don't check the metadata type.

Maybe we should just nuke this EDID parsing stuff entirely? Seems
pretty much pointless.

> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@nxp.com>
> ---
>  include/linux/hdmi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index 9918a6c..216e25e 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -155,7 +155,7 @@ enum hdmi_content_type {
>  };
>  
>  enum hdmi_metadata_type {
> -	HDMI_STATIC_METADATA_TYPE1 = 1,
> +	HDMI_STATIC_METADATA_TYPE1 = 0,
>  };
>  
>  enum hdmi_eotf {
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel

Re: [EXT] Re: [PATCH] drm: fix HDR static metadata type field numbering

[Laurentiu Palcu]

On Wed, Nov 27, 2019 at 05:17:03PM +0200, Ville Syrjälä wrote:
> Caution: EXT Email
> 
> On Wed, Nov 27, 2019 at 02:42:35PM +0000, Laurentiu Palcu wrote:
> > According to CTA-861 specification, HDR static metadata data block allows a
> > sink to indicate which HDR metadata types it supports by setting the SM_0 to
> > SM_7 bits. Currently, only Static Metadata Type 1 is supported and this is
> > indicated by setting the SM_0 bit to 1.
> >
> > However, the connector->hdr_sink_metadata.hdmi_type1.metadata_type is always
> > 0, because hdr_metadata_type() in drm_edid.c checks the wrong bit.
> >
> > This patch corrects the HDMI_STATIC_METADATA_TYPE1 bit position.
> 
> Was confused for a while why this has even been workning, but I guess
> that's due to userspace populating the metadata infoframe blob correctly
> even if we misreported the metadata types in the parsed EDID metadata
> blob.
> 
> Hmm. Actually on further inspection this all seems to be dead code. The
> only thing we seem to use from the parsed EDID metadata stuff is
> eotf bitmask. We check that in drm_hdmi_infoframe_set_hdr_metadata()
> but we don't check the metadata type.
> 
> Maybe we should just nuke this EDID parsing stuff entirely? Seems
> pretty much pointless.

I've been thinking about that but we may need the rest of the fields as
well, even though they're not currently used. I'm referring to sink's
min/max luminance data. Shouldn't we also check min/max cll, besides
eotf, to make sure the source does not pass higher/lower luminance
values, than the sink supports, for optimal content rendering?

However, CTA-861 is not very clear on how a sink should behave if
the CLL values exceed the allowed range... :/ Also, if the CLL range or
the FALL values passed in the DRM infoframe exceed the sink's advertised
min/max values, I guess the sink cannot go lower/higher than it can
anyway. In which case, we don't really need the rest of the HDR static
metadata block and nuking that part should be ok.

Thanks,
laurentiu


> 
> >
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@nxp.com>
> > ---
> >  include/linux/hdmi.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> > index 9918a6c..216e25e 100644
> > --- a/include/linux/hdmi.h
> > +++ b/include/linux/hdmi.h
> > @@ -155,7 +155,7 @@ enum hdmi_content_type {
> >  };
> >
> >  enum hdmi_metadata_type {
> > -     HDMI_STATIC_METADATA_TYPE1 = 1,
> > +     HDMI_STATIC_METADATA_TYPE1 = 0,
> >  };
> >
> >  enum hdmi_eotf {
> > --
> > 2.7.4
> 
> --
> Ville Syrjälä
> Intel

Re: [EXT] Re: [PATCH] drm: fix HDR static metadata type field numbering

[Ville Syrjälä]

On Thu, Nov 28, 2019 at 08:39:41AM +0000, Laurentiu Palcu wrote:
> On Wed, Nov 27, 2019 at 05:17:03PM +0200, Ville Syrjälä wrote:
> > Caution: EXT Email
> > 
> > On Wed, Nov 27, 2019 at 02:42:35PM +0000, Laurentiu Palcu wrote:
> > > According to CTA-861 specification, HDR static metadata data block allows a
> > > sink to indicate which HDR metadata types it supports by setting the SM_0 to
> > > SM_7 bits. Currently, only Static Metadata Type 1 is supported and this is
> > > indicated by setting the SM_0 bit to 1.
> > >
> > > However, the connector->hdr_sink_metadata.hdmi_type1.metadata_type is always
> > > 0, because hdr_metadata_type() in drm_edid.c checks the wrong bit.
> > >
> > > This patch corrects the HDMI_STATIC_METADATA_TYPE1 bit position.
> > 
> > Was confused for a while why this has even been workning, but I guess
> > that's due to userspace populating the metadata infoframe blob correctly
> > even if we misreported the metadata types in the parsed EDID metadata
> > blob.
> > 
> > Hmm. Actually on further inspection this all seems to be dead code. The
> > only thing we seem to use from the parsed EDID metadata stuff is
> > eotf bitmask. We check that in drm_hdmi_infoframe_set_hdr_metadata()
> > but we don't check the metadata type.
> > 
> > Maybe we should just nuke this EDID parsing stuff entirely? Seems
> > pretty much pointless.
> 
> I've been thinking about that but we may need the rest of the fields as
> well, even though they're not currently used. I'm referring to sink's
> min/max luminance data. Shouldn't we also check min/max cll, besides
> eotf, to make sure the source does not pass higher/lower luminance
> values, than the sink supports, for optimal content rendering?
> 
> However, CTA-861 is not very clear on how a sink should behave if
> the CLL values exceed the allowed range... :/ Also, if the CLL range or
> the FALL values passed in the DRM infoframe exceed the sink's advertised
> min/max values, I guess the sink cannot go lower/higher than it can
> anyway. In which case, we don't really need the rest of the HDR static
> metadata block and nuking that part should be ok.

I'm thinking we should just conclude that such userspace is a 
buggy mess and deserves whatever it gets.

-- 
Ville Syrjälä
Intel

Re: [EXT] Re: [PATCH] drm: fix HDR static metadata type field numbering

[abhinavk]

Hi Ville and Laurentiu

On 2019-11-28 03:14, Ville Syrjälä wrote:
> On Thu, Nov 28, 2019 at 08:39:41AM +0000, Laurentiu Palcu wrote:
>> On Wed, Nov 27, 2019 at 05:17:03PM +0200, Ville Syrjälä wrote:
>> > Caution: EXT Email
>> >
>> > On Wed, Nov 27, 2019 at 02:42:35PM +0000, Laurentiu Palcu wrote:
>> > > According to CTA-861 specification, HDR static metadata data block allows a
>> > > sink to indicate which HDR metadata types it supports by setting the SM_0 to
>> > > SM_7 bits. Currently, only Static Metadata Type 1 is supported and this is
>> > > indicated by setting the SM_0 bit to 1.
>> > >
>> > > However, the connector->hdr_sink_metadata.hdmi_type1.metadata_type is always
>> > > 0, because hdr_metadata_type() in drm_edid.c checks the wrong bit.
>> > >
>> > > This patch corrects the HDMI_STATIC_METADATA_TYPE1 bit position.
>> >
>> > Was confused for a while why this has even been workning, but I guess
>> > that's due to userspace populating the metadata infoframe blob correctly
>> > even if we misreported the metadata types in the parsed EDID metadata
>> > blob.
>> >
>> > Hmm. Actually on further inspection this all seems to be dead code. The
>> > only thing we seem to use from the parsed EDID metadata stuff is
>> > eotf bitmask. We check that in drm_hdmi_infoframe_set_hdr_metadata()
>> > but we don't check the metadata type.
>> >
>> > Maybe we should just nuke this EDID parsing stuff entirely? Seems
>> > pretty much pointless.
>> 
>> I've been thinking about that but we may need the rest of the fields 
>> as
>> well, even though they're not currently used. I'm referring to sink's
>> min/max luminance data. Shouldn't we also check min/max cll, besides
>> eotf, to make sure the source does not pass higher/lower luminance
>> values, than the sink supports, for optimal content rendering?
>> 
>> However, CTA-861 is not very clear on how a sink should behave if
>> the CLL values exceed the allowed range... :/ Also, if the CLL range 
>> or
>> the FALL values passed in the DRM infoframe exceed the sink's 
>> advertised
>> min/max values, I guess the sink cannot go lower/higher than it can
>> anyway. In which case, we don't really need the rest of the HDR static
>> metadata block and nuking that part should be ok.
> 
> I'm thinking we should just conclude that such userspace is a
> buggy mess and deserves whatever it gets.

[Abhinav] The display driver for MSM chipsets relies on the drm_edid.c 
parsing for the CEA extension blocks. The parts which use this shall be 
posted later when we post our changes for HDR support for the display 
driver for MSM chipset. Meanwhile, if there are no further concerns on 
this, we would like to go ahead with this change and get it merged as 
its an important bug fix. Thanks.