On Tue, Apr 22, 2014 at 12:07:41PM +0000, Sharma, Shashank wrote: > Thanks again David, > Comments inline.
Three things: - Please don't send out .pptx files to upstream/public mailing lists, that's just not how the upstream community works. - Please either fix up ms outlook to do proper in-line quoting or switch to a proper mail client for discussions on dri-devel. I'm ok with this on intel-gfx to some extend since that's our own turf, but on dri-devel the usual rules apply. - I think we should discuss this internally first or at least just on intel-gfx. David, thanks for taking a look at this but imo this shouldn't have escaped yet to the public. My apologies for wasting your time trying to review this proposal. Thanks, Daniel > > Regards > Shashank > -----Original Message----- > From: David Herrmann [mailto:dh.herrmann at gmail.com] > Sent: Tuesday, April 22, 2014 5:10 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 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. > > >>> 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 at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
