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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8f0a5138-e074-e374-102e-76f1bab52b2e@st.com>
Date:   Fri, 24 Nov 2017 13:54:05 +0000
From:   Philippe CORNU <philippe.cornu@...com>
To:     Peter Rosin <peda@...ntia.se>,
        Yannick FERTRE <yannick.fertre@...com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Vincent ABRIOU <vincent.abriou@...com>,
        David Airlie <airlied@...ux.ie>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     Fabien DESSENNE <fabien.dessenne@...com>,
        Mickael REULIER <mickael.reulier@...com>,
        Gabriel FERNANDEZ <gabriel.fernandez@...com>,
        Ludovic BARRE <ludovic.barre@...com>,
        Alexandre TORGUE <alexandre.torgue@...com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>
Subject: Re: [PATCH] drm/stm: ltdc: add clut mode support

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?
Many thanks,
Philippe :-)

> 
> Many thanks
> Philippe :)
> 
>> Cheers.
>> Peter
>>