Re: [RESEND PATCH 2/2] drm: GEM CMA: Add DRM PRIME support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Rob,

Thank you for the review.

On Monday 15 April 2013 15:00:58 Rob Clark wrote:
> Hi Laurent, a few mostly-minor comments, although from a quick look
> the sg_alloc_table()/sg_free_table() doesn't look quite right in all
> cases.  The other comments could just be a subject for a later patch
> if it is something that somebody needs some day..
> 
> On Mon, Apr 15, 2013 at 9:57 AM, Laurent Pinchart wrote:
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/drm_gem_cma_helper.c | 311 +++++++++++++++++++++++++++++-
> >  include/drm/drm_gem_cma_helper.h     |   9 +
> >  2 files changed, 317 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
> > b/drivers/gpu/drm/drm_gem_cma_helper.c index 8cce330..c428a51 100644
> > --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_cma_helper.c

[snip]

> > +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");
> > +               sgt = ERR_PTR(-EIO);
> > +               goto err_unlock;
> 
> don't we miss a sg_free_table() in this error path?  Or, well, I guess
> you still call it in _detach(), but if you call _map() again, I think
> we'll re-sg_alloc_table(), which doesn't seem quite right..

Indeed, good catch. I'll fix it.

> > +       }
> > +
> > +       cma_attach->dir = dir;
> > +       attach->priv = cma_attach;
> > +
> > +       DRM_DEBUG_PRIME("buffer size = %zu\n", cma_obj->base.size);
> > +
> > +err_unlock:
> > +       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. */
> 
> hmm, I wonder if it makes sense to support _unmap() and then _map()
> again with a different direction?  Not entirely sure when that would
> be needed and I suppose it is ok to add later.

I don't have a use case for that right now, I would thus vote for adding it 
later if needed :-)

> > +}

[snip]

> > +static void *drm_gem_cma_dmabuf_kmap_atomic(struct dma_buf *dmabuf,
> > +                                           unsigned long page_num)
> > +{
> > +       /* TODO */
> > +
> > +       return NULL;
> > +}
> > +
> > +static void drm_gem_cma_dmabuf_kunmap_atomic(struct dma_buf *dmabuf,
> > +                                            unsigned long page_num, void
> > *addr)
> > +{
> > +       /* TODO */
> > +}
> > +
> > +static void *drm_gem_cma_dmabuf_kmap(struct dma_buf *dmabuf,
> > +                                    unsigned long page_num)
> > +{
> > +       /* TODO */
> > +
> > +       return NULL;
> > +}
> > +
> > +static void drm_gem_cma_dmabuf_kunmap(struct dma_buf *dmabuf,
> > +                                     unsigned long page_num, void *addr)
> > +{
> > +       /* TODO */
> > +}
> 
> again, not really sure if it is required, but it should be pretty
> trivial to support kmap and friends
> 
> > +static int drm_gem_cma_dmabuf_mmap(struct dma_buf *dmabuf,
> > +                                  struct vm_area_struct *vma)
> > +{
> > +       return -ENOTTY;
> > +}
> 
> should also be pretty trivial to redirect _dmabuf_mmap() into
> drm_gem_cma_mmap()..

It will require a bit of code shuffling, but I'll give it a try.

-- 
Regards,

Laurent Pinchart