[10/23] drm/i915: Add gamma correction handlers

Commit Message

Sharma, Shashank Sept. 16, 2015, 5:37 p.m. UTC

I915 driver registers gamma correction as palette correction
property with DRM layer. This patch adds set_property() and get_property()
handlers for pipe level gamma correction.

The set function attaches the Gamma correction blob to CRTC state, these
values will be committed during atomic commit.

Signed-off-by: Shashank Sharma <[e-mail]>
Signed-off-by: Kausal Malladi <[e-mail]>
---
 drivers/gpu/drm/i915/intel_atomic.c        | 20 ++++++++++++++++++++
 drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h           |  5 +++++
 3 files changed, 46 insertions(+)

Comments

Daniel Vetter Sept. 22, 2015, 1:15 p.m. UTC | #1

On Wed, Sep 16, 2015 at 11:07:07PM +0530, Shashank Sharma wrote:
> I915 driver registers gamma correction as palette correction
> property with DRM layer. This patch adds set_property() and get_property()
> handlers for pipe level gamma correction.
> 
> The set function attaches the Gamma correction blob to CRTC state, these
> values will be committed during atomic commit.
> 
> Signed-off-by: Shashank Sharma <[e-mail]>
> Signed-off-by: Kausal Malladi <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c        | 20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h           |  5 +++++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 500d2998..0b61fef 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>  				   struct drm_property *property,
>  				   uint64_t val)
>  {
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->cm_palette_after_ctm_property)
> +		return intel_color_manager_set_pipe_gamma(dev, state,
> +				&crtc->base, val);
> +
>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>  	return -EINVAL;
>  }
> @@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
>  				   struct drm_property *property,
>  				   uint64_t *val)
>  {
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->cm_palette_after_ctm_property) {
> +		*val = (state->palette_after_ctm_blob) ?
> +			state->palette_after_ctm_blob->base.id : 0;
> +		goto found;
> +	}

Since color manager properties are meant as a new standardize KMS
extension (we put them into the core drm_crtc_state) the get/set support
should also be in the core. See e.g. how the rotation property is handled
in drm_atomic_plane_get/set_property. So all this code should be added to
drm_atomic_crtc_get/set_property.


> +
>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>  	return -EINVAL;
> +
> +found:
> +	DRM_DEBUG_KMS("Found property %s\n", property->name);
> +	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> index 77f58f2..9421bb6 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,27 @@
>  
>  #include "intel_color_manager.h"
>  
> +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> +		struct drm_crtc_state *crtc_state,
> +		struct drm_mode_object *obj, uint32_t blob_id)
> +{
> +	struct drm_property_blob *blob;
> +
> +	blob = drm_property_lookup_blob(dev, blob_id);
> +	if (!blob) {
> +		DRM_DEBUG_KMS("Invalid Blob ID\n");
> +		return -EINVAL;
> +	}
> +
> +	if (crtc_state->palette_after_ctm_blob)
> +		drm_property_unreference_blob(
> +			crtc_state->palette_after_ctm_blob);
> +
> +	/* Attach the blob to be committed in state */
> +	crtc_state->palette_after_ctm_blob = blob;
> +	return 0;
> +}

What is this used for? It looks a bit like legacy property code, and we
have a generic helper to make that happen
(drm_atomic_helper_crtc_set_property).

Generally please don't add functions/structs without also adding a user,
it means that reviewers have to constantly jump around in your patch
series to figure out how something is used. Instead if you want to split
things up really fine add a stub function frist (but including relevant
callers) and then fill out the bits separately.
-Daniel

> +
>  int get_pipe_capabilities(struct drm_device *dev,
>  		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e27e754..d0193e2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1466,4 +1466,9 @@ void intel_plane_destroy_state(struct drm_plane *plane,
>  			       struct drm_plane_state *state);
>  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>  
> +/* intel_color_manager.c */
> +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> +		struct drm_crtc_state *crtc_state,
> +		struct drm_mode_object *obj, uint32_t blob_id);
> +
>  #endif /* __INTEL_DRV_H__ */
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter Sept. 22, 2015, 1:19 p.m. UTC | #2

On Tue, Sep 22, 2015 at 03:15:11PM +0200, Daniel Vetter wrote:
> On Wed, Sep 16, 2015 at 11:07:07PM +0530, Shashank Sharma wrote:
> > I915 driver registers gamma correction as palette correction
> > property with DRM layer. This patch adds set_property() and get_property()
> > handlers for pipe level gamma correction.
> > 
> > The set function attaches the Gamma correction blob to CRTC state, these
> > values will be committed during atomic commit.
> > 
> > Signed-off-by: Shashank Sharma <[e-mail]>
> > Signed-off-by: Kausal Malladi <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic.c        | 20 ++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h           |  5 +++++
> >  3 files changed, 46 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> > index 500d2998..0b61fef 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
> >  				   struct drm_property *property,
> >  				   uint64_t val)
> >  {
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_mode_config *config = &dev->mode_config;
> > +
> > +	if (property == config->cm_palette_after_ctm_property)
> > +		return intel_color_manager_set_pipe_gamma(dev, state,
> > +				&crtc->base, val);
> > +
> >  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> >  	return -EINVAL;
> >  }
> > @@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
> >  				   struct drm_property *property,
> >  				   uint64_t *val)
> >  {
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_mode_config *config = &dev->mode_config;
> > +
> > +	if (property == config->cm_palette_after_ctm_property) {
> > +		*val = (state->palette_after_ctm_blob) ?
> > +			state->palette_after_ctm_blob->base.id : 0;
> > +		goto found;
> > +	}
> 
> Since color manager properties are meant as a new standardize KMS
> extension (we put them into the core drm_crtc_state) the get/set support
> should also be in the core. See e.g. how the rotation property is handled
> in drm_atomic_plane_get/set_property. So all this code should be added to
> drm_atomic_crtc_get/set_property.

I've forgotten to explain how drivers can then opt-in KMS extensions if we
have the decode support unconditionally there and also register the props
unconditionally: That's done by only attaching these standardized props to
a crtc/plane if the corresponding object supports it.
-Daniel

> 
> 
> > +
> >  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> >  	return -EINVAL;
> > +
> > +found:
> > +	DRM_DEBUG_KMS("Found property %s\n", property->name);
> > +	return 0;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> > index 77f58f2..9421bb6 100644
> > --- a/drivers/gpu/drm/i915/intel_color_manager.c
> > +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> > @@ -27,6 +27,27 @@
> >  
> >  #include "intel_color_manager.h"
> >  
> > +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> > +		struct drm_crtc_state *crtc_state,
> > +		struct drm_mode_object *obj, uint32_t blob_id)
> > +{
> > +	struct drm_property_blob *blob;
> > +
> > +	blob = drm_property_lookup_blob(dev, blob_id);
> > +	if (!blob) {
> > +		DRM_DEBUG_KMS("Invalid Blob ID\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (crtc_state->palette_after_ctm_blob)
> > +		drm_property_unreference_blob(
> > +			crtc_state->palette_after_ctm_blob);
> > +
> > +	/* Attach the blob to be committed in state */
> > +	crtc_state->palette_after_ctm_blob = blob;
> > +	return 0;
> > +}
> 
> What is this used for? It looks a bit like legacy property code, and we
> have a generic helper to make that happen
> (drm_atomic_helper_crtc_set_property).
> 
> Generally please don't add functions/structs without also adding a user,
> it means that reviewers have to constantly jump around in your patch
> series to figure out how something is used. Instead if you want to split
> things up really fine add a stub function frist (but including relevant
> callers) and then fill out the bits separately.
> -Daniel
> 
> > +
> >  int get_pipe_capabilities(struct drm_device *dev,
> >  		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index e27e754..d0193e2 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1466,4 +1466,9 @@ void intel_plane_destroy_state(struct drm_plane *plane,
> >  			       struct drm_plane_state *state);
> >  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> >  
> > +/* intel_color_manager.c */
> > +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> > +		struct drm_crtc_state *crtc_state,
> > +		struct drm_mode_object *obj, uint32_t blob_id);
> > +
> >  #endif /* __INTEL_DRV_H__ */
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Sharma, Shashank Sept. 23, 2015, 8:22 a.m. UTC | #3

Regards
Shashank

On 9/22/2015 6:45 PM, Daniel Vetter wrote:
> On Wed, Sep 16, 2015 at 11:07:07PM +0530, Shashank Sharma wrote:
>> I915 driver registers gamma correction as palette correction
>> property with DRM layer. This patch adds set_property() and get_property()
>> handlers for pipe level gamma correction.
>>
>> The set function attaches the Gamma correction blob to CRTC state, these
>> values will be committed during atomic commit.
>>
>> Signed-off-by: Shashank Sharma <[e-mail]>
>> Signed-off-by: Kausal Malladi <[e-mail]>
>> ---
>>   drivers/gpu/drm/i915/intel_atomic.c        | 20 ++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h           |  5 +++++
>>   3 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 500d2998..0b61fef 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>>   				   struct drm_property *property,
>>   				   uint64_t val)
>>   {
>> +	struct drm_device *dev = crtc->dev;
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +
>> +	if (property == config->cm_palette_after_ctm_property)
>> +		return intel_color_manager_set_pipe_gamma(dev, state,
>> +				&crtc->base, val);
>> +
>>   	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>>   	return -EINVAL;
>>   }
>> @@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
>>   				   struct drm_property *property,
>>   				   uint64_t *val)
>>   {
>> +	struct drm_device *dev = crtc->dev;
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +
>> +	if (property == config->cm_palette_after_ctm_property) {
>> +		*val = (state->palette_after_ctm_blob) ?
>> +			state->palette_after_ctm_blob->base.id : 0;
>> +		goto found;
>> +	}
>
> Since color manager properties are meant as a new standardize KMS
> extension (we put them into the core drm_crtc_state) the get/set support
> should also be in the core. See e.g. how the rotation property is handled
> in drm_atomic_plane_get/set_property. So all this code should be added to
> drm_atomic_crtc_get/set_property.
Thanks, sounds like a good one. Will move this.
>
>
>> +
>>   	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>>   	return -EINVAL;
>> +
>> +found:
>> +	DRM_DEBUG_KMS("Found property %s\n", property->name);
>> +	return 0;
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
>> index 77f58f2..9421bb6 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -27,6 +27,27 @@
>>
>>   #include "intel_color_manager.h"
>>
>> +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
>> +		struct drm_crtc_state *crtc_state,
>> +		struct drm_mode_object *obj, uint32_t blob_id)
>> +{
>> +	struct drm_property_blob *blob;
>> +
>> +	blob = drm_property_lookup_blob(dev, blob_id);
>> +	if (!blob) {
>> +		DRM_DEBUG_KMS("Invalid Blob ID\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (crtc_state->palette_after_ctm_blob)
>> +		drm_property_unreference_blob(
>> +			crtc_state->palette_after_ctm_blob);
>> +
>> +	/* Attach the blob to be committed in state */
>> +	crtc_state->palette_after_ctm_blob = blob;
>> +	return 0;
>> +}
>
> What is this used for? It looks a bit like legacy property code, and we
> have a generic helper to make that happen
> (drm_atomic_helper_crtc_set_property).
>
No Daniel, its not the legacy part. Please check atomic_begin() part in 
the next patches. This is like the top half of atomic set_property for 
CRTC and will be applicable for any property. As you already know, to 
set a property:
- userspace creates a blob, and sends the blob id via set_property() 
interface
- set_property() saves the blob_id in the corresponding pointer space in 
the crtc_state()
- From the atomic_commit_begin() we will extract the values from blob 
and start committing to the registers. Its kind of bottom_half() of 
atomic set_property for CRTC.
> Generally please don't add functions/structs without also adding a user,
> it means that reviewers have to constantly jump around in your patch
> series to figure out how something is used. Instead if you want to split
> things up really fine add a stub function frist (but including relevant
> callers) and then fill out the bits separately.
I think the above explanation should make it clear.
> -Daniel
>
>> +
>>   int get_pipe_capabilities(struct drm_device *dev,
>>   		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
>>   {
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index e27e754..d0193e2 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1466,4 +1466,9 @@ void intel_plane_destroy_state(struct drm_plane *plane,
>>   			       struct drm_plane_state *state);
>>   extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>>
>> +/* intel_color_manager.c */
>> +int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
>> +		struct drm_crtc_state *crtc_state,
>> +		struct drm_mode_object *obj, uint32_t blob_id);
>> +
>>   #endif /* __INTEL_DRV_H__ */
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> [e-mail]
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Daniel Vetter Sept. 23, 2015, 1:02 p.m. UTC | #4

On Wed, Sep 23, 2015 at 01:52:21PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 9/22/2015 6:45 PM, Daniel Vetter wrote:
> >On Wed, Sep 16, 2015 at 11:07:07PM +0530, Shashank Sharma wrote:
> >>I915 driver registers gamma correction as palette correction
> >>property with DRM layer. This patch adds set_property() and get_property()
> >>handlers for pipe level gamma correction.
> >>
> >>The set function attaches the Gamma correction blob to CRTC state, these
> >>values will be committed during atomic commit.
> >>
> >>Signed-off-by: Shashank Sharma <[e-mail]>
> >>Signed-off-by: Kausal Malladi <[e-mail]>
> >>---
> >>  drivers/gpu/drm/i915/intel_atomic.c        | 20 ++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_drv.h           |  5 +++++
> >>  3 files changed, 46 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> >>index 500d2998..0b61fef 100644
> >>--- a/drivers/gpu/drm/i915/intel_atomic.c
> >>+++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>@@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
> >>  				   struct drm_property *property,
> >>  				   uint64_t val)
> >>  {
> >>+	struct drm_device *dev = crtc->dev;
> >>+	struct drm_mode_config *config = &dev->mode_config;
> >>+
> >>+	if (property == config->cm_palette_after_ctm_property)
> >>+		return intel_color_manager_set_pipe_gamma(dev, state,
> >>+				&crtc->base, val);
> >>+
> >>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> >>  	return -EINVAL;
> >>  }
> >>@@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
> >>  				   struct drm_property *property,
> >>  				   uint64_t *val)
> >>  {
> >>+	struct drm_device *dev = crtc->dev;
> >>+	struct drm_mode_config *config = &dev->mode_config;
> >>+
> >>+	if (property == config->cm_palette_after_ctm_property) {
> >>+		*val = (state->palette_after_ctm_blob) ?
> >>+			state->palette_after_ctm_blob->base.id : 0;
> >>+		goto found;
> >>+	}
> >
> >Since color manager properties are meant as a new standardize KMS
> >extension (we put them into the core drm_crtc_state) the get/set support
> >should also be in the core. See e.g. how the rotation property is handled
> >in drm_atomic_plane_get/set_property. So all this code should be added to
> >drm_atomic_crtc_get/set_property.
> Thanks, sounds like a good one. Will move this.

When moving this please don't forget to add the blob->length sanity checks
mentioned in another thread in this discussion.

> >
> >
> >>+
> >>  	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> >>  	return -EINVAL;
> >>+
> >>+found:
> >>+	DRM_DEBUG_KMS("Found property %s\n", property->name);
> >>+	return 0;
> >>  }
> >>diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
> >>index 77f58f2..9421bb6 100644
> >>--- a/drivers/gpu/drm/i915/intel_color_manager.c
> >>+++ b/drivers/gpu/drm/i915/intel_color_manager.c
> >>@@ -27,6 +27,27 @@
> >>
> >>  #include "intel_color_manager.h"
> >>
> >>+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> >>+		struct drm_crtc_state *crtc_state,
> >>+		struct drm_mode_object *obj, uint32_t blob_id)
> >>+{
> >>+	struct drm_property_blob *blob;
> >>+
> >>+	blob = drm_property_lookup_blob(dev, blob_id);
> >>+	if (!blob) {
> >>+		DRM_DEBUG_KMS("Invalid Blob ID\n");
> >>+		return -EINVAL;
> >>+	}
> >>+
> >>+	if (crtc_state->palette_after_ctm_blob)
> >>+		drm_property_unreference_blob(
> >>+			crtc_state->palette_after_ctm_blob);
> >>+
> >>+	/* Attach the blob to be committed in state */
> >>+	crtc_state->palette_after_ctm_blob = blob;
> >>+	return 0;
> >>+}
> >
> >What is this used for? It looks a bit like legacy property code, and we
> >have a generic helper to make that happen
> >(drm_atomic_helper_crtc_set_property).
> >
> No Daniel, its not the legacy part. Please check atomic_begin() part in the
> next patches. This is like the top half of atomic set_property for CRTC and
> will be applicable for any property. As you already know, to set a property:
> - userspace creates a blob, and sends the blob id via set_property()
> interface
> - set_property() saves the blob_id in the corresponding pointer space in the
> crtc_state()
> - From the atomic_commit_begin() we will extract the values from blob and
> start committing to the registers. Its kind of bottom_half() of atomic
> set_property for CRTC.
> >Generally please don't add functions/structs without also adding a user,
> >it means that reviewers have to constantly jump around in your patch
> >series to figure out how something is used. Instead if you want to split
> >things up really fine add a stub function frist (but including relevant
> >callers) and then fill out the bits separately.
> I think the above explanation should make it clear.

In my defence I'd like to say that I was still jetlagged ;-) Reading
things now again it all looks good.
-Daniel

Sharma, Shashank Sept. 26, 2015, 3:48 p.m. UTC | #5

On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
>> Since color manager properties are meant as a new standardize KMS
>> extension (we put them into the core drm_crtc_state) the get/set support
>> should also be in the core. See e.g. how the rotation property is handled
>> in drm_atomic_plane_get/set_property. So all this code should be added to
>> drm_atomic_crtc_get/set_property.
> Thanks, sounds like a good one. Will move this.
Actually, while implementing this, I realized that this change is not 
required.
What we want to do in drm_atomic_crtc_get/set code is:
if (prop == config->cm_palette_after_ctm_property || prop == 	
	config->cm_palette_before_ctm_property) {
	crtc->funcs->atomic_get_property();
}

Which is already being done in the current code:
else if (crtc->funcs->atomic_get_property)
	return crtc->funcs->atomic_get_property(crtc, state, property, val);

so I dont really think we need this change.

Regards
Shashank

Daniel Vetter Sept. 28, 2015, 6:43 a.m. UTC | #6

On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
> On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
> >>Since color manager properties are meant as a new standardize KMS
> >>extension (we put them into the core drm_crtc_state) the get/set support
> >>should also be in the core. See e.g. how the rotation property is handled
> >>in drm_atomic_plane_get/set_property. So all this code should be added to
> >>drm_atomic_crtc_get/set_property.
> >Thanks, sounds like a good one. Will move this.
> Actually, while implementing this, I realized that this change is not
> required.
> What we want to do in drm_atomic_crtc_get/set code is:
> if (prop == config->cm_palette_after_ctm_property || prop == 	
> 	config->cm_palette_before_ctm_property) {
> 	crtc->funcs->atomic_get_property();
> }
> 
> Which is already being done in the current code:
> else if (crtc->funcs->atomic_get_property)
> 	return crtc->funcs->atomic_get_property(crtc, state, property, val);

This code is to pass any property unknown to the drm core into the driver.
But since we want this to be a new drm core property set (that's why it's
in drm_crtc_state) the decoding should be done in the core too.

Note that atomic_get/set_property _only_ map between the property as seen
by userspace and the state structures. They're not allowed to do anything
else like compute derived state, check constraints or put the state into
the hw. That's for the atomic_check and atomic_commit callbacks. So for
this patchset here you should move all the code in the
atomic_get/set_property callbacks you add in i915 into the drm core. Like
it is doen for the rotation property.
-Daniel

Sharma, Shashank Sept. 28, 2015, 8:19 a.m. UTC | #7

Matt, your opinion about this ? 

Regards
Shashank
-----Original Message-----
From: Daniel Vetter [mailto:[e-mail]] On Behalf Of Daniel Vetter
Sent: Monday, September 28, 2015 12:14 PM
To: Sharma, Shashank
Cc: Daniel Vetter; Roper, Matthew D; Bish, Jim; Bradford, Robert; Smith, Gary K; [e-mail]; [e-mail]; Matheson, Annie J; [e-mail]; Vetter, Daniel
Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers

On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
> On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
> >>Since color manager properties are meant as a new standardize KMS 
> >>extension (we put them into the core drm_crtc_state) the get/set 
> >>support should also be in the core. See e.g. how the rotation 
> >>property is handled in drm_atomic_plane_get/set_property. So all 
> >>this code should be added to drm_atomic_crtc_get/set_property.
> >Thanks, sounds like a good one. Will move this.
> Actually, while implementing this, I realized that this change is not 
> required.
> What we want to do in drm_atomic_crtc_get/set code is:
> if (prop == config->cm_palette_after_ctm_property || prop == 	
> 	config->cm_palette_before_ctm_property) {
> 	crtc->funcs->atomic_get_property();
> }
> 
> Which is already being done in the current code:
> else if (crtc->funcs->atomic_get_property)
> 	return crtc->funcs->atomic_get_property(crtc, state, property, val);

This code is to pass any property unknown to the drm core into the driver.
But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.

Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Matt Roper Sept. 28, 2015, 9:42 p.m. UTC | #8

Yep, Daniel's right; for properties that are not specific to the driver,
the core core should take care of stuffing the values provided by
userspace into the state structures so that every driver that wants to
use these doesn't need to replicate that logic.  My bad for not catching
this in my earlier reviews; sorry about that.  Basically the change
required is to add another else clause to
drm_atomic_crtc_{get,set}_property(), before the core tries to call into
the driver-specific handler.  You'll notice that what you're doing with
color management blobs here is actually very similar to what's already
done for the existing mode blob, so you can take a look at that case for
an example.

Also, the functions like intel_color_manager_set_pipe_gamma() will get
moved into the DRM core and the "intel_" prefix will switch to "drm_"
since there's really nothing Intel-specific about what they're doing.

Sorry again for not noticing this and bringing it up on the earlier
reviews.  Fortunately the changes required should be pretty small.


Matt

On Mon, Sep 28, 2015 at 01:19:13AM -0700, Sharma, Shashank wrote:
> Matt, your opinion about this ? 
> 
> Regards
> Shashank
> -----Original Message-----
> From: Daniel Vetter [mailto:[e-mail]] On Behalf Of Daniel Vetter
> Sent: Monday, September 28, 2015 12:14 PM
> To: Sharma, Shashank
> Cc: Daniel Vetter; Roper, Matthew D; Bish, Jim; Bradford, Robert; Smith, Gary K; [e-mail]; [e-mail]; Matheson, Annie J; [e-mail]; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers
> 
> On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
> > On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
> > >>Since color manager properties are meant as a new standardize KMS 
> > >>extension (we put them into the core drm_crtc_state) the get/set 
> > >>support should also be in the core. See e.g. how the rotation 
> > >>property is handled in drm_atomic_plane_get/set_property. So all 
> > >>this code should be added to drm_atomic_crtc_get/set_property.
> > >Thanks, sounds like a good one. Will move this.
> > Actually, while implementing this, I realized that this change is not 
> > required.
> > What we want to do in drm_atomic_crtc_get/set code is:
> > if (prop == config->cm_palette_after_ctm_property || prop == 	
> > 	config->cm_palette_before_ctm_property) {
> > 	crtc->funcs->atomic_get_property();
> > }
> > 
> > Which is already being done in the current code:
> > else if (crtc->funcs->atomic_get_property)
> > 	return crtc->funcs->atomic_get_property(crtc, state, property, val);
> 
> This code is to pass any property unknown to the drm core into the driver.
> But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.
> 
> Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Sharma, Shashank Sept. 29, 2015, 4:29 a.m. UTC | #9

Ok, anyways it was sounding like a good idea. 
Will do the changes accordingly. 

Regards
Shashank
-----Original Message-----
From: Roper, Matthew D 
Sent: Tuesday, September 29, 2015 3:13 AM
To: Sharma, Shashank
Cc: Daniel Vetter; Bish, Jim; Bradford, Robert; Smith, Gary K; [e-mail]; [e-mail]; Matheson, Annie J; [e-mail]; Vetter, Daniel
Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers

Yep, Daniel's right; for properties that are not specific to the driver, the core core should take care of stuffing the values provided by userspace into the state structures so that every driver that wants to use these doesn't need to replicate that logic.  My bad for not catching this in my earlier reviews; sorry about that.  Basically the change required is to add another else clause to drm_atomic_crtc_{get,set}_property(), before the core tries to call into the driver-specific handler.  You'll notice that what you're doing with color management blobs here is actually very similar to what's already done for the existing mode blob, so you can take a look at that case for an example.

Also, the functions like intel_color_manager_set_pipe_gamma() will get moved into the DRM core and the "intel_" prefix will switch to "drm_"
since there's really nothing Intel-specific about what they're doing.

Sorry again for not noticing this and bringing it up on the earlier reviews.  Fortunately the changes required should be pretty small.


Matt

On Mon, Sep 28, 2015 at 01:19:13AM -0700, Sharma, Shashank wrote:
> Matt, your opinion about this ? 
> 
> Regards
> Shashank
> -----Original Message-----
> From: Daniel Vetter [mailto:[e-mail]] On Behalf Of 
> Daniel Vetter
> Sent: Monday, September 28, 2015 12:14 PM
> To: Sharma, Shashank
> Cc: Daniel Vetter; Roper, Matthew D; Bish, Jim; Bradford, Robert; 
> Smith, Gary K; [e-mail]; 
> [e-mail]; Matheson, Annie J; 
> [e-mail]; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction 
> handlers
> 
> On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
> > On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
> > >>Since color manager properties are meant as a new standardize KMS 
> > >>extension (we put them into the core drm_crtc_state) the get/set 
> > >>support should also be in the core. See e.g. how the rotation 
> > >>property is handled in drm_atomic_plane_get/set_property. So all 
> > >>this code should be added to drm_atomic_crtc_get/set_property.
> > >Thanks, sounds like a good one. Will move this.
> > Actually, while implementing this, I realized that this change is 
> > not required.
> > What we want to do in drm_atomic_crtc_get/set code is:
> > if (prop == config->cm_palette_after_ctm_property || prop == 	
> > 	config->cm_palette_before_ctm_property) {
> > 	crtc->funcs->atomic_get_property();
> > }
> > 
> > Which is already being done in the current code:
> > else if (crtc->funcs->atomic_get_property)
> > 	return crtc->funcs->atomic_get_property(crtc, state, property, 
> > val);
> 
> This code is to pass any property unknown to the drm core into the driver.
> But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.
> 
> Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
[phone]

Matheson, Annie J Sept. 29, 2015, 4:29 a.m. UTC | #10

Thank you Shashank! 

Annie Matheson

> On Sep 28, 2015, at 9:29 PM, Sharma, Shashank <[e-mail]> wrote:
> 
> Ok, anyways it was sounding like a good idea. 
> Will do the changes accordingly. 
> 
> Regards
> Shashank
> -----Original Message-----
> From: Roper, Matthew D 
> Sent: Tuesday, September 29, 2015 3:13 AM
> To: Sharma, Shashank
> Cc: Daniel Vetter; Bish, Jim; Bradford, Robert; Smith, Gary K; [e-mail]; [e-mail]; Matheson, Annie J; [e-mail]; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction handlers
> 
> Yep, Daniel's right; for properties that are not specific to the driver, the core core should take care of stuffing the values provided by userspace into the state structures so that every driver that wants to use these doesn't need to replicate that logic.  My bad for not catching this in my earlier reviews; sorry about that.  Basically the change required is to add another else clause to drm_atomic_crtc_{get,set}_property(), before the core tries to call into the driver-specific handler.  You'll notice that what you're doing with color management blobs here is actually very similar to what's already done for the existing mode blob, so you can take a look at that case for an example.
> 
> Also, the functions like intel_color_manager_set_pipe_gamma() will get moved into the DRM core and the "intel_" prefix will switch to "drm_"
> since there's really nothing Intel-specific about what they're doing.
> 
> Sorry again for not noticing this and bringing it up on the earlier reviews.  Fortunately the changes required should be pretty small.
> 
> 
> Matt
> 
>> On Mon, Sep 28, 2015 at 01:19:13AM -0700, Sharma, Shashank wrote:
>> Matt, your opinion about this ? 
>> 
>> Regards
>> Shashank
>> -----Original Message-----
>> From: Daniel Vetter [mailto:[e-mail]] On Behalf Of 
>> Daniel Vetter
>> Sent: Monday, September 28, 2015 12:14 PM
>> To: Sharma, Shashank
>> Cc: Daniel Vetter; Roper, Matthew D; Bish, Jim; Bradford, Robert; 
>> Smith, Gary K; [e-mail]; 
>> [e-mail]; Matheson, Annie J; 
>> [e-mail]; Vetter, Daniel
>> Subject: Re: [Intel-gfx] [PATCH 10/23] drm/i915: Add gamma correction 
>> handlers
>> 
>>> On Sat, Sep 26, 2015 at 09:18:48PM +0530, Sharma, Shashank wrote:
>>> On 9/23/2015 1:52 PM, Sharma, Shashank wrote:
>>>>> Since color manager properties are meant as a new standardize KMS 
>>>>> extension (we put them into the core drm_crtc_state) the get/set 
>>>>> support should also be in the core. See e.g. how the rotation 
>>>>> property is handled in drm_atomic_plane_get/set_property. So all 
>>>>> this code should be added to drm_atomic_crtc_get/set_property.
>>>> Thanks, sounds like a good one. Will move this.
>>> Actually, while implementing this, I realized that this change is 
>>> not required.
>>> What we want to do in drm_atomic_crtc_get/set code is:
>>> if (prop == config->cm_palette_after_ctm_property || prop ==    
>>>    config->cm_palette_before_ctm_property) {
>>>    crtc->funcs->atomic_get_property();
>>> }
>>> 
>>> Which is already being done in the current code:
>>> else if (crtc->funcs->atomic_get_property)
>>>    return crtc->funcs->atomic_get_property(crtc, state, property, 
>>> val);
>> 
>> This code is to pass any property unknown to the drm core into the driver.
>> But since we want this to be a new drm core property set (that's why it's in drm_crtc_state) the decoding should be done in the core too.
>> 
>> Note that atomic_get/set_property _only_ map between the property as seen by userspace and the state structures. They're not allowed to do anything else like compute derived state, check constraints or put the state into the hw. That's for the atomic_check and atomic_commit callbacks. So for this patchset here you should move all the code in the atomic_get/set_property callbacks you add in i915 into the drm core. Like it is doen for the rotation property.
>> -Daniel
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
> 
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> [phone]

Patch

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 500d2998..0b61fef 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -315,6 +315,13 @@  int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
 				   struct drm_property *property,
 				   uint64_t val)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (property == config->cm_palette_after_ctm_property)
+		return intel_color_manager_set_pipe_gamma(dev, state,
+				&crtc->base, val);
+
 	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
 	return -EINVAL;
 }
@@ -324,6 +331,19 @@  int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
 				   struct drm_property *property,
 				   uint64_t *val)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (property == config->cm_palette_after_ctm_property) {
+		*val = (state->palette_after_ctm_blob) ?
+			state->palette_after_ctm_blob->base.id : 0;
+		goto found;
+	}
+
 	DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
 	return -EINVAL;
+
+found:
+	DRM_DEBUG_KMS("Found property %s\n", property->name);
+	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_color_manager.c b/drivers/gpu/drm/i915/intel_color_manager.c
index 77f58f2..9421bb6 100644
--- a/drivers/gpu/drm/i915/intel_color_manager.c
+++ b/drivers/gpu/drm/i915/intel_color_manager.c
@@ -27,6 +27,27 @@ 
 
 #include "intel_color_manager.h"
 
+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id)
+{
+	struct drm_property_blob *blob;
+
+	blob = drm_property_lookup_blob(dev, blob_id);
+	if (!blob) {
+		DRM_DEBUG_KMS("Invalid Blob ID\n");
+		return -EINVAL;
+	}
+
+	if (crtc_state->palette_after_ctm_blob)
+		drm_property_unreference_blob(
+			crtc_state->palette_after_ctm_blob);
+
+	/* Attach the blob to be committed in state */
+	crtc_state->palette_after_ctm_blob = blob;
+	return 0;
+}
+
 int get_pipe_capabilities(struct drm_device *dev,
 		struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e27e754..d0193e2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1466,4 +1466,9 @@  void intel_plane_destroy_state(struct drm_plane *plane,
 			       struct drm_plane_state *state);
 extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
 
+/* intel_color_manager.c */
+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
+		struct drm_crtc_state *crtc_state,
+		struct drm_mode_object *obj, uint32_t blob_id);
+
 #endif /* __INTEL_DRV_H__ */