Re: [PATCH 05/24] drm/i915: Replace the array of pages with a scatterlist

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Sep 10, 2012 at 05:34:48PM +0100, Chris Wilson wrote:
> On Thu, 6 Sep 2012 18:49:24 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > On Tue,  4 Sep 2012 21:02:57 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 
> > > Rather than have multiple data structures for describing our page layout
> > > in conjunction with the array of pages, we can migrate all users over to
> > > a scatterlist.
> > > 
> > > One major advantage, other than unifying the page tracking structures,
> > > this offers is that we replace the vmalloc'ed array (which can be up to
> > > a megabyte in size) with a chain of individual pages which helps reduce
> > > memory pressure.
> > > 
> > > The disadvantage is that we then do not have a simple array to iterate,
> > > or to access randomly. The common case for this is in the relocation
> > > processing, which will typically fit within a single scatterlist page
> > > and so be almost the same cost as the simple array. For iterating over
> > > the array, the extra function call could be optimised away, but in
> > > reality is an insignificant cost of either binding the pages, or
> > > performing the pwrite/pread.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Ok, I agree with Chris' comments here and slurped in patches 1-5.

Thanks, Daniel

> > 
> > 
> > Now that my eyes are done bleeding, easy ones:
> > 
> > ERROR: space required after that ',' (ctx:VxV)
> > #69: FILE: drivers/char/agp/intel-gtt.c:99:
> > +	for_each_sg(st->sgl, sg, num_entries,i)
> >  	                                    ^
> > 
> > WARNING: Prefer pr_err(... to printk(KERN_ERR, ...
> > #189: FILE: drivers/gpu/drm/drm_cache.c:117:
> > +		printk(KERN_ERR "Timed out waiting for cache
> > flush.\n");
> > 
> > WARNING: Prefer pr_err(... to printk(KERN_ERR, ...
> > #191: FILE: drivers/gpu/drm/drm_cache.c:119:
> > +	printk(KERN_ERR "Architecture has no drm_cache.c support\n");
> 
> Hmm, the drm_cache one is tricky as it is a continuation of the style of
> the file and so is probably best kept and then the whole file fixed to
> follow the new conventions.
> 
> > In addition to the inline comments, it would have been even slightly
> > easier to review without the s/page/i since it seems to just be for no
> > compelling reason anyway.
> 
> It was motivated by using the common idiom for for_each_sg() and by
> allowing 'struct page *page' as being the natural local variable within
> the loop. So I think the end result justifies the small amount of extra
> churn in the patch.
> 
> > >  	if (intel_private.base.needs_dmar) {
> > > -		ret = intel_gtt_map_memory(mem->pages, mem->page_count,
> > > -					   &mem->sg_list, &mem->num_sg);
> > > +		struct sg_table st;
> > > +
> > > +		ret = intel_gtt_map_memory(mem->pages, mem->page_count, &st);
> > >  		if (ret != 0)
> > >  			return ret;
> > >  
> > > -		intel_gtt_insert_sg_entries(mem->sg_list, mem->num_sg,
> > > -					    pg_start, type);
> > > +		intel_gtt_insert_sg_entries(&st, pg_start, type);
> > > +		mem->sg_list = st.sgl;
> > > +		mem->num_sg = st.nents;
> > 
> > Can you explain how the corresponding free for the sg_table gets called
> > here?
> 
> The sg_table is just a small placeholder that is reconstructed in
> intel_gtt_unmap_memory() for sg_free_table().
> 
> > > @@ -1749,20 +1771,27 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> > >  	BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
> > >  	BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
> > >  
> > > -	/* Get the list of pages out of our struct file.  They'll be pinned
> > > -	 * at this point until we release them.
> > > -	 */
> > > +	st = kmalloc(sizeof(*st), GFP_KERNEL);
> > > +	if (st == NULL)
> > > +		return -ENOMEM;
> > > +
> > >  	page_count = obj->base.size / PAGE_SIZE;
> > > -	obj->pages = drm_malloc_ab(page_count, sizeof(struct page *));
> > > -	if (obj->pages == NULL)
> > > +	if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
> > > +		sg_free_table(st);
> > > +		kfree(st);
> > >  		return -ENOMEM;
> > > +	}
> > 
> > I think the call here to sg_free_table is bogus.
> 
> Experience says otherwise ;-)
> 
> The reason is that the sg_alloc_table chains together its individual
> page allocations but doesn't perform any unwind if one fails before
> reporting the error. sg_free_table() does the right thing in those
> circumstances.
> 
> > > -	/* link the pages into an SG then map the sg */
> > > -	sg = drm_prime_pages_to_sg(obj->pages, npages);
> > > -	nents = dma_map_sg(attachment->dev, sg->sgl, sg->nents, dir);
> > >  	i915_gem_object_pin_pages(obj);
> > 
> > <bikeshed>
> > I think the right way to go about this is to add rm_prime_pages_to_st
> > since you're pushing the whole st>sg thing, other drivers can leverage
> > it.
> > </bikeshed>
> 
> Quite possibly true, but the code will change later and lose some of its
> generality. Or at least no else is like i915 yet.
>  
> > The lifetime description we discussed on IRC would have helped here as
> > well.
> 
> > >  static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
> > > @@ -80,7 +104,9 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
> > >  {
> > >  	struct drm_i915_gem_object *obj = dma_buf->priv;
> > >  	struct drm_device *dev = obj->base.dev;
> > > -	int ret;
> > > +	struct scatterlist *sg;
> > > +	struct page **pages;
> > > +	int ret, i;
> > >  
> > >  	ret = i915_mutex_lock_interruptible(dev);
> > >  	if (ret)
> > > @@ -92,22 +118,33 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
> > >  	}
> > >  
> > >  	ret = i915_gem_object_get_pages(obj);
> > > -	if (ret) {
> > > -		mutex_unlock(&dev->struct_mutex);
> > > -		return ERR_PTR(ret);
> > > -	}
> > > +	if (ret)
> > > +		goto error;
> > >  
> > > -	obj->dma_buf_vmapping = vmap(obj->pages, obj->base.size / PAGE_SIZE, 0, PAGE_KERNEL);
> > > -	if (!obj->dma_buf_vmapping) {
> > > -		DRM_ERROR("failed to vmap object\n");
> > > -		goto out_unlock;
> > > -	}
> > > +	ret = -ENOMEM;
> > > +
> > > +	pages = drm_malloc_ab(obj->pages->nents, sizeof(struct page *));
> > > +	if (pages == NULL)
> > > +		goto error;
> > > +
> > > +	for_each_sg(obj->pages->sgl, sg, obj->pages->nents, i)
> > > +		pages[i] = sg_page(sg);
> > > +
> > > +	obj->dma_buf_vmapping = vmap(pages, obj->pages->nents, 0, PAGE_KERNEL);
> > > +	drm_free_large(pages);
> > > +
> > > +	if (!obj->dma_buf_vmapping)
> > > +		goto error;
> > >  
> > >  	obj->vmapping_count = 1;
> > >  	i915_gem_object_pin_pages(obj);
> > >  out_unlock:
> > >  	mutex_unlock(&dev->struct_mutex);
> > >  	return obj->dma_buf_vmapping;
> > > +
> > > +error:
> > > +	mutex_unlock(&dev->struct_mutex);
> > > +	return ERR_PTR(ret);
> > >  }
> > >  
> > >  static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
> > 
> > The return on vmap failing looks incorrect to me here. Also, I think
> > leaving the DRM_ERROR would have been nice.
> 
> Since we already return the ERR_PTR(-ENOMEM) we are not breaking any
> semantics by reporting the oom for vmap as well. And yes it would be
> nice if vmap gave a specific error as well. So other than the change to
> an explicit errno, I'm not sure what mistake you are point out.
> 
> In this case the DRM_ERROR has an obvious errno returned to userspace,
> much more informative.
> > 
> > > @@ -270,26 +233,10 @@ void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
> > >  		BUG();
> > >  	}
> > >  
> > > -	if (obj->sg_table) {
> > > -		i915_ppgtt_insert_sg_entries(ppgtt,
> > > -					     obj->sg_table->sgl,
> > > -					     obj->sg_table->nents,
> > > -					     obj->gtt_space->start >> PAGE_SHIFT,
> > > -					     pte_flags);
> > > -	} else if (dev_priv->mm.gtt->needs_dmar) {
> > > -		BUG_ON(!obj->sg_list);
> > > -
> > > -		i915_ppgtt_insert_sg_entries(ppgtt,
> > > -					     obj->sg_list,
> > > -					     obj->num_sg,
> > > -					     obj->gtt_space->start >> PAGE_SHIFT,
> > > -					     pte_flags);
> > > -	} else
> > > -		i915_ppgtt_insert_pages(ppgtt,
> > > -					obj->gtt_space->start >> PAGE_SHIFT,
> > > -					obj->base.size >> PAGE_SHIFT,
> > > -					obj->pages,
> > > -					pte_flags);
> > > +	i915_ppgtt_insert_sg_entries(ppgtt,
> > > +				     obj->sg_table ?: obj->pages,
> > > +				     obj->gtt_space->start >> PAGE_SHIFT,
> > > +				     pte_flags);
> > >  }
> > 
> > I got lost here. Is it, if there is a prime sg_table use that, otherwise
> > just use the object's sgt? If so, I think has_dma_mapping is more
> > readable.
> > Also, I wonder if ?: pissed off the clang people?
> 
> Right, this is just a step along the path to enlightment. 2 out of the 3
> paths now use obj->pages with the dmabuf being the only exception to
> still create an obj->sg_table scatterlist. '?:' is widely used by the
> kernel, if clang doesn't yet support it, that's their problem. But rest
> assured it is removed in a couple of patches after migrating dmabuf over
> to the page ops.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> 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