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.
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
