| Message ID | 1429877392.19506.9.camel@jlahtine-mobl1 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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
On 04/27/2015 03:50 PM, Tvrtko Ursulin wrote: >> + 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? Scratch this, on second thought it makes no sense. Only if we had a smarter helper like sg_alloc_table_from_table_range() but no one cared about coalescing in the past. Regards, Tvrtko
On ma, 2015-04-27 at 15:50 +0100, Tvrtko Ursulin wrote: > 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@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. :) > I initially stripped it w/r your code, but I was so desperate debugging the for_each_sg_page interface I tried everything ;) Removed 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? > Disregarded regards to your later mail. > > + > > + return st; > > + > > +err_sg_alloc: > > + kfree(st); > > Here you lose ret from sg_alloc_table. > Good catch. > > +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. > Yeah, using unsigned long for offset and unsigned int for size, maps more directly to the functions they're used with. > > + } 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. > I'd rather not have special cases for each view type all around the code, as I think even the current ones that there are should be reduced. Regards, Joonas > Regards, > > Tvrtko
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; + 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(-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; + } 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. 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 | 15 ++++++++++-- 2 files changed, 59 insertions(+), 2 deletions(-)