[PATCH 0/3] Improve drm printer code

[PATCH 0/3] Improve drm printer code

[Michal Wajdeczko]

We already have some drm printk functions that need to duplicate
a code to get a similar format of the final result, for example:

  [ ] 0000:00:00.0: [drm:foo] bar
  [ ] 0000:00:00.0: [drm] foo bar
  [ ] 0000:00:00.0: [drm] *ERROR* foo

Add a generic __drm_dev_vprintk() function that can format the
final message like all other existing function do and allows us
to keep the formatting code in one place.

Above also allows to improve drm_dbg_printer() that today lacks
of outputing symbolic name of the caller, like drm_dbg() does.

Cc: Maxime Ripard <mripard@kernel.org>
Cc: Jani Nikula <jani.nikula@intel.com>

Michal Wajdeczko (3):
  drm/print: Add generic drm dev printk function
  drm/print: Improve drm_dbg_printer
  drm/i915: Don't use __func__ as prefix for drm_dbg_printer

 drivers/gpu/drm/drm_print.c                | 50 ++++++++++++----------
 drivers/gpu/drm/i915/gt/intel_reset.c      |  2 +-
 drivers/gpu/drm/i915/gt/selftest_context.c |  2 +-
 include/drm/drm_print.h                    |  5 +++
 4 files changed, 34 insertions(+), 25 deletions(-)

-- 
2.43.0

[PATCH 1/3] drm/print: Add generic drm dev printk function

[Michal Wajdeczko]

We already have some drm printk functions that need to duplicate
a code to get a similar format of the final result, for example:

  [ ] 0000:00:00.0: [drm:foo] bar
  [ ] 0000:00:00.0: [drm] foo bar
  [ ] 0000:00:00.0: [drm] *ERROR* foo

Add a generic __drm_dev_vprintk() function that can format the
final message like all other existing function do and allows us
to keep the formatting code in one place.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_print.c | 49 ++++++++++++++++++++-----------------
 include/drm/drm_print.h     |  3 +++
 2 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index cf2efb44722c..a2b60c8245a1 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -187,19 +187,12 @@ void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf)
 	const struct drm_device *drm = p->arg;
 	const struct device *dev = drm ? drm->dev : NULL;
 	enum drm_debug_category category = p->category;
-	const char *prefix = p->prefix ?: "";
-	const char *prefix_pad = p->prefix ? " " : "";
 
 	if (!__drm_debug_enabled(category))
 		return;
 
 	/* Note: __builtin_return_address(0) is useless here. */
-	if (dev)
-		dev_printk(KERN_DEBUG, dev, "[" DRM_NAME "]%s%s %pV",
-			   prefix_pad, prefix, vaf);
-	else
-		printk(KERN_DEBUG "[" DRM_NAME "]%s%s %pV",
-		       prefix_pad, prefix, vaf);
+	__drm_dev_vprintk(dev, KERN_DEBUG, NULL, p->prefix, vaf);
 }
 EXPORT_SYMBOL(__drm_printfn_dbg);
 
@@ -277,6 +270,29 @@ void drm_print_bits(struct drm_printer *p, unsigned long value,
 }
 EXPORT_SYMBOL(drm_print_bits);
 
+void __drm_dev_vprintk(const struct device *dev, const char *level,
+		       const void *origin, const char *prefix,
+		       struct va_format *vaf)
+{
+	const char *prefix_pad = prefix ? " " : (prefix = "");
+
+	if (dev)
+		if (origin)
+			dev_printk(level, dev, "[" DRM_NAME ":%ps]%s%s %pV",
+				   origin, prefix_pad, prefix, vaf);
+		else
+			dev_printk(level, dev, "[" DRM_NAME "]%s%s %pV",
+				   prefix_pad, prefix, vaf);
+	else
+		if (origin)
+			printk("%s" "[" DRM_NAME ":%ps]%s%s %pV",
+			       level, origin, prefix_pad, prefix, vaf);
+		else
+			printk("%s" "[" DRM_NAME "]%s%s %pV",
+			       level, prefix_pad, prefix, vaf);
+}
+EXPORT_SYMBOL(__drm_dev_vprintk);
+
 void drm_dev_printk(const struct device *dev, const char *level,
 		    const char *format, ...)
 {
@@ -287,12 +303,7 @@ void drm_dev_printk(const struct device *dev, const char *level,
 	vaf.fmt = format;
 	vaf.va = &args;
 
-	if (dev)
-		dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
-			   __builtin_return_address(0), &vaf);
-	else
-		printk("%s" "[" DRM_NAME ":%ps] %pV",
-		       level, __builtin_return_address(0), &vaf);
+	__drm_dev_vprintk(dev, level, __builtin_return_address(0), NULL, &vaf);
 
 	va_end(args);
 }
@@ -312,12 +323,7 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
 	vaf.fmt = format;
 	vaf.va = &args;
 
-	if (dev)
-		dev_printk(KERN_DEBUG, dev, "[" DRM_NAME ":%ps] %pV",
-			   __builtin_return_address(0), &vaf);
-	else
-		printk(KERN_DEBUG "[" DRM_NAME ":%ps] %pV",
-		       __builtin_return_address(0), &vaf);
+	__drm_dev_vprintk(dev, KERN_DEBUG, __builtin_return_address(0), NULL, &vaf);
 
 	va_end(args);
 }
@@ -351,8 +357,7 @@ void __drm_err(const char *format, ...)
 	vaf.fmt = format;
 	vaf.va = &args;
 
-	printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV",
-	       __builtin_return_address(0), &vaf);
+	__drm_dev_vprintk(NULL, KERN_ERR, __builtin_return_address(0), "*ERROR*", &vaf);
 
 	va_end(args);
 }
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 089950ad8681..bb1801c58544 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -366,6 +366,9 @@ static inline struct drm_printer drm_err_printer(struct drm_device *drm,
 __printf(3, 4)
 void drm_dev_printk(const struct device *dev, const char *level,
 		    const char *format, ...);
+void __drm_dev_vprintk(const struct device *dev, const char *level,
+		       const void *origin, const char *prefix,
+		       struct va_format *vaf);
 struct _ddebug;
 __printf(4, 5)
 void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
-- 
2.43.0

[PATCH 2/3] drm/print: Improve drm_dbg_printer

[Michal Wajdeczko]

With recent introduction of a generic drm dev printk function, we
can now store and use location where drm_dbg_printer was invoked
and output it's symbolic name like we do for all drm debug prints.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_print.c | 3 +--
 include/drm/drm_print.h     | 2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index a2b60c8245a1..0a205fdee7cf 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -191,8 +191,7 @@ void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf)
 	if (!__drm_debug_enabled(category))
 		return;
 
-	/* Note: __builtin_return_address(0) is useless here. */
-	__drm_dev_vprintk(dev, KERN_DEBUG, NULL, p->prefix, vaf);
+	__drm_dev_vprintk(dev, KERN_DEBUG, p->origin, p->prefix, vaf);
 }
 EXPORT_SYMBOL(__drm_printfn_dbg);
 
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index bb1801c58544..761ce01761b7 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -175,6 +175,7 @@ struct drm_printer {
 	void (*printfn)(struct drm_printer *p, struct va_format *vaf);
 	void (*puts)(struct drm_printer *p, const char *str);
 	void *arg;
+	const void *origin;
 	const char *prefix;
 	enum drm_debug_category category;
 };
@@ -332,6 +333,7 @@ static inline struct drm_printer drm_dbg_printer(struct drm_device *drm,
 	struct drm_printer p = {
 		.printfn = __drm_printfn_dbg,
 		.arg = drm,
+		.origin = (void *)_THIS_IP_, /* it's fine as we will be inlined */
 		.prefix = prefix,
 		.category = category,
 	};
-- 
2.43.0

[PATCH 3/3] drm/i915: Don't use __func__ as prefix for drm_dbg_printer

[Michal Wajdeczko]

Updated code of drm_dbg_printer() is already printing symbolic
name of the caller like drm_dbg() does.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_reset.c      | 2 +-
 drivers/gpu/drm/i915/gt/selftest_context.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
index 6161f7a3ff70..735cd23a43c6 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -1025,7 +1025,7 @@ void intel_gt_set_wedged(struct intel_gt *gt)
 
 	if (GEM_SHOW_DEBUG()) {
 		struct drm_printer p = drm_dbg_printer(&gt->i915->drm,
-						       DRM_UT_DRIVER, __func__);
+						       DRM_UT_DRIVER, NULL);
 		struct intel_engine_cs *engine;
 		enum intel_engine_id id;
 
diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c b/drivers/gpu/drm/i915/gt/selftest_context.c
index 12eca750f7d0..5eb46700dc4e 100644
--- a/drivers/gpu/drm/i915/gt/selftest_context.c
+++ b/drivers/gpu/drm/i915/gt/selftest_context.c
@@ -286,7 +286,7 @@ static int __live_active_context(struct intel_engine_cs *engine)
 
 	if (intel_engine_pm_is_awake(engine)) {
 		struct drm_printer p = drm_dbg_printer(&engine->i915->drm,
-						       DRM_UT_DRIVER, __func__);
+						       DRM_UT_DRIVER, NULL);
 
 		intel_engine_dump(engine, &p,
 				  "%s is still awake:%d after idle-barriers\n",
-- 
2.43.0

Re: [PATCH 1/3] drm/print: Add generic drm dev printk function

[Jani Nikula]

On Fri, 17 May 2024, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> We already have some drm printk functions that need to duplicate
> a code to get a similar format of the final result, for example:
>
>   [ ] 0000:00:00.0: [drm:foo] bar
>   [ ] 0000:00:00.0: [drm] foo bar
>   [ ] 0000:00:00.0: [drm] *ERROR* foo
>
> Add a generic __drm_dev_vprintk() function that can format the
> final message like all other existing function do and allows us
> to keep the formatting code in one place.

Nice idea!

> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_print.c | 49 ++++++++++++++++++++-----------------
>  include/drm/drm_print.h     |  3 +++
>  2 files changed, 30 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index cf2efb44722c..a2b60c8245a1 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -187,19 +187,12 @@ void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf)
>  	const struct drm_device *drm = p->arg;
>  	const struct device *dev = drm ? drm->dev : NULL;
>  	enum drm_debug_category category = p->category;
> -	const char *prefix = p->prefix ?: "";
> -	const char *prefix_pad = p->prefix ? " " : "";
>  
>  	if (!__drm_debug_enabled(category))
>  		return;
>  
>  	/* Note: __builtin_return_address(0) is useless here. */
> -	if (dev)
> -		dev_printk(KERN_DEBUG, dev, "[" DRM_NAME "]%s%s %pV",
> -			   prefix_pad, prefix, vaf);
> -	else
> -		printk(KERN_DEBUG "[" DRM_NAME "]%s%s %pV",
> -		       prefix_pad, prefix, vaf);
> +	__drm_dev_vprintk(dev, KERN_DEBUG, NULL, p->prefix, vaf);
>  }
>  EXPORT_SYMBOL(__drm_printfn_dbg);
>  
> @@ -277,6 +270,29 @@ void drm_print_bits(struct drm_printer *p, unsigned long value,
>  }
>  EXPORT_SYMBOL(drm_print_bits);
>  
> +void __drm_dev_vprintk(const struct device *dev, const char *level,
> +		       const void *origin, const char *prefix,
> +		       struct va_format *vaf)
> +{
> +	const char *prefix_pad = prefix ? " " : (prefix = "");

Too clever, please just keep it simple:

	const char *prefix_pad = prefix ? " " : "";

	if (!prefix)
		prefix = "";

> +
> +	if (dev)
> +		if (origin)
> +			dev_printk(level, dev, "[" DRM_NAME ":%ps]%s%s %pV",
> +				   origin, prefix_pad, prefix, vaf);
> +		else
> +			dev_printk(level, dev, "[" DRM_NAME "]%s%s %pV",
> +				   prefix_pad, prefix, vaf);
> +	else
> +		if (origin)
> +			printk("%s" "[" DRM_NAME ":%ps]%s%s %pV",
> +			       level, origin, prefix_pad, prefix, vaf);
> +		else
> +			printk("%s" "[" DRM_NAME "]%s%s %pV",
> +			       level, prefix_pad, prefix, vaf);

I'd sprinkle a few curly braces around the top level if-else blocks.

Side note, feels like using DRM_NAME makes things harder, not
easier. But that's for another patch.

> +}
> +EXPORT_SYMBOL(__drm_dev_vprintk);

AFAICT this could be a non-exported static function. And probably moved
earlier in the file to not require a declaration.

BR,
Jani.

> +
>  void drm_dev_printk(const struct device *dev, const char *level,
>  		    const char *format, ...)
>  {
> @@ -287,12 +303,7 @@ void drm_dev_printk(const struct device *dev, const char *level,
>  	vaf.fmt = format;
>  	vaf.va = &args;
>  
> -	if (dev)
> -		dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
> -			   __builtin_return_address(0), &vaf);
> -	else
> -		printk("%s" "[" DRM_NAME ":%ps] %pV",
> -		       level, __builtin_return_address(0), &vaf);
> +	__drm_dev_vprintk(dev, level, __builtin_return_address(0), NULL, &vaf);
>  
>  	va_end(args);
>  }
> @@ -312,12 +323,7 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
>  	vaf.fmt = format;
>  	vaf.va = &args;
>  
> -	if (dev)
> -		dev_printk(KERN_DEBUG, dev, "[" DRM_NAME ":%ps] %pV",
> -			   __builtin_return_address(0), &vaf);
> -	else
> -		printk(KERN_DEBUG "[" DRM_NAME ":%ps] %pV",
> -		       __builtin_return_address(0), &vaf);
> +	__drm_dev_vprintk(dev, KERN_DEBUG, __builtin_return_address(0), NULL, &vaf);
>  
>  	va_end(args);
>  }
> @@ -351,8 +357,7 @@ void __drm_err(const char *format, ...)
>  	vaf.fmt = format;
>  	vaf.va = &args;
>  
> -	printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV",
> -	       __builtin_return_address(0), &vaf);
> +	__drm_dev_vprintk(NULL, KERN_ERR, __builtin_return_address(0), "*ERROR*", &vaf);
>  
>  	va_end(args);
>  }
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 089950ad8681..bb1801c58544 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -366,6 +366,9 @@ static inline struct drm_printer drm_err_printer(struct drm_device *drm,
>  __printf(3, 4)
>  void drm_dev_printk(const struct device *dev, const char *level,
>  		    const char *format, ...);
> +void __drm_dev_vprintk(const struct device *dev, const char *level,
> +		       const void *origin, const char *prefix,
> +		       struct va_format *vaf);
>  struct _ddebug;
>  __printf(4, 5)
>  void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,

-- 
Jani Nikula, Intel

Re: [PATCH 2/3] drm/print: Improve drm_dbg_printer

[Jani Nikula]

On Fri, 17 May 2024, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> With recent introduction of a generic drm dev printk function, we
> can now store and use location where drm_dbg_printer was invoked
> and output it's symbolic name like we do for all drm debug prints.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_print.c | 3 +--
>  include/drm/drm_print.h     | 2 ++
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index a2b60c8245a1..0a205fdee7cf 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -191,8 +191,7 @@ void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf)
>  	if (!__drm_debug_enabled(category))
>  		return;
>  
> -	/* Note: __builtin_return_address(0) is useless here. */
> -	__drm_dev_vprintk(dev, KERN_DEBUG, NULL, p->prefix, vaf);
> +	__drm_dev_vprintk(dev, KERN_DEBUG, p->origin, p->prefix, vaf);
>  }
>  EXPORT_SYMBOL(__drm_printfn_dbg);
>  
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index bb1801c58544..761ce01761b7 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -175,6 +175,7 @@ struct drm_printer {
>  	void (*printfn)(struct drm_printer *p, struct va_format *vaf);
>  	void (*puts)(struct drm_printer *p, const char *str);
>  	void *arg;
> +	const void *origin;
>  	const char *prefix;
>  	enum drm_debug_category category;
>  };
> @@ -332,6 +333,7 @@ static inline struct drm_printer drm_dbg_printer(struct drm_device *drm,
>  	struct drm_printer p = {
>  		.printfn = __drm_printfn_dbg,
>  		.arg = drm,
> +		.origin = (void *)_THIS_IP_, /* it's fine as we will be inlined */

Not that it makes a difference, but I guess I'd cast to (const void *)
to match the member.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  		.prefix = prefix,
>  		.category = category,
>  	};

-- 
Jani Nikula, Intel

Re: [PATCH 3/3] drm/i915: Don't use __func__ as prefix for drm_dbg_printer

[Jani Nikula]

On Fri, 17 May 2024, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> Updated code of drm_dbg_printer() is already printing symbolic
> name of the caller like drm_dbg() does.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/gt/intel_reset.c      | 2 +-
>  drivers/gpu/drm/i915/gt/selftest_context.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 6161f7a3ff70..735cd23a43c6 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -1025,7 +1025,7 @@ void intel_gt_set_wedged(struct intel_gt *gt)
>  
>  	if (GEM_SHOW_DEBUG()) {
>  		struct drm_printer p = drm_dbg_printer(&gt->i915->drm,
> -						       DRM_UT_DRIVER, __func__);
> +						       DRM_UT_DRIVER, NULL);
>  		struct intel_engine_cs *engine;
>  		enum intel_engine_id id;
>  
> diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c b/drivers/gpu/drm/i915/gt/selftest_context.c
> index 12eca750f7d0..5eb46700dc4e 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_context.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_context.c
> @@ -286,7 +286,7 @@ static int __live_active_context(struct intel_engine_cs *engine)
>  
>  	if (intel_engine_pm_is_awake(engine)) {
>  		struct drm_printer p = drm_dbg_printer(&engine->i915->drm,
> -						       DRM_UT_DRIVER, __func__);
> +						       DRM_UT_DRIVER, NULL);
>  
>  		intel_engine_dump(engine, &p,
>  				  "%s is still awake:%d after idle-barriers\n",

-- 
Jani Nikula, Intel

✗ Fi.CI.CHECKPATCH: warning for Improve drm printer code

[Patchwork]

== Series Details ==

Series: Improve drm printer code
URL   : https://patchwork.freedesktop.org/series/133749/
State : warning

== Summary ==

Error: dim checkpatch failed
6cab5b4517bb drm/print: Add generic drm dev printk function
-:64: WARNING:PRINTK_WITHOUT_KERN_LEVEL: printk() should include KERN_<LEVEL> facility level
#64: FILE: drivers/gpu/drm/drm_print.c:288:
+			printk("%s" "[" DRM_NAME ":%ps]%s%s %pV",

-:64: WARNING:STRING_FRAGMENTS: Consecutive strings are generally better as a single string
#64: FILE: drivers/gpu/drm/drm_print.c:288:
+			printk("%s" "[" DRM_NAME ":%ps]%s%s %pV",

-:67: WARNING:PRINTK_WITHOUT_KERN_LEVEL: printk() should include KERN_<LEVEL> facility level
#67: FILE: drivers/gpu/drm/drm_print.c:291:
+			printk("%s" "[" DRM_NAME "]%s%s %pV",

-:67: WARNING:STRING_FRAGMENTS: Consecutive strings are generally better as a single string
#67: FILE: drivers/gpu/drm/drm_print.c:291:
+			printk("%s" "[" DRM_NAME "]%s%s %pV",

total: 0 errors, 4 warnings, 0 checks, 93 lines checked
a4fd3a4ec7e7 drm/print: Improve drm_dbg_printer
6b5b0966356e drm/i915: Don't use __func__ as prefix for drm_dbg_printer

✗ Fi.CI.SPARSE: warning for Improve drm printer code

[Patchwork]

== Series Details ==

Series: Improve drm printer code
URL   : https://patchwork.freedesktop.org/series/133749/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

✓ Fi.CI.BAT: success for Improve drm printer code

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

== Series Details ==

Series: Improve drm printer code
URL   : https://patchwork.freedesktop.org/series/133749/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_14781 -> Patchwork_133749v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133749v1/index.html

Participating hosts (45 -> 43)
------------------------------

  Missing    (2): fi-snb-2520m fi-kbl-8809g 

Known issues
------------

  Here are the changes found in Patchwork_133749v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@load:
    - bat-arls-3:         [PASS][1] -> [ABORT][2] ([i915#11041])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14781/bat-arls-3/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133749v1/bat-arls-3/igt@i915_module_load@load.html

  
#### Possible fixes ####

  * igt@gem_lmem_swapping@basic@lmem0:
    - bat-dg2-8:          [FAIL][3] ([i915#10378]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14781/bat-dg2-8/igt@gem_lmem_swapping@basic@lmem0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133749v1/bat-dg2-8/igt@gem_lmem_swapping@basic@lmem0.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#10378]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10378
  [i915#10435]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10435
  [i915#11041]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11041
  [i915#9157]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9157


Build changes
-------------

  * Linux: CI_DRM_14781 -> Patchwork_133749v1

  CI-20190529: 20190529
  CI_DRM_14781: d953159dab6ba6800d7316fb516ca2330dcbefe4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7860: 05b3f5540c6dcaacdf2169dc730c126df9ffd7e2 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_133749v1: d953159dab6ba6800d7316fb516ca2330dcbefe4 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133749v1/index.html

[-- Attachment #2: Type: text/html, Size: 2791 bytes --]

Re: [PATCH 1/3] drm/print: Add generic drm dev printk function

[Michal Wajdeczko]

On 17.05.2024 15:33, Jani Nikula wrote:
> On Fri, 17 May 2024, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
>> We already have some drm printk functions that need to duplicate
>> a code to get a similar format of the final result, for example:
>>
>>   [ ] 0000:00:00.0: [drm:foo] bar
>>   [ ] 0000:00:00.0: [drm] foo bar
>>   [ ] 0000:00:00.0: [drm] *ERROR* foo
>>
>> Add a generic __drm_dev_vprintk() function that can format the
>> final message like all other existing function do and allows us
>> to keep the formatting code in one place.
> 
> Nice idea!
> 
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/drm_print.c | 49 ++++++++++++++++++++-----------------
>>  include/drm/drm_print.h     |  3 +++
>>  2 files changed, 30 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
>> index cf2efb44722c..a2b60c8245a1 100644
>> --- a/drivers/gpu/drm/drm_print.c
>> +++ b/drivers/gpu/drm/drm_print.c
>> @@ -187,19 +187,12 @@ void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf)
>>  	const struct drm_device *drm = p->arg;
>>  	const struct device *dev = drm ? drm->dev : NULL;
>>  	enum drm_debug_category category = p->category;
>> -	const char *prefix = p->prefix ?: "";
>> -	const char *prefix_pad = p->prefix ? " " : "";
>>  
>>  	if (!__drm_debug_enabled(category))
>>  		return;
>>  
>>  	/* Note: __builtin_return_address(0) is useless here. */
>> -	if (dev)
>> -		dev_printk(KERN_DEBUG, dev, "[" DRM_NAME "]%s%s %pV",
>> -			   prefix_pad, prefix, vaf);
>> -	else
>> -		printk(KERN_DEBUG "[" DRM_NAME "]%s%s %pV",
>> -		       prefix_pad, prefix, vaf);
>> +	__drm_dev_vprintk(dev, KERN_DEBUG, NULL, p->prefix, vaf);
>>  }
>>  EXPORT_SYMBOL(__drm_printfn_dbg);
>>  
>> @@ -277,6 +270,29 @@ void drm_print_bits(struct drm_printer *p, unsigned long value,
>>  }
>>  EXPORT_SYMBOL(drm_print_bits);
>>  
>> +void __drm_dev_vprintk(const struct device *dev, const char *level,
>> +		       const void *origin, const char *prefix,
>> +		       struct va_format *vaf)
>> +{
>> +	const char *prefix_pad = prefix ? " " : (prefix = "");
> 
> Too clever, please just keep it simple:
> 
> 	const char *prefix_pad = prefix ? " " : "";
> 
> 	if (!prefix)
> 		prefix = "";
> 
>> +
>> +	if (dev)
>> +		if (origin)
>> +			dev_printk(level, dev, "[" DRM_NAME ":%ps]%s%s %pV",
>> +				   origin, prefix_pad, prefix, vaf);
>> +		else
>> +			dev_printk(level, dev, "[" DRM_NAME "]%s%s %pV",
>> +				   prefix_pad, prefix, vaf);
>> +	else
>> +		if (origin)
>> +			printk("%s" "[" DRM_NAME ":%ps]%s%s %pV",
>> +			       level, origin, prefix_pad, prefix, vaf);
>> +		else
>> +			printk("%s" "[" DRM_NAME "]%s%s %pV",
>> +			       level, prefix_pad, prefix, vaf);
> 
> I'd sprinkle a few curly braces around the top level if-else blocks.
> 
> Side note, feels like using DRM_NAME makes things harder, not
> easier. But that's for another patch.
> 
>> +}
>> +EXPORT_SYMBOL(__drm_dev_vprintk);
> 
> AFAICT this could be a non-exported static function. And probably moved
> earlier in the file to not require a declaration.

true for now, but I was planning to add Xe GT specific printer that
would use this function like this:

+static inline void __xe_gt_printfn_dbg(struct drm_printer *p, struct
va_format *vaf)
+{
+       struct xe_gt *gt = p->arg;
+       const struct device *dev = gt_to_xe(gt)->drm.dev;
+       char prefix[8];
+
+       if (!drm_debug_enabled(DRM_UT_DRIVER))
+               return;
+
+       /* xe_gt_dbg() callsite decorations are unhelpful */
+       snprintf(prefix, sizeof(prefix), "GT%u:", gt->info.id);
+       __drm_dev_vprintk(dev, KERN_DEBUG, p->origin, prefix, vaf);
+}
+

but I can add this new custom printer to the series right now in v2 to
show the usage and avoid any confusion

> 
> BR,
> Jani.
> 
>> +
>>  void drm_dev_printk(const struct device *dev, const char *level,
>>  		    const char *format, ...)
>>  {
>> @@ -287,12 +303,7 @@ void drm_dev_printk(const struct device *dev, const char *level,
>>  	vaf.fmt = format;
>>  	vaf.va = &args;
>>  
>> -	if (dev)
>> -		dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
>> -			   __builtin_return_address(0), &vaf);
>> -	else
>> -		printk("%s" "[" DRM_NAME ":%ps] %pV",
>> -		       level, __builtin_return_address(0), &vaf);
>> +	__drm_dev_vprintk(dev, level, __builtin_return_address(0), NULL, &vaf);
>>  
>>  	va_end(args);
>>  }
>> @@ -312,12 +323,7 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
>>  	vaf.fmt = format;
>>  	vaf.va = &args;
>>  
>> -	if (dev)
>> -		dev_printk(KERN_DEBUG, dev, "[" DRM_NAME ":%ps] %pV",
>> -			   __builtin_return_address(0), &vaf);
>> -	else
>> -		printk(KERN_DEBUG "[" DRM_NAME ":%ps] %pV",
>> -		       __builtin_return_address(0), &vaf);
>> +	__drm_dev_vprintk(dev, KERN_DEBUG, __builtin_return_address(0), NULL, &vaf);
>>  
>>  	va_end(args);
>>  }
>> @@ -351,8 +357,7 @@ void __drm_err(const char *format, ...)
>>  	vaf.fmt = format;
>>  	vaf.va = &args;
>>  
>> -	printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV",
>> -	       __builtin_return_address(0), &vaf);
>> +	__drm_dev_vprintk(NULL, KERN_ERR, __builtin_return_address(0), "*ERROR*", &vaf);
>>  
>>  	va_end(args);
>>  }
>> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
>> index 089950ad8681..bb1801c58544 100644
>> --- a/include/drm/drm_print.h
>> +++ b/include/drm/drm_print.h
>> @@ -366,6 +366,9 @@ static inline struct drm_printer drm_err_printer(struct drm_device *drm,
>>  __printf(3, 4)
>>  void drm_dev_printk(const struct device *dev, const char *level,
>>  		    const char *format, ...);
>> +void __drm_dev_vprintk(const struct device *dev, const char *level,
>> +		       const void *origin, const char *prefix,
>> +		       struct va_format *vaf);
>>  struct _ddebug;
>>  __printf(4, 5)
>>  void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
>