>-----Original Message----- >From: Sharma, Shashank >Sent: Tuesday, January 8, 2019 11:26 AM >To: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; >dri-devel@xxxxxxxxxxxxxxxxxxxxx >Cc: Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten ><maarten.lankhorst@xxxxxxxxx> >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@xxxxxxxxxxxxxxxxxxxxx] On >>> Behalf Of Sharma, Shashank >>> Sent: Thursday, December 20, 2018 11:47 PM >>> To: Shankar, Uma <uma.shankar@xxxxxxxxx>; >>> intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx >>> Cc: Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten >>> <maarten.lankhorst@xxxxxxxxx> >>> 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@xxxxxxxxx> >>>> --- >>>> 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@xxxxxxxxxxxxxxxxxxxxx >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
