Nouveau - Jun 2017 - [PATCH v2 13/14] drm: stm: remove dead code and pointless local lut storage

On 06/22/2017 08:06 AM, Peter Rosin wrote:
> The redundant fb helper .load_lut is no longer used, and can not
> work right without also providing the fb helpers .gamma_set and
> .gamma_get thus rendering the code in this driver suspect.
> 
Hi Peter,
STM32 chipsets supports 8-bit CLUT mode but this driver version does not 
support it "yet" (final patch has not been upstreamed because it was a
too big fbdev patch for simply adding CLUT...).

Regarding your patch below, if it helps you to ease the drm framework 
update then I am agree to "acknowledge it" asap, else if you are not
in
a hurry, I would prefer a better and definitive patch handling 8-bit 
CLUT properly and I am ok to help or/and to do it : )

Extra questions:
- any plan to update modetest with the DRM_FORMAT_C8 support + gamma 
get/set?
- do you have a simple way to test clut with fbdev, last year we where 
using an old version of the SDL but I am still looking for a small piece 
of code to do it (else I will do it myself but C8 on fbdev is not really 
a priority ;-)

best regards,
Philippe
> Just remove the dead code.
> 
> Signed-off-by: Peter Rosin <peda at axentia.se>
> ---
>   drivers/gpu/drm/stm/ltdc.c | 12 ------------
>   drivers/gpu/drm/stm/ltdc.h |  1 -
>   2 files changed, 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 1b9483d..87829b9 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -375,17 +375,6 @@ static irqreturn_t ltdc_irq(int irq, void *arg)
>    * DRM_CRTC
>    */
>   
> -static void ltdc_crtc_load_lut(struct drm_crtc *crtc)
> -{
> -	struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> -	unsigned int i, lay;
> -
> -	for (lay = 0; lay < ldev->caps.nb_layers; lay++)
> -		for (i = 0; i < 256; i++)
> -			reg_write(ldev->regs, LTDC_L1CLUTWR + lay * LAY_OFS,
> -				  ldev->clut[i]);
> -}
> -
>   static void ltdc_crtc_enable(struct drm_crtc *crtc)
>   {
>   	struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> @@ -523,7 +512,6 @@ static void ltdc_crtc_atomic_flush(struct drm_crtc
*crtc,
>   }
>   
>   static struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
> -	.load_lut = ltdc_crtc_load_lut,
>   	.enable = ltdc_crtc_enable,
>   	.disable = ltdc_crtc_disable,
>   	.mode_set_nofb = ltdc_crtc_mode_set_nofb,
> diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
> index d7a9c73..620ca55 100644
> --- a/drivers/gpu/drm/stm/ltdc.h
> +++ b/drivers/gpu/drm/stm/ltdc.h
> @@ -27,7 +27,6 @@ struct ltdc_device {
>   	struct drm_panel *panel;
>   	struct mutex err_lock;	/* protecting error_status */
>   	struct ltdc_caps caps;
> -	u32 clut[256];		/* color look up table */
>   	u32 error_status;
>   	u32 irq_status;
>   };
>

On 2017-06-22 13:49, Philippe CORNU wrote:
> On 06/22/2017 08:06 AM, Peter Rosin wrote:
>> The redundant fb helper .load_lut is no longer used, and can not
>> work right without also providing the fb helpers .gamma_set and
>> .gamma_get thus rendering the code in this driver suspect.
>>
> 
> Hi Peter,
> STM32 chipsets supports 8-bit CLUT mode but this driver version does not 
> support it "yet" (final patch has not been upstreamed because it
was a
> too big fbdev patch for simply adding CLUT...).
> 
> Regarding your patch below, if it helps you to ease the drm framework 
> update then I am agree to "acknowledge it" asap, else if you are
not in
> a hurry, I would prefer a better and definitive patch handling 8-bit 
> CLUT properly and I am ok to help or/and to do it : )
Hi!

The thing is, without my series you will have to provide four callbacks.
The crtc .gamma_set and the three redundant fb helpers .gamma_get,
.gamma_set and .load_lut that pretty much does exactly what the crtc
.gamma_set is doing. Well not .gamma_get, but...

With my series, you only have to provide the crtc .gamma_set, which you
have to provide anyway. and ...the core will handle everything that
.gamma_get was used for...

I.e., your work to provide CLUT support should start with drm support,
which means the crtc .gamma_set, and then move on to the fbdev
emulation. And I have just eliminated the second step for you, and
as suger on top, you no longer have to convince the core drm maintainers
that adding a lot of fbdev emulation code is needed.

So, I think you actually want to wait for my series to land before adding
CLUT support.
> Extra questions:
> - any plan to update modetest with the DRM_FORMAT_C8 support + gamma 
> get/set?
I don't know that code base at all, but from the glimpse I got when browsing
it, it seemed like it was pretty hardwired to non-palettized modes. I ended
up having no need for it, see below...
> - do you have a simple way to test clut with fbdev, last year we where 
> using an old version of the SDL but I am still looking for a small piece 
> of code to do it (else I will do it myself but C8 on fbdev is not really 
> a priority ;-)
I'm doing pretty much the same thing, I have an application that requires
an old SDL, and I'm using the programs/demos/demo.c program from the very
old libggi as a second test app. But that's just because libggi is what
I'm most familiar with, and it doesn't try to be "nice" and do
things
automatically, instead you have to manually insert helpers providing
e.g. palette emulation if the application assumes a palettized mode and
only truecolor modes are available from the HW. SDL tends to add those
things for you, making it less easy to test thing, but I'm not an
"SDL-guy", so there may very well exist some knobs I don't know
about.

Oh, you probably didn't see this:
http://marc.info/?l=linux-kernel&m=149786920731175&w=4

It sports modeset-pal.c that sets the C8 mode, and does a 5 second
palette animation, w/o using fbdev. I used it instead of digging
further into modetest.

Cheers,
peda

On Thu, Jun 22, 2017 at 11:49:34AM +0000, Philippe CORNU
wrote:
> 
> 
> On 06/22/2017 08:06 AM, Peter Rosin wrote:
> > The redundant fb helper .load_lut is no longer used, and can not
> > work right without also providing the fb helpers .gamma_set and
> > .gamma_get thus rendering the code in this driver suspect.
> > 
> 
> Hi Peter,
> STM32 chipsets supports 8-bit CLUT mode but this driver version does not 
> support it "yet" (final patch has not been upstreamed because it
was a
> too big fbdev patch for simply adding CLUT...).
> 
> Regarding your patch below, if it helps you to ease the drm framework 
> update then I am agree to "acknowledge it" asap, else if you are
not in
> a hurry, I would prefer a better and definitive patch handling 8-bit 
> CLUT properly and I am ok to help or/and to do it : )
I'll take your ack, since your 8bit lut support will be massively simpler
with Peter's series here :-)
> Extra questions:
> - any plan to update modetest with the DRM_FORMAT_C8 support + gamma 
> get/set?
We have gamma igts, well for the fancy new atomic color manager stuff.
Testing drivers with modetest is kinda not that awesome :-)
-Daniel
> - do you have a simple way to test clut with fbdev, last year we where 
> using an old version of the SDL but I am still looking for a small piece 
> of code to do it (else I will do it myself but C8 on fbdev is not really 
> a priority ;-)
> 
> best regards,
> Philippe
> 
> > Just remove the dead code.
> > 
> > Signed-off-by: Peter Rosin <peda at axentia.se>
> > ---
> >   drivers/gpu/drm/stm/ltdc.c | 12 ------------
> >   drivers/gpu/drm/stm/ltdc.h |  1 -
> >   2 files changed, 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> > index 1b9483d..87829b9 100644
> > --- a/drivers/gpu/drm/stm/ltdc.c
> > +++ b/drivers/gpu/drm/stm/ltdc.c
> > @@ -375,17 +375,6 @@ static irqreturn_t ltdc_irq(int irq, void *arg)
> >    * DRM_CRTC
> >    */
> >   
> > -static void ltdc_crtc_load_lut(struct drm_crtc *crtc)
> > -{
> > -	struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> > -	unsigned int i, lay;
> > -
> > -	for (lay = 0; lay < ldev->caps.nb_layers; lay++)
> > -		for (i = 0; i < 256; i++)
> > -			reg_write(ldev->regs, LTDC_L1CLUTWR + lay * LAY_OFS,
> > -				  ldev->clut[i]);
> > -}
> > -
> >   static void ltdc_crtc_enable(struct drm_crtc *crtc)
> >   {
> >   	struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> > @@ -523,7 +512,6 @@ static void ltdc_crtc_atomic_flush(struct drm_crtc
*crtc,
> >   }
> >   
> >   static struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
> > -	.load_lut = ltdc_crtc_load_lut,
> >   	.enable = ltdc_crtc_enable,
> >   	.disable = ltdc_crtc_disable,
> >   	.mode_set_nofb = ltdc_crtc_mode_set_nofb,
> > diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
> > index d7a9c73..620ca55 100644
> > --- a/drivers/gpu/drm/stm/ltdc.h
> > +++ b/drivers/gpu/drm/stm/ltdc.h
> > @@ -27,7 +27,6 @@ struct ltdc_device {
> >   	struct drm_panel *panel;
> >   	struct mutex err_lock;	/* protecting error_status */
> >   	struct ltdc_caps caps;
> > -	u32 clut[256];		/* color look up table */
> >   	u32 error_status;
> >   	u32 irq_status;
> >   };
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch