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

Tvrtko Ursulin
2015-04-30T12:16:39Z

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