[v8 2/2] drm/i915: Attach colorspace property and enable modeset
Shankar, Uma
uma.shankar at intel.com
Tue Jan 29 16:46:02 UTC 2019
>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of
>Ville Syrjälä
>Sent: Tuesday, January 29, 2019 10:09 PM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: Syrjala, Ville <ville.syrjala at intel.com>; intel-gfx at lists.freedesktop.org;
>emil.l.velikov at gmail.com; dri-devel at lists.freedesktop.org; Lankhorst, Maarten
><maarten.lankhorst at intel.com>
>Subject: Re: [v8 2/2] drm/i915: Attach colorspace property and enable modeset
>
>On Tue, Jan 29, 2019 at 04:20:39PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>> >Sent: Tuesday, January 29, 2019 9:34 PM
>> >To: Shankar, Uma <uma.shankar at intel.com>
>> >Cc: emil.l.velikov at gmail.com; intel-gfx at lists.freedesktop.org;
>> >Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst, Maarten
>> ><maarten.lankhorst at intel.com>; dri-devel at lists.freedesktop.org
>> >Subject: Re: [v8 2/2] drm/i915: Attach colorspace property and enable
>> >modeset
>> >
>> >On Tue, Jan 29, 2019 at 03:57:29PM +0000, Shankar, Uma wrote:
>> >>
>> >>
>> >> >-----Original Message-----
>> >> >From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org]
>> >> >On Behalf Of Ville Syrjälä
>> >> >Sent: Tuesday, January 29, 2019 9:14 PM
>> >> >To: Shankar, Uma <uma.shankar at intel.com>
>> >> >Cc: emil.l.velikov at gmail.com; intel-gfx at lists.freedesktop.org;
>> >> >Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst, Maarten
>> >> ><maarten.lankhorst at intel.com>; dri-devel at lists.freedesktop.org
>> >> >Subject: Re: [v8 2/2] drm/i915: Attach colorspace property and
>> >> >enable modeset
>> >> >
>> >> >On Tue, Jan 29, 2019 at 09:22:56PM +0530, Uma Shankar wrote:
>> >> >> This patch attaches the colorspace connector property to the
>> >> >> hdmi connector. Based on colorspace change, modeset will be
>> >> >> triggered to switch to new colorspace.
>> >> >>
>> >> >> Based on colorspace property value create an infoframe with
>> >> >> appropriate colorspace. This can be used to send an infoframe
>> >> >> packet with proper colorspace value set which will help to
>> >> >> enable wider color gamut like BT2020 on sink.
>> >> >>
>> >> >> This patch attaches and enables HDMI colorspace, DP will be
>> >> >> taken care separately.
>> >> >>
>> >> >> v2: Merged the changes of creating infoframe as well to this
>> >> >> patch as per Maarten's suggestion.
>> >> >>
>> >> >> v3: Addressed review comments from Shashank. Separated HDMI and
>> >> >> DP colorspaces as suggested by Ville and Maarten.
>> >> >>
>> >> >> v4: Addressed Chris and Ville's review comments, and created a
>> >> >> common colorspace property for DP and HDMI, filtered the list
>> >> >> based on the colorspaces supported by the respective protocol standard.
>> >> >> Handle the default case properly.
>> >> >>
>> >> >> v5: Added Platform specific colorspace enums and called the
>> >> >> property creation helper using the same.
>> >> >>
>> >> >> v6: Addressed Shashank's review comments.
>> >> >>
>> >> >> v7: Rebase
>> >> >>
>> >> >> v8: Addressed Maarten's review comments.
>> >> >>
>> >> >> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> >> >> Reviewed-by: Shashank Sharma <shashank.sharma at intel.com>
>> >> >> ---
>> >> >> drivers/gpu/drm/i915/intel_atomic.c | 1 +
>> >> >> drivers/gpu/drm/i915/intel_connector.c | 63
>> >> >++++++++++++++++++++++++++++++++++
>> >> >> drivers/gpu/drm/i915/intel_drv.h | 1 +
>> >> >> drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++
>> >> >> 4 files changed, 89 insertions(+)
>> >> >>
>> >> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> >> >> b/drivers/gpu/drm/i915/intel_atomic.c
>> >> >> index 16263ad..76b7114 100644
>> >> >> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> >> >> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> >> >> @@ -124,6 +124,7 @@ int
>> >> >> intel_digital_connector_atomic_check(struct
>> >> >drm_connector *conn,
>> >> >> */
>> >> >> if (new_conn_state->force_audio != old_conn_state-
>>force_audio ||
>> >> >> new_conn_state->broadcast_rgb !=
>> >> >> old_conn_state->broadcast_rgb
>> >> >> ||
>> >> >> + new_state->colorspace != old_state->colorspace ||
>> >> >> new_conn_state->base.picture_aspect_ratio !=
>> >> >>old_conn_state- base.picture_aspect_ratio ||
>> >> >> new_conn_state->base.content_type != old_conn_state-
>> >> >>base.content_type ||
>> >> >> new_conn_state->base.scaling_mode !=
>> >> >> old_conn_state->base.scaling_mode)
>> >> >> diff --git a/drivers/gpu/drm/i915/intel_connector.c
>> >> >> b/drivers/gpu/drm/i915/intel_connector.c
>> >> >> index ee16758..9a12d5e 100644
>> >> >> --- a/drivers/gpu/drm/i915/intel_connector.c
>> >> >> +++ b/drivers/gpu/drm/i915/intel_connector.c
>> >> >> @@ -30,6 +30,48 @@
>> >> >> #include "intel_drv.h"
>> >> >> #include "i915_drv.h"
>> >> >>
>> >> >> +static const struct drm_prop_enum_list gen10_hdmi_colorspaces[] = {
>> >> >> + /* For Default case, driver will set the colorspace */
>> >> >> + { DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
>> >> >> + /* Standard Definition Colorimetry based on CEA 861 */
>> >> >> + { DRM_MODE_COLORIMETRY_ITU_601, "ITU_601" },
>> >> >> + { DRM_MODE_COLORIMETRY_ITU_709, "ITU_709" },
>> >> >> + /* Standard Definition Colorimetry based on IEC 61966-2-4 */
>> >> >> + { DRM_MODE_COLORIMETRY_XV_YCC_601, "XV_YCC_601" },
>> >> >> + /* High Definition Colorimetry based on IEC 61966-2-4 */
>> >> >> + { DRM_MODE_COLORIMETRY_XV_YCC_709, "XV_YCC_709" },
>> >> >> + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>> >> >> + { DRM_MODE_COLORIMETRY_S_YCC_601, "S_YCC_601" },
>> >> >> + /* Colorimetry based on IEC 61966-2-5 [33] */
>> >> >> + { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
>> >> >> + /* Colorimetry based on IEC 61966-2-5 */
>> >> >> + { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
>> >> >> + /* Colorimetry based on ITU-R BT.2020 */
>> >> >> + { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
>> >> >> + /* Colorimetry based on ITU-R BT.2020 */
>> >> >> + { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
>> >> >> + /* Colorimetry based on ITU-R BT.2020 */
>> >> >> + { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
>};
>> >> >> +
>> >> >> +static const struct drm_prop_enum_list legacy_hdmi_colorspaces[] = {
>> >> >> + /* For Default case, driver will set the colorspace */
>> >> >> + { DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
>> >> >> + /* Standard Definition Colorimetry based on CEA 861 */
>> >> >> + { DRM_MODE_COLORIMETRY_ITU_601, "ITU_601" },
>> >> >> + { DRM_MODE_COLORIMETRY_ITU_709, "ITU_709" },
>> >> >> + /* Standard Definition Colorimetry based on IEC 61966-2-4 */
>> >> >> + { DRM_MODE_COLORIMETRY_XV_YCC_601, "XV_YCC_601" },
>> >> >> + /* High Definition Colorimetry based on IEC 61966-2-4 */
>> >> >> + { DRM_MODE_COLORIMETRY_XV_YCC_709, "XV_YCC_709" },
>> >> >> + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */
>> >> >> + { DRM_MODE_COLORIMETRY_S_YCC_601, "S_YCC_601" },
>> >> >> + /* Colorimetry based on IEC 61966-2-5 [33] */
>> >> >> + { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
>> >> >> + /* Colorimetry based on IEC 61966-2-5 */
>> >> >> + { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" }, };
>> >> >
>> >> >Why are we duplicating these in the driver code? And why do we
>> >> >have different sets for gen10 and older?
>> >>
>> >> This is to expose colorspaces which a particular platforms supports
>> >> instead of giving full HDMI protocol supported list as some
>> >> platforms may not
>> >support all of it.
>> >>
>> >> Here, BT2020 support is added for Gen10 onwards, while older
>> >> platforms support all the rest apart from BT2020.
>> >
>> >This is just about the infoframe. I don't think we have any platform
>> >limitations there.
>>
>> BT2020 is not supported on legacy platforms so we thought to not
>> expose this here as well, else it may lead to confusion.
>
>Nothing prevents you from scanning out BT.2020 content even on a gen2.
Ok, I will remove this platform specific implementation and create a generic
HDMI colorspace property. Will resend the next version with this updated.
Thanks Ville.
Regards,
Uma Shankar
>> Not sure what's the best approach here.
>> Earlier it was done without any platform checks in v4.
>>
>> If you suggest to expose all HDMI supported colorspaces, I will make
>> the change and update this series. What do you recommend ?
>>
>> Regards,
>> Uma Shankar
>>
>> >>
>> >> For sink supported colorspaces, userspace will retrieve it from
>> >> EDID and set the colorspace which is supported by both platform as well as
>sink.
>> >>
>> >> Regards,
>> >> Uma Shankar
>> >>
>> >> >> +
>> >> >> int intel_connector_init(struct intel_connector *connector) {
>> >> >> struct intel_digital_connector_state *conn_state; @@ -265,3
>> >> >> +307,24 @@ int intel_ddc_get_modes(struct drm_connector
>> >> >> +*connector,
>> >> >> connector->dev-
>>mode_config.aspect_ratio_property,
>> >> >> DRM_MODE_PICTURE_ASPECT_NONE); }
>> >> >> +
>> >> >> +void
>> >> >> +intel_attach_colorspace_property(struct drm_connector *connector) {
>> >> >> + struct drm_device *dev = connector->dev;
>> >> >> + struct drm_i915_private *dev_priv = to_i915(dev);
>> >> >> +
>> >> >> + if (INTEL_GEN(dev_priv) >= 10) {
>> >> >> + if (!drm_mode_create_colorspace_property(connector,
>> >> >> + gen10_hdmi_colorspaces,
>> >> >> + ARRAY_SIZE(gen10_hdmi_colorspaces)))
>> >> >> + drm_object_attach_property(&connector-
>>base,
>> >> >> + connector->colorspace_property, 0);
>> >> >> + } else {
>> >> >> + if (!drm_mode_create_colorspace_property(connector,
>> >> >> + legacy_hdmi_colorspaces,
>> >> >> + ARRAY_SIZE(legacy_hdmi_colorspaces)))
>> >> >> + drm_object_attach_property(&connector-
>>base,
>> >> >> + connector->colorspace_property, 0);
>> >> >> + }
>> >> >> +}
>> >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> >> >> b/drivers/gpu/drm/i915/intel_drv.h
>> >> >> index 85b913e..5178a9a 100644
>> >> >> --- a/drivers/gpu/drm/i915/intel_drv.h
>> >> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> >> >> @@ -1783,6 +1783,7 @@ int intel_connector_update_modes(struct
>> >> >> drm_connector *connector, void
>> >> >> intel_attach_force_audio_property(struct drm_connector
>> >> >> *connector); void intel_attach_broadcast_rgb_property(struct
>> >> >> drm_connector *connector); void
>> >> >> intel_attach_aspect_ratio_property(struct
>> >> >> drm_connector *connector);
>> >> >> +void intel_attach_colorspace_property(struct drm_connector
>> >> >> +*connector);
>> >> >>
>> >> >> /* intel_csr.c */
>> >> >> void intel_csr_ucode_init(struct drm_i915_private *); diff
>> >> >> --git a/drivers/gpu/drm/i915/intel_hdmi.c
>> >> >> b/drivers/gpu/drm/i915/intel_hdmi.c
>> >> >> index 97a98e1..9ed00e3 100644
>> >> >> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> >> >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> >> >> @@ -498,6 +498,24 @@ static void
>> >> >> intel_hdmi_set_avi_infoframe(struct
>> >> >intel_encoder *encoder,
>> >> >> else
>> >> >> frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>> >> >>
>> >> >> + if (conn_state->colorspace ==
>DRM_MODE_COLORIMETRY_DEFAULT) {
>> >> >> + /* Set ITU 709 as default for HDMI */
>> >> >> + frame.avi.colorimetry =
>DRM_MODE_COLORIMETRY_ITU_709;
>> >> >> + } else if (conn_state->colorspace <
>> >> >DRM_MODE_COLORIMETRY_XV_YCC_601) {
>> >> >> + frame.avi.colorimetry = conn_state->colorspace;
>> >> >> + } else {
>> >> >> + frame.avi.colorimetry =
>HDMI_COLORIMETRY_EXTENDED;
>> >> >> + /*
>> >> >> + * Starting from extended list where
>COLORIMETRY_XV_YCC_601
>> >> >> + * is the first extended mode and its value is 0 as per
>HDMI
>> >> >> + * specification.
>> >> >> + * TODO: This needs to be extended for LSPCON
>implementation
>> >> >> + * as well. Will be implemented separately.
>> >> >> + */
>> >> >> + frame.avi.extended_colorimetry = conn_state-
>>colorspace -
>> >> >> + DRM_MODE_COLORIMETRY_XV_YCC_601;
>> >> >> + }
>> >> >> +
>> >> >> drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>> >> >> conn_state->connector,
>> >> >> adjusted_mode,
>> >> >> @@ -2143,10 +2161,16 @@ static void intel_hdmi_destroy(struct
>> >> >> drm_connector *connector) intel_hdmi_add_properties(struct
>> >> >> intel_hdmi *intel_hdmi, struct drm_connector *connector) {
>> >> >> struct drm_i915_private *dev_priv = to_i915(connector->dev);
>> >> >> + struct intel_digital_port *intel_dig_port =
>> >> >> + hdmi_to_dig_port(intel_hdmi);
>> >> >>
>> >> >> intel_attach_force_audio_property(connector);
>> >> >> intel_attach_broadcast_rgb_property(connector);
>> >> >> intel_attach_aspect_ratio_property(connector);
>> >> >> + /* Attach Colorspace property for Non LSPCON based device */
>> >> >> + if (!intel_dig_port->lspcon.active)
>> >> >> + intel_attach_colorspace_property(connector);
>> >> >> +
>> >> >> drm_connector_attach_content_type_property(connector);
>> >> >> connector->state->picture_aspect_ratio =
>> >> >HDMI_PICTURE_ASPECT_NONE;
>> >> >>
>> >> >> --
>> >> >> 1.9.1
>> >> >>
>> >> >> _______________________________________________
>> >> >> dri-devel mailing list
>> >> >> dri-devel at lists.freedesktop.org
>> >> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >> >
>> >> >--
>> >> >Ville Syrjälä
>> >> >Intel
>> >> >_______________________________________________
>> >> >dri-devel mailing list
>> >> >dri-devel at lists.freedesktop.org
>> >> >https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>
>--
>Ville Syrjälä
>Intel
>_______________________________________________
>dri-devel mailing list
>dri-devel at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list