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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 04/30/2015 12:20 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.
>
> v2:
> - Retain error value from sg_alloc_table (Tvrtko Ursulin)
> - Do not zero already zeroed variable (Tvrtko Ursulin)
> - Use more common variable types for page size/offset(Tvrtko Ursulin)
>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 45 +++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.h | 15 +++++++++++--
>   2 files changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 640584f..ba28a67 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2762,6 +2762,46 @@ 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 = -ENOMEM;
> +
> +	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;
> +	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++;
> +	}
> +
> +	return st;
> +
> +err_sg_alloc:
> +	kfree(st);
> +err_st_alloc:
> +	return ERR_PTR(ret);
> +}
> +
>   static int
>   i915_get_ggtt_vma_pages(struct i915_vma *vma)
>   {
> @@ -2775,6 +2815,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);
> @@ -2864,6 +2907,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..96a9ef7 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 {
> +			unsigned long offset;
> +			unsigned int size;
> +		} 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));

Still don't like this, would this be so bad:

if (a->type != PARTIAL)
	return a->type == b->type;
else
	return !memcmp(...)

?

Regards,

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