* [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message
@ 2017-10-04 13:08 Boris Brezillon
2017-10-04 16:10 ` Daniel Vetter
2017-10-04 22:28 ` Eric Anholt
0 siblings, 2 replies; 6+ messages in thread
From: Boris Brezillon @ 2017-10-04 13:08 UTC (permalink / raw)
To: David Airlie, Daniel Vetter, dri-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message
2017-10-04 13:08 [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message Boris Brezillon
@ 2017-10-04 16:10 ` Daniel Vetter
2017-10-04 22:28 ` Eric Anholt
1 sibling, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2017-10-04 16:10 UTC (permalink / raw)
To: Boris Brezillon; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message
2017-10-04 13:08 [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message Boris Brezillon
2017-10-04 16:10 ` Daniel Vetter
@ 2017-10-04 22:28 ` Eric Anholt
2017-10-05 8:48 ` Eric Engestrom
1 sibling, 1 reply; 6+ messages in thread
From: Eric Anholt @ 2017-10-04 22:28 UTC (permalink / raw)
To: David Airlie, Daniel Vetter, dri-devel; +Cc: Boris Brezillon
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message
2017-10-04 22:28 ` Eric Anholt
@ 2017-10-05 8:48 ` Eric Engestrom
2017-10-05 8:57 ` Boris Brezillon
0 siblings, 1 reply; 6+ messages in thread
From: Eric Engestrom @ 2017-10-05 8:48 UTC (permalink / raw)
To: Eric Anholt; +Cc: Boris Brezillon, dri-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message
2017-10-05 8:48 ` Eric Engestrom
@ 2017-10-05 8:57 ` Boris Brezillon
2017-10-05 9:13 ` Eric Engestrom
0 siblings, 1 reply; 6+ messages in thread
From: Boris Brezillon @ 2017-10-05 8:57 UTC (permalink / raw)
To: Eric Engestrom; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message
2017-10-05 8:57 ` Boris Brezillon
@ 2017-10-05 9:13 ` Eric Engestrom
0 siblings, 0 replies; 6+ messages in thread
From: Eric Engestrom @ 2017-10-05 9:13 UTC (permalink / raw)
To: Boris Brezillon; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-05 9:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-04 13:08 [PATCH] drm/gem-cma-helper: Change the level of the allocation failure message Boris Brezillon
2017-10-04 16:10 ` Daniel Vetter
2017-10-04 22:28 ` Eric Anholt
2017-10-05 8:48 ` Eric Engestrom
2017-10-05 8:57 ` Boris Brezillon
2017-10-05 9:13 ` Eric Engestrom
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.