[v15 3/4] drm: Add colorspace info to AVI Infoframe
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Feb 13 11:04:03 UTC 2019
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.
>
> 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