[PATCH] drm/gem-cma-helper: Change the level of the allocation failure message

[PATCH] drm/gem-cma-helper: Change the level of the allocation failure message

[Boris Brezillon]

drm_gem_cma_create() prints an error message when dma_alloc_wc() fails to
allocate the amount of memory we requested. This can lead to annoying
error messages when CMA is only one possible source of memory for the BO
allocation.

Turn this error message into a debug one and add a __must_check specifier
to make sure all callers are checking the return value.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
Hello,

This problem happens with the VC4 driver which can flush its internal
cache if case of CMA allocation failures. We should only complain if the
last CMA allocation fails (the one happening after all internal caches
have been flushed).

Regards,

Boris
---
 drivers/gpu/drm/drm_gem_cma_helper.c | 4 ++--
 include/drm/drm_gem_cma_helper.h     | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 373e33f22be4..e96c95c4fd23 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -95,7 +95,7 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size)
  *
  * Returns:
  * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
- * error code on failure.
+ * error code on failure. Callers must check the return value.
  */
 struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
 					      size_t size)
@@ -112,7 +112,7 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
 	cma_obj->vaddr = dma_alloc_wc(drm->dev, size, &cma_obj->paddr,
 				      GFP_KERNEL | __GFP_NOWARN);
 	if (!cma_obj->vaddr) {
-		dev_err(drm->dev, "failed to allocate buffer with size %zu\n",
+		dev_dbg(drm->dev, "failed to allocate buffer with size %zu\n",
 			size);
 		ret = -ENOMEM;
 		goto error;
diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
index 58a739bf15f1..1b7d938a31a0 100644
--- a/include/drm/drm_gem_cma_helper.h
+++ b/include/drm/drm_gem_cma_helper.h
@@ -77,8 +77,8 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
 int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma);
 
 /* allocate physical memory */
-struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
-					      size_t size);
+struct drm_gem_cma_object *
+__must_check drm_gem_cma_create(struct drm_device *drm, size_t size);
 
 extern const struct vm_operations_struct drm_gem_cma_vm_ops;
 
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message

[Daniel Vetter]

On Wed, Oct 04, 2017 at 03:08:39PM +0200, Boris Brezillon wrote:
> drm_gem_cma_create() prints an error message when dma_alloc_wc() fails to
> allocate the amount of memory we requested. This can lead to annoying
> error messages when CMA is only one possible source of memory for the BO
> allocation.
> 
> Turn this error message into a debug one and add a __must_check specifier
> to make sure all callers are checking the return value.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Assuming gcc doesn't spot any driver that now stumbles over the
__must_check:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Would be good to get Laurent's ack too.
-Daniel

> ---
> Hello,
> 
> This problem happens with the VC4 driver which can flush its internal
> cache if case of CMA allocation failures. We should only complain if the
> last CMA allocation fails (the one happening after all internal caches
> have been flushed).
> 
> Regards,
> 
> Boris
> ---
>  drivers/gpu/drm/drm_gem_cma_helper.c | 4 ++--
>  include/drm/drm_gem_cma_helper.h     | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 373e33f22be4..e96c95c4fd23 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -95,7 +95,7 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size)
>   *
>   * Returns:
>   * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
> - * error code on failure.
> + * error code on failure. Callers must check the return value.
>   */
>  struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>  					      size_t size)
> @@ -112,7 +112,7 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>  	cma_obj->vaddr = dma_alloc_wc(drm->dev, size, &cma_obj->paddr,
>  				      GFP_KERNEL | __GFP_NOWARN);
>  	if (!cma_obj->vaddr) {
> -		dev_err(drm->dev, "failed to allocate buffer with size %zu\n",
> +		dev_dbg(drm->dev, "failed to allocate buffer with size %zu\n",
>  			size);
>  		ret = -ENOMEM;
>  		goto error;
> diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
> index 58a739bf15f1..1b7d938a31a0 100644
> --- a/include/drm/drm_gem_cma_helper.h
> +++ b/include/drm/drm_gem_cma_helper.h
> @@ -77,8 +77,8 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
>  int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma);
>  
>  /* allocate physical memory */
> -struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> -					      size_t size);
> +struct drm_gem_cma_object *
> +__must_check drm_gem_cma_create(struct drm_device *drm, size_t size);
>  
>  extern const struct vm_operations_struct drm_gem_cma_vm_ops;
>  
> -- 
> 2.11.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 740 bytes --]

Boris Brezillon <boris.brezillon@free-electrons.com> writes:

> drm_gem_cma_create() prints an error message when dma_alloc_wc() fails to
> allocate the amount of memory we requested. This can lead to annoying
> error messages when CMA is only one possible source of memory for the BO
> allocation.
>
> Turn this error message into a debug one and add a __must_check specifier
> to make sure all callers are checking the return value.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>

The __must_check seems unnecessary to me -- you're definitely going to
be doing something with the return value, because otherwise why did you
call the object allocate function?

That said, thanks for cleaning up the error message.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message

[Eric Engestrom]

On Wednesday, 2017-10-04 22:28:54 +0000, Eric Anholt wrote:
> Boris Brezillon <boris.brezillon@free-electrons.com> writes:
> 
> > drm_gem_cma_create() prints an error message when dma_alloc_wc() fails to
> > allocate the amount of memory we requested. This can lead to annoying
> > error messages when CMA is only one possible source of memory for the BO
> > allocation.
> >
> > Turn this error message into a debug one and add a __must_check specifier
> > to make sure all callers are checking the return value.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
> The __must_check seems unnecessary to me -- you're definitely going to
> be doing something with the return value, because otherwise why did you
> call the object allocate function?

Indeed, `__must_check` (aka `warn_unused_result`) only makes sure the
return value is not discarded, which will probably always be true here.

> The `warn_unused_result` attribute causes a warning to be emitted if
> a caller of the function with this attribute does not use its return
> value.

I think we need a sparse attribute to check that the return value is
IS_ERR()-checked?
(not volunteering, I have no idea how to add a sparse attribute :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message

[Boris Brezillon]

On Thu, 5 Oct 2017 09:48:24 +0100
Eric Engestrom <eric.engestrom@imgtec.com> wrote:

> On Wednesday, 2017-10-04 22:28:54 +0000, Eric Anholt wrote:
> > Boris Brezillon <boris.brezillon@free-electrons.com> writes:
> >   
> > > drm_gem_cma_create() prints an error message when dma_alloc_wc() fails to
> > > allocate the amount of memory we requested. This can lead to annoying
> > > error messages when CMA is only one possible source of memory for the BO
> > > allocation.
> > >
> > > Turn this error message into a debug one and add a __must_check specifier
> > > to make sure all callers are checking the return value.
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>  
> > 
> > The __must_check seems unnecessary to me -- you're definitely going to
> > be doing something with the return value, because otherwise why did you
> > call the object allocate function?  
> 
> Indeed, `__must_check` (aka `warn_unused_result`) only makes sure the
> return value is not discarded, which will probably always be true here.

I agree, this __must_check is not really useful here.

> 
> > The `warn_unused_result` attribute causes a warning to be emitted if
> > a caller of the function with this attribute does not use its return
> > value.  
> 
> I think we need a sparse attribute to check that the return value is
> IS_ERR()-checked?

Can we just turn the dev_err() into dev_dbg() for now? The 'force
callers to check IS_ERR()' is kind of orthogonal to the change I'm
doing here.

> (not volunteering, I have no idea how to add a sparse attribute :)

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message

[Eric Engestrom]

On Thursday, 2017-10-05 10:57:47 +0200, Boris Brezillon wrote:
> On Thu, 5 Oct 2017 09:48:24 +0100
> Eric Engestrom <eric.engestrom@imgtec.com> wrote:
> 
> > On Wednesday, 2017-10-04 22:28:54 +0000, Eric Anholt wrote:
> > > Boris Brezillon <boris.brezillon@free-electrons.com> writes:
> > >   
> > > > drm_gem_cma_create() prints an error message when dma_alloc_wc() fails to
> > > > allocate the amount of memory we requested. This can lead to annoying
> > > > error messages when CMA is only one possible source of memory for the BO
> > > > allocation.
> > > >
> > > > Turn this error message into a debug one and add a __must_check specifier
> > > > to make sure all callers are checking the return value.
> > > >
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>  
> > > 
> > > The __must_check seems unnecessary to me -- you're definitely going to
> > > be doing something with the return value, because otherwise why did you
> > > call the object allocate function?  
> > 
> > Indeed, `__must_check` (aka `warn_unused_result`) only makes sure the
> > return value is not discarded, which will probably always be true here.
> 
> I agree, this __must_check is not really useful here.
> 
> > 
> > > The `warn_unused_result` attribute causes a warning to be emitted if
> > > a caller of the function with this attribute does not use its return
> > > value.  
> > 
> > I think we need a sparse attribute to check that the return value is
> > IS_ERR()-checked?
> 
> Can we just turn the dev_err() into dev_dbg() for now? The 'force
> callers to check IS_ERR()' is kind of orthogonal to the change I'm
> doing here.

Definitely, the function you're touching just happens to be a user of
this potential new attribute.

IMO just drop the header part of the change, and this is
Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>

On the sparse attribute discussion, anyone knows someone with experience
doing that?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel