[Intel-gfx] [PATCH 4/5] drm/i915: Add a partial GGTT view type

[Tvrtko Ursulin]

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 at 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. :)

> +	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?

> +
> +	return st;
> +
> +err_sg_alloc:
> +	kfree(st);

Here you lose ret from sg_alloc_table.

> +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.

> +		} 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.

Regards,

Tvrtko