[1/2] hw/misc/bcm2835_property: Fix framebuffer with recent RPi kernels

The framebuffer driver fails to initialize with recent Raspberry Pi
kernels, such as the ones shipped in the current RaspiOS images
(with the out of tree bcm2708_fb.c driver)

The reason is that this driver uses a new firmware call to query the
number of displays, and the fallback when this call fails is broken.

So implement the call and claim we have exactly one display

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/misc/bcm2835_property.c | 4 ++++
 1 file changed, 4 insertions(+)

Hi Benjamin,

On 10/17/21 09:48, Benjamin Herrenschmidt wrote:
> The framebuffer driver fails to initialize with recent Raspberry Pi
> kernels, such as the ones shipped in the current RaspiOS images
> (with the out of tree bcm2708_fb.c driver)

Which particular version?

> 
> The reason is that this driver uses a new firmware call to query the
> number of displays, and the fallback when this call fails is broken.
> 
> So implement the call and claim we have exactly one display
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  hw/misc/bcm2835_property.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
> index 73941bdae9..b958fa6a5c 100644
> --- a/hw/misc/bcm2835_property.c
> +++ b/hw/misc/bcm2835_property.c
> @@ -269,6 +269,10 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>              stl_le_phys(&s->dma_as, value + 12, 0);
>              resplen = 4;
>              break;
> +        case 0x00040013: /* Get num displays */
> +            stl_le_phys(&s->dma_as, value + 12, 1);
> +            resplen = 4;
> +            break;
>  
>          case 0x00060001: /* Get DMA channels */
>              /* channels 2-5 */
> 

I carry (among others) this patch for the raspi4:

-- >8 --
diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index 3b3f5a804d9..8bd149211af 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -36,6 +36,7 @@ static void
bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
      */
     BCM2835FBConfig fbconfig = s->fbdev->config;
     bool fbconfig_updated = false;
+    int fbconfig_idx = 0;

     value &= ~0xf;

@@ -290,6 +291,25 @@ static void
bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 4;
             break;

+        case 0x00040013: /* Get number of displays */
+            stl_le_phys(&s->dma_as, value + 12, 0 /* old fw: unique
display */);
+            resplen = 4;
+            break;
+
+        case 0x00048013: /* Select display  */
+            fbconfig_idx = ldl_le_phys(&s->dma_as, value + 12);
+            if (fbconfig_idx) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "bcm2835_property: Only one display
implemented,"
+                              " requested display #%d\n", fbconfig_idx);
+            }
+            resplen = 4;
+            break;
+
+        case 0x00040014: /* Get display settings */
+            resplen = 0; /* as old fw we don't support that */
+            break;
+
         case 0x00060001: /* Get DMA channels */
             /* channels 2-5 */
             stl_le_phys(&s->dma_as, value + 12, 0x003C);
---

FYI I plan to respin Alex's recent series with these patches:
https://lore.kernel.org/qemu-devel/20211004134742.2044280-1-alex.bennee@linaro.org/
as soon as I get some time...

On Sun, 2021-10-17 at 17:08 +0200, Philippe Mathieu-Daudé wrote:
> Hi Benjamin,
> 
> On 10/17/21 09:48, Benjamin Herrenschmidt wrote:
> > The framebuffer driver fails to initialize with recent Raspberry Pi
> > kernels, such as the ones shipped in the current RaspiOS images
> > (with the out of tree bcm2708_fb.c driver)
> 
> Which particular version?

The one I dug out of 2021-05-07-raspios-buster-arm64-lite.img (note
that this then fails to boot some time after the fb is setup, even
after the fix, in the vchip driver init (before serial is up even).

That said, the same fb problem happens with 5.10.60-v8+ from raspbian.

I'm not sure your fix will work on these, see below:

> +        case 0x00040013: /* Get number of displays */
> +            stl_le_phys(&s->dma_as, value + 12, 0 /* old fw: unique display */);
> +            resplen = 4;
> +            break;
> 
This should have been equivalen to not having the property. However,
the failure path in the driver looks like this (note the mismatch
between the comment and the code.. this is rpi 5.10.73 from the rpi
repo :

	ret = rpi_firmware_property(fw,
				    RPI_FIRMWARE_FRAMEBUFFER_GET_NUM_DISPLAYS,
				    &num_displays, sizeof(u32));

	/* If we fail to get the number of displays, or it returns 0, then
	 * assume old firmware that doesn't have the mailbox call, so just
	 * set one display
	 */
	if (ret || num_displays == 0) {
		dev_err(&dev->dev,
			"Unable to determine number of FBs. Disabling driver.\n");
		return -ENOENT;
	} else {
		fbdev->firmware_supports_multifb = 1;
	}

So it appears that the intention at some stage was to set only one display but
the code as written will fail to initialize the drive if the properly is absent
*or* if it returns 0.

I've just checked the rpi-5.15.y branch and it's the same.

Cheers,
Ben.

On 10/18/21 02:41, Benjamin Herrenschmidt wrote:
> On Sun, 2021-10-17 at 17:08 +0200, Philippe Mathieu-Daudé wrote:
>> Hi Benjamin,
>>
>> On 10/17/21 09:48, Benjamin Herrenschmidt wrote:
>>> The framebuffer driver fails to initialize with recent Raspberry Pi
>>> kernels, such as the ones shipped in the current RaspiOS images
>>> (with the out of tree bcm2708_fb.c driver)
>>
>> Which particular version?
> 
> The one I dug out of 2021-05-07-raspios-buster-arm64-lite.img (note
> that this then fails to boot some time after the fb is setup, even
> after the fix, in the vchip driver init (before serial is up even).
> 
> That said, the same fb problem happens with 5.10.60-v8+ from raspbian.
> 
> I'm not sure your fix will work on these, see below:
> 
>> +        case 0x00040013: /* Get number of displays */
>> +            stl_le_phys(&s->dma_as, value + 12, 0 /* old fw: unique display */);
>> +            resplen = 4;
>> +            break;
>>
> This should have been equivalen to not having the property. However,
> the failure path in the driver looks like this (note the mismatch
> between the comment and the code.. this is rpi 5.10.73 from the rpi
> repo :
> 
> 	ret = rpi_firmware_property(fw,
> 				    RPI_FIRMWARE_FRAMEBUFFER_GET_NUM_DISPLAYS,
> 				    &num_displays, sizeof(u32));
> 
> 	/* If we fail to get the number of displays, or it returns 0, then
> 	 * assume old firmware that doesn't have the mailbox call, so just
> 	 * set one display
> 	 */
> 	if (ret || num_displays == 0) {
> 		dev_err(&dev->dev,
> 			"Unable to determine number of FBs. Disabling driver.\n");
> 		return -ENOENT;
> 	} else {
> 		fbdev->firmware_supports_multifb = 1;
> 	}
> 
> So it appears that the intention at some stage was to set only one display but
> the code as written will fail to initialize the drive if the properly is absent
> *or* if it returns 0.
> 
> I've just checked the rpi-5.15.y branch and it's the same.

Indeed. I stopped testing recent kernels because they use too many
features QEMU don't implement.

Our model should generate the DTB blob of devices implemented, instead
of giving false expectations to the guest by passing an unmodified dtb.

This is on my TODO, I might give it a try next WE.

On Mon, 2021-10-18 at 11:47 +0200, Philippe Mathieu-Daudé wrote:
> 
> > I've just checked the rpi-5.15.y branch and it's the same.
> 
> Indeed. I stopped testing recent kernels because they use too many
> features QEMU don't implement.
> 
> Our model should generate the DTB blob of devices implemented, instead
> of giving false expectations to the guest by passing an unmodified dtb.
> 
> This is on my TODO, I might give it a try next WE.

Indeed. That said, we do implement the fb, so we probably want that
fix. The fix for the virtual gpios is probably unnecessary however if
we do what you want.

That being said, with those two fixes, I can boot the latest 5.10 I get
from raspbian.

Cheers,
Ben.

On 10/18/21 12:27, Benjamin Herrenschmidt wrote:
> On Mon, 2021-10-18 at 11:47 +0200, Philippe Mathieu-Daudé wrote:
>>
>>> I've just checked the rpi-5.15.y branch and it's the same.
>>
>> Indeed. I stopped testing recent kernels because they use too many
>> features QEMU don't implement.
>>
>> Our model should generate the DTB blob of devices implemented, instead
>> of giving false expectations to the guest by passing an unmodified dtb.
>>
>> This is on my TODO, I might give it a try next WE.
> 
> Indeed. That said, we do implement the fb, so we probably want that
> fix. The fix for the virtual gpios is probably unnecessary however if
> we do what you want.
> 
> That being said, with those two fixes, I can boot the latest 5.10 I get
> from raspbian.

Great. This test should pass with your 5.10 kernel then:

https://lore.kernel.org/qemu-devel/20200215192216.4899-9-f4bug@amsat.org/

Do you mind providing the equivalent 'deb_url' / 'deb_hash' you
used, so I can adapt this test to cover a Raspbian 5.10 kernel
in our CI?

On 10/17/21 09:48, Benjamin Herrenschmidt wrote:
> The framebuffer driver fails to initialize with recent Raspberry Pi
> kernels, such as the ones shipped in the current RaspiOS images
> (with the out of tree bcm2708_fb.c driver)
> 
> The reason is that this driver uses a new firmware call to query the
> number of displays, and the fallback when this call fails is broken.
> 
> So implement the call and claim we have exactly one display
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  hw/misc/bcm2835_property.c | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>