| Message ID | 1509016666-18927-1-git-send-email-philippe.cornu@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
+ Peter Hi Peter, CLUT support on STM32 has been removed thanks to your clean up patch 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@st.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, <dc_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 */ >
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! 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? 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@st.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, <dc_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 */
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@st.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, <dc_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 */ >
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? Cheers. Peter
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. Many thanks Philippe :) > Cheers. > Peter >
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 >>
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
Hi all, Do you think the patch is "acceptable" or should I change it somehow? Any opinion is welcomed : ) Many thanks, Philippe :-) On 11/24/2017 02:54 PM, 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? > Many thanks, > Philippe :-) > >> >> Many thanks >> Philippe :) >> >>> Cheers. >>> Peter >>>
On 2018-01-09 12:28, Philippe CORNU wrote: > Hi all, > > Do you think the patch is "acceptable" or should I change it somehow? > Any opinion is welcomed : ) Maybe you should try Daniel Vetter? He was very helpful (thanks) when I worked on the clut changes... Cheers, Peter
2018-01-09 12:56 GMT+01:00 Peter Rosin <peda@axentia.se>: > On 2018-01-09 12:28, Philippe CORNU wrote: >> Hi all, >> >> Do you think the patch is "acceptable" or should I change it somehow? >> Any opinion is welcomed : ) > > Maybe you should try Daniel Vetter? He was very helpful (thanks) when > I worked on the clut changes... Since it only impact stm driver, I have applied it on drm-misc-next. Regards, Benjamin > > Cheers, > Peter
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, <dc_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 */
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@st.com> --- drivers/gpu/drm/stm/ltdc.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)