Re: [PATCH 1/8] drm/i915: Synchronize pread/pwrite with wait_rendering

[Ben Widawsky <ben@bwidawsk.net>]

On Tue, Sep 03, 2013 at 06:08:19PM +0200, Daniel Vetter wrote:
> On Mon, Sep 02, 2013 at 04:12:36PM +0200, Daniel Vetter wrote:
> > 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 <daniel.vetter@ffwll.ch>
> > > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > > > > 
> > > > > > 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.
> 
> Actually it won't blow up since you always set readonly = false. But it'll
> kill the neat read-read optimization ...
> -Daniel

Doh! Sorry about this. Fixed locally.

-- 
Ben Widawsky, Intel Open Source Technology Center