[v15 3/4] drm: Add colorspace info to AVI Infoframe
Shankar, Uma
uma.shankar at intel.com
Wed Feb 13 11:07:12 UTC 2019
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>Sent: Wednesday, February 13, 2019 4:34 PM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; Syrjala, Ville
><ville.syrjala at intel.com>; emil.l.velikov at gmail.com; Lankhorst, Maarten
><maarten.lankhorst at intel.com>
>Subject: Re: [v15 3/4] drm: Add colorspace info to AVI Infoframe
>
>On Tue, Feb 12, 2019 at 09:30:50PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>> >Sent: Tuesday, February 12, 2019 10:35 PM
>> >To: Shankar, Uma <uma.shankar at intel.com>
>> >Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
>> >Syrjala, Ville <ville.syrjala at intel.com>; emil.l.velikov at gmail.com;
>> >Lankhorst, Maarten <maarten.lankhorst at intel.com>
>> >Subject: Re: [v15 3/4] drm: Add colorspace info to AVI Infoframe
>> >
>> >On Fri, Feb 08, 2019 at 11:54:55PM +0530, Uma Shankar wrote:
>> >> This adds colorspace information to HDMI AVI infoframe.
>> >> A helper function is added to program the same.
>> >>
>> >> v2: Moved this to drm core instead of i915 driver.
>> >>
>> >> v3: Exported the helper function.
>> >>
>> >> v4: Added separate HDMI specific macro as per CTA spec.
>> >> This is separate from user exposed enum values. This is as per
>> >> Ville's suggestion.
>> >>
>> >> v5: Appended BT709 and SMPTE 170M with YCC information as per
>> >> Ville's review comment to be clear and not to be confused with RGB.
>> >>
>> >> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> >> ---
>> >> drivers/gpu/drm/drm_edid.c | 57
>> >> +++++++++++++++++++++++++++++++++++++++++++++
>> >> include/drm/drm_connector.h | 20 ++++++++++++++++
>> >> include/drm/drm_edid.h | 6 +++++
>> >> 3 files changed, 83 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_edid.c
>> >> b/drivers/gpu/drm/drm_edid.c index 990b190..5202fea 100644
>> >> --- a/drivers/gpu/drm/drm_edid.c
>> >> +++ b/drivers/gpu/drm/drm_edid.c
>> >> @@ -4925,6 +4925,63 @@ static bool is_hdmi2_sink(struct
>> >> drm_connector
>> >> *connector)
>> >> EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
>> >>
>> >> /**
>> >> + * drm_hdmi_avi_infoframe_colorspace() - fill the HDMI AVI infoframe
>> >> + * colorspace information
>> >> + * @frame: HDMI AVI infoframe
>> >> + * @conn_state: connector state
>> >> + */
>> >> +void
>> >> +drm_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame,
>> >> + const struct drm_connector_state *conn_state) {
>> >> + u32 colorimetry_val = conn_state->colorspace &
>> >> + FULL_COLORIMETRY_MASK;
>> >> +
>> >> + switch (colorimetry_val) {
>> >> + case DRM_MODE_COLORIMETRY_NO_DATA:
>> >> + colorimetry_val = HDMI_COLORIMETRY_NO_DATA;
>> >> + break;
>> >> + case HDMI_COLORIMETRY_SMPTE_170M_YCC:
>> >> + colorimetry_val = HDMI_COLORIMETRY_SMPTE_170M_YCC;
>> >> + break;
>> >> + case DRM_MODE_COLORIMETRY_BT709_YCC:
>> >> + colorimetry_val = HDMI_COLORIMETRY_BT709_YCC;
>> >> + break;
>> >> + case DRM_MODE_COLORIMETRY_XVYCC_601:
>> >> + colorimetry_val = HDMI_COLORIMETRY_XVYCC_601;
>> >> + break;
>> >> + case DRM_MODE_COLORIMETRY_XVYCC_709:
>> >> + colorimetry_val = HDMI_COLORIMETRY_XVYCC_709;
>> >> + break;
>> >> + case DRM_MODE_COLORIMETRY_SYCC_601:
>> >> + colorimetry_val = HDMI_COLORIMETRY_SYCC_601;
>> >> + break;
>> >> + case DRM_MODE_COLORIMETRY_OPYCC_601:
>> >> + colorimetry_val = HDMI_COLORIMETRY_OPYCC_601;
>> >> + break;
>> >> + case DRM_MODE_COLORIMETRY_OPRGB:
>> >> + colorimetry_val = HDMI_COLORIMETRY_OPRGB;
>> >> + break;
>> >> + case DRM_MODE_COLORIMETRY_BT2020_RGB:
>> >> + case DRM_MODE_COLORIMETRY_BT2020_YCC:
>> >> + colorimetry_val = HDMI_COLORIMETRY_BT2020_RGB;
>> >> + break;
>> >> + case DRM_MODE_COLORIMETRY_BT2020_CYCC:
>> >> + colorimetry_val = HDMI_COLORIMETRY_BT2020_CYCC;
>> >> + break;
>> >> + default:
>> >> + /* ToDo: DCI-P3 will be handled as part of ACE enabling */
>> >> + colorimetry_val = HDMI_COLORIMETRY_NO_DATA;
>> >> + break;
>> >> + }
>> >
>> >A table would probably be prettier.
>>
>> Ok, I will add that as table.
>>
>> >> +
>> >> + frame->colorimetry = colorimetry_val & NORMAL_COLORIMETRY_MASK;
>> >> + frame->extended_colorimetry = (colorimetry_val >> 2) &
>> >> + EXTENDED_COLORIMETRY_MASK;
>> >> +}
>> >> +EXPORT_SYMBOL(drm_hdmi_avi_infoframe_colorspace);
>> >> +
>> >> +/**
>> >> * drm_hdmi_avi_infoframe_quant_range() - fill the HDMI AVI infoframe
>> >> * quantization range information
>> >> * @frame: HDMI AVI infoframe
>> >> diff --git a/include/drm/drm_connector.h
>> >> b/include/drm/drm_connector.h index 4d9aa2f..efacf29 100644
>> >> --- a/include/drm/drm_connector.h
>> >> +++ b/include/drm/drm_connector.h
>> >> @@ -288,6 +288,26 @@ enum drm_panel_orientation {
>> >> #define DRM_MODE_DP_COLORIMETRY_RGB_WIDE_GAMUT 16
>> >> #define DRM_MODE_DP_COLORIMETRY_SCRGB 17
>> >>
>> >> +/* HDMI Colorspace Spec Definitions */
>> >> +#define FULL_COLORIMETRY_MASK 0x1FF
>> >> +#define NORMAL_COLORIMETRY_MASK 0x3
>> >> +#define EXTENDED_COLORIMETRY_MASK 0x7
>> >> +#define EXTENDED_ACE_COLORIMETRY_MASK 0xF
>> >> +
>> >> +#define HDMI_COLORIMETRY_NO_DATA 0x0
>> >> +#define HDMI_COLORIMETRY_SMPTE_170M_YCC 0x1
>> >> +#define HDMI_COLORIMETRY_BT709_YCC 0x2
>> >> +#define HDMI_COLORIMETRY_XVYCC_601 0x3
>> >> +#define HDMI_COLORIMETRY_XVYCC_709 0x7
>> >> +#define HDMI_COLORIMETRY_SYCC_601 0xB
>> >> +#define HDMI_COLORIMETRY_OPYCC_601 0xF
>> >> +#define HDMI_COLORIMETRY_OPRGB 0x13
>> >> +#define HDMI_COLORIMETRY_BT2020_CYCC 0x17
>> >> +#define HDMI_COLORIMETRY_BT2020_RGB 0x1C
>> >> +#define HDMI_COLORIMETRY_BT2020_YCC 0x1C
>> >
>> >Those should be 0x1b
>> >
>> >I would have suggested something like:
>> >#define C0(x) ((x) << 0)
>> >#define C1(x) ((x) << 1)
>> >#define EC0(x) ((x) << 2)
>> >#define EC1(x) ((x) << 3)
>> >etc.
>> >
>> >to make it easier to cross check against the spec. Or if a define for
>> >each bit is too much maybe just something like:
>> >
>> >#define C(x) ((x) << 0)
>> >#define EC(x) ((x) << 2)
>> >#define ACE(x) ((x) << 5)
>>
>> This looks good, will modify as per this.
>>
>> >One problem is namespace pollution though. But since these aren't
>> >used elsewhere we could move them into .c file instead. Or maybe we
>> >should stuff them into linux/hdmi.h and avoid the namespace pollution
>> >by C/EC/ACE by using an enum and undeffing the helper macros after
>> >the enum has been defined. The use of an enum would be in line with other stuff in
>hdmi.h.
>>
>> Since no one else is supposed to be using this, should I just move it
>> to the C file where we are using them to avoid any namespace issues as
>> such. If that is fine, will send out the next version addressing all the comments.
>
>Yeah, I guess having it in the c file is fine. Someone can move it if they need it
>elsewhere.
Hi Ville,
Thanks, I have floated the next version with all the comments addressed. Please check and approve
if all looks ok.
Regards,
Uma Shankar
>>
>> Thanks Ville for the review and valuable inputs.
>>
>> Regards,
>> Uma Shankar
>>
>> >
>> >> +#define HDMI_COLORIMETRY_DCI_P3_RGB_D65 0x1F
>> >> +#define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER 0x3F
>> >> +
>> >> /**
>> >> * struct drm_display_info - runtime data about the connected sink
>> >> *
>> >> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index
>> >> 8dc1a08..9d3b5b9 100644
>> >> --- a/include/drm/drm_edid.h
>> >> +++ b/include/drm/drm_edid.h
>> >> @@ -331,6 +331,7 @@ struct cea_sad {
>> >>
>> >> struct drm_encoder;
>> >> struct drm_connector;
>> >> +struct drm_connector_state;
>> >> struct drm_display_mode;
>> >>
>> >> int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads); @@
>> >> -358,6 +359,11 @@ int drm_av_sync_delay(struct drm_connector
>> >> *connector, drm_hdmi_vendor_infoframe_from_display_mode(struct
>> >hdmi_vendor_infoframe *frame,
>> >> struct drm_connector *connector,
>> >> const struct drm_display_mode *mode);
>> >> +
>> >> +void
>> >> +drm_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame,
>> >> + const struct drm_connector_state *conn_state);
>> >> +
>> >> void
>> >> drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
>> >> struct drm_connector *connector,
>> >> --
>> >> 1.9.1
>> >>
>> >> _______________________________________________
>> >> dri-devel mailing list
>> >> dri-devel at lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>
>--
>Ville Syrjälä
>Intel
More information about the dri-devel
mailing list