Thanks for the review and comments David. Please find my comments inline.
Regards Shashank -----Original Message----- From: David Herrmann [mailto:dh.herrm...@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
