On Tue, Jun 04, 2013 at 11:05:36PM -0400, Rob Clark wrote:
> 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
I'm not sure whether we should even allow this at all. If an importer
wants to switch dma directions he should probably map it bidirectional and
we should finally bite the bullet and add a attachment_sync callback to
flush caches without dropping the mapping on the floor ...
Tbh I'm not even sure whether multiple mappings make any sense at all for
one attachment. Imo that sounds like the importer seriously lost track of
things ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
