1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v3 0/9] Misc sam460ex improvements
  4. View patch

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

BALATON Zoltan posted 9 patches 7 years, 8 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v3 8/9] sm501: Perform a full update after palette change
Posted by BALATON Zoltan 7 years, 8 months ago 
From: Sebastian Bauer <mail@sebastianbauer.info>

Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 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);
-- 
2.7.6
Re: [Qemu-devel] [PATCH v3 8/9] sm501: Perform a full update after palette change
Posted by David Gibson 7 years, 8 months ago 
On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> From: Sebastian Bauer <mail@sebastianbauer.info>
> 
> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Commit message.  Why is this necessary?

> ---
>  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);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [PATCH v3 8/9] sm501: Perform a full update after palette change
Posted by BALATON Zoltan 7 years, 8 months ago 
On Thu, 14 Jun 2018, David Gibson wrote:
> On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
>> From: Sebastian Bauer <mail@sebastianbauer.info>
>>
>> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> 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);
>
>
Re: [Qemu-devel] [PATCH v3 8/9] sm501: Perform a full update after palette change
Posted by David Gibson 7 years, 8 months ago 
On Thu, Jun 14, 2018 at 10:00:50AM +0200, BALATON Zoltan wrote:
> On Thu, 14 Jun 2018, David Gibson wrote:
> > On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> > > From: Sebastian Bauer <mail@sebastianbauer.info>
> > > 
> > > Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > 
> > 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.

Sure, so, put that into a commit message in the next spin.

> 
> 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);
> > 
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.