[Intel-gfx] [v8 08/10] drm/i915:Enabled Modeset when HDR Infoframe changes

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>Sent: Tuesday, May 7, 2019 5:32 PM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; Lankhorst,
>Maarten <maarten.lankhorst at intel.com>; Syrjala, Ville <ville.syrjala at intel.com>;
>Sharma, Shashank <shashank.sharma at intel.com>; emil.l.velikov at gmail.com;
>brian.starkey at arm.com; dcastagna at chromium.org; seanpaul at chromium.org;
>Roper, Matthew D <matthew.d.roper at intel.com>; jonas at kwiboo.se
>Subject: Re: [v8 08/10] drm/i915:Enabled Modeset when HDR Infoframe changes
>
>On Tue, Apr 09, 2019 at 10:14:42PM +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.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> Reviewed-by: Shashank Sharma <shashank.sharma at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c | 14 +++++++++++++-
>>  drivers/gpu/drm/i915/intel_hdmi.c   | 26 ++++++++++++++++++++++++++
>>  2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index 8c8fae3..e8b5f84 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -104,6 +104,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;
>> +}
>
>I have a feeling the memcmp() is overkill. We could just check for whether the blob is
>the same or not. If userspace is an idiot and creates a new blob with identical content
>so be it.

I feel it's good to have such check in kernel and avoid extra infoframe programming.
More like how we suppress modeset request if same mode is requested as part of
modeset.  Hope its ok.

>> +
>>  int intel_digital_connector_atomic_check(struct drm_connector *conn,
>>  					 struct drm_connector_state *new_state)  {
>@@ -131,7 +141,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_blob_ptr,
>> +			old_conn_state->base.hdr_output_metadata_blob_ptr))
>>  		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 0ecfda0..85333a7 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -799,6 +799,20 @@ void intel_read_infoframe(struct intel_encoder *encoder,
>>  	return true;
>>  }
>>
>> +static bool is_eotf_supported(u8 output_eotf, u8 sink_eotf) {
>> +	if (output_eotf == 0)
>> +		return (sink_eotf & (1 << 0));
>> +	if (output_eotf == 1)
>> +		return (sink_eotf & (1 << 1));
>> +	if (output_eotf == 2)
>> +		return (sink_eotf & (1 << 2));
>> +	if (output_eotf == 3)
>> +		return (sink_eotf & (1 << 3));
>> +
>> +	return false;
>
>return sink_eotf & BIT(output_eotf);

Nice suggestion. Will update this.

Regards,
Uma Shankar

>> +}
>> +
>>  static bool
>>  intel_hdmi_compute_drm_infoframe(struct intel_encoder *encoder,
>>  				 struct intel_crtc_state *crtc_state, @@ -806,11
>+820,23 @@ 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_connector *connector = conn_state->connector;
>>  	int ret;
>>
>> +	if (!conn_state->hdr_output_metadata_blob_ptr ||
>> +	    conn_state->hdr_output_metadata_blob_ptr->length == 0)
>> +		return true;
>> +
>>  	hdr_metadata = (struct hdr_output_metadata *)
>>  			conn_state->hdr_output_metadata_blob_ptr->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;
>> +	}
>> +
>>  	ret = drm_hdmi_infoframe_set_hdr_metadata(frame, hdr_metadata);
>>  	if (ret < 0) {
>>  		DRM_ERROR("couldn't set HDR metadata in infoframe\n");
>> --
>> 1.9.1
>
>--
>Ville Syrjälä
>Intel