| Message ID | 1574865719-24490-1-git-send-email-laurentiu.palcu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: fix HDR static metadata type field numbering | expand |
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
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
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.
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.
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 {
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(-)