[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
>>> <shashank.sharma at 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.