[Intel-gfx] [v5 2/2] drm/i915: Attach colorspace property and enable modeset
Shankar, Uma
uma.shankar at intel.com
Thu Dec 27 15:11:04 UTC 2018
>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of
>Sharma, Shashank
>Sent: Thursday, December 20, 2018 11:02 PM
>To: Shankar, Uma <uma.shankar at intel.com>; intel-gfx at lists.freedesktop.org;
>dri-devel at lists.freedesktop.org
>Cc: hansverk at cisco.com; Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst,
>Maarten <maarten.lankhorst at intel.com>; jonas at kwiboo.se
>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 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 | 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 at intel.com>
Thanks Shashank for the review.
Regards,
Uma Shankar
>_______________________________________________
>dri-devel mailing list
>dri-devel at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the Intel-gfx
mailing list