[v6 05/13] drm: Implement HDR output metadata set and get property handling

[Shankar, Uma]

>-----Original Message-----
>From: Brian Starkey [mailto:Brian.Starkey at arm.com]
>Sent: Thursday, March 21, 2019 5:17 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; Liviu
>Dudau <Liviu.Dudau at arm.com>; nd <nd at arm.com>
>Subject: Re: [v6 05/13] drm: Implement HDR output metadata set and get property
>handling
>
>Hi,
>
>On Wed, Mar 20, 2019 at 04:18:18PM +0530, Uma Shankar wrote:
>> This patch implements get() and set() functions for HDR output
>> metadata property.The blob data is received from userspace and saved
>> in connector state, the same is returned as blob in get property call
>> to userspace.
>>
>> v2: Rebase and added Ville's POC changes
>>
>> v3: No Change
>>
>> v4: Addressed Shashank's review comments
>>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>
>This looks like a candidate to be squashed into patch 1.

Ok, will merged it with 1.

>> ---
>>  drivers/gpu/drm/drm_atomic.c      |  2 ++
>>  drivers/gpu/drm/drm_atomic_uapi.c | 13 +++++++++++++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c
>> b/drivers/gpu/drm/drm_atomic.c index 5eb4013..8b9c126 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -881,6 +881,8 @@ static void
>> drm_atomic_connector_print_state(struct drm_printer *p,
>>
>>  	drm_printf(p, "connector[%u]: %s\n", connector->base.id, connector-
>>name);
>>  	drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name :
>> "(null)");
>> +	drm_printf(p, "\thdr_metadata_changed=%d\n",
>> +		   state->hdr_metadata_changed);
>>
>>  	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
>>  		if (state->writeback_job && state->writeback_job->fb) diff --git
>> a/drivers/gpu/drm/drm_atomic_uapi.c
>> b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 4eb81f1..18c8b81f 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -686,6 +686,8 @@ static int
>> drm_atomic_connector_set_property(struct drm_connector *connector,  {
>>  	struct drm_device *dev = connector->dev;
>>  	struct drm_mode_config *config = &dev->mode_config;
>> +	bool replaced = false;
>> +	int ret;
>>
>>  	if (property == config->prop_crtc_id) {
>>  		struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val); @@ -734,6
>> +736,14 @@ static int drm_atomic_connector_set_property(struct drm_connector
>*connector,
>>  		 */
>>  		if (state->link_status != DRM_LINK_STATUS_GOOD)
>>  			state->link_status = val;
>> +	} else if (property == config->hdr_output_metadata_property) {
>> +		ret = drm_atomic_replace_property_blob_from_id(dev,
>> +				&state->hdr_output_metadata_blob_ptr,
>> +				val,
>> +				-1, sizeof(struct hdr_static_metadata),
>> +				&replaced);
>> +		state->hdr_metadata_changed |= replaced;
>
>I've said the same about other flags: Do we really need them?
>It seems easy enough to just compare the blob pointers or their IDs

This was just an extension of how gamma luts are used. It's just to have this check
here and later use it where ever needed, since we may require it at multiple places for
certain checks. 

Regards,
Uma Shankar

>
>Thanks,
>-Brian
>
>> +		return ret;
>>  	} else if (property == config->aspect_ratio_property) {
>>  		state->picture_aspect_ratio = val;
>>  	} else if (property == config->content_type_property) { @@ -820,6
>> +830,9 @@ static int drm_atomic_connector_set_property(struct drm_connector
>*connector,
>>  		*val = state->colorspace;
>>  	} else if (property == connector->scaling_mode_property) {
>>  		*val = state->scaling_mode;
>> +	} else if (property == config->hdr_output_metadata_property) {
>> +		*val = (state->hdr_output_metadata_blob_ptr) ?
>> +			state->hdr_output_metadata_blob_ptr->base.id : 0;
>>  	} else if (property == connector->content_protection_property) {
>>  		*val = state->content_protection;
>>  	} else if (property == config->writeback_fb_id_property) {
>> --
>> 1.9.1
>>