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

[Laurent Pinchart]

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 at 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.

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.

> > +}

-- 
Regards,

Laurent Pinchart