[PATCH 1/2] drm: Only #define DEBUG if CONFIG_DYNAMIC_DEBUG is disabled

[Michel Dänzer]

From: Michel Dänzer <***@amd.com>

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 <***@amd.com>
---
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 <***@gmail.com>
*/

-#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)

[Michel Dänzer]

From: Michel Dänzer <***@amd.com>

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 <***@amd.com>
---
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;
}

[Zhang, Jerry(Junwei)]

Post by Michel Dänzer
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.

Sounds reasonable, but personally prefer to show error when some
vital incident happens, e.g. no memory on eviction.

Regards,
Jerry

Post by Michel Dänzer
---
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;
}

[Michel Dänzer]

Post by Zhang, Jerry(Junwei)

Post by Michel Dänzer
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.

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. :)

[Koenig, Christian]

Post by Michel Dänzer

Post by Zhang, Jerry(Junwei)

Post by Michel Dänzer
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.

Sounds reasonable, but personally prefer to show error when some
vital incident happens, e.g. no memory on eviction.

[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.

[Michel Dänzer]

Post by Koenig, Christian

Post by Michel Dänzer

Post by Zhang, Jerry(Junwei)

Post by Michel Dänzer
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.

Sounds reasonable, but personally prefer to show error when some
vital incident happens, e.g. no memory on eviction.

[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.

[Michel Dänzer]

Post by Michel Dänzer

Post by Koenig, Christian

Post by Michel Dänzer

Post by Zhang, Jerry(Junwei)

Post by Michel Dänzer
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.

Sounds reasonable, but personally prefer to show error when some
vital incident happens, e.g. no memory on eviction.

[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.

[Zhang, Jerry(Junwei)]

Post by Koenig, Christian

Post by Michel Dänzer

Post by Zhang, Jerry(Junwei)

Post by Michel Dänzer
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.

Sounds reasonable, but personally prefer to show error when some
vital incident happens, e.g. no memory on eviction.

[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

Post by Koenig, Christian

Post by Michel Dänzer
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.

[Christian König]

Post by Zhang, Jerry(Junwei)

Post by Koenig, Christian

Post by Zhang, Jerry(Junwei)

Post by Michel Dänzer
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.

Sounds reasonable, but personally prefer to show error when some
vital incident happens, e.g. no memory on eviction.

   [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.

Post by Zhang, Jerry(Junwei)
Regards,
Jerry

Post by Koenig, Christian

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
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Michel Dänzer]

Post by Christian König

Post by Zhang, Jerry(Junwei)

Post by Zhang, Jerry(Junwei)

Post by Michel Dänzer
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.

Sounds reasonable, but personally prefer to show error when some
vital incident happens, e.g. no memory on eviction.

   [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.

[Joe Perches]

Post by Michel Dänzer

Post by Zhang, Jerry(Junwei)

Post by Michel Dänzer
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.

Sounds reasonable, but personally prefer to show error when some
vital incident happens, e.g. no memory on eviction.

[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.

[Michel Dänzer]

Post by Joe Perches

Post by Michel Dänzer

Post by Zhang, Jerry(Junwei)

Post by Michel Dänzer
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.

Sounds reasonable, but personally prefer to show error when some
vital incident happens, e.g. no memory on eviction.

[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.

[Zhang, Jerry(Junwei)]

Post by Michel Dänzer
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")
---
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 @@
*/
-#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)

[Joe Perches]

Post by Michel Dänzer
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?

Post by Michel Dänzer
---
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 @@
*/
-#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)

[Michel Dänzer]

Post by Joe Perches

Post by Michel Dänzer
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.

Post by Joe Perches
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.

Post by Joe Perches

Post by Michel Dänzer
---
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 @@
*/
-#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
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Joe Perches]

Post by Michel Dänzer

Post by Joe Perches

Post by Michel Dänzer
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'.

Post by Michel Dänzer

Post by Joe Perches
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

[Michel Dänzer]

Post by Joe Perches

Post by Michel Dänzer

Post by Joe Perches

Post by Michel Dänzer
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.

[Joe Perches]

Post by Michel Dänzer
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"

[Michel Dänzer]

Post by Joe Perches

Post by Michel Dänzer
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. :)

Post by Joe Perches
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).

[Daniel Thompson]

Post by Michel Dänzer

Post by Joe Perches

Post by Michel Dänzer
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. :)

Post by Joe Perches
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.

[Michel Dänzer]

Post by Daniel Thompson

Post by Michel Dänzer

Post by Joe Perches

Post by Michel Dänzer
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. :)

Post by Joe Perches
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.

[Joe Perches]

Post by Michel Dänzer

Post by Joe Perches

Post by Michel Dänzer
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.

Post by Michel Dänzer

Post by Joe Perches
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 <***@gmail.com>
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

[Chris Wilson]

Quoting Zhang, Jerry(Junwei) (2018-12-06 02:40:42)

Post by Michel Dänzer
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")

At the cost of 1a? Nah.
-Chris

[Michel Dänzer]

Post by Chris Wilson
Quoting Zhang, Jerry(Junwei) (2018-12-06 02:40:42)

Post by Michel Dänzer
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")

At the cost of 1a? Nah.

We still #define DEBUG if CONFIG_DYNAMIC_DEBUG isn't defined, so it's
still 1b), not 1a).

[Chris Wilson]

Quoting Michel Dänzer (2018-12-06 09:21:40)

Post by Michel Dänzer

Post by Chris Wilson
Quoting Zhang, Jerry(Junwei) (2018-12-06 02:40:42)

Post by Michel Dänzer
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")

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