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: <19d562e6-2d1a-0e66-e3f5-390bee1073ad@st.com>
Date:   Fri, 10 Nov 2017 16:12:58 +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/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 : )

> 
> 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...

Many thanks,
Philippe :-)

> 
> Cheers,
> peda
> 
>> named "drm: stm: remove dead code and pointless local lut storage"
>> (https://patchwork.freedesktop.org/patch/166898/)
>>
>> This below patch puts back the clut mode support using the new drm gamma
>> api.
>>
>> May I ask you please a short review on this patch?
>>
>> Many thanks,
>> Philippe :-)
>>
>>
>> On 10/26/2017 01:17 PM, Philippe Cornu wrote:
>>> Add the 8-bit clut mode support at crtc level.
>>> Useful for low memory footprint user interfaces but also for
>>> 8-bit old games (including color shifting visual effects).
>>> Tested with fbdev FBIOPUTCMAP & drm DRM_IOCTL_MODE_SETGAMMA
>>> ioctls.
>>>
>>> Signed-off-by: Philippe Cornu <philippe.cornu@...com>
>>> ---
>>>    drivers/gpu/drm/stm/ltdc.c | 30 ++++++++++++++++++++++++++++++
>>>    1 file changed, 30 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
>>> index 7be6710..d5c8a42 100644
>>> --- a/drivers/gpu/drm/stm/ltdc.c
>>> +++ b/drivers/gpu/drm/stm/ltdc.c
>>> @@ -174,6 +174,8 @@
>>>    
>>>    #define LXCFBLNR_CFBLN	GENMASK(10, 0)	/* Color Frame Buffer Line Number */
>>>    
>>> +#define CLUT_SIZE	256
>>> +
>>>    #define CONSTA_MAX	0xFF		/* CONSTant Alpha MAX= 1.0 */
>>>    #define BF1_PAXCA	0x600		/* Pixel Alpha x Constant Alpha */
>>>    #define BF1_CA		0x400		/* Constant Alpha */
>>> @@ -362,6 +364,28 @@ static irqreturn_t ltdc_irq(int irq, void *arg)
>>>     * DRM_CRTC
>>>     */
>>>    
>>> +static void ltdc_crtc_update_clut(struct drm_crtc *crtc)
>>> +{
>>> +	struct ltdc_device *ldev = crtc_to_ltdc(crtc);
>>> +	struct drm_color_lut *lut;
>>> +	u32 val;
>>> +	int i;
>>> +
>>> +	if (!crtc || !crtc->state)
>>> +		return;
>>> +
>>> +	if (!crtc->state->color_mgmt_changed || !crtc->state->gamma_lut)
>>> +		return;
>>> +
>>> +	lut = (struct drm_color_lut *)crtc->state->gamma_lut->data;
>>> +
>>> +	for (i = 0; i < CLUT_SIZE; i++, lut++) {
>>> +		val = ((lut->red << 8) & 0xff0000) | (lut->green & 0xff00) |
>>> +			(lut->blue >> 8) | (i << 24);
>>> +		reg_write(ldev->regs, LTDC_L1CLUTWR, val);
>>> +	}
>>> +}
>>> +
>>>    static void ltdc_crtc_atomic_enable(struct drm_crtc *crtc,
>>>    				    struct drm_crtc_state *old_state)
>>>    {
>>> @@ -485,6 +509,8 @@ static void ltdc_crtc_atomic_flush(struct drm_crtc *crtc,
>>>    
>>>    	DRM_DEBUG_ATOMIC("\n");
>>>    
>>> +	ltdc_crtc_update_clut(crtc);
>>> +
>>>    	/* Commit shadow registers = update planes at next vblank */
>>>    	reg_set(ldev->regs, LTDC_SRCR, SRCR_VBR);
>>>    
>>> @@ -532,6 +558,7 @@ void ltdc_crtc_disable_vblank(struct drm_device *ddev, unsigned int pipe)
>>>    	.reset = drm_atomic_helper_crtc_reset,
>>>    	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>>>    	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>> +	.gamma_set = drm_atomic_helper_legacy_gamma_set,
>>>    };
>>>    
>>>    /*
>>> @@ -764,6 +791,9 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
>>>    
>>>    	drm_crtc_helper_add(crtc, &ltdc_crtc_helper_funcs);
>>>    
>>> +	drm_mode_crtc_set_gamma_size(crtc, CLUT_SIZE);
>>> +	drm_crtc_enable_color_mgmt(crtc, 0, false, CLUT_SIZE);
>>> +
>>>    	DRM_DEBUG_DRIVER("CRTC:%d created\n", crtc->base.id);
>>>    
>>>    	/* Add planes. Note : the first layer is used by primary plane */
>