From: Marc-André Lureau <marcandre.lureau@redhat.com>
Drop the "x-pixman" property and use fallback path in such case.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
hw/display/sm501.c | 59 +++++++++++++++++++++++++++++++++-------------
1 file changed, 42 insertions(+), 17 deletions(-)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 0eecd00701..282671d1b3 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,20 +904,23 @@ 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)) {
- /* 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;
- for (y = 0; y < height; y++) {
- i = (dst_x + (dst_y + y) * dst_pitch) * bypp;
- for (x = 0; x < width; x++, i += bypp) {
- stn_he_p(&d[i], bypp, 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;
+ for (y = 0; y < height; y++) {
+ i = (dst_x + (dst_y + y) * dst_pitch) * bypp;
+ for (x = 0; x < width; x++, i += bypp) {
+ stn_he_p(&d[i], bypp, color);
+ }
}
}
- }
break;
}
default:
@@ -1875,9 +1888,15 @@ 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(&error_warn, "x-pixman != 0, not effective without PIXMAN");
+ }
+#endif
+
s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
/* local memory */
@@ -1916,6 +1935,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 +2034,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 +2060,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),
+ /* this a debug option, prefer PROP_UINT over PROP_BIT for simplicity */
+ DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, DEFAULT_X_PIXMAN),
DEFINE_PROP_END_OF_LIST(),
};
@@ -2112,7 +2135,9 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
{
SM501PCIState *s = PCI_SM501(dev);
- sm501_init(&s->state, DEVICE(dev), s->vram_size);
+ if (!sm501_init(&s->state, DEVICE(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));
@@ -2126,7 +2151,7 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
static Property sm501_pci_properties[] = {
DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),
- DEFINE_PROP_UINT8("x-pixman", SM501PCIState, state.use_pixman, 7),
+ DEFINE_PROP_UINT8("x-pixman", SM501PCIState, state.use_pixman, DEFAULT_X_PIXMAN),
DEFINE_PROP_END_OF_LIST(),
};
--
2.41.0
On Mon, 23 Oct 2023, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Drop the "x-pixman" property and use fallback path in such case.
Patch diverged from commit message, the x-pixman property is not dropped
just the default changed. Needs updated commit message to reflect that.
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/display/sm501.c | 59 +++++++++++++++++++++++++++++++++-------------
> 1 file changed, 42 insertions(+), 17 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 0eecd00701..282671d1b3 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,20 +904,23 @@ 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)) {
> - /* 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;
> - for (y = 0; y < height; y++) {
> - i = (dst_x + (dst_y + y) * dst_pitch) * bypp;
> - for (x = 0; x < width; x++, i += bypp) {
> - stn_he_p(&d[i], bypp, 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;
> + for (y = 0; y < height; y++) {
> + i = (dst_x + (dst_y + y) * dst_pitch) * bypp;
> + for (x = 0; x < width; x++, i += bypp) {
> + stn_he_p(&d[i], bypp, color);
> + }
> }
> }
> - }
> break;
> }
> default:
> @@ -1875,9 +1888,15 @@ 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(&error_warn, "x-pixman != 0, not effective without PIXMAN");
I think this should just be warn_report not an error
> + }
> +#endif
> +
> s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
>
> /* local memory */
> @@ -1916,6 +1935,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;
and no need to add errp as we never return false so it won't be printed
anyway.
> }
>
> static const VMStateDescription vmstate_sm501_state = {
> @@ -2014,7 +2034,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;
> + }
This could be left alone too, just issuing a warning above should be
enough, no other cahnges are needed.
Regards,
BALATON Zoltan
> 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 +2060,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),
> + /* this a debug option, prefer PROP_UINT over PROP_BIT for simplicity */
> + DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, DEFAULT_X_PIXMAN),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2112,7 +2135,9 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
> {
> SM501PCIState *s = PCI_SM501(dev);
>
> - sm501_init(&s->state, DEVICE(dev), s->vram_size);
> + if (!sm501_init(&s->state, DEVICE(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));
> @@ -2126,7 +2151,7 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
>
> static Property sm501_pci_properties[] = {
> DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),
> - DEFINE_PROP_UINT8("x-pixman", SM501PCIState, state.use_pixman, 7),
> + DEFINE_PROP_UINT8("x-pixman", SM501PCIState, state.use_pixman, DEFAULT_X_PIXMAN),
> DEFINE_PROP_END_OF_LIST(),
> };
>
>
Hi
On Mon, Oct 23, 2023 at 5:15 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Mon, 23 Oct 2023, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Drop the "x-pixman" property and use fallback path in such case.
>
> Patch diverged from commit message, the x-pixman property is not dropped
> just the default changed. Needs updated commit message to reflect that.
>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > hw/display/sm501.c | 59 +++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 42 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> > index 0eecd00701..282671d1b3 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,20 +904,23 @@ 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)) {
> > - /* 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;
> > - for (y = 0; y < height; y++) {
> > - i = (dst_x + (dst_y + y) * dst_pitch) * bypp;
> > - for (x = 0; x < width; x++, i += bypp) {
> > - stn_he_p(&d[i], bypp, 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;
> > + for (y = 0; y < height; y++) {
> > + i = (dst_x + (dst_y + y) * dst_pitch) * bypp;
> > + for (x = 0; x < width; x++, i += bypp) {
> > + stn_he_p(&d[i], bypp, color);
> > + }
> > }
> > }
> > - }
> > break;
> > }
> > default:
> > @@ -1875,9 +1888,15 @@ 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(&error_warn, "x-pixman != 0, not effective without PIXMAN");
>
> I think this should just be warn_report not an error
It's a warning (error_warn), but I will change it to warn_report()
>
> > + }
> > +#endif
> > +
> > s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
> >
> > /* local memory */
> > @@ -1916,6 +1935,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;
>
> and no need to add errp as we never return false so it won't be printed
> anyway.
>
ok
> > }
> >
> > static const VMStateDescription vmstate_sm501_state = {
> > @@ -2014,7 +2034,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;
> > + }
>
> This could be left alone too, just issuing a warning above should be
> enough, no other cahnges are needed.
>
> Regards,
> BALATON Zoltan
>
> > 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 +2060,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),
> > + /* this a debug option, prefer PROP_UINT over PROP_BIT for simplicity */
> > + DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, DEFAULT_X_PIXMAN),
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
> > @@ -2112,7 +2135,9 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
> > {
> > SM501PCIState *s = PCI_SM501(dev);
> >
> > - sm501_init(&s->state, DEVICE(dev), s->vram_size);
> > + if (!sm501_init(&s->state, DEVICE(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));
> > @@ -2126,7 +2151,7 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
> >
> > static Property sm501_pci_properties[] = {
> > DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),
> > - DEFINE_PROP_UINT8("x-pixman", SM501PCIState, state.use_pixman, 7),
> > + DEFINE_PROP_UINT8("x-pixman", SM501PCIState, state.use_pixman, DEFAULT_X_PIXMAN),
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
> >
© 2016 - 2025 Red Hat, Inc.