From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f51.google.com ([74.125.82.51]:34524 "EHLO mail-wg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751481AbbEZNfy (ORCPT ); Tue, 26 May 2015 09:35:54 -0400 Received: by wghq2 with SMTP id q2so97440260wgh.1 for ; Tue, 26 May 2015 06:35:52 -0700 (PDT) Date: Tue, 26 May 2015 11:56:18 +0200 From: Daniel Vetter To: Chris Wilson , Daniel Vetter , intel-gfx@lists.freedesktop.org, Akash Goel , stable@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Force wmb() on using GTT io_mapping_map_wc Message-ID: <20150526095618.GJ12971@phenom.ffwll.local> References: <1432568322-20651-1-git-send-email-chris@chris-wilson.co.uk> <1432576124-20523-1-git-send-email-chris@chris-wilson.co.uk> <20150526080124.GF12971@phenom.ffwll.local> <20150526084915.GD20703@nuc-i3427.alporthouse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150526084915.GD20703@nuc-i3427.alporthouse.com> Sender: stable-owner@vger.kernel.org List-ID: On Tue, May 26, 2015 at 09:49:15AM +0100, Chris Wilson wrote: > On Tue, May 26, 2015 at 10:01:24AM +0200, Daniel Vetter wrote: > > On Mon, May 25, 2015 at 06:48:44PM +0100, Chris Wilson wrote: > > > Since the advent of mmap(wc), where we reused the same cache domain for > > > WC and GTT paths (oh, how I regret that double-edged advice), we need to > > > be extra cautious when using GTT iomap_wc internally. Since userspace maybe > > > modifying through the mmap(wc) and we then modify modifying through an > > > aliased WC path through the GTT, those writes may overlap and not be > > > visible to the other path. Easiest to trigger appears to be write the > > > batch through mmap(wc) and then attempt to perform reloc the GTT, > > > corruption quickly ensues. > > > > > > v2: Be stricter and do a full mb() in case we are reading through one > > > path and about to write through the second path. Also, apply the > > > barriers on transitioning into the GTT domain as well to cover the white > > > lie asychronous cases. > > > > > > Signed-off-by: Chris Wilson > > > Cc: Akash Goel > > > Cc: stable@vger.kernel.org > > > > There are a lot more mb() and wmb() in the gem code. I count execlist, > > legacy rings, pwrite, set_domain and finish_gtt. > > You have more than I have... Well yeah, but the patch is also cc: stable. Which means we'd need to frob the barriers first, then refactor. And I'm still not sure whether this is road we want to go down, or whether adding explicit begin/end coherency ioctls wouldn't be the better long-term option. It always irked me a bit that the kernel needs to infer the other edge of a cpu access, explicit begin/end seemes better (and more in line with what all the gpu apis expose). Something like this struct i915_mmap_control { #define BEGIN 0 #define END 1 #define READ 0 #define WRITE 2 #define UNSYNCHRONIZED 4 /* or whetever the bikeshed will be */ #define MAPPING_MASK 0xf0 #define GTT_WC (1 << 4) #define CPU_WC (2 << 4) #define CPU_CACHED (3 << 4) u32 flags; u32 bo; u64 start; u64 length; }; Pretty much modelled after the begin/end_cpu_access hooks from dma-buf. Which seems to gain some traction for a generic up/download interface ... It just feels like incrementally fixing things might lead us to a very bad place. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch