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

[Peter Rosin]

On 2017-11-24 14:54, Philippe CORNU wrote:
> Hi Peter,
> 
> On 11/13/2017 11:40 AM, Philippe CORNU wrote:
>> 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.
> 
>  From the explanations above, do you think the patch is "acceptable" or 
> should I change it somehow?
> What is your opinion?

Oops, sorry about forgetting to respond. Since I don't know how the hw
behaves with respect to the clut registers for the other layers, I'm
just not qualified to say if you should feed the one-and-only clut to
all layers or if it is sufficient to only feed it to the first plane.

I don't even know what the expected outcome is if you use clut modes on
other planes? Since there is no API for setting up plane-specific cluts,
the easy out seems to be to just reuse the one-and-only clut for all
planes. Otherwise, there is no way at all to change the clut of the other
planes. No?

Other than that, when I added clut-support for atmel-hlcdc, I updated
the hw registers as part of drm_plane_helper_funcs.atomic_update (as
requested by Boris Brezillon) while you do it in
drm_crtc_helper_funcs.atomic_flush. I don't know which is the better
place to do it, but since the hw presumably(?) do have clut registers
for each plane (as indicated by the code I removed), the plane-helper
.atomic_update seems like a better fit (at least from here). And maybe
it doesn't matter, but what do I know?

For the record, I know very little about the subsystem. I'm probably
wrong in oh so many ways...

Cheers,
Peter