[Intel-gfx] [v10 3/3] drm/i915: Attach colorspace property and enable modeset
Shankar, Uma
uma.shankar at intel.com
Mon Feb 4 17:18:50 UTC 2019
>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of
>Ville Syrjälä
>Sent: Saturday, February 2, 2019 12:31 AM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: 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: [Intel-gfx] [v10 3/3] drm/i915: Attach colorspace property and
>enable modeset
>
>On Wed, Jan 30, 2019 at 06:24:26PM +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: Merged the DP handling along with platform colorspace handling as
>> per Shashank's comments.
>>
>> v6: Reverted to old design of exposing all colorspaces to userspace as
>> per Ville's review comment
>>
>> v7: Fixed a checkpatch complaint, Addressed Maarten' review comment,
>> updated the RB from Maarten and Jani's ack.
>>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> Acked-by: Jani Nikula <jani.nikula at intel.com>
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_atomic.c | 1 +
>> drivers/gpu/drm/i915/intel_connector.c | 8 ++++++++
>> drivers/gpu/drm/i915/intel_drv.h | 1 +
>> drivers/gpu/drm/i915/intel_hdmi.c | 25 +++++++++++++++++++++++++
>> 4 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index 16263ad..a4bb906 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_conn_state->base.colorspace !=
>> +old_conn_state->base.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..8352d0b 100644
>> --- a/drivers/gpu/drm/i915/intel_connector.c
>> +++ b/drivers/gpu/drm/i915/intel_connector.c
>> @@ -265,3 +265,11 @@ 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) {
>> + if (!drm_mode_create_colorspace_property(connector))
>> + 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..5c5009d 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;
>
>Default should map to NONE.
Ok, I will make it NO DATA i.e. 0 for DEFAULT.
>
>> + } 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;
>> + }
>
>This code seems like something that should be in the core.
>
>Like said earlier maybe we could just directly map the values to these bits, and
>then this should just become
>
>if.colorimetry = val & 0x3;
>if.extended_colorimetry = (val >> 2) & 0x7; if.ace = (val >> 5) & 0xf;
Ok, will modify along these lines.
>Hmm, looks like hdmi.h doesn't even know about the ACE bits yet :(
:)
Thanks Ville for all your comments and suggestion.
Regards,
Uma Shankar
>
>> +
>> drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>> conn_state->connector,
>> adjusted_mode,
>> @@ -2143,10 +2161,17 @@ 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
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>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