From: Michel Dänzer <[email protected]>
The following cases are possible for pr_debug():
1. CONFIG_DYNAMIC_DEBUG disabled
a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
it never generates any output.
b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
i.e. it generates output which doesn't appear in dmesg by default,
can be enabled dynamically.
2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
dynamic_pr_debug()
a) DEBUG not defined: dynamic_pr_debug() generates no output by
default, can be enabled dynamically.
b) DEBUG defined: dynamic_pr_debug() generates output by default,
can be disabled dynamically.
The intention for drm_debug_printer() is to generate output which
doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
Signed-off-by: Michel Dänzer <[email protected]>
---
drivers/gpu/drm/drm_print.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 0e7fc3e7dfb4..ee56e4a1b343 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -23,11 +23,13 @@
* Rob Clark <[email protected]>
*/
-#define DEBUG /* for pr_debug() */
-
#include <stdarg.h>
#include <linux/seq_file.h>
#include <drm/drmP.h>
+
+#ifndef CONFIG_DYNAMIC_DEBUG
+#define DEBUG /* for pr_debug() */
+#endif
#include <drm/drm_print.h>
void __drm_puts_coredump(struct drm_printer *p, const char *str)
--
2.20.0.rc2
From: Michel Dänzer <[email protected]>
All the output is related, so it should all be printed the same way.
Some of it was using pr_debug, but some of it appeared in dmesg by
default. The caller should handle failure, so there's no need to spam
dmesg with potentially quite a lot of output by default.
Signed-off-by: Michel Dänzer <[email protected]>
---
drivers/gpu/drm/ttm/ttm_bo.c | 39 ++++++++++++++++++------------------
1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index d87935bf8e30..5e9b9dd91629 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -77,38 +77,39 @@ static inline int ttm_mem_type_from_place(const struct ttm_place *place,
return 0;
}
-static void ttm_mem_type_debug(struct ttm_bo_device *bdev, int mem_type)
+static void ttm_mem_type_debug(struct ttm_bo_device *bdev, struct drm_printer *p,
+ int mem_type)
{
struct ttm_mem_type_manager *man = &bdev->man[mem_type];
- struct drm_printer p = drm_debug_printer(TTM_PFX);
- pr_err(" has_type: %d\n", man->has_type);
- pr_err(" use_type: %d\n", man->use_type);
- pr_err(" flags: 0x%08X\n", man->flags);
- pr_err(" gpu_offset: 0x%08llX\n", man->gpu_offset);
- pr_err(" size: %llu\n", man->size);
- pr_err(" available_caching: 0x%08X\n", man->available_caching);
- pr_err(" default_caching: 0x%08X\n", man->default_caching);
+ drm_printf(p, " has_type: %d\n", man->has_type);
+ drm_printf(p, " use_type: %d\n", man->use_type);
+ drm_printf(p, " flags: 0x%08X\n", man->flags);
+ drm_printf(p, " gpu_offset: 0x%08llX\n", man->gpu_offset);
+ drm_printf(p, " size: %llu\n", man->size);
+ drm_printf(p, " available_caching: 0x%08X\n", man->available_caching);
+ drm_printf(p, " default_caching: 0x%08X\n", man->default_caching);
if (mem_type != TTM_PL_SYSTEM)
- (*man->func->debug)(man, &p);
+ (*man->func->debug)(man, p);
}
static void ttm_bo_mem_space_debug(struct ttm_buffer_object *bo,
struct ttm_placement *placement)
{
+ struct drm_printer p = drm_debug_printer(TTM_PFX);
int i, ret, mem_type;
- pr_err("No space for %p (%lu pages, %luK, %luM)\n",
- bo, bo->mem.num_pages, bo->mem.size >> 10,
- bo->mem.size >> 20);
+ drm_printf(&p, "No space for %p (%lu pages, %luK, %luM)\n",
+ bo, bo->mem.num_pages, bo->mem.size >> 10,
+ bo->mem.size >> 20);
for (i = 0; i < placement->num_placement; i++) {
ret = ttm_mem_type_from_place(&placement->placement[i],
&mem_type);
if (ret)
return;
- pr_err(" placement[%d]=0x%08X (%d)\n",
- i, placement->placement[i].flags, mem_type);
- ttm_mem_type_debug(bo->bdev, mem_type);
+ drm_printf(&p, " placement[%d]=0x%08X (%d)\n",
+ i, placement->placement[i].flags, mem_type);
+ ttm_mem_type_debug(bo->bdev, &p, mem_type);
}
}
@@ -728,8 +729,8 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
ret = ttm_bo_mem_space(bo, &placement, &evict_mem, ctx);
if (ret) {
if (ret != -ERESTARTSYS) {
- pr_err("Failed to find memory space for buffer 0x%p eviction\n",
- bo);
+ pr_debug("Failed to find memory space for buffer 0x%p eviction\n",
+ bo);
ttm_bo_mem_space_debug(bo, &placement);
}
goto out;
@@ -738,7 +739,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx);
if (unlikely(ret)) {
if (ret != -ERESTARTSYS)
- pr_err("Buffer eviction failed\n");
+ pr_debug("Buffer eviction failed\n");
ttm_bo_mem_put(bo, &evict_mem);
goto out;
}
--
2.20.0.rc2
On 12/6/18 12:56 AM, Michel Dänzer wrote:
> From: Michel Dänzer <[email protected]>
>
> The following cases are possible for pr_debug():
>
> 1. CONFIG_DYNAMIC_DEBUG disabled
> a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
> it never generates any output.
> b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
> i.e. it generates output which doesn't appear in dmesg by default,
> can be enabled dynamically.
>
> 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
> dynamic_pr_debug()
> a) DEBUG not defined: dynamic_pr_debug() generates no output by
> default, can be enabled dynamically.
> b) DEBUG defined: dynamic_pr_debug() generates output by default,
> can be disabled dynamically.
>
> The intention for drm_debug_printer() is to generate output which
> doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
> cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
> instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
>
> Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
> Signed-off-by: Michel Dänzer <[email protected]>
Reviewed-by: Junwei Zhang <[email protected]>
> ---
> drivers/gpu/drm/drm_print.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index 0e7fc3e7dfb4..ee56e4a1b343 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -23,11 +23,13 @@
> * Rob Clark <[email protected]>
> */
>
> -#define DEBUG /* for pr_debug() */
> -
> #include <stdarg.h>
> #include <linux/seq_file.h>
> #include <drm/drmP.h>
> +
> +#ifndef CONFIG_DYNAMIC_DEBUG
> +#define DEBUG /* for pr_debug() */
> +#endif
> #include <drm/drm_print.h>
>
> void __drm_puts_coredump(struct drm_printer *p, const char *str)
On 12/6/18 12:56 AM, Michel Dänzer wrote:
> From: Michel Dänzer <[email protected]>
>
> All the output is related, so it should all be printed the same way.
> Some of it was using pr_debug, but some of it appeared in dmesg by
> default. The caller should handle failure, so there's no need to spam
> dmesg with potentially quite a lot of output by default.
>
> Signed-off-by: Michel Dänzer <[email protected]>
Sounds reasonable, but personally prefer to show error when some
vital incident happens, e.g. no memory on eviction.
Regards,
Jerry
> ---
> drivers/gpu/drm/ttm/ttm_bo.c | 39 ++++++++++++++++++------------------
> 1 file changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index d87935bf8e30..5e9b9dd91629 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -77,38 +77,39 @@ static inline int ttm_mem_type_from_place(const struct ttm_place *place,
> return 0;
> }
>
> -static void ttm_mem_type_debug(struct ttm_bo_device *bdev, int mem_type)
> +static void ttm_mem_type_debug(struct ttm_bo_device *bdev, struct drm_printer *p,
> + int mem_type)
> {
> struct ttm_mem_type_manager *man = &bdev->man[mem_type];
> - struct drm_printer p = drm_debug_printer(TTM_PFX);
>
> - pr_err(" has_type: %d\n", man->has_type);
> - pr_err(" use_type: %d\n", man->use_type);
> - pr_err(" flags: 0x%08X\n", man->flags);
> - pr_err(" gpu_offset: 0x%08llX\n", man->gpu_offset);
> - pr_err(" size: %llu\n", man->size);
> - pr_err(" available_caching: 0x%08X\n", man->available_caching);
> - pr_err(" default_caching: 0x%08X\n", man->default_caching);
> + drm_printf(p, " has_type: %d\n", man->has_type);
> + drm_printf(p, " use_type: %d\n", man->use_type);
> + drm_printf(p, " flags: 0x%08X\n", man->flags);
> + drm_printf(p, " gpu_offset: 0x%08llX\n", man->gpu_offset);
> + drm_printf(p, " size: %llu\n", man->size);
> + drm_printf(p, " available_caching: 0x%08X\n", man->available_caching);
> + drm_printf(p, " default_caching: 0x%08X\n", man->default_caching);
> if (mem_type != TTM_PL_SYSTEM)
> - (*man->func->debug)(man, &p);
> + (*man->func->debug)(man, p);
> }
>
> static void ttm_bo_mem_space_debug(struct ttm_buffer_object *bo,
> struct ttm_placement *placement)
> {
> + struct drm_printer p = drm_debug_printer(TTM_PFX);
> int i, ret, mem_type;
>
> - pr_err("No space for %p (%lu pages, %luK, %luM)\n",
> - bo, bo->mem.num_pages, bo->mem.size >> 10,
> - bo->mem.size >> 20);
> + drm_printf(&p, "No space for %p (%lu pages, %luK, %luM)\n",
> + bo, bo->mem.num_pages, bo->mem.size >> 10,
> + bo->mem.size >> 20);
> for (i = 0; i < placement->num_placement; i++) {
> ret = ttm_mem_type_from_place(&placement->placement[i],
> &mem_type);
> if (ret)
> return;
> - pr_err(" placement[%d]=0x%08X (%d)\n",
> - i, placement->placement[i].flags, mem_type);
> - ttm_mem_type_debug(bo->bdev, mem_type);
> + drm_printf(&p, " placement[%d]=0x%08X (%d)\n",
> + i, placement->placement[i].flags, mem_type);
> + ttm_mem_type_debug(bo->bdev, &p, mem_type);
> }
> }
>
> @@ -728,8 +729,8 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
> ret = ttm_bo_mem_space(bo, &placement, &evict_mem, ctx);
> if (ret) {
> if (ret != -ERESTARTSYS) {
> - pr_err("Failed to find memory space for buffer 0x%p eviction\n",
> - bo);
> + pr_debug("Failed to find memory space for buffer 0x%p eviction\n",
> + bo);
> ttm_bo_mem_space_debug(bo, &placement);
> }
> goto out;
> @@ -738,7 +739,7 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
> ret = ttm_bo_handle_move_mem(bo, &evict_mem, true, ctx);
> if (unlikely(ret)) {
> if (ret != -ERESTARTSYS)
> - pr_err("Buffer eviction failed\n");
> + pr_debug("Buffer eviction failed\n");
> ttm_bo_mem_put(bo, &evict_mem);
> goto out;
> }
On Thu, 2018-12-06 at 10:40 +0800, Zhang, Jerry(Junwei) wrote:
> On 12/6/18 12:56 AM, Michel D?nzer wrote:
> > From: Michel D?nzer <[email protected]>
> >
> > The following cases are possible for pr_debug():
> >
> > 1. CONFIG_DYNAMIC_DEBUG disabled
> > a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
> > it never generates any output.
> > b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
> > i.e. it generates output which doesn't appear in dmesg by default,
> > can be enabled dynamically.
> >
> > 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
> > dynamic_pr_debug()
> > a) DEBUG not defined: dynamic_pr_debug() generates no output by
> > default, can be enabled dynamically.
> > b) DEBUG defined: dynamic_pr_debug() generates output by default,
> > can be disabled dynamically.
> >
> > The intention for drm_debug_printer() is to generate output which
> > doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
> > cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
> > instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
> >
> > Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
I very much doubt this is a fix.
Did you read the commit log for this commit?
It says "make sure it will always produce output"
And why didn't you cc Chris Wilson, the author of that patch?
> > Signed-off-by: Michel D?nzer <[email protected]>
> Reviewed-by: Junwei Zhang <[email protected]>
>
> > ---
> > drivers/gpu/drm/drm_print.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> > index 0e7fc3e7dfb4..ee56e4a1b343 100644
> > --- a/drivers/gpu/drm/drm_print.c
> > +++ b/drivers/gpu/drm/drm_print.c
> > @@ -23,11 +23,13 @@
> > * Rob Clark <[email protected]>
> > */
> >
> > -#define DEBUG /* for pr_debug() */
> > -
> > #include <stdarg.h>
> > #include <linux/seq_file.h>
> > #include <drm/drmP.h>
> > +
> > +#ifndef CONFIG_DYNAMIC_DEBUG
> > +#define DEBUG /* for pr_debug() */
> > +#endif
> > #include <drm/drm_print.h>
> >
> > void __drm_puts_coredump(struct drm_printer *p, const char *str)
On 2018-12-06 3:43 a.m., Zhang, Jerry(Junwei) wrote:
> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>> From: Michel Dänzer <[email protected]>
>>
>> All the output is related, so it should all be printed the same way.
>> Some of it was using pr_debug, but some of it appeared in dmesg by
>> default. The caller should handle failure, so there's no need to spam
>> dmesg with potentially quite a lot of output by default.
>>
>> Signed-off-by: Michel Dänzer <[email protected]>
> Sounds reasonable, but personally prefer to show error when some
> vital incident happens, e.g. no memory on eviction.
The amdgpu driver still prints these in that case:
[drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* amdgpu_cs_list_validate(validated) failed.
[drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* Not enough memory for command submission!
That's plenty as far as I'm concerned. :)
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
Quoting Zhang, Jerry(Junwei) (2018-12-06 02:40:42)
> On 12/6/18 12:56 AM, Michel Dänzer wrote:
> > From: Michel Dänzer <[email protected]>
> >
> > The following cases are possible for pr_debug():
> >
> > 1. CONFIG_DYNAMIC_DEBUG disabled
> > a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
> > it never generates any output.
> > b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
> > i.e. it generates output which doesn't appear in dmesg by default,
> > can be enabled dynamically.
> >
> > 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
> > dynamic_pr_debug()
> > a) DEBUG not defined: dynamic_pr_debug() generates no output by
> > default, can be enabled dynamically.
> > b) DEBUG defined: dynamic_pr_debug() generates output by default,
> > can be disabled dynamically.
> >
> > The intention for drm_debug_printer() is to generate output which
> > doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
> > cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
> > instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
> >
> > Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
> > Signed-off-by: Michel Dänzer <[email protected]>
> Reviewed-by: Junwei Zhang <[email protected]>
At the cost of 1a? Nah.
-Chris
On 2018-12-06 10:12 a.m., Chris Wilson wrote:
> Quoting Zhang, Jerry(Junwei) (2018-12-06 02:40:42)
>> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>>> From: Michel Dänzer <[email protected]>
>>>
>>> The following cases are possible for pr_debug():
>>>
>>> 1. CONFIG_DYNAMIC_DEBUG disabled
>>> a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
>>> it never generates any output.
>>> b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
>>> i.e. it generates output which doesn't appear in dmesg by default,
>>> can be enabled dynamically.
>>>
>>> 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
>>> dynamic_pr_debug()
>>> a) DEBUG not defined: dynamic_pr_debug() generates no output by
>>> default, can be enabled dynamically.
>>> b) DEBUG defined: dynamic_pr_debug() generates output by default,
>>> can be disabled dynamically.
>>>
>>> The intention for drm_debug_printer() is to generate output which
>>> doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
>>> cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
>>> instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
>>>
>>> Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
>>> Signed-off-by: Michel Dänzer <[email protected]>
>> Reviewed-by: Junwei Zhang <[email protected]>
>
> At the cost of 1a? Nah.
We still #define DEBUG if CONFIG_DYNAMIC_DEBUG isn't defined, so it's
still 1b), not 1a).
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On 2018-12-06 3:51 a.m., Joe Perches wrote:
> On Thu, 2018-12-06 at 10:40 +0800, Zhang, Jerry(Junwei) wrote:
>> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>>> From: Michel Dänzer <[email protected]>
>>>
>>> The following cases are possible for pr_debug():
>>>
>>> 1. CONFIG_DYNAMIC_DEBUG disabled
>>> a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
>>> it never generates any output.
>>> b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
>>> i.e. it generates output which doesn't appear in dmesg by default,
>>> can be enabled dynamically.
>>>
>>> 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
>>> dynamic_pr_debug()
>>> a) DEBUG not defined: dynamic_pr_debug() generates no output by
>>> default, can be enabled dynamically.
>>> b) DEBUG defined: dynamic_pr_debug() generates output by default,
>>> can be disabled dynamically.
>>>
>>> The intention for drm_debug_printer() is to generate output which
>>> doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
>>> cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
>>> instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
>>>
>>> Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
>
> I very much doubt this is a fix.
>
> Did you read the commit log for this commit?
>
> It says "make sure it will always produce output"
I thought the commit log covered this, suggestions for improvement welcome.
Chris' change addressed case 1a), but also took us from 2a) to 2b). But
we want 2a).
I suspect Chris missed that pr_debug()'s output is visible by default if
CONFIG_DYNAMIC_DEBUG and DEBUG are both defined.
> And why didn't you cc Chris Wilson, the author of that patch?
I used the get_maintainer.pl script. Thanks for adding Chris.
P.S. FYI, your e-mail had a very aggressive tone to me, not sure what for.
>>> Signed-off-by: Michel Dänzer <[email protected]>
>> Reviewed-by: Junwei Zhang <[email protected]>
>>
>>> ---
>>> drivers/gpu/drm/drm_print.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
>>> index 0e7fc3e7dfb4..ee56e4a1b343 100644
>>> --- a/drivers/gpu/drm/drm_print.c
>>> +++ b/drivers/gpu/drm/drm_print.c
>>> @@ -23,11 +23,13 @@
>>> * Rob Clark <[email protected]>
>>> */
>>>
>>> -#define DEBUG /* for pr_debug() */
>>> -
>>> #include <stdarg.h>
>>> #include <linux/seq_file.h>
>>> #include <drm/drmP.h>
>>> +
>>> +#ifndef CONFIG_DYNAMIC_DEBUG
>>> +#define DEBUG /* for pr_debug() */
>>> +#endif
>>> #include <drm/drm_print.h>
>>>
>>> void __drm_puts_coredump(struct drm_printer *p, const char *str)
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
Quoting Michel Dänzer (2018-12-06 09:21:40)
> On 2018-12-06 10:12 a.m., Chris Wilson wrote:
> > Quoting Zhang, Jerry(Junwei) (2018-12-06 02:40:42)
> >> On 12/6/18 12:56 AM, Michel Dänzer wrote:
> >>> From: Michel Dänzer <[email protected]>
> >>>
> >>> The following cases are possible for pr_debug():
> >>>
> >>> 1. CONFIG_DYNAMIC_DEBUG disabled
> >>> a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
> >>> it never generates any output.
> >>> b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
> >>> i.e. it generates output which doesn't appear in dmesg by default,
> >>> can be enabled dynamically.
> >>>
> >>> 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
> >>> dynamic_pr_debug()
> >>> a) DEBUG not defined: dynamic_pr_debug() generates no output by
> >>> default, can be enabled dynamically.
> >>> b) DEBUG defined: dynamic_pr_debug() generates output by default,
> >>> can be disabled dynamically.
> >>>
> >>> The intention for drm_debug_printer() is to generate output which
> >>> doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
> >>> cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
> >>> instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
> >>>
> >>> Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
> >>> Signed-off-by: Michel Dänzer <[email protected]>
> >> Reviewed-by: Junwei Zhang <[email protected]>
> >
> > At the cost of 1a? Nah.
>
> We still #define DEBUG if CONFIG_DYNAMIC_DEBUG isn't defined, so it's
> still 1b), not 1a).
I completely fluffed my reading of ifndef.
#if !IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
-Chris
Am 06.12.18 um 10:09 schrieb Michel Dänzer:
> On 2018-12-06 3:43 a.m., Zhang, Jerry(Junwei) wrote:
>> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>>> From: Michel Dänzer <[email protected]>
>>>
>>> All the output is related, so it should all be printed the same way.
>>> Some of it was using pr_debug, but some of it appeared in dmesg by
>>> default. The caller should handle failure, so there's no need to spam
>>> dmesg with potentially quite a lot of output by default.
>>>
>>> Signed-off-by: Michel Dänzer <[email protected]>
>> Sounds reasonable, but personally prefer to show error when some
>> vital incident happens, e.g. no memory on eviction.
> The amdgpu driver still prints these in that case:
>
> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* amdgpu_cs_list_validate(validated) failed.
> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* Not enough memory for command submission!
>
> That's plenty as far as I'm concerned. :)
Yeah, but in this case I would rather make the amdgpu messages debug
level and leave the TTM meassages on error level.
Christian.
On 2018-12-06 10:33 a.m., Koenig, Christian wrote:
> Am 06.12.18 um 10:09 schrieb Michel Dänzer:
>> On 2018-12-06 3:43 a.m., Zhang, Jerry(Junwei) wrote:
>>> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>>>> From: Michel Dänzer <[email protected]>
>>>>
>>>> All the output is related, so it should all be printed the same way.
>>>> Some of it was using pr_debug, but some of it appeared in dmesg by
>>>> default. The caller should handle failure, so there's no need to spam
>>>> dmesg with potentially quite a lot of output by default.
>>>>
>>>> Signed-off-by: Michel Dänzer <[email protected]>
>>> Sounds reasonable, but personally prefer to show error when some
>>> vital incident happens, e.g. no memory on eviction.
>> The amdgpu driver still prints these in that case:
>>
>> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* amdgpu_cs_list_validate(validated) failed.
>> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* Not enough memory for command submission!
>>
>> That's plenty as far as I'm concerned. :)
>
> Yeah, but in this case I would rather make the amdgpu messages debug
> level and leave the TTM meassages on error level.
That makes no sense to me.
The amdgpu messages have some value for normal users / bug reports,
indicating that something isn't going quite as planned.
The TTM messages are orders of magnitude longer, and are basically noise
for a normal user.
Seems like a no-brainer to me which of these should be visible by default.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On 12/6/18 5:33 PM, Koenig, Christian wrote:
> Am 06.12.18 um 10:09 schrieb Michel Dänzer:
>> On 2018-12-06 3:43 a.m., Zhang, Jerry(Junwei) wrote:
>>> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>>>> From: Michel Dänzer <[email protected]>
>>>>
>>>> All the output is related, so it should all be printed the same way.
>>>> Some of it was using pr_debug, but some of it appeared in dmesg by
>>>> default. The caller should handle failure, so there's no need to spam
>>>> dmesg with potentially quite a lot of output by default.
>>>>
>>>> Signed-off-by: Michel Dänzer <[email protected]>
>>> Sounds reasonable, but personally prefer to show error when some
>>> vital incident happens, e.g. no memory on eviction.
>> The amdgpu driver still prints these in that case:
>>
>> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* amdgpu_cs_list_validate(validated) failed.
>> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* Not enough memory for command submission!
That's from cs submit, perhaps it may come from other places by
ttm_bo_evict_mm().
Is that right? Christian.
Regards,
Jerry
>>
>> That's plenty as far as I'm concerned. :)
> Yeah, but in this case I would rather make the amdgpu messages debug
> level and leave the TTM meassages on error level.
>
> Christian.
Am 06.12.18 um 10:39 schrieb Zhang, Jerry(Junwei):
> On 12/6/18 5:33 PM, Koenig, Christian wrote:
>> Am 06.12.18 um 10:09 schrieb Michel Dänzer:
>>> On 2018-12-06 3:43 a.m., Zhang, Jerry(Junwei) wrote:
>>>> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>>>>> From: Michel Dänzer <[email protected]>
>>>>>
>>>>> All the output is related, so it should all be printed the same way.
>>>>> Some of it was using pr_debug, but some of it appeared in dmesg by
>>>>> default. The caller should handle failure, so there's no need to spam
>>>>> dmesg with potentially quite a lot of output by default.
>>>>>
>>>>> Signed-off-by: Michel Dänzer <[email protected]>
>>>> Sounds reasonable, but personally prefer to show error when some
>>>> vital incident happens, e.g. no memory on eviction.
>>> The amdgpu driver still prints these in that case:
>>>
>>> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR*
>>> amdgpu_cs_list_validate(validated) failed.
>>> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* Not enough memory for
>>> command submission!
>
> That's from cs submit, perhaps it may come from other places by
> ttm_bo_evict_mm().
> Is that right? Christian.
Yeah, exactly my thinking as well. When we silence the TTM messages we
might miss those cases.
Additional to that other drivers using TTM might not have those messages
either.
If TTM is to noisy we should use ratelimit and/or reduce the number and
size of the warning messages.
A simple "Warning, I ran out of memory during eviction!" should do.
Regards,
Christian.
>
> Regards,
> Jerry
>>>
>>> That's plenty as far as I'm concerned. :)
>> Yeah, but in this case I would rather make the amdgpu messages debug
>> level and leave the TTM meassages on error level.
>>
>> Christian.
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2018-12-06 10:38 a.m., Michel Dänzer wrote:
> On 2018-12-06 10:33 a.m., Koenig, Christian wrote:
>> Am 06.12.18 um 10:09 schrieb Michel Dänzer:
>>> On 2018-12-06 3:43 a.m., Zhang, Jerry(Junwei) wrote:
>>>> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>>>>> From: Michel Dänzer <[email protected]>
>>>>>
>>>>> All the output is related, so it should all be printed the same way.
>>>>> Some of it was using pr_debug, but some of it appeared in dmesg by
>>>>> default. The caller should handle failure, so there's no need to spam
>>>>> dmesg with potentially quite a lot of output by default.
>>>>>
>>>>> Signed-off-by: Michel Dänzer <[email protected]>
>>>> Sounds reasonable, but personally prefer to show error when some
>>>> vital incident happens, e.g. no memory on eviction.
>>> The amdgpu driver still prints these in that case:
>>>
>>> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* amdgpu_cs_list_validate(validated) failed.
>>> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* Not enough memory for command submission!
>>>
>>> That's plenty as far as I'm concerned. :)
>>
>> Yeah, but in this case I would rather make the amdgpu messages debug
>> level and leave the TTM meassages on error level.
>
> That makes no sense to me.
>
> The amdgpu messages have some value for normal users / bug reports,
> indicating that something isn't going quite as planned.
>
> The TTM messages are orders of magnitude longer, and are basically noise
> for a normal user.
>
> Seems like a no-brainer to me which of these should be visible by default.
Moreover, not every case producing the driver output also produces the
TTM output, so it could make it difficult to realize that there's a
memory pressure situation.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On 2018-12-06 10:49 a.m., Christian König wrote:
> Am 06.12.18 um 10:39 schrieb Zhang, Jerry(Junwei):
>> On 12/6/18 5:33 PM, Koenig, Christian wrote:
>>> Am 06.12.18 um 10:09 schrieb Michel Dänzer:
>>>> On 2018-12-06 3:43 a.m., Zhang, Jerry(Junwei) wrote:
>>>>> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>>>>>> From: Michel Dänzer <[email protected]>
>>>>>>
>>>>>> All the output is related, so it should all be printed the same way.
>>>>>> Some of it was using pr_debug, but some of it appeared in dmesg by
>>>>>> default. The caller should handle failure, so there's no need to spam
>>>>>> dmesg with potentially quite a lot of output by default.
>>>>>>
>>>>>> Signed-off-by: Michel Dänzer <[email protected]>
>>>>> Sounds reasonable, but personally prefer to show error when some
>>>>> vital incident happens, e.g. no memory on eviction.
>>>> The amdgpu driver still prints these in that case:
>>>>
>>>> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR*
>>>> amdgpu_cs_list_validate(validated) failed.
>>>> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* Not enough memory for
>>>> command submission!
>>
>> That's from cs submit, perhaps it may come from other places by
>> ttm_bo_evict_mm().
>> Is that right? Christian.
>
> Yeah, exactly my thinking as well. When we silence the TTM messages we
> might miss those cases.
>
> Additional to that other drivers using TTM might not have those messages
> either.
>
> If TTM is to noisy we should use ratelimit and/or reduce the number and
> size of the warning messages.
>
> A simple "Warning, I ran out of memory during eviction!" should do.
Just dropping the last hunk of this patch should do then. I can do that.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On Thu, 2018-12-06 at 10:23 +0100, Michel D?nzer wrote:
> On 2018-12-06 3:51 a.m., Joe Perches wrote:
> > On Thu, 2018-12-06 at 10:40 +0800, Zhang, Jerry(Junwei) wrote:
> > > On 12/6/18 12:56 AM, Michel D?nzer wrote:
> > > > From: Michel D?nzer <[email protected]>
> > > >
> > > > The following cases are possible for pr_debug():
> > > >
> > > > 1. CONFIG_DYNAMIC_DEBUG disabled
> > > > a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
> > > > it never generates any output.
> > > > b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
> > > > i.e. it generates output which doesn't appear in dmesg by default,
> > > > can be enabled dynamically.
> > > >
> > > > 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
> > > > dynamic_pr_debug()
> > > > a) DEBUG not defined: dynamic_pr_debug() generates no output by
> > > > default, can be enabled dynamically.
> > > > b) DEBUG defined: dynamic_pr_debug() generates output by default,
> > > > can be disabled dynamically.
> > > >
> > > > The intention for drm_debug_printer() is to generate output which
> > > > doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
> > > > cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
> > > > instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
> > > >
> > > > Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
> >
> > I very much doubt this is a fix.
> >
> > Did you read the commit log for this commit?
> >
> > It says "make sure it will always produce output"
>
> I thought the commit log covered this, suggestions for improvement welcome.
>
> Chris' change addressed case 1a), but also took us from 2a) to 2b). But
> we want 2a)
> I suspect Chris missed that pr_debug()'s output is visible by default if
> CONFIG_DYNAMIC_DEBUG and DEBUG are both defined.
I believe you and Chris have different goals and that your
point of 2b is
misguided.
I think the language used in the Chris' commit log is
plain ans obvious. 'always produce output'.
> > And why didn't you cc Chris Wilson, the author of that patch?k
>
> I used the get_maintainer.pl script. Thanks for adding Chris.
> P.S. FYI, your e-mail had a very aggressive tone to me, not sure what for.
No worries.
I think the language I used is also plain and obvious
and you are overreading.
cheers, Joe
On 2018-12-06 12:41 p.m., Joe Perches wrote:
> On Thu, 2018-12-06 at 10:23 +0100, Michel Dänzer wrote:
>> On 2018-12-06 3:51 a.m., Joe Perches wrote:
>>> On Thu, 2018-12-06 at 10:40 +0800, Zhang, Jerry(Junwei) wrote:
>>>> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>>>>> From: Michel Dänzer <[email protected]>
>>>>>
>>>>> The following cases are possible for pr_debug():
>>>>>
>>>>> 1. CONFIG_DYNAMIC_DEBUG disabled
>>>>> a) DEBUG not defined: pr_debug() translates to no_printk(...), i.e.
>>>>> it never generates any output.
>>>>> b) DEBUG defined: pr_debug() translates to printk(KERN_DEBUG ...),
>>>>> i.e. it generates output which doesn't appear in dmesg by default,
>>>>> can be enabled dynamically.
>>>>>
>>>>> 2. CONFIG_DYNAMIC_DEBUG enabled: pr_debug() translates to
>>>>> dynamic_pr_debug()
>>>>> a) DEBUG not defined: dynamic_pr_debug() generates no output by
>>>>> default, can be enabled dynamically.
>>>>> b) DEBUG defined: dynamic_pr_debug() generates output by default,
>>>>> can be disabled dynamically.
>>>>>
>>>>> The intention for drm_debug_printer() is to generate output which
>>>>> doesn't appear in dmesg by default, but can be enabled dynamically, i.e.
>>>>> cases 1b) and 2a). However, defining DEBUG unconditionally gave us 2b)
>>>>> instead of 2a) with CONFIG_DYNAMIC_DEBUG enabled.
>>>>>
>>>>> Fixes: 79a5ad2fdb3c ("drm: Enable pr_debug() for drm_printer")
>>>
>>> I very much doubt this is a fix.
>>>
>>> Did you read the commit log for this commit?
>>>
>>> It says "make sure it will always produce output"
>>
>> I thought the commit log covered this, suggestions for improvement welcome.
>>
>> Chris' change addressed case 1a), but also took us from 2a) to 2b). But
>> we want 2a)
>
>> I suspect Chris missed that pr_debug()'s output is visible by default if
>> CONFIG_DYNAMIC_DEBUG and DEBUG are both defined.
>
> I believe you and Chris have different goals and that your
> point of 2b is
> misguided.
>
> I think the language used in the Chris' commit log is
> plain ans obvious. 'always produce output'.
That was probably meant as "make sure there always can be output", which
isn't the case with 1a (no_printk suppresses the output at compile time,
the strings might not even exist in the binaries).
In contrast to the 2b case, the pr_debug output isn't visible by default
with 1b, so the latter doesn't fit "always produce output" either. But
Chris' initial follow-up in this thread seems to confirm my assumption
that his change was about 1a/b.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On Thu, 2018-12-06 at 12:52 +0100, Michel D?nzer wrote:
> In contrast to the 2b case, the pr_debug output isn't visible by default
> with 1b, so the latter doesn't fit "always produce output" either.
I think you are mistaken here.
Adding #define DEBUG as Chris did enables pr_debug output
and is your 1b.
Perhaps your default console logging level is set to a
non-default value.
CONSOLE_LOGLEVEL_DEFAULT
lib/Kconfig.debug:config CONSOLE_LOGLEVEL_DEFAULT
lib/Kconfig.debug- int "Default console loglevel (1-15)"
lib/Kconfig.debug- range 1 15
lib/Kconfig.debug- default "7"
On 2018-12-06 1:23 p.m., Joe Perches wrote:
> On Thu, 2018-12-06 at 12:52 +0100, Michel Dänzer wrote:
>> In contrast to the 2b case, the pr_debug output isn't visible by default
>> with 1b, so the latter doesn't fit "always produce output" either.
>
> I think you are mistaken here.
Still puzzled as to what you're hoping to achieve with that kind of
language. None of the confusion about this patch has been on my part. :)
> Adding #define DEBUG as Chris did enables pr_debug output
> and is your 1b.
>
> Perhaps your default console logging level is set to a
> non-default value.
I have CONFIG_DYNAMIC_DEBUG enabled in my kernels. The problem addressed
by this patch is that messages from drm_debug_printer are visible by
default (case 2b), whereas they shouldn't be (case 2a, like 1b).
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On Thu, Dec 06, 2018 at 03:41:16PM +0100, Michel D?nzer wrote:
> On 2018-12-06 1:23 p.m., Joe Perches wrote:
> > On Thu, 2018-12-06 at 12:52 +0100, Michel D?nzer wrote:
> >> In contrast to the 2b case, the pr_debug output isn't visible by default
> >> with 1b, so the latter doesn't fit "always produce output" either.
> >
> > I think you are mistaken here.
>
> Still puzzled as to what you're hoping to achieve with that kind of
> language. None of the confusion about this patch has been on my part. :)
>
>
> > Adding #define DEBUG as Chris did enables pr_debug output
> > and is your 1b.
> >
> > Perhaps your default console logging level is set to a
> > non-default value.
>
> I have CONFIG_DYNAMIC_DEBUG enabled in my kernels. The problem addressed
> by this patch is that messages from drm_debug_printer are visible by
> default (case 2b), whereas they shouldn't be (case 2a, like 1b).
When enabled (either dynamically or statically) pr_debug() will emit
output at KERN_DEBUG level regardless of whether CONFIG_DYNAMIC_DEBUG
is defined or not.
Thus unless you change additional settings (either dynamically or
statically) then debug messages should not be shown on the console
because the default settings filter KERN_DEBUG messages. However they
are available via dmesg and system loggers (syslogd, journal, etc).
The patch proposed will change the behaviour of the debug messages
w.r.t. system loggers based on whether the user has enabled
CONFIG_DYNAMIC_DEBUG or not, violating the principle of least surprise.
Daniel.
On 2018-12-06 5:10 p.m., Daniel Thompson wrote:
> On Thu, Dec 06, 2018 at 03:41:16PM +0100, Michel Dänzer wrote:
>> On 2018-12-06 1:23 p.m., Joe Perches wrote:
>>> On Thu, 2018-12-06 at 12:52 +0100, Michel Dänzer wrote:
>>>> In contrast to the 2b case, the pr_debug output isn't visible by default
>>>> with 1b, so the latter doesn't fit "always produce output" either.
>>>
>>> I think you are mistaken here.
>>
>> Still puzzled as to what you're hoping to achieve with that kind of
>> language. None of the confusion about this patch has been on my part. :)
>>
>>
>>> Adding #define DEBUG as Chris did enables pr_debug output
>>> and is your 1b.
>>>
>>> Perhaps your default console logging level is set to a
>>> non-default value.
>>
>> I have CONFIG_DYNAMIC_DEBUG enabled in my kernels. The problem addressed
>> by this patch is that messages from drm_debug_printer are visible by
>> default (case 2b), whereas they shouldn't be (case 2a, like 1b).
>
> When enabled (either dynamically or statically) pr_debug() will emit
> output at KERN_DEBUG level regardless of whether CONFIG_DYNAMIC_DEBUG
> is defined or not.
>
> Thus unless you change additional settings (either dynamically or
> statically) then debug messages should not be shown on the console
> because the default settings filter KERN_DEBUG messages. However they
> are available via dmesg and system loggers (syslogd, journal, etc).
>
> The patch proposed will change the behaviour of the debug messages
> w.r.t. system loggers based on whether the user has enabled
> CONFIG_DYNAMIC_DEBUG or not, violating the principle of least surprise.
Ah, that makes sense now, thanks.
I'm withdrawing this patch.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On Thu, 2018-12-06 at 15:41 +0100, Michel D?nzer wrote:
> On 2018-12-06 1:23 p.m., Joe Perches wrote:
> > On Thu, 2018-12-06 at 12:52 +0100, Michel D?nzer wrote:
> > > In contrast to the 2b case, the pr_debug output isn't visible by default
> > > with 1b, so the latter doesn't fit "always produce output" either.
> >
> > I think you are mistaken here.
>
> Still puzzled as to what you're hoping to achieve with that kind of
> language. None of the confusion about this patch has been on my part. :)
Doubtful.
> > Adding #define DEBUG as Chris did enables pr_debug output
> > and is your 1b.
> >
> > Perhaps your default console logging level is set to a
> > non-default value.
>
> I have CONFIG_DYNAMIC_DEBUG enabled in my kernels. The problem addressed
> by this patch is that messages from drm_debug_printer are visible by
> default (case 2b), whereas they shouldn't be (case 2a, like 1b).
The default for config_dynamic_debug when DEBUG is defined is
to output the message equivalent to when DEBUG is defined and
config_dynamic_debug is not.
That's a good thing and has been the general default since 2011.
You want to override that, I suppose that's OK but it's
definitely not a _fix_ as you have written.
It's just a change in behavior.
You added a "Fixes:" line. I believe that wrong.
A long time ago, at my suggestion:
commit b558c96ffa53f4b3dd52b774e4fb7a52982ab52b
Author: Jim Cromie <[email protected]>
Date: Mon Dec 19 17:11:18 2011 -0500
dynamic_debug: make dynamic-debug supersede DEBUG ccflag
If CONFIG_DYNAMIC_DEBUG is defined, honor it over DEBUG, so that
pr_debug()s are controllable, instead of always-on. When DEBUG is
also defined, change _DPRINTK_FLAGS_DEFAULT to enable printing by
default.
cheers, Joe
On Thu, 2018-12-06 at 17:39 +0800, Zhang, Jerry(Junwei) wrote:
> On 12/6/18 5:33 PM, Koenig, Christian wrote:
> > Am 06.12.18 um 10:09 schrieb Michel D?nzer:
> > > On 2018-12-06 3:43 a.m., Zhang, Jerry(Junwei) wrote:
> > > > On 12/6/18 12:56 AM, Michel D?nzer wrote:
> > > > > From: Michel D?nzer <[email protected]>
> > > > >
> > > > > All the output is related, so it should all be printed the same way.
> > > > > Some of it was using pr_debug, but some of it appeared in dmesg by
> > > > > default. The caller should handle failure, so there's no need to spam
> > > > > dmesg with potentially quite a lot of output by default.
> > > > >
> > > > > Signed-off-by: Michel D?nzer <[email protected]>
> > > > Sounds reasonable, but personally prefer to show error when some
> > > > vital incident happens, e.g. no memory on eviction.
> > > The amdgpu driver still prints these in that case:
> > >
> > > [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* amdgpu_cs_list_validate(validated) failed.
> > > [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* Not enough memory for command submission!
Aren't dump_stack()s already done on all these allocation failures?
I don't notice any use of __GFP_NOWARN on generic allocations in drm.
On 2018-12-06 5:46 p.m., Joe Perches wrote:
> On Thu, 2018-12-06 at 17:39 +0800, Zhang, Jerry(Junwei) wrote:
>> On 12/6/18 5:33 PM, Koenig, Christian wrote:
>>> Am 06.12.18 um 10:09 schrieb Michel Dänzer:
>>>> On 2018-12-06 3:43 a.m., Zhang, Jerry(Junwei) wrote:
>>>>> On 12/6/18 12:56 AM, Michel Dänzer wrote:
>>>>>> From: Michel Dänzer <[email protected]>
>>>>>>
>>>>>> All the output is related, so it should all be printed the same way.
>>>>>> Some of it was using pr_debug, but some of it appeared in dmesg by
>>>>>> default. The caller should handle failure, so there's no need to spam
>>>>>> dmesg with potentially quite a lot of output by default.
>>>>>>
>>>>>> Signed-off-by: Michel Dänzer <[email protected]>
>>>>> Sounds reasonable, but personally prefer to show error when some
>>>>> vital incident happens, e.g. no memory on eviction.
>>>> The amdgpu driver still prints these in that case:
>>>>
>>>> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* amdgpu_cs_list_validate(validated) failed.
>>>> [drm:amdgpu_cs_ioctl [amdgpu]] *ERROR* Not enough memory for command submission!
>
> Aren't dump_stack()s already done on all these allocation failures?
> I don't notice any use of __GFP_NOWARN on generic allocations in drm.
Most of the time, these messages are due to being unable to allocate a
TTM BO or move it where it needs to go, not due to the kernel failing to
allocate memory in general.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer