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

From: "Shankar, Uma" <uma.shankar@intel.com>
To: Brian Starkey <Brian.Starkey@arm.com>
Cc: "Syrjala, Ville" <ville.syrjala@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>, nd <nd@arm.com>,
	"Lankhorst, Maarten" <maarten.lankhorst@intel.com>
Subject: Re: [v6 05/13] drm: Implement HDR output metadata set and get property handling
Date: Fri, 29 Mar 2019 06:21:03 +0000	[thread overview]
Message-ID: <E7C9878FBA1C6D42A1CA3F62AEB6945F81F90C79@BGSMSX104.gar.corp.intel.com> (raw)
In-Reply-To: <20190321114631.uzrbpvbgghm5x6pd@DESKTOP-E1NTVVP.localdomain>

>-----Original Message-----
>From: Brian Starkey [mailto:Brian.Starkey@arm.com]
>Sent: Thursday, March 21, 2019 5:17 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Lankhorst,
>Maarten <maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
>Sharma, Shashank <shashank.sharma@intel.com>; emil.l.velikov@gmail.com; Liviu
>Dudau <Liviu.Dudau@arm.com>; nd <nd@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@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
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx