>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of >Sharma, Shashank >Sent: Thursday, December 20, 2018 11:02 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; >dri-devel@xxxxxxxxxxxxxxxxxxxxx >Cc: hansverk@xxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, >Maarten <maarten.lankhorst@xxxxxxxxx>; jonas@xxxxxxxxx >Subject: Re: [v5 2/2] drm/i915: Attach colorspace property and enable modeset > >Regards > >Shashank > > >On 12/11/2018 11:44 PM, 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. >> >> Signed-off-by: Uma Shankar <uma.shankar@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 | 18 ++++++++++ >> 4 files changed, 83 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c >> b/drivers/gpu/drm/i915/intel_atomic.c >> index a5a2c8f..35ef70a 100644 >> --- a/drivers/gpu/drm/i915/intel_atomic.c >> +++ b/drivers/gpu/drm/i915/intel_atomic.c >> @@ -125,6 +125,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 18e370f..59fa420 100644 >> --- a/drivers/gpu/drm/i915/intel_connector.c >> +++ b/drivers/gpu/drm/i915/intel_connector.c >> @@ -31,6 +31,48 @@ >> #include "intel_drv.h" >> #include "i915_drv.h" >> >> +static const struct drm_prop_enum_list gen10_hdmi_colorspace[] = { >> + /* For Default case, driver will set the colorspace */ >> + { COLORIMETRY_DEFAULT, "Default" }, >> + /* Standard Definition Colorimetry based on CEA 861 */ >> + { COLORIMETRY_ITU_601, "ITU_601" }, >> + { COLORIMETRY_ITU_709, "ITU_709" }, >> + /* Standard Definition Colorimetry based on IEC 61966-2-4 */ >> + { COLORIMETRY_XV_YCC_601, "XV_YCC_601" }, >> + /* High Definition Colorimetry based on IEC 61966-2-4 */ >> + { COLORIMETRY_XV_YCC_709, "XV_YCC_709" }, >> + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ >> + { COLORIMETRY_S_YCC_601, "S_YCC_601" }, >> + /* Colorimetry based on IEC 61966-2-5 [33] */ >> + { COLORIMETRY_OPYCC_601, "opYCC_601" }, >> + /* Colorimetry based on IEC 61966-2-5 */ >> + { COLORIMETRY_OPRGB, "opRGB" }, >> + /* 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" }, }; >> + >> +static const struct drm_prop_enum_list legacy_hdmi_colorspace[] = { >> + /* For Default case, driver will set the colorspace */ >> + { COLORIMETRY_DEFAULT, "Default" }, >> + /* Standard Definition Colorimetry based on CEA 861 */ >> + { COLORIMETRY_ITU_601, "ITU_601" }, >> + { COLORIMETRY_ITU_709, "ITU_709" }, >> + /* Standard Definition Colorimetry based on IEC 61966-2-4 */ >> + { COLORIMETRY_XV_YCC_601, "XV_YCC_601" }, >> + /* High Definition Colorimetry based on IEC 61966-2-4 */ >> + { COLORIMETRY_XV_YCC_709, "XV_YCC_709" }, >> + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ >> + { COLORIMETRY_S_YCC_601, "S_YCC_601" }, >> + /* Colorimetry based on IEC 61966-2-5 [33] */ >> + { COLORIMETRY_OPYCC_601, "opYCC_601" }, >> + /* Colorimetry based on IEC 61966-2-5 */ >> + { COLORIMETRY_OPRGB, "opRGB" }, >> +}; >> + >> int intel_connector_init(struct intel_connector *connector) >> { >> struct intel_digital_connector_state *conn_state; @@ -262,3 +304,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) { >I am slightly unsure on if we should expose BT2020 colorspace support for >CNL/GEN10 too, even though on paper it supports it, but do we have a proper >color pipeline which can handle input degamma and more precisely the output >gamma lut precision, which might be required post blending to restore BT2020 >non-linearity. Can you please confirm on this, and until, we should just keep it > >10, instead of >= 10. Gen10 does have capability to handle this. So we can enable this safely here. >> + if (!drm_mode_create_colorspace_property(connector, >> + gen10_hdmi_colorspace, >> + ARRAY_SIZE(gen10_hdmi_colorspace))) >> + drm_object_attach_property(&connector->base, >> + connector->colorspace_property, 0); >> + } else { >> + if (!drm_mode_create_colorspace_property(connector, >> + legacy_hdmi_colorspace, >> + ARRAY_SIZE(legacy_hdmi_colorspace))) >> + 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 a62d77b..891bc82 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1762,6 +1762,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 e2c6a2b..7ddc723 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -486,6 +486,22 @@ static void intel_hdmi_set_avi_infoframe(struct >intel_encoder *encoder, >> else >> frame.avi.colorspace = HDMI_COLORSPACE_RGB; >> >> + if (conn_state->colorspace == COLORIMETRY_DEFAULT) { >> + /* Set ITU 709 as default for HDMI */ >> + frame.avi.colorimetry = COLORIMETRY_ITU_709; >> + } else if (conn_state->colorspace < 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. >> + */ >> + frame.avi.extended_colorimetry = conn_state->colorspace - >> + > COLORIMETRY_XV_YCC_601; >> + } >We will need these changes for lspcon_set_infoframes() function too, as there >are talks about needing HDR suppot using LSPCON. If you want to defer this to >next patch, I would like to see a TODO: note. Sure, I will add a TODO to track this. >> + >> drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode, >> crtc_state->limited_color_range ? >> >HDMI_QUANTIZATION_RANGE_LIMITED : >> @@ -2135,6 +2151,8 @@ static void intel_hdmi_destroy(struct drm_connector >*connector) >> intel_attach_force_audio_property(connector); >> intel_attach_broadcast_rgb_property(connector); >> intel_attach_aspect_ratio_property(connector); >> + intel_attach_colorspace_property(connector); >> + >seems like extra blank line ? Will fix this. >> drm_connector_attach_content_type_property(connector); >> connector->state->picture_aspect_ratio = >HDMI_PICTURE_ASPECT_NONE; >> >With the above minor comments fixed, patch looks good to me, feel free to use: >Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> Thanks Shashank for the review. Regards, Uma Shankar >_______________________________________________ >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
