| Message ID | 1544560702-16447-4-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add HDR Metadata Parsing and handling in DRM layer | expand |
Regards Shashank On 12/12/2018 2:08 AM, Uma Shankar wrote: > HDR metadata block is introduced in CEA-861.3 spec. > Parsing the same to get the panel's HDR metadata. > > v2: Rebase and added Ville's POC changes to the patch. > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > --- > drivers/gpu/drm/drm_edid.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 106fd38..d12b74e 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -3818,6 +3818,49 @@ static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode) > mode->clock = clock; > } I guess the previous patch (or a art of previous patch) should be merged in this patch. > > +static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db) > +{ > + if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG) > + return false; > + > + if (db[1] != HDR_STATIC_METADATA_BLOCK) > + return false; > + > + return true; > +} > + > +static uint16_t eotf_supported(const u8 *edid_ext) Why uint16 ? why not uint8_t ? this is clearly one byte. > +{ > + > + return edid_ext[2] & > + (BIT(HDMI_EOTF_TRADITIONAL_GAMMA_SDR) | Should we bother about SDR bit at all ? Why not just check HDR bits ? > + BIT(HDMI_EOTF_TRADITIONAL_GAMMA_HDR) | > + BIT(HDMI_EOTF_SMPTE_ST2084)); > + > +} > + > +static uint16_t hdr_metadata_type(const u8 *edid_ext) > +{ Same uint8_t stuff > + > + return edid_ext[3] & > + BIT(HDMI_STATIC_METADATA_TYPE1); > +} > + > +static void > +drm_parse_hdr_metadata_block(struct drm_connector *connector, const u8 *db) > +{ > + uint16_t len; > + > + len = cea_db_payload_len(db); Length is incorrect here for extended tag blocks, as this function doesn't account the extended tag byte's size. So the payload length is looking for is len - 1. May be a good time to add cea_extended_db_payload_len() ? > + connector->hdr_metadata.eotf = eotf_supported(db); > + connector->hdr_metadata.metadata_type = hdr_metadata_type(db); > + > + if (len >= 5) > + connector->hdr_metadata.max_fall = db[5]; > + if (len >= 4) > + connector->hdr_metadata.max_cll = db[4]; > +} > + > static void > drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db) > { > @@ -4468,6 +4511,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector, > drm_parse_hdmi_forum_vsdb(connector, db); > if (cea_db_is_y420cmdb(db)) > drm_parse_y420cmdb_bitmap(connector, db); > + if (cea_db_is_hdmi_hdr_metadata_block(db)) > + drm_parse_hdr_metadata_block(connector, db); > } > } > - Shashank
>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of >Sharma, Shashank >Sent: Thursday, December 20, 2018 11:47 PM >To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; >dri-devel@lists.freedesktop.org >Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten ><maarten.lankhorst@intel.com> >Subject: Re: [v2 03/14] drm: Parse HDR metadata info from EDID > >Regards > >Shashank > > >On 12/12/2018 2:08 AM, Uma Shankar wrote: >> HDR metadata block is introduced in CEA-861.3 spec. >> Parsing the same to get the panel's HDR metadata. >> >> v2: Rebase and added Ville's POC changes to the patch. >> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com> >> --- >> drivers/gpu/drm/drm_edid.c | 45 >+++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 45 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> index 106fd38..d12b74e 100644 >> --- a/drivers/gpu/drm/drm_edid.c >> +++ b/drivers/gpu/drm/drm_edid.c >> @@ -3818,6 +3818,49 @@ static void fixup_detailed_cea_mode_clock(struct >drm_display_mode *mode) >> mode->clock = clock; >> } >I guess the previous patch (or a art of previous patch) should be merged in this >patch. Sure, will update this. >> +static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db) { >> + if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG) >> + return false; >> + >> + if (db[1] != HDR_STATIC_METADATA_BLOCK) >> + return false; >> + >> + return true; >> +} >> + >> +static uint16_t eotf_supported(const u8 *edid_ext) >Why uint16 ? why not uint8_t ? this is clearly one byte. Ok, will change this. >> +{ >> + >> + return edid_ext[2] & >> + (BIT(HDMI_EOTF_TRADITIONAL_GAMMA_SDR) | >Should we bother about SDR bit at all ? Why not just check HDR bits ? As per spec, all of these are EOTF types. So lets update whatever is supported. User should put correct EOTF for HDR from this list. >> + BIT(HDMI_EOTF_TRADITIONAL_GAMMA_HDR) | >> + BIT(HDMI_EOTF_SMPTE_ST2084)); >> + >> +} >> + >> +static uint16_t hdr_metadata_type(const u8 *edid_ext) { >Same uint8_t stuff Ok. Will update. >> + >> + return edid_ext[3] & >> + BIT(HDMI_STATIC_METADATA_TYPE1); >> +} >> + >> +static void >> +drm_parse_hdr_metadata_block(struct drm_connector *connector, const >> +u8 *db) { >> + uint16_t len; >> + >> + len = cea_db_payload_len(db); >Length is incorrect here for extended tag blocks, as this function doesn't account >the extended tag byte's size. So the payload length is looking for is len - 1. May be >a good time to add >cea_extended_db_payload_len() ? As per spec, the definition says length after this byte so it factors the extended tag byte well, IIUC. Please correct me if my understanding is wrong. Regards, Uma Shankar >> + connector->hdr_metadata.eotf = eotf_supported(db); >> + connector->hdr_metadata.metadata_type = hdr_metadata_type(db); >> + >> + if (len >= 5) >> + connector->hdr_metadata.max_fall = db[5]; >> + if (len >= 4) >> + connector->hdr_metadata.max_cll = db[4]; } >> + >> static void >> drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db) >> { >> @@ -4468,6 +4511,8 @@ static void drm_parse_cea_ext(struct drm_connector >*connector, >> drm_parse_hdmi_forum_vsdb(connector, db); >> if (cea_db_is_y420cmdb(db)) >> drm_parse_y420cmdb_bitmap(connector, db); >> + if (cea_db_is_hdmi_hdr_metadata_block(db)) >> + drm_parse_hdr_metadata_block(connector, db); >> } >> } >> >- Shashank > >_______________________________________________ >dri-devel mailing list >dri-devel@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
Regards Shashank On 1/8/2019 11:10 AM, Shankar, Uma wrote: > >> -----Original Message----- >> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of >> Sharma, Shashank >> Sent: Thursday, December 20, 2018 11:47 PM >> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; >> dri-devel@lists.freedesktop.org >> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten >> <maarten.lankhorst@intel.com> >> Subject: Re: [v2 03/14] drm: Parse HDR metadata info from EDID >> >> Regards >> >> Shashank >> >> >> On 12/12/2018 2:08 AM, Uma Shankar wrote: >>> HDR metadata block is introduced in CEA-861.3 spec. >>> Parsing the same to get the panel's HDR metadata. >>> >>> v2: Rebase and added Ville's POC changes to the patch. >>> >>> Signed-off-by: Uma Shankar <uma.shankar@intel.com> >>> --- >>> drivers/gpu/drm/drm_edid.c | 45 >> +++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 45 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >>> index 106fd38..d12b74e 100644 >>> --- a/drivers/gpu/drm/drm_edid.c >>> +++ b/drivers/gpu/drm/drm_edid.c >>> @@ -3818,6 +3818,49 @@ static void fixup_detailed_cea_mode_clock(struct >> drm_display_mode *mode) >>> mode->clock = clock; >>> } >> I guess the previous patch (or a art of previous patch) should be merged in this >> patch. > Sure, will update this. > >>> +static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db) { >>> + if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG) >>> + return false; >>> + >>> + if (db[1] != HDR_STATIC_METADATA_BLOCK) >>> + return false; >>> + >>> + return true; >>> +} >>> + >>> +static uint16_t eotf_supported(const u8 *edid_ext) >> Why uint16 ? why not uint8_t ? this is clearly one byte. > Ok, will change this. > >>> +{ >>> + >>> + return edid_ext[2] & >>> + (BIT(HDMI_EOTF_TRADITIONAL_GAMMA_SDR) | >> Should we bother about SDR bit at all ? Why not just check HDR bits ? > As per spec, all of these are EOTF types. So lets update whatever is supported. > User should put correct EOTF for HDR from this list. > >>> + BIT(HDMI_EOTF_TRADITIONAL_GAMMA_HDR) | >>> + BIT(HDMI_EOTF_SMPTE_ST2084)); >>> + >>> +} >>> + >>> +static uint16_t hdr_metadata_type(const u8 *edid_ext) { >> Same uint8_t stuff > Ok. Will update. > >>> + >>> + return edid_ext[3] & >>> + BIT(HDMI_STATIC_METADATA_TYPE1); >>> +} >>> + >>> +static void >>> +drm_parse_hdr_metadata_block(struct drm_connector *connector, const >>> +u8 *db) { >>> + uint16_t len; >>> + >>> + len = cea_db_payload_len(db); >> Length is incorrect here for extended tag blocks, as this function doesn't account >> the extended tag byte's size. So the payload length is looking for is len - 1. May be >> a good time to add >> cea_extended_db_payload_len() ? > As per spec, the definition says length after this byte so it factors the extended tag byte > well, IIUC. Please correct me if my understanding is wrong. This is how the data is arranged in a normal CEA DB and Extended CEA DB +--------------------+ +-----------------+ |Tag|Length= 3 | |Tag|Length=3 | +--------------------+ +-----------------+ | Data | |Extended tag | +--------------------+ +-----------------+ | Data | |Data | +--------------------+ +-----------------+ | Data | |Data | +--------------------+ +-----------------+ This function cea_db_payload_len() was written before the CEA extended blocks were introduced, so it is unaware of Extended tag code, and assumes its a part of data. But in case of extended tag block the actual data length is 3 -1 = 2. You can have a look at how we are parsing the YCBCR 4:2:0 blocks. That's why I made this comment "may be a good time to add cea_extended_db_payload_len() function" or enhance the existing function to reflect the extended tag. - Shashank > > Regards, > Uma Shankar > >>> + connector->hdr_metadata.eotf = eotf_supported(db); >>> + connector->hdr_metadata.metadata_type = hdr_metadata_type(db); >>> + >>> + if (len >= 5) >>> + connector->hdr_metadata.max_fall = db[5]; >>> + if (len >= 4) >>> + connector->hdr_metadata.max_cll = db[4]; } >>> + >>> static void >>> drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db) >>> { >>> @@ -4468,6 +4511,8 @@ static void drm_parse_cea_ext(struct drm_connector >> *connector, >>> drm_parse_hdmi_forum_vsdb(connector, db); >>> if (cea_db_is_y420cmdb(db)) >>> drm_parse_y420cmdb_bitmap(connector, db); >>> + if (cea_db_is_hdmi_hdr_metadata_block(db)) >>> + drm_parse_hdr_metadata_block(connector, db); >>> } >>> } >>> >> - Shashank >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>-----Original Message----- >From: Sharma, Shashank >Sent: Tuesday, January 8, 2019 11:26 AM >To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; >dri-devel@lists.freedesktop.org >Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten ><maarten.lankhorst@intel.com> >Subject: Re: [v2 03/14] drm: Parse HDR metadata info from EDID > >Regards > >Shashank > > >On 1/8/2019 11:10 AM, Shankar, Uma wrote: >> >>> -----Original Message----- >>> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On >>> Behalf Of Sharma, Shashank >>> Sent: Thursday, December 20, 2018 11:47 PM >>> To: Shankar, Uma <uma.shankar@intel.com>; >>> intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten >>> <maarten.lankhorst@intel.com> >>> Subject: Re: [v2 03/14] drm: Parse HDR metadata info from EDID >>> >>> Regards >>> >>> Shashank >>> >>> >>> On 12/12/2018 2:08 AM, Uma Shankar wrote: >>>> HDR metadata block is introduced in CEA-861.3 spec. >>>> Parsing the same to get the panel's HDR metadata. >>>> >>>> v2: Rebase and added Ville's POC changes to the patch. >>>> >>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com> >>>> --- >>>> drivers/gpu/drm/drm_edid.c | 45 >>> +++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 45 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >>>> index 106fd38..d12b74e 100644 >>>> --- a/drivers/gpu/drm/drm_edid.c >>>> +++ b/drivers/gpu/drm/drm_edid.c >>>> @@ -3818,6 +3818,49 @@ static void >>>> fixup_detailed_cea_mode_clock(struct >>> drm_display_mode *mode) >>>> mode->clock = clock; >>>> } >>> I guess the previous patch (or a art of previous patch) should be >>> merged in this patch. >> Sure, will update this. >> >>>> +static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db) { >>>> + if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG) >>>> + return false; >>>> + >>>> + if (db[1] != HDR_STATIC_METADATA_BLOCK) >>>> + return false; >>>> + >>>> + return true; >>>> +} >>>> + >>>> +static uint16_t eotf_supported(const u8 *edid_ext) >>> Why uint16 ? why not uint8_t ? this is clearly one byte. >> Ok, will change this. >> >>>> +{ >>>> + >>>> + return edid_ext[2] & >>>> + (BIT(HDMI_EOTF_TRADITIONAL_GAMMA_SDR) | >>> Should we bother about SDR bit at all ? Why not just check HDR bits ? >> As per spec, all of these are EOTF types. So lets update whatever is supported. >> User should put correct EOTF for HDR from this list. >> >>>> + BIT(HDMI_EOTF_TRADITIONAL_GAMMA_HDR) | >>>> + BIT(HDMI_EOTF_SMPTE_ST2084)); >>>> + >>>> +} >>>> + >>>> +static uint16_t hdr_metadata_type(const u8 *edid_ext) { >>> Same uint8_t stuff >> Ok. Will update. >> >>>> + >>>> + return edid_ext[3] & >>>> + BIT(HDMI_STATIC_METADATA_TYPE1); >>>> +} >>>> + >>>> +static void >>>> +drm_parse_hdr_metadata_block(struct drm_connector *connector, const >>>> +u8 *db) { >>>> + uint16_t len; >>>> + >>>> + len = cea_db_payload_len(db); >>> Length is incorrect here for extended tag blocks, as this function >>> doesn't account the extended tag byte's size. So the payload length >>> is looking for is len - 1. May be a good time to add >>> cea_extended_db_payload_len() ? >> As per spec, the definition says length after this byte so it factors >> the extended tag byte well, IIUC. Please correct me if my understanding is >wrong. >This is how the data is arranged in a normal CEA DB and Extended CEA DB >+--------------------+ +-----------------+ >|Tag|Length= 3 | |Tag|Length=3 | >+--------------------+ +-----------------+ >| Data | |Extended tag | >+--------------------+ +-----------------+ >| Data | |Data | >+--------------------+ +-----------------+ >| Data | |Data | >+--------------------+ +-----------------+ > >This function cea_db_payload_len() was written before the CEA extended blocks >were introduced, so it is unaware of Extended tag code, and assumes its a part of >data. But in case of extended tag block the actual data length is 3 -1 = 2. You can >have a look at how we are parsing the YCBCR 4:2:0 blocks. That's why I made this >comment "may be a good time to add cea_extended_db_payload_len() function" >or enhance the existing function to reflect the extended tag. Oh ok got it, thanks for the explanation. Will update the patch accordingly. Regards, Uma Shankar >- Shashank >> >> Regards, >> Uma Shankar >> >>>> + connector->hdr_metadata.eotf = eotf_supported(db); >>>> + connector->hdr_metadata.metadata_type = hdr_metadata_type(db); >>>> + >>>> + if (len >= 5) >>>> + connector->hdr_metadata.max_fall = db[5]; >>>> + if (len >= 4) >>>> + connector->hdr_metadata.max_cll = db[4]; } >>>> + >>>> static void >>>> drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 >*db) >>>> { >>>> @@ -4468,6 +4511,8 @@ static void drm_parse_cea_ext(struct >>>> drm_connector >>> *connector, >>>> drm_parse_hdmi_forum_vsdb(connector, db); >>>> if (cea_db_is_y420cmdb(db)) >>>> drm_parse_y420cmdb_bitmap(connector, db); >>>> + if (cea_db_is_hdmi_hdr_metadata_block(db)) >>>> + drm_parse_hdr_metadata_block(connector, db); >>>> } >>>> } >>>> >>> - Shashank >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 106fd38..d12b74e 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3818,6 +3818,49 @@ static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode) mode->clock = clock; } +static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db) +{ + if (cea_db_tag(db) != DATA_BLOCK_EXTENDED_TAG) + return false; + + if (db[1] != HDR_STATIC_METADATA_BLOCK) + return false; + + return true; +} + +static uint16_t eotf_supported(const u8 *edid_ext) +{ + + return edid_ext[2] & + (BIT(HDMI_EOTF_TRADITIONAL_GAMMA_SDR) | + BIT(HDMI_EOTF_TRADITIONAL_GAMMA_HDR) | + BIT(HDMI_EOTF_SMPTE_ST2084)); + +} + +static uint16_t hdr_metadata_type(const u8 *edid_ext) +{ + + return edid_ext[3] & + BIT(HDMI_STATIC_METADATA_TYPE1); +} + +static void +drm_parse_hdr_metadata_block(struct drm_connector *connector, const u8 *db) +{ + uint16_t len; + + len = cea_db_payload_len(db); + connector->hdr_metadata.eotf = eotf_supported(db); + connector->hdr_metadata.metadata_type = hdr_metadata_type(db); + + if (len >= 5) + connector->hdr_metadata.max_fall = db[5]; + if (len >= 4) + connector->hdr_metadata.max_cll = db[4]; +} + static void drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db) { @@ -4468,6 +4511,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector, drm_parse_hdmi_forum_vsdb(connector, db); if (cea_db_is_y420cmdb(db)) drm_parse_y420cmdb_bitmap(connector, db); + if (cea_db_is_hdmi_hdr_metadata_block(db)) + drm_parse_hdr_metadata_block(connector, db); } }
HDR metadata block is introduced in CEA-861.3 spec. Parsing the same to get the panel's HDR metadata. v2: Rebase and added Ville's POC changes to the patch. Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- drivers/gpu/drm/drm_edid.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)