[Intel-gfx] [v2 2/2] drm/i915: Attach colorspace property and enable modeset

[Maarten Lankhorst]

Op 02-11-18 om 16:41 schreef Ville Syrjälä:
> On Fri, Nov 02, 2018 at 02:18:42PM +0000, Shankar, Uma wrote:
>>
>>> -----Original Message-----
>>> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
>>> Sent: Friday, November 2, 2018 2:53 PM
>>> To: Shankar, Uma <uma.shankar at intel.com>; dri-devel at lists.freedesktop.org;
>>> intel-gfx at lists.freedesktop.org
>>> Cc: Syrjala, Ville <ville.syrjala at intel.com>; ajax at redhat.com; Lankhorst, Maarten
>>> <maarten.lankhorst at intel.com>
>>> Subject: Re: [Intel-gfx] [v2 2/2] drm/i915: Attach colorspace property and enable
>>> modeset
>>>
>>> Op 31-10-18 om 13:05 schreef Uma Shankar:
>>>> 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.
>>>>
>>>> v2: Merged the changes of creating infoframe as well to this patch as
>>>> per Maarten's suggestion.
>>>>
>>>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_atomic.c | 1 +
>>>>  drivers/gpu/drm/i915/intel_hdmi.c   | 5 +++++
>>>>  2 files changed, 6 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_hdmi.c
>>>> b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> index 129b880..8a41fb3 100644
>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> @@ -486,6 +486,8 @@ static void intel_hdmi_set_avi_infoframe(struct
>>> intel_encoder *encoder,
>>>>  	else
>>>>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>>>
>>>> +	frame.avi.extended_colorimetry = conn_state->colorspace;
>>>> +
>>>>  	drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode,
>>>>  					   crtc_state->limited_color_range ?
>>>>
>>> HDMI_QUANTIZATION_RANGE_LIMITED :
>>>> @@ -2125,6 +2127,9 @@ static void intel_hdmi_destroy(struct drm_connector
>>> *connector)
>>>>  	intel_attach_broadcast_rgb_property(connector);
>>>>  	intel_attach_aspect_ratio_property(connector);
>>>>  	drm_connector_attach_content_type_property(connector);
>>>> +	drm_object_attach_property(&connector->base,
>>>> +			connector->dev->mode_config.colorspace_property,
>>>> +			COLORIMETRY_ITU_709);
>>> Just put 0 here..
>>> If you want to init the default colorspace, put it in the first patch.
>> Ok, will update this.
>>
>>> We should perhaps hide color spaces that are not supported on HDMI?
>> Currently the supported colorspaces will be picked from edid by userspace and
>> they should use the current property interface to set the one which is supported.
>> Even on HDMI, some connectors may not support certain colorspace, so keeping
>> it on userspace to set the one which is supported by the particular connector. Hope
>> this approach is fine ?
> I think we want to trim the list to whatever the infoframe vs. MSA/VSC
> SDP can carry. So HDMI will have one list, DP another. And I guess for
> lspcon we want to go with the HDMI definition since we populate the
> infoframe by hand.
>
What about passive DP to HDMI convertors?