[v10,09/12] drm/i915:Enabled Modeset when HDR Infoframe changes
diff mbox series

Message ID 1557855394-12214-10-git-send-email-uma.shankar@intel.com
State New
Headers show
Series
  • Add HDR Metadata Parsing and handling in DRM layer
Related show

Commit Message

Shankar, Uma May 14, 2019, 5:36 p.m. UTC
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(-)

Comments

Ville Syrjälä May 15, 2019, 7:27 p.m. UTC | #1
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.

> +
>  	crtc_state->infoframes.enable |=
>  		intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_DRM);
>  
> -- 
> 1.9.1
Shankar, Uma May 16, 2019, 10:54 a.m. UTC | #2
>-----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.

>> +
>>  	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ä May 16, 2019, 1:13 p.m. UTC | #3
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

Patch
diff mbox series

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;
+	}
+
 	crtc_state->infoframes.enable |=
 		intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_DRM);