[Intel-gfx] [PATCH v2 04/10] drm: Add Gamma correction structure

[Malladi, Kausal]

Hi Sonika,

Please find my responses inline.

Thanks,
Kausal

On Friday 05 June 2015 05:30 PM, Jindal, Sonika wrote:
>
>
> On 6/4/2015 7:12 PM, Kausal Malladi wrote:
>> From: Kausal Malladi <Kausal.Malladi at 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
> Do you have any macro defines for these levels? Maybe an enum will be 
> better?
Yes, we have macro defines for these. There are only two possible levels 
(pipe/plane).
>>     applied on Pipe/plane)
>>         __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)
>>
>>     __u32 num_samples;
>>     (The num_samples indicates the number of Gamma correction
>>     coefficients)
>>     __u32 reserved;
> You need this reserved?
In case any future platforms require any additional checks, we can use 
this member. Having said that, yes.. we are currently not using it.
>>     __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 at intel.com>
>> Signed-off-by: Kausal Malladi <Kausal.Malladi at 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;
>> +
> Just trying to understand, why do we need to store the blob id 
> separately?
Saving Blob ID allows us to do a lookup of the blob using ID. This will 
be useful in get_property call.
>> +    /* 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;
>> +    __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;
>>