Re: [PATCH] drm/i915: Do not access stolen memory directly by the CPU, even for error capture

Chris Wilson
2014-03-05T09:18:40Z

On Tue, Mar 04, 2014 at 10:01:56PM +0000, Chris Wilson wrote:
> On Tue, Mar 04, 2014 at 01:27:05PM -0800, Ben Widawsky wrote:
> > On Tue, Mar 04, 2014 at 03:45:56PM +0100, Daniel Vetter wrote:
> > > On Tue, Feb 18, 2014 at 11:18:04AM -0800, Ben Widawsky wrote:
> > > > On Wed, Feb 12, 2014 at 07:18:40PM +0000, Chris Wilson wrote:
> > > > > For stolen pages, since it is verboten to access them directly on many
> > > > > architectures, we have to read them through the GTT aperture. If they
> > > > > are not accessible through the aperture, then we have to abort.
> > > > >
> > > > > This was complicated by
> > > > >
> > > > > commit 8b6124a633d8095b0c8364f585edff9c59568a96
> > > > > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Date: Thu Jan 30 14:38:16 2014 +0000
> > > > >
> > > > > drm/i915: Don't access snooped pages through the GTT (even for error capture)
> > > > >
> > > > > and the desire to use stolen memory for ringbuffers, contexts and
> > > > > batches in the future.
> > > > >
> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > >
> > > > I'd prefer separate functions for the different types of error objects
> > > > (gtt vs CPU mapped). Or maybe just pass in the capture type as an
> > > > argument and then create a helper to determine the right thing. It'd at
> > > > least be a bit easier for review.
> > > >
> > > > Anyway, having not actually looked at the code, the idea is solid:
> > > > Acked-by: Ben Widawsky <ben@bwidawsk.net>
> > >
> > > Can you please actually look at the code and upgrade this to a full
> > > review?
> > >
> > > Thanks, Daniel
> >
> > I had been hoping for a bit of a rewrite to do the review. Chris are you
> > opposed to my suggestions?

Actually. Look at it, replacing the single if() with a
copy_page_from_bo(dev_priv, src, i) is not the beautification you seek.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre