Thanks again David, Comments inline. Regards Shashank -----Original Message----- From: David Herrmann [mailto:dh.herrm...@gmail.com] Sent: Tuesday, April 22, 2014 5:10 PM To: Sharma, Shashank Cc: intel-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; Ville Syrjälä; Thierry Reding; Alex Deucher; Sean Paul; robdcl...@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 12:21 PM, Sharma, Shashank <shashank.sha...@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. >>> Actually we also do not want to populate the property space also, as if >>> there are 10 color correction methods possible for a hardware, we might end >>> up listing 10 properties. And there won't be common properties across all >>> the hardwares also. For example, Hardware A can have properties X Y Z but >>> Hardware B can have W X and Z. This will make the property space >>> inconsistent. But if we provide one common interface which will cover for >>> all the properties, for all the hardwares in a single blob. The driver will >>> dynamically register its property, in its own preferred name. A get_prop() >>> will always list down all the supported color property by this hardware and >>> driver. > 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. >>> The design is to register color-manager as a CRTC property, to make it >>> consistent, and then give the fine tuning via this identifier byte. Else we have to keep track of this in userspace, that which property is valid for which extent. For example, Hue and saturation correction, on VLV, can be applied on Sprite planes only(not on primary plane). So we have to send a plane as an object here. Rather in color manager case, we will always send the CRTC as an object to IOCTL, but will specify SPRITE_PLANE as identifier. Does this sound less weird now :) ? > 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. >>>> Sure, I will further document it very clearly about arguments and >>>> descriptions. Actually we have discussed the protocol in the color EDID >>>> section, which tells us about the 4 byte protocol and expectation, but >>>> that’s elementary. > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
