Re: [v2 2/2] drm/i915: Attach colorspace property and enable modeset

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 02, 2018 at 04:44:10PM +0100, Maarten Lankhorst wrote:
> 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@linux.intel.com]
> >>> Sent: Friday, November 2, 2018 2:53 PM
> >>> To: Shankar, Uma <uma.shankar@intel.com>; dri-devel@lists.freedesktop.org;
> >>> intel-gfx@lists.freedesktop.org
> >>> Cc: Syrjala, Ville <ville.syrjala@intel.com>; ajax@redhat.com; Lankhorst, Maarten
> >>> <maarten.lankhorst@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@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?

By passive you mean DP++? Those are HDMI for us.

External DP->HDMI protocol converters are perhaps more tricky, but
since we treat those as pure DP now and don't send any infoframes I
think we should do the same when it comes to the exposed valeus for
the property.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx