[Intel-gfx] 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
>  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
>

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

[Thierry Reding]

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
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Sharma, Shashank]

Sounds good to me. 

Regards
Shashank 
-Original Message-
From: Thierry Reding [mailto:thierry.red...@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

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

[Thierry Reding]

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
> >>> 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
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[Intel-gfx] 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
>>>  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 

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

[David Herrmann]

Hi

On Mon, May 12, 2014 at 5:28 PM, Daniel Vetter  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

[Intel-gfx] 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
>  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.

[Intel-gfx] 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
> > 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

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

[Sharma, Shashank]

ister 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 at 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 at ffwll.ch] On Behalf Of Daniel
>>> Vetter
>>> Sent: Tuesday, April 22, 2014 7:18 PM
>>> To: Sharma, Shashank
>>> Cc: David Herrmann; intel-gfx at lists.freedesktop.org;
>>> dri-devel at 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 +, 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 discu

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

[David Herrmann]

Hi

On Mon, May 12, 2014 at 12:26 PM, Sharma, Shashank
 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

[Intel-gfx] 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
 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 at 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

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

[Daniel Vetter]

On Tue, Apr 22, 2014 at 12:07:41PM +, Sharma, Shashank wrote:
> Thanks again David, 
> Comments inline. 

Three things:
- Please don't send out .pptx files to upstream/public mailing lists,
  that's just not how the upstream community works.

- Please either fix up ms outlook to do proper in-line quoting or switch
  to a proper mail client for discussions on dri-devel. I'm ok with this
  on intel-gfx to some extend since that's our own turf, but on dri-devel
  the usual rules apply.

- I think we should discuss this internally first or at least just on
  intel-gfx.

David, thanks for taking a look at this but imo this shouldn't have
escaped yet to the public. My apologies for wasting your time trying to
review this proposal.

Thanks, Daniel

> 
> Regards
> Shashank
> -Original Message-
> From: David Herrmann [mailto:dh.herrmann at gmail.com] 
> Sent: Tuesday, April 22, 2014 5:10 PM
> To: Sharma, Shashank
> Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; 
> Ville Syrj?l?; Thierry Reding; Alex Deucher; Sean Paul; robdclark at 
> gmail.com; Mukherjee, Indranil; Jindal, Sonika; Korjani, Vikas; Shankar, Uma; 
> Cn, Ramakrishnan
> Subject: Re: Design review request: DRM color manager
> 
> Hi
> 
> On Tue, Apr 22, 2014 at 12:21 PM, Sharma, Shashank  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.
> >>>