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