On Tue, Jan 29, 2019 at 04:20:39PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > >Sent: Tuesday, January 29, 2019 9:34 PM > >To: Shankar, Uma <uma.shankar@xxxxxxxxx> > >Cc: emil.l.velikov@xxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville > ><ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx>; > >dri-devel@xxxxxxxxxxxxxxxxxxxxx > >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@xxxxxxxxxxxxxxxxxxxxx] On > >> >Behalf Of Ville Syrjälä > >> >Sent: Tuesday, January 29, 2019 9:14 PM > >> >To: Shankar, Uma <uma.shankar@xxxxxxxxx> > >> >Cc: emil.l.velikov@xxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; > >> >Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten > >> ><maarten.lankhorst@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx > >> >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@xxxxxxxxx> > >> >> Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> > >> >> --- > >> >> 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. > 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@xxxxxxxxxxxxxxxxxxxxx > >> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >> > > >> >-- > >> >Ville Syrjälä > >> >Intel > >> >_______________________________________________ > >> >dri-devel mailing list > >> >dri-devel@xxxxxxxxxxxxxxxxxxxxx > >> >https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > >-- > >Ville Syrjälä > >Intel -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
