RE: [v2 07/14] drm: Implement HDR source metadata set and get property handling

From: "Shankar, Uma" <uma.shankar@intel.com>
To: "Sharma, Shashank" <shashank.sharma@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Cc: "Syrjala, Ville" <ville.syrjala@intel.com>,
	"Lankhorst, Maarten" <maarten.lankhorst@intel.com>
Subject: RE: [v2 07/14] drm: Implement HDR source metadata set and get property handling
Date: Tue, 8 Jan 2019 06:48:14 +0000	[thread overview]
Message-ID: <E7C9878FBA1C6D42A1CA3F62AEB6945F81E9B96A@BGSMSX104.gar.corp.intel.com> (raw)
In-Reply-To: <388e694e-787a-55b0-455d-eaa541765e0c@intel.com>

>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of
>Sharma, Shashank
>Sent: Friday, December 21, 2018 12:04 AM
>To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
>dri-devel@lists.freedesktop.org
>Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
><maarten.lankhorst@intel.com>
>Subject: Re: [v2 07/14] drm: Implement HDR source metadata set and get
>property handling
>
>Regards
>
>Shashank
>
>
>On 12/12/2018 2:08 AM, Uma Shankar wrote:
>> HDR source metadata set and get property implemented in this patch.
>Again, HDR output metadata ? How about re-arranging the line like "This patch
>implements get() and set() functions for HDR output metadata property" just to
>make it more clear ?

Sure, will update this.

>> 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
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>   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..4e71c6b 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);
>Alignment

Ok, will update.

>>
>>   	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 c408898..b721b12 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_source_metadata_property) {
>> +		ret = drm_atomic_replace_property_blob_from_id(dev,
>> +				&state->hdr_source_metadata_blob_ptr,
>> +				val,
>> +				-1, sizeof(struct hdr_static_metadata),
>> +				&replaced);
>Alignment, but I know it might be difficult to contain the params within
>80 chars.

Yeah, not sure what is higher priority - 80 character limit or the
alignment here. Currently went with 80 character limit.

Regards,
Uma Shankar

>> +		state->hdr_metadata_changed |= replaced;
>> +		return ret;
>>   	} else if (property == config->aspect_ratio_property) {
>>   		state->picture_aspect_ratio = val;
>>   	} else if (property == config->content_type_property) { @@ -816,6
>> +826,9 @@ static int drm_atomic_connector_set_property(struct
>drm_connector *connector,
>>   		*val = state->content_type;
>>   	} else if (property == connector->scaling_mode_property) {
>>   		*val = state->scaling_mode;
>> +	} else if (property == config->hdr_source_metadata_property) {
>> +		*val = (state->hdr_source_metadata_blob_ptr) ?
>> +			state->hdr_source_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) {
>- Shashank
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel