Re: [Qemu-ppc] [PATCH v3 8/9] sm501: Perform a full update after palette change

[BALATON Zoltan]

On Thu, 14 Jun 2018, David Gibson wrote:
> On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> > From: Sebastian Bauer <address@hidden>
> >
> > Signed-off-by: Sebastian Bauer <address@hidden>
> > Signed-off-by: BALATON Zoltan <address@hidden>
>
> Commit message.  Why is this necessary?

Maybe because changing palette does not change display unless an update is 
done and dirty tracking which is used in update_display does not detect 
changes in device registers where palette is stored.

I'm not sure this is needed in all modes but I guess palette is not used 
in modes that are not indexed so unecessary updates should not happen evem 
in those cases because if palette is not used anyway, guest is unlikely to 
change it unless doing something really strange but we haven't seen any 
guests yet that would do that. Therefore this simple way of handling this 
should be OK.

Regards,
BALATON Zoltan

> > ---
> >  hw/display/sm501.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> > index 0625cf5..a2ee6e3 100644
> > --- a/hw/display/sm501.c
> > +++ b/hw/display/sm501.c
> > @@ -479,6 +479,7 @@ typedef struct SM501State {
> >      MemoryRegion twoD_engine_region;
> >      uint32_t last_width;
> >      uint32_t last_height;
> > +    uint32_t do_full_update; /* perform a full update next time */
> >      I2CBus *i2c_bus;
> >
> >      /* mmio registers */
> > @@ -1032,6 +1033,7 @@ static void sm501_palette_write(void *opaque, hwaddr addr,
> >
> >      assert(range_covers_byte(0, 0x400 * 3, addr));
> >      *(uint32_t *)&s->dc_palette[addr] = value;
> > +    s->do_full_update = 1;
> >  }
> >
> >  static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
> > @@ -1620,6 +1622,12 @@ static void sm501_update_display(void *opaque)
> >          full_update = 1;
> >      }
> >
> > +    /* someone else requested a full update */
> > +    if (s->do_full_update) {
> > +        s->do_full_update = 0;
> > +        full_update = 1;
> > +    }
> > +
> >      /* draw each line according to conditions */
> >      snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
> >                offset, width * height * src_bpp, DIRTY_MEMORY_VGA);