Re-adding dri-devel, all drm core stuff must be discussed there.
But on the actual issue at hand I still don't understand what you're trying to solve. You add a complete new set of properties, using Intel names (pipes, planes) for some attributes which at first seems completely redundant to all the properties we already have.
What's the technical reasons for adding this interface? Your proposal is only describing how it works, so is lacking this crucial information. -Daniel
On Wed, May 7, 2014 at 4:22 PM, Sharma, Shashank shashank.sharma@intel.com wrote:
Hello all,
As per previous discussions, I am sending the drm-color-manager design for review, as inline text. Please have a look and let me know your feedback.
(I tried hard to align the inline text diagram in mail, hope you can see the proper picture too)
================= DRM Color Manager =================
- Color manager provides a common interface for all color correction / enhancement properties supported by various hardwares.
- Color manager will be one Umbrella DRM property (blob type)
- Driver can register the color correction properties of its HW during the init time, and all the corrections can be applied using the same interface.
How does a driver register color properties with DRM color manager:
- A DRM driver will call drm_register_color_manager() function with
following information: .prop_set_handler .prop_get_hanlder .color_prop_identifier structure {porp_name, prop_id} {porp_name, prop_id} {porp_name, prop_id}
For example: I915 driver can register as: .prop_set_handler = intel_clrmgr_set() .prop_get_hanlder = intel_clrmgr_get() .color_prop_identifier structure {gamma_correction, 0} {csc_correction, 1} {hue_saturation, 2}
How will color_manager_set/get() functions work:
Color EDID:
|| property || Enable/ || Pipe/Plane/ || No of || || ID || Disable || Identifier || Data bytes ||data.. || <1 Byte> || <1 Byte> || <1 Byte> || <1 Byte> || ======================================================================
Color EDID is a protocol to extract the color correction characterictics from a blob, coming from DRM layer as property_set_blob() or loading a blob for property_get_blob()
Userspace will load this blob with corresponding values and use the drm_set_blob(color-manager) interface.
DRM layer of get/set_color_manager() will validate the entry, extract the following information from blob - property - enable/disable - identifier - ptr to start of raw data
With this information, drm_get/set_color_manager() layer will call driver's .get/set_handler() which will do the correction using those values.
Based on the driver's requirement, user space can send any type of values in byte stream, like direct reg values, correction coefficients or any other form of data.
Benefits of this common interface:
- Single interface for all color properties. No need to create multiple properties.
- HW agonist. Its in hands of driver to register the properties.
- Any no of properties can be registered.
- Can be clubbed with modeset implementations.
Regards Shashank
On 5/7/2014 7:41 PM, Sharma, Shashank wrote:
FYI
-----Original Message----- From: Sharma, Shashank Sent: Tuesday, April 22, 2014 8:32 PM To: Daniel Vetter Cc: David Herrmann; intel-gfx@lists.freedesktop.org; Cn, Ramakrishnan; Jindal, Sonika; Shankar, Uma Subject: RE: [Intel-gfx] Design review request: DRM color manager
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@ffwll.ch] On Behalf Of Daniel Vetter Sent: Tuesday, April 22, 2014 7:18 PM To: Sharma, Shashank Cc: David Herrmann; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; 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@gmail.com] Sent: Tuesday, April 22, 2014 5:10 PM To: Sharma, Shashank Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Ville Syrjälä; Thierry Reding; Alex Deucher; Sean Paul; robdclark@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@intel.com wrote:
- 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.
- 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 :) ?
- 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.
- 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
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hello Daniel,
Please find the actual problem statement and design overview : ============================================================== 1. There are different color correction methods, supported by various SOCs, across various platforms. 2. These properties vary platform-by-platform, driver-to-driver. For example, one of the Intel SOC support CSC correction, Gamma correction, Hue and Saturation correction, Brightness and Contrast correction. One other Intel SOC supports two of above mentioned properties, and 2 other properties. Hardly any of these properties are exposed properly to userspace to be used to its real potential due to lack of proper interface. 3. Even if we go for a direct DRM property interface for each of the correction possible, if some hardware supports 10 color correction properties, a driver is supposed to create 10 DRM properties corresponding to each. But most of the time what these color correction properties do is, apply some correction values on some hardware registers, and enable / disable the property. 4. Color manager is the one common interface for all such properties supported by all these drivers, which follows a particular protocol, to extract this color correction information from the userspace call, call the drivers apply method with all the required Information.
Benefits of using color manager: ================================ 1. Unique framework for all the color correction properties, across all DRM drivers, across various platforms. 2. Only one set/get call for all kind of properties using the common protocol. One get call can tell what are the color correction capabilities of the SOC, registered by driver. 3. The corrections value range that can be covered using this can be single valued to any no. We can apply from single values brightness/contrast to full range LUT of a gamma correction using the same interface. 4. Hardware specific quirks can be applied on specific corrections. 5. A few of color corrections(like gamma correction) deal with floating point values, which is not easy to handle in Kernel. We can always delegate this FP conversion work to some userspace library, and get the direct register values using the interface. This kind of library can be expanded for various facilities to provide color-support.
We have implemented color manager implementation for one of the MCG code line, and lot of commercial solutions are using those color properties using the simple interface for Brightness, CSC and Gamma correction, and fine tuning to the best display experience for their panel. This part was not very well used due to lack of proper interface.
I think this design will help us to expose color correction capabilities of various SOCs and drivers, and will give an centrally controlled interface for ease of access also.
Please let me know if this makes a point.
Regards Shashank
On 5/12/2014 2:21 PM, Daniel Vetter wrote:
Re-adding dri-devel, all drm core stuff must be discussed there.
But on the actual issue at hand I still don't understand what you're trying to solve. You add a complete new set of properties, using Intel names (pipes, planes) for some attributes which at first seems completely redundant to all the properties we already have.
What's the technical reasons for adding this interface? Your proposal is only describing how it works, so is lacking this crucial information. -Daniel
On Wed, May 7, 2014 at 4:22 PM, Sharma, Shashank shashank.sharma@intel.com wrote:
Hello all,
As per previous discussions, I am sending the drm-color-manager design for review, as inline text. Please have a look and let me know your feedback.
(I tried hard to align the inline text diagram in mail, hope you can see the proper picture too)
================= DRM Color Manager =================
- Color manager provides a common interface for all color correction / enhancement properties supported by various hardwares.
- Color manager will be one Umbrella DRM property (blob type)
- Driver can register the color correction properties of its HW during the init time, and all the corrections can be applied using the same interface.
How does a driver register color properties with DRM color manager:
- A DRM driver will call drm_register_color_manager() function with
following information: .prop_set_handler .prop_get_hanlder .color_prop_identifier structure {porp_name, prop_id} {porp_name, prop_id} {porp_name, prop_id}
For example: I915 driver can register as: .prop_set_handler = intel_clrmgr_set() .prop_get_hanlder = intel_clrmgr_get() .color_prop_identifier structure {gamma_correction, 0} {csc_correction, 1} {hue_saturation, 2}
How will color_manager_set/get() functions work:
Color EDID:
|| property || Enable/ || Pipe/Plane/ || No of || || ID || Disable || Identifier || Data bytes ||data.. || <1 Byte> || <1 Byte> || <1 Byte> || <1 Byte> || ======================================================================
Color EDID is a protocol to extract the color correction characterictics from a blob, coming from DRM layer as property_set_blob() or loading a blob for property_get_blob()
Userspace will load this blob with corresponding values and use the drm_set_blob(color-manager) interface.
DRM layer of get/set_color_manager() will validate the entry, extract the following information from blob - property - enable/disable - identifier - ptr to start of raw data
With this information, drm_get/set_color_manager() layer will call driver's .get/set_handler() which will do the correction using those values.
Based on the driver's requirement, user space can send any type of values in byte stream, like direct reg values, correction coefficients or any other form of data.
Benefits of this common interface:
- Single interface for all color properties. No need to create multiple properties.
- HW agonist. Its in hands of driver to register the properties.
- Any no of properties can be registered.
- Can be clubbed with modeset implementations.
Regards Shashank
On 5/7/2014 7:41 PM, Sharma, Shashank wrote:
FYI
-----Original Message----- From: Sharma, Shashank Sent: Tuesday, April 22, 2014 8:32 PM To: Daniel Vetter Cc: David Herrmann; intel-gfx@lists.freedesktop.org; Cn, Ramakrishnan; Jindal, Sonika; Shankar, Uma Subject: RE: [Intel-gfx] Design review request: DRM color manager
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@ffwll.ch] On Behalf Of Daniel Vetter Sent: Tuesday, April 22, 2014 7:18 PM To: Sharma, Shashank Cc: David Herrmann; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; 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@gmail.com] Sent: Tuesday, April 22, 2014 5:10 PM To: Sharma, Shashank Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Ville Syrjälä; Thierry Reding; Alex Deucher; Sean Paul; robdclark@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@intel.com wrote:
- 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.
- 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 :) ?
- 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.
- 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
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi
On Mon, May 12, 2014 at 12:26 PM, Sharma, Shashank shashank.sharma@intel.com wrote:
Benefits of using color manager:
- Unique framework for all the color correction properties, across all DRM drivers, across various platforms.
- Only one set/get call for all kind of properties using the common
protocol. One get call can tell what are the color correction capabilities of the SOC, registered by driver.
What's the advantage of that? We should add a DRM_MODE_OBJ_SET_PROPERTIES call if we want to accelerate things, instead of adding huge payloads.
I really think we should define one property for each color-correction interface. I cannot see any downside of that except that we should add DRM_MODE_OBJ_SET_PROPERTIES. But afaik that's already the plan for atomic-modesetting, right?
Thanks David
Thanks for your time and the comments David. please find mine inline.
Regards Shashank On 5/12/2014 5:20 PM, David Herrmann wrote:
Hi
On Mon, May 12, 2014 at 12:26 PM, Sharma, Shashank shashank.sharma@intel.com wrote:
Benefits of using color manager:
- Unique framework for all the color correction properties, across all DRM drivers, across various platforms.
- Only one set/get call for all kind of properties using the common
protocol. One get call can tell what are the color correction capabilities of the SOC, registered by driver.
What's the advantage of that? We should add a DRM_MODE_OBJ_SET_PROPERTIES call if we want to accelerate things, instead of adding huge payloads.
The problem here is a DRM_OBJECT_MODE_SET_PROPERTIES call can pass limited value. But there are few properties like gamma correction which need a full LUT(256 vals) to be passed according to the correction values. The plan here is pretty much aligned to your suggestion, ie to use DRM_OBJECT_MODE_SET_BLOB kind of property, and extract the data from the blob based on a protocol. Why do you think its a huge payload ?
I really think we should define one property for each color-correction interface. I cannot see any downside of that except that we should add DRM_MODE_OBJ_SET_PROPERTIES.
As I was discussing, if there are 10 color correction properties a SOC supports, we have to create 10 properties which doesn't look good, when all the properties will do the same (set/reset few registers as correction). We already have a case where we expose 6 different type of corrections.
One more benefit is, this part is centrally controlled, so you can always know what's the state of color correction in single shot at any time. This will also provide us to do a lot of common things to do at the same place, like floating point arithmetic to register value conversion in a supporting userspace library, which might be required for many cases.
INHO, its always good to have a controlled interface/ framework to have similar things aligned to.
But afaik that's already the plan for
atomic-modesetting, right?
Thanks David
AFAIK color management is not a part of atomic modeset, but once we create such an interface, it would be really easy to club that in the atomic modeset.
On Mon, May 12, 2014 at 05:35:13PM +0530, Sharma, Shashank wrote:
Thanks for your time and the comments David. please find mine inline.
Regards Shashank On 5/12/2014 5:20 PM, David Herrmann wrote:
Hi
On Mon, May 12, 2014 at 12:26 PM, Sharma, Shashank shashank.sharma@intel.com wrote:
Benefits of using color manager:
- Unique framework for all the color correction properties, across all DRM drivers, across various platforms.
- Only one set/get call for all kind of properties using the common
protocol. One get call can tell what are the color correction capabilities of the SOC, registered by driver.
What's the advantage of that? We should add a DRM_MODE_OBJ_SET_PROPERTIES call if we want to accelerate things, instead of adding huge payloads.
The problem here is a DRM_OBJECT_MODE_SET_PROPERTIES call can pass limited value. But there are few properties like gamma correction which need a full LUT(256 vals) to be passed according to the correction values. The plan here is pretty much aligned to your suggestion, ie to use DRM_OBJECT_MODE_SET_BLOB kind of property, and extract the data from the blob based on a protocol. Why do you think its a huge payload ?
Gamma correction lut is already supported. For the other stuff we can use SET_BLOB (or fix it if it doesn't work).
I really think we should define one property for each color-correction interface. I cannot see any downside of that except that we should add DRM_MODE_OBJ_SET_PROPERTIES.
As I was discussing, if there are 10 color correction properties a SOC supports, we have to create 10 properties which doesn't look good, when all the properties will do the same (set/reset few registers as correction). We already have a case where we expose 6 different type of corrections.
One more benefit is, this part is centrally controlled, so you can always know what's the state of color correction in single shot at any time. This will also provide us to do a lot of common things to do at the same place, like floating point arithmetic to register value conversion in a supporting userspace library, which might be required for many cases.
INHO, its always good to have a controlled interface/ framework to have similar things aligned to.
Those are all just reasons for atomic modeset and maybe an atomic modeget ioctl which transfers the entire blob of things. Maybe we should start with the atomic modeget to get things rolling. Otoh you can always do that in userspace since we assume there's only one display manager.
And we absolutely want color correction to be part of the atomic modeset stuff (since some hw has e.g. per-plane gamma correction). So adding a new way to set them despite that the current atomic modeset proposal is fully centered on properties and that we've added all these properties for just this reasons is important. I still don't see what you can do with the color manager that's impossible to do with properties.
And having a large pile of properties imo doesn't count as a good reason, since with the color manager you still have all these settings. But maybe just encoded differently, e.g. in a bitfield or something. Changing the way you set stuff doesn't fundamentally change things at all.
And for modeset we can throw efficiency of the marshalling/de-marshalling over board (within some limits of course) since we do about 60*num_pipes modeset/pageflip calls per second at most. And the overhead of the encoding/decoding of the properties will be completely washed out by all the register i/o we need to do to UC pci bars.
But afaik that's already the plan for
atomic-modesetting, right?
Thanks David
AFAIK color management is not a part of atomic modeset, but once we create such an interface, it would be really easy to club that in the atomic modeset.
See above, this is a reason to _not_ add a separate color manager. -Daniel
Hi
On Mon, May 12, 2014 at 5:28 PM, Daniel Vetter daniel@ffwll.ch wrote:
Those are all just reasons for atomic modeset and maybe an atomic modeget ioctl which transfers the entire blob of things. Maybe we should start with the atomic modeget to get things rolling. Otoh you can always do that in userspace since we assume there's only one display manager.
DRM_MODE_OBJ_GET_PROPERTIES is already available and allows atomic retrieval of 'n' properties (as it calls drm_modeset_lock_all()). I guess that qualifies as what you describe with "atomic modeget"?
And for modeset we can throw efficiency of the marshalling/de-marshalling over board (within some limits of course) since we do about 60*num_pipes modeset/pageflip calls per second at most. And the overhead of the encoding/decoding of the properties will be completely washed out by all the register i/o we need to do to UC pci bars.
Yepp, I fully agree. And if properties turn out to become a bottleneck, we should optimize them.
Thanks David
Daniel, Please find my comments inline.
Regards Shashank On 5/12/2014 8:58 PM, Daniel Vetter wrote:
On Mon, May 12, 2014 at 05:35:13PM +0530, Sharma, Shashank wrote:
Thanks for your time and the comments David. please find mine inline.
Regards Shashank On 5/12/2014 5:20 PM, David Herrmann wrote:
Hi
On Mon, May 12, 2014 at 12:26 PM, Sharma, Shashank shashank.sharma@intel.com wrote:
Benefits of using color manager:
- Unique framework for all the color correction properties, across all DRM drivers, across various platforms.
- Only one set/get call for all kind of properties using the common
protocol. One get call can tell what are the color correction capabilities of the SOC, registered by driver.
What's the advantage of that? We should add a DRM_MODE_OBJ_SET_PROPERTIES call if we want to accelerate things, instead of adding huge payloads.
The problem here is a DRM_OBJECT_MODE_SET_PROPERTIES call can pass limited value. But there are few properties like gamma correction which need a full LUT(256 vals) to be passed according to the correction values. The plan here is pretty much aligned to your suggestion, ie to use DRM_OBJECT_MODE_SET_BLOB kind of property, and extract the data from the blob based on a protocol. Why do you think its a huge payload ?
Gamma correction lut is already supported. For the other stuff we can use SET_BLOB (or fix it if it doesn't work).
Current gamma correction supports only 8 bit mode, which cant do a real gamma correction. This is only to initialize the LUT. Actual gamma correction needs 10 bit support.
As discussed in design, the idea is same, ie to fix (implement) SET_BLOB. But see some of the requirements on LUT size of VLV:
1. Gamma correction: 256 values 2. CSC : 9 values in form of 6 register 3. Hue : 1 value (Plane level) 4. Saturation: 1 value (Plane level) 5. Contrast: 1 value (Plane level) 6. Brightness: 1 value (Plane level)
For CHV, the requirement is again different. There are different values, which vary from platform to platform and property-by-property. Now, one method of supporting these values is create a DRM property for each, some blob, some single valued, set individual interface and set them all at random. IMHO, this looks the non-systematic way of doing it.
The same thing has to be done differently for different platfroms, with some new color corrections added, some removed, and some no of coefficients changed. I can clearly see a requirement here.
I really think we should define one property for each color-correction interface. I cannot see any downside of that except that we should add DRM_MODE_OBJ_SET_PROPERTIES.
As I was discussing, if there are 10 color correction properties a SOC supports, we have to create 10 properties which doesn't look good, when all the properties will do the same (set/reset few registers as correction). We already have a case where we expose 6 different type of corrections.
One more benefit is, this part is centrally controlled, so you can always know what's the state of color correction in single shot at any time. This will also provide us to do a lot of common things to do at the same place, like floating point arithmetic to register value conversion in a supporting userspace library, which might be required for many cases.
INHO, its always good to have a controlled interface/ framework to have similar things aligned to.
Those are all just reasons for atomic modeset and maybe an atomic modeget ioctl which transfers the entire blob of things. Maybe we should start with the atomic modeget to get things rolling. Otoh you can always do that in userspace since we assume there's only one display manager. And we absolutely want color correction to be part of the atomic modeset stuff (since some hw has e.g. per-plane gamma correction). So adding a new way to set them despite that the current atomic modeset proposal is fully centered on properties and that we've added all these properties for just this reasons is important. I still don't see what you can do with the color manager that's impossible to do with properties.
If you remember, the initial design of color manager was from sysfs only, and we moved it to drm properties due to this big reason that it can be clubbed to atomic modeset directly. So I think we are aligned here, and please see that finally color manager is based on DRM properties only.
And having a large pile of properties imo doesn't count as a good reason, since with the color manager you still have all these settings. But maybe just encoded differently, e.g. in a bitfield or something. Changing the way you set stuff doesn't fundamentally change things at all.
With all due respect, in that case what was the need to create DRM property above IOCTL interface ? IOCTL was working fine. I believe just adding a custom layer above and clubbing similar things together gives a lot of control and customization, and that's why its better than random scattered properties.
And for modeset we can throw efficiency of the marshalling/de-marshalling over board (within some limits of course) since we do about 60*num_pipes modeset/pageflip calls per second at most. And the overhead of the encoding/decoding of the properties will be completely washed out by all the register i/o we need to do to UC pci bars.
There is hardly 4 byte read and 2 decision making conditions, and then this is the same as a set DRM property, which anyways a modeset has to do. If you have to add color correction in modeset, you anyways have to send a BLOB to a property, this is to sending it to a specific property, and diverging from there.
We need not to do color set every flip, only in case of a change it will come into picture. For example, we wont set CSC correction per flip, we will just set it once, and until there is a change, the output will be corrected. Same for other properties.
But afaik that's already the plan for
atomic-modesetting, right?
Thanks David
AFAIK color management is not a part of atomic modeset, but once we create such an interface, it would be really easy to club that in the atomic modeset.
See above, this is a reason to _not_ add a separate color manager. -Daniel
As I mentioned above, color manager is designed to be clubbed with atomic modeset, and will not be any blockage there.
On Tue, May 13, 2014 at 09:18:45AM +0530, Sharma, Shashank wrote:
Daniel, Please find my comments inline.
Regards Shashank On 5/12/2014 8:58 PM, Daniel Vetter wrote:
On Mon, May 12, 2014 at 05:35:13PM +0530, Sharma, Shashank wrote:
Thanks for your time and the comments David. please find mine inline.
Regards Shashank On 5/12/2014 5:20 PM, David Herrmann wrote:
Hi
On Mon, May 12, 2014 at 12:26 PM, Sharma, Shashank shashank.sharma@intel.com wrote:
Gamma correction lut is already supported. For the other stuff we can use SET_BLOB (or fix it if it doesn't work).
Current gamma correction supports only 8 bit mode, which cant do a real gamma correction. This is only to initialize the LUT. Actual gamma correction needs 10 bit support.
As discussed in design, the idea is same, ie to fix (implement) SET_BLOB. But see some of the requirements on LUT size of VLV:
- Gamma correction: 256 values
- CSC : 9 values in form of 6 register
- Hue : 1 value (Plane level)
- Saturation: 1 value (Plane level)
- Contrast: 1 value (Plane level)
- Brightness: 1 value (Plane level)
For CHV, the requirement is again different. There are different values, which vary from platform to platform and property-by-property. Now, one method of supporting these values is create a DRM property for each, some blob, some single valued, set individual interface and set them all at random. IMHO, this looks the non-systematic way of doing it.
That's exactly what atomic modeset/pageflip is meant to address. You get the flexibility of individual properties and on top of that a way to apply them all atomically.
The same thing has to be done differently for different platfroms, with some new color corrections added, some removed, and some no of coefficients changed. I can clearly see a requirement here.
Having them separated into individual properties will make it easy for userspace to determine at runtime which of them are available and which aren't. Also it seems to me that all of these properties should have a unified userspace interface. Drivers would then be free to implement the kernel side with the hardware-specific details.
AFAIK color management is not a part of atomic modeset, but once we create such an interface, it would be really easy to club that in the atomic modeset.
See above, this is a reason to _not_ add a separate color manager. -Daniel
As I mentioned above, color manager is designed to be clubbed with atomic modeset, and will not be any blockage there.
I think the point here is that once we have atomic modesetting/pageflip then there's no longer a need to have an "atomic" color manager property since there will be a mechanism to atomically apply any number of properties.
Thierry
What I understood from the reviews comments from the experts, is having a central color management at DRM kernel layer is not a good idea, and we should create individual DRM properties for the color correction methods, and let the control be there in the user space level, where an atomic modeset call will take decisions and figure out what and how to be done.
I will change my design accordingly, and make them all DRM properties so that this can be directly clubbed with atomic modeset.
Please note that the color correction methods changes per platform and what's valid for one Intel platform may not be valid for other. So the atomic modeset should have a clear idea of what is supported on which platforms.
Thanks for your time and review.
Regards Shashank On 5/14/2014 9:24 PM, Thierry Reding wrote:
On Tue, May 13, 2014 at 09:18:45AM +0530, Sharma, Shashank wrote:
Daniel, Please find my comments inline.
Regards Shashank On 5/12/2014 8:58 PM, Daniel Vetter wrote:
On Mon, May 12, 2014 at 05:35:13PM +0530, Sharma, Shashank wrote:
Thanks for your time and the comments David. please find mine inline.
Regards Shashank On 5/12/2014 5:20 PM, David Herrmann wrote:
Hi
On Mon, May 12, 2014 at 12:26 PM, Sharma, Shashank shashank.sharma@intel.com wrote:
Gamma correction lut is already supported. For the other stuff we can use SET_BLOB (or fix it if it doesn't work).
Current gamma correction supports only 8 bit mode, which cant do a real gamma correction. This is only to initialize the LUT. Actual gamma correction needs 10 bit support.
As discussed in design, the idea is same, ie to fix (implement) SET_BLOB. But see some of the requirements on LUT size of VLV:
- Gamma correction: 256 values
- CSC : 9 values in form of 6 register
- Hue : 1 value (Plane level)
- Saturation: 1 value (Plane level)
- Contrast: 1 value (Plane level)
- Brightness: 1 value (Plane level)
For CHV, the requirement is again different. There are different values, which vary from platform to platform and property-by-property. Now, one method of supporting these values is create a DRM property for each, some blob, some single valued, set individual interface and set them all at random. IMHO, this looks the non-systematic way of doing it.
That's exactly what atomic modeset/pageflip is meant to address. You get the flexibility of individual properties and on top of that a way to apply them all atomically.
The same thing has to be done differently for different platfroms, with some new color corrections added, some removed, and some no of coefficients changed. I can clearly see a requirement here.
Having them separated into individual properties will make it easy for userspace to determine at runtime which of them are available and which aren't. Also it seems to me that all of these properties should have a unified userspace interface. Drivers would then be free to implement the kernel side with the hardware-specific details.
AFAIK color management is not a part of atomic modeset, but once we create such an interface, it would be really easy to club that in the atomic modeset.
See above, this is a reason to _not_ add a separate color manager. -Daniel
As I mentioned above, color manager is designed to be clubbed with atomic modeset, and will not be any blockage there.
I think the point here is that once we have atomic modesetting/pageflip then there's no longer a need to have an "atomic" color manager property since there will be a mechanism to atomically apply any number of properties.
Thierry
On Thu, May 15, 2014 at 10:52:38AM +0530, Sharma, Shashank wrote: [...]
Please note that the color correction methods changes per platform and what's valid for one Intel platform may not be valid for other. So the atomic modeset should have a clear idea of what is supported on which platforms.
I don't think this will be an issue at all. The DRM driver should only expose what's supported on the particular device that it drives. And similarily userspace drivers should enumerate properties to find out which ones are available. Atomic modeset is only the mechanism to make sure it's all applied in one go. But that mechanism is completely generic and can be applied to any properties, therefore no specific knowledge about the available properties will be required in atomic modesetting itself.
Thierry
Sounds good to me.
Regards Shashank -----Original Message----- From: Thierry Reding [mailto:thierry.reding@gmail.com] Sent: Thursday, May 15, 2014 12:36 PM To: Sharma, Shashank Cc: Daniel Vetter; intel-gfx; dri-devel; Purushothaman, Vijay A; Mukherjee, Indranil; Shankar, Uma; Jindal, Sonika; Korjani, Vikas Subject: Re: [Intel-gfx] Design review request: DRM color manager
On Thu, May 15, 2014 at 10:52:38AM +0530, Sharma, Shashank wrote: [...]
Please note that the color correction methods changes per platform and what's valid for one Intel platform may not be valid for other. So the atomic modeset should have a clear idea of what is supported on which platforms.
I don't think this will be an issue at all. The DRM driver should only expose what's supported on the particular device that it drives. And similarily userspace drivers should enumerate properties to find out which ones are available. Atomic modeset is only the mechanism to make sure it's all applied in one go. But that mechanism is completely generic and can be applied to any properties, therefore no specific knowledge about the available properties will be required in atomic modesetting itself.
Thierry
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
David Herrmann -
Sharma, Shashank -
Thierry Reding