David,
My apologies for starting a pre-mature design discussion.
Daniel,
Thanks for pointing out first two things, It was not known to me, I will take care of this in future.
First time I presented color-manager design, in internal display design forum, where most of the reviewers were not there.
We took the feedback from people who were present, and implemented the design.
When we shared color manager implementation, that design was rejected and one of the feedbacks was that it would be better to discuss it on dri-devel where people outside Intel can give their opinion,
and that’s the only reason why I added dri-devel for the new design (Please see the attached mail, I replied to all who were in last communication).
Please let me know how do we want to proceed now.
Regards
Shashank
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel Vetter
Sent: Tuesday, April 22, 2014 7:18 PM
To: Sharma, Shashank
Cc: David Herrmann; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Thierry Reding; Cn, Ramakrishnan; Alex Deucher; Jindal, Sonika; Shankar, Uma
Subject: Re: [Intel-gfx] Design review request: DRM color manager
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@xxxxxxxxx]
> Sent: Tuesday, April 22, 2014 5:10 PM
> To: Sharma, Shashank
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> Ville Syrjälä; Thierry Reding; Alex Deucher; Sean Paul;
> robdclark@xxxxxxxxx; 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--- Begin Message ---
On Fri, Feb 21, 2014 at 03:34:43AM +0000, Sharma, Shashank wrote:
> Hi Ville/All,
>
> We gave a presentation on design on this framework, few months ago, in one of our common forum with OTC folks.
> We discussed, took review comments, and re-designed the framework, as per the feedbacks.
Apparently I wasn't there. And in any case it would be better to discuss
it on dri-devel where people outside Intel can give their opinion.
>
> We also discussed the benefits of providing the controls directly from /sysfs over going for a UI manager based property settings.
> So I don't understand where are we going wrong, can you please elaborate a bit ?
The most important issue from my POV is that it can't be part of the
atomic modeset.
Another issue is that it make the whole API inconsistent. Some stuff
through ioctl, some stuff through sysfs, some stuff through whatever the
next guy thinks of. It's not pretty. I've worked in the past with a
driver where I had to poke at various standardish ioctls, custom ioctls,
and sysfs to make it do anything useful, and I have no interest in
repeating that experience. sysfs is especially painful since you have
do the string<->binary conversions all over the place, and also you
en up doing open+read/write+close cycles for every little thing.
It also adds more entrypoints into the driver for us to worry about.
That means extra worries about the power management stuff and locking
at the very least.
Also the rules of sysfs say "one item per file". The only allowed
exception to this rule I know of is hardware provided blobs (like
EDID, PCI ROM etc.). Your current implementation breaks this rule
blatantly.
>
> This is just a basic design, and once go ahead with this, we can always work on making hardware agnostic, as you recommended.
>
> IMHO, controls from /sysfs would be a very generic interface for all linux/drm based platform, where any userspace can read/write and control properties.
> We don't even need a UI manager or a minimum executable to play around, just a small script can do. But we can always write something on top of this,
> to be included in any UI framework or property.
If there's a real need to get at properties through sysfs, then we could
think about exposing them all. But that may presents some issues where
the current master suddenly gets confused about its state since someone
else went behind its back and changed a bunch of stuff.
>
> Regards
> Shashank
>
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx]
> Sent: Thursday, February 20, 2014 6:41 PM
> To: Sharma, Shashank
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Shankar, Uma; dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 0/6] Intel Color Manager Framework
>
> On Thu, Feb 20, 2014 at 06:07:21PM +0530, Shashank Sharma wrote:
> > Color manager is a new framework in i915 driver, which provides a
> > unified interface for various color correction methods supported by
> > intel hardwares. The high level overview of this change is:
>
> Would have been good to discuss this idea before implementing it. The plan is to use kms properties for this kind of stuff which allows us to hook it up with the upcoming atomic modeset API. Just yesterday there was some discussion on #dri-devel about exposing user settable blob properties even before the atomic modeset API lands (it was always the plan for the atomic modeset API anyway). So based on a cursory glance, this looks like it's going in the wrong direction.
>
> Also ideally the properties should be hardware agnostic, so a generic userspace could use them regardless of the hardware/driver. Obviously that might not be possible in all cases, but we should at least spend a bit of effort on trying to make that happen for most properties.
>
> --
> Ville Syrjälä
> Intel OTC
--
Ville Syrjälä
Intel OTC
--- End Message ---
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
