[Intel-gfx] Design review request: DRM color manager

[Sharma, Shashank]

Thanks for the review and comments David. 
Please find my comments inline. 

Regards
Shashank  
-----Original Message-----
From: David Herrmann [mailto:dh.herrmann at gmail.com] 
Sent: Tuesday, April 22, 2014 3:08 PM
To: Sharma, Shashank
Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; Ville Syrjälä; Thierry Reding; Alex Deucher; Sean Paul; robdclark at gmail.com; Mukherjee, Indranil; Jindal, Sonika; Korjani, Vikas; Shankar, Uma; Cn, Ramakrishnan
Subject: Re: Design review request: DRM color manager

Hi

On Tue, Apr 22, 2014 at 6:11 AM, Sharma, Shashank <shashank.sharma at intel.com> wrote:
> Gentle reminder

Usual approach is to send any proposals as inline plain-text. It's really hard to comment on attachments, especially if it's an MS-office format. Anyhow, some comments on the proposal:
>> Sorry, I found it difficult to share that design on text only style. I will keep this in mind. 

1) Why do you register only a single property? Why not register a separate property for each color-correction that is available? This way you can drop the property-id and use the high-level DRM-prop IDs/names.
>> That’s the whole idea of color manager. If we keep on creating properties for each color correction, there would be a big list and a lot of properties will be exposed. Instead one common blob which can represent all the properties, correction values and identifiers. It would be easy to club with atomic modeset kind-of designs also I believe.  

2) What is the CRTC-ID for? DRM properties can be set on a specific CRTC and/or plane. Isn't that enough information for the driver?
>> This is to make it HW agonist. Actually that's CRTC ID / Plane ID / PIPE ID / all together an identifier. For example if I want to set gamma correction for sprite planes only, not on primary plane or pipe level, on VLV, its possible. This gives me flexibility to mention fine-tuned correction even in a CRTC.  The driver's .enable method can take decision on this identifier based on the hardware capabilities.   

3) Please document the payload for each of the properties you define.
If the property is a blob, there is no reason to make the properties generic. User-space requires a common syntax across all drivers, otherwise, it cannot make use of generic properties and you should use driver-dependent properties instead.
>> Can you please elaborate a bit more ? I believe that a blob is a superset of single and multi-valued properties. So we can use the byte defined for <no of correction bytes> and specify both single value and multi value correction using the same interface, >> method and protocol.  So any userspace can just follow this, any can give commands to any driver. 

4) We have a tuple-type for properties. So in case you only need 32bit payloads for a given property, you can combine enable/disable and value in a single 64bit property.
>> But properties like CSC and Gamma correction need multiple correction values, up to 256 32-bit values. For this we need more no of values. AM I getting it right ?   

5) Please use common prefixes to group related functions: Use
drm_color_manager_register() instead of drm_register_color_manager().
Similarly, use drm_color_manager_set_property() instead of drm_set_color_manager_property()..
>> Sure, I will do so. 


Thanks
David