Re: Design review request: DRM color manager

["Sharma, Shashank" <shashank.sharma@intel.com>]

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
>