On Tue, Jun 4, 2013 at 9:22 PM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> Hi Rob,
>
> On Tuesday 04 June 2013 17:56:36 Rob Clark wrote:
>> couple small comments, other than those it looks ok
>
> Thanks for the review.
>
>> On Mon, Jun 3, 2013 at 10:20 PM, Laurent Pinchart wrote:
>> > Signed-off-by: Laurent Pinchart
>> > <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>> > ---
>> >
>> > drivers/gpu/drm/drm_gem_cma_helper.c | 321 +++++++++++++++++++++++++++++-
>> > include/drm/drm_gem_cma_helper.h | 9 +
>> > 2 files changed, 327 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
>> > b/drivers/gpu/drm/drm_gem_cma_helper.c index 7a4db4e..1dc2157 100644
>> > --- a/drivers/gpu/drm/drm_gem_cma_helper.c
>> > +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
>> > @@ -21,6 +21,9 @@
>> > #include <linux/slab.h>
>> > #include <linux/mutex.h>
>> > #include <linux/export.h>
>> > +#if CONFIG_DMA_SHARED_BUFFER
>> > +#include <linux/dma-buf.h>
>> > +#endif
>>
>> I don't think we need the #if, since drm selects DMA_SHARED_BUFFER
>>
>> and same for other spot below
>
> Indeed. Will be fixed in the next version.
>
>> > #include <linux/dma-mapping.h>
>> >
>> > #include <drm/drmP.h>
>
> [snip]
>
>> > @@ -291,3 +321,288 @@ void drm_gem_cma_describe(struct drm_gem_cma_object
>> > *cma_obj, struct seq_file *m>
>> > }
>> > EXPORT_SYMBOL_GPL(drm_gem_cma_describe);
>> > #endif
>> >
>> > +
>> > +/*
>> > -------------------------------------------------------------------------
>> > ---- + * DMA-BUF
>> > + */
>> > +
>> > +#if CONFIG_DMA_SHARED_BUFFER
>> > +struct drm_gem_cma_dmabuf_attachment {
>> > + struct sg_table sgt;
>> > + enum dma_data_direction dir;
>> > +};
>> > +
>> > +static int drm_gem_cma_dmabuf_attach(struct dma_buf *dmabuf, struct
>> > device *dev, + struct
>> > dma_buf_attachment *attach) +{
>> > + struct drm_gem_cma_dmabuf_attachment *cma_attach;
>> > +
>> > + cma_attach = kzalloc(sizeof(*cma_attach), GFP_KERNEL);
>> > + if (!cma_attach)
>> > + return -ENOMEM;
>> > +
>> > + cma_attach->dir = DMA_NONE;
>> > + attach->priv = cma_attach;
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static void drm_gem_cma_dmabuf_detach(struct dma_buf *dmabuf,
>> > + struct dma_buf_attachment *attach)
>> > +{
>> > + struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv;
>> > + struct sg_table *sgt;
>> > +
>> > + if (cma_attach == NULL)
>> > + return;
>> > +
>> > + sgt = &cma_attach->sgt;
>> > +
>> > + if (cma_attach->dir != DMA_NONE)
>> > + dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents,
>> > + cma_attach->dir);
>> > +
>> > + sg_free_table(sgt);
>> > + kfree(cma_attach);
>> > + attach->priv = NULL;
>> > +}
>> > +
>> > +static struct sg_table *
>> > +drm_gem_cma_dmabuf_map(struct dma_buf_attachment *attach,
>> > + enum dma_data_direction dir)
>> > +{
>> > + struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv;
>> > + struct drm_gem_cma_object *cma_obj = attach->dmabuf->priv;
>> > + struct drm_device *drm = cma_obj->base.dev;
>> > + struct scatterlist *rd, *wr;
>> > + struct sg_table *sgt;
>> > + unsigned int i;
>> > + int nents, ret;
>> > +
>> > + DRM_DEBUG_PRIME("\n");
>> > +
>> > + if (WARN_ON(dir == DMA_NONE))
>> > + return ERR_PTR(-EINVAL);
>> > +
>> > + /* Return the cached mapping when possible. */
>> > + if (cma_attach->dir == dir)
>> > + return &cma_attach->sgt;
>> > +
>> > + /* Two mappings with different directions for the same attachment
>> > are + * not allowed.
>> > + */
>> > + if (WARN_ON(cma_attach->dir != DMA_NONE))
>> > + return ERR_PTR(-EBUSY);
>> > +
>> > + sgt = &cma_attach->sgt;
>> > +
>> > + ret = sg_alloc_table(sgt, cma_obj->sgt->orig_nents, GFP_KERNEL);
>> > + if (ret) {
>> > + DRM_ERROR("failed to alloc sgt.\n");
>> > + return ERR_PTR(-ENOMEM);
>> > + }
>> > +
>> > + mutex_lock(&drm->struct_mutex);
>> > +
>> > + rd = cma_obj->sgt->sgl;
>> > + wr = sgt->sgl;
>> > + for (i = 0; i < sgt->orig_nents; ++i) {
>> > + sg_set_page(wr, sg_page(rd), rd->length, rd->offset);
>> > + rd = sg_next(rd);
>> > + wr = sg_next(wr);
>> > + }
>> > +
>> > + nents = dma_map_sg(attach->dev, sgt->sgl, sgt->orig_nents, dir);
>> > + if (!nents) {
>> > + DRM_ERROR("failed to map sgl with iommu.\n");
>> > + sg_free_table(sgt);
>> > + sgt = ERR_PTR(-EIO);
>> > + goto done;
>> > + }
>> > +
>> > + cma_attach->dir = dir;
>> > + attach->priv = cma_attach;
>> > +
>> > + DRM_DEBUG_PRIME("buffer size = %zu\n", cma_obj->base.size);
>> > +
>> > +done:
>> > + mutex_unlock(&drm->struct_mutex);
>> > + return sgt;
>> > +}
>> > +
>> > +static void drm_gem_cma_dmabuf_unmap(struct dma_buf_attachment *attach,
>> > + struct sg_table *sgt,
>> > + enum dma_data_direction dir)
>> > +{
>> > + /* Nothing to do. */
>>
>> I kinda think that if you don't support multiple mappings with
>> different direction, that you should at least support unmap and then
>> let someone else map with other direction
>
> That would make sense, but in that case I wouldn't be able to cache the
> mapping. It would probably be better to add proper support for multiple
> mappings with different directions.
well, you could still cache it, you'd just have to invalidate that
cache on transition in direction
> Given that the mapping is cached in the attachment, this will only be an issue
> if a driver tries to map the same attachment twice with different directions.
> Isn't that an uncommon use case that could be fixed later ? :-) I'd like to
> get this set in v3.11 if possible.
I don't feel strongly that this should block merging, vs fixing at a
(not too much later) date
BR,
-R
>> > +}
>
> --
> Regards,
>
> Laurent Pinchart
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
