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@xxxxxxxxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
