[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.4 2/2] framebuffer: set DIRTY_MEMORY_VGA o
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH for-2.4 2/2] framebuffer: set DIRTY_MEMORY_VGA on RAM that is used for the framebuffer |
Date: |
Thu, 23 Jul 2015 12:28:22 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.0.1 |
On 23/07/2015 12:22, Peter Maydell wrote:
> I have a few minor nits below, but the patch works and looks
> good overall.
>
> At the moment the pattern in all the callsites is
> if (s->invalidate) {
> framebuffer_update_memory_section(...);
> }
> framebuffer_update_display(...);
>
> What's the rationale for not just having framebuffer_update_display()
> do this -- that we might in future want to be cleverer about how
> often we call framebuffer_update_memory_section() ?
Yes. I initially set out adding framebuffer_update_memory_section calls
after the register writes, but using s->invalidate is simpler albeit
marginally less efficient.
>> + memory_region_reset_dirty(mem, mem_section->offset_within_region,
>> src_len,
>> DIRTY_MEMORY_VGA);
>
> Not new in this patch, but isn't there technically a race
> condition if the guest writes to the framebuffer memory
> after we've done the memory_region_get_dirty() for that
> row but before we clear all the dirty bits again here?
Yes, I think you're right.
>>
>> +void framebuffer_update_memory_section(
>> + MemoryRegionSection *mem_section,
>> + MemoryRegion *root,
>> + hwaddr base,
>> + unsigned rows,
>> + unsigned src_width);
>
> A doc-comment header would be nice.
Ok.
>>
>> + if (s->invalidated) {
>> + framebuffer_update_memory_section(&s->fbsection, s->sysmem,
>> + addr, src_width, s->yres);
>
> Aren't src_width and s->yres in the wrong order here?
> They should be in the same order as they are in the
> (ditto in the other hunks in the patch for this file).
Ouch, indeed.
Paolo