[PATCH] hw/display/bcm2835_fb: fix fb_use_offsets condition

SamJakob posted 1 patch 4 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240713155656.59431-1-me@samjakob.com
Maintainers: Peter Maydell <peter.maydell@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
There is a newer version of this series
hw/display/bcm2835_fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] hw/display/bcm2835_fb: fix fb_use_offsets condition
Posted by SamJakob 4 months, 1 week ago
	It is common practice when implementing double-buffering
	on VideoCore to do so by multiplying the height of the
	virtual buffer by the number of virtual screens desired
	(i.e., two - in the case of double-bufferring).

	At present, this won't work in QEMU because the logic in
	fb_use_offsets require that both the virtual width and
	height exceed their physical counterparts.

	This appears to be unintentional/a typo and indeed the
	comment states; "Experimentally, the hardware seems to
	do this only if the viewport size is larger than the
	physical screen". The viewport/virtual size would be
	larger than the physical size if either virtual dimension
	were larger than their physical counterparts and not
	necessarily both.

Signed-off-by: SamJakob <me@samjakob.com>
---
 hw/display/bcm2835_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index e40ed2d2e1..650db3da82 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -145,7 +145,7 @@ static bool fb_use_offsets(BCM2835FBConfig *config)
      * viewport size is larger than the physical screen. (It doesn't
      * prevent the guest setting this silly viewport setting, though...)
      */
-    return config->xres_virtual > config->xres &&
+    return config->xres_virtual > config->xres ||
         config->yres_virtual > config->yres;
 }
 
-- 
2.45.2
Re: [PATCH] hw/display/bcm2835_fb: fix fb_use_offsets condition
Posted by Philippe Mathieu-Daudé 4 months, 1 week ago
On 13/7/24 17:56, SamJakob wrote:
> 	It is common practice when implementing double-buffering
> 	on VideoCore to do so by multiplying the height of the
> 	virtual buffer by the number of virtual screens desired
> 	(i.e., two - in the case of double-bufferring).
> 
> 	At present, this won't work in QEMU because the logic in
> 	fb_use_offsets require that both the virtual width and
> 	height exceed their physical counterparts.
> 
> 	This appears to be unintentional/a typo and indeed the
> 	comment states; "Experimentally, the hardware seems to
> 	do this only if the viewport size is larger than the
> 	physical screen". The viewport/virtual size would be
> 	larger than the physical size if either virtual dimension
> 	were larger than their physical counterparts and not
> 	necessarily both.
> 
> Signed-off-by: SamJakob <me@samjakob.com>
> ---
>   hw/display/bcm2835_fb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
> index e40ed2d2e1..650db3da82 100644
> --- a/hw/display/bcm2835_fb.c
> +++ b/hw/display/bcm2835_fb.c
> @@ -145,7 +145,7 @@ static bool fb_use_offsets(BCM2835FBConfig *config)
>        * viewport size is larger than the physical screen. (It doesn't
>        * prevent the guest setting this silly viewport setting, though...)
>        */
> -    return config->xres_virtual > config->xres &&
> +    return config->xres_virtual > config->xres ||
>           config->yres_virtual > config->yres;
>   }
>   

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>