>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >Sent: Wednesday, February 13, 2019 4:34 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville ><ville.syrjala@xxxxxxxxx>; emil.l.velikov@xxxxxxxxx; Lankhorst, Maarten ><maarten.lankhorst@xxxxxxxxx> >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@xxxxxxxxxxxxxxx] >> >Sent: Tuesday, February 12, 2019 10:35 PM >> >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; >> >Syrjala, Ville <ville.syrjala@xxxxxxxxx>; emil.l.velikov@xxxxxxxxx; >> >Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx> >> >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@xxxxxxxxx> >> >> --- >> >> 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@xxxxxxxxxxxxxxxxxxxxx >> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > >> >-- >> >Ville Syrjälä >> >Intel > >-- >Ville Syrjälä >Intel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
