| Message ID | 20190425155114.533-2-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/prime: split drm_gem_prime_fd_to_handle() | expand |
Hi Emil. Thans, again a nice simplification. But the export_handle_to_buf now have to mutex_unlock. Please use proper goto error handlign with a single unlock in the end of the fuction. This makes it simple to check that we always do the unlock. Likewise in next function. Avoid two mutex_unlock, we prefer goto error handling. Sam On Thu, Apr 25, 2019 at 04:51:14PM +0100, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > Currently drm_gem_prime_handle_to_fd() consists of illegible amount of > goto labels, for no obvious reason. > > Split it in two (as below) making the code far simpler and obvious. > > drm_gem_prime_handle_to_fd() > - prime mtx handling > - drm_gem_object lookup and refcounting > - creating an fd for the dmabuf > - hash table lookup > > export_handle_to_buf() > - drm_gem_object export and locking > - adding the handle/fd to the hash table > > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > --- > Currently we dma_buf_put() [in the error path] even for re-export of > original imported object and "self-export" - aka > obj->import_attach->dmabuf and obj->dmabuf. > > Have not looked at it too closely, but gut suggests that may be off. > --- > drivers/gpu/drm/drm_prime.c | 106 +++++++++++++++++++----------------- > 1 file changed, 57 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 0d83b9bbf793..2b0b6e7a6a47 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -545,11 +545,54 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, > * will clean it up. > */ > obj->dma_buf = dmabuf; > - get_dma_buf(obj->dma_buf); > > return dmabuf; > } > > +static struct dma_buf *export_handle_to_buf(struct drm_device *dev, > + struct drm_file *file_priv, > + struct drm_gem_object *obj, > + uint32_t handle, > + uint32_t flags) > +{ > + struct dma_buf *dmabuf = NULL; > + int ret; > + > + mutex_lock(&dev->object_name_lock); > + /* re-export the original imported object */ > + if (obj->import_attach) > + dmabuf = obj->import_attach->dmabuf; > + > + if (!dmabuf) > + dmabuf = obj->dma_buf; > + > + if (!dmabuf) > + dmabuf = export_and_register_object(dev, obj, flags); > + > + if (IS_ERR(dmabuf)) { > + /* normally the created dma-buf takes ownership of the ref, > + * but if that fails then drop the ref > + */ > + mutex_unlock(&dev->object_name_lock); One mutex_unlock > + return dmabuf; > + } > + > + get_dma_buf(dmabuf); > + > + /* > + * If we've exported this buffer then cheat and add it to the import list > + * so we get the correct handle back. We must do this under the > + * protection of dev->object_name_lock to ensure that a racing gem close > + * ioctl doesn't miss to remove this buffer handle from the cache. > + */ > + ret = drm_prime_add_buf_handle(&file_priv->prime, dmabuf, handle); > + mutex_unlock(&dev->object_name_lock); Second mutex_unlock > + if (ret) { > + dma_buf_put(dmabuf); > + dmabuf = ERR_PTR(ret); > + } This error handling looks a little off. It assume it works correct, but please try to make it more obvious. > + return dmabuf; > +} > /** > * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers > * @dev: dev to export the buffer from > @@ -569,7 +612,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, > int *prime_fd) > { > struct drm_gem_object *obj; > - int ret = 0; > + int ret; > struct dma_buf *dmabuf; > > mutex_lock(&file_priv->prime.lock); HEre we have the mutex_lock > @@ -580,49 +623,14 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, > } > > dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle); > - if (dmabuf) { > - get_dma_buf(dmabuf); > - goto out_have_handle; > - } > + if (!dmabuf) > + dmabuf = export_handle_to_buf(dev, file_priv, obj, handle, flags); > > - mutex_lock(&dev->object_name_lock); > - /* re-export the original imported object */ > - if (obj->import_attach) { > - dmabuf = obj->import_attach->dmabuf; > - get_dma_buf(dmabuf); > - goto out_have_obj; > - } > - > - if (obj->dma_buf) { > - get_dma_buf(obj->dma_buf); > - dmabuf = obj->dma_buf; > - goto out_have_obj; > - } > - > - dmabuf = export_and_register_object(dev, obj, flags); > if (IS_ERR(dmabuf)) { > - /* normally the created dma-buf takes ownership of the ref, > - * but if that fails then drop the ref > - */ > ret = PTR_ERR(dmabuf); > - mutex_unlock(&dev->object_name_lock); > - goto out; > + goto out_object_put; HEre you move mutex_unlock down - this is good! > } > > -out_have_obj: > - /* > - * If we've exported this buffer then cheat and add it to the import list > - * so we get the correct handle back. We must do this under the > - * protection of dev->object_name_lock to ensure that a racing gem close > - * ioctl doesn't miss to remove this buffer handle from the cache. > - */ > - ret = drm_prime_add_buf_handle(&file_priv->prime, > - dmabuf, handle); > - mutex_unlock(&dev->object_name_lock); > - if (ret) > - goto fail_put_dmabuf; > - > -out_have_handle: > ret = dma_buf_fd(dmabuf, flags); > /* > * We must _not_ remove the buffer from the handle cache since the newly > @@ -630,18 +638,18 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, > * and that is invariant as long as a userspace gem handle exists. > * Closing the handle will clean out the cache anyway, so we don't leak. > */ > - if (ret < 0) { > - goto fail_put_dmabuf; > - } else { > - *prime_fd = ret; > - ret = 0; > - } > + if (ret < 0) > + goto out_dmabuf_put; > > - goto out; > + *prime_fd = ret; > > -fail_put_dmabuf: > + drm_gem_object_put_unlocked(obj); > + mutex_unlock(&file_priv->prime.lock); Here is one mutex_unlock. > + return 0; > + > +out_dmabuf_put: > dma_buf_put(dmabuf); > -out: > +out_object_put: > drm_gem_object_put_unlocked(obj); > out_unlock: > mutex_unlock(&file_priv->prime.lock); Here is the second mutex_unlock. Redo this so there is only a single mutex_unlock. So we end up with something better than before. Sam
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 0d83b9bbf793..2b0b6e7a6a47 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -545,11 +545,54 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, * will clean it up. */ obj->dma_buf = dmabuf; - get_dma_buf(obj->dma_buf); return dmabuf; } +static struct dma_buf *export_handle_to_buf(struct drm_device *dev, + struct drm_file *file_priv, + struct drm_gem_object *obj, + uint32_t handle, + uint32_t flags) +{ + struct dma_buf *dmabuf = NULL; + int ret; + + mutex_lock(&dev->object_name_lock); + /* re-export the original imported object */ + if (obj->import_attach) + dmabuf = obj->import_attach->dmabuf; + + if (!dmabuf) + dmabuf = obj->dma_buf; + + if (!dmabuf) + dmabuf = export_and_register_object(dev, obj, flags); + + if (IS_ERR(dmabuf)) { + /* normally the created dma-buf takes ownership of the ref, + * but if that fails then drop the ref + */ + mutex_unlock(&dev->object_name_lock); + return dmabuf; + } + + get_dma_buf(dmabuf); + + /* + * If we've exported this buffer then cheat and add it to the import list + * so we get the correct handle back. We must do this under the + * protection of dev->object_name_lock to ensure that a racing gem close + * ioctl doesn't miss to remove this buffer handle from the cache. + */ + ret = drm_prime_add_buf_handle(&file_priv->prime, dmabuf, handle); + mutex_unlock(&dev->object_name_lock); + if (ret) { + dma_buf_put(dmabuf); + dmabuf = ERR_PTR(ret); + } + return dmabuf; +} /** * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers * @dev: dev to export the buffer from @@ -569,7 +612,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, int *prime_fd) { struct drm_gem_object *obj; - int ret = 0; + int ret; struct dma_buf *dmabuf; mutex_lock(&file_priv->prime.lock); @@ -580,49 +623,14 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, } dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle); - if (dmabuf) { - get_dma_buf(dmabuf); - goto out_have_handle; - } + if (!dmabuf) + dmabuf = export_handle_to_buf(dev, file_priv, obj, handle, flags); - mutex_lock(&dev->object_name_lock); - /* re-export the original imported object */ - if (obj->import_attach) { - dmabuf = obj->import_attach->dmabuf; - get_dma_buf(dmabuf); - goto out_have_obj; - } - - if (obj->dma_buf) { - get_dma_buf(obj->dma_buf); - dmabuf = obj->dma_buf; - goto out_have_obj; - } - - dmabuf = export_and_register_object(dev, obj, flags); if (IS_ERR(dmabuf)) { - /* normally the created dma-buf takes ownership of the ref, - * but if that fails then drop the ref - */ ret = PTR_ERR(dmabuf); - mutex_unlock(&dev->object_name_lock); - goto out; + goto out_object_put; } -out_have_obj: - /* - * If we've exported this buffer then cheat and add it to the import list - * so we get the correct handle back. We must do this under the - * protection of dev->object_name_lock to ensure that a racing gem close - * ioctl doesn't miss to remove this buffer handle from the cache. - */ - ret = drm_prime_add_buf_handle(&file_priv->prime, - dmabuf, handle); - mutex_unlock(&dev->object_name_lock); - if (ret) - goto fail_put_dmabuf; - -out_have_handle: ret = dma_buf_fd(dmabuf, flags); /* * We must _not_ remove the buffer from the handle cache since the newly @@ -630,18 +638,18 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, * and that is invariant as long as a userspace gem handle exists. * Closing the handle will clean out the cache anyway, so we don't leak. */ - if (ret < 0) { - goto fail_put_dmabuf; - } else { - *prime_fd = ret; - ret = 0; - } + if (ret < 0) + goto out_dmabuf_put; - goto out; + *prime_fd = ret; -fail_put_dmabuf: + drm_gem_object_put_unlocked(obj); + mutex_unlock(&file_priv->prime.lock); + return 0; + +out_dmabuf_put: dma_buf_put(dmabuf); -out: +out_object_put: drm_gem_object_put_unlocked(obj); out_unlock: mutex_unlock(&file_priv->prime.lock);