Re: [PATCH 07/12] macfb: add qdev property to specify display type

[Laurent Vivier]

On 04/10/2021 11:24, Laurent Vivier wrote:
> Le 02/10/2021 à 13:00, Mark Cave-Ayland a écrit :
> > Since the available resolutions and colour depths are determined by the attached
> > display type, add a qdev property to allow the display type to be specified.
> >
> > The main resolutions of interest are high resolution 1152x870 with 8-bit colour
> > and SVGA resolution up to 800x600 with 32-bit colour so update the q800 machine
> > to allow high resolution mode if specified and otherwise fall back to SVGA.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >   hw/display/macfb.c         | 6 +++++-
> >   hw/m68k/q800.c             | 5 +++++
> >   include/hw/display/macfb.h | 1 +
> >   3 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> > index 5c95aa4a11..023d1f0cd1 100644
> > --- a/hw/display/macfb.c
> > +++ b/hw/display/macfb.c
> > @@ -316,7 +316,7 @@ static uint32_t macfb_sense_read(MacfbState *s)
> >       MacFbSense *macfb_sense;
> >       uint8_t sense;
> >   
> > -    macfb_sense = &macfb_sense_table[MACFB_DISPLAY_VGA];
> > +    macfb_sense = &macfb_sense_table[s->type];
> >       if (macfb_sense->sense == 0x7) {
> >           /* Extended sense */
> >           sense = 0;
> > @@ -545,6 +545,8 @@ static Property macfb_sysbus_properties[] = {
> >       DEFINE_PROP_UINT32("width", MacfbSysBusState, macfb.width, 640),
> >       DEFINE_PROP_UINT32("height", MacfbSysBusState, macfb.height, 480),
> >       DEFINE_PROP_UINT8("depth", MacfbSysBusState, macfb.depth, 8),
> > +    DEFINE_PROP_UINT8("display", MacfbSysBusState, macfb.type,
> > +                      MACFB_DISPLAY_VGA),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >   
> > @@ -552,6 +554,8 @@ static Property macfb_nubus_properties[] = {
> >       DEFINE_PROP_UINT32("width", MacfbNubusState, macfb.width, 640),
> >       DEFINE_PROP_UINT32("height", MacfbNubusState, macfb.height, 480),
> >       DEFINE_PROP_UINT8("depth", MacfbNubusState, macfb.depth, 8),
> > +    DEFINE_PROP_UINT8("display", MacfbNubusState, macfb.type,
> > +                      MACFB_DISPLAY_VGA),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >   
> > diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> > index 09b3366024..5223b880bc 100644
> > --- a/hw/m68k/q800.c
> > +++ b/hw/m68k/q800.c
> > @@ -421,6 +421,11 @@ static void q800_init(MachineState *machine)
> >       qdev_prop_set_uint32(dev, "width", graphic_width);
> >       qdev_prop_set_uint32(dev, "height", graphic_height);
> >       qdev_prop_set_uint8(dev, "depth", graphic_depth);
> > +    if (graphic_width == 1152 && graphic_height == 870 && graphic_depth == 8) {
> > +        qdev_prop_set_uint8(dev, "display", MACFB_DISPLAY_APPLE_21_COLOR);
> > +    } else {
> > +        qdev_prop_set_uint8(dev, "display", MACFB_DISPLAY_VGA);
> > +    }
> >       qdev_realize_and_unref(dev, BUS(nubus), &error_fatal);
> >   
> >       cs = CPU(cpu);
> > diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
> > index febf4ce0e8..e95a97ebdc 100644
> > --- a/include/hw/display/macfb.h
> > +++ b/include/hw/display/macfb.h
> > @@ -46,6 +46,7 @@ typedef struct MacfbState {
> >       uint8_t color_palette[256 * 3];
> >       uint32_t width, height; /* in pixels */
> >       uint8_t depth;
> > +    uint8_t type;
> >   
> >       uint32_t sense;
> >   } MacfbState;
>
> I think the display modes should be documentend somewhere to be directly usable 
> by the user and get
> ride of the graphic_XXX variables (and -g).

By reading following patch I can see it's not really needed anymore, so:

Reviewed-by: Laurent Vivier <laurent@vivier.eu>