| Message ID | 1498111597-10714-14-git-send-email-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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; };
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. Just remove the dead code. Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/gpu/drm/stm/ltdc.c | 12 ------------ drivers/gpu/drm/stm/ltdc.h | 1 - 2 files changed, 13 deletions(-)