| Message ID | 1346788996-19080-7-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
On Tue, 4 Sep 2012 21:02:58 +0100 Chris Wilson <chris@chris-wilson.co.uk> wrote: > By providing a callback for when we need to bind the pages, and then > release them again later, we can shorten the amount of time we hold the > foreign pages mapped and pinned, and importantly the dmabuf objects then > behave as any other normal object with respect to the shrinker and > memory management. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> with nitpicks below: Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/i915/i915_gem.c | 10 ++++---- > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 44 ++++++++++++++++++++++---------- > drivers/gpu/drm/i915/i915_gem_gtt.c | 4 +-- > 4 files changed, 37 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 1a714fa..a86f50d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -998,7 +998,6 @@ struct drm_i915_gem_object { > int pages_pin_count; > > /* prime dma-buf support */ > - struct sg_table *sg_table; > void *dma_buf_vmapping; > int vmapping_count; > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 06589a9..58075e3 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1692,7 +1692,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj) > { > const struct drm_i915_gem_object_ops *ops = obj->ops; > > - if (obj->sg_table || obj->pages == NULL) > + if (obj->pages == NULL) > return 0; > > BUG_ON(obj->gtt_space); > @@ -1838,7 +1838,7 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj) > const struct drm_i915_gem_object_ops *ops = obj->ops; > int ret; > > - if (obj->sg_table || obj->pages) > + if (obj->pages) > return 0; > > BUG_ON(obj->pages_pin_count); > @@ -3731,9 +3731,6 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) > > trace_i915_gem_object_destroy(obj); > > - if (gem_obj->import_attach) > - drm_prime_gem_destroy(gem_obj, obj->sg_table); > - > if (obj->phys_obj) > i915_gem_detach_phys_object(dev, obj); > > @@ -3755,6 +3752,9 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) > > BUG_ON(obj->pages); > > + if (obj->base.import_attach) > + drm_prime_gem_destroy(&obj->base, NULL); > + > drm_gem_object_release(&obj->base); > i915_gem_info_remove_obj(dev_priv, obj->base.size); > Was the order in which destroy happens moved intentionally? > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > index 4bb1b94..ca3497e 100644 > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > @@ -82,7 +82,8 @@ out: > } > > static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, > - struct sg_table *sg, enum dma_data_direction dir) > + struct sg_table *sg, > + enum dma_data_direction dir) > { > dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); > sg_free_table(sg); I thought we frown upon unnecessary whitespace fixes in patches which have behavioral changes? > @@ -228,11 +229,35 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev, > return dma_buf_export(obj, &i915_dmabuf_ops, obj->base.size, 0600); > } > > +static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj) > +{ > + struct sg_table *sg; > + > + sg = dma_buf_map_attachment(obj->base.import_attach, DMA_BIDIRECTIONAL); > + if (IS_ERR(sg)) > + return PTR_ERR(sg); > + > + obj->pages = sg; > + obj->has_dma_mapping = true; > + return 0; > +} > + > +static void i915_gem_object_put_pages_dmabuf(struct drm_i915_gem_object *obj) > +{ > + dma_buf_unmap_attachment(obj->base.import_attach, > + obj->pages, DMA_BIDIRECTIONAL); > + obj->has_dma_mapping = false; > +} > + > +static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = { > + .get_pages = i915_gem_object_get_pages_dmabuf, > + .put_pages = i915_gem_object_put_pages_dmabuf, > +}; > + > struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, > struct dma_buf *dma_buf) > { > struct dma_buf_attachment *attach; > - struct sg_table *sg; > struct drm_i915_gem_object *obj; > int ret; > > @@ -251,34 +276,25 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, > if (IS_ERR(attach)) > return ERR_CAST(attach); > > - sg = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL); > - if (IS_ERR(sg)) { > - ret = PTR_ERR(sg); > - goto fail_detach; > - } > > obj = kzalloc(sizeof(*obj), GFP_KERNEL); > if (obj == NULL) { > ret = -ENOMEM; > - goto fail_unmap; > + goto fail_detach; > } > > ret = drm_gem_private_object_init(dev, &obj->base, dma_buf->size); > if (ret) { > kfree(obj); > - goto fail_unmap; > + goto fail_detach; > } > > - obj->has_dma_mapping = true; > - obj->sg_table = sg; > + i915_gem_object_init(obj, &i915_gem_object_dmabuf_ops); > obj->base.import_attach = attach; > > return &obj->base; > > -fail_unmap: > - dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL); > fail_detach: > dma_buf_detach(dma_buf, attach); > return ERR_PTR(ret); > } > - > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 6746109..c86dc59 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -234,7 +234,7 @@ void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt, > } > > i915_ppgtt_insert_sg_entries(ppgtt, > - obj->sg_table ?: obj->pages, > + obj->pages, > obj->gtt_space->start >> PAGE_SHIFT, > pte_flags); > } > @@ -325,7 +325,7 @@ void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj, > struct drm_device *dev = obj->base.dev; > unsigned int agp_type = cache_level_to_agp_type(dev, cache_level); > > - intel_gtt_insert_sg_entries(obj->sg_table ?: obj->pages, > + intel_gtt_insert_sg_entries(obj->pages, > obj->gtt_space->start >> PAGE_SHIFT, > agp_type); > obj->has_global_gtt_mapping = 1;
On Fri, 14 Sep 2012 11:02:02 -0700, Ben Widawsky <ben@bwidawsk.net> wrote: > On Tue, 4 Sep 2012 21:02:58 +0100 > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > @@ -3731,9 +3731,6 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) > > > > trace_i915_gem_object_destroy(obj); > > > > - if (gem_obj->import_attach) > > - drm_prime_gem_destroy(gem_obj, obj->sg_table); > > - > > if (obj->phys_obj) > > i915_gem_detach_phys_object(dev, obj); > > > > @@ -3755,6 +3752,9 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) > > > > BUG_ON(obj->pages); > > > > + if (obj->base.import_attach) > > + drm_prime_gem_destroy(&obj->base, NULL); > > + > > drm_gem_object_release(&obj->base); > > i915_gem_info_remove_obj(dev_priv, obj->base.size); > > > > Was the order in which destroy happens moved intentionally? Yes. ;) > > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > > index 4bb1b94..ca3497e 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > > @@ -82,7 +82,8 @@ out: > > } > > > > static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, > > - struct sg_table *sg, enum dma_data_direction dir) > > + struct sg_table *sg, > > + enum dma_data_direction dir) > > { > > dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); > > sg_free_table(sg); > > I thought we frown upon unnecessary whitespace fixes in patches which > have behavioral changes? Call it a leftover from the time I spent moving much of the common code to drm_prime.c -Chris
On Tue, Sep 04, 2012 at 09:02:58PM +0100, Chris Wilson wrote: > By providing a callback for when we need to bind the pages, and then > release them again later, we can shorten the amount of time we hold the > foreign pages mapped and pinned, and importantly the dmabuf objects then > behave as any other normal object with respect to the shrinker and > memory management. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Queued for -next, thanks for the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1a714fa..a86f50d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -998,7 +998,6 @@ struct drm_i915_gem_object { int pages_pin_count; /* prime dma-buf support */ - struct sg_table *sg_table; void *dma_buf_vmapping; int vmapping_count; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 06589a9..58075e3 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1692,7 +1692,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj) { const struct drm_i915_gem_object_ops *ops = obj->ops; - if (obj->sg_table || obj->pages == NULL) + if (obj->pages == NULL) return 0; BUG_ON(obj->gtt_space); @@ -1838,7 +1838,7 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj) const struct drm_i915_gem_object_ops *ops = obj->ops; int ret; - if (obj->sg_table || obj->pages) + if (obj->pages) return 0; BUG_ON(obj->pages_pin_count); @@ -3731,9 +3731,6 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) trace_i915_gem_object_destroy(obj); - if (gem_obj->import_attach) - drm_prime_gem_destroy(gem_obj, obj->sg_table); - if (obj->phys_obj) i915_gem_detach_phys_object(dev, obj); @@ -3755,6 +3752,9 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) BUG_ON(obj->pages); + if (obj->base.import_attach) + drm_prime_gem_destroy(&obj->base, NULL); + drm_gem_object_release(&obj->base); i915_gem_info_remove_obj(dev_priv, obj->base.size); diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 4bb1b94..ca3497e 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -82,7 +82,8 @@ out: } static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, - struct sg_table *sg, enum dma_data_direction dir) + struct sg_table *sg, + enum dma_data_direction dir) { dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); sg_free_table(sg); @@ -228,11 +229,35 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev, return dma_buf_export(obj, &i915_dmabuf_ops, obj->base.size, 0600); } +static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj) +{ + struct sg_table *sg; + + sg = dma_buf_map_attachment(obj->base.import_attach, DMA_BIDIRECTIONAL); + if (IS_ERR(sg)) + return PTR_ERR(sg); + + obj->pages = sg; + obj->has_dma_mapping = true; + return 0; +} + +static void i915_gem_object_put_pages_dmabuf(struct drm_i915_gem_object *obj) +{ + dma_buf_unmap_attachment(obj->base.import_attach, + obj->pages, DMA_BIDIRECTIONAL); + obj->has_dma_mapping = false; +} + +static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = { + .get_pages = i915_gem_object_get_pages_dmabuf, + .put_pages = i915_gem_object_put_pages_dmabuf, +}; + struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf) { struct dma_buf_attachment *attach; - struct sg_table *sg; struct drm_i915_gem_object *obj; int ret; @@ -251,34 +276,25 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, if (IS_ERR(attach)) return ERR_CAST(attach); - sg = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL); - if (IS_ERR(sg)) { - ret = PTR_ERR(sg); - goto fail_detach; - } obj = kzalloc(sizeof(*obj), GFP_KERNEL); if (obj == NULL) { ret = -ENOMEM; - goto fail_unmap; + goto fail_detach; } ret = drm_gem_private_object_init(dev, &obj->base, dma_buf->size); if (ret) { kfree(obj); - goto fail_unmap; + goto fail_detach; } - obj->has_dma_mapping = true; - obj->sg_table = sg; + i915_gem_object_init(obj, &i915_gem_object_dmabuf_ops); obj->base.import_attach = attach; return &obj->base; -fail_unmap: - dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL); fail_detach: dma_buf_detach(dma_buf, attach); return ERR_PTR(ret); } - diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 6746109..c86dc59 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -234,7 +234,7 @@ void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt, } i915_ppgtt_insert_sg_entries(ppgtt, - obj->sg_table ?: obj->pages, + obj->pages, obj->gtt_space->start >> PAGE_SHIFT, pte_flags); } @@ -325,7 +325,7 @@ void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj, struct drm_device *dev = obj->base.dev; unsigned int agp_type = cache_level_to_agp_type(dev, cache_level); - intel_gtt_insert_sg_entries(obj->sg_table ?: obj->pages, + intel_gtt_insert_sg_entries(obj->pages, obj->gtt_space->start >> PAGE_SHIFT, agp_type); obj->has_global_gtt_mapping = 1;
By providing a callback for when we need to bind the pages, and then release them again later, we can shorten the amount of time we hold the foreign pages mapped and pinned, and importantly the dmabuf objects then behave as any other normal object with respect to the shrinker and memory management. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/i915_gem.c | 10 ++++---- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 44 ++++++++++++++++++++++---------- drivers/gpu/drm/i915/i915_gem_gtt.c | 4 +-- 4 files changed, 37 insertions(+), 22 deletions(-)