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.