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

Submitted by Shankar, Uma on March 20, 2019, 10:48 a.m.

Details

Message ID 1553078906-5991-6-git-send-email-uma.shankar@intel.com
State New
Headers show
Series "Add HDR Metadata Parsing and handling in DRM layer" ( rev: 6 ) in DRI devel

Commit Message

Shankar, Uma March 20, 2019, 10:48 a.m.
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>
---
 drivers/gpu/drm/drm_atomic.c      |  2 ++
 drivers/gpu/drm/drm_atomic_uapi.c | 13 +++++++++++++
 2 files changed, 15 insertions(+)

Patch hide | download patch | download mbox

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;
+		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) {

Comments

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.

> ---
>  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

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
>
>-----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
>>