[Intel-gfx] [PATCH] drm/i915: Mark the final obj->pages sg entry as last

[Chris Wilson]

On Tue, Jun 09, 2015 at 12:46:30PM +0300, Imre Deak wrote:
> On ti, 2015-06-09 at 10:19 +0100, Chris Wilson wrote:
> > Currently we may mark the subsequent sg entry as the last, instead of
> > the actual last element we used. If a later iterator only used
> > sg_is_last() (such as sg_next()) then we may access the NULL page stored
> > in the elements beyond the contracted table. This may explain the
> > occasional NULL dereference we see in insert pages, such as
> > https://bugzilla.redhat.com/show_bug.cgi?id=1227892
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Imre Deak <imre.deak at intel.com>
> > Cc: stable at vger.kernel.org
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index be35f0486202..f3b66461dc68 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2195,7 +2195,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >  	int page_count, i;
> >  	struct address_space *mapping;
> >  	struct sg_table *st;
> > -	struct scatterlist *sg;
> > +	struct scatterlist *sg, *end;
> >  	struct sg_page_iter sg_iter;
> >  	struct page *page;
> >  	unsigned long last_pfn = 0;	/* suppress gcc warning */
> > @@ -2227,7 +2227,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >  	gfp = mapping_gfp_mask(mapping);
> >  	gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD;
> >  	gfp &= ~(__GFP_IO | __GFP_WAIT);
> > -	sg = st->sgl;
> > +	end = sg = st->sgl;
> >  	st->nents = 0;
> >  	for (i = 0; i < page_count; i++) {
> >  		page = shmem_read_mapping_page_gfp(mapping, i, gfp);
> > @@ -2253,13 +2253,13 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >  		if (swiotlb_nr_tbl()) {
> >  			st->nents++;
> >  			sg_set_page(sg, page, PAGE_SIZE, 0);
> > -			sg = sg_next(sg);
> > +			sg = sg_next(end = sg);
> 
> This would make sense, but for swiotlb_nr_tbl() we don't mark the last
> element, it's done by sg_alloc_table().

sg_alloc_table() uses sg_mark_end(&sgl[st->orig_nents-1]);
Hmm, we don't do page compression for swiotlb? Ok, then this part is
redundant.

> 
> >  			continue;
> >  		}
> >  #endif
> >  		if (!i || page_to_pfn(page) != last_pfn + 1) {
> >  			if (i)
> > -				sg = sg_next(sg);
> > +				sg = sg_next(end = sg);
> 
> But this looks incorrect to me, sg points now to the last element for
> which we set the page below and end points to one before the last. Or
> I'm (still) missing something..

Nah, this was a change I made to try and reduce te patch size.
Originally I did sg_set_page(); end = sg; but then thought I could avoid
resetting end to the same sg in a few cases.
Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre