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 <ch...@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 <ch...@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 <b...@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?
> >
> > > ---
> > > drivers/gpu/drm/i915/i915_gpu_error.c | 50
> > > ++++++++++++++++++++++-------------
> > > 1 file changed, 31 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > index 0e1f7b691082..a2c3a639c3cc 100644
> > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > @@ -542,10 +542,11 @@ static struct drm_i915_error_object *
> > > i915_error_object_create_sized(struct drm_i915_private *dev_priv,
> > > struct drm_i915_gem_object *src,
> > > struct i915_address_space *vm,
> > > - const int num_pages)
> > > + int num_pages)
> > > {
> > > struct drm_i915_error_object *dst;
> > > - int i;
> > > + bool use_ggtt;
> > > + int i = 0;
> > > u32 reloc_offset;
> > >
> > > if (src == NULL || src->pages == NULL)
> > > @@ -555,8 +556,32 @@ i915_error_object_create_sized(struct
> > > drm_i915_private *dev_priv,
> > > if (dst == NULL)
> > > return NULL;
> > >
> > > - reloc_offset = dst->gtt_offset = i915_gem_obj_offset(src, vm);
> > > - for (i = 0; i < num_pages; i++) {
> > > + dst->gtt_offset = i915_gem_obj_offset(src, vm);
> > > +
> > > + reloc_offset = dst->gtt_offset;
> > > + use_ggtt = (src->cache_level == I915_CACHE_NONE &&
> > > + i915_is_ggtt(vm) &&
> > > + src->has_global_gtt_mapping &&
> > > + reloc_offset + num_pages * PAGE_SIZE <=
> > > dev_priv->gtt.mappable_end);
> > > +
> > > + /* Cannot access stolen address directly, try to use the aperture */
> > > + if (src->stolen) {
> > > + use_ggtt = true;
> > > +
> > > + if (!src->has_global_gtt_mapping)
> > > + goto unwind;
> > > +
> > > + reloc_offset = i915_gem_obj_ggtt_offset(src);
> > > + if (reloc_offset + num_pages * PAGE_SIZE >
> > > dev_priv->gtt.mappable_end)
> > > + goto unwind;
> > > + }
> > > +
> > > + /* Cannot access snooped pages through the aperture */
> > > + if (use_ggtt && src->cache_level != I915_CACHE_NONE &&
> > > !HAS_LLC(dev_priv->dev))
> > > + goto unwind;
> > > +
> > > + dst->page_count = num_pages;
> > > + while (num_pages--) {
> > > unsigned long flags;
> > > void *d;
> > >
> > > @@ -565,10 +590,7 @@ i915_error_object_create_sized(struct
> > > drm_i915_private *dev_priv,
> > > goto unwind;
> > >
> > > local_irq_save(flags);
> > > - if (src->cache_level == I915_CACHE_NONE &&
> > > - reloc_offset < dev_priv->gtt.mappable_end &&
> > > - src->has_global_gtt_mapping &&
> > > - i915_is_ggtt(vm)) {
> > > + if (use_ggtt) {
> > > void __iomem *s;
> > >
> > > /* Simply ignore tiling or any overlapping fence.
> > > @@ -580,14 +602,6 @@ i915_error_object_create_sized(struct
> > > drm_i915_private *dev_priv,
> > > reloc_offset);
> > > memcpy_fromio(d, s, PAGE_SIZE);
> > > io_mapping_unmap_atomic(s);
> > > - } else if (src->stolen) {
> > > - unsigned long offset;
> > > -
> > > - offset = dev_priv->mm.stolen_base;
> > > - offset += src->stolen->start;
> > > - offset += i << PAGE_SHIFT;
> > > -
> > > - memcpy_fromio(d, (void __iomem *) offset, PAGE_SIZE);
> > > } else {
> > > struct page *page;
> > > void *s;
> > > @@ -604,11 +618,9 @@ i915_error_object_create_sized(struct
> > > drm_i915_private *dev_priv,
> > > }
> > > local_irq_restore(flags);
> > >
> > > - dst->pages[i] = d;
> > > -
> > > + dst->pages[i++] = d;
> > > reloc_offset += PAGE_SIZE;
> > > }
> > > - dst->page_count = num_pages;
> > >
> > > return dst;
> > >
> > > --
> > > 1.9.0.rc3
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ben Widawsky, Intel Open Source Technology Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
