From: Daniel Vetter Subject: Re: [PATCH 1/8] drm/i915: Synchronize pread/pwrite with wait_rendering Date: Mon, 2 Sep 2013 16:12:36 +0200 Message-ID: <20130902141236.GV9374@phenom.ffwll.local> References: <1377906241-8463-1-git-send-email-benjamin.widawsky@intel.com> <1377906241-8463-2-git-send-email-benjamin.widawsky@intel.com> <20130830235030.GP4726@nuc-i3427.alporthouse.com> <20130831033946.GA1088@bwidawsk.net> <20130902063224.GL9374@phenom.ffwll.local> <20130902131412.GC6439@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f44.google.com (mail-ee0-f44.google.com [74.125.83.44]) by gabe.freedesktop.org (Postfix) with ESMTP id 49867E695B for ; Mon, 2 Sep 2013 07:12:23 -0700 (PDT) Received: by mail-ee0-f44.google.com with SMTP id b47so2387637eek.17 for ; Mon, 02 Sep 2013 07:12:22 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130902131412.GC6439@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson , Daniel Vetter , Ben Widawsky , Ben Widawsky , Intel GFX List-Id: intel-gfx@lists.freedesktop.org On Mon, Sep 02, 2013 at 02:14:12PM +0100, Chris Wilson wrote: > On Mon, Sep 02, 2013 at 08:32:24AM +0200, Daniel Vetter wrote: > > On Fri, Aug 30, 2013 at 08:39:46PM -0700, Ben Widawsky wrote: > > > On Sat, Aug 31, 2013 at 12:50:30AM +0100, Chris Wilson wrote: > > > > On Fri, Aug 30, 2013 at 04:43:54PM -0700, Ben Widawsky wrote: > > > > > lifted from Daniel: > > > > > pread/pwrite isn't about the object's domain at all, but purely about > > > > > synchronizing for outstanding rendering. Replacing the call to > > > > > set_to_gtt_domain with a wait_rendering would imo improve code > > > > > readability. Furthermore we could pimp pread to only block for > > > > > outstanding writes and not for reads. > > > > > > > > > > Since you're not the first one to trip over this: Can I volunteer you > > > > > for a follow-up patch to fix this? > > > > > > > > > > Recommended-by: Daniel Vetter > > > > > Signed-off-by: Ben Widawsky > > > > > > > > This should fail i-g-t... > > > > -Chris > > > > > > > > > > Daniel, how have I failed your plan? > > > > It should work ... Since the enclosing if-block checks for !cpu domain > > (for either reads or writes) that implies that going into the gtt domain > > is a noop (or better should be) wrt clflushing and we only wait for > > outstanding gpu rendering. wait_rendering is an interface that's been > > added afterwards. Unfortunately I've failed to explain this trickery in > > either a comment or the commit message. Bad me ;-) > > The issue is that in the patch pwrite is not waiting for any outstanding > GPU reads. Oh right, silly me didn't spot the s/true/false/ switch Ben sneaked in. This /should/ have been caught by the gem_concurrent_blt subtests that exercise pwrites ... Ben can you please check that this indeed blew up on igt? Should fail on any platform, no special caching mode required. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch