Re: [PATCH v2 04/10] drm: Add Gamma correction structure

["Sharma, Shashank" <shashank.sharma@intel.com>]

Regards
Shashank

On 6/6/2015 6:30 AM, Matt Roper wrote:
> On Thu, Jun 04, 2015 at 07:12:35PM +0530, Kausal Malladi wrote:
>> From: Kausal Malladi <Kausal.Malladi@intel.com>
>>
>> This patch adds a new structure in DRM layer for Gamma color correction.
>> This structure will be used by all user space agents to configure
>> appropriate Gamma precision and Gamma level.
>>
>> struct drm_intel_gamma {
>>         __u32 gamma_level;
>> 	(The gamma_level variable indicates if the Gamma correction is to be
>> 	applied on Pipe/plane)
>
> I'm not sure I understand the need for this one...you're getting the
> set_property call against a specific DRM object, so I don't think there
> should be any confusion at that point about what the values apply to?
>
Actually that is for the get_property path. If you have a look at the 
design document, there is a provision to query the current applied gamma 
which can be pipe/plane level. Hence keeping this.
>
>>         __u32 gamma_precision;
>> 	(The Gamma precision indicates the Gamma mode to be applied)
>>
>> 	Supported precisions are -
>> 	#define I915_GAMMA_PRECISION_UNKNOWN	0
>> 	#define I915_GAMMA_PRECISION_CURRENT	0xFFFFFFFF
>> 	#define I915_GAMMA_PRECISION_LEGACY	(1 << 0)
>> 	#define I915_GAMMA_PRECISION_10BIT	(1 << 1)
>> 	#define I915_GAMMA_PRECISION_12BIT	(1 << 2)
>> 	#define I915_GAMMA_PRECISION_14BIT	(1 << 3)
>> 	#define I915_GAMMA_PRECISION_16BIT	(1 << 4)
>
> I feel like the precision would work better as a separate enum property
> rather than being part of your blob; I think it would be cleaner if your
> blob only held the actual values if possible.
>
Again, this would be required for the get_property path. If we create a 
separate property for this, the design might be very complex.

This is the current implementation:
1. Pack the correction values and configurations in blob()
2. call a create_blob() and get the blob_id
3. do a set_porperty() with the blob_id
4. set_property will find this blob, with the blob_id and apply 
corrections.

Adding a separate property for gamma_level will add one more state here, 
which will make it further complex. Do you agree ?
>>
>> 	__u32 num_samples;
>> 	(The num_samples indicates the number of Gamma correction
>> 	coefficients)
>> 	__u32 reserved;
>> 	__u16 values[0];
>> 	(An array of size 0, to accommodate the "num_samples" number of
>> 	R16G16B16 pixels, dynamically)
>> };
>>
>> v2: Addressing Daniel Stone's comment, added a variable sized array to
>> carry Gamma correction values as blob property.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
>> ---
>>   include/drm/drm_crtc.h |  3 +++
>>   include/uapi/drm/drm.h | 10 ++++++++++
>>   2 files changed, 13 insertions(+)
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 2a75d7d..bc44f27 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -483,6 +483,9 @@ struct drm_crtc {
>>   	 * acquire context.
>>   	 */
>>   	struct drm_modeset_acquire_ctx *acquire_ctx;
>> +
>> +	/* Color Management Blob IDs */
>> +	u32 gamma_blob_id;
>>   };
>>
>>   /**
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 3801584..fc2661c 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -829,6 +829,16 @@ struct drm_event_vblank {
>>   	__u32 reserved;
>>   };
>>
>> +/* Color Management structure for Gamma */
>> +struct drm_gamma {
>> +	__u32 flags;
>
> The flags aren't described in your commit message...what are they used
> for?  I guess it will become more clear as I read farther through your
> patch series.
>
>
> Matt
We are currently using this to define gamma/degamma as such, but yes I 
agree that we have added this for future / undefined last minute usages, 
and can be removed.
>
>
>> +	__u32 gamma_level;
>> +	__u32 gamma_precision;
>> +	__u32 num_samples;
>> +	__u32 reserved;
>> +	__u16 values[0];
>> +};
>> +
>>   /* typedef area */
>>   #ifndef __KERNEL__
>>   typedef struct drm_clip_rect drm_clip_rect_t;
>> --
>> 2.4.2
>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx