Re: [PATCH] drm/stm: ltdc: add clut mode support

[Philippe CORNU]

Hi Peter,

On 11/12/2017 01:31 PM, Peter Rosin wrote:
> On 2017-11-10 17:12, Philippe CORNU wrote:
>> Hi Peter,
>>
>> On 11/07/2017 05:34 PM, Peter Rosin wrote:
>>> On 2017-11-07 16:53, Philippe CORNU wrote:
>>>> + Peter
>>>>
>>>> Hi Peter,
>>>>
>>>> CLUT support on STM32 has been removed thanks to your clean up patch
>>>
>>> Support is a bit strong for what I thought was a dead function, or
>>> are you saying that it used to work before my series? Really sorry
>>> if that is the case!
>>
>> As I wrote in the previous related thread
>> (https://lists.freedesktop.org/archives/dri-devel/2017-June/145070.html),
>> STM32 chipsets supports 8-bit CLUT mode but this driver version does not
>> support it "yet"...
>>
>> So, no worry regarding your clean up, I gave you an "acked-by" for that : )
> 
> Ok, good. Thanks for clearing that up!
> 
>>>
>>> Anyway, the function I removed seemed to indicate that the hardware
>>> could handle a separate clut for each layer, but your new version
>>> does not. Why is that?
>>
>> Yes I confirm the clut support is available for each layer... but I
>> thought the gamma_lut was only at the crtc level, not at layer level...
>> Maybe I am wrong.
>> Moreover, small test applications I used play only with clut at crtc
>> level...
>>
>> Anyway, could you please help me to "find" a per-layer clut
>> implementation because when I read "crtc->state->gamma_lut->data" it
>> looks like gamma_lut is per crtc, not per plane...? or maybe I have to
>> add extra properties for that...
> 
> I wasn't clear enough. Yes, there is to my knowledge only one clut,
> not one per plane. What I noticed was that the function I removed
> seemed to touch clut registers for multiple layers, but your new
> function appears to only touch registers for one layer. So, I
> wondered if the "one and only" clut needed to be copied to the
> registers for the other layers, or if the old dead code was simply
> confused. Clearer?
> 

The old code was a generic helper function (ie. for all layers) but used 
only for the 1st layer. So, we could say that "old dead code was simply 
confused" :-)

When I put back the clut support in this patch, I decided to update only 
the 1st layer (because there is no API for handling it on other layers). 
I also decided to not re-use the former generic helper function as the 
update loop is pretty small.

This patch offers the clut mode feature for fbdev (only one plane in 
fbdev) and for drm (single plane for many use cases, 2nd plane being 
used mostly for video...)

If tomorrow the API offers clut support per plane, the update loop will 
be moved to the plane update function, means the generic helper function 
will not be require anymore too.

Many thanks
Philippe :)

> Cheers.
> Peter
>