Hi
On Tue, Oct 10, 2023 at 3:59 PM BALATON Zoltan <bala...@eik.bme.hu> wrote:
>
> On Tue, 10 Oct 2023, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > Drop the "x-pixman" property and use fallback path in such case.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > ---
> > hw/display/sm501.c | 46 +++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 37 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> > index 0eecd00701..bc5faa19f0 100644
> > --- a/hw/display/sm501.c
> > +++ b/hw/display/sm501.c
> > @@ -438,6 +438,12 @@
> > #define SM501_HWC_WIDTH 64
> > #define SM501_HWC_HEIGHT 64
> >
> > +#ifdef CONFIG_PIXMAN
> > +#define DEFAULT_X_PIXMAN 7
> > +#else
> > +#define DEFAULT_X_PIXMAN 0
> > +#endif
> > +
> > /* SM501 local memory size taken from "linux/drivers/mfd/sm501.c" */
> > static const uint32_t sm501_mem_local_size[] = {
> > [0] = 4 * MiB,
> > @@ -730,7 +736,6 @@ static void sm501_2d_operation(SM501State *s)
> > switch (cmd) {
> > case 0: /* BitBlt */
> > {
> > - static uint32_t tmp_buf[16384];
> > unsigned int src_x = (s->twoD_source >> 16) & 0x01FFF;
> > unsigned int src_y = s->twoD_source & 0xFFFF;
> > uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
> > @@ -828,9 +833,11 @@ static void sm501_2d_operation(SM501State *s)
> > de = db + (width + (height - 1) * dst_pitch) * bypp;
> > overlap = (db < se && sb < de);
> > }
> > +#ifdef CONFIG_PIXMAN
> > if (overlap && (s->use_pixman & BIT(2))) {
> > /* pixman can't do reverse blit: copy via temporary */
> > int tmp_stride = DIV_ROUND_UP(width * bypp,
> > sizeof(uint32_t));
> > + static uint32_t tmp_buf[16384];
> > uint32_t *tmp = tmp_buf;
> >
> > if (tmp_stride * sizeof(uint32_t) * height >
> > sizeof(tmp_buf)) {
> > @@ -860,9 +867,12 @@ static void sm501_2d_operation(SM501State *s)
> > dst_pitch * bypp / sizeof(uint32_t),
> > 8 * bypp, 8 * bypp, src_x, src_y,
> > dst_x, dst_y, width, height);
> > - } else {
> > + } else
> > +#else
> > + {
> > fallback = true;
> > }
> > +#endif
> > if (fallback) {
> > uint8_t *sp = s->local_mem + src_base;
> > uint8_t *d = s->local_mem + dst_base;
> > @@ -894,10 +904,13 @@ static void sm501_2d_operation(SM501State *s)
> > color = cpu_to_le16(color);
> > }
> >
> > +#ifdef CONFIG_PIXMAN
> > if (!(s->use_pixman & BIT(0)) || (width == 1 && height == 1) ||
> > !pixman_fill((uint32_t *)&s->local_mem[dst_base],
> > dst_pitch * bypp / sizeof(uint32_t), 8 * bypp,
> > - dst_x, dst_y, width, height, color)) {
> > + dst_x, dst_y, width, height, color))
> > +#endif
> > + {
> > /* fallback when pixman failed or we don't want to call it */
> > uint8_t *d = s->local_mem + dst_base;
> > unsigned int x, y, i;
> > @@ -1875,9 +1888,16 @@ static void sm501_reset(SM501State *s)
> > s->twoD_wrap = 0;
> > }
> >
> > -static void sm501_init(SM501State *s, DeviceState *dev,
> > - uint32_t local_mem_bytes)
> > +static bool sm501_init(SM501State *s, DeviceState *dev,
> > + uint32_t local_mem_bytes, Error **errp)
> > {
> > +#ifndef CONFIG_PIXMAN
> > + if (s->use_pixman != 0) {
> > + error_setg(errp, "x-pixman!=0, however PIXMAN is not available");
> > + return false;
> > + }
> > +#endif
>
> I think this should be just a warning saying that x-pixman != 0 is not
> effective without pixman and not a hard error. The goal is to keep command
> lines working the same not to annoy users unnecesarily.
ok
>
> > +
> > s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
> >
> > /* local memory */
> > @@ -1916,6 +1936,7 @@ static void sm501_init(SM501State *s, DeviceState
> > *dev,
> >
> > /* create qemu graphic console */
> > s->con = graphic_console_init(dev, 0, &sm501_ops, s);
> > + return true;
> > }
> >
> > static const VMStateDescription vmstate_sm501_state = {
> > @@ -2014,7 +2035,9 @@ static void sm501_realize_sysbus(DeviceState *dev,
> > Error **errp)
> > SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > MemoryRegion *mr;
> >
> > - sm501_init(&s->state, dev, s->vram_size);
> > + if (!sm501_init(&s->state, dev, s->vram_size, errp)) {
> > + return;
> > + }
> > if (get_local_mem_size(&s->state) != s->vram_size) {
> > error_setg(errp, "Invalid VRAM size, nearest valid size is %"
> > PRIu32,
> > get_local_mem_size(&s->state));
> > @@ -2038,7 +2061,8 @@ static void sm501_realize_sysbus(DeviceState *dev,
> > Error **errp)
> >
> > static Property sm501_sysbus_properties[] = {
> > DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
> > - DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 7),
> > + /* TODO: consider PROP_BIT instead */
>
> I don't think a comment is needed here but don't mind it you think it
> helps to note this here too but I'd keep the single value instead of
> separate BIT properties.
yes, that will avoid someone else proposing the same change again.
thanks
