| Message ID | 1430392833.1189.9.camel@jlahtine-mobl1 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Thu, Apr 30, 2015 at 01:16:30PM +0100, Tvrtko Ursulin wrote: > On 04/30/2015 12:20 PM, Joonas Lahtinen wrote: > >@@ -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(...) Why do we even need this? memcmp implies comparing just one part of the struct, doesn't it? Of course this means we need to clear it, but imo that's the rtdt anyway. -Daniel
On ke, 2015-05-06 at 12:20 +0200, Daniel Vetter wrote: > On Thu, Apr 30, 2015 at 01:16:30PM +0100, Tvrtko Ursulin wrote: > > On 04/30/2015 12:20 PM, Joonas Lahtinen wrote: > > >@@ -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(...) > > Why do we even need this? memcmp implies comparing just one part of the > struct, doesn't it? Of course this means we need to clear it, but imo > that's the rtdt anyway. I only noticed this comment now (just sent v3 of the series out), I'm fine with leaving it like it was in the revision 1, which means that the last line is return !memcmp(..), so if the patch is otherwise OK, shall I make a one more revision, or will you merge manually? Regards, Joonas > -Daniel
On Wed, May 06, 2015 at 02:40:48PM +0300, Joonas Lahtinen wrote: > On ke, 2015-05-06 at 12:20 +0200, Daniel Vetter wrote: > > On Thu, Apr 30, 2015 at 01:16:30PM +0100, Tvrtko Ursulin wrote: > > > On 04/30/2015 12:20 PM, Joonas Lahtinen wrote: > > > >@@ -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(...) > > > > Why do we even need this? memcmp implies comparing just one part of the > > struct, doesn't it? Of course this means we need to clear it, but imo > > that's the rtdt anyway. > > I only noticed this comment now (just sent v3 of the series out), I'm > fine with leaving it like it was in the revision 1, which means that the > last line is return !memcmp(..), so if the patch is otherwise OK, shall > I make a one more revision, or will you merge manually? Already pulled in, please do a follow-up patch. Thanks, Daniel
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)); } size_t
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(-)