| Message ID | 1570454955-21298-2-git-send-email-yannick.fertre@st.com |
|---|---|
| State | Superseded |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
| Series | splash screen on the stm32f769 & stm32mp1 boards | expand |
On 10/7/19 3:29 PM, Yannick Fertré wrote: > If the size of the bitmap is bigger than the size of > the panel then errors appear when calculating axis alignment > and the copy of bitmap is done outside of framebuffer. > > Signed-off-by: Yannick Fertré <yannick.fertre@st.com> > --- > drivers/video/video_bmp.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/video/video_bmp.c b/drivers/video/video_bmp.c > index 193f37d..4af1fb4 100644 > --- a/drivers/video/video_bmp.c > +++ b/drivers/video/video_bmp.c > @@ -249,6 +249,13 @@ int video_bmp_display(struct udevice *dev, ulong bmp_image, int x, int y, > > padded_width = (width & 0x3 ? (width & ~0x3) + 4 : width); > > + /* check if picture size exceeds panel size */ > + if ((pwidth < width) || (priv->ysize < height)) { > + printf("Error: BMP size %d x %d is bigger than panel size %d x %d\n", > + (int)width, (int)height, priv->xsize, priv->ysize); > + return -EINVAL; > + } > + > if (align) { > video_splash_align_axis(&x, priv->xsize, width); > video_splash_align_axis(&y, priv->ysize, height); This is followed by: if ((x + width) > pwidth) width = pwidth - x; if ((y + height) > priv->ysize) height = priv->ysize - y; These lines will clip a bitmap that given the left upper corner x, y does not fit onto the screen. So isn't this patch superfluous? What is missing are checks for x and y. For testing I have used qemu_x86 and added #define CONFIG_BMP_24BPP y to the top of drivers/video/video_bmp.c and loaded a 24bit bitmap. Clipping works as expected. But passing a value of x exceeding the screen width lead to a crash. What I really dislike in the existing coding is that CONFIG_BMP_*BPP is defined in includes instead of using Kconfig but that is a different story. Best regards Heinrich
Hi Heinrich, all On 10/7/19 7:34 PM, Heinrich Schuchardt wrote: > On 10/7/19 3:29 PM, Yannick Fertré wrote: >> If the size of the bitmap is bigger than the size of >> the panel then errors appear when calculating axis alignment >> and the copy of bitmap is done outside of framebuffer. >> >> Signed-off-by: Yannick Fertré <yannick.fertre@st.com> >> --- >> drivers/video/video_bmp.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/video/video_bmp.c b/drivers/video/video_bmp.c >> index 193f37d..4af1fb4 100644 >> --- a/drivers/video/video_bmp.c >> +++ b/drivers/video/video_bmp.c >> @@ -249,6 +249,13 @@ int video_bmp_display(struct udevice *dev, ulong bmp_image, int x, int y, >> >> padded_width = (width & 0x3 ? (width & ~0x3) + 4 : width); >> >> + /* check if picture size exceeds panel size */ >> + if ((pwidth < width) || (priv->ysize < height)) { >> + printf("Error: BMP size %d x %d is bigger than panel size %d x %d\n", >> + (int)width, (int)height, priv->xsize, priv->ysize); >> + return -EINVAL; >> + } >> + >> if (align) { >> video_splash_align_axis(&x, priv->xsize, width); >> video_splash_align_axis(&y, priv->ysize, height); > > This is followed by: > > if ((x + width) > pwidth) > width = pwidth - x; > if ((y + height) > priv->ysize) > height = priv->ysize - y; > > These lines will clip a bitmap that given the left upper corner x, y > does not fit onto the screen. > > So isn't this patch superfluous? > > What is missing are checks for x and y. > > For testing I have used qemu_x86 and added > > #define CONFIG_BMP_24BPP y > > to the top of drivers/video/video_bmp.c and loaded a 24bit bitmap. > > Clipping works as expected. But passing a value of x exceeding the > screen width lead to a crash. > > What I really dislike in the existing coding is that CONFIG_BMP_*BPP is > defined in includes instead of using Kconfig but that is a different story. > > Best regards > > Heinrich For information all this series patches have been applied on u-boot/master excepts this one. Unfortunately, without this patch, stm32f746-discovery board doesn't boot anymore. Heinrich, could this patch be merged, waiting for a clean patch from Yannick ? Regards Patrice
Hi Patrice, On Thu, 24 Oct 2019 14:05:41 +0000 Patrice CHOTARD patrice.chotard@st.com wrote: ... > For information all this series patches have been applied on > u-boot/master excepts this one. Yes, I skipped this patch when applying the series since it was under discussion. > Unfortunately, without this patch, stm32f746-discovery board doesn't boot anymore. > > Heinrich, could this patch be merged, waiting for a clean patch from Yannick ? As far as I can see, Yannick didn't respond to questions from Heinrich yet. -- Anatolij
On 10/24/19 4:05 PM, Patrice CHOTARD wrote: > Hi Heinrich, all > > On 10/7/19 7:34 PM, Heinrich Schuchardt wrote: >> On 10/7/19 3:29 PM, Yannick Fertré wrote: >>> If the size of the bitmap is bigger than the size of >>> the panel then errors appear when calculating axis alignment >>> and the copy of bitmap is done outside of framebuffer. >>> >>> Signed-off-by: Yannick Fertré <yannick.fertre@st.com> >>> --- >>> drivers/video/video_bmp.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/video/video_bmp.c b/drivers/video/video_bmp.c >>> index 193f37d..4af1fb4 100644 >>> --- a/drivers/video/video_bmp.c >>> +++ b/drivers/video/video_bmp.c >>> @@ -249,6 +249,13 @@ int video_bmp_display(struct udevice *dev, ulong bmp_image, int x, int y, >>> >>> padded_width = (width & 0x3 ? (width & ~0x3) + 4 : width); >>> >>> + /* check if picture size exceeds panel size */ >>> + if ((pwidth < width) || (priv->ysize < height)) { >>> + printf("Error: BMP size %d x %d is bigger than panel size %d x %d\n", >>> + (int)width, (int)height, priv->xsize, priv->ysize); >>> + return -EINVAL; >>> + } >>> + >>> if (align) { >>> video_splash_align_axis(&x, priv->xsize, width); >>> video_splash_align_axis(&y, priv->ysize, height); >> >> This is followed by: >> >> if ((x + width) > pwidth) >> width = pwidth - x; >> if ((y + height) > priv->ysize) >> height = priv->ysize - y; >> >> These lines will clip a bitmap that given the left upper corner x, y >> does not fit onto the screen. >> >> So isn't this patch superfluous? >> >> What is missing are checks for x and y. >> >> For testing I have used qemu_x86 and added >> >> #define CONFIG_BMP_24BPP y >> >> to the top of drivers/video/video_bmp.c and loaded a 24bit bitmap. >> >> Clipping works as expected. But passing a value of x exceeding the >> screen width lead to a crash. >> >> What I really dislike in the existing coding is that CONFIG_BMP_*BPP is >> defined in includes instead of using Kconfig but that is a different story. >> >> Best regards >> >> Heinrich > > For information all this series patches have been applied on u-boot/master excepts this one. > > Unfortunately, without this patch, stm32f746-discovery board doesn't boot anymore. I still do not understand why this patch is needed. Could you, please, try to analyze why the board does not boot. What is wrong with the existing code for clipping? Or is the non-booting just coincidence but the bug is somewhere else? What are the values of the parameters passed to video_bmp_display()? Which bitmap file are you using? What is the size of the display? Best regards Heinrich > > Heinrich, could this patch be merged, waiting for a clean patch from Yannick ? > > Regards > > Patrice >
Hi Heinrich On 10/24/19 10:43 PM, Heinrich Schuchardt wrote: > On 10/24/19 4:05 PM, Patrice CHOTARD wrote: >> Hi Heinrich, all >> >> On 10/7/19 7:34 PM, Heinrich Schuchardt wrote: >>> On 10/7/19 3:29 PM, Yannick Fertré wrote: >>>> If the size of the bitmap is bigger than the size of >>>> the panel then errors appear when calculating axis alignment >>>> and the copy of bitmap is done outside of framebuffer. >>>> >>>> Signed-off-by: Yannick Fertré <yannick.fertre@st.com> >>>> --- >>>> drivers/video/video_bmp.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/drivers/video/video_bmp.c b/drivers/video/video_bmp.c >>>> index 193f37d..4af1fb4 100644 >>>> --- a/drivers/video/video_bmp.c >>>> +++ b/drivers/video/video_bmp.c >>>> @@ -249,6 +249,13 @@ int video_bmp_display(struct udevice *dev, ulong bmp_image, int x, int y, >>>> >>>> padded_width = (width & 0x3 ? (width & ~0x3) + 4 : width); >>>> >>>> + /* check if picture size exceeds panel size */ >>>> + if ((pwidth < width) || (priv->ysize < height)) { >>>> + printf("Error: BMP size %d x %d is bigger than panel size %d x %d\n", >>>> + (int)width, (int)height, priv->xsize, priv->ysize); >>>> + return -EINVAL; >>>> + } >>>> + >>>> if (align) { >>>> video_splash_align_axis(&x, priv->xsize, width); >>>> video_splash_align_axis(&y, priv->ysize, height); >>> >>> This is followed by: >>> >>> if ((x + width) > pwidth) >>> width = pwidth - x; >>> if ((y + height) > priv->ysize) >>> height = priv->ysize - y; >>> >>> These lines will clip a bitmap that given the left upper corner x, y >>> does not fit onto the screen. >>> >>> So isn't this patch superfluous? >>> >>> What is missing are checks for x and y. >>> >>> For testing I have used qemu_x86 and added >>> >>> #define CONFIG_BMP_24BPP y >>> >>> to the top of drivers/video/video_bmp.c and loaded a 24bit bitmap. >>> >>> Clipping works as expected. But passing a value of x exceeding the >>> screen width lead to a crash. >>> >>> What I really dislike in the existing coding is that CONFIG_BMP_*BPP is >>> defined in includes instead of using Kconfig but that is a different story. >>> >>> Best regards >>> >>> Heinrich >> >> For information all this series patches have been applied on u-boot/master excepts this one. >> >> Unfortunately, without this patch, stm32f746-discovery board doesn't boot anymore. > > I still do not understand why this patch is needed. > > Could you, please, try to analyze why the board does not boot. > > What is wrong with the existing code for clipping? > Or is the non-booting just coincidence but the bug is somewhere else? > > What are the values of the parameters passed to video_bmp_display()? > Which bitmap file are you using? > What is the size of the display? To sum-up, on all STM32 boards, the same BMP splashcreen is used. In the particular case of STM32F746-Disco board, the panel size is smaller then the BMP size. So, some size check must be done to avoid overflow when writing inside the framebuffer. If needed, Yannick, which is multimedia expert, can give you more precise details. Thanks Patrice > > Best regards > > Heinrich > >> >> Heinrich, could this patch be merged, waiting for a clean patch from Yannick ? >> >> Regards >> >> Patrice >> >
Hello Heinrich, Sorry for the delay. This match is not superfluous. On the STM32F746 board, a bitmap larger than the panel resolution is embedded. Without this patch, the board does not boot. I propose to send an additional patch that checks the coordinates. Best regards
On 10/25/19 9:43 AM, Patrice CHOTARD wrote: > Hi Heinrich > > On 10/24/19 10:43 PM, Heinrich Schuchardt wrote: >> On 10/24/19 4:05 PM, Patrice CHOTARD wrote: >>> Hi Heinrich, all >>> >>> On 10/7/19 7:34 PM, Heinrich Schuchardt wrote: >>>> On 10/7/19 3:29 PM, Yannick Fertré wrote: >>>>> If the size of the bitmap is bigger than the size of >>>>> the panel then errors appear when calculating axis alignment >>>>> and the copy of bitmap is done outside of framebuffer. >>>>> >>>>> Signed-off-by: Yannick Fertré <yannick.fertre@st.com> >>>>> --- >>>>> drivers/video/video_bmp.c | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/drivers/video/video_bmp.c b/drivers/video/video_bmp.c >>>>> index 193f37d..4af1fb4 100644 >>>>> --- a/drivers/video/video_bmp.c >>>>> +++ b/drivers/video/video_bmp.c >>>>> @@ -249,6 +249,13 @@ int video_bmp_display(struct udevice *dev, ulong bmp_image, int x, int y, >>>>> >>>>> padded_width = (width & 0x3 ? (width & ~0x3) + 4 : width); >>>>> >>>>> + /* check if picture size exceeds panel size */ >>>>> + if ((pwidth < width) || (priv->ysize < height)) { >>>>> + printf("Error: BMP size %d x %d is bigger than panel size %d x %d\n", >>>>> + (int)width, (int)height, priv->xsize, priv->ysize); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> if (align) { >>>>> video_splash_align_axis(&x, priv->xsize, width); >>>>> video_splash_align_axis(&y, priv->ysize, height); >>>> >>>> This is followed by: >>>> >>>> if ((x + width) > pwidth) >>>> width = pwidth - x; >>>> if ((y + height) > priv->ysize) >>>> height = priv->ysize - y; >>>> >>>> These lines will clip a bitmap that given the left upper corner x, y >>>> does not fit onto the screen. >>>> >>>> So isn't this patch superfluous? >>>> >>>> What is missing are checks for x and y. >>>> >>>> For testing I have used qemu_x86 and added >>>> >>>> #define CONFIG_BMP_24BPP y >>>> >>>> to the top of drivers/video/video_bmp.c and loaded a 24bit bitmap. >>>> >>>> Clipping works as expected. But passing a value of x exceeding the >>>> screen width lead to a crash. >>>> >>>> What I really dislike in the existing coding is that CONFIG_BMP_*BPP is >>>> defined in includes instead of using Kconfig but that is a different story. >>>> >>>> Best regards >>>> >>>> Heinrich >>> >>> For information all this series patches have been applied on u-boot/master excepts this one. >>> >>> Unfortunately, without this patch, stm32f746-discovery board doesn't boot anymore. >> >> I still do not understand why this patch is needed. >> >> Could you, please, try to analyze why the board does not boot. >> >> What is wrong with the existing code for clipping? >> Or is the non-booting just coincidence but the bug is somewhere else? >> >> What are the values of the parameters passed to video_bmp_display()? >> Which bitmap file are you using? >> What is the size of the display? > > > To sum-up, on all STM32 boards, the same BMP splashcreen is used. > > In the particular case of STM32F746-Disco board, the panel size is smaller then the BMP size. > > So, some size check must be done to avoid overflow when writing inside the framebuffer. That is why we have lines to clip images: if ((x + width) > pwidth) width = pwidth - x; if ((y + height) > priv->ysize) height = priv->ysize - y; Why is this not working in you case? Best regards Heinrich > > If needed, Yannick, which is multimedia expert, can give you more precise details. > > Thanks > > Patrice > > >> >> Best regards >> >> Heinrich >> >>> >>> Heinrich, could this patch be merged, waiting for a clean patch from Yannick ? >>> >>> Regards >>> >>> Patrice >>> >> >
diff --git a/drivers/video/video_bmp.c b/drivers/video/video_bmp.c index 193f37d..4af1fb4 100644 --- a/drivers/video/video_bmp.c +++ b/drivers/video/video_bmp.c @@ -249,6 +249,13 @@ int video_bmp_display(struct udevice *dev, ulong bmp_image, int x, int y, padded_width = (width & 0x3 ? (width & ~0x3) + 4 : width); + /* check if picture size exceeds panel size */ + if ((pwidth < width) || (priv->ysize < height)) { + printf("Error: BMP size %d x %d is bigger than panel size %d x %d\n", + (int)width, (int)height, priv->xsize, priv->ysize); + return -EINVAL; + } + if (align) { video_splash_align_axis(&x, priv->xsize, width); video_splash_align_axis(&y, priv->ysize, height);
If the size of the bitmap is bigger than the size of the panel then errors appear when calculating axis alignment and the copy of bitmap is done outside of framebuffer. Signed-off-by: Yannick Fertré <yannick.fertre@st.com> --- drivers/video/video_bmp.c | 7 +++++++ 1 file changed, 7 insertions(+)