RE: [Intel-gfx] [RFC 1/7] drm: Add Plane Degamma properties

2017-11-09 Thread Shankar, Uma 


>-Original Message-
>From: Brian Starkey [mailto:brian.star...@arm.com]
>Sent: Tuesday, November 7, 2017 11:40 PM
>To: Shankar, Uma 
>Cc: intel-...@lists.freedesktop.org; Syrjala, Ville ;
>Lankhorst, Maarten ; dri-
>de...@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [RFC 1/7] drm: Add Plane Degamma properties
>
>On Tue, Nov 07, 2017 at 05:49:56PM +, Brian Starkey wrote:
>>
>>In one of the previous discussions[1] related to per-plane color
>>management, Lionel suggested that the 16-bit color lut entries weren't
>>enough when considering HDR.
>>
>>It might be worth creating a new gamma lut format with 32-bit entries
>>for these new properties, as HDR is very much a real rather than
>>hypothetical concern these days.
>>
>>Thanks,
>>-Brian
>
>Sorry, failed to paste the link:
>
>[1] https://patchwork.kernel.org/patch/9546905/

Thanks for the input Brian and sharing the link with earlier discussions around 
this.
I will try to create a separate LUT structure with 32 bit entries which gets 
used for plane color
features. Not sure what to do for pipe level color since we already use u16 
fields. May be the same
color_lut structure (we can call it color_lut2)  can be used for high precision 
use cases like HDR.

Regards,
Uma Shankar


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC 1/7] drm: Add Plane Degamma properties

2017-11-07 Thread Brian Starkey 

On Tue, Nov 07, 2017 at 05:49:56PM +, Brian Starkey wrote:


In one of the previous discussions[1] related to per-plane color
management, Lionel suggested that the 16-bit color lut entries weren't
enough when considering HDR.

It might be worth creating a new gamma lut format with 32-bit entries
for these new properties, as HDR is very much a real rather than
hypothetical concern these days.

Thanks,
-Brian


Sorry, failed to paste the link:

[1] https://patchwork.kernel.org/patch/9546905/
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC 1/7] drm: Add Plane Degamma properties

2017-11-07 Thread Brian Starkey 

Hi,

On Tue, Nov 07, 2017 at 05:36:25PM +0530, Uma Shankar wrote:

Add Plane Degamma as a blob property and plane
degamma size as a range property.

v2: Rebase

Signed-off-by: Uma Shankar 
---
drivers/gpu/drm/drm_atomic.c|   12 
drivers/gpu/drm/drm_atomic_helper.c |6 ++
drivers/gpu/drm/drm_mode_config.c   |   14 ++
include/drm/drm_mode_config.h   |   11 +++
include/drm/drm_plane.h |   10 ++
5 files changed, 53 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7e0e7be..30853c7 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -713,6 +713,8 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
{
struct drm_device *dev = plane->dev;
struct drm_mode_config *config = &dev->mode_config;
+   bool replaced = false;
+   int ret;

if (property == config->prop_fb_id) {
struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, 
val);
@@ -758,6 +760,12 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
+   } else if (property == config->plane_degamma_lut_property) {
+   ret = drm_atomic_replace_property_blob_from_id(dev,
+   &state->degamma_lut,
+   val, -1, &replaced);
+   state->color_mgmt_changed |= replaced;
+   return ret;
} else {
return -EINVAL;
}
@@ -816,6 +824,9 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
*val = state->zpos;
} else if (plane->funcs->atomic_get_property) {
return plane->funcs->atomic_get_property(plane, state, 
property, val);
+   } else if (property == config->plane_degamma_lut_property) {
+   *val = (state->degamma_lut) ?
+   state->degamma_lut->base.id : 0;
} else {
return -EINVAL;
}
@@ -953,6 +964,7 @@ static void drm_atomic_plane_print_state(struct drm_printer 
*p,
drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest));
drm_printf(p, "\tsrc-pos=" DRM_RECT_FP_FMT "\n", DRM_RECT_FP_ARG(&src));
drm_printf(p, "\trotation=%x\n", state->rotation);
+   drm_printf(p, "\tcolor_mgmt_changed=%d\n", state->color_mgmt_changed);

if (plane->funcs->atomic_print_state)
plane->funcs->atomic_print_state(p, state);
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 71d712f..ba924cf 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3395,6 +3395,10 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
drm_plane *plane,

state->fence = NULL;
state->commit = NULL;
+
+   if (state->degamma_lut)
+   drm_property_blob_get(state->degamma_lut);
+   state->color_mgmt_changed = false;
}
EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);

@@ -3439,6 +3443,8 @@ void __drm_atomic_helper_plane_destroy_state(struct 
drm_plane_state *state)

if (state->commit)
drm_crtc_commit_put(state->commit);
+
+   drm_property_blob_put(state->degamma_lut);
}
EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);

diff --git a/drivers/gpu/drm/drm_mode_config.c 
b/drivers/gpu/drm/drm_mode_config.c
index cda8bfa..118f6ac 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -348,6 +348,20 @@ static int drm_mode_create_standard_properties(struct 
drm_device *dev)
return -ENOMEM;
dev->mode_config.modifiers_property = prop;

+   prop = drm_property_create(dev,
+   DRM_MODE_PROP_BLOB,
+   "PLANE_DEGAMMA_LUT", 0);
+   if (!prop)
+   return -ENOMEM;
+   dev->mode_config.plane_degamma_lut_property = prop;
+
+   prop = drm_property_create_range(dev,
+   DRM_MODE_PROP_IMMUTABLE,
+   "PLANE_DEGAMMA_LUT_SIZE", 0, UINT_MAX);
+   if (!prop)
+   return -ENOMEM;
+   dev->mode_config.plane_degamma_lut_size_property = prop;
+
return 0;
}

diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 0b4ac2e..6ee2df6 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -718,6 +718,17 @@ struct drm_mode_config {
struct drm_property *gamma_lut_size_property;

/**
+* @plane_degamma_lut_property: Optional Plane property to set the LUT
+* used to convert the framebuffer's colors to linear gamma.
+*/
+   struct drm_property *plane_degamma_lut_property;
+   /**
+* @plane_

Re: [Intel-gfx] [RFC 1/7] drm: Add Plane Degamma properties

2017-11-07 Thread Emil Velikov 
On 7 November 2017 at 12:06, Uma Shankar  wrote:
> Add Plane Degamma as a blob property and plane
> degamma size as a range property.
>
> v2: Rebase
>
Hi Uma, seems like something has gone wrong during the rebase.

> Signed-off-by: Uma Shankar 
> ---
>  drivers/gpu/drm/drm_atomic.c|   12 
>  drivers/gpu/drm/drm_atomic_helper.c |6 ++
>  drivers/gpu/drm/drm_mode_config.c   |   14 ++
>  include/drm/drm_mode_config.h   |   11 +++
>  include/drm/drm_plane.h |   10 ++
>  5 files changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7e0e7be..30853c7 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -713,6 +713,8 @@ static int drm_atomic_plane_set_property(struct drm_plane 
> *plane,
>  {
> struct drm_device *dev = plane->dev;
> struct drm_mode_config *config = &dev->mode_config;
> +   bool replaced = false;
> +   int ret;
>
> if (property == config->prop_fb_id) {
> struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, 
> NULL, val);
> @@ -758,6 +760,12 @@ static int drm_atomic_plane_set_property(struct 
> drm_plane *plane,
> } else if (plane->funcs->atomic_set_property) {
> return plane->funcs->atomic_set_property(plane, state,
> property, val);
> +   } else if (property == config->plane_degamma_lut_property) {
> +   ret = drm_atomic_replace_property_blob_from_id(dev,
> +   &state->degamma_lut,
> +   val, -1, &replaced);
> +   state->color_mgmt_changed |= replaced;
> +   return ret;
Namely: the driver specific atomic_set_property will be called and the
newly added code will not be reached.
I think we should keep the atomic_set_property call last in the
if/else chain. Converting the lot to a switch statement might make
things a bit more obvious.


> } else {
> return -EINVAL;
> }
> @@ -816,6 +824,9 @@ static int drm_atomic_plane_set_property(struct drm_plane 
> *plane,
> *val = state->zpos;
> } else if (plane->funcs->atomic_get_property) {
> return plane->funcs->atomic_get_property(plane, state, 
> property, val);
> +   } else if (property == config->plane_degamma_lut_property) {
> +   *val = (state->degamma_lut) ?
> +   state->degamma_lut->base.id : 0;
Analogous thing happens here.

Did you test the updated series through IGT - it should have caught
the above (considering we have tests, and I'm not loosing my marbles).
Same comments apply for CTM and gamma, patches 2 and 3 respectively.

HTH
Emil
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 1/7] drm: Add Plane Degamma properties

2017-11-07 Thread Uma Shankar 
Add Plane Degamma as a blob property and plane
degamma size as a range property.

v2: Rebase

Signed-off-by: Uma Shankar 
---
 drivers/gpu/drm/drm_atomic.c|   12 
 drivers/gpu/drm/drm_atomic_helper.c |6 ++
 drivers/gpu/drm/drm_mode_config.c   |   14 ++
 include/drm/drm_mode_config.h   |   11 +++
 include/drm/drm_plane.h |   10 ++
 5 files changed, 53 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7e0e7be..30853c7 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -713,6 +713,8 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
 {
struct drm_device *dev = plane->dev;
struct drm_mode_config *config = &dev->mode_config;
+   bool replaced = false;
+   int ret;
 
if (property == config->prop_fb_id) {
struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, 
val);
@@ -758,6 +760,12 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
+   } else if (property == config->plane_degamma_lut_property) {
+   ret = drm_atomic_replace_property_blob_from_id(dev,
+   &state->degamma_lut,
+   val, -1, &replaced);
+   state->color_mgmt_changed |= replaced;
+   return ret;
} else {
return -EINVAL;
}
@@ -816,6 +824,9 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
*val = state->zpos;
} else if (plane->funcs->atomic_get_property) {
return plane->funcs->atomic_get_property(plane, state, 
property, val);
+   } else if (property == config->plane_degamma_lut_property) {
+   *val = (state->degamma_lut) ?
+   state->degamma_lut->base.id : 0;
} else {
return -EINVAL;
}
@@ -953,6 +964,7 @@ static void drm_atomic_plane_print_state(struct drm_printer 
*p,
drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest));
drm_printf(p, "\tsrc-pos=" DRM_RECT_FP_FMT "\n", DRM_RECT_FP_ARG(&src));
drm_printf(p, "\trotation=%x\n", state->rotation);
+   drm_printf(p, "\tcolor_mgmt_changed=%d\n", state->color_mgmt_changed);
 
if (plane->funcs->atomic_print_state)
plane->funcs->atomic_print_state(p, state);
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 71d712f..ba924cf 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3395,6 +3395,10 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
drm_plane *plane,
 
state->fence = NULL;
state->commit = NULL;
+
+   if (state->degamma_lut)
+   drm_property_blob_get(state->degamma_lut);
+   state->color_mgmt_changed = false;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
 
@@ -3439,6 +3443,8 @@ void __drm_atomic_helper_plane_destroy_state(struct 
drm_plane_state *state)
 
if (state->commit)
drm_crtc_commit_put(state->commit);
+
+   drm_property_blob_put(state->degamma_lut);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
 
diff --git a/drivers/gpu/drm/drm_mode_config.c 
b/drivers/gpu/drm/drm_mode_config.c
index cda8bfa..118f6ac 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -348,6 +348,20 @@ static int drm_mode_create_standard_properties(struct 
drm_device *dev)
return -ENOMEM;
dev->mode_config.modifiers_property = prop;
 
+   prop = drm_property_create(dev,
+   DRM_MODE_PROP_BLOB,
+   "PLANE_DEGAMMA_LUT", 0);
+   if (!prop)
+   return -ENOMEM;
+   dev->mode_config.plane_degamma_lut_property = prop;
+
+   prop = drm_property_create_range(dev,
+   DRM_MODE_PROP_IMMUTABLE,
+   "PLANE_DEGAMMA_LUT_SIZE", 0, UINT_MAX);
+   if (!prop)
+   return -ENOMEM;
+   dev->mode_config.plane_degamma_lut_size_property = prop;
+
return 0;
 }
 
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 0b4ac2e..6ee2df6 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -718,6 +718,17 @@ struct drm_mode_config {
struct drm_property *gamma_lut_size_property;
 
/**
+* @plane_degamma_lut_property: Optional Plane property to set the LUT
+* used to convert the framebuffer's colors to linear gamma.
+*/
+   struct drm_property *plane_degamma_lut_property;
+   /**
+* @plane_degamma_lut_size_property: Optional Plane prope