From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35944) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZIDjr-0000Iz-Tu for qemu-devel@nongnu.org; Thu, 23 Jul 2015 06:28:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZIDjm-0008JS-Vw for qemu-devel@nongnu.org; Thu, 23 Jul 2015 06:28:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55798) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZIDjm-0008J9-RM for qemu-devel@nongnu.org; Thu, 23 Jul 2015 06:28:26 -0400 References: <1437569191-4881-1-git-send-email-pbonzini@redhat.com> <1437569191-4881-3-git-send-email-pbonzini@redhat.com> From: Paolo Bonzini Message-ID: <55B0C1C6.3090502@redhat.com> Date: Thu, 23 Jul 2015 12:28:22 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for-2.4 2/2] framebuffer: set DIRTY_MEMORY_VGA on RAM that is used for the framebuffer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers 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