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.