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

[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 Peter Maydell 4 months, 1 week ago

On Sat, 13 Jul 2024 at 17:04, SamJakob <me@samjakob.com> 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>

Thanks for this bugfix; I've applied it to my target-arm.next
queue and it should get upstream within a week or so.

-- PMM

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

Posted by Philippe Mathieu-Daudé 4 months, 1 week ago

On 16/7/24 12:35, Peter Maydell wrote:
> On Sat, 13 Jul 2024 at 17:04, SamJakob <me@samjakob.com> 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>
> 
> Thanks for this bugfix; I've applied it to my target-arm.next
> queue and it should get upstream within a week or so.

Since I'm seeing 2 times the same patch, adding R-b again on
this one:

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

(BTW Peter the commit description is mis-aligned, if you
don't mind, correcting it while applying would be appreciated!)

Thanks,

Phil.

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

Posted by Peter Maydell 4 months, 1 week ago

On Tue, 16 Jul 2024 at 15:37, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 16/7/24 12:35, Peter Maydell wrote:
> > On Sat, 13 Jul 2024 at 17:04, SamJakob <me@samjakob.com> 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>
> >
> > Thanks for this bugfix; I've applied it to my target-arm.next
> > queue and it should get upstream within a week or so.
>
> Since I'm seeing 2 times the same patch, adding R-b again on
> this one:
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> (BTW Peter the commit description is mis-aligned, if you
> don't mind, correcting it while applying would be appreciated!)

Yep, I already picked up your r-by and rewrapped the commit
message.

-- PMM

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

Posted by Sam M. 4 months, 1 week ago

Thank you!

My apologies for the duplicates and alignment - it’s my first time using a mailing list!

> On Jul 16, 2024, at 3:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On Tue, 16 Jul 2024 at 15:37, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>> 
>>> On 16/7/24 12:35, Peter Maydell wrote:
>>> On Sat, 13 Jul 2024 at 17:04, SamJakob <me@samjakob.com> 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>
>>> 
>>> Thanks for this bugfix; I've applied it to my target-arm.next
>>> queue and it should get upstream within a week or so.
>> 
>> Since I'm seeing 2 times the same patch, adding R-b again on
>> this one:
>> 
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> 
>> (BTW Peter the commit description is mis-aligned, if you
>> don't mind, correcting it while applying would be appreciated!)
> 
> Yep, I already picked up your r-by and rewrapped the commit
> message.
> 
> -- PMM