Re: [PATCH] hw/display/sm501: Add alternatives to pixman routines

[BALATON Zoltan]

On Fri, 24 Feb 2023, Peter Maydell wrote:
> On Fri, 24 Feb 2023 at 00:18, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > Pixman can sometimes return false so add fallbacks for such cases and
> > also add a property to disable pixman and always use the fallbacks
> > which can be useful on platforms where pixman is broken or for testing
> > different drawing methods.
> >
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > ---
> > Also ping for the other sm501 patch I've sent a week ago:
> > 20230216144043.D632874634B@zero.eik.bme.hu/">https://patchew.org/QEMU/20230216144043.D632874634B@zero.eik.bme.hu/
> > These two patches are needed to fix graphics issues with AmigaOS so
> > I'd like them to be merged for 8.0
> >
> > @@ -2010,6 +2035,7 @@ 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_UINT32("base", SM501SysBusState, base, 0),
> > +    DEFINE_PROP_BOOL("x-pixman", SM501SysBusState, state.use_pixman, true),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > @@ -2093,6 +2119,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_BOOL("x-pixman", SM501PCIState, state.use_pixman, true),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
>
> I don't think this should be a user-facing property on a single
> graphics device. Either pixman works, or it doesn't (in which
> case we might need to do configure time checks and have a
> fallback), but we shouldn't make the user have to set an
> undocumented property on the device to get it to work.

This is mostly for testing now, that's why I've also called it with an x- 
prefix and not intended as a final solution or something that users would 
need to use. I'd decide later if we want to keep pixman or replace it with 
something else (as the problems we found with recently makes me think it 
might not be such a good idea after all) but for now I need a way to 
control it and do testing with or without pixman to get some data to help 
to decide what to do with it. This patch should handle tha cases where 
pixman returns false (like missing implementation on macOS aarch64) so no 
need to set this option normally. It's only when I want to easily test if 
a problem is pixman related, so I can easily set it to fallback or ask 
users who report a problem to try that. If you think this should be 
clarified in the commit message can you please suggest a consise sentence 
to describe this in a way that makes sense in English?

Regards,
BALATON Zoltan