| Message ID | 20221011231048.505967-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/stm: Fix resolution bitmasks | expand |
Hi Marek, thanks for the patch. Reviewed-by: Yannick Fertre <yannick.fertre@foss.st.com> On 10/12/22 01:10, Marek Vasut wrote: > STM32MP15xx RM0436 Rev 6 "35.7.3 LTDC synchronization size configuration > register (LTDC_SSCR)" on page 1784 and onward indicates VSH and similar > bits are all [11:0] instead of [10:0] wide. Fix this. > > [1] https://www.st.com/resource/en/reference_manual/DM00327659-.pdf > > Fixes: b759012c5fa7 ("drm/stm: Add STM32 LTDC driver") > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > Cc: Antonio Borneo <antonio.borneo@foss.st.com> > Cc: Benjamin Gaignard <benjamin.gaignard@foss.st.com> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Philippe Cornu <philippe.cornu@foss.st.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Vincent Abriou <vincent.abriou@foss.st.com> > Cc: Yannick Fertre <yannick.fertre@foss.st.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-stm32@st-md-mailman.stormreply.com > To: dri-devel@lists.freedesktop.org > --- > drivers/gpu/drm/stm/ltdc.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c > index 03c6becda795c..639ed00b44a57 100644 > --- a/drivers/gpu/drm/stm/ltdc.c > +++ b/drivers/gpu/drm/stm/ltdc.c > @@ -111,16 +111,16 @@ > #define LTDC_L1FPF1R (ldev->caps.layer_regs[24]) /* L1 Flexible Pixel Format 1 */ > > /* Bit definitions */ > -#define SSCR_VSH GENMASK(10, 0) /* Vertical Synchronization Height */ > +#define SSCR_VSH GENMASK(11, 0) /* Vertical Synchronization Height */ > #define SSCR_HSW GENMASK(27, 16) /* Horizontal Synchronization Width */ > > -#define BPCR_AVBP GENMASK(10, 0) /* Accumulated Vertical Back Porch */ > +#define BPCR_AVBP GENMASK(11, 0) /* Accumulated Vertical Back Porch */ > #define BPCR_AHBP GENMASK(27, 16) /* Accumulated Horizontal Back Porch */ > > -#define AWCR_AAH GENMASK(10, 0) /* Accumulated Active Height */ > +#define AWCR_AAH GENMASK(11, 0) /* Accumulated Active Height */ > #define AWCR_AAW GENMASK(27, 16) /* Accumulated Active Width */ > > -#define TWCR_TOTALH GENMASK(10, 0) /* TOTAL Height */ > +#define TWCR_TOTALH GENMASK(11, 0) /* TOTAL Height */ > #define TWCR_TOTALW GENMASK(27, 16) /* TOTAL Width */ > > #define GCR_LTDCEN BIT(0) /* LTDC ENable */
Hi Marek, The genmask of regsiter SSCR, BPCR & others were setted accordly to the chipset stm32f4. You can see more details on page 493 of reference manual RM0090: https://www.st.com/resource/en/reference_manual/DM00031020-.pdf With future hardware, all of these registers will aligned on 16bits. I would like to know if you use a display which resolution exceed 2048. Best regards Yannick Fertré On 10/14/22 14:17, Yannick FERTRE wrote: > Hi Marek, > > thanks for the patch. > > Reviewed-by: Yannick Fertre <yannick.fertre@foss.st.com> > > On 10/12/22 01:10, Marek Vasut wrote: >> STM32MP15xx RM0436 Rev 6 "35.7.3 LTDC synchronization size configuration >> register (LTDC_SSCR)" on page 1784 and onward indicates VSH and similar >> bits are all [11:0] instead of [10:0] wide. Fix this. >> >> [1] https://www.st.com/resource/en/reference_manual/DM00327659-.pdf >> >> Fixes: b759012c5fa7 ("drm/stm: Add STM32 LTDC driver") >> Signed-off-by: Marek Vasut <marex@denx.de> >> --- >> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> >> Cc: Antonio Borneo <antonio.borneo@foss.st.com> >> Cc: Benjamin Gaignard <benjamin.gaignard@foss.st.com> >> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> >> Cc: Philippe Cornu <philippe.cornu@foss.st.com> >> Cc: Sam Ravnborg <sam@ravnborg.org> >> Cc: Vincent Abriou <vincent.abriou@foss.st.com> >> Cc: Yannick Fertre <yannick.fertre@foss.st.com> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-stm32@st-md-mailman.stormreply.com >> To: dri-devel@lists.freedesktop.org >> --- >> drivers/gpu/drm/stm/ltdc.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c >> index 03c6becda795c..639ed00b44a57 100644 >> --- a/drivers/gpu/drm/stm/ltdc.c >> +++ b/drivers/gpu/drm/stm/ltdc.c >> @@ -111,16 +111,16 @@ >> #define LTDC_L1FPF1R (ldev->caps.layer_regs[24]) /* L1 >> Flexible Pixel Format 1 */ >> /* Bit definitions */ >> -#define SSCR_VSH GENMASK(10, 0) /* Vertical Synchronization >> Height */ >> +#define SSCR_VSH GENMASK(11, 0) /* Vertical Synchronization >> Height */ >> #define SSCR_HSW GENMASK(27, 16) /* Horizontal >> Synchronization Width */ >> -#define BPCR_AVBP GENMASK(10, 0) /* Accumulated Vertical >> Back Porch */ >> +#define BPCR_AVBP GENMASK(11, 0) /* Accumulated Vertical Back >> Porch */ >> #define BPCR_AHBP GENMASK(27, 16) /* Accumulated Horizontal >> Back Porch */ >> -#define AWCR_AAH GENMASK(10, 0) /* Accumulated Active Height */ >> +#define AWCR_AAH GENMASK(11, 0) /* Accumulated Active Height */ >> #define AWCR_AAW GENMASK(27, 16) /* Accumulated Active Width */ >> -#define TWCR_TOTALH GENMASK(10, 0) /* TOTAL Height */ >> +#define TWCR_TOTALH GENMASK(11, 0) /* TOTAL Height */ >> #define TWCR_TOTALW GENMASK(27, 16) /* TOTAL Width */ >> #define GCR_LTDCEN BIT(0) /* LTDC ENable */
On 10/14/22 15:42, Yannick FERTRE wrote: > Hi Marek, Hello Yannick, > The genmask of regsiter SSCR, BPCR & others were setted accordly to the > chipset stm32f4. So that means: F4 -> 2048x2048 framebuffer H7/MP1 -> 4096x4096 framebuffer ? We should then also update STM_MAX_FB_WIDTH/STM_MAX_FB_HEIGHT per SoC type ? > You can see more details on page 493 of reference manual RM0090: > > https://www.st.com/resource/en/reference_manual/DM00031020-.pdf > > With future hardware, all of these registers will aligned on 16bits. > > I would like to know if you use a display which resolution exceed 2048. Not at all, 480x640 . The display is just flaky and I'm trying to figure out what's wrong with it, so I'm checking all over the place and noticed this discrepancy.
On 10/14/22 17:55, Marek Vasut wrote: > On 10/14/22 15:42, Yannick FERTRE wrote: >> Hi Marek, > > Hello Yannick, > >> The genmask of regsiter SSCR, BPCR & others were setted accordly to >> the chipset stm32f4. > > So that means: > F4 -> 2048x2048 framebuffer > H7/MP1 -> 4096x4096 framebuffer > ? Worse F4 is 2048x2048 F7 is 4096x2048 MP1 is 4096x4096 and there is no IDR register on F4/F7 like on MP1, or is there ? How else can we tell those LTDC versions apart ?
On 10/14/22 19:15, Marek Vasut wrote: > On 10/14/22 17:55, Marek Vasut wrote: >> On 10/14/22 15:42, Yannick FERTRE wrote: >>> Hi Marek, >> >> Hello Yannick, >> >>> The genmask of regsiter SSCR, BPCR & others were setted accordly to >>> the chipset stm32f4. >> >> So that means: >> F4 -> 2048x2048 framebuffer >> H7/MP1 -> 4096x4096 framebuffer >> ? > > Worse > > F4 is 2048x2048 > F7 is 4096x2048 > MP1 is 4096x4096 > > and there is no IDR register on F4/F7 like on MP1, or is there ? > > How else can we tell those LTDC versions apart ? > > Dear Marek, Many thanks for your patch (and sorry for this late reply). Your patch is good and fixes this ltdc driver source code vs. the related reference manual. imho, it will not be an issue for F4 & F7 series if these bit-fields are "bigger" as I am pretty sure stm32 MCUs are not really using such high resolutions. Yannick already replied with his reviewed-by. I add my Acked-by: Philippe Cornu <philippe.cornu@foss.st.com> If you agree, I will merge your patch really soon. Dear Yannick, You may add to your todo list to double check if there is a need to detect stm32 MCUs vs. these bit-field sizes... Many thanks Philippe :-)
On 5/15/23 18:02, Philippe CORNU wrote: Hi, >>>> The genmask of regsiter SSCR, BPCR & others were setted accordly to >>>> the chipset stm32f4. >>> >>> So that means: >>> F4 -> 2048x2048 framebuffer >>> H7/MP1 -> 4096x4096 framebuffer >>> ? >> >> Worse >> >> F4 is 2048x2048 >> F7 is 4096x2048 >> MP1 is 4096x4096 >> >> and there is no IDR register on F4/F7 like on MP1, or is there ? >> >> How else can we tell those LTDC versions apart ? >> >> > > Dear Marek, > Many thanks for your patch (and sorry for this late reply). > Your patch is good and fixes this ltdc driver source code vs. the > related reference manual. > imho, it will not be an issue for F4 & F7 series if these bit-fields are > "bigger" as I am pretty sure stm32 MCUs are not really using such high > resolutions. > Yannick already replied with his reviewed-by. I add my > > Acked-by: Philippe Cornu <philippe.cornu@foss.st.com> > > If you agree, I will merge your patch really soon. I would say there is no rush, so let's get this done right . > Dear Yannick, > You may add to your todo list to double check if there is a need to > detect stm32 MCUs vs. these bit-field sizes... Can we use a compatible string , or I think there is some ID register ? [...] btw I only received this email now, odd, I wonder whether it was stuck in some SMTP server. Sorry for the delayed reply, it was out of my control.
On 5/26/23 11:05, Marek Vasut wrote: > On 5/15/23 18:02, Philippe CORNU wrote: > > Hi, > >>>>> The genmask of regsiter SSCR, BPCR & others were setted accordly to >>>>> the chipset stm32f4. >>>> >>>> So that means: >>>> F4 -> 2048x2048 framebuffer >>>> H7/MP1 -> 4096x4096 framebuffer >>>> ? >>> >>> Worse >>> >>> F4 is 2048x2048 >>> F7 is 4096x2048 >>> MP1 is 4096x4096 >>> >>> and there is no IDR register on F4/F7 like on MP1, or is there ? >>> >>> How else can we tell those LTDC versions apart ? >>> >>> >> >> Dear Marek, >> Many thanks for your patch (and sorry for this late reply). >> Your patch is good and fixes this ltdc driver source code vs. the >> related reference manual. >> imho, it will not be an issue for F4 & F7 series if these bit-fields >> are "bigger" as I am pretty sure stm32 MCUs are not really using such >> high resolutions. >> Yannick already replied with his reviewed-by. I add my >> >> Acked-by: Philippe Cornu <philippe.cornu@foss.st.com> >> >> If you agree, I will merge your patch really soon. > > I would say there is no rush, so let's get this done right . > >> Dear Yannick, >> You may add to your todo list to double check if there is a need to >> detect stm32 MCUs vs. these bit-field sizes... > > Can we use a compatible string , or I think there is some ID register ? > > [...] > > btw I only received this email now, odd, I wonder whether it was stuck > in some SMTP server. Sorry for the delayed reply, it was out of my control. Hi Marek, Thank you for your feedback, I agree it is better to fix this properly. Note: After a quick check, I'm 99% sure that the smtp problem was on my end (although I don't understand why I know we've had some issues with the mailing lists over the past few weeks), so all my apologies for that :-) Hi Yannick, May I ask you please to prepare this clean up (taking into account all ltdc versions). Many thanks Philippe :-)
diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c index 03c6becda795c..639ed00b44a57 100644 --- a/drivers/gpu/drm/stm/ltdc.c +++ b/drivers/gpu/drm/stm/ltdc.c @@ -111,16 +111,16 @@ #define LTDC_L1FPF1R (ldev->caps.layer_regs[24]) /* L1 Flexible Pixel Format 1 */ /* Bit definitions */ -#define SSCR_VSH GENMASK(10, 0) /* Vertical Synchronization Height */ +#define SSCR_VSH GENMASK(11, 0) /* Vertical Synchronization Height */ #define SSCR_HSW GENMASK(27, 16) /* Horizontal Synchronization Width */ -#define BPCR_AVBP GENMASK(10, 0) /* Accumulated Vertical Back Porch */ +#define BPCR_AVBP GENMASK(11, 0) /* Accumulated Vertical Back Porch */ #define BPCR_AHBP GENMASK(27, 16) /* Accumulated Horizontal Back Porch */ -#define AWCR_AAH GENMASK(10, 0) /* Accumulated Active Height */ +#define AWCR_AAH GENMASK(11, 0) /* Accumulated Active Height */ #define AWCR_AAW GENMASK(27, 16) /* Accumulated Active Width */ -#define TWCR_TOTALH GENMASK(10, 0) /* TOTAL Height */ +#define TWCR_TOTALH GENMASK(11, 0) /* TOTAL Height */ #define TWCR_TOTALW GENMASK(27, 16) /* TOTAL Width */ #define GCR_LTDCEN BIT(0) /* LTDC ENable */
STM32MP15xx RM0436 Rev 6 "35.7.3 LTDC synchronization size configuration register (LTDC_SSCR)" on page 1784 and onward indicates VSH and similar bits are all [11:0] instead of [10:0] wide. Fix this. [1] https://www.st.com/resource/en/reference_manual/DM00327659-.pdf Fixes: b759012c5fa7 ("drm/stm: Add STM32 LTDC driver") Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> Cc: Antonio Borneo <antonio.borneo@foss.st.com> Cc: Benjamin Gaignard <benjamin.gaignard@foss.st.com> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> Cc: Philippe Cornu <philippe.cornu@foss.st.com> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Vincent Abriou <vincent.abriou@foss.st.com> Cc: Yannick Fertre <yannick.fertre@foss.st.com> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-stm32@st-md-mailman.stormreply.com To: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/stm/ltdc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)