| Message ID | 20191010120000.1421-2-jjhiblot@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: OMAP_BO flags | expand |
Hi JJ, On 10/10/2019 14:59, Jean-Jacques Hiblot wrote: > This would give us a WARN_ON() if the pin/unpin calls are unbalanced. > > Proposed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/omapdrm/omap_gem.c | 44 +++++++++++++++--------------- > 1 file changed, 22 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c > index 08f539efddfb..61caa7a1a24b 100644 > --- a/drivers/gpu/drm/omapdrm/omap_gem.c > +++ b/drivers/gpu/drm/omapdrm/omap_gem.c > @@ -67,7 +67,7 @@ struct omap_gem_object { > /** > * # of users of dma_addr > */ > - u32 dma_addr_cnt; > + refcount_t dma_addr_cnt; > > /** > * If the buffer has been imported from a dmabuf the OMAP_DB_DMABUF flag > @@ -773,13 +773,15 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr) > mutex_lock(&omap_obj->lock); > > if (!omap_gem_is_contiguous(omap_obj) && priv->has_dmm) { > - if (omap_obj->dma_addr_cnt == 0) { > + if (refcount_read(&omap_obj->dma_addr_cnt) == 0) { > u32 npages = obj->size >> PAGE_SHIFT; > enum tiler_fmt fmt = gem2fmt(omap_obj->flags); > struct tiler_block *block; > > BUG_ON(omap_obj->block); > > + refcount_set(&omap_obj->dma_addr_cnt, 1); > + > ret = omap_gem_attach_pages(obj); > if (ret) > goto fail; > @@ -813,10 +815,10 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr) > omap_obj->block = block; > > DBG("got dma address: %pad", &omap_obj->dma_addr); > + } else { > + refcount_inc(&omap_obj->dma_addr_cnt); > } > > - omap_obj->dma_addr_cnt++; > - > *dma_addr = omap_obj->dma_addr; > } else if (omap_gem_is_contiguous(omap_obj)) { > *dma_addr = omap_obj->dma_addr; > @@ -846,22 +848,19 @@ void omap_gem_unpin(struct drm_gem_object *obj) > > mutex_lock(&omap_obj->lock); > > - if (omap_obj->dma_addr_cnt > 0) { > - omap_obj->dma_addr_cnt--; > - if (omap_obj->dma_addr_cnt == 0) { > - ret = tiler_unpin(omap_obj->block); > - if (ret) { > - dev_err(obj->dev->dev, > - "could not unpin pages: %d\n", ret); > - } > - ret = tiler_release(omap_obj->block); > - if (ret) { > - dev_err(obj->dev->dev, > - "could not release unmap: %d\n", ret); > - } > - omap_obj->dma_addr = 0; > - omap_obj->block = NULL; > + if (refcount_dec_and_test(&omap_obj->dma_addr_cnt)) { > + ret = tiler_unpin(omap_obj->block); > + if (ret) { > + dev_err(obj->dev->dev, > + "could not unpin pages: %d\n", ret); > + } > + ret = tiler_release(omap_obj->block); > + if (ret) { > + dev_err(obj->dev->dev, > + "could not release unmap: %d\n", ret); > } > + omap_obj->dma_addr = 0; > + omap_obj->block = NULL; This is not correct, and causes problems on devices without DMM (or, I think, also on devices with DMM if you disable it manually). The old code was tracking dma_addr_cnt only for DMM buffers, and in omap_gem_unpin() the code checks if dma_addr_cnt > 0, and if so, it goes and decrements and possibly frees the DMM buffer. So for non-DMM buffers, nothing was done there. But this version always decrements the dma_addr_cnt, which then breaks down. Tomi
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 08f539efddfb..61caa7a1a24b 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -67,7 +67,7 @@ struct omap_gem_object { /** * # of users of dma_addr */ - u32 dma_addr_cnt; + refcount_t dma_addr_cnt; /** * If the buffer has been imported from a dmabuf the OMAP_DB_DMABUF flag @@ -773,13 +773,15 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr) mutex_lock(&omap_obj->lock); if (!omap_gem_is_contiguous(omap_obj) && priv->has_dmm) { - if (omap_obj->dma_addr_cnt == 0) { + if (refcount_read(&omap_obj->dma_addr_cnt) == 0) { u32 npages = obj->size >> PAGE_SHIFT; enum tiler_fmt fmt = gem2fmt(omap_obj->flags); struct tiler_block *block; BUG_ON(omap_obj->block); + refcount_set(&omap_obj->dma_addr_cnt, 1); + ret = omap_gem_attach_pages(obj); if (ret) goto fail; @@ -813,10 +815,10 @@ int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr) omap_obj->block = block; DBG("got dma address: %pad", &omap_obj->dma_addr); + } else { + refcount_inc(&omap_obj->dma_addr_cnt); } - omap_obj->dma_addr_cnt++; - *dma_addr = omap_obj->dma_addr; } else if (omap_gem_is_contiguous(omap_obj)) { *dma_addr = omap_obj->dma_addr; @@ -846,22 +848,19 @@ void omap_gem_unpin(struct drm_gem_object *obj) mutex_lock(&omap_obj->lock); - if (omap_obj->dma_addr_cnt > 0) { - omap_obj->dma_addr_cnt--; - if (omap_obj->dma_addr_cnt == 0) { - ret = tiler_unpin(omap_obj->block); - if (ret) { - dev_err(obj->dev->dev, - "could not unpin pages: %d\n", ret); - } - ret = tiler_release(omap_obj->block); - if (ret) { - dev_err(obj->dev->dev, - "could not release unmap: %d\n", ret); - } - omap_obj->dma_addr = 0; - omap_obj->block = NULL; + if (refcount_dec_and_test(&omap_obj->dma_addr_cnt)) { + ret = tiler_unpin(omap_obj->block); + if (ret) { + dev_err(obj->dev->dev, + "could not unpin pages: %d\n", ret); + } + ret = tiler_release(omap_obj->block); + if (ret) { + dev_err(obj->dev->dev, + "could not release unmap: %d\n", ret); } + omap_obj->dma_addr = 0; + omap_obj->block = NULL; } mutex_unlock(&omap_obj->lock); @@ -879,7 +878,7 @@ int omap_gem_rotated_dma_addr(struct drm_gem_object *obj, u32 orient, mutex_lock(&omap_obj->lock); - if ((omap_obj->dma_addr_cnt > 0) && omap_obj->block && + if ((refcount_read(&omap_obj->dma_addr_cnt) > 0) && omap_obj->block && (omap_obj->flags & OMAP_BO_TILED)) { *dma_addr = tiler_tsptr(omap_obj->block, orient, x, y); ret = 0; @@ -1030,7 +1029,8 @@ void omap_gem_describe(struct drm_gem_object *obj, struct seq_file *m) seq_printf(m, "%08x: %2d (%2d) %08llx %pad (%2d) %p %4d", omap_obj->flags, obj->name, kref_read(&obj->refcount), - off, &omap_obj->dma_addr, omap_obj->dma_addr_cnt, + off, &omap_obj->dma_addr, + refcount_read(&omap_obj->dma_addr_cnt), omap_obj->vaddr, omap_obj->roll); if (omap_obj->flags & OMAP_BO_TILED) { @@ -1093,7 +1093,7 @@ void omap_gem_free_object(struct drm_gem_object *obj) mutex_lock(&omap_obj->lock); /* The object should not be pinned. */ - WARN_ON(omap_obj->dma_addr_cnt > 0); + WARN_ON(refcount_read(&omap_obj->dma_addr_cnt) > 0); if (omap_obj->pages) { if (omap_obj->flags & OMAP_BO_MEM_DMABUF)