On Wed, Jun 5, 2013 at 4:44 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> 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 ;-)
oh, good point.. I was thinking more the case of multiple importers,
but you are right, there would be multiple attachments in that case so
this wouldn't be the problem
BR,
-R
> -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
