>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of >Ville Syrjälä >Sent: Tuesday, January 29, 2019 10:09 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >Cc: Syrjala, Ville <ville.syrjala@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; >emil.l.velikov@xxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Lankhorst, Maarten ><maarten.lankhorst@xxxxxxxxx> >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@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. 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@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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
