| Message ID | 1553078906-5991-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 |
Hi Uma, On Wed, Mar 20, 2019 at 04:18:16PM +0530, Uma Shankar wrote: > CEA 861.3 spec adds colorimetry data block for HDMI. > Parsing the block to get the colorimetry data from > panel. > While code which parses these parts (patches 2 and 3) out of EDID could be useful - it doesn't seem to actually be used anywhere in the kernel (sorry if I missed it), and I'm not sure it will/can be unless we expose more properties. The discussion last time leant towards a more useful/generic userspace library for EDID parsing, so perhaps patches 2 and 3 should be dropped. Did you have an intended use for them in the kernel? Thanks, -Brian > v2: Rebase > > v3: No Change > > v4: Addressed Shashank's review comments. Updated > colorimetry field to 16 bit as DCI-P3 got added > in CEA 861-G spec, as pointed out by Shashank. > > v5: Fixed checkpatch warnings with --strict option. > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > Reviewed-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/drm_edid.c | 25 +++++++++++++++++++++++++ > include/drm/drm_connector.h | 3 +++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index fd8a621a..676569b 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -2840,6 +2840,7 @@ static int drm_cvt_modes(struct drm_connector *connector, > #define VIDEO_BLOCK 0x02 > #define VENDOR_BLOCK 0x03 > #define SPEAKER_BLOCK 0x04 > +#define COLORIMETRY_DATA_BLOCK 0x5 > #define HDR_STATIC_METADATA_BLOCK 0x6 > #define USE_EXTENDED_TAG 0x07 > #define EXT_VIDEO_CAPABILITY_BLOCK 0x00 > @@ -3829,6 +3830,28 @@ static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode) > mode->clock = clock; > } > > +static bool cea_db_is_hdmi_colorimetry_data_block(const u8 *db) > +{ > + if (cea_db_tag(db) != USE_EXTENDED_TAG) > + return false; > + > + if (db[1] != COLORIMETRY_DATA_BLOCK) > + return false; > + > + return true; > +} > + > +static void > +drm_parse_colorimetry_data_block(struct drm_connector *connector, const u8 *db) > +{ > + struct drm_hdmi_info *info = &connector->display_info.hdmi; > + u16 len; > + > + len = cea_db_payload_len_ext(db); > + /* As per CEA 861-G spec */ > + info->colorimetry = ((db[3] & (0x1 << 7)) << 1) | db[2]; > +} > + > static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db) > { > if (cea_db_tag(db) != USE_EXTENDED_TAG) > @@ -4498,6 +4521,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector, > drm_parse_vcdb(connector, db); > if (cea_db_is_hdmi_hdr_metadata_block(db)) > drm_parse_hdr_metadata_block(connector, db); > + if (cea_db_is_hdmi_colorimetry_data_block(db)) > + drm_parse_colorimetry_data_block(connector, db); > } > } > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 29388bd..94f926e 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -206,6 +206,9 @@ struct drm_hdmi_info { > > /** @y420_dc_modes: bitmap of deep color support index */ > u8 y420_dc_modes; > + > + /* @colorimetry: bitmap of supported colorimetry modes */ > + u16 colorimetry; > }; > > /** > -- > 1.9.1 >
>-----Original Message----- >From: Brian Starkey [mailto:Brian.Starkey@arm.com] >Sent: Thursday, March 21, 2019 4:47 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Lankhorst, >Maarten <maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; >Sharma, Shashank <shashank.sharma@intel.com>; emil.l.velikov@gmail.com; Liviu >Dudau <Liviu.Dudau@arm.com>; nd <nd@arm.com> >Subject: Re: [v6 03/13] drm: Parse Colorimetry data block from EDID > >Hi Uma, > >On Wed, Mar 20, 2019 at 04:18:16PM +0530, Uma Shankar wrote: >> CEA 861.3 spec adds colorimetry data block for HDMI. >> Parsing the block to get the colorimetry data from panel. >> > >While code which parses these parts (patches 2 and 3) out of EDID could be useful - it >doesn't seem to actually be used anywhere in the kernel (sorry if I missed it), and I'm >not sure it will/can be unless we expose more properties. > >The discussion last time leant towards a more useful/generic userspace library for >EDID parsing, so perhaps patches 2 and 3 should be dropped. > >Did you have an intended use for them in the kernel? The idea was to use them for any kernel defaults or some state and error checking, when we have more such types exposed. But yeah, may be till we have it I can drop it for now and re-introduced them later when needed. Thanks & Regards, Uma Shankar >Thanks, >-Brian > >> v2: Rebase >> >> v3: No Change >> >> v4: Addressed Shashank's review comments. Updated colorimetry field to >> 16 bit as DCI-P3 got added in CEA 861-G spec, as pointed out by >> Shashank. >> >> v5: Fixed checkpatch warnings with --strict option. >> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com> >> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/drm_edid.c | 25 +++++++++++++++++++++++++ >> include/drm/drm_connector.h | 3 +++ >> 2 files changed, 28 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> index fd8a621a..676569b 100644 >> --- a/drivers/gpu/drm/drm_edid.c >> +++ b/drivers/gpu/drm/drm_edid.c >> @@ -2840,6 +2840,7 @@ static int drm_cvt_modes(struct drm_connector >*connector, >> #define VIDEO_BLOCK 0x02 >> #define VENDOR_BLOCK 0x03 >> #define SPEAKER_BLOCK 0x04 >> +#define COLORIMETRY_DATA_BLOCK 0x5 >> #define HDR_STATIC_METADATA_BLOCK 0x6 >> #define USE_EXTENDED_TAG 0x07 >> #define EXT_VIDEO_CAPABILITY_BLOCK 0x00 @@ -3829,6 +3830,28 @@ static >> void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode) >> mode->clock = clock; >> } >> >> +static bool cea_db_is_hdmi_colorimetry_data_block(const u8 *db) { >> + if (cea_db_tag(db) != USE_EXTENDED_TAG) >> + return false; >> + >> + if (db[1] != COLORIMETRY_DATA_BLOCK) >> + return false; >> + >> + return true; >> +} >> + >> +static void >> +drm_parse_colorimetry_data_block(struct drm_connector *connector, >> +const u8 *db) { >> + struct drm_hdmi_info *info = &connector->display_info.hdmi; >> + u16 len; >> + >> + len = cea_db_payload_len_ext(db); >> + /* As per CEA 861-G spec */ >> + info->colorimetry = ((db[3] & (0x1 << 7)) << 1) | db[2]; } >> + >> static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db) { >> if (cea_db_tag(db) != USE_EXTENDED_TAG) @@ -4498,6 +4521,8 @@ static >> void drm_parse_cea_ext(struct drm_connector *connector, >> drm_parse_vcdb(connector, db); >> if (cea_db_is_hdmi_hdr_metadata_block(db)) >> drm_parse_hdr_metadata_block(connector, db); >> + if (cea_db_is_hdmi_colorimetry_data_block(db)) >> + drm_parse_colorimetry_data_block(connector, db); >> } >> } >> >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h >> index 29388bd..94f926e 100644 >> --- a/include/drm/drm_connector.h >> +++ b/include/drm/drm_connector.h >> @@ -206,6 +206,9 @@ struct drm_hdmi_info { >> >> /** @y420_dc_modes: bitmap of deep color support index */ >> u8 y420_dc_modes; >> + >> + /* @colorimetry: bitmap of supported colorimetry modes */ >> + u16 colorimetry; >> }; >> >> /** >> -- >> 1.9.1 >>
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index fd8a621a..676569b 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2840,6 +2840,7 @@ static int drm_cvt_modes(struct drm_connector *connector, #define VIDEO_BLOCK 0x02 #define VENDOR_BLOCK 0x03 #define SPEAKER_BLOCK 0x04 +#define COLORIMETRY_DATA_BLOCK 0x5 #define HDR_STATIC_METADATA_BLOCK 0x6 #define USE_EXTENDED_TAG 0x07 #define EXT_VIDEO_CAPABILITY_BLOCK 0x00 @@ -3829,6 +3830,28 @@ static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode) mode->clock = clock; } +static bool cea_db_is_hdmi_colorimetry_data_block(const u8 *db) +{ + if (cea_db_tag(db) != USE_EXTENDED_TAG) + return false; + + if (db[1] != COLORIMETRY_DATA_BLOCK) + return false; + + return true; +} + +static void +drm_parse_colorimetry_data_block(struct drm_connector *connector, const u8 *db) +{ + struct drm_hdmi_info *info = &connector->display_info.hdmi; + u16 len; + + len = cea_db_payload_len_ext(db); + /* As per CEA 861-G spec */ + info->colorimetry = ((db[3] & (0x1 << 7)) << 1) | db[2]; +} + static bool cea_db_is_hdmi_hdr_metadata_block(const u8 *db) { if (cea_db_tag(db) != USE_EXTENDED_TAG) @@ -4498,6 +4521,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector, drm_parse_vcdb(connector, db); if (cea_db_is_hdmi_hdr_metadata_block(db)) drm_parse_hdr_metadata_block(connector, db); + if (cea_db_is_hdmi_colorimetry_data_block(db)) + drm_parse_colorimetry_data_block(connector, db); } } diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 29388bd..94f926e 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -206,6 +206,9 @@ struct drm_hdmi_info { /** @y420_dc_modes: bitmap of deep color support index */ u8 y420_dc_modes; + + /* @colorimetry: bitmap of supported colorimetry modes */ + u16 colorimetry; }; /**