Re: [v10 09/12] drm/i915:Enabled Modeset when HDR Infoframe changes

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

On Thu, May 16, 2019 at 10:54:14AM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville
> >Syrjälä
> >Sent: Thursday, May 16, 2019 12:57 AM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: dcastagna@chromium.org; jonas@kwiboo.se; intel-gfx@lists.freedesktop.org;
> >emil.l.velikov@gmail.com; dri-devel@lists.freedesktop.org; seanpaul@chromium.org
> >Subject: Re: [v10 09/12] drm/i915:Enabled Modeset when HDR Infoframe changes
> >
> >On Tue, May 14, 2019 at 11:06:31PM +0530, Uma Shankar wrote:
> >> This patch enables modeset whenever HDR metadata needs to be updated
> >> to sink.
> >>
> >> v2: Addressed Shashank's review comments.
> >>
> >> v3: Added Shashank's RB.
> >>
> >> v4: Addressed Ville's review comments.
> >>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_atomic.c | 14 +++++++++++++-
> >>  drivers/gpu/drm/i915/intel_hdmi.c   | 13 +++++++++++++
> >>  2 files changed, 26 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> >> b/drivers/gpu/drm/i915/intel_atomic.c
> >> index 58b8049..6b985e8 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >> @@ -105,6 +105,16 @@ int intel_digital_connector_atomic_set_property(struct
> >drm_connector *connector,
> >>  	return -EINVAL;
> >>  }
> >>
> >> +static bool blob_equal(const struct drm_property_blob *a,
> >> +		       const struct drm_property_blob *b) {
> >> +	if (a && b)
> >> +		return a->length == b->length &&
> >> +			!memcmp(a->data, b->data, a->length);
> >> +
> >> +	return !a == !b;
> >> +}
> >> +
> >>  int intel_digital_connector_atomic_check(struct drm_connector *conn,
> >>  					 struct drm_connector_state *new_state)  {
> >@@ -132,7 +142,9 @@
> >> int intel_digital_connector_atomic_check(struct drm_connector *conn,
> >>  	    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)
> >> +	    new_conn_state->base.scaling_mode != old_conn_state-
> >>base.scaling_mode ||
> >> +	    !blob_equal(new_conn_state->base.hdr_output_metadata,
> >> +			old_conn_state->base.hdr_output_metadata))
> >>  		crtc_state->mode_changed = true;
> >>
> >>  	return 0;
> >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> >> b/drivers/gpu/drm/i915/intel_hdmi.c
> >> index b80406b..e97bf6e 100644
> >> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >> @@ -806,6 +806,11 @@ void intel_read_infoframe(struct intel_encoder *encoder,
> >>  	return true;
> >>  }
> >>
> >> +static inline bool is_eotf_supported(u8 output_eotf, u8 sink_eotf) {
> >> +	return sink_eotf & BIT(output_eotf); }
> >> +
> >>  static bool
> >>  intel_hdmi_compute_drm_infoframe(struct intel_encoder *encoder,
> >>  				 struct intel_crtc_state *crtc_state, @@ -814,6
> >+819,7 @@ void
> >> intel_read_infoframe(struct intel_encoder *encoder,
> >>  	struct hdmi_drm_infoframe *frame = &crtc_state->infoframes.drm.drm;
> >>  	struct hdr_output_metadata *hdr_metadata;
> >>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >> +	struct drm_connector *connector = conn_state->connector;
> >>  	int ret;
> >>
> >>  	if (!(INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))) @@
> >> -828,6 +834,13 @@ void intel_read_infoframe(struct intel_encoder
> >> *encoder,
> >>
> >>  	hdr_metadata = conn_state->hdr_output_metadata->data;
> >>
> >> +	/* Sink EOTF is Bit map while infoframe is absolute values */
> >> +	if (!is_eotf_supported(hdr_metadata->hdmi_metadata_type1.eotf,
> >> +	    connector->hdr_sink_metadata.hdmi_type1.eotf)) {
> >> +		DRM_ERROR("EOTF Not Supported\n");
> >> +		return true;
> >> +	}
> >
> >I was going to say that this should probably be in the
> >drm_set_hdr_metdata() or whatever it was called.
> >
> >But now I'm now wondering if we can even have this check here. What happens if
> >someone does a display switcheroo while the machine is suspended? Depends on
> >when we're going to reprobe the displays I suppose. Hmm. Maybe it's fine. We
> >already have a similar issue after all wih the has_hdmi2_sink stuff.
> >
> >Either way the user triggerable DRM_ERROR()s are at least a nono.
> 
> Ok, Will keep the check and move it inside the drm_set_hdr_metdata(). Also downgrade
> the print as INFO instead of ERROR. Hope this is fine.

DEBUG_KMS like everything else.

> 
> >> +
> >>  	crtc_state->infoframes.enable |=
> >>  		intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_DRM);
> >>
> >> --
> >> 1.9.1
> >
> >--
> >Ville Syrjälä
> >Intel
> >_______________________________________________
> >dri-devel mailing list
> >dri-devel@lists.freedesktop.org
> >https://lists.freedesktop.org/mailman/listinfo/dri-devel

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