Design review request: DRM color manager

Design review request: DRM color manager

[Sharma, Shashank]

[-- Attachment #1: Type: text/plain, Size: 4426 bytes --]

Hi All,

Based on all the previous feedbacks, we have done the design changes in previous color manager implementation.
This new design addresses following previous comments / suggestions / requirements: 

1. HW agnostic design, so each driver can register its own capabilities, and own color correction methods. 
2. DRM property based interface design, so can be plugged in with atomic modeset() and other DRM implementation. 
3. Single interface for all color correction related properties.

Please find the design document attached with the mail. I would request you all to have a look, and help us to freeze the design by provide suggestions, design changes and improvements needs etc.
Once we agree on the design, we will start the implementation. 

Regards
Shashank  
-----Original Message-----
From: Thierry Reding [mailto:thierry.reding@gmail.com] 
Sent: Tuesday, February 25, 2014 5:12 PM
To: Alex Deucher
Cc: Ville Syrjälä; intel-gfx@lists.freedesktop.org; Shankar, Uma; dri-devel@lists.freedesktop.org; Sharma, Shashank
Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

On Fri, Feb 21, 2014 at 10:41:14AM -0500, Alex Deucher wrote:
> On Fri, Feb 21, 2014 at 9:46 AM, Ville Syrjälä 
> <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Feb 21, 2014 at 02:20:24PM +0000, Sharma, Shashank wrote:
> >> Hi Ville,
> >>
> >> Thanks for your time and comments.
> >> I can understand two basic problems what you see in this implementation:
> >>
> >> 1.  The most important issue from my POV is that it can't be part of the atomic modeset.
> >> 2.  it make the whole API inconsistent.
> >>
> >> I am not sure if its good to block all current implementation because we have thought something for this in atomic modeset.
> >> I think even in atomic modeset we need the core implementation like this, but the interface would be different, which might come in from of a DRM property.
> >> So at that time we can use this core implementation as it is, only the interfaces/framework needs to be changed.
> >>
> >> In this way we can always go ahead with a current implementation, and can just change the interfaces to fit in to the final interface like DRM property in atomic modeset.
> >> Or you can suggest us the expected interface, and we can work on modifying that as per expectation.
> >
> > The exptected interface will be range properties for stuff like 
> > brightness, contrast etc. controls. There are already such things as 
> > connector properties, but we're going to want something similar as 
> > plane or crtc properties. One thing that worries me about such 
> > properties though is whether we can make them hardware agnostic and 
> > yet allow userspace precise control over the final image. That is, 
> > if we map some fixed input range to a hardware specific output 
> > range, userspace can't know how the actual output will change when 
> > the input changes. On the other hand if the input is hardware 
> > specific, userspace can't know what value to put in there to get the expected change on the output side.
> >
> > For bigger stuff like CSC matrices and gamma ramps we will want to 
> > use some reasonably well defined blobs. Ie. the internal strucuture 
> > of the blob has to be documented and it shouldn't contain more than necessary.
> > Ie. just the CSC matrix coefficients for one matrix, or just the 
> > entries for a single gamma ramp. Again ideally we should make the 
> > blobs hardware agnostic, but still allow precise control over the output data.
> >
> > I think this is going to involve first going over our hardware 
> > features, trying to find the common patterns between different 
> > generations. If there's a way to make something that works across 
> > the board for us, or at least across a wide range, then we should 
> > also ask for some input on dri-devel whether the proposed property 
> > would work for other people. We may need to define new property 
> > types to more precisely define what the value of the property actually means.
> >
> 
> Our hardware has similar features, so I'm sure there will be quite a 
> bit of common ground.  I also vote for properties.

Thirded. Tegra should be able to use a hardware-agnostic description of these as well. I wonder if perhaps VESA or some other standard already defines such a format for some of these properties.

Thierry

[-- Attachment #2: DRMColorManager.pptx --]
[-- Type: application/vnd.openxmlformats-officedocument.presentationml.presentation, Size: 78606 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Design review request: DRM color manager

[Sharma, Shashank]

Gentle reminder

Regards
Shashank 
-----Original Message-----
From: Sharma, Shashank 
Sent: Friday, April 18, 2014 12:01 PM
To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Ville Syrjälä; Thierry Reding; Alex Deucher; Sean Paul; robdclark@gmail.com
Cc: Shankar, Uma; Korjani, Vikas; Jindal, Sonika; Mukherjee, Indranil; Cn, Ramakrishnan
Subject: Design review request: DRM color manager

Hi All,

Based on all the previous feedbacks, we have done the design changes in previous color manager implementation.
This new design addresses following previous comments / suggestions / requirements: 

1. HW agnostic design, so each driver can register its own capabilities, and own color correction methods. 
2. DRM property based interface design, so can be plugged in with atomic modeset() and other DRM implementation. 
3. Single interface for all color correction related properties.

Please find the design document attached with the mail. I would request you all to have a look, and help us to freeze the design by provide suggestions, design changes and improvements needs etc.
Once we agree on the design, we will start the implementation. 

Regards
Shashank
-----Original Message-----
From: Thierry Reding [mailto:thierry.reding@gmail.com]
Sent: Tuesday, February 25, 2014 5:12 PM
To: Alex Deucher
Cc: Ville Syrjälä; intel-gfx@lists.freedesktop.org; Shankar, Uma; dri-devel@lists.freedesktop.org; Sharma, Shashank
Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework

On Fri, Feb 21, 2014 at 10:41:14AM -0500, Alex Deucher wrote:
> On Fri, Feb 21, 2014 at 9:46 AM, Ville Syrjälä 
> <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Feb 21, 2014 at 02:20:24PM +0000, Sharma, Shashank wrote:
> >> Hi Ville,
> >>
> >> Thanks for your time and comments.
> >> I can understand two basic problems what you see in this implementation:
> >>
> >> 1.  The most important issue from my POV is that it can't be part of the atomic modeset.
> >> 2.  it make the whole API inconsistent.
> >>
> >> I am not sure if its good to block all current implementation because we have thought something for this in atomic modeset.
> >> I think even in atomic modeset we need the core implementation like this, but the interface would be different, which might come in from of a DRM property.
> >> So at that time we can use this core implementation as it is, only the interfaces/framework needs to be changed.
> >>
> >> In this way we can always go ahead with a current implementation, and can just change the interfaces to fit in to the final interface like DRM property in atomic modeset.
> >> Or you can suggest us the expected interface, and we can work on modifying that as per expectation.
> >
> > The exptected interface will be range properties for stuff like 
> > brightness, contrast etc. controls. There are already such things as 
> > connector properties, but we're going to want something similar as 
> > plane or crtc properties. One thing that worries me about such 
> > properties though is whether we can make them hardware agnostic and 
> > yet allow userspace precise control over the final image. That is, 
> > if we map some fixed input range to a hardware specific output 
> > range, userspace can't know how the actual output will change when 
> > the input changes. On the other hand if the input is hardware 
> > specific, userspace can't know what value to put in there to get the expected change on the output side.
> >
> > For bigger stuff like CSC matrices and gamma ramps we will want to 
> > use some reasonably well defined blobs. Ie. the internal strucuture 
> > of the blob has to be documented and it shouldn't contain more than necessary.
> > Ie. just the CSC matrix coefficients for one matrix, or just the 
> > entries for a single gamma ramp. Again ideally we should make the 
> > blobs hardware agnostic, but still allow precise control over the output data.
> >
> > I think this is going to involve first going over our hardware 
> > features, trying to find the common patterns between different 
> > generations. If there's a way to make something that works across 
> > the board for us, or at least across a wide range, then we should 
> > also ask for some input on dri-devel whether the proposed property 
> > would work for other people. We may need to define new property 
> > types to more precisely define what the value of the property actually means.
> >
> 
> Our hardware has similar features, so I'm sure there will be quite a 
> bit of common ground.  I also vote for properties.

Thirded. Tegra should be able to use a hardware-agnostic description of these as well. I wonder if perhaps VESA or some other standard already defines such a format for some of these properties.

Thierry
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Design review request: DRM color manager

[David Herrmann]

Hi

On Tue, Apr 22, 2014 at 6:11 AM, Sharma, Shashank
<shashank.sharma@intel.com> wrote:
> Gentle reminder

Usual approach is to send any proposals as inline plain-text. It's
really hard to comment on attachments, especially if it's an MS-office
format. Anyhow, some comments on the proposal:

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.

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?

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.

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.

5) Please use common prefixes to group related functions: Use
drm_color_manager_register() instead of drm_register_color_manager().
Similarly, use drm_color_manager_set_property() instead of
drm_set_color_manager_property()..

Thanks
David

Re: Design review request: DRM color manager

[Sharma, Shashank]

Thanks for the review and comments David. 
Please find my comments inline. 

Regards
Shashank  
-----Original Message-----
From: David Herrmann [mailto:dh.herrmann@gmail.com] 
Sent: Tuesday, April 22, 2014 3:08 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 6:11 AM, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> Gentle reminder

Usual approach is to send any proposals as inline plain-text. It's really hard to comment on attachments, especially if it's an MS-office format. Anyhow, some comments on the proposal:
>> Sorry, I found it difficult to share that design on text only style. I will keep this in mind. 

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.  

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.   

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. 

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 ?   

5) Please use common prefixes to group related functions: Use
drm_color_manager_register() instead of drm_register_color_manager().
Similarly, use drm_color_manager_set_property() instead of drm_set_color_manager_property()..
>> Sure, I will do so. 


Thanks
David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Design review request: DRM color manager

[David Herrmann]

Hi

On Tue, Apr 22, 2014 at 12:21 PM, Sharma, Shashank
<shashank.sharma@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Design review request: DRM color manager

[Sharma, Shashank]

Thanks again David, 
Comments inline. 

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

Re: Design review request: DRM color manager

[Daniel Vetter]

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

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

Re: Design review request: DRM color manager

[Sharma, Shashank]

[-- Attachment #1: Type: text/plain, Size: 7571 bytes --]

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

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[-- Attachment #2: Type: message/rfc822, Size: 5878 bytes --]

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: "Sharma, Shashank" <shashank.sharma@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>, "Shankar, Uma" <uma.shankar@intel.com>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 0/6] Intel Color Manager Framework
Date: Fri, 21 Feb 2014 09:17:06 +0000
Message-ID: <20140221091706.GJ3852@intel.com>

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@linux.intel.com]
> Sent: Thursday, February 20, 2014 6:41 PM
> To: Sharma, Shashank
> Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [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

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Design review request: DRM color manager

[Sharma, Shashank]

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

Re: Design review request: DRM color manager

[Sharma, Shashank]

Gentle reminder. 

Regards
Shashank 
-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, Shashank
Sent: Wednesday, May 07, 2014 7:53 PM
To: Vetter, Daniel; ville.syrjala@linux.intel.com; intel-gfx@lists.freedesktop.org
Cc: uma.shakar@intel.com
Subject: [Intel-gfx] Design review request: DRM color manager

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:
>>> 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
>
> --
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Design review request: DRM color manager

[Daniel Vetter]

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



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

Re: Design review request: DRM color manager

[Sharma, Shashank]

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:
>>>>>
>>>>> 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
>>>
>>>
>>> --
>>> 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
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Design review request: DRM color manager

[David Herrmann]

Hi

On Mon, May 12, 2014 at 12:26 PM, Sharma, Shashank
<shashank.sharma@intel.com> wrote:
> 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.

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

Re: Design review request: DRM color manager

[Sharma, Shashank]

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

Re: Design review request: DRM color manager

[Daniel Vetter]

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:
> >>================================
> >>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.
> >
> >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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[David Herrmann]

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

Re: Design review request: DRM color manager

[Sharma, Shashank]

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

Re: Design review request: DRM color manager

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2936 bytes --]

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

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Design review request: DRM color manager

[Sharma, Shashank]

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

Re: Design review request: DRM color manager

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 815 bytes --]

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Design review request: DRM color manager

[Sharma, Shashank]

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