[v3,3/4] drm/i915: Add a partial GGTT view type
diff mbox

Message ID 1430912138.9311.7.camel@jlahtine-mobl1
State New
Headers show

Commit Message

Joonas Lahtinen May 6, 2015, 11:35 a.m. UTC
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)
v3:
- Only compare additional view parameters when need to (Tvrtko Ursulin)
v4:
- Do zero out the variable that needs to be (bug introduced in v2).

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 | 46 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h | 16 +++++++++++--
 2 files changed, 60 insertions(+), 2 deletions(-)

Comments

Tvrtko Ursulin May 6, 2015, 12:24 p.m. UTC | #1
On 05/06/2015 12:35 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)
> v3:
> - Only compare additional view parameters when need to (Tvrtko Ursulin)
> v4:
> - Do zero out the variable that needs to be (bug introduced in v2).

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
Daniel Vetter May 7, 2015, 4:17 p.m. UTC | #2
On Wed, May 06, 2015 at 01:24:18PM +0100, Tvrtko Ursulin wrote:
> 
> 
> On 05/06/2015 12:35 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)
> >v3:
> >- Only compare additional view parameters when need to (Tvrtko Ursulin)
> >v4:
> >- Do zero out the variable that needs to be (bug introduced in v2).
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Pulled in entire series, thanks. Imo we have now an even bigger chaos
between i915_gem.c and i915_gem_gtt.c around vma handling. But as
discussed this is something we can clean up once the dust has settled a
bit I think. I guess we could extract an i915_gem_vma.c for all this code,
leaving only low-level gtt handling to i915_gem_gtt.c.
-Daniel
Chris Wilson June 9, 2015, 8:56 a.m. UTC | #3
On Wed, May 06, 2015 at 02:35:38PM +0300, Joonas Lahtinen wrote:
> +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;
> +	st->nents = 0;
> +	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;

This is a nasty bug, as is the converse where st->nents <
st->orig_nents.

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index a7e39d4..115df10 100644
@@ -2890,7 +2900,7 @@ intel_partial_pages(const struct i915_ggtt_view *view,
                    struct drm_i915_gem_object *obj)
 {
        struct sg_table *st;
-       struct scatterlist *sg;
+       struct scatterlist *sg, *end;
        struct sg_page_iter obj_sg_iter;
        int ret = -ENOMEM;
 
@@ -2902,24 +2912,31 @@ intel_partial_pages(const struct i915_ggtt_view *view,
        if (ret)
                goto err_sg_alloc;
 
-       sg = st->sgl;
+       end = sg = st->sgl;
        st->nents = 0;
        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;
+               if (WARN_ON(st->nents >= view->params.partial.size)) {
+                       ret = -ENODEV;
+                       goto err_pages;
+               }
 
                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;
 
+               end = sg;
                sg = sg_next(sg);
                st->nents++;
        }
+       sg_mark_end(end);
 
        return st;
 
+err_pages:
+       sg_free_table(st);
 err_sg_alloc:
        kfree(st);
 err_st_alloc:

-Chris
Joonas Lahtinen June 10, 2015, 10:38 a.m. UTC | #4
Hi,

As discussed in IRC, this patch is not relevant. The interface is bit
misbehaving. CC'd Imre who agreed to go and change the interface to more
intuitive one.

Regards, Joonas

On ti, 2015-06-09 at 09:56 +0100, Chris Wilson wrote:
> On Wed, May 06, 2015 at 02:35:38PM +0300, Joonas Lahtinen wrote:
> > +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;
> > +	st->nents = 0;
> > +	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;
> 
> This is a nasty bug, as is the converse where st->nents <
> st->orig_nents.
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index a7e39d4..115df10 100644
> @@ -2890,7 +2900,7 @@ intel_partial_pages(const struct i915_ggtt_view *view,
>                     struct drm_i915_gem_object *obj)
>  {
>         struct sg_table *st;
> -       struct scatterlist *sg;
> +       struct scatterlist *sg, *end;
>         struct sg_page_iter obj_sg_iter;
>         int ret = -ENOMEM;
>  
> @@ -2902,24 +2912,31 @@ intel_partial_pages(const struct i915_ggtt_view *view,
>         if (ret)
>                 goto err_sg_alloc;
>  
> -       sg = st->sgl;
> +       end = sg = st->sgl;
>         st->nents = 0;
>         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;
> +               if (WARN_ON(st->nents >= view->params.partial.size)) {
> +                       ret = -ENODEV;
> +                       goto err_pages;
> +               }
>  
>                 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;
>  
> +               end = sg;
>                 sg = sg_next(sg);
>                 st->nents++;
>         }
> +       sg_mark_end(end);
>  
>         return st;
>  
> +err_pages:
> +       sg_free_table(st);
>  err_sg_alloc:
>         kfree(st);
>  err_st_alloc:
> 
> -Chris
>
Chris Wilson June 10, 2015, 10:56 a.m. UTC | #5
On Wed, Jun 10, 2015 at 01:38:55PM +0300, Joonas Lahtinen wrote:
> Hi,
> 
> As discussed in IRC, this patch is not relevant. The interface is bit
> misbehaving. CC'd Imre who agreed to go and change the interface to more
> intuitive one.

partial.offset + partial.size is never checked for correctness, that
would be worthy of an assert during i915_vma_create to catch future
bugs.
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7f3c03a..b058931 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2753,6 +2753,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 = -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;
+	st->nents = 0;
+	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)
 {
@@ -2766,6 +2807,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);
@@ -2855,6 +2899,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..0d46dd2 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,11 @@  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;
+	if (a->type == I915_GGTT_VIEW_PARTIAL)
+		return !memcmp(&a->params, &b->params, sizeof(a->params));
+	return true;
 }
 
 size_t