Re: [PATCH 43/44] Add missed BCM2835 properties

[Peter Maydell]

On Wed, 26 Jul 2023 at 15:24, Sergey Kambalin <serg.oker@gmail.com> wrote:
>
> Signed-off-by: Sergey Kambalin <sergey.kambalin@auriga.com>
> ---
>  hw/misc/bcm2835_property.c            | 170 ++++++++++++++++++++++++++
>  include/hw/misc/raspberrypi-fw-defs.h |  11 ++
>  2 files changed, 181 insertions(+)
>
> diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
> index 4ed9faa54a..7d2d6e518d 100644
> --- a/hw/misc/bcm2835_property.c
> +++ b/hw/misc/bcm2835_property.c
> @@ -19,6 +19,31 @@
>  #include "trace.h"
>  #include "hw/arm/raspi_platform.h"
>
> +#define RPI_EXP_GPIO_BASE       128
> +#define VC4_GPIO_EXPANDER_COUNT 8
> +#define VCHI_BUSADDR_SIZE       sizeof(uint32_t)
> +
> +struct vc4_display_settings_t {
> +    uint32_t display_num;
> +    uint32_t width;
> +    uint32_t height;
> +    uint32_t depth;
> +    uint16_t pitch;
> +    uint32_t virtual_width;
> +    uint32_t virtual_height;
> +    uint16_t virtual_width_offset;
> +    uint32_t virtual_height_offset;
> +    unsigned long fb_bus_address;

'long' type in a packed struct ? Sounds fishy...
'long' type for any kind of address is also generally
not the correct type.

> +} QEMU_PACKED;
> +
> +struct vc4_gpio_expander_t {
> +    uint32_t direction;
> +    uint32_t polarity;
> +    uint32_t term_en;
> +    uint32_t term_pull_up;
> +    uint32_t state;
> +} vc4_gpio_expander[VC4_GPIO_EXPANDER_COUNT];

What is this state doing here ? It looks like the property is
supposed to be changing the handling of GPIOs, but in that case
we should wire the properties through to a gpio device, not
just keep hold of the values written here. Simpler would be
to ignore writes if we don't care to implement it properly.
Otherwise the state needs to live in some device and be handled
on vmsave, reset, etc.

> +
>  /* https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface */

> +        case RPI_FWREQ_FRAMEBUFFER_GET_DISPLAY_SETTINGS:

What's this one doing? 0x00040014 isn't documented in the URL above.

> +            stl_le_phys(&s->dma_as, value + 12, 0); /* display_num */
> +            stl_le_phys(&s->dma_as, value + 16, 800); /* width */
> +            stl_le_phys(&s->dma_as, value + 20, 600); /* height */
> +            stl_le_phys(&s->dma_as, value + 24, 32); /* depth */
> +            stl_le_phys(&s->dma_as, value + 28, 32); /* pitch */
> +            stl_le_phys(&s->dma_as, value + 30, 0); /* virtual_width */
> +            stl_le_phys(&s->dma_as, value + 34, 0); /* virtual_height */
> +            stl_le_phys(&s->dma_as, value + 38, 0); /* virtual_width_offset 
> */
> +            stl_le_phys(&s->dma_as, value + 40, 0); /* virtual_height_offset 
> */
> +            stl_le_phys(&s->dma_as, value + 44, 0); /* fb_bus_address low */
> +            stl_le_phys(&s->dma_as, value + 48, 0); /* fb_bus_address hi */
> +            resplen = sizeof(struct vc4_display_settings_t);
> +            break;

Shouldn't this return the same values as the existing
RPI_FWREQ_FRAMEBUFFER_GET_PHYSICAL_WIDTH_HEIGHT etc etc properties,
rather than hardcoded constant values ?
> +
> +        case RPI_FWREQ_FRAMEBUFFER_SET_PITCH:
> +            resplen = 0;
> +            break;

What is setting the pitch supposed to do? At the moment we
assume it's a fixed value depending on the xres, xres_virtual
and bpp (see bcm2835_fb_get_pitch()), so if the guest can
arbitrarily set it then we need to change something in our
display device model. Failing that we should maybe LOG_UNIMP
any attempt by the guest to set it.

thanks
-- PMM