[PATCH 0/2] drm: Neaten and reduce object size

[PATCH 0/2] drm: Neaten and reduce object size

[Joe Perches]

Joe Perches (2):
  drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE
  drm: Simplify drm_printk to reduce object size quite a bit

 drivers/gpu/drm/drm_drv.c               |  5 +--
 drivers/gpu/drm/i915/intel_guc_loader.c | 22 +++++++------
 include/drm/drmP.h                      | 56 ++++++++++++++++-----------------
 3 files changed, 42 insertions(+), 41 deletions(-)

-- 
2.10.0.rc2.1.g053435c

[PATCH 0/2] drm: Neaten and reduce object size

[Joe Perches]

Joe Perches (2):
  drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE
  drm: Simplify drm_printk to reduce object size quite a bit

 drivers/gpu/drm/drm_drv.c               |  5 +--
 drivers/gpu/drm/i915/intel_guc_loader.c | 22 +++++++------
 include/drm/drmP.h                      | 56 ++++++++++++++++-----------------
 3 files changed, 42 insertions(+), 41 deletions(-)

-- 
2.10.0.rc2.1.g053435c

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

[PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Joe Perches]

Use a bit more consistent style with kernel loglevels without
using macro argument concatenation.

Miscellanea:

o Single statement macros don't need do {} while (0)

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 22 ++++++++++++----------
 include/drm/drmP.h                      | 26 +++++++++++++-------------
 2 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 6fd39efb7894..bc4f9895f356 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -566,7 +566,7 @@ int intel_guc_setup(struct drm_device *dev)
 	else if (err == 0)
 		DRM_INFO("GuC firmware load skipped\n");
 	else if (ret != -EIO)
-		DRM_NOTE("GuC firmware load failed: %d\n", err);
+		DRM_NOTICE("GuC firmware load failed: %d\n", err);
 	else
 		DRM_WARN("GuC firmware load failed: %d\n", err);
 
@@ -574,7 +574,7 @@ int intel_guc_setup(struct drm_device *dev)
 		if (fw_path == NULL)
 			DRM_INFO("GuC submission without firmware not supported\n");
 		if (ret == 0)
-			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
+			DRM_NOTICE("Falling back from GuC submission to execlist mode\n");
 		else
 			DRM_ERROR("GuC init failed: %d\n", ret);
 	}
@@ -606,7 +606,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 
 	/* Check the size of the blob before examining buffer contents */
 	if (fw->size < sizeof(struct guc_css_header)) {
-		DRM_NOTE("Firmware header is missing\n");
+		DRM_NOTICE("Firmware header is missing\n");
 		goto fail;
 	}
 
@@ -618,7 +618,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 		css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
 
 	if (guc_fw->header_size != sizeof(struct guc_css_header)) {
-		DRM_NOTE("CSS header definition mismatch\n");
+		DRM_NOTICE("CSS header definition mismatch\n");
 		goto fail;
 	}
 
@@ -628,7 +628,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 
 	/* now RSA */
 	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
-		DRM_NOTE("RSA key size is bad\n");
+		DRM_NOTICE("RSA key size is bad\n");
 		goto fail;
 	}
 	guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;
@@ -637,14 +637,14 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 	/* At least, it should have header, uCode and RSA. Size of all three. */
 	size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;
 	if (fw->size < size) {
-		DRM_NOTE("Missing firmware components\n");
+		DRM_NOTICE("Missing firmware components\n");
 		goto fail;
 	}
 
 	/* Header and uCode will be loaded to WOPCM. Size of the two. */
 	size = guc_fw->header_size + guc_fw->ucode_size;
 	if (size > guc_wopcm_size(to_i915(dev))) {
-		DRM_NOTE("Firmware is too large to fit in WOPCM\n");
+		DRM_NOTICE("Firmware is too large to fit in WOPCM\n");
 		goto fail;
 	}
 
@@ -659,9 +659,11 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 
 	if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
 	    guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
-		DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",
-			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
-			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
+		DRM_NOTICE("GuC firmware version %d.%d, required %d.%d\n",
+			   guc_fw->guc_fw_major_found,
+			   guc_fw->guc_fw_minor_found,
+			   guc_fw->guc_fw_major_wanted,
+			   guc_fw->guc_fw_minor_wanted);
 		err = -ENOEXEC;
 		goto fail;
 	}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index c53dc90942e0..95cd04aa9bf7 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -168,25 +168,25 @@ void drm_printk(const char *level, unsigned int category,
 /** \name Macros to make printk easier */
 /*@{*/
 
-#define _DRM_PRINTK(once, level, fmt, ...)				\
-	do {								\
-		printk##once(KERN_##level "[" DRM_NAME "] " fmt,	\
-			     ##__VA_ARGS__);				\
-	} while (0)
+#define _drm_printk(level, fmt, ...)					\
+	printk(level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
 
 #define DRM_INFO(fmt, ...)						\
-	_DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__)
-#define DRM_NOTE(fmt, ...)						\
-	_DRM_PRINTK(, NOTICE, fmt, ##__VA_ARGS__)
+	_drm_printk(KERN_INFO, fmt, ##__VA_ARGS__)
+#define DRM_NOTICE(fmt, ...)						\
+	_drm_printk(KERN_NOTICE, fmt, ##__VA_ARGS__)
 #define DRM_WARN(fmt, ...)						\
-	_DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__)
+	_drm_printk(KERN_WARNING, fmt, ##__VA_ARGS__)
+
+#define _drm_printk_once(level, fmt, ...)				\
+	printk_once(level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
 
 #define DRM_INFO_ONCE(fmt, ...)						\
-	_DRM_PRINTK(_once, INFO, fmt, ##__VA_ARGS__)
-#define DRM_NOTE_ONCE(fmt, ...)						\
-	_DRM_PRINTK(_once, NOTICE, fmt, ##__VA_ARGS__)
+	_drm_printk_once(KERN_INFO, fmt, ##__VA_ARGS__)
+#define DRM_NOTICE_ONCE(fmt, ...)					\
+	_drm_printk_once(KERN_NOTICE, fmt, ##__VA_ARGS__)
 #define DRM_WARN_ONCE(fmt, ...)						\
-	_DRM_PRINTK(_once, WARNING, fmt, ##__VA_ARGS__)
+	_drm_printk_once(KERN_WARNING, fmt, ##__VA_ARGS__)
 
 /**
  * Error output.
-- 
2.10.0.rc2.1.g053435c

[PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Joe Perches]

Use a bit more consistent style with kernel loglevels without
using macro argument concatenation.

Miscellanea:

o Single statement macros don't need do {} while (0)

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 22 ++++++++++++----------
 include/drm/drmP.h                      | 26 +++++++++++++-------------
 2 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 6fd39efb7894..bc4f9895f356 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -566,7 +566,7 @@ int intel_guc_setup(struct drm_device *dev)
 	else if (err == 0)
 		DRM_INFO("GuC firmware load skipped\n");
 	else if (ret != -EIO)
-		DRM_NOTE("GuC firmware load failed: %d\n", err);
+		DRM_NOTICE("GuC firmware load failed: %d\n", err);
 	else
 		DRM_WARN("GuC firmware load failed: %d\n", err);
 
@@ -574,7 +574,7 @@ int intel_guc_setup(struct drm_device *dev)
 		if (fw_path == NULL)
 			DRM_INFO("GuC submission without firmware not supported\n");
 		if (ret == 0)
-			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
+			DRM_NOTICE("Falling back from GuC submission to execlist mode\n");
 		else
 			DRM_ERROR("GuC init failed: %d\n", ret);
 	}
@@ -606,7 +606,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 
 	/* Check the size of the blob before examining buffer contents */
 	if (fw->size < sizeof(struct guc_css_header)) {
-		DRM_NOTE("Firmware header is missing\n");
+		DRM_NOTICE("Firmware header is missing\n");
 		goto fail;
 	}
 
@@ -618,7 +618,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 		css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
 
 	if (guc_fw->header_size != sizeof(struct guc_css_header)) {
-		DRM_NOTE("CSS header definition mismatch\n");
+		DRM_NOTICE("CSS header definition mismatch\n");
 		goto fail;
 	}
 
@@ -628,7 +628,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 
 	/* now RSA */
 	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
-		DRM_NOTE("RSA key size is bad\n");
+		DRM_NOTICE("RSA key size is bad\n");
 		goto fail;
 	}
 	guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;
@@ -637,14 +637,14 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 	/* At least, it should have header, uCode and RSA. Size of all three. */
 	size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;
 	if (fw->size < size) {
-		DRM_NOTE("Missing firmware components\n");
+		DRM_NOTICE("Missing firmware components\n");
 		goto fail;
 	}
 
 	/* Header and uCode will be loaded to WOPCM. Size of the two. */
 	size = guc_fw->header_size + guc_fw->ucode_size;
 	if (size > guc_wopcm_size(to_i915(dev))) {
-		DRM_NOTE("Firmware is too large to fit in WOPCM\n");
+		DRM_NOTICE("Firmware is too large to fit in WOPCM\n");
 		goto fail;
 	}
 
@@ -659,9 +659,11 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 
 	if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
 	    guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
-		DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",
-			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
-			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
+		DRM_NOTICE("GuC firmware version %d.%d, required %d.%d\n",
+			   guc_fw->guc_fw_major_found,
+			   guc_fw->guc_fw_minor_found,
+			   guc_fw->guc_fw_major_wanted,
+			   guc_fw->guc_fw_minor_wanted);
 		err = -ENOEXEC;
 		goto fail;
 	}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index c53dc90942e0..95cd04aa9bf7 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -168,25 +168,25 @@ void drm_printk(const char *level, unsigned int category,
 /** \name Macros to make printk easier */
 /*@{*/
 
-#define _DRM_PRINTK(once, level, fmt, ...)				\
-	do {								\
-		printk##once(KERN_##level "[" DRM_NAME "] " fmt,	\
-			     ##__VA_ARGS__);				\
-	} while (0)
+#define _drm_printk(level, fmt, ...)					\
+	printk(level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
 
 #define DRM_INFO(fmt, ...)						\
-	_DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__)
-#define DRM_NOTE(fmt, ...)						\
-	_DRM_PRINTK(, NOTICE, fmt, ##__VA_ARGS__)
+	_drm_printk(KERN_INFO, fmt, ##__VA_ARGS__)
+#define DRM_NOTICE(fmt, ...)						\
+	_drm_printk(KERN_NOTICE, fmt, ##__VA_ARGS__)
 #define DRM_WARN(fmt, ...)						\
-	_DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__)
+	_drm_printk(KERN_WARNING, fmt, ##__VA_ARGS__)
+
+#define _drm_printk_once(level, fmt, ...)				\
+	printk_once(level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
 
 #define DRM_INFO_ONCE(fmt, ...)						\
-	_DRM_PRINTK(_once, INFO, fmt, ##__VA_ARGS__)
-#define DRM_NOTE_ONCE(fmt, ...)						\
-	_DRM_PRINTK(_once, NOTICE, fmt, ##__VA_ARGS__)
+	_drm_printk_once(KERN_INFO, fmt, ##__VA_ARGS__)
+#define DRM_NOTICE_ONCE(fmt, ...)					\
+	_drm_printk_once(KERN_NOTICE, fmt, ##__VA_ARGS__)
 #define DRM_WARN_ONCE(fmt, ...)						\
-	_DRM_PRINTK(_once, WARNING, fmt, ##__VA_ARGS__)
+	_drm_printk_once(KERN_WARNING, fmt, ##__VA_ARGS__)
 
 /**
  * Error output.
-- 
2.10.0.rc2.1.g053435c

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

[PATCH 2/2] drm: Simplify drm_printk to reduce object size quite a bit

[Joe Perches]

Remove function name and special " *ERROR*" from argument list

$ size drivers/gpu/drm/built-in.o* (x86-32 defconfig, most drm selected)
   text	   data	    bss	    dec	    hex	filename
5635366	 182579	  14328	5832273	 58fe51	drivers/gpu/drm/built-in.o.new
5779552	 182579	  14328	5976459	 5b318b	drivers/gpu/drm/built-in.o.old

Using "%ps", __builtin_return_address(0) is the same as "%s", __func__
except for static inlines, but it's more or less the same output.

Miscellanea:

o Convert args... to ##__VA_ARGS__
o The equivalent DRM_DEV_<FOO> macros are rarely used and not
  worth conversion

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/gpu/drm/drm_drv.c |  5 +++--
 include/drm/drmP.h        | 30 ++++++++++++++----------------
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 80c7f25b5b74..6efdba4993fc 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -89,7 +89,6 @@ void drm_dev_printk(const struct device *dev, const char *level,
 EXPORT_SYMBOL(drm_dev_printk);
 
 void drm_printk(const char *level, unsigned int category,
-		const char *function_name, const char *prefix,
 		const char *format, ...)
 {
 	struct va_format vaf;
@@ -102,7 +101,9 @@ void drm_printk(const char *level, unsigned int category,
 	vaf.fmt = format;
 	vaf.va = &args;
 
-	printk("%s" DRM_PRINTK_FMT, level, function_name, prefix, &vaf);
+	printk("%s" "[" DRM_NAME ":%ps]%s %pV",
+	       level, __builtin_return_address(0),
+	       strcmp(level, KERN_ERR) == 0 ? " *ERROR*" : "", &vaf);
 
 	va_end(args);
 }
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 95cd04aa9bf7..4729c543d877 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -140,9 +140,8 @@ void drm_dev_printk(const struct device *dev, const char *level,
 		    unsigned int category, const char *function_name,
 		    const char *prefix, const char *format, ...);
 
-extern __printf(5, 6)
+extern __printf(3, 4)
 void drm_printk(const char *level, unsigned int category,
-		const char *function_name, const char *prefix,
 		const char *format, ...);
 
 /***********************************************************************/
@@ -198,8 +197,7 @@ void drm_printk(const char *level, unsigned int category,
 	drm_dev_printk(dev, KERN_ERR, DRM_UT_NONE, __func__, " *ERROR*",\
 		       fmt, ##__VA_ARGS__)
 #define DRM_ERROR(fmt, ...)						\
-	drm_printk(KERN_ERR, DRM_UT_NONE, __func__, " *ERROR*", fmt,	\
-		   ##__VA_ARGS__)
+	drm_printk(KERN_ERR, DRM_UT_NONE, fmt,	##__VA_ARGS__)
 
 /**
  * Rate limited error output.  Like DRM_ERROR() but won't flood the log.
@@ -241,38 +239,38 @@ void drm_printk(const char *level, unsigned int category,
 #define DRM_DEV_DEBUG(dev, fmt, args...)				\
 	drm_dev_printk(dev, KERN_DEBUG, DRM_UT_CORE, __func__, "", fmt,	\
 		       ##args)
-#define DRM_DEBUG(fmt, args...)						\
-	drm_printk(KERN_DEBUG, DRM_UT_CORE, __func__, "", fmt, ##args)
+#define DRM_DEBUG(fmt, ...)						\
+	drm_printk(KERN_DEBUG, DRM_UT_CORE, fmt, ##__VA_ARGS__)
 
 #define DRM_DEV_DEBUG_DRIVER(dev, fmt, args...)				\
 	drm_dev_printk(dev, KERN_DEBUG, DRM_UT_DRIVER, __func__, "",	\
 		       fmt, ##args)
-#define DRM_DEBUG_DRIVER(fmt, args...)					\
-	drm_printk(KERN_DEBUG, DRM_UT_DRIVER, __func__, "", fmt, ##args)
+#define DRM_DEBUG_DRIVER(fmt, ...)					\
+	drm_printk(KERN_DEBUG, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
 
 #define DRM_DEV_DEBUG_KMS(dev, fmt, args...)				\
 	drm_dev_printk(dev, KERN_DEBUG, DRM_UT_KMS, __func__, "", fmt,	\
 		       ##args)
-#define DRM_DEBUG_KMS(fmt, args...)					\
-	drm_printk(KERN_DEBUG, DRM_UT_KMS, __func__, "", fmt, ##args)
+#define DRM_DEBUG_KMS(fmt, ...)					\
+	drm_printk(KERN_DEBUG, DRM_UT_KMS, fmt, ##__VA_ARGS__)
 
 #define DRM_DEV_DEBUG_PRIME(dev, fmt, args...)				\
 	drm_dev_printk(dev, KERN_DEBUG, DRM_UT_PRIME, __func__, "",	\
 		       fmt, ##args)
-#define DRM_DEBUG_PRIME(fmt, args...)					\
-	drm_printk(KERN_DEBUG, DRM_UT_PRIME, __func__, "", fmt, ##args)
+#define DRM_DEBUG_PRIME(fmt, ...)					\
+	drm_printk(KERN_DEBUG, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
 
 #define DRM_DEV_DEBUG_ATOMIC(dev, fmt, args...)				\
 	drm_dev_printk(dev, KERN_DEBUG, DRM_UT_ATOMIC, __func__, "",	\
 		       fmt, ##args)
-#define DRM_DEBUG_ATOMIC(fmt, args...)					\
-	drm_printk(KERN_DEBUG, DRM_UT_ATOMIC, __func__, "", fmt, ##args)
+#define DRM_DEBUG_ATOMIC(fmt, ...)					\
+	drm_printk(KERN_DEBUG, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
 
 #define DRM_DEV_DEBUG_VBL(dev, fmt, args...)				\
 	drm_dev_printk(dev, KERN_DEBUG, DRM_UT_VBL, __func__, "", fmt,	\
 		       ##args)
-#define DRM_DEBUG_VBL(fmt, args...)					\
-	drm_printk(KERN_DEBUG, DRM_UT_VBL, __func__, "", fmt, ##args)
+#define DRM_DEBUG_VBL(fmt, ...)					\
+	drm_printk(KERN_DEBUG, DRM_UT_VBL, fmt, ##__VA_ARGS__)
 
 #define _DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, level, fmt, args...)	\
 ({									\
-- 
2.10.0.rc2.1.g053435c

Re: [PATCH 2/2] drm: Simplify drm_printk to reduce object size quite a bit

[Chris Wilson]

On Sun, Sep 25, 2016 at 07:18:34PM -0700, Joe Perches wrote:
> Remove function name and special " *ERROR*" from argument list
> 
> $ size drivers/gpu/drm/built-in.o* (x86-32 defconfig, most drm selected)
>    text	   data	    bss	    dec	    hex	filename
> 5635366	 182579	  14328	5832273	 58fe51	drivers/gpu/drm/built-in.o.new
> 5779552	 182579	  14328	5976459	 5b318b	drivers/gpu/drm/built-in.o.old
> 
> Using "%ps", __builtin_return_address(0) is the same as "%s", __func__
> except for static inlines, but it's more or less the same output.

And for static inlines we more often want to know which of the many
callers triggered the error. We would have to play it by ear to see
which error messages need to be improved in cases where it not obvious
without the context of the static inline function name.

Lgtm, reducing the call size for DRM_DEBUG is a definite improvement.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> 
> Miscellanea:
> 
> o Convert args... to ##__VA_ARGS__
> o The equivalent DRM_DEV_<FOO> macros are rarely used and not
>   worth conversion

Today. I would rather see us to migrate to DRM_DEV (i.e. dev_printk) so
that the drm subsystem is consistent with the rest of the kernel and
providing the necessary information for syslog filtering.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm: Simplify drm_printk to reduce object size quite a bit

[Sean Paul]

On Mon, Sep 26, 2016 at 4:36 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sun, Sep 25, 2016 at 07:18:34PM -0700, Joe Perches wrote:
>> Remove function name and special " *ERROR*" from argument list
>>
>> $ size drivers/gpu/drm/built-in.o* (x86-32 defconfig, most drm selected)
>>    text          data     bss     dec     hex filename
>> 5635366        182579   14328 5832273  58fe51 drivers/gpu/drm/built-in.o.new
>> 5779552        182579   14328 5976459  5b318b drivers/gpu/drm/built-in.o.old
>>
>> Using "%ps", __builtin_return_address(0) is the same as "%s", __func__
>> except for static inlines, but it's more or less the same output.
>
> And for static inlines we more often want to know which of the many
> callers triggered the error. We would have to play it by ear to see
> which error messages need to be improved in cases where it not obvious
> without the context of the static inline function name.
>
> Lgtm, reducing the call size for DRM_DEBUG is a definite improvement.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>

Agreed on the static inlines, hopefully we won't break anyone's log
grep workflow.

I will push this to -misc in a couple days if no one objects.

Reviewed-by: Sean Paul <seanpaul@chromium.org>

>>
>> Miscellanea:
>>
>> o Convert args... to ##__VA_ARGS__
>> o The equivalent DRM_DEV_<FOO> macros are rarely used and not
>>   worth conversion
>
> Today. I would rather see us to migrate to DRM_DEV (i.e. dev_printk) so
> that the drm subsystem is consistent with the rest of the kernel and
> providing the necessary information for syslog filtering.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: Simplify drm_printk to reduce object size quite a bit

[Sean Paul]

On Mon, Sep 26, 2016 at 4:36 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sun, Sep 25, 2016 at 07:18:34PM -0700, Joe Perches wrote:
>> Remove function name and special " *ERROR*" from argument list
>>
>> $ size drivers/gpu/drm/built-in.o* (x86-32 defconfig, most drm selected)
>>    text          data     bss     dec     hex filename
>> 5635366        182579   14328 5832273  58fe51 drivers/gpu/drm/built-in.o.new
>> 5779552        182579   14328 5976459  5b318b drivers/gpu/drm/built-in.o.old
>>
>> Using "%ps", __builtin_return_address(0) is the same as "%s", __func__
>> except for static inlines, but it's more or less the same output.
>
> And for static inlines we more often want to know which of the many
> callers triggered the error. We would have to play it by ear to see
> which error messages need to be improved in cases where it not obvious
> without the context of the static inline function name.
>
> Lgtm, reducing the call size for DRM_DEBUG is a definite improvement.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>

Agreed on the static inlines, hopefully we won't break anyone's log
grep workflow.

I will push this to -misc in a couple days if no one objects.

Reviewed-by: Sean Paul <seanpaul@chromium.org>

>>
>> Miscellanea:
>>
>> o Convert args... to ##__VA_ARGS__
>> o The equivalent DRM_DEV_<FOO> macros are rarely used and not
>>   worth conversion
>
> Today. I would rather see us to migrate to DRM_DEV (i.e. dev_printk) so
> that the drm subsystem is consistent with the rest of the kernel and
> providing the necessary information for syslog filtering.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Sean Paul]

On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
> Use a bit more consistent style with kernel loglevels

I'm not convinced this is worth doing if we're going to keep the
WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
to DRM_WARNING since it's so widely used.

Sean

> without
> using macro argument concatenation.
>
> Miscellanea:
>
> o Single statement macros don't need do {} while (0)
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_loader.c | 22 ++++++++++++----------
>  include/drm/drmP.h                      | 26 +++++++++++++-------------
>  2 files changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 6fd39efb7894..bc4f9895f356 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -566,7 +566,7 @@ int intel_guc_setup(struct drm_device *dev)
>         else if (err == 0)
>                 DRM_INFO("GuC firmware load skipped\n");
>         else if (ret != -EIO)
> -               DRM_NOTE("GuC firmware load failed: %d\n", err);
> +               DRM_NOTICE("GuC firmware load failed: %d\n", err);
>         else
>                 DRM_WARN("GuC firmware load failed: %d\n", err);
>
> @@ -574,7 +574,7 @@ int intel_guc_setup(struct drm_device *dev)
>                 if (fw_path == NULL)
>                         DRM_INFO("GuC submission without firmware not supported\n");
>                 if (ret == 0)
> -                       DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> +                       DRM_NOTICE("Falling back from GuC submission to execlist mode\n");
>                 else
>                         DRM_ERROR("GuC init failed: %d\n", ret);
>         }
> @@ -606,7 +606,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>
>         /* Check the size of the blob before examining buffer contents */
>         if (fw->size < sizeof(struct guc_css_header)) {
> -               DRM_NOTE("Firmware header is missing\n");
> +               DRM_NOTICE("Firmware header is missing\n");
>                 goto fail;
>         }
>
> @@ -618,7 +618,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>                 css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
>
>         if (guc_fw->header_size != sizeof(struct guc_css_header)) {
> -               DRM_NOTE("CSS header definition mismatch\n");
> +               DRM_NOTICE("CSS header definition mismatch\n");
>                 goto fail;
>         }
>
> @@ -628,7 +628,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>
>         /* now RSA */
>         if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
> -               DRM_NOTE("RSA key size is bad\n");
> +               DRM_NOTICE("RSA key size is bad\n");
>                 goto fail;
>         }
>         guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;
> @@ -637,14 +637,14 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>         /* At least, it should have header, uCode and RSA. Size of all three. */
>         size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;
>         if (fw->size < size) {
> -               DRM_NOTE("Missing firmware components\n");
> +               DRM_NOTICE("Missing firmware components\n");
>                 goto fail;
>         }
>
>         /* Header and uCode will be loaded to WOPCM. Size of the two. */
>         size = guc_fw->header_size + guc_fw->ucode_size;
>         if (size > guc_wopcm_size(to_i915(dev))) {
> -               DRM_NOTE("Firmware is too large to fit in WOPCM\n");
> +               DRM_NOTICE("Firmware is too large to fit in WOPCM\n");
>                 goto fail;
>         }
>
> @@ -659,9 +659,11 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>
>         if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
>             guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
> -               DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",
> -                       guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
> -                       guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> +               DRM_NOTICE("GuC firmware version %d.%d, required %d.%d\n",
> +                          guc_fw->guc_fw_major_found,
> +                          guc_fw->guc_fw_minor_found,
> +                          guc_fw->guc_fw_major_wanted,
> +                          guc_fw->guc_fw_minor_wanted);
>                 err = -ENOEXEC;
>                 goto fail;
>         }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index c53dc90942e0..95cd04aa9bf7 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -168,25 +168,25 @@ void drm_printk(const char *level, unsigned int category,
>  /** \name Macros to make printk easier */
>  /*@{*/
>
> -#define _DRM_PRINTK(once, level, fmt, ...)                             \
> -       do {                                                            \
> -               printk##once(KERN_##level "[" DRM_NAME "] " fmt,        \
> -                            ##__VA_ARGS__);                            \
> -       } while (0)
> +#define _drm_printk(level, fmt, ...)                                   \
> +       printk(level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
>
>  #define DRM_INFO(fmt, ...)                                             \
> -       _DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__)
> -#define DRM_NOTE(fmt, ...)                                             \
> -       _DRM_PRINTK(, NOTICE, fmt, ##__VA_ARGS__)
> +       _drm_printk(KERN_INFO, fmt, ##__VA_ARGS__)
> +#define DRM_NOTICE(fmt, ...)                                           \
> +       _drm_printk(KERN_NOTICE, fmt, ##__VA_ARGS__)
>  #define DRM_WARN(fmt, ...)                                             \
> -       _DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__)
> +       _drm_printk(KERN_WARNING, fmt, ##__VA_ARGS__)
> +
> +#define _drm_printk_once(level, fmt, ...)                              \
> +       printk_once(level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
>
>  #define DRM_INFO_ONCE(fmt, ...)                                                \
> -       _DRM_PRINTK(_once, INFO, fmt, ##__VA_ARGS__)
> -#define DRM_NOTE_ONCE(fmt, ...)                                                \
> -       _DRM_PRINTK(_once, NOTICE, fmt, ##__VA_ARGS__)
> +       _drm_printk_once(KERN_INFO, fmt, ##__VA_ARGS__)
> +#define DRM_NOTICE_ONCE(fmt, ...)                                      \
> +       _drm_printk_once(KERN_NOTICE, fmt, ##__VA_ARGS__)
>  #define DRM_WARN_ONCE(fmt, ...)                                                \
> -       _DRM_PRINTK(_once, WARNING, fmt, ##__VA_ARGS__)
> +       _drm_printk_once(KERN_WARNING, fmt, ##__VA_ARGS__)
>
>  /**
>   * Error output.
> --
> 2.10.0.rc2.1.g053435c
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Sean Paul]

On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
> Use a bit more consistent style with kernel loglevels

I'm not convinced this is worth doing if we're going to keep the
WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
to DRM_WARNING since it's so widely used.

Sean

> without
> using macro argument concatenation.
>
> Miscellanea:
>
> o Single statement macros don't need do {} while (0)
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_loader.c | 22 ++++++++++++----------
>  include/drm/drmP.h                      | 26 +++++++++++++-------------
>  2 files changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 6fd39efb7894..bc4f9895f356 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -566,7 +566,7 @@ int intel_guc_setup(struct drm_device *dev)
>         else if (err == 0)
>                 DRM_INFO("GuC firmware load skipped\n");
>         else if (ret != -EIO)
> -               DRM_NOTE("GuC firmware load failed: %d\n", err);
> +               DRM_NOTICE("GuC firmware load failed: %d\n", err);
>         else
>                 DRM_WARN("GuC firmware load failed: %d\n", err);
>
> @@ -574,7 +574,7 @@ int intel_guc_setup(struct drm_device *dev)
>                 if (fw_path == NULL)
>                         DRM_INFO("GuC submission without firmware not supported\n");
>                 if (ret == 0)
> -                       DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> +                       DRM_NOTICE("Falling back from GuC submission to execlist mode\n");
>                 else
>                         DRM_ERROR("GuC init failed: %d\n", ret);
>         }
> @@ -606,7 +606,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>
>         /* Check the size of the blob before examining buffer contents */
>         if (fw->size < sizeof(struct guc_css_header)) {
> -               DRM_NOTE("Firmware header is missing\n");
> +               DRM_NOTICE("Firmware header is missing\n");
>                 goto fail;
>         }
>
> @@ -618,7 +618,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>                 css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
>
>         if (guc_fw->header_size != sizeof(struct guc_css_header)) {
> -               DRM_NOTE("CSS header definition mismatch\n");
> +               DRM_NOTICE("CSS header definition mismatch\n");
>                 goto fail;
>         }
>
> @@ -628,7 +628,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>
>         /* now RSA */
>         if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
> -               DRM_NOTE("RSA key size is bad\n");
> +               DRM_NOTICE("RSA key size is bad\n");
>                 goto fail;
>         }
>         guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;
> @@ -637,14 +637,14 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>         /* At least, it should have header, uCode and RSA. Size of all three. */
>         size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;
>         if (fw->size < size) {
> -               DRM_NOTE("Missing firmware components\n");
> +               DRM_NOTICE("Missing firmware components\n");
>                 goto fail;
>         }
>
>         /* Header and uCode will be loaded to WOPCM. Size of the two. */
>         size = guc_fw->header_size + guc_fw->ucode_size;
>         if (size > guc_wopcm_size(to_i915(dev))) {
> -               DRM_NOTE("Firmware is too large to fit in WOPCM\n");
> +               DRM_NOTICE("Firmware is too large to fit in WOPCM\n");
>                 goto fail;
>         }
>
> @@ -659,9 +659,11 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>
>         if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
>             guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
> -               DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",
> -                       guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
> -                       guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
> +               DRM_NOTICE("GuC firmware version %d.%d, required %d.%d\n",
> +                          guc_fw->guc_fw_major_found,
> +                          guc_fw->guc_fw_minor_found,
> +                          guc_fw->guc_fw_major_wanted,
> +                          guc_fw->guc_fw_minor_wanted);
>                 err = -ENOEXEC;
>                 goto fail;
>         }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index c53dc90942e0..95cd04aa9bf7 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -168,25 +168,25 @@ void drm_printk(const char *level, unsigned int category,
>  /** \name Macros to make printk easier */
>  /*@{*/
>
> -#define _DRM_PRINTK(once, level, fmt, ...)                             \
> -       do {                                                            \
> -               printk##once(KERN_##level "[" DRM_NAME "] " fmt,        \
> -                            ##__VA_ARGS__);                            \
> -       } while (0)
> +#define _drm_printk(level, fmt, ...)                                   \
> +       printk(level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
>
>  #define DRM_INFO(fmt, ...)                                             \
> -       _DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__)
> -#define DRM_NOTE(fmt, ...)                                             \
> -       _DRM_PRINTK(, NOTICE, fmt, ##__VA_ARGS__)
> +       _drm_printk(KERN_INFO, fmt, ##__VA_ARGS__)
> +#define DRM_NOTICE(fmt, ...)                                           \
> +       _drm_printk(KERN_NOTICE, fmt, ##__VA_ARGS__)
>  #define DRM_WARN(fmt, ...)                                             \
> -       _DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__)
> +       _drm_printk(KERN_WARNING, fmt, ##__VA_ARGS__)
> +
> +#define _drm_printk_once(level, fmt, ...)                              \
> +       printk_once(level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
>
>  #define DRM_INFO_ONCE(fmt, ...)                                                \
> -       _DRM_PRINTK(_once, INFO, fmt, ##__VA_ARGS__)
> -#define DRM_NOTE_ONCE(fmt, ...)                                                \
> -       _DRM_PRINTK(_once, NOTICE, fmt, ##__VA_ARGS__)
> +       _drm_printk_once(KERN_INFO, fmt, ##__VA_ARGS__)
> +#define DRM_NOTICE_ONCE(fmt, ...)                                      \
> +       _drm_printk_once(KERN_NOTICE, fmt, ##__VA_ARGS__)
>  #define DRM_WARN_ONCE(fmt, ...)                                                \
> -       _DRM_PRINTK(_once, WARNING, fmt, ##__VA_ARGS__)
> +       _drm_printk_once(KERN_WARNING, fmt, ##__VA_ARGS__)
>
>  /**
>   * Error output.
> --
> 2.10.0.rc2.1.g053435c
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Joe Perches]

On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote:
> > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
> > Use a bit more consistent style with kernel loglevels
> > I'm not convinced this is worth doing if we're going to keep the
> WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
> to DRM_WARNING since it's so widely used.

There is no DRM_WARN inconsistency.

What is used is pr_warn and dev_warn, not pr_warning and dev_warning

Well, there are still a few pr_warning uses, but those
will eventually be removed/converted.

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Joe Perches]

On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote:
> > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
> > Use a bit more consistent style with kernel loglevels
> > I'm not convinced this is worth doing if we're going to keep the
> WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
> to DRM_WARNING since it's so widely used.

There is no DRM_WARN inconsistency.

What is used is pr_warn and dev_warn, not pr_warning and dev_warning

Well, there are still a few pr_warning uses, but those
will eventually be removed/converted.

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

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Emil Velikov]

On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote:
> On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote:
>> > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
>> > Use a bit more consistent style with kernel loglevels
>> > I'm not convinced this is worth doing if we're going to keep the
>> WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
>> to DRM_WARNING since it's so widely used.
>
> There is no DRM_WARN inconsistency.
>
DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and
DRM_NOTE is to DRM_NOTICE... is what I'm thinking and seemingly so
does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the
patch, so it might be worth keeping separate ?

-Emil

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Emil Velikov]

On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote:
> On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote:
>> > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
>> > Use a bit more consistent style with kernel loglevels
>> > I'm not convinced this is worth doing if we're going to keep the
>> WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
>> to DRM_WARNING since it's so widely used.
>
> There is no DRM_WARN inconsistency.
>
DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and
DRM_NOTE is to DRM_NOTICE... is what I'm thinking and seemingly so
does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the
patch, so it might be worth keeping separate ?

-Emil
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Joe Perches]

On Tue, 2016-09-27 at 17:36 +0100, Emil Velikov wrote:
> On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote:
> > > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
> > > > Use a bit more consistent style with kernel loglevels
> > > I'm not convinced this is worth doing if we're going to keep the
> > > WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
> > > to DRM_WARNING since it's so widely used.
> > There is no DRM_WARN inconsistency.
> DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and
> DRM_NOTE is to DRM_NOTICE...

DRM_INFORMATION doesn't exist in the kernel tree.

> is what I'm thinking and seemingly so
> does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the
> patch, so it might be worth keeping separate ?

To me, simplifying the macro means using the common kernel
macro forms.

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Joe Perches]

On Tue, 2016-09-27 at 17:36 +0100, Emil Velikov wrote:
> On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote:
> > > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
> > > > Use a bit more consistent style with kernel loglevels
> > > I'm not convinced this is worth doing if we're going to keep the
> > > WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
> > > to DRM_WARNING since it's so widely used.
> > There is no DRM_WARN inconsistency.
> DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and
> DRM_NOTE is to DRM_NOTICE...

DRM_INFORMATION doesn't exist in the kernel tree.

> is what I'm thinking and seemingly so
> does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the
> patch, so it might be worth keeping separate ?

To me, simplifying the macro means using the common kernel
macro forms.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Emil Velikov]

On 27 September 2016 at 17:43, Joe Perches <joe@perches.com> wrote:
> On Tue, 2016-09-27 at 17:36 +0100, Emil Velikov wrote:
>> On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote:
>> > On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote:
>> > > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
>> > > > Use a bit more consistent style with kernel loglevels
>> > > I'm not convinced this is worth doing if we're going to keep the
>> > > WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
>> > > to DRM_WARNING since it's so widely used.
>> > There is no DRM_WARN inconsistency.
>> DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and
>> DRM_NOTE is to DRM_NOTICE...
>
> DRM_INFORMATION doesn't exist in the kernel tree.
>
>> is what I'm thinking and seemingly so
>> does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the
>> patch, so it might be worth keeping separate ?
>
> To me, simplifying the macro means using the common kernel
> macro forms.
>
"unify" might be better, but I agree.

Either way there's no point in elaborating on the point me(Sean?)
meant since it's just going to get shoot down like a dog ;-)

Regards,
Emil

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Emil Velikov]

On 27 September 2016 at 17:43, Joe Perches <joe@perches.com> wrote:
> On Tue, 2016-09-27 at 17:36 +0100, Emil Velikov wrote:
>> On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote:
>> > On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote:
>> > > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
>> > > > Use a bit more consistent style with kernel loglevels
>> > > I'm not convinced this is worth doing if we're going to keep the
>> > > WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
>> > > to DRM_WARNING since it's so widely used.
>> > There is no DRM_WARN inconsistency.
>> DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and
>> DRM_NOTE is to DRM_NOTICE...
>
> DRM_INFORMATION doesn't exist in the kernel tree.
>
>> is what I'm thinking and seemingly so
>> does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the
>> patch, so it might be worth keeping separate ?
>
> To me, simplifying the macro means using the common kernel
> macro forms.
>
"unify" might be better, but I agree.

Either way there's no point in elaborating on the point me(Sean?)
meant since it's just going to get shoot down like a dog ;-)

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

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Joe Perches]

On Tue, 2016-09-27 at 17:36 +0100, Emil Velikov wrote:
> On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote:
> > > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
> > > > Use a bit more consistent style with kernel loglevels
> > > I'm not convinced this is worth doing if we're going to keep the
> > > WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
> > > to DRM_WARNING since it's so widely used.
> > There is no DRM_WARN inconsistency.
> DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and
> DRM_NOTE is to DRM_NOTICE...

DRM_INFORMATION doesn't exist in the kernel tree.

> is what I'm thinking and seemingly so
> does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the
> patch, so it might be worth keeping separate ?

To me, simplifying the macro means using the common kernel
macro forms.

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Joe Perches]

On Tue, 2016-09-27 at 17:36 +0100, Emil Velikov wrote:
> On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote:
> > > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
> > > > Use a bit more consistent style with kernel loglevels
> > > I'm not convinced this is worth doing if we're going to keep the
> > > WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
> > > to DRM_WARNING since it's so widely used.
> > There is no DRM_WARN inconsistency.
> DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and
> DRM_NOTE is to DRM_NOTICE...

DRM_INFORMATION doesn't exist in the kernel tree.

> is what I'm thinking and seemingly so
> does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the
> patch, so it might be worth keeping separate ?

To me, simplifying the macro means using the common kernel
macro forms.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Sean Paul]

On Tue, Sep 27, 2016 at 12:54 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 27 September 2016 at 17:43, Joe Perches <joe@perches.com> wrote:
>> On Tue, 2016-09-27 at 17:36 +0100, Emil Velikov wrote:
>>> On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote:
>>> > On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote:
>>> > > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
>>> > > > Use a bit more consistent style with kernel loglevels
>>> > > I'm not convinced this is worth doing if we're going to keep the
>>> > > WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
>>> > > to DRM_WARNING since it's so widely used.
>>> > There is no DRM_WARN inconsistency.
>>> DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and
>>> DRM_NOTE is to DRM_NOTICE...
>>
>> DRM_INFORMATION doesn't exist in the kernel tree.
>>
>>> is what I'm thinking and seemingly so
>>> does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the
>>> patch, so it might be worth keeping separate ?
>>
>> To me, simplifying the macro means using the common kernel
>> macro forms.
>>
> "unify" might be better, but I agree.
>
> Either way there's no point in elaborating on the point me(Sean?)
> meant since it's just going to get shoot down like a dog ;-)

Yeah, I can see both sides, and I suppose I don't really care either
way. Given that DRM_NOTE/NOTICE is only used 7 places (in one file), I
doubt there are going to be any strong feelings.

Sean

>
> Regards,
> Emil

Re: [PATCH 1/2] drm: Simplify logging macros, convert DRM_NOTE to DRM_NOTICE

[Sean Paul]

On Tue, Sep 27, 2016 at 12:54 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 27 September 2016 at 17:43, Joe Perches <joe@perches.com> wrote:
>> On Tue, 2016-09-27 at 17:36 +0100, Emil Velikov wrote:
>>> On 27 September 2016 at 17:04, Joe Perches <joe@perches.com> wrote:
>>> > On Tue, 2016-09-27 at 11:58 -0400, Sean Paul wrote:
>>> > > On Sun, Sep 25, 2016 at 10:18 PM, Joe Perches <joe@perches.com> wrote:
>>> > > > Use a bit more consistent style with kernel loglevels
>>> > > I'm not convinced this is worth doing if we're going to keep the
>>> > > WARN/WARNING discrepancy, and I don't think we should switch DRM_WARN
>>> > > to DRM_WARNING since it's so widely used.
>>> > There is no DRM_WARN inconsistency.
>>> DRM_WARN is to DRM_WARNING like DRM_INFO is to DRM_INFORMATION and
>>> DRM_NOTE is to DRM_NOTICE...
>>
>> DRM_INFORMATION doesn't exist in the kernel tree.
>>
>>> is what I'm thinking and seemingly so
>>> does Sean. Fwiw that part seem cosmetic/unrelated to the rest of the
>>> patch, so it might be worth keeping separate ?
>>
>> To me, simplifying the macro means using the common kernel
>> macro forms.
>>
> "unify" might be better, but I agree.
>
> Either way there's no point in elaborating on the point me(Sean?)
> meant since it's just going to get shoot down like a dog ;-)

Yeah, I can see both sides, and I suppose I don't really care either
way. Given that DRM_NOTE/NOTICE is only used 7 places (in one file), I
doubt there are going to be any strong feelings.

Sean

>
> Regards,
> Emil
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm: Simplify drm_printk to reduce object size quite a bit

[Sean Paul]

On Tue, Sep 27, 2016 at 11:54 AM, Sean Paul <seanpaul@chromium.org> wrote:
> On Mon, Sep 26, 2016 at 4:36 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> On Sun, Sep 25, 2016 at 07:18:34PM -0700, Joe Perches wrote:
>>> Remove function name and special " *ERROR*" from argument list
>>>
>>> $ size drivers/gpu/drm/built-in.o* (x86-32 defconfig, most drm selected)
>>>    text          data     bss     dec     hex filename
>>> 5635366        182579   14328 5832273  58fe51 drivers/gpu/drm/built-in.o.new
>>> 5779552        182579   14328 5976459  5b318b drivers/gpu/drm/built-in.o.old
>>>
>>> Using "%ps", __builtin_return_address(0) is the same as "%s", __func__
>>> except for static inlines, but it's more or less the same output.
>>
>> And for static inlines we more often want to know which of the many
>> callers triggered the error. We would have to play it by ear to see
>> which error messages need to be improved in cases where it not obvious
>> without the context of the static inline function name.
>>
>> Lgtm, reducing the call size for DRM_DEBUG is a definite improvement.
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>>
>
> Agreed on the static inlines, hopefully we won't break anyone's log
> grep workflow.
>
> I will push this to -misc in a couple days if no one objects.

Applied, thanks!

Sean


>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
>
>>>
>>> Miscellanea:
>>>
>>> o Convert args... to ##__VA_ARGS__
>>> o The equivalent DRM_DEV_<FOO> macros are rarely used and not
>>>   worth conversion
>>
>> Today. I would rather see us to migrate to DRM_DEV (i.e. dev_printk) so
>> that the drm subsystem is consistent with the rest of the kernel and
>> providing the necessary information for syslog filtering.
>> -Chris
>>
>> --
>> Chris Wilson, Intel Open Source Technology Centre
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: Simplify drm_printk to reduce object size quite a bit

[Sean Paul]

On Tue, Sep 27, 2016 at 11:54 AM, Sean Paul <seanpaul@chromium.org> wrote:
> On Mon, Sep 26, 2016 at 4:36 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> On Sun, Sep 25, 2016 at 07:18:34PM -0700, Joe Perches wrote:
>>> Remove function name and special " *ERROR*" from argument list
>>>
>>> $ size drivers/gpu/drm/built-in.o* (x86-32 defconfig, most drm selected)
>>>    text          data     bss     dec     hex filename
>>> 5635366        182579   14328 5832273  58fe51 drivers/gpu/drm/built-in.o.new
>>> 5779552        182579   14328 5976459  5b318b drivers/gpu/drm/built-in.o.old
>>>
>>> Using "%ps", __builtin_return_address(0) is the same as "%s", __func__
>>> except for static inlines, but it's more or less the same output.
>>
>> And for static inlines we more often want to know which of the many
>> callers triggered the error. We would have to play it by ear to see
>> which error messages need to be improved in cases where it not obvious
>> without the context of the static inline function name.
>>
>> Lgtm, reducing the call size for DRM_DEBUG is a definite improvement.
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>>
>
> Agreed on the static inlines, hopefully we won't break anyone's log
> grep workflow.
>
> I will push this to -misc in a couple days if no one objects.

Applied, thanks!

Sean


>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
>
>>>
>>> Miscellanea:
>>>
>>> o Convert args... to ##__VA_ARGS__
>>> o The equivalent DRM_DEV_<FOO> macros are rarely used and not
>>>   worth conversion
>>
>> Today. I would rather see us to migrate to DRM_DEV (i.e. dev_printk) so
>> that the drm subsystem is consistent with the rest of the kernel and
>> providing the necessary information for syslog filtering.
>> -Chris
>>
>> --
>> Chris Wilson, Intel Open Source Technology Centre
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel