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. > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
