On 2018-11-02 15:13, Shankar, Uma wrote: > >> -----Original Message----- >> From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >> Sent: Friday, November 2, 2018 5:00 PM >> To: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> >> Cc: Shankar, Uma <uma.shankar@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx; >> intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; >> Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx>; Hans Verkuil >> <hansverk@xxxxxxxxx> >> Subject: Re: [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@xxxxxxxxx> >>>> --- >>>> 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@xxxxxxxxxxxxxxxxxxxxx >>> 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@xxxxxxxxxxxxxxxxxxxxx > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
