Re: [PATCH v2 5/5] drm: GEM CMA: Add DRM PRIME support

From: Rob Clark <robdclark@gmail.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 5/5] drm: GEM CMA: Add DRM PRIME support
Date: Tue, 4 Jun 2013 23:05:36 -0400	[thread overview]
Message-ID: <CAF6AEGv+VB7bjtN=5kSwfKedqhmecupWKe7ESx7AUkwoiPFQVQ@mail.gmail.com> (raw)
In-Reply-To: <2321159.EjDod52Pmh@avalon>

On Tue, Jun 4, 2013 at 9:22 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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@ideasonboard.com>
>> > ---
>> >
>> >  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
>