Re: [PATCH 4/5] drm/i915: Add a partial GGTT view type

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ma, 2015-04-27 at 15:50 +0100, Tvrtko Ursulin wrote:
> Hi,
> 
> On 04/24/2015 01:09 PM, Joonas Lahtinen wrote:
> >
> > Partial view type allows manipulating parts of huge BOs through the GGTT,
> > which was not previously possible due to constraint that whole object had
> > to be mapped for any access to it through GGTT.
> >
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem_gtt.c |   46 +++++++++++++++++++++++++++++++++++
> >   drivers/gpu/drm/i915/i915_gem_gtt.h |   15 ++++++++++--
> >   2 files changed, 59 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 5babbd3..5937d3d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2764,6 +2764,47 @@ err_st_alloc:
> >   	return ERR_PTR(ret);
> >   }
> >
> > +static struct sg_table *
> > +intel_partial_pages(const struct i915_ggtt_view *view,
> > +		    struct drm_i915_gem_object *obj)
> > +{
> > +	struct sg_table *st;
> > +	struct scatterlist *sg;
> > +	struct sg_page_iter obj_sg_iter;
> > +	int ret;
> > +
> > +	st = kmalloc(sizeof(*st), GFP_KERNEL);
> > +	if (!st)
> > +		goto err_st_alloc;
> > +
> > +	ret = sg_alloc_table(st, view->params.partial.size, GFP_KERNEL);
> > +	if (ret)
> > +		goto err_sg_alloc;
> > +
> > +	sg = st->sgl;
> > +	st->nents = 0;
> 
> sg_alloc_table configures the sg_table so not needed I think. Although I 
> do see I am also doing it. :)
> 

I initially stripped it w/r your code, but I was so desperate debugging
the for_each_sg_page interface I tried everything ;)

Removed it.

> > +	for_each_sg_page(obj->pages->sgl, &obj_sg_iter, obj->pages->nents,
> > +		view->params.partial.offset)
> > +	{
> > +		if (st->nents >= view->params.partial.size)
> > +			break;
> > +
> > +		sg_set_page(sg, NULL, PAGE_SIZE, 0);
> > +		sg_dma_address(sg) = sg_page_iter_dma_address(&obj_sg_iter);
> > +		sg_dma_len(sg) = PAGE_SIZE;
> > +
> > +		sg = sg_next(sg);
> > +		st->nents++;
> > +	}
> 
> I suppose in this case (as opposed to rotated view) using 
> sg_alloc_table_from_pages() could produce a more compact table. With the 
> caveat of that it doesn't always work (see i915_gem_userptr.c/st_set_pages).
> 
> So maybe promote to driver public st_set_pages and call in on an array 
> of pages?
> 

Disregarded regards to your later mail.

> > +
> > +	return st;
> > +
> > +err_sg_alloc:
> > +	kfree(st);
> 
> Here you lose ret from sg_alloc_table.
> 

Good catch.

> > +err_st_alloc:
> > +	return ERR_PTR(-ENOMEM);
> > +}
> > +
> >   static int
> >   i915_get_ggtt_vma_pages(struct i915_vma *vma)
> >   {
> > @@ -2777,6 +2818,9 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
> >   	else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED)
> >   		vma->ggtt_view.pages =
> >   			intel_rotate_fb_obj_pages(&vma->ggtt_view, vma->obj);
> > +	else if (vma->ggtt_view.type == I915_GGTT_VIEW_PARTIAL)
> > +		vma->ggtt_view.pages =
> > +			intel_partial_pages(&vma->ggtt_view, vma->obj);
> >   	else
> >   		WARN_ONCE(1, "GGTT view %u not implemented!\n",
> >   			  vma->ggtt_view.type);
> > @@ -2859,6 +2903,8 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> >   	if (view->type == I915_GGTT_VIEW_NORMAL ||
> >   	    view->type == I915_GGTT_VIEW_ROTATED) {
> >   		return obj->base.size;
> > +	} else if (view->type == I915_GGTT_VIEW_PARTIAL) {
> > +		return view->params.partial.size << PAGE_SHIFT;
> >   	} else {
> >   		WARN_ONCE(1, "GGTT view %u not implemented!\n", view->type);
> >   		return obj->base.size;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > index 34b7cca..ab1ad8a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > @@ -117,7 +117,8 @@ typedef uint64_t gen8_pde_t;
> >
> >   enum i915_ggtt_view_type {
> >   	I915_GGTT_VIEW_NORMAL = 0,
> > -	I915_GGTT_VIEW_ROTATED
> > +	I915_GGTT_VIEW_ROTATED,
> > +	I915_GGTT_VIEW_PARTIAL,
> >   };
> >
> >   struct intel_rotation_info {
> > @@ -130,6 +131,13 @@ struct intel_rotation_info {
> >   struct i915_ggtt_view {
> >   	enum i915_ggtt_view_type type;
> >
> > +	union {
> > +		struct {
> > +			pgoff_t offset;
> > +			size_t size;
> 
> Size is in pages right? Maybe it would be more self-documenting to use 
> some basic type like unsigned int or long since size_t, to me at least, 
> suggests bytes.
> 

Yeah, using unsigned long for offset and unsigned int for size, maps
more directly to the functions they're used with.

> > +		} partial;
> > +	} params;
> > +
> >   	struct sg_table *pages;
> >
> >   	union {
> > @@ -495,7 +503,10 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
> >   	if (WARN_ON(!a || !b))
> >   		return false;
> >
> > -	return a->type == b->type;
> > +	if (a->type != b->type)
> > +		return false;
> > +
> > +	return !memcmp(&a->params, &b->params, sizeof(a->params));
> 
> So for rotated views it would still do memcmp. OK structure is zeroed on 
> alloc, but it is pointless to do so.
> 

I'd rather not have special cases for each view type all around the
code, as I think even the current ones that there are should be reduced.

Regards, Joonas

> Regards,
> 
> Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx