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

[David Herrmann]

Hi

On Tue, Apr 22, 2014 at 12:21 PM, Sharma, Shashank
<shashank.sharma at intel.com> wrote:
> 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.

Where is the difference? With one _well-defined_ property for each
type we simply move the identification one level up. With your
approach you just move the type-id one level down into the blob.

Or in other words: Where is the difference between calling
SetProperty() n-times, or calling it once but with a parameter
describing n-properties? With atomic-modesetting we can set as many
properties as we want and make the kernel apply them atomically.

> 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.

Yeah, but I meant the drmModeObjectSetProperty() ioctl already tales a
CRTC/Plane/Connector ID. So why duplicate that information in the
blob? And more importantly, what happens if you call
drmModeObjectSetProperty() on a plane but specify a CRTC ID in the
blob? Seems weird to me to support such setups.

> 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.

Well, your document doesn't describe the payload at all. I just wanted
a description of what kind of information is expected. Number of
arguments, argument size, argument types, argument description.. and
so on.

> 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 ?

Sure.

Thanks
David