| Message ID | 1540987546-3142-2-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Colorspace connector property interface | expand |
Op 31-10-18 om 13:05 schreef Uma Shankar: > This patch adds a colorspace property enabling > userspace to switch to various supported colorspaces. > This will help enable BT2020 along with other colorspaces. > > v2: Addressed Maarten and Ville's review comments. Enhanced > the colorspace enum to incorporate both HDMI and DP supported > colorspaces. Also, added a default option for colorspace. > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > --- > drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ > drivers/gpu/drm/drm_connector.c | 44 +++++++++++++++++++++++++++++++++++++++ > include/drm/drm_connector.h | 7 +++++++ > include/drm/drm_mode_config.h | 6 ++++++ > include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++++ > 5 files changed, 85 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index d5b7f31..9e1d46b 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > state->picture_aspect_ratio = val; > } else if (property == config->content_type_property) { > state->content_type = val; > + } else if (property == config->colorspace_property) { > + state->colorspace = val; > } else if (property == connector->scaling_mode_property) { > state->scaling_mode = val; > } else if (property == connector->content_protection_property) { > @@ -795,6 +797,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > *val = state->picture_aspect_ratio; > } else if (property == config->content_type_property) { > *val = state->content_type; > + } else if (property == config->colorspace_property) { > + *val = state->colorspace; > } else if (property == connector->scaling_mode_property) { > *val = state->scaling_mode; > } else if (property == connector->content_protection_property) { > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index aa18b1d..5ad52a0 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -826,6 +826,38 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > }; > DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list) > > +static const struct drm_prop_enum_list colorspace[] = { > + /* Standard Definition Colorimetry based on CEA 861 */ > + { COLORIMETRY_ITU_601, "601" }, > + { COLORIMETRY_ITU_709, "709" }, > + /* Standard Definition Colorimetry based on IEC 61966-2-4 */ > + { COLORIMETRY_XV_YCC_601, "601" }, > + /* High Definition Colorimetry based on IEC 61966-2-4 */ > + { COLORIMETRY_XV_YCC_709, "709" }, 2 definitions that share the same name? All in all, I think the enum strings need to be more descriptive and self-consistent. > + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ > + { COLORIMETRY_S_YCC_601, "s601" }, > + /* Colorimetry based on IEC 61966-2-5 [33] */ > + { COLORIMETRY_ADOBE_YCC_601, "adobe601" }, > + /* Colorimetry based on IEC 61966-2-5 */ > + { COLORIMETRY_ADOBE_RGB, "adobe_rgb" }, > + /* Colorimetry based on ITU-R BT.2020 */ > + { COLORIMETRY_BT2020_RGB, "BT2020_rgb" }, > + /* Colorimetry based on ITU-R BT.2020 */ > + { COLORIMETRY_BT2020_YCC, "BT2020_ycc" }, > + /* Colorimetry based on ITU-R BT.2020 */ > + { COLORIMETRY_BT2020_CYCC, "BT2020_cycc" }, > + /* DP MSA Colorimetry */ > + { COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" }, > + { COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" }, > + { COLORIMETRY_SRGB, "SRGB" }, sRGB > + { COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" }, > + { COLORIMETRY_SCRGB, "SCRGB" }, scRGB? > + { COLORIMETRY_DCI_P3, "DCIP3" }, DCI-P3? > + { COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" }, ^Typo > + /* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */ > + { COLORIMETRY_DEFAULT, "Default: ITU_709" }, This enum together with the code below is broken. + COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, I would just make it COLORIMETRY_UNSET = "Unset". To set the default colorimetry for all drivers, just make the default value 0 (preferred), or add it to __drm_atomic_helper_connector_reset(). > +}; > + > /** > * DOC: standard connector properties > * > @@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > * can also expose this property to external outputs, in which case they > * must support "None", which should be the default (since external screens > * have a built-in scaler). > + * > + * colorspace: > + * This property helps select a suitable colorspace based on the sink > + * capability. Modern sink devices support wider gamut like BT2020. > + * This helps switch to BT2020 mode if the BT2020 encoded video stream > + * is being played by the user, same for any other colorspace. > */ > > int drm_connector_create_standard_properties(struct drm_device *dev) > @@ -1020,6 +1058,12 @@ int drm_connector_create_standard_properties(struct drm_device *dev) > return -ENOMEM; > dev->mode_config.non_desktop_property = prop; > > + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace", > + colorspace, ARRAY_SIZE(colorspace)); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.colorspace_property = prop; > + > return 0; > } > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index dd0552c..b7f5373 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -497,6 +497,13 @@ struct drm_connector_state { > unsigned int content_protection; > > /** > + * @colorspace: Connector property to request colorspace > + * change. This is most commonly used to switch to wider color > + * gamuts like BT2020. > + */ > + enum encoder_colorimetry colorspace; > + > + /** > * @writeback_job: Writeback job for writeback connectors > * > * Holds the framebuffer and out-fence for a writeback connector. As > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index d643d26..a6eb0aa 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -863,6 +863,12 @@ struct drm_mode_config { > uint32_t cursor_width, cursor_height; > > /** > + * @colorspace_property: Connector property to set the suitable > + * colorspace supported by the sink. > + */ > + struct drm_property *colorspace_property; > + > + /** > * @suspend_state: > * > * Atomic state when suspended. > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index d3e0fe3..831cc77 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -210,6 +210,30 @@ > #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 > #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 > > +enum encoder_colorimetry { > + /* CEA 861 Normal Colorimetry options */ > + COLORIMETRY_ITU_601 = 0, > + COLORIMETRY_ITU_709, > + /* CEA 861 Extended Colorimetry Options */ > + COLORIMETRY_XV_YCC_601, > + COLORIMETRY_XV_YCC_709, > + COLORIMETRY_S_YCC_601, > + COLORIMETRY_ADOBE_YCC_601, > + COLORIMETRY_ADOBE_RGB, > + COLORIMETRY_BT2020_RGB, > + COLORIMETRY_BT2020_YCC, > + COLORIMETRY_BT2020_CYCC, > + /* DP MSA Colorimetry Options */ > + COLORIMETRY_Y_CBCR_ITU_601, > + COLORIMETRY_Y_CBCR_ITU_709, > + COLORIMETRY_SRGB, > + COLORIMETRY_RGB_WIDE_GAMUT, > + COLORIMETRY_SCRGB, > + COLORIMETRY_DCI_P3, > + COLORIMETRY_CUSTOM_COLOR_PROFILE, > + COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, > +}; > + > struct drm_mode_modeinfo { > __u32 clock; > __u16 hdisplay;
On Fri, Nov 02, 2018 at 10:19:10AM +0100, Maarten Lankhorst wrote: > Op 31-10-18 om 13:05 schreef Uma Shankar: > > This patch adds a colorspace property enabling > > userspace to switch to various supported colorspaces. > > This will help enable BT2020 along with other colorspaces. > > > > v2: Addressed Maarten and Ville's review comments. Enhanced > > the colorspace enum to incorporate both HDMI and DP supported > > colorspaces. Also, added a default option for colorspace. > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > --- > > drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ > > drivers/gpu/drm/drm_connector.c | 44 +++++++++++++++++++++++++++++++++++++++ > > include/drm/drm_connector.h | 7 +++++++ > > include/drm/drm_mode_config.h | 6 ++++++ > > include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++++ > > 5 files changed, 85 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > > index d5b7f31..9e1d46b 100644 > > --- a/drivers/gpu/drm/drm_atomic_uapi.c > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > > @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > > state->picture_aspect_ratio = val; > > } else if (property == config->content_type_property) { > > state->content_type = val; > > + } else if (property == config->colorspace_property) { > > + state->colorspace = val; > > } else if (property == connector->scaling_mode_property) { > > state->scaling_mode = val; > > } else if (property == connector->content_protection_property) { > > @@ -795,6 +797,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > > *val = state->picture_aspect_ratio; > > } else if (property == config->content_type_property) { > > *val = state->content_type; > > + } else if (property == config->colorspace_property) { > > + *val = state->colorspace; > > } else if (property == connector->scaling_mode_property) { > > *val = state->scaling_mode; > > } else if (property == connector->content_protection_property) { > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > > index aa18b1d..5ad52a0 100644 > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -826,6 +826,38 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > > }; > > DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list) > > > > +static const struct drm_prop_enum_list colorspace[] = { > > + /* Standard Definition Colorimetry based on CEA 861 */ > > + { COLORIMETRY_ITU_601, "601" }, > > + { COLORIMETRY_ITU_709, "709" }, > > + /* Standard Definition Colorimetry based on IEC 61966-2-4 */ > > + { COLORIMETRY_XV_YCC_601, "601" }, > > + /* High Definition Colorimetry based on IEC 61966-2-4 */ > > + { COLORIMETRY_XV_YCC_709, "709" }, > 2 definitions that share the same name? > All in all, I think the enum strings need to be more descriptive and self-consistent. +1 > > + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ > > + { COLORIMETRY_S_YCC_601, "s601" }, > > + /* Colorimetry based on IEC 61966-2-5 [33] */ > > + { COLORIMETRY_ADOBE_YCC_601, "adobe601" }, Hans (cc:d) recetly noted that these things aren't called Adobe<something> anymore in the CTA-861 spec. There is some new name for them, which I now forget. > > + /* Colorimetry based on IEC 61966-2-5 */ > > + { COLORIMETRY_ADOBE_RGB, "adobe_rgb" }, > > + /* Colorimetry based on ITU-R BT.2020 */ > > + { COLORIMETRY_BT2020_RGB, "BT2020_rgb" }, > > + /* Colorimetry based on ITU-R BT.2020 */ > > + { COLORIMETRY_BT2020_YCC, "BT2020_ycc" }, > > + /* Colorimetry based on ITU-R BT.2020 */ > > + { COLORIMETRY_BT2020_CYCC, "BT2020_cycc" }, > > + /* DP MSA Colorimetry */ > > + { COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" }, > > + { COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" }, > > + { COLORIMETRY_SRGB, "SRGB" }, > sRGB > > + { COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" }, > > + { COLORIMETRY_SCRGB, "SCRGB" }, > scRGB? > > + { COLORIMETRY_DCI_P3, "DCIP3" }, > DCI-P3? > > + { COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" }, > ^Typo > > + /* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */ > > + { COLORIMETRY_DEFAULT, "Default: ITU_709" }, > This enum together with the code below is broken. > > + COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, > > I would just make it COLORIMETRY_UNSET = "Unset". "Unset" might work. Though feels a bit strange to me. Other ideas would be "Default", "Automatic", "Undefined" or something along those lines. Ideally it should convey the meaning of "the driver will pick this for you", and for that I'd lean towards "Default" or "Automatic". > > To set the default colorimetry for all drivers, just make the default value 0 (preferred), > or add it to __drm_atomic_helper_connector_reset(). > > > +}; > > + > > /** > > * DOC: standard connector properties > > * > > @@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > > * can also expose this property to external outputs, in which case they > > * must support "None", which should be the default (since external screens > > * have a built-in scaler). > > + * > > + * colorspace: > > + * This property helps select a suitable colorspace based on the sink > > + * capability. Modern sink devices support wider gamut like BT2020. > > + * This helps switch to BT2020 mode if the BT2020 encoded video stream > > + * is being played by the user, same for any other colorspace. > > */ > > > > int drm_connector_create_standard_properties(struct drm_device *dev) > > @@ -1020,6 +1058,12 @@ int drm_connector_create_standard_properties(struct drm_device *dev) > > return -ENOMEM; > > dev->mode_config.non_desktop_property = prop; > > > > + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace", > > + colorspace, ARRAY_SIZE(colorspace)); > > + if (!prop) > > + return -ENOMEM; > > + dev->mode_config.colorspace_property = prop; > > + > > return 0; > > } > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > index dd0552c..b7f5373 100644 > > --- a/include/drm/drm_connector.h > > +++ b/include/drm/drm_connector.h > > @@ -497,6 +497,13 @@ struct drm_connector_state { > > unsigned int content_protection; > > > > /** > > + * @colorspace: Connector property to request colorspace > > + * change. This is most commonly used to switch to wider color > > + * gamuts like BT2020. > > + */ > > + enum encoder_colorimetry colorspace; > > + > > + /** > > * @writeback_job: Writeback job for writeback connectors > > * > > * Holds the framebuffer and out-fence for a writeback connector. As > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > > index d643d26..a6eb0aa 100644 > > --- a/include/drm/drm_mode_config.h > > +++ b/include/drm/drm_mode_config.h > > @@ -863,6 +863,12 @@ struct drm_mode_config { > > uint32_t cursor_width, cursor_height; > > > > /** > > + * @colorspace_property: Connector property to set the suitable > > + * colorspace supported by the sink. > > + */ > > + struct drm_property *colorspace_property; > > + > > + /** > > * @suspend_state: > > * > > * Atomic state when suspended. > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > > index d3e0fe3..831cc77 100644 > > --- a/include/uapi/drm/drm_mode.h > > +++ b/include/uapi/drm/drm_mode.h > > @@ -210,6 +210,30 @@ > > #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 > > #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 > > > > +enum encoder_colorimetry { > > + /* CEA 861 Normal Colorimetry options */ > > + COLORIMETRY_ITU_601 = 0, > > + COLORIMETRY_ITU_709, > > + /* CEA 861 Extended Colorimetry Options */ > > + COLORIMETRY_XV_YCC_601, > > + COLORIMETRY_XV_YCC_709, > > + COLORIMETRY_S_YCC_601, > > + COLORIMETRY_ADOBE_YCC_601, > > + COLORIMETRY_ADOBE_RGB, > > + COLORIMETRY_BT2020_RGB, > > + COLORIMETRY_BT2020_YCC, > > + COLORIMETRY_BT2020_CYCC, > > + /* DP MSA Colorimetry Options */ > > + COLORIMETRY_Y_CBCR_ITU_601, > > + COLORIMETRY_Y_CBCR_ITU_709, > > + COLORIMETRY_SRGB, > > + COLORIMETRY_RGB_WIDE_GAMUT, > > + COLORIMETRY_SCRGB, > > + COLORIMETRY_DCI_P3, > > + COLORIMETRY_CUSTOM_COLOR_PROFILE, > > + COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, > > +}; > > + > > struct drm_mode_modeinfo { > > __u32 clock; > > __u16 hdisplay; > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Friday, November 2, 2018 5:00 PM >To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >Cc: Shankar, Uma <uma.shankar@intel.com>; dri-devel@lists.freedesktop.org; >intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; >Lankhorst, Maarten <maarten.lankhorst@intel.com>; Hans Verkuil ><hansverk@cisco.com> >Subject: Re: [Intel-gfx] [v2 1/2] drm: Add colorspace property > >On Fri, Nov 02, 2018 at 10:19:10AM +0100, Maarten Lankhorst wrote: >> Op 31-10-18 om 13:05 schreef Uma Shankar: >> > This patch adds a colorspace property enabling userspace to switch >> > to various supported colorspaces. >> > This will help enable BT2020 along with other colorspaces. >> > >> > v2: Addressed Maarten and Ville's review comments. Enhanced the >> > colorspace enum to incorporate both HDMI and DP supported >> > colorspaces. Also, added a default option for colorspace. >> > >> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> >> > --- >> > drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ >> > drivers/gpu/drm/drm_connector.c | 44 >+++++++++++++++++++++++++++++++++++++++ >> > include/drm/drm_connector.h | 7 +++++++ >> > include/drm/drm_mode_config.h | 6 ++++++ >> > include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++++ >> > 5 files changed, 85 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c >> > b/drivers/gpu/drm/drm_atomic_uapi.c >> > index d5b7f31..9e1d46b 100644 >> > --- a/drivers/gpu/drm/drm_atomic_uapi.c >> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c >> > @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct >drm_connector *connector, >> > state->picture_aspect_ratio = val; >> > } else if (property == config->content_type_property) { >> > state->content_type = val; >> > + } else if (property == config->colorspace_property) { >> > + state->colorspace = val; >> > } else if (property == connector->scaling_mode_property) { >> > state->scaling_mode = val; >> > } else if (property == connector->content_protection_property) { >> > @@ -795,6 +797,8 @@ static int drm_atomic_connector_set_property(struct >drm_connector *connector, >> > *val = state->picture_aspect_ratio; >> > } else if (property == config->content_type_property) { >> > *val = state->content_type; >> > + } else if (property == config->colorspace_property) { >> > + *val = state->colorspace; >> > } else if (property == connector->scaling_mode_property) { >> > *val = state->scaling_mode; >> > } else if (property == connector->content_protection_property) { >> > diff --git a/drivers/gpu/drm/drm_connector.c >> > b/drivers/gpu/drm/drm_connector.c index aa18b1d..5ad52a0 100644 >> > --- a/drivers/gpu/drm/drm_connector.c >> > +++ b/drivers/gpu/drm/drm_connector.c >> > @@ -826,6 +826,38 @@ int drm_display_info_set_bus_formats(struct >> > drm_display_info *info, }; >> > DRM_ENUM_NAME_FN(drm_get_content_protection_name, >drm_cp_enum_list) >> > >> > +static const struct drm_prop_enum_list colorspace[] = { >> > + /* Standard Definition Colorimetry based on CEA 861 */ >> > + { COLORIMETRY_ITU_601, "601" }, >> > + { COLORIMETRY_ITU_709, "709" }, >> > + /* Standard Definition Colorimetry based on IEC 61966-2-4 */ >> > + { COLORIMETRY_XV_YCC_601, "601" }, >> > + /* High Definition Colorimetry based on IEC 61966-2-4 */ >> > + { COLORIMETRY_XV_YCC_709, "709" }, >> 2 definitions that share the same name? >> All in all, I think the enum strings need to be more descriptive and self- >consistent. >+1 Sure, will modify this. >> > + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ >> > + { COLORIMETRY_S_YCC_601, "s601" }, >> > + /* Colorimetry based on IEC 61966-2-5 [33] */ >> > + { COLORIMETRY_ADOBE_YCC_601, "adobe601" }, > >Hans (cc:d) recetly noted that these things aren't called Adobe<something> >anymore in the CTA-861 spec. There is some new name for them, which I now >forget. EC2 EC1 EC0 Extended Colorimetry 0 1 1 AdobeYCC601 This is the bit format mentioned in CEA861.F while defining the AVI infoframe, so looks like they are still keeping it that way. Not sure if its updated as part of any latest spec update. >> > + /* Colorimetry based on IEC 61966-2-5 */ >> > + { COLORIMETRY_ADOBE_RGB, "adobe_rgb" }, >> > + /* Colorimetry based on ITU-R BT.2020 */ >> > + { COLORIMETRY_BT2020_RGB, "BT2020_rgb" }, >> > + /* Colorimetry based on ITU-R BT.2020 */ >> > + { COLORIMETRY_BT2020_YCC, "BT2020_ycc" }, >> > + /* Colorimetry based on ITU-R BT.2020 */ >> > + { COLORIMETRY_BT2020_CYCC, "BT2020_cycc" }, >> > + /* DP MSA Colorimetry */ >> > + { COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" }, >> > + { COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" }, >> > + { COLORIMETRY_SRGB, "SRGB" }, >> sRGB Was trying to avoid camelcase, but for these names, I guess we can keep how spec defines them. Will change this. >> > + { COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" }, >> > + { COLORIMETRY_SCRGB, "SCRGB" }, >> scRGB? Will update this. >> > + { COLORIMETRY_DCI_P3, "DCIP3" }, >> DCI-P3? Ok, will update >> > + { COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" }, >> ^Typo Yeah, will rectify this. >> > + /* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */ >> > + { COLORIMETRY_DEFAULT, "Default: ITU_709" }, >> This enum together with the code below is broken. >> >> + COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, >> >> I would just make it COLORIMETRY_UNSET = "Unset". > >"Unset" might work. Though feels a bit strange to me. Other ideas would be >"Default", "Automatic", "Undefined" or something along those lines. >Ideally it should convey the meaning of "the driver will pick this for you", and for >that I'd lean towards "Default" or "Automatic". Ok, will change this to Default. >> >> To set the default colorimetry for all drivers, just make the default >> value 0 (preferred), or add it to __drm_atomic_helper_connector_reset(). Ok, will modify this and resend the next version. Thanks Ville and Maarten for your review and suggestions. Regards, Uma Shankar >> > +}; >> > + >> > /** >> > * DOC: standard connector properties >> > * >> > @@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct >drm_display_info *info, >> > * can also expose this property to external outputs, in which case they >> > * must support "None", which should be the default (since external screens >> > * have a built-in scaler). >> > + * >> > + * colorspace: >> > + * This property helps select a suitable colorspace based on the sink >> > + * capability. Modern sink devices support wider gamut like BT2020. >> > + * This helps switch to BT2020 mode if the BT2020 encoded video stream >> > + * is being played by the user, same for any other colorspace. >> > */ >> > >> > int drm_connector_create_standard_properties(struct drm_device >> > *dev) @@ -1020,6 +1058,12 @@ int >drm_connector_create_standard_properties(struct drm_device *dev) >> > return -ENOMEM; >> > dev->mode_config.non_desktop_property = prop; >> > >> > + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, >"Colorspace", >> > + colorspace, ARRAY_SIZE(colorspace)); >> > + if (!prop) >> > + return -ENOMEM; >> > + dev->mode_config.colorspace_property = prop; >> > + >> > return 0; >> > } >> > >> > diff --git a/include/drm/drm_connector.h >> > b/include/drm/drm_connector.h index dd0552c..b7f5373 100644 >> > --- a/include/drm/drm_connector.h >> > +++ b/include/drm/drm_connector.h >> > @@ -497,6 +497,13 @@ struct drm_connector_state { >> > unsigned int content_protection; >> > >> > /** >> > + * @colorspace: Connector property to request colorspace >> > + * change. This is most commonly used to switch to wider color >> > + * gamuts like BT2020. >> > + */ >> > + enum encoder_colorimetry colorspace; >> > + >> > + /** >> > * @writeback_job: Writeback job for writeback connectors >> > * >> > * Holds the framebuffer and out-fence for a writeback connector. >> > As diff --git a/include/drm/drm_mode_config.h >> > b/include/drm/drm_mode_config.h index d643d26..a6eb0aa 100644 >> > --- a/include/drm/drm_mode_config.h >> > +++ b/include/drm/drm_mode_config.h >> > @@ -863,6 +863,12 @@ struct drm_mode_config { >> > uint32_t cursor_width, cursor_height; >> > >> > /** >> > + * @colorspace_property: Connector property to set the suitable >> > + * colorspace supported by the sink. >> > + */ >> > + struct drm_property *colorspace_property; >> > + >> > + /** >> > * @suspend_state: >> > * >> > * Atomic state when suspended. >> > diff --git a/include/uapi/drm/drm_mode.h >> > b/include/uapi/drm/drm_mode.h index d3e0fe3..831cc77 100644 >> > --- a/include/uapi/drm/drm_mode.h >> > +++ b/include/uapi/drm/drm_mode.h >> > @@ -210,6 +210,30 @@ >> > #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 >> > #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 >> > >> > +enum encoder_colorimetry { >> > + /* CEA 861 Normal Colorimetry options */ >> > + COLORIMETRY_ITU_601 = 0, >> > + COLORIMETRY_ITU_709, >> > + /* CEA 861 Extended Colorimetry Options */ >> > + COLORIMETRY_XV_YCC_601, >> > + COLORIMETRY_XV_YCC_709, >> > + COLORIMETRY_S_YCC_601, >> > + COLORIMETRY_ADOBE_YCC_601, >> > + COLORIMETRY_ADOBE_RGB, >> > + COLORIMETRY_BT2020_RGB, >> > + COLORIMETRY_BT2020_YCC, >> > + COLORIMETRY_BT2020_CYCC, >> > + /* DP MSA Colorimetry Options */ >> > + COLORIMETRY_Y_CBCR_ITU_601, >> > + COLORIMETRY_Y_CBCR_ITU_709, >> > + COLORIMETRY_SRGB, >> > + COLORIMETRY_RGB_WIDE_GAMUT, >> > + COLORIMETRY_SCRGB, >> > + COLORIMETRY_DCI_P3, >> > + COLORIMETRY_CUSTOM_COLOR_PROFILE, >> > + COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, }; >> > + >> > struct drm_mode_modeinfo { >> > __u32 clock; >> > __u16 hdisplay; >> >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >-- >Ville Syrjälä >Intel
On 2018-11-02 15:13, Shankar, Uma wrote: > >> -----Original Message----- >> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >> Sent: Friday, November 2, 2018 5:00 PM >> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Shankar, Uma <uma.shankar@intel.com>; dri-devel@lists.freedesktop.org; >> intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; >> Lankhorst, Maarten <maarten.lankhorst@intel.com>; Hans Verkuil >> <hansverk@cisco.com> >> Subject: Re: [Intel-gfx] [v2 1/2] drm: Add colorspace property >> >> On Fri, Nov 02, 2018 at 10:19:10AM +0100, Maarten Lankhorst wrote: >>> Op 31-10-18 om 13:05 schreef Uma Shankar: >>>> This patch adds a colorspace property enabling userspace to switch >>>> to various supported colorspaces. >>>> This will help enable BT2020 along with other colorspaces. >>>> >>>> v2: Addressed Maarten and Ville's review comments. Enhanced the >>>> colorspace enum to incorporate both HDMI and DP supported >>>> colorspaces. Also, added a default option for colorspace. >>>> >>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com> >>>> --- >>>> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ >>>> drivers/gpu/drm/drm_connector.c | 44 >> +++++++++++++++++++++++++++++++++++++++ >>>> include/drm/drm_connector.h | 7 +++++++ >>>> include/drm/drm_mode_config.h | 6 ++++++ >>>> include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++++ >>>> 5 files changed, 85 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c >>>> b/drivers/gpu/drm/drm_atomic_uapi.c >>>> index d5b7f31..9e1d46b 100644 >>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c >>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >>>> @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct >> drm_connector *connector, >>>> state->picture_aspect_ratio = val; >>>> } else if (property == config->content_type_property) { >>>> state->content_type = val; >>>> + } else if (property == config->colorspace_property) { >>>> + state->colorspace = val; >>>> } else if (property == connector->scaling_mode_property) { >>>> state->scaling_mode = val; >>>> } else if (property == connector->content_protection_property) { >>>> @@ -795,6 +797,8 @@ static int drm_atomic_connector_set_property(struct >> drm_connector *connector, >>>> *val = state->picture_aspect_ratio; >>>> } else if (property == config->content_type_property) { >>>> *val = state->content_type; >>>> + } else if (property == config->colorspace_property) { >>>> + *val = state->colorspace; >>>> } else if (property == connector->scaling_mode_property) { >>>> *val = state->scaling_mode; >>>> } else if (property == connector->content_protection_property) { >>>> diff --git a/drivers/gpu/drm/drm_connector.c >>>> b/drivers/gpu/drm/drm_connector.c index aa18b1d..5ad52a0 100644 >>>> --- a/drivers/gpu/drm/drm_connector.c >>>> +++ b/drivers/gpu/drm/drm_connector.c >>>> @@ -826,6 +826,38 @@ int drm_display_info_set_bus_formats(struct >>>> drm_display_info *info, }; >>>> DRM_ENUM_NAME_FN(drm_get_content_protection_name, >> drm_cp_enum_list) >>>> +static const struct drm_prop_enum_list colorspace[] = { >>>> + /* Standard Definition Colorimetry based on CEA 861 */ >>>> + { COLORIMETRY_ITU_601, "601" }, >>>> + { COLORIMETRY_ITU_709, "709" }, >>>> + /* Standard Definition Colorimetry based on IEC 61966-2-4 */ >>>> + { COLORIMETRY_XV_YCC_601, "601" }, >>>> + /* High Definition Colorimetry based on IEC 61966-2-4 */ >>>> + { COLORIMETRY_XV_YCC_709, "709" }, >>> 2 definitions that share the same name? >>> All in all, I think the enum strings need to be more descriptive and self- >> consistent. >> +1 > Sure, will modify this. > >>>> + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ >>>> + { COLORIMETRY_S_YCC_601, "s601" }, >>>> + /* Colorimetry based on IEC 61966-2-5 [33] */ >>>> + { COLORIMETRY_ADOBE_YCC_601, "adobe601" }, >> Hans (cc:d) recetly noted that these things aren't called Adobe<something> >> anymore in the CTA-861 spec. There is some new name for them, which I now >> forget. > EC2 EC1 EC0 Extended Colorimetry > 0 1 1 AdobeYCC601 > This is the bit format mentioned in CEA861.F while defining the AVI infoframe, so looks > like they are still keeping it that way. Not sure if its updated as part of any latest spec > update. New names is opRGB and opYCC601 according to the notice on the first page of CTA-861-G [1] Updated CTA-861-E/F/G can be found at https://standards.cta.tech/kwspub/published_docs/ [1] https://standards.cta.tech/kwspub/published_docs/CTA-861-G_FINAL_revised_2017.pdf > >>>> + /* Colorimetry based on IEC 61966-2-5 */ >>>> + { COLORIMETRY_ADOBE_RGB, "adobe_rgb" }, >>>> + /* Colorimetry based on ITU-R BT.2020 */ >>>> + { COLORIMETRY_BT2020_RGB, "BT2020_rgb" }, >>>> + /* Colorimetry based on ITU-R BT.2020 */ >>>> + { COLORIMETRY_BT2020_YCC, "BT2020_ycc" }, >>>> + /* Colorimetry based on ITU-R BT.2020 */ >>>> + { COLORIMETRY_BT2020_CYCC, "BT2020_cycc" }, >>>> + /* DP MSA Colorimetry */ >>>> + { COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" }, >>>> + { COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" }, >>>> + { COLORIMETRY_SRGB, "SRGB" }, >>> sRGB > Was trying to avoid camelcase, but for these names, I guess we can keep how > spec defines them. Will change this. > >>>> + { COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" }, >>>> + { COLORIMETRY_SCRGB, "SCRGB" }, >>> scRGB? > Will update this. > >>>> + { COLORIMETRY_DCI_P3, "DCIP3" }, >>> DCI-P3? > Ok, will update > >>>> + { COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" }, >>> ^Typo > Yeah, will rectify this. > >>>> + /* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */ >>>> + { COLORIMETRY_DEFAULT, "Default: ITU_709" }, >>> This enum together with the code below is broken. >>> >>> + COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, >>> >>> I would just make it COLORIMETRY_UNSET = "Unset". >> "Unset" might work. Though feels a bit strange to me. Other ideas would be >> "Default", "Automatic", "Undefined" or something along those lines. >> Ideally it should convey the meaning of "the driver will pick this for you", and for >> that I'd lean towards "Default" or "Automatic". > Ok, will change this to Default. > >>> To set the default colorimetry for all drivers, just make the default >>> value 0 (preferred), or add it to __drm_atomic_helper_connector_reset(). > Ok, will modify this and resend the next version. > > Thanks Ville and Maarten for your review and suggestions. > > Regards, > Uma Shankar >>>> +}; >>>> + >>>> /** >>>> * DOC: standard connector properties >>>> * >>>> @@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct >> drm_display_info *info, >>>> * can also expose this property to external outputs, in which case they >>>> * must support "None", which should be the default (since external screens >>>> * have a built-in scaler). >>>> + * >>>> + * colorspace: >>>> + * This property helps select a suitable colorspace based on the sink >>>> + * capability. Modern sink devices support wider gamut like BT2020. >>>> + * This helps switch to BT2020 mode if the BT2020 encoded video stream >>>> + * is being played by the user, same for any other colorspace. >>>> */ >>>> >>>> int drm_connector_create_standard_properties(struct drm_device >>>> *dev) @@ -1020,6 +1058,12 @@ int >> drm_connector_create_standard_properties(struct drm_device *dev) >>>> return -ENOMEM; >>>> dev->mode_config.non_desktop_property = prop; >>>> >>>> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, >> "Colorspace", >>>> + colorspace, ARRAY_SIZE(colorspace)); >>>> + if (!prop) >>>> + return -ENOMEM; >>>> + dev->mode_config.colorspace_property = prop; >>>> + >>>> return 0; >>>> } >>>> >>>> diff --git a/include/drm/drm_connector.h >>>> b/include/drm/drm_connector.h index dd0552c..b7f5373 100644 >>>> --- a/include/drm/drm_connector.h >>>> +++ b/include/drm/drm_connector.h >>>> @@ -497,6 +497,13 @@ struct drm_connector_state { >>>> unsigned int content_protection; >>>> >>>> /** >>>> + * @colorspace: Connector property to request colorspace >>>> + * change. This is most commonly used to switch to wider color >>>> + * gamuts like BT2020. >>>> + */ >>>> + enum encoder_colorimetry colorspace; >>>> + >>>> + /** >>>> * @writeback_job: Writeback job for writeback connectors >>>> * >>>> * Holds the framebuffer and out-fence for a writeback connector. >>>> As diff --git a/include/drm/drm_mode_config.h >>>> b/include/drm/drm_mode_config.h index d643d26..a6eb0aa 100644 >>>> --- a/include/drm/drm_mode_config.h >>>> +++ b/include/drm/drm_mode_config.h >>>> @@ -863,6 +863,12 @@ struct drm_mode_config { >>>> uint32_t cursor_width, cursor_height; >>>> >>>> /** >>>> + * @colorspace_property: Connector property to set the suitable >>>> + * colorspace supported by the sink. >>>> + */ >>>> + struct drm_property *colorspace_property; >>>> + >>>> + /** >>>> * @suspend_state: >>>> * >>>> * Atomic state when suspended. >>>> diff --git a/include/uapi/drm/drm_mode.h >>>> b/include/uapi/drm/drm_mode.h index d3e0fe3..831cc77 100644 >>>> --- a/include/uapi/drm/drm_mode.h >>>> +++ b/include/uapi/drm/drm_mode.h >>>> @@ -210,6 +210,30 @@ >>>> #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 >>>> #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 >>>> >>>> +enum encoder_colorimetry { >>>> + /* CEA 861 Normal Colorimetry options */ >>>> + COLORIMETRY_ITU_601 = 0, >>>> + COLORIMETRY_ITU_709, >>>> + /* CEA 861 Extended Colorimetry Options */ >>>> + COLORIMETRY_XV_YCC_601, >>>> + COLORIMETRY_XV_YCC_709, >>>> + COLORIMETRY_S_YCC_601, >>>> + COLORIMETRY_ADOBE_YCC_601, >>>> + COLORIMETRY_ADOBE_RGB, >>>> + COLORIMETRY_BT2020_RGB, >>>> + COLORIMETRY_BT2020_YCC, >>>> + COLORIMETRY_BT2020_CYCC, >>>> + /* DP MSA Colorimetry Options */ >>>> + COLORIMETRY_Y_CBCR_ITU_601, >>>> + COLORIMETRY_Y_CBCR_ITU_709, >>>> + COLORIMETRY_SRGB, >>>> + COLORIMETRY_RGB_WIDE_GAMUT, >>>> + COLORIMETRY_SCRGB, >>>> + COLORIMETRY_DCI_P3, >>>> + COLORIMETRY_CUSTOM_COLOR_PROFILE, >>>> + COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, }; >>>> + >>>> struct drm_mode_modeinfo { >>>> __u32 clock; >>>> __u16 hdisplay; >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7C%7C2e75af29da3342d27bac08d640cd62cd%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636767648196557839&sdata=V2VlqgzetFVCtiXJE8yt%2Fl2uPhC8M7498peHNubR4Fw%3D&reserved=0 >> -- >> Ville Syrjälä >> Intel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7C%7C2e75af29da3342d27bac08d640cd62cd%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636767648196557839&sdata=V2VlqgzetFVCtiXJE8yt%2Fl2uPhC8M7498peHNubR4Fw%3D&reserved=0
Regards Shashank On 10/31/2018 5:35 PM, Uma Shankar wrote: > This patch adds a colorspace property enabling > userspace to switch to various supported colorspaces. > This will help enable BT2020 along with other colorspaces. > > v2: Addressed Maarten and Ville's review comments. Enhanced > the colorspace enum to incorporate both HDMI and DP supported > colorspaces. Also, added a default option for colorspace. > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > --- > drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ > drivers/gpu/drm/drm_connector.c | 44 +++++++++++++++++++++++++++++++++++++++ > include/drm/drm_connector.h | 7 +++++++ > include/drm/drm_mode_config.h | 6 ++++++ > include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++++ > 5 files changed, 85 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index d5b7f31..9e1d46b 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > state->picture_aspect_ratio = val; > } else if (property == config->content_type_property) { > state->content_type = val; > + } else if (property == config->colorspace_property) { > + state->colorspace = val; > } else if (property == connector->scaling_mode_property) { > state->scaling_mode = val; > } else if (property == connector->content_protection_property) { > @@ -795,6 +797,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > *val = state->picture_aspect_ratio; > } else if (property == config->content_type_property) { > *val = state->content_type; > + } else if (property == config->colorspace_property) { > + *val = state->colorspace; > } else if (property == connector->scaling_mode_property) { > *val = state->scaling_mode; > } else if (property == connector->content_protection_property) { > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index aa18b1d..5ad52a0 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -826,6 +826,38 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > }; > DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list) > > +static const struct drm_prop_enum_list colorspace[] = { > + /* Standard Definition Colorimetry based on CEA 861 */ > + { COLORIMETRY_ITU_601, "601" }, > + { COLORIMETRY_ITU_709, "709" }, > + /* Standard Definition Colorimetry based on IEC 61966-2-4 */ > + { COLORIMETRY_XV_YCC_601, "601" }, > + /* High Definition Colorimetry based on IEC 61966-2-4 */ > + { COLORIMETRY_XV_YCC_709, "709" }, > + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ > + { COLORIMETRY_S_YCC_601, "s601" }, > + /* Colorimetry based on IEC 61966-2-5 [33] */ > + { COLORIMETRY_ADOBE_YCC_601, "adobe601" }, > + /* Colorimetry based on IEC 61966-2-5 */ > + { COLORIMETRY_ADOBE_RGB, "adobe_rgb" }, > + /* Colorimetry based on ITU-R BT.2020 */ > + { COLORIMETRY_BT2020_RGB, "BT2020_rgb" }, > + /* Colorimetry based on ITU-R BT.2020 */ > + { COLORIMETRY_BT2020_YCC, "BT2020_ycc" }, > + /* Colorimetry based on ITU-R BT.2020 */ > + { COLORIMETRY_BT2020_CYCC, "BT2020_cycc" }, > + /* DP MSA Colorimetry */ > + { COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" }, > + { COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" }, > + { COLORIMETRY_SRGB, "SRGB" }, > + { COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" }, > + { COLORIMETRY_SCRGB, "SCRGB" }, > + { COLORIMETRY_DCI_P3, "DCIP3" }, > + { COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" }, > + /* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */ > + { COLORIMETRY_DEFAULT, "Default: ITU_709" }, > +}; > + > /** > * DOC: standard connector properties > * > @@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > * can also expose this property to external outputs, in which case they > * must support "None", which should be the default (since external screens > * have a built-in scaler). > + * > + * colorspace: > + * This property helps select a suitable colorspace based on the sink > + * capability. Modern sink devices support wider gamut like BT2020. > + * This helps switch to BT2020 mode if the BT2020 encoded video stream > + * is being played by the user, same for any other colorspace. As commented on the previous patch, we might have to consider the HW capability while doing this. > */ > > int drm_connector_create_standard_properties(struct drm_device *dev) > @@ -1020,6 +1058,12 @@ int drm_connector_create_standard_properties(struct drm_device *dev) > return -ENOMEM; > dev->mode_config.non_desktop_property = prop; > > + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace", > + colorspace, ARRAY_SIZE(colorspace)); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.colorspace_property = prop; > + > return 0; > } > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index dd0552c..b7f5373 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -497,6 +497,13 @@ struct drm_connector_state { > unsigned int content_protection; > > /** > + * @colorspace: Connector property to request colorspace > + * change. This is most commonly used to switch to wider color > + * gamuts like BT2020. > + */ > + enum encoder_colorimetry colorspace; A connector state property with enum name encoder_colorimetry is a bit confusing, but I might be nitpicking ;-) > > + > + /** > * @writeback_job: Writeback job for writeback connectors > * > * Holds the framebuffer and out-fence for a writeback connector. As > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index d643d26..a6eb0aa 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -863,6 +863,12 @@ struct drm_mode_config { > uint32_t cursor_width, cursor_height; > > /** > + * @colorspace_property: Connector property to set the suitable > + * colorspace supported by the sink. > + */ > + struct drm_property *colorspace_property; > + > + /** > * @suspend_state: > * > * Atomic state when suspended. > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index d3e0fe3..831cc77 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -210,6 +210,30 @@ > #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 > #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 > > +enum encoder_colorimetry { > + /* CEA 861 Normal Colorimetry options */ > + COLORIMETRY_ITU_601 = 0, > + COLORIMETRY_ITU_709, > + /* CEA 861 Extended Colorimetry Options */ > + COLORIMETRY_XV_YCC_601, > + COLORIMETRY_XV_YCC_709, > + COLORIMETRY_S_YCC_601, > + COLORIMETRY_ADOBE_YCC_601, > + COLORIMETRY_ADOBE_RGB, > + COLORIMETRY_BT2020_RGB, > + COLORIMETRY_BT2020_YCC, > + COLORIMETRY_BT2020_CYCC, > + /* DP MSA Colorimetry Options */ > + COLORIMETRY_Y_CBCR_ITU_601, > + COLORIMETRY_Y_CBCR_ITU_709, > + COLORIMETRY_SRGB, > + COLORIMETRY_RGB_WIDE_GAMUT, > + COLORIMETRY_SCRGB, > + COLORIMETRY_DCI_P3, > + COLORIMETRY_CUSTOM_COLOR_PROFILE, > + COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, > +}; > + > struct drm_mode_modeinfo { > __u32 clock; > __u16 hdisplay; - Shashank
On 11/02/2018 03:29 PM, Jonas Karlman wrote: > On 2018-11-02 15:13, Shankar, Uma wrote: >> >>> -----Original Message----- >>> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >>> Sent: Friday, November 2, 2018 5:00 PM >>> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Cc: Shankar, Uma <uma.shankar@intel.com>; dri-devel@lists.freedesktop.org; >>> intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; >>> Lankhorst, Maarten <maarten.lankhorst@intel.com>; Hans Verkuil >>> <hansverk@cisco.com> >>> Subject: Re: [Intel-gfx] [v2 1/2] drm: Add colorspace property >>> >>> On Fri, Nov 02, 2018 at 10:19:10AM +0100, Maarten Lankhorst wrote: >>>> Op 31-10-18 om 13:05 schreef Uma Shankar: >>>>> This patch adds a colorspace property enabling userspace to switch >>>>> to various supported colorspaces. >>>>> This will help enable BT2020 along with other colorspaces. >>>>> >>>>> v2: Addressed Maarten and Ville's review comments. Enhanced the >>>>> colorspace enum to incorporate both HDMI and DP supported >>>>> colorspaces. Also, added a default option for colorspace. >>>>> >>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com> >>>>> --- >>>>> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ >>>>> drivers/gpu/drm/drm_connector.c | 44 >>> +++++++++++++++++++++++++++++++++++++++ >>>>> include/drm/drm_connector.h | 7 +++++++ >>>>> include/drm/drm_mode_config.h | 6 ++++++ >>>>> include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++++ >>>>> 5 files changed, 85 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c >>>>> b/drivers/gpu/drm/drm_atomic_uapi.c >>>>> index d5b7f31..9e1d46b 100644 >>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c >>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >>>>> @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct >>> drm_connector *connector, >>>>> state->picture_aspect_ratio = val; >>>>> } else if (property == config->content_type_property) { >>>>> state->content_type = val; >>>>> + } else if (property == config->colorspace_property) { >>>>> + state->colorspace = val; >>>>> } else if (property == connector->scaling_mode_property) { >>>>> state->scaling_mode = val; >>>>> } else if (property == connector->content_protection_property) { >>>>> @@ -795,6 +797,8 @@ static int drm_atomic_connector_set_property(struct >>> drm_connector *connector, >>>>> *val = state->picture_aspect_ratio; >>>>> } else if (property == config->content_type_property) { >>>>> *val = state->content_type; >>>>> + } else if (property == config->colorspace_property) { >>>>> + *val = state->colorspace; >>>>> } else if (property == connector->scaling_mode_property) { >>>>> *val = state->scaling_mode; >>>>> } else if (property == connector->content_protection_property) { >>>>> diff --git a/drivers/gpu/drm/drm_connector.c >>>>> b/drivers/gpu/drm/drm_connector.c index aa18b1d..5ad52a0 100644 >>>>> --- a/drivers/gpu/drm/drm_connector.c >>>>> +++ b/drivers/gpu/drm/drm_connector.c >>>>> @@ -826,6 +826,38 @@ int drm_display_info_set_bus_formats(struct >>>>> drm_display_info *info, }; >>>>> DRM_ENUM_NAME_FN(drm_get_content_protection_name, >>> drm_cp_enum_list) >>>>> +static const struct drm_prop_enum_list colorspace[] = { >>>>> + /* Standard Definition Colorimetry based on CEA 861 */ >>>>> + { COLORIMETRY_ITU_601, "601" }, >>>>> + { COLORIMETRY_ITU_709, "709" }, >>>>> + /* Standard Definition Colorimetry based on IEC 61966-2-4 */ >>>>> + { COLORIMETRY_XV_YCC_601, "601" }, >>>>> + /* High Definition Colorimetry based on IEC 61966-2-4 */ >>>>> + { COLORIMETRY_XV_YCC_709, "709" }, >>>> 2 definitions that share the same name? >>>> All in all, I think the enum strings need to be more descriptive and self- >>> consistent. >>> +1 >> Sure, will modify this. >> >>>>> + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ >>>>> + { COLORIMETRY_S_YCC_601, "s601" }, >>>>> + /* Colorimetry based on IEC 61966-2-5 [33] */ >>>>> + { COLORIMETRY_ADOBE_YCC_601, "adobe601" }, >>> Hans (cc:d) recetly noted that these things aren't called Adobe<something> >>> anymore in the CTA-861 spec. There is some new name for them, which I now >>> forget. >> EC2 EC1 EC0 Extended Colorimetry >> 0 1 1 AdobeYCC601 >> This is the bit format mentioned in CEA861.F while defining the AVI infoframe, so looks >> like they are still keeping it that way. Not sure if its updated as part of any latest spec >> update. > > New names is opRGB and opYCC601 according to the notice on the first page of CTA-861-G [1] > > Updated CTA-861-E/F/G can be found at https://standards.cta.tech/kwspub/published_docs/ > > [1] https://standards.cta.tech/kwspub/published_docs/CTA-861-G_FINAL_revised_2017.pdf Correct. The names were updated since Adobe complained to the CTA about trademark issues. And in all fairness, it makes sense to refer to the official international standard name instead of a company standard, even though they are effectively identical. Basically, just avoid the name 'Adobe' :-) Regards, Hans
Hi Uma, On Wed, Oct 31, 2018 at 05:35:45PM +0530, Uma Shankar wrote: >This patch adds a colorspace property enabling >userspace to switch to various supported colorspaces. >This will help enable BT2020 along with other colorspaces. > >v2: Addressed Maarten and Ville's review comments. Enhanced >the colorspace enum to incorporate both HDMI and DP supported >colorspaces. Also, added a default option for colorspace. > >Signed-off-by: Uma Shankar <uma.shankar at intel.com> >--- > drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ > drivers/gpu/drm/drm_connector.c | 44 +++++++++++++++++++++++++++++++++++++++ > include/drm/drm_connector.h | 7 +++++++ > include/drm/drm_mode_config.h | 6 ++++++ > include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++++ > 5 files changed, 85 insertions(+) > >diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c >index d5b7f31..9e1d46b 100644 >--- a/drivers/gpu/drm/drm_atomic_uapi.c >+++ b/drivers/gpu/drm/drm_atomic_uapi.c >@@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > state->picture_aspect_ratio = val; > } else if (property == config->content_type_property) { > state->content_type = val; >+ } else if (property == config->colorspace_property) { >+ state->colorspace = val; > } else if (property == connector->scaling_mode_property) { > state->scaling_mode = val; > } else if (property == connector->content_protection_property) { >@@ -795,6 +797,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > *val = state->picture_aspect_ratio; > } else if (property == config->content_type_property) { > *val = state->content_type; >+ } else if (property == config->colorspace_property) { >+ *val = state->colorspace; > } else if (property == connector->scaling_mode_property) { > *val = state->scaling_mode; > } else if (property == connector->content_protection_property) { >diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >index aa18b1d..5ad52a0 100644 >--- a/drivers/gpu/drm/drm_connector.c >+++ b/drivers/gpu/drm/drm_connector.c >@@ -826,6 +826,38 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > }; > DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list) > >+static const struct drm_prop_enum_list colorspace[] = { >+ /* Standard Definition Colorimetry based on CEA 861 */ >+ { COLORIMETRY_ITU_601, "601" }, >+ { COLORIMETRY_ITU_709, "709" }, >+ /* Standard Definition Colorimetry based on IEC 61966-2-4 */ >+ { COLORIMETRY_XV_YCC_601, "601" }, >+ /* High Definition Colorimetry based on IEC 61966-2-4 */ >+ { COLORIMETRY_XV_YCC_709, "709" }, >+ /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ >+ { COLORIMETRY_S_YCC_601, "s601" }, >+ /* Colorimetry based on IEC 61966-2-5 [33] */ >+ { COLORIMETRY_ADOBE_YCC_601, "adobe601" }, >+ /* Colorimetry based on IEC 61966-2-5 */ >+ { COLORIMETRY_ADOBE_RGB, "adobe_rgb" }, >+ /* Colorimetry based on ITU-R BT.2020 */ >+ { COLORIMETRY_BT2020_RGB, "BT2020_rgb" }, >+ /* Colorimetry based on ITU-R BT.2020 */ >+ { COLORIMETRY_BT2020_YCC, "BT2020_ycc" }, >+ /* Colorimetry based on ITU-R BT.2020 */ >+ { COLORIMETRY_BT2020_CYCC, "BT2020_cycc" }, >+ /* DP MSA Colorimetry */ >+ { COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" }, >+ { COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" }, >+ { COLORIMETRY_SRGB, "SRGB" }, >+ { COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" }, >+ { COLORIMETRY_SCRGB, "SCRGB" }, >+ { COLORIMETRY_DCI_P3, "DCIP3" }, >+ { COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" }, >+ /* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */ >+ { COLORIMETRY_DEFAULT, "Default: ITU_709" }, >+}; >+ > /** > * DOC: standard connector properties > * >@@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > * can also expose this property to external outputs, in which case they > * must support "None", which should be the default (since external screens > * have a built-in scaler). >+ * >+ * colorspace: Is it "colorspace" or "Colorspace"? The code says capital-C, the doc (and whatever little convention there is) says lower-case. >+ * This property helps select a suitable colorspace based on the sink >+ * capability. Modern sink devices support wider gamut like BT2020. >+ * This helps switch to BT2020 mode if the BT2020 encoded video stream >+ * is being played by the user, same for any other colorspace. > */ > > int drm_connector_create_standard_properties(struct drm_device *dev) >@@ -1020,6 +1058,12 @@ int drm_connector_create_standard_properties(struct drm_device *dev) > return -ENOMEM; > dev->mode_config.non_desktop_property = prop; > >+ prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace", >+ colorspace, ARRAY_SIZE(colorspace)); Similar to what others have said: I'm not sure it's a great idea to expose the full set by default. At a minimum, actually applying this property requires some control over the HDMI/DP transmitter doesn't it? And I believe they vary in capabilities? Further, I'd expect some of them to require more extensive changes further through the pipe (e.g. scRGB), though I admit I don't know much about how Sinks handle this stuff. Do you have a subset which you have a real user and use-case for which we might settle on before throwing them all in and making it UAPI? Cheers, -Brian >+ if (!prop) >+ return -ENOMEM; >+ dev->mode_config.colorspace_property = prop; >+ > return 0; > } > >diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h >index dd0552c..b7f5373 100644 >--- a/include/drm/drm_connector.h >+++ b/include/drm/drm_connector.h >@@ -497,6 +497,13 @@ struct drm_connector_state { > unsigned int content_protection; > > /** >+ * @colorspace: Connector property to request colorspace >+ * change. This is most commonly used to switch to wider color >+ * gamuts like BT2020. >+ */ >+ enum encoder_colorimetry colorspace; >+ >+ /** > * @writeback_job: Writeback job for writeback connectors > * > * Holds the framebuffer and out-fence for a writeback connector. As >diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h >index d643d26..a6eb0aa 100644 >--- a/include/drm/drm_mode_config.h >+++ b/include/drm/drm_mode_config.h >@@ -863,6 +863,12 @@ struct drm_mode_config { > uint32_t cursor_width, cursor_height; > > /** >+ * @colorspace_property: Connector property to set the suitable >+ * colorspace supported by the sink. >+ */ >+ struct drm_property *colorspace_property; >+ >+ /** > * @suspend_state: > * > * Atomic state when suspended. >diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h >index d3e0fe3..831cc77 100644 >--- a/include/uapi/drm/drm_mode.h >+++ b/include/uapi/drm/drm_mode.h >@@ -210,6 +210,30 @@ > #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 > #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 > >+enum encoder_colorimetry { >+ /* CEA 861 Normal Colorimetry options */ >+ COLORIMETRY_ITU_601 = 0, >+ COLORIMETRY_ITU_709, >+ /* CEA 861 Extended Colorimetry Options */ >+ COLORIMETRY_XV_YCC_601, >+ COLORIMETRY_XV_YCC_709, >+ COLORIMETRY_S_YCC_601, >+ COLORIMETRY_ADOBE_YCC_601, >+ COLORIMETRY_ADOBE_RGB, >+ COLORIMETRY_BT2020_RGB, >+ COLORIMETRY_BT2020_YCC, >+ COLORIMETRY_BT2020_CYCC, >+ /* DP MSA Colorimetry Options */ >+ COLORIMETRY_Y_CBCR_ITU_601, >+ COLORIMETRY_Y_CBCR_ITU_709, >+ COLORIMETRY_SRGB, >+ COLORIMETRY_RGB_WIDE_GAMUT, >+ COLORIMETRY_SCRGB, >+ COLORIMETRY_DCI_P3, >+ COLORIMETRY_CUSTOM_COLOR_PROFILE, >+ COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, >+}; >+ > struct drm_mode_modeinfo { > __u32 clock; > __u16 hdisplay; >-- >1.9.1 > >
>-----Original Message----- >From: Brian Starkey [mailto:Brian.Starkey@arm.com] >Sent: Tuesday, November 20, 2018 9:06 PM >To: Shankar, Uma <uma.shankar@intel.com>; Syrjala, Ville ><ville.syrjala@intel.com>; Lankhorst, Maarten <maarten.lankhorst@intel.com>; >emil.l.velikov@gmail.com; dri-devel@lists.freedesktop.org >Cc: nd <nd@arm.com> >Subject: Re: [v2 1/2] drm: Add colorspace property > >Hi Uma, > >On Wed, Oct 31, 2018 at 05:35:45PM +0530, Uma Shankar wrote: >>This patch adds a colorspace property enabling userspace to switch to >>various supported colorspaces. >>This will help enable BT2020 along with other colorspaces. >> >>v2: Addressed Maarten and Ville's review comments. Enhanced the >>colorspace enum to incorporate both HDMI and DP supported colorspaces. >>Also, added a default option for colorspace. >> >>Signed-off-by: Uma Shankar <uma.shankar at intel.com> >>--- >> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ >> drivers/gpu/drm/drm_connector.c | 44 >+++++++++++++++++++++++++++++++++++++++ >> include/drm/drm_connector.h | 7 +++++++ >> include/drm/drm_mode_config.h | 6 ++++++ >> include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++++ >> 5 files changed, 85 insertions(+) >> >>diff --git a/drivers/gpu/drm/drm_atomic_uapi.c >>b/drivers/gpu/drm/drm_atomic_uapi.c >>index d5b7f31..9e1d46b 100644 >>--- a/drivers/gpu/drm/drm_atomic_uapi.c >>+++ b/drivers/gpu/drm/drm_atomic_uapi.c >>@@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct >drm_connector *connector, >> state->picture_aspect_ratio = val; >> } else if (property == config->content_type_property) { >> state->content_type = val; >>+ } else if (property == config->colorspace_property) { >>+ state->colorspace = val; >> } else if (property == connector->scaling_mode_property) { >> state->scaling_mode = val; >> } else if (property == connector->content_protection_property) { @@ >>-795,6 +797,8 @@ static int drm_atomic_connector_set_property(struct >drm_connector *connector, >> *val = state->picture_aspect_ratio; >> } else if (property == config->content_type_property) { >> *val = state->content_type; >>+ } else if (property == config->colorspace_property) { >>+ *val = state->colorspace; >> } else if (property == connector->scaling_mode_property) { >> *val = state->scaling_mode; >> } else if (property == connector->content_protection_property) { diff >>--git a/drivers/gpu/drm/drm_connector.c >>b/drivers/gpu/drm/drm_connector.c index aa18b1d..5ad52a0 100644 >>--- a/drivers/gpu/drm/drm_connector.c >>+++ b/drivers/gpu/drm/drm_connector.c >>@@ -826,6 +826,38 @@ int drm_display_info_set_bus_formats(struct >>drm_display_info *info, }; >>DRM_ENUM_NAME_FN(drm_get_content_protection_name, >drm_cp_enum_list) >> >>+static const struct drm_prop_enum_list colorspace[] = { >>+ /* Standard Definition Colorimetry based on CEA 861 */ >>+ { COLORIMETRY_ITU_601, "601" }, >>+ { COLORIMETRY_ITU_709, "709" }, >>+ /* Standard Definition Colorimetry based on IEC 61966-2-4 */ >>+ { COLORIMETRY_XV_YCC_601, "601" }, >>+ /* High Definition Colorimetry based on IEC 61966-2-4 */ >>+ { COLORIMETRY_XV_YCC_709, "709" }, >>+ /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ >>+ { COLORIMETRY_S_YCC_601, "s601" }, >>+ /* Colorimetry based on IEC 61966-2-5 [33] */ >>+ { COLORIMETRY_ADOBE_YCC_601, "adobe601" }, >>+ /* Colorimetry based on IEC 61966-2-5 */ >>+ { COLORIMETRY_ADOBE_RGB, "adobe_rgb" }, >>+ /* Colorimetry based on ITU-R BT.2020 */ >>+ { COLORIMETRY_BT2020_RGB, "BT2020_rgb" }, >>+ /* Colorimetry based on ITU-R BT.2020 */ >>+ { COLORIMETRY_BT2020_YCC, "BT2020_ycc" }, >>+ /* Colorimetry based on ITU-R BT.2020 */ >>+ { COLORIMETRY_BT2020_CYCC, "BT2020_cycc" }, >>+ /* DP MSA Colorimetry */ >>+ { COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" }, >>+ { COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" }, >>+ { COLORIMETRY_SRGB, "SRGB" }, >>+ { COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" }, >>+ { COLORIMETRY_SCRGB, "SCRGB" }, >>+ { COLORIMETRY_DCI_P3, "DCIP3" }, >>+ { COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" }, >>+ /* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */ >>+ { COLORIMETRY_DEFAULT, "Default: ITU_709" }, }; >>+ >> /** >> * DOC: standard connector properties >> * >>@@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct >drm_display_info *info, >> * can also expose this property to external outputs, in which case they >> * must support "None", which should be the default (since external screens >> * have a built-in scaler). >>+ * >>+ * colorspace: > >Is it "colorspace" or "Colorspace"? The code says capital-C, the doc (and >whatever little convention there is) says lower-case. I will update this. It should be "Colorspace". >>+ * This property helps select a suitable colorspace based on the sink >>+ * capability. Modern sink devices support wider gamut like BT2020. >>+ * This helps switch to BT2020 mode if the BT2020 encoded video stream >>+ * is being played by the user, same for any other colorspace. >> */ >> >> int drm_connector_create_standard_properties(struct drm_device *dev) >>@@ -1020,6 +1058,12 @@ int >drm_connector_create_standard_properties(struct drm_device *dev) >> return -ENOMEM; >> dev->mode_config.non_desktop_property = prop; >> >>+ prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, >"Colorspace", >>+ colorspace, ARRAY_SIZE(colorspace)); > >Similar to what others have said: I'm not sure it's a great idea to expose the full >set by default. At a minimum, actually applying this property requires some >control over the HDMI/DP transmitter doesn't it? And I believe they vary in >capabilities? > >Further, I'd expect some of them to require more extensive changes further >through the pipe (e.g. scRGB), though I admit I don't know much about how Sinks >handle this stuff. I am planning to separate out HDMI and DP as separate properties (same name for userspace), but with connector specific colorspaces in the list. Will float the v4 soon, please have a look and share your feedback. Regards, Uma Shankar >Do you have a subset which you have a real user and use-case for which we might >settle on before throwing them all in and making it UAPI? > >Cheers, >-Brian > > >>+ if (!prop) >>+ return -ENOMEM; >>+ dev->mode_config.colorspace_property = prop; >>+ >> return 0; >> } >> >>diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h >>index dd0552c..b7f5373 100644 >>--- a/include/drm/drm_connector.h >>+++ b/include/drm/drm_connector.h >>@@ -497,6 +497,13 @@ struct drm_connector_state { >> unsigned int content_protection; >> >> /** >>+ * @colorspace: Connector property to request colorspace >>+ * change. This is most commonly used to switch to wider color >>+ * gamuts like BT2020. >>+ */ >>+ enum encoder_colorimetry colorspace; >>+ >>+ /** >> * @writeback_job: Writeback job for writeback connectors >> * >> * Holds the framebuffer and out-fence for a writeback connector. As >>diff --git a/include/drm/drm_mode_config.h >>b/include/drm/drm_mode_config.h index d643d26..a6eb0aa 100644 >>--- a/include/drm/drm_mode_config.h >>+++ b/include/drm/drm_mode_config.h >>@@ -863,6 +863,12 @@ struct drm_mode_config { >> uint32_t cursor_width, cursor_height; >> >> /** >>+ * @colorspace_property: Connector property to set the suitable >>+ * colorspace supported by the sink. >>+ */ >>+ struct drm_property *colorspace_property; >>+ >>+ /** >> * @suspend_state: >> * >> * Atomic state when suspended. >>diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h >>index d3e0fe3..831cc77 100644 >>--- a/include/uapi/drm/drm_mode.h >>+++ b/include/uapi/drm/drm_mode.h >>@@ -210,6 +210,30 @@ >> #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 >> #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 >> >>+enum encoder_colorimetry { >>+ /* CEA 861 Normal Colorimetry options */ >>+ COLORIMETRY_ITU_601 = 0, >>+ COLORIMETRY_ITU_709, >>+ /* CEA 861 Extended Colorimetry Options */ >>+ COLORIMETRY_XV_YCC_601, >>+ COLORIMETRY_XV_YCC_709, >>+ COLORIMETRY_S_YCC_601, >>+ COLORIMETRY_ADOBE_YCC_601, >>+ COLORIMETRY_ADOBE_RGB, >>+ COLORIMETRY_BT2020_RGB, >>+ COLORIMETRY_BT2020_YCC, >>+ COLORIMETRY_BT2020_CYCC, >>+ /* DP MSA Colorimetry Options */ >>+ COLORIMETRY_Y_CBCR_ITU_601, >>+ COLORIMETRY_Y_CBCR_ITU_709, >>+ COLORIMETRY_SRGB, >>+ COLORIMETRY_RGB_WIDE_GAMUT, >>+ COLORIMETRY_SCRGB, >>+ COLORIMETRY_DCI_P3, >>+ COLORIMETRY_CUSTOM_COLOR_PROFILE, >>+ COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, }; >>+ >> struct drm_mode_modeinfo { >> __u32 clock; >> __u16 hdisplay; >>-- >>1.9.1 >> >>
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index d5b7f31..9e1d46b 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -721,6 +721,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, state->picture_aspect_ratio = val; } else if (property == config->content_type_property) { state->content_type = val; + } else if (property == config->colorspace_property) { + state->colorspace = val; } else if (property == connector->scaling_mode_property) { state->scaling_mode = val; } else if (property == connector->content_protection_property) { @@ -795,6 +797,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, *val = state->picture_aspect_ratio; } else if (property == config->content_type_property) { *val = state->content_type; + } else if (property == config->colorspace_property) { + *val = state->colorspace; } else if (property == connector->scaling_mode_property) { *val = state->scaling_mode; } else if (property == connector->content_protection_property) { diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index aa18b1d..5ad52a0 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -826,6 +826,38 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, }; DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list) +static const struct drm_prop_enum_list colorspace[] = { + /* Standard Definition Colorimetry based on CEA 861 */ + { COLORIMETRY_ITU_601, "601" }, + { COLORIMETRY_ITU_709, "709" }, + /* Standard Definition Colorimetry based on IEC 61966-2-4 */ + { COLORIMETRY_XV_YCC_601, "601" }, + /* High Definition Colorimetry based on IEC 61966-2-4 */ + { COLORIMETRY_XV_YCC_709, "709" }, + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ + { COLORIMETRY_S_YCC_601, "s601" }, + /* Colorimetry based on IEC 61966-2-5 [33] */ + { COLORIMETRY_ADOBE_YCC_601, "adobe601" }, + /* Colorimetry based on IEC 61966-2-5 */ + { COLORIMETRY_ADOBE_RGB, "adobe_rgb" }, + /* Colorimetry based on ITU-R BT.2020 */ + { COLORIMETRY_BT2020_RGB, "BT2020_rgb" }, + /* Colorimetry based on ITU-R BT.2020 */ + { COLORIMETRY_BT2020_YCC, "BT2020_ycc" }, + /* Colorimetry based on ITU-R BT.2020 */ + { COLORIMETRY_BT2020_CYCC, "BT2020_cycc" }, + /* DP MSA Colorimetry */ + { COLORIMETRY_Y_CBCR_ITU_601, "YCBCR_ITU_601" }, + { COLORIMETRY_Y_CBCR_ITU_709, "YCBCR_ITU_709" }, + { COLORIMETRY_SRGB, "SRGB" }, + { COLORIMETRY_RGB_WIDE_GAMUT, "RGB Wide Gamut" }, + { COLORIMETRY_SCRGB, "SCRGB" }, + { COLORIMETRY_DCI_P3, "DCIP3" }, + { COLORIMETRY_CUSTOM_COLOR_PROFILE, "Custom Proflie" }, + /* FOR HD 720p+, Default Colorimetry is based on ITU-R BT.709 */ + { COLORIMETRY_DEFAULT, "Default: ITU_709" }, +}; + /** * DOC: standard connector properties * @@ -972,6 +1004,12 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, * can also expose this property to external outputs, in which case they * must support "None", which should be the default (since external screens * have a built-in scaler). + * + * colorspace: + * This property helps select a suitable colorspace based on the sink + * capability. Modern sink devices support wider gamut like BT2020. + * This helps switch to BT2020 mode if the BT2020 encoded video stream + * is being played by the user, same for any other colorspace. */ int drm_connector_create_standard_properties(struct drm_device *dev) @@ -1020,6 +1058,12 @@ int drm_connector_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.non_desktop_property = prop; + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "Colorspace", + colorspace, ARRAY_SIZE(colorspace)); + if (!prop) + return -ENOMEM; + dev->mode_config.colorspace_property = prop; + return 0; } diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index dd0552c..b7f5373 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -497,6 +497,13 @@ struct drm_connector_state { unsigned int content_protection; /** + * @colorspace: Connector property to request colorspace + * change. This is most commonly used to switch to wider color + * gamuts like BT2020. + */ + enum encoder_colorimetry colorspace; + + /** * @writeback_job: Writeback job for writeback connectors * * Holds the framebuffer and out-fence for a writeback connector. As diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index d643d26..a6eb0aa 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -863,6 +863,12 @@ struct drm_mode_config { uint32_t cursor_width, cursor_height; /** + * @colorspace_property: Connector property to set the suitable + * colorspace supported by the sink. + */ + struct drm_property *colorspace_property; + + /** * @suspend_state: * * Atomic state when suspended. diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index d3e0fe3..831cc77 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -210,6 +210,30 @@ #define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 #define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 +enum encoder_colorimetry { + /* CEA 861 Normal Colorimetry options */ + COLORIMETRY_ITU_601 = 0, + COLORIMETRY_ITU_709, + /* CEA 861 Extended Colorimetry Options */ + COLORIMETRY_XV_YCC_601, + COLORIMETRY_XV_YCC_709, + COLORIMETRY_S_YCC_601, + COLORIMETRY_ADOBE_YCC_601, + COLORIMETRY_ADOBE_RGB, + COLORIMETRY_BT2020_RGB, + COLORIMETRY_BT2020_YCC, + COLORIMETRY_BT2020_CYCC, + /* DP MSA Colorimetry Options */ + COLORIMETRY_Y_CBCR_ITU_601, + COLORIMETRY_Y_CBCR_ITU_709, + COLORIMETRY_SRGB, + COLORIMETRY_RGB_WIDE_GAMUT, + COLORIMETRY_SCRGB, + COLORIMETRY_DCI_P3, + COLORIMETRY_CUSTOM_COLOR_PROFILE, + COLORIMETRY_DEFAULT = COLORIMETRY_ITU_709, +}; + struct drm_mode_modeinfo { __u32 clock; __u16 hdisplay;
This patch adds a colorspace property enabling userspace to switch to various supported colorspaces. This will help enable BT2020 along with other colorspaces. v2: Addressed Maarten and Ville's review comments. Enhanced the colorspace enum to incorporate both HDMI and DP supported colorspaces. Also, added a default option for colorspace. Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ drivers/gpu/drm/drm_connector.c | 44 +++++++++++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 7 +++++++ include/drm/drm_mode_config.h | 6 ++++++ include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++++ 5 files changed, 85 insertions(+)