[PATCH] drm/i915: Tune down init error message due to failure injection

[PATCH] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

Atm, in case failure injection forces an error a subsequent error
messages will make automated tests (CI) report this event as a breakage,
even though the event is expected. To fix this print the error message
with debug log level in this case.

While at it print the error message for any init failure for
consistency.

CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 68592b0..d107871 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -66,6 +66,14 @@ bool __i915_inject_load_failure(const char *func, int line)
 	return false;
 }
 
+#define i915_load_error(fmt, ...) do {					\
+	if (i915.inject_load_failure &&					\
+	    i915_load_fail_count == i915.inject_load_failure)		\
+		DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__);			\
+	else								\
+		DRM_ERROR(fmt, ##__VA_ARGS__);				\
+} while(0)
+
 static int i915_getparam(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv)
 {
@@ -1332,10 +1340,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	}
 
 	ret = i915_load_modeset_init(dev);
-	if (ret < 0) {
-		DRM_ERROR("failed to init modeset\n");
+	if (ret < 0)
 		goto out_cleanup_vblank;
-	}
 
 	i915_driver_register(dev_priv);
 
@@ -1357,6 +1363,8 @@ out_runtime_pm_put:
 out_free_priv:
 	kfree(dev_priv);
 
+	i915_load_error("device initialization failed (%d)\n", ret);
+
 	return ret;
 }
 
-- 
2.5.0

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

Re: [PATCH] drm/i915: Tune down init error message due to failure injection

[Chris Wilson]

On Thu, Mar 17, 2016 at 04:42:12PM +0200, Imre Deak wrote:
> Atm, in case failure injection forces an error a subsequent error
> messages will make automated tests (CI) report this event as a breakage,
> even though the event is expected. To fix this print the error message
> with debug log level in this case.
> 
> While at it print the error message for any init failure for
> consistency.
> 
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 68592b0..d107871 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -66,6 +66,14 @@ bool __i915_inject_load_failure(const char *func, int line)
>  	return false;
>  }
>  
> +#define i915_load_error(fmt, ...) do {					\
> +	if (i915.inject_load_failure &&					\
> +	    i915_load_fail_count == i915.inject_load_failure)		\
> +		DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__);			\
> +	else								\
> +		DRM_ERROR(fmt, ##__VA_ARGS__);				\
> +} while(0)
> +
>  static int i915_getparam(struct drm_device *dev, void *data,
>  			 struct drm_file *file_priv)
>  {
> @@ -1332,10 +1340,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	}
>  
>  	ret = i915_load_modeset_init(dev);
> -	if (ret < 0) {
> -		DRM_ERROR("failed to init modeset\n");
> +	if (ret < 0)
>  		goto out_cleanup_vblank;
> -	}
>  
>  	i915_driver_register(dev_priv);
>  
> @@ -1357,6 +1363,8 @@ out_runtime_pm_put:
>  out_free_priv:
>  	kfree(dev_priv);
>  
> +	i915_load_error("device initialization failed (%d)\n", ret);

Please put a representative sample output into the changelog.

I think we should include a request to file bug with full drm.debug=0xf.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

Atm, in case failure injection forces an error the subsequent
"*ERROR* failed to init modeset" error message will make automated
tests (CI) report this event as a breakage even though the event is
expected. To fix this print the error message with debug log level
in this case.

While at it print the error message for any init failure for
consistency.

v2:
- Include the problematic error message in the commit log, add a
  request to file an fdo bug to the message (Chris)

CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 68592b0..b4ece9e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -66,6 +66,14 @@ bool __i915_inject_load_failure(const char *func, int line)
 	return false;
 }
 
+#define i915_load_error(fmt, ...) do {					\
+	if (i915.inject_load_failure &&					\
+	    i915_load_fail_count == i915.inject_load_failure)		\
+		DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__);			\
+	else								\
+		DRM_ERROR(fmt, ##__VA_ARGS__);				\
+} while(0)
+
 static int i915_getparam(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv)
 {
@@ -1332,10 +1340,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	}
 
 	ret = i915_load_modeset_init(dev);
-	if (ret < 0) {
-		DRM_ERROR("failed to init modeset\n");
+	if (ret < 0)
 		goto out_cleanup_vblank;
-	}
 
 	i915_driver_register(dev_priv);
 
@@ -1357,6 +1363,8 @@ out_runtime_pm_put:
 out_free_priv:
 	kfree(dev_priv);
 
+	i915_load_error("Device initialization failed (%d), please file a bug at https://bugs.freedesktop.org providing the dmesg log by booting with drm.debug=0xf\n", ret);
+
 	return ret;
 }
 
-- 
2.5.0

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

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Chris Wilson]

On Thu, Mar 17, 2016 at 05:46:11PM +0200, Imre Deak wrote:
> Atm, in case failure injection forces an error the subsequent
> "*ERROR* failed to init modeset" error message will make automated
> tests (CI) report this event as a breakage even though the event is
> expected. To fix this print the error message with debug log level
> in this case.
> 
> While at it print the error message for any init failure for
> consistency.
> 
> v2:
> - Include the problematic error message in the commit log, add a
>   request to file an fdo bug to the message (Chris)

And the new one! Helps when cross-referencing a message in dmesg.
> 
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 68592b0..b4ece9e 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -66,6 +66,14 @@ bool __i915_inject_load_failure(const char *func, int line)
>  	return false;
>  }
>  
> +#define i915_load_error(fmt, ...) do {					\
> +	if (i915.inject_load_failure &&					\
> +	    i915_load_fail_count == i915.inject_load_failure)		\
> +		DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__);			\
> +	else								\
> +		DRM_ERROR(fmt, ##__VA_ARGS__);				\
> +} while(0)
> +
>  static int i915_getparam(struct drm_device *dev, void *data,
>  			 struct drm_file *file_priv)
>  {
> @@ -1332,10 +1340,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	}
>  
>  	ret = i915_load_modeset_init(dev);
> -	if (ret < 0) {
> -		DRM_ERROR("failed to init modeset\n");
> +	if (ret < 0)
>  		goto out_cleanup_vblank;
> -	}
>  
>  	i915_driver_register(dev_priv);
>  
> @@ -1357,6 +1363,8 @@ out_runtime_pm_put:
>  out_free_priv:
>  	kfree(dev_priv);
>  
> +	i915_load_error("Device initialization failed (%d), please file a bug at https://bugs.freedesktop.org providing the dmesg log by booting with drm.debug=0xf\n", ret);

80cols rules still apply to messages :)

"Device initialization failed (%d). "
"Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi "
"against DRI/DRM/Intel providing the dmesg log by booting "
"with drm.debug=0xf\n",

I would personally make i915_load_error() a proper function and add the
"Please..." output there at a lower logging level than ERROR.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

On Thu, 2016-03-17 at 15:55 +0000, Chris Wilson wrote:
> On Thu, Mar 17, 2016 at 05:46:11PM +0200, Imre Deak wrote:
> > Atm, in case failure injection forces an error the subsequent
> > "*ERROR* failed to init modeset" error message will make automated
> > tests (CI) report this event as a breakage even though the event is
> > expected. To fix this print the error message with debug log level
> > in this case.
> > 
> > While at it print the error message for any init failure for
> > consistency.
> > 
> > v2:
> > - Include the problematic error message in the commit log, add a
> >   request to file an fdo bug to the message (Chris)
> 
> And the new one! Helps when cross-referencing a message in dmesg.
> > 
> > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c
> > index 68592b0..b4ece9e 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -66,6 +66,14 @@ bool __i915_inject_load_failure(const char
> > *func, int line)
> >  	return false;
> >  }
> >  
> > +#define i915_load_error(fmt, ...) do {				
> > 	\
> > +	if (i915.inject_load_failure &&				
> > 	\
> > +	    i915_load_fail_count == i915.inject_load_failure)	
> > 	\
> > +		DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__);		
> > 	\
> > +	else							
> > 	\
> > +		DRM_ERROR(fmt, ##__VA_ARGS__);			
> > 	\
> > +} while(0)
> > +
> >  static int i915_getparam(struct drm_device *dev, void *data,
> >  			 struct drm_file *file_priv)
> >  {
> > @@ -1332,10 +1340,8 @@ int i915_driver_load(struct drm_device *dev,
> > unsigned long flags)
> >  	}
> >  
> >  	ret = i915_load_modeset_init(dev);
> > -	if (ret < 0) {
> > -		DRM_ERROR("failed to init modeset\n");
> > +	if (ret < 0)
> >  		goto out_cleanup_vblank;
> > -	}
> >  
> >  	i915_driver_register(dev_priv);
> >  
> > @@ -1357,6 +1363,8 @@ out_runtime_pm_put:
> >  out_free_priv:
> >  	kfree(dev_priv);
> >  
> > +	i915_load_error("Device initialization failed (%d), please
> > file a bug at https://bugs.freedesktop.org providing the dmesg log
> > by booting with drm.debug=0xf\n", ret);
> 
> 80cols rules still apply to messages :)
> 
> "Device initialization failed (%d). "
> "Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi "
> "against DRI/DRM/Intel providing the dmesg log by booting "
> "with drm.debug=0xf\n",

I thought for strings emitted to dmesg it's discouraged, since you
can't easily grep then. But I can make the link more precise.

> I would personally make i915_load_error() a proper function and add
> the "Please..." output there at a lower logging level than ERROR.

Using a function is not straightforward since there is no vprintf like
interface for DRM_ERROR. But I can move that part of the message to
macro.

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

[PATCH v3] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

Atm, in case failure injection forces an error the subsequent
"*ERROR* failed to init modeset" error message will make automated
tests (CI) report this event as a breakage even though the event is
expected. To fix this print the error message with debug log level
in this case.

While at it print the error message for any init failure and change it
to "*ERROR* Device initialization failed (errno)" for consistency.

v2:
- Include the problematic error message in the commit log, add a
  request to file an fdo bug to the message (Chris)
v3:
- Include the new error message too in the commit log, make the
  fdo link more precise and print part of the message with info log
  level (Chris)

CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 68592b0..4ae16fb 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -66,6 +66,19 @@ bool __i915_inject_load_failure(const char *func, int line)
 	return false;
 }
 
+#define BUGS_FDO_URL "https://bugs.freedesktop.org/enter_bug.cgi?product=DRI"
+#define i915_load_error(fmt, ...) do {					\
+	if (i915.inject_load_failure &&					\
+	    i915_load_fail_count == i915.inject_load_failure) {		\
+		DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__);			\
+	} else {							\
+		DRM_ERROR(fmt, ##__VA_ARGS__);				\
+		DRM_INFO("Please file a bug at " BUGS_FDO_URL " "	\
+			 "against DRM/Intel providing the dmesg log "	\
+			 "by booting with drm.debug=0xf\n");		\
+	}								\
+} while(0)
+
 static int i915_getparam(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv)
 {
@@ -1332,10 +1345,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	}
 
 	ret = i915_load_modeset_init(dev);
-	if (ret < 0) {
-		DRM_ERROR("failed to init modeset\n");
+	if (ret < 0)
 		goto out_cleanup_vblank;
-	}
 
 	i915_driver_register(dev_priv);
 
@@ -1357,6 +1368,8 @@ out_runtime_pm_put:
 out_free_priv:
 	kfree(dev_priv);
 
+	i915_load_error("Device initialization failed (%d)\n", ret);
+
 	return ret;
 }
 
-- 
2.5.0

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

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Chris Wilson]

On Thu, Mar 17, 2016 at 06:08:05PM +0200, Imre Deak wrote:
> On Thu, 2016-03-17 at 15:55 +0000, Chris Wilson wrote:
> > 80cols rules still apply to messages :)
> > 
> > "Device initialization failed (%d). "
> > "Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi "
> > "against DRI/DRM/Intel providing the dmesg log by booting "
> > "with drm.debug=0xf\n",
> 
> I thought for strings emitted to dmesg it's discouraged, since you
> can't easily grep then. But I can make the link more precise.

Hmm, we haven't abided by that. I don't mind really :)
 
> > I would personally make i915_load_error() a proper function and add
> > the "Please..." output there at a lower logging level than ERROR.
> 
> Using a function is not straightforward since there is no vprintf like
> interface for DRM_ERROR. But I can move that part of the message to
> macro.

In that case, I'm happy enough with ditching the DRM_ERROR here and go
with dev_err(). Another nail in the DRM_ERROR coffin.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

On Thu, 2016-03-17 at 19:41 +0000, Chris Wilson wrote:
> On Thu, Mar 17, 2016 at 06:08:05PM +0200, Imre Deak wrote:
> > On Thu, 2016-03-17 at 15:55 +0000, Chris Wilson wrote:
> > > 80cols rules still apply to messages :)
> > > 
> > > "Device initialization failed (%d). "
> > > "Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi
> > > "
> > > "against DRI/DRM/Intel providing the dmesg log by booting "
> > > "with drm.debug=0xf\n",
> > 
> > I thought for strings emitted to dmesg it's discouraged, since you
> > can't easily grep then. But I can make the link more precise.
> 
> Hmm, we haven't abided by that. I don't mind really :)

Ok, will follow this rule then.

> > > I would personally make i915_load_error() a proper function and
> > > add
> > > the "Please..." output there at a lower logging level than ERROR.
> > 
> > Using a function is not straightforward since there is no vprintf
> > like
> > interface for DRM_ERROR. But I can move that part of the message to
> > macro.
> 
> In that case, I'm happy enough with ditching the DRM_ERROR here and
> go
> with dev_err(). Another nail in the DRM_ERROR coffin.

Hm, there doesn't seem to be vprintf like dev_err either. And dev_dbg
depends on CONFIG_DEBUG while DRM_DEBUG doesn't.

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

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Chris Wilson]

On Thu, Mar 17, 2016 at 09:50:19PM +0200, Imre Deak wrote:
> On Thu, 2016-03-17 at 19:41 +0000, Chris Wilson wrote:
> > On Thu, Mar 17, 2016 at 06:08:05PM +0200, Imre Deak wrote:
> > > On Thu, 2016-03-17 at 15:55 +0000, Chris Wilson wrote:
> > > > 80cols rules still apply to messages :)
> > > > 
> > > > "Device initialization failed (%d). "
> > > > "Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi
> > > > "
> > > > "against DRI/DRM/Intel providing the dmesg log by booting "
> > > > "with drm.debug=0xf\n",
> > > 
> > > I thought for strings emitted to dmesg it's discouraged, since you
> > > can't easily grep then. But I can make the link more precise.
> > 
> > Hmm, we haven't abided by that. I don't mind really :)
> 
> Ok, will follow this rule then.
> 
> > > > I would personally make i915_load_error() a proper function and
> > > > add
> > > > the "Please..." output there at a lower logging level than ERROR.
> > > 
> > > Using a function is not straightforward since there is no vprintf
> > > like
> > > interface for DRM_ERROR. But I can move that part of the message to
> > > macro.
> > 
> > In that case, I'm happy enough with ditching the DRM_ERROR here and
> > go
> > with dev_err(). Another nail in the DRM_ERROR coffin.
> 
> Hm, there doesn't seem to be vprintf like dev_err either. And dev_dbg
> depends on CONFIG_DEBUG while DRM_DEBUG doesn't.

It uses dev_printk_emit(KERN_ERR, dev, fmt, va_list) underneath.

Does that help?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

On Thu, 2016-03-17 at 20:44 +0000, Chris Wilson wrote:
> On Thu, Mar 17, 2016 at 09:50:19PM +0200, Imre Deak wrote:
> > On Thu, 2016-03-17 at 19:41 +0000, Chris Wilson wrote:
> > > On Thu, Mar 17, 2016 at 06:08:05PM +0200, Imre Deak wrote:
> > > > On Thu, 2016-03-17 at 15:55 +0000, Chris Wilson wrote:
> > > > > 80cols rules still apply to messages :)
> > > > > 
> > > > > "Device initialization failed (%d). "
> > > > > "Please file a bug at https://bugs.freedesktop.org/enter_bug.
> > > > > cgi
> > > > > "
> > > > > "against DRI/DRM/Intel providing the dmesg log by booting "
> > > > > "with drm.debug=0xf\n",
> > > > 
> > > > I thought for strings emitted to dmesg it's discouraged, since
> > > > you
> > > > can't easily grep then. But I can make the link more precise.
> > > 
> > > Hmm, we haven't abided by that. I don't mind really :)
> > 
> > Ok, will follow this rule then.
> > 
> > > > > I would personally make i915_load_error() a proper function
> > > > > and
> > > > > add
> > > > > the "Please..." output there at a lower logging level than
> > > > > ERROR.
> > > > 
> > > > Using a function is not straightforward since there is no
> > > > vprintf
> > > > like
> > > > interface for DRM_ERROR. But I can move that part of the
> > > > message to
> > > > macro.
> > > 
> > > In that case, I'm happy enough with ditching the DRM_ERROR here
> > > and
> > > go
> > > with dev_err(). Another nail in the DRM_ERROR coffin.
> > 
> > Hm, there doesn't seem to be vprintf like dev_err either. And
> > dev_dbg
> > depends on CONFIG_DEBUG while DRM_DEBUG doesn't.
> 
> It uses dev_printk_emit(KERN_ERR, dev, fmt, va_list) underneath.
> 
> Does that help?

It uses

__dev_printk(kern_level, dev, &vaf);

which is not exported. This one in turn translates to

dev_printk_emit(KERN_ERR[0] - '0', dev, "%s %s: %pV",
                dev_driver_string(dev), dev_name(dev), vaf);

which we probably don't want to hard-code here.


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

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Chris Wilson]

On Thu, Mar 17, 2016 at 10:53:08PM +0200, Imre Deak wrote:
> On Thu, 2016-03-17 at 20:44 +0000, Chris Wilson wrote:
> > On Thu, Mar 17, 2016 at 09:50:19PM +0200, Imre Deak wrote:
> > > On Thu, 2016-03-17 at 19:41 +0000, Chris Wilson wrote:
> > > > On Thu, Mar 17, 2016 at 06:08:05PM +0200, Imre Deak wrote:
> > > > > On Thu, 2016-03-17 at 15:55 +0000, Chris Wilson wrote:
> > > > > > 80cols rules still apply to messages :)
> > > > > > 
> > > > > > "Device initialization failed (%d). "
> > > > > > "Please file a bug at https://bugs.freedesktop.org/enter_bug.
> > > > > > cgi
> > > > > > "
> > > > > > "against DRI/DRM/Intel providing the dmesg log by booting "
> > > > > > "with drm.debug=0xf\n",
> > > > > 
> > > > > I thought for strings emitted to dmesg it's discouraged, since
> > > > > you
> > > > > can't easily grep then. But I can make the link more precise.
> > > > 
> > > > Hmm, we haven't abided by that. I don't mind really :)
> > > 
> > > Ok, will follow this rule then.
> > > 
> > > > > > I would personally make i915_load_error() a proper function
> > > > > > and
> > > > > > add
> > > > > > the "Please..." output there at a lower logging level than
> > > > > > ERROR.
> > > > > 
> > > > > Using a function is not straightforward since there is no
> > > > > vprintf
> > > > > like
> > > > > interface for DRM_ERROR. But I can move that part of the
> > > > > message to
> > > > > macro.
> > > > 
> > > > In that case, I'm happy enough with ditching the DRM_ERROR here
> > > > and
> > > > go
> > > > with dev_err(). Another nail in the DRM_ERROR coffin.
> > > 
> > > Hm, there doesn't seem to be vprintf like dev_err either. And
> > > dev_dbg
> > > depends on CONFIG_DEBUG while DRM_DEBUG doesn't.
> > 
> > It uses dev_printk_emit(KERN_ERR, dev, fmt, va_list) underneath.
> > 
> > Does that help?
> 
> It uses
> 
> __dev_printk(kern_level, dev, &vaf);
> 
> which is not exported. This one in turn translates to
> 
> dev_printk_emit(KERN_ERR[0] - '0', dev, "%s %s: %pV",
>                 dev_driver_string(dev), dev_name(dev), vaf);
> 
> which we probably don't want to hard-code here.

dev_vprintk_emit() is exported. I am not sure why dev_vprintk() would
not be...

I agree it is a nuisance. So just food for thought.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

On Thu, 2016-03-17 at 21:03 +0000, Chris Wilson wrote:
> On Thu, Mar 17, 2016 at 10:53:08PM +0200, Imre Deak wrote:
> > On Thu, 2016-03-17 at 20:44 +0000, Chris Wilson wrote:
> > > On Thu, Mar 17, 2016 at 09:50:19PM +0200, Imre Deak wrote:
> > > > On Thu, 2016-03-17 at 19:41 +0000, Chris Wilson wrote:
> > > > > On Thu, Mar 17, 2016 at 06:08:05PM +0200, Imre Deak wrote:
> > > > > > On Thu, 2016-03-17 at 15:55 +0000, Chris Wilson wrote:
> > > > > > > 80cols rules still apply to messages :)
> > > > > > > 
> > > > > > > "Device initialization failed (%d). "
> > > > > > > "Please file a bug at https://bugs.freedesktop.org/enter_
> > > > > > > bug.
> > > > > > > cgi
> > > > > > > "
> > > > > > > "against DRI/DRM/Intel providing the dmesg log by booting
> > > > > > > "
> > > > > > > "with drm.debug=0xf\n",
> > > > > > 
> > > > > > I thought for strings emitted to dmesg it's discouraged,
> > > > > > since
> > > > > > you
> > > > > > can't easily grep then. But I can make the link more
> > > > > > precise.
> > > > > 
> > > > > Hmm, we haven't abided by that. I don't mind really :)
> > > > 
> > > > Ok, will follow this rule then.
> > > > 
> > > > > > > I would personally make i915_load_error() a proper
> > > > > > > function
> > > > > > > and
> > > > > > > add
> > > > > > > the "Please..." output there at a lower logging level
> > > > > > > than
> > > > > > > ERROR.
> > > > > > 
> > > > > > Using a function is not straightforward since there is no
> > > > > > vprintf
> > > > > > like
> > > > > > interface for DRM_ERROR. But I can move that part of the
> > > > > > message to
> > > > > > macro.
> > > > > 
> > > > > In that case, I'm happy enough with ditching the DRM_ERROR
> > > > > here
> > > > > and
> > > > > go
> > > > > with dev_err(). Another nail in the DRM_ERROR coffin.
> > > > 
> > > > Hm, there doesn't seem to be vprintf like dev_err either. And
> > > > dev_dbg
> > > > depends on CONFIG_DEBUG while DRM_DEBUG doesn't.
> > > 
> > > It uses dev_printk_emit(KERN_ERR, dev, fmt, va_list) underneath.
> > > 
> > > Does that help?
> > 
> > It uses
> > 
> > __dev_printk(kern_level, dev, &vaf);
> > 
> > which is not exported. This one in turn translates to
> > 
> > dev_printk_emit(KERN_ERR[0] - '0', dev, "%s %s: %pV",
> >                 dev_driver_string(dev), dev_name(dev), vaf);
> > 
> > which we probably don't want to hard-code here.
> 
> dev_vprintk_emit() is exported. I am not sure why dev_vprintk() would
> not be...
> 
> I agree it is a nuisance. So just food for thought.

Well, I can follow-up on an export patch for that, but getting that
merged may take a while, so could we go with the current version until
that?

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

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Chris Wilson]

On Thu, Mar 17, 2016 at 11:10:42PM +0200, Imre Deak wrote:
> Well, I can follow-up on an export patch for that, but getting that
> merged may take a while, so could we go with the current version until
> that?

What's the current version? ;)

For my tastes, I really want the "please file a bug" to be separate, and
I think KERN_NOTICE. I would also like this to be the preferred
DRM_ERROR reporting mechanism i.e. anytime we emit an ERROR we should be
encouraging the user to file a bug, and do so in a user friendly fashion.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

On Thu, 2016-03-17 at 21:48 +0000, Chris Wilson wrote:
> On Thu, Mar 17, 2016 at 11:10:42PM +0200, Imre Deak wrote:
> > Well, I can follow-up on an export patch for that, but getting that
> > merged may take a while, so could we go with the current version
> > until
> > that?
> 
> What's the current version? ;)

v3. Actually you were right, there is a reason why no dev_vprintk
exists. It's because dev_printk provides that with %pV. So learned
something new today and it wasn't a nuisance.. I'll use that then and a
function instead of the macro.

> For my tastes, I really want the "please file a bug" to be separate,
> and I think KERN_NOTICE.

Ok, I used KERN_INFO but will change that to KERN_NOTICE.

> I would also like this to be the preferred
> DRM_ERROR reporting mechanism i.e. anytime we emit an ERROR we should
> be
> encouraging the user to file a bug, and do so in a user friendly
> fashion.

Ok, but only in i915 I assume. Should we also convert then all
DRM_ERROR to dev_err - with an *ERROR* prefix - or still use DRM_ERROR?

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

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Chris Wilson]

On Fri, Mar 18, 2016 at 12:09:30AM +0200, Imre Deak wrote:
> On Thu, 2016-03-17 at 21:48 +0000, Chris Wilson wrote:
> > I would also like this to be the preferred
> > DRM_ERROR reporting mechanism i.e. anytime we emit an ERROR we should
> > be
> > encouraging the user to file a bug, and do so in a user friendly
> > fashion.
> 
> Ok, but only in i915 I assume. Should we also convert then all
> DRM_ERROR to dev_err - with an *ERROR* prefix - or still use DRM_ERROR?

Within i915. I am thinking along the lines that no DRM_ERROR should
exist that doesn't acknowlege that it is a user facing error message
(i.e. written in plain English and is informative, and includes a bug
reporting reference). So i915_report_error() or somesuch.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

On Thu, 2016-03-17 at 22:14 +0000, Chris Wilson wrote:
> On Fri, Mar 18, 2016 at 12:09:30AM +0200, Imre Deak wrote:
> > On Thu, 2016-03-17 at 21:48 +0000, Chris Wilson wrote:
> > > I would also like this to be the preferred
> > > DRM_ERROR reporting mechanism i.e. anytime we emit an ERROR we
> > > should
> > > be
> > > encouraging the user to file a bug, and do so in a user friendly
> > > fashion.
> > 
> > Ok, but only in i915 I assume. Should we also convert then all
> > DRM_ERROR to dev_err - with an *ERROR* prefix - or still use
> > DRM_ERROR?
> 
> Within i915. I am thinking along the lines that no DRM_ERROR should
> exist that doesn't acknowlege that it is a user facing error message
> (i.e. written in plain English and is informative, and includes a bug
> reporting reference). So i915_report_error() or somesuch.

Ok, will give it a go.

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

[PATCH v4] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

Atm, in case failure injection forces an error the subsequent
"*ERROR* failed to init modeset" error message will make automated
tests (CI) report this event as a breakage even though the event is
expected. To fix this print the error message with debug log level
in this case.

While at it print the error message for any init failure and change it
to
"""
*ERROR* Device initialization failed (errno)
Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi?product=DRI
against DRM/Intel providing the dmesg log by booting with drm.debug=0xf
"""
and export a helper printing error messages using this same format.
A follow-up patch will convert all uses of DRM_ERROR reporting a user facing
problem to use this new helper instead.

v2:
- Include the problematic error message in the commit log, add a
  request to file an fdo bug to the message (Chris)
v3:
- Include the new error message too in the commit log, make the
  fdo link more precise and print part of the message with info log
  level (Chris)
v4: (Chris)
- Use dev_printk instead of DRM_ERROR/INFO and use NOTICE instead of
  NOTE loglevel
- Export a helper for printing user facing error messages

CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 56 +++++++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_drv.h |  6 +++++
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 68592b0..241206c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -66,6 +66,52 @@ bool __i915_inject_load_failure(const char *func, int line)
 	return false;
 }
 
+#define FDO_BUG_URL "https://bugs.freedesktop.org/enter_bug.cgi?product=DRI"
+#define FDO_BUG_MSG "Please file a bug at " FDO_BUG_URL " against DRM/Intel " \
+		    "providing the dmesg log by booting with drm.debug=0xf"
+
+void
+__i915_printk(struct drm_i915_private *dev_priv, const char *level,
+		const char *fmt, ...)
+{
+	struct device *dev = dev_priv->dev->dev;
+	bool is_error = level[1] <= KERN_ERR[1];
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	dev_printk(level, dev, "%s%pV", is_error ? "*ERROR* " : "", &vaf);
+	if (is_error)
+		dev_printk(KERN_NOTICE, dev, "%s", FDO_BUG_MSG);
+
+	va_end(args);
+}
+
+static void __printf(2, 3)
+i915_load_error(struct drm_i915_private *dev_priv, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+	bool forced_error;
+
+	forced_error = i915.inject_load_failure &&
+		       i915_load_fail_count == i915.inject_load_failure;
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	__i915_printk(dev_priv, forced_error ? KERN_DEBUG : KERN_ERR, "%pV",
+		      &vaf);
+
+	va_end(args);
+}
+
 static int i915_getparam(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv)
 {
@@ -972,8 +1018,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
 	if (i915_inject_load_failure())
 		return -ENODEV;
 
-	dev_priv->dev = dev;
-
 	/* Setup the write-once "constant" device info */
 	device_info = (struct intel_device_info *)&dev_priv->info;
 	memcpy(device_info, info, sizeof(dev_priv->info));
@@ -1303,6 +1347,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		return -ENOMEM;
 
 	dev->dev_private = dev_priv;
+	/* Must be set before calling __i915_printk */
+	dev_priv->dev = dev;
 
 	ret = i915_driver_init_early(dev_priv, dev,
 				     (struct intel_device_info *)flags);
@@ -1332,10 +1378,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	}
 
 	ret = i915_load_modeset_init(dev);
-	if (ret < 0) {
-		DRM_ERROR("failed to init modeset\n");
+	if (ret < 0)
 		goto out_cleanup_vblank;
-	}
 
 	i915_driver_register(dev_priv);
 
@@ -1357,6 +1401,8 @@ out_runtime_pm_put:
 out_free_priv:
 	kfree(dev_priv);
 
+	i915_load_error(dev_priv, "Device initialization failed (%d)\n", ret);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 00c41a4..ca79b15 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2696,6 +2696,12 @@ extern int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state);
 extern int i915_resume_switcheroo(struct drm_device *dev);
 
 /* i915_dma.c */
+void __printf(3, 4)
+__i915_printk(struct drm_i915_private *dev_priv, const char *level,
+		const char *fmt, ...);
+#define i915_report_error(dev_priv, fmt, ...) \
+	__i915_printk(dev_priv, KERN_ERR, fmt, ##__VA_ARGS__)
+
 extern int i915_driver_load(struct drm_device *, unsigned long flags);
 extern int i915_driver_unload(struct drm_device *);
 extern int i915_driver_open(struct drm_device *dev, struct drm_file *file);
-- 
2.5.0

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

[PATCH v5] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

Atm, in case failure injection forces an error the subsequent "*ERROR*
failed to init modeset" error message will make automated tests (CI)
report this event as a breakage even though the event is expected. To
fix this print the error message with debug log level in this case.

While at it print the error message for any init failure and change it
to
"""
Device initialization failed (errno)
Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi?product=DRI
against DRM/Intel providing the dmesg log by booting with drm.debug=0xf
"""
and export a helper printing error messages using this same format.
A follow-up patch will convert all uses of DRM_ERROR reporting a user
facing problem to use this new helper instead.

v2:
- Include the problematic error message in the commit log, add a
  request to file an fdo bug to the message (Chris)
v3:
- Include the new error message too in the commit log, make the
  fdo link more precise and print part of the message with info log
  level (Chris)
v4: (Chris)
- Use dev_printk instead of DRM_ERROR/INFO and use NOTICE instead of
  INFO loglevel
- Export a helper for printing user facing error messages
v5:
- Keep the DRM_ERROR message prefix used by piglit-igt/CI to filter
  relevant dmesg lines
- Use dev_notice(), instead of dev_printk(KERN_NOTICE,...)

CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 48 ++++++++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/i915_drv.h |  7 ++++++
 2 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 68592b0..096dc35 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -66,6 +66,44 @@ bool __i915_inject_load_failure(const char *func, int line)
 	return false;
 }
 
+#define FDO_BUG_URL "https://bugs.freedesktop.org/enter_bug.cgi?product=DRI"
+#define FDO_BUG_MSG "Please file a bug at " FDO_BUG_URL " against DRM/Intel " \
+		    "providing the dmesg log by booting with drm.debug=0xf"
+
+void
+__i915_printk(struct drm_i915_private *dev_priv, const char *level,
+	      const char *fmt, ...)
+{
+	struct device *dev = dev_priv->dev->dev;
+	bool is_error = level[1] <= KERN_ERR[1];
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
+		   __builtin_return_address(0), &vaf);
+
+	if (is_error)
+		dev_notice(dev, "%s", FDO_BUG_MSG);
+
+	va_end(args);
+}
+
+static bool i915_error_injected(struct drm_i915_private *dev_priv)
+{
+	return i915.inject_load_failure &&
+	       i915_load_fail_count == i915.inject_load_failure;
+}
+
+#define i915_load_error(dev_priv, fmt, ...)				     \
+	__i915_printk(dev_priv,						     \
+		      i915_error_injected(dev_priv) ? KERN_DEBUG : KERN_ERR, \
+		      fmt, ##__VA_ARGS__)
+
 static int i915_getparam(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv)
 {
@@ -972,8 +1010,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
 	if (i915_inject_load_failure())
 		return -ENODEV;
 
-	dev_priv->dev = dev;
-
 	/* Setup the write-once "constant" device info */
 	device_info = (struct intel_device_info *)&dev_priv->info;
 	memcpy(device_info, info, sizeof(dev_priv->info));
@@ -1303,6 +1339,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		return -ENOMEM;
 
 	dev->dev_private = dev_priv;
+	/* Must be set before calling __i915_printk */
+	dev_priv->dev = dev;
 
 	ret = i915_driver_init_early(dev_priv, dev,
 				     (struct intel_device_info *)flags);
@@ -1332,10 +1370,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	}
 
 	ret = i915_load_modeset_init(dev);
-	if (ret < 0) {
-		DRM_ERROR("failed to init modeset\n");
+	if (ret < 0)
 		goto out_cleanup_vblank;
-	}
 
 	i915_driver_register(dev_priv);
 
@@ -1357,6 +1393,8 @@ out_runtime_pm_put:
 out_free_priv:
 	kfree(dev_priv);
 
+	i915_load_error(dev_priv, "Device initialization failed (%d)\n", ret);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 00c41a4..e3b019a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2696,6 +2696,13 @@ extern int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state);
 extern int i915_resume_switcheroo(struct drm_device *dev);
 
 /* i915_dma.c */
+void __printf(3, 4)
+__i915_printk(struct drm_i915_private *dev_priv, const char *level,
+	      const char *fmt, ...);
+
+#define i915_report_error(dev_priv, fmt, ...)				   \
+	__i915_printk(dev_priv, KERN_ERR, fmt, ##__VA_ARGS__)
+
 extern int i915_driver_load(struct drm_device *, unsigned long flags);
 extern int i915_driver_unload(struct drm_device *);
 extern int i915_driver_open(struct drm_device *dev, struct drm_file *file);
-- 
2.5.0

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

Re: [PATCH v5] drm/i915: Tune down init error message due to failure injection

[Chris Wilson]

On Fri, Mar 18, 2016 at 08:50:37AM +0200, Imre Deak wrote:
> Atm, in case failure injection forces an error the subsequent "*ERROR*
> failed to init modeset" error message will make automated tests (CI)
> report this event as a breakage even though the event is expected. To
> fix this print the error message with debug log level in this case.
> 
> While at it print the error message for any init failure and change it
> to
> """
> Device initialization failed (errno)
> Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi?product=DRI
> against DRM/Intel providing the dmesg log by booting with drm.debug=0xf
> """
> and export a helper printing error messages using this same format.
> A follow-up patch will convert all uses of DRM_ERROR reporting a user
> facing problem to use this new helper instead.
> 
> v2:
> - Include the problematic error message in the commit log, add a
>   request to file an fdo bug to the message (Chris)
> v3:
> - Include the new error message too in the commit log, make the
>   fdo link more precise and print part of the message with info log
>   level (Chris)
> v4: (Chris)
> - Use dev_printk instead of DRM_ERROR/INFO and use NOTICE instead of
>   INFO loglevel
> - Export a helper for printing user facing error messages
> v5:
> - Keep the DRM_ERROR message prefix used by piglit-igt/CI to filter
>   relevant dmesg lines
> - Use dev_notice(), instead of dev_printk(KERN_NOTICE,...)
> 
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c | 48 ++++++++++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/i915_drv.h |  7 ++++++
>  2 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 68592b0..096dc35 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -66,6 +66,44 @@ bool __i915_inject_load_failure(const char *func, int line)
>  	return false;
>  }
>  
> +#define FDO_BUG_URL "https://bugs.freedesktop.org/enter_bug.cgi?product=DRI"
> +#define FDO_BUG_MSG "Please file a bug at " FDO_BUG_URL " against DRM/Intel " \
> +		    "providing the dmesg log by booting with drm.debug=0xf"
> +
> +void
> +__i915_printk(struct drm_i915_private *dev_priv, const char *level,
> +	      const char *fmt, ...)
> +{

static int shown_bug_once;

> +	struct device *dev = dev_priv->dev->dev;
> +	bool is_error = level[1] <= KERN_ERR[1];
> +	struct va_format vaf;
> +	va_list args;
> +
> +	va_start(args, fmt);
> +
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
> +
> +	dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
> +		   __builtin_return_address(0), &vaf);

Also on my wishlist is to
#undef DRM_NAME
#define DRM_NAME "drm/i915"
in i915_drv.h

Don't tell me that would break CI!

> +	if (is_error)

if (is_error && !shown_bug_once) {
> +		dev_notice(dev, "%s", FDO_BUG_MSG);
shown_bug_once = 1;
}

With just showing the bug once,
Reviwed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

On Fri, 2016-03-18 at 07:58 +0000, Chris Wilson wrote:
> On Fri, Mar 18, 2016 at 08:50:37AM +0200, Imre Deak wrote:
> > Atm, in case failure injection forces an error the subsequent "*ERROR*
> > failed to init modeset" error message will make automated tests (CI)
> > report this event as a breakage even though the event is expected. To
> > fix this print the error message with debug log level in this case.
> > 
> > While at it print the error message for any init failure and change it
> > to
> > """
> > Device initialization failed (errno)
> > Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi?product=DRI
> > against DRM/Intel providing the dmesg log by booting with drm.debug=0xf
> > """
> > and export a helper printing error messages using this same format.
> > A follow-up patch will convert all uses of DRM_ERROR reporting a user
> > facing problem to use this new helper instead.
> > 
> > v2:
> > - Include the problematic error message in the commit log, add a
> >   request to file an fdo bug to the message (Chris)
> > v3:
> > - Include the new error message too in the commit log, make the
> >   fdo link more precise and print part of the message with info log
> >   level (Chris)
> > v4: (Chris)
> > - Use dev_printk instead of DRM_ERROR/INFO and use NOTICE instead of
> >   INFO loglevel
> > - Export a helper for printing user facing error messages
> > v5:
> > - Keep the DRM_ERROR message prefix used by piglit-igt/CI to filter
> >   relevant dmesg lines
> > - Use dev_notice(), instead of dev_printk(KERN_NOTICE,...)
> > 
> > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c | 48 ++++++++++++++++++++++++++++++++++++-----
> >  drivers/gpu/drm/i915/i915_drv.h |  7 ++++++
> >  2 files changed, 50 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 68592b0..096dc35 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -66,6 +66,44 @@ bool __i915_inject_load_failure(const char *func, int line)
> >  	return false;
> >  }
> >  
> > +#define FDO_BUG_URL "https://bugs.freedesktop.org/enter_bug.cgi?product=DRI"
> > +#define FDO_BUG_MSG "Please file a bug at " FDO_BUG_URL " against DRM/Intel " \
> > +		    "providing the dmesg log by booting with drm.debug=0xf"
> > +
> > +void
> > +__i915_printk(struct drm_i915_private *dev_priv, const char *level,
> > +	      const char *fmt, ...)
> > +{
> 
> static int shown_bug_once;
> 
> > +	struct device *dev = dev_priv->dev->dev;
> > +	bool is_error = level[1] <= KERN_ERR[1];
> > +	struct va_format vaf;
> > +	va_list args;
> > +
> > +	va_start(args, fmt);
> > +
> > +	vaf.fmt = fmt;
> > +	vaf.va = &args;
> > +
> > +	dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
> > +		   __builtin_return_address(0), &vaf);
> 
> Also on my wishlist is to
> #undef DRM_NAME
> #define DRM_NAME "drm/i915"
> in i915_drv.h
> 
> Don't tell me that would break CI!

The current regex it uses on all messages with a loglevel <=
KERN_NOTICE:
(\[drm:|drm_|intel_|i915_)

Probably could be changed to 'i915 0000:00:02.0:' if we converted all
user facing DRM_ERRORs to use the new helper. Btw, according to your
plan the rest non-user facing DRM_ERRORs would be printed with debug
log level I assume, so isn't it a problem that they won't be treated as
test failures by CI?

> > +	if (is_error)
> 
> if (is_error && !shown_bug_once) {
> > +		dev_notice(dev, "%s", FDO_BUG_MSG);
> shown_bug_once = 1;
> }

Ok, will respin it.

--Imre

> 
> With just showing the bug once,
> Reviwed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5] drm/i915: Tune down init error message due to failure injection

[Chris Wilson]

On Fri, Mar 18, 2016 at 10:16:28AM +0200, Imre Deak wrote:
> On Fri, 2016-03-18 at 07:58 +0000, Chris Wilson wrote:
> > On Fri, Mar 18, 2016 at 08:50:37AM +0200, Imre Deak wrote:
> > > Atm, in case failure injection forces an error the subsequent "*ERROR*
> > > failed to init modeset" error message will make automated tests (CI)
> > > report this event as a breakage even though the event is expected. To
> > > fix this print the error message with debug log level in this case.
> > > 
> > > While at it print the error message for any init failure and change it
> > > to
> > > """
> > > Device initialization failed (errno)
> > > Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi?product=DRI
> > > against DRM/Intel providing the dmesg log by booting with drm.debug=0xf
> > > """
> > > and export a helper printing error messages using this same format.
> > > A follow-up patch will convert all uses of DRM_ERROR reporting a user
> > > facing problem to use this new helper instead.
> > > 
> > > v2:
> > > - Include the problematic error message in the commit log, add a
> > >   request to file an fdo bug to the message (Chris)
> > > v3:
> > > - Include the new error message too in the commit log, make the
> > >   fdo link more precise and print part of the message with info log
> > >   level (Chris)
> > > v4: (Chris)
> > > - Use dev_printk instead of DRM_ERROR/INFO and use NOTICE instead of
> > >   INFO loglevel
> > > - Export a helper for printing user facing error messages
> > > v5:
> > > - Keep the DRM_ERROR message prefix used by piglit-igt/CI to filter
> > >   relevant dmesg lines
> > > - Use dev_notice(), instead of dev_printk(KERN_NOTICE,...)
> > > 
> > > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_dma.c | 48 ++++++++++++++++++++++++++++++++++++-----
> > >  drivers/gpu/drm/i915/i915_drv.h |  7 ++++++
> > >  2 files changed, 50 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index 68592b0..096dc35 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -66,6 +66,44 @@ bool __i915_inject_load_failure(const char *func, int line)
> > >  	return false;
> > >  }
> > >  
> > > +#define FDO_BUG_URL "https://bugs.freedesktop.org/enter_bug.cgi?product=DRI"
> > > +#define FDO_BUG_MSG "Please file a bug at " FDO_BUG_URL " against DRM/Intel " \
> > > +		    "providing the dmesg log by booting with drm.debug=0xf"
> > > +
> > > +void
> > > +__i915_printk(struct drm_i915_private *dev_priv, const char *level,
> > > +	      const char *fmt, ...)
> > > +{
> > 
> > static int shown_bug_once;
> > 
> > > +	struct device *dev = dev_priv->dev->dev;
> > > +	bool is_error = level[1] <= KERN_ERR[1];
> > > +	struct va_format vaf;
> > > +	va_list args;
> > > +
> > > +	va_start(args, fmt);
> > > +
> > > +	vaf.fmt = fmt;
> > > +	vaf.va = &args;
> > > +
> > > +	dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
> > > +		   __builtin_return_address(0), &vaf);
> > 
> > Also on my wishlist is to
> > #undef DRM_NAME
> > #define DRM_NAME "drm/i915"
> > in i915_drv.h
> > 
> > Don't tell me that would break CI!
> 
> The current regex it uses on all messages with a loglevel <=
> KERN_NOTICE:
> (\[drm:|drm_|intel_|i915_)
> 
> Probably could be changed to 'i915 0000:00:02.0:' if we converted all
> user facing DRM_ERRORs to use the new helper. Btw, according to your
> plan the rest non-user facing DRM_ERRORs would be printed with debug
> log level I assume, so isn't it a problem that they won't be treated as
> test failures by CI?

If a DRM_ERROR didn't result in a user visible error then it wasn't much
of an error and CI is only complaining because we didn't keep dmesg
clean - i.e. the only bug there would be using DRM_ERROR as a debugging
aide, so yes I'm okay with that. (And if the DRM_ERROR did result in a
user visible error and the only reason CI was complaining was because of
the dmesg spam, then our testing is snafu.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

Atm, in case failure injection forces an error the subsequent "*ERROR*
failed to init modeset" error message will make automated tests (CI)
report this event as a breakage even though the event is expected. To
fix this print the error message with debug log level in this case.

While at it print the error message for any init failure and change it
to
"""
Device initialization failed (errno)
Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi?product=DRI
against DRM/Intel providing the dmesg log by booting with drm.debug=0xf
"""
and export a helper printing error messages using this same format.
A follow-up patch will convert all uses of DRM_ERROR reporting a user
facing problem to use this new helper instead.

v2:
- Include the problematic error message in the commit log, add a
  request to file an fdo bug to the message (Chris)
v3:
- Include the new error message too in the commit log, make the
  fdo link more precise and print part of the message with info log
  level (Chris)
v4: (Chris)
- Use dev_printk instead of DRM_ERROR/INFO and use NOTICE instead of
  INFO loglevel
- Export a helper for printing user facing error messages
v5:
- Keep the DRM_ERROR message prefix used by piglit-igt/CI to filter
  relevant dmesg lines
- Use dev_notice(), instead of dev_printk(KERN_NOTICE,...)
v6:
- Print the fdo bug link only once (Chris)

CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviwed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_dma.c | 51 +++++++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_drv.h |  7 ++++++
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 68592b0..147a54e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -66,6 +66,47 @@ bool __i915_inject_load_failure(const char *func, int line)
 	return false;
 }
 
+#define FDO_BUG_URL "https://bugs.freedesktop.org/enter_bug.cgi?product=DRI"
+#define FDO_BUG_MSG "Please file a bug at " FDO_BUG_URL " against DRM/Intel " \
+		    "providing the dmesg log by booting with drm.debug=0xf"
+
+void
+__i915_printk(struct drm_i915_private *dev_priv, const char *level,
+	      const char *fmt, ...)
+{
+	static bool shown_bug_once;
+	struct device *dev = dev_priv->dev->dev;
+	bool is_error = level[1] <= KERN_ERR[1];
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
+		   __builtin_return_address(0), &vaf);
+
+	if (is_error && !shown_bug_once) {
+		dev_notice(dev, "%s", FDO_BUG_MSG);
+		shown_bug_once = true;
+	}
+
+	va_end(args);
+}
+
+static bool i915_error_injected(struct drm_i915_private *dev_priv)
+{
+	return i915.inject_load_failure &&
+	       i915_load_fail_count == i915.inject_load_failure;
+}
+
+#define i915_load_error(dev_priv, fmt, ...)				     \
+	__i915_printk(dev_priv,						     \
+		      i915_error_injected(dev_priv) ? KERN_DEBUG : KERN_ERR, \
+		      fmt, ##__VA_ARGS__)
+
 static int i915_getparam(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv)
 {
@@ -972,8 +1013,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
 	if (i915_inject_load_failure())
 		return -ENODEV;
 
-	dev_priv->dev = dev;
-
 	/* Setup the write-once "constant" device info */
 	device_info = (struct intel_device_info *)&dev_priv->info;
 	memcpy(device_info, info, sizeof(dev_priv->info));
@@ -1303,6 +1342,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		return -ENOMEM;
 
 	dev->dev_private = dev_priv;
+	/* Must be set before calling __i915_printk */
+	dev_priv->dev = dev;
 
 	ret = i915_driver_init_early(dev_priv, dev,
 				     (struct intel_device_info *)flags);
@@ -1332,10 +1373,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	}
 
 	ret = i915_load_modeset_init(dev);
-	if (ret < 0) {
-		DRM_ERROR("failed to init modeset\n");
+	if (ret < 0)
 		goto out_cleanup_vblank;
-	}
 
 	i915_driver_register(dev_priv);
 
@@ -1357,6 +1396,8 @@ out_runtime_pm_put:
 out_free_priv:
 	kfree(dev_priv);
 
+	i915_load_error(dev_priv, "Device initialization failed (%d)\n", ret);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 00c41a4..e3b019a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2696,6 +2696,13 @@ extern int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state);
 extern int i915_resume_switcheroo(struct drm_device *dev);
 
 /* i915_dma.c */
+void __printf(3, 4)
+__i915_printk(struct drm_i915_private *dev_priv, const char *level,
+	      const char *fmt, ...);
+
+#define i915_report_error(dev_priv, fmt, ...)				   \
+	__i915_printk(dev_priv, KERN_ERR, fmt, ##__VA_ARGS__)
+
 extern int i915_driver_load(struct drm_device *, unsigned long flags);
 extern int i915_driver_unload(struct drm_device *);
 extern int i915_driver_open(struct drm_device *dev, struct drm_file *file);
-- 
2.5.0

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

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Joonas Lahtinen]

On pe, 2016-03-18 at 00:18 +0200, Imre Deak wrote:
> On Thu, 2016-03-17 at 22:14 +0000, Chris Wilson wrote:
> > 
> > On Fri, Mar 18, 2016 at 12:09:30AM +0200, Imre Deak wrote:
> > > 
> > > On Thu, 2016-03-17 at 21:48 +0000, Chris Wilson wrote:
> > > > 
> > > > I would also like this to be the preferred
> > > > DRM_ERROR reporting mechanism i.e. anytime we emit an ERROR we
> > > > should
> > > > be
> > > > encouraging the user to file a bug, and do so in a user friendly
> > > > fashion.
> > > Ok, but only in i915 I assume. Should we also convert then all
> > > DRM_ERROR to dev_err - with an *ERROR* prefix - or still use
> > > DRM_ERROR?
> > Within i915. I am thinking along the lines that no DRM_ERROR should
> > exist that doesn't acknowlege that it is a user facing error message
> > (i.e. written in plain English and is informative, and includes a bug
> > reporting reference). So i915_report_error() or somesuch.
> Ok, will give it a go.

Daniel didn't want i915 debugging/erroring mechanisms to deviate from
core DRM. So I guess this would follow in the same category.

I'm all in for structuring a coherent debugging/error message logic and
functions for it and then everyone can follow the suit.

Regards, Joonas

> 
> --Imre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

On Fri, 2016-03-18 at 10:59 +0200, Joonas Lahtinen wrote:
> On pe, 2016-03-18 at 00:18 +0200, Imre Deak wrote:
> > On Thu, 2016-03-17 at 22:14 +0000, Chris Wilson wrote:
> > > 
> > > On Fri, Mar 18, 2016 at 12:09:30AM +0200, Imre Deak wrote:
> > > > 
> > > > On Thu, 2016-03-17 at 21:48 +0000, Chris Wilson wrote:
> > > > > 
> > > > > I would also like this to be the preferred
> > > > > DRM_ERROR reporting mechanism i.e. anytime we emit an ERROR
> > > > > we
> > > > > should
> > > > > be
> > > > > encouraging the user to file a bug, and do so in a user
> > > > > friendly
> > > > > fashion.
> > > > Ok, but only in i915 I assume. Should we also convert then all
> > > > DRM_ERROR to dev_err - with an *ERROR* prefix - or still use
> > > > DRM_ERROR?
> > > Within i915. I am thinking along the lines that no DRM_ERROR
> > > should
> > > exist that doesn't acknowlege that it is a user facing error
> > > message
> > > (i.e. written in plain English and is informative, and includes a
> > > bug
> > > reporting reference). So i915_report_error() or somesuch.
> > Ok, will give it a go.
> 
> Daniel didn't want i915 debugging/erroring mechanisms to deviate from
> core DRM. So I guess this would follow in the same category.
> 
> I'm all in for structuring a coherent debugging/error message logic
> and
> functions for it and then everyone can follow the suit.

The dev_err/dbg method has obvious advantages, like dynamic debug or
showing the device instance, so I think that's something we want in any
case. I don't see a problem with first rolling it out in i915 then
proposing it for more generic use.

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

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Chris Wilson]

On Fri, Mar 18, 2016 at 10:59:41AM +0200, Joonas Lahtinen wrote:
> On pe, 2016-03-18 at 00:18 +0200, Imre Deak wrote:
> > On Thu, 2016-03-17 at 22:14 +0000, Chris Wilson wrote:
> > > 
> > > On Fri, Mar 18, 2016 at 12:09:30AM +0200, Imre Deak wrote:
> > > > 
> > > > On Thu, 2016-03-17 at 21:48 +0000, Chris Wilson wrote:
> > > > > 
> > > > > I would also like this to be the preferred
> > > > > DRM_ERROR reporting mechanism i.e. anytime we emit an ERROR we
> > > > > should
> > > > > be
> > > > > encouraging the user to file a bug, and do so in a user friendly
> > > > > fashion.
> > > > Ok, but only in i915 I assume. Should we also convert then all
> > > > DRM_ERROR to dev_err - with an *ERROR* prefix - or still use
> > > > DRM_ERROR?
> > > Within i915. I am thinking along the lines that no DRM_ERROR should
> > > exist that doesn't acknowlege that it is a user facing error message
> > > (i.e. written in plain English and is informative, and includes a bug
> > > reporting reference). So i915_report_error() or somesuch.
> > Ok, will give it a go.
> 
> Daniel didn't want i915 debugging/erroring mechanisms to deviate from
> core DRM. So I guess this would follow in the same category.

Too late really. We have key messages for i915 failures now being
generted by other subsystems using pr_err/dev_err, which for some
strange reason are using the core reporting mechanims not DRM's.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Tune down init error message due to failure injection (rev6)

[Patchwork]

== Series Details ==

Series: drm/i915: Tune down init error message due to failure injection (rev6)
URL   : https://patchwork.freedesktop.org/series/4577/
State : failure

== Summary ==

Series 4577v6 drm/i915: Tune down init error message due to failure injection
http://patchwork.freedesktop.org/api/1.0/series/4577/revisions/6/mbox/

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (bsw-nuc-2)
                dmesg-warn -> PASS       (skl-i5k-2)
                dmesg-warn -> PASS       (skl-nuci5)
                dmesg-warn -> PASS       (hsw-gt2)
                dmesg-warn -> PASS       (bdw-ultra)
                dmesg-warn -> PASS       (skl-i7k-2)
                dmesg-warn -> PASS       (ivb-t430s)
                dmesg-warn -> PASS       (snb-dellxps)
                dmesg-warn -> PASS       (hsw-brixbox)
                dmesg-warn -> PASS       (bdw-nuci7)
                dmesg-warn -> PASS       (ilk-hp8440p)
Test gem_mmap_gtt:
        Subgroup basic-write-gtt-no-prefault:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (bsw-nuc-2)
                pass       -> DMESG-WARN (hsw-gt2)
                dmesg-warn -> PASS       (hsw-brixbox)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (bsw-nuc-2)
                pass       -> DMESG-WARN (snb-dellxps)

bdw-nuci7        total:194  pass:182  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:194  pass:173  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:194  pass:154  dwarn:2   dfail:0   fail:1   skip:37 
byt-nuc          total:194  pass:155  dwarn:4   dfail:0   fail:0   skip:35 
hsw-brixbox      total:194  pass:171  dwarn:1   dfail:0   fail:0   skip:22 
hsw-gt2          total:194  pass:176  dwarn:1   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:194  pass:131  dwarn:0   dfail:0   fail:0   skip:63 
ivb-t430s        total:194  pass:169  dwarn:0   dfail:0   fail:0   skip:25 
skl-i5k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23 
skl-i7k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:194  pass:183  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:194  pass:158  dwarn:2   dfail:0   fail:0   skip:34 

Results at /archive/results/CI_IGT_test/Patchwork_1642/

851d270da650c16883e87bdaad7ca8b37ccbdea3 drm-intel-nightly: 2016y-03m-18d-11h-13m-55s UTC integration manifest
e8243dad16bc7ad17f08c3cacf270e3d6e551825 drm/i915: Tune down init error message due to failure injection

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

Re: ✗ Fi.CI.BAT: failure for drm/i915: Tune down init error message due to failure injection (rev6)

[Imre Deak]

On Fri, 2016-03-18 at 12:38 +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Tune down init error message due to failure
> injection (rev6)
> URL   : https://patchwork.freedesktop.org/series/4577/
> State : failure
> 
> == Summary ==
> 
> Series 4577v6 drm/i915: Tune down init error message due to failure
> injection
> http://patchwork.freedesktop.org/api/1.0/series/4577/revisions/6/mbox
> /
> 
> Test drv_module_reload_basic:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
>                 dmesg-warn -> PASS       (skl-i5k-2)
>                 dmesg-warn -> PASS       (skl-nuci5)
>                 dmesg-warn -> PASS       (hsw-gt2)
>                 dmesg-warn -> PASS       (bdw-ultra)
>                 dmesg-warn -> PASS       (skl-i7k-2)
>                 dmesg-warn -> PASS       (ivb-t430s)
>                 dmesg-warn -> PASS       (snb-dellxps)
>                 dmesg-warn -> PASS       (hsw-brixbox)
>                 dmesg-warn -> PASS       (bdw-nuci7)
>                 dmesg-warn -> PASS       (ilk-hp8440p)
> Test gem_mmap_gtt:
>         Subgroup basic-write-gtt-no-prefault:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
> Test kms_flip:
>         Subgroup basic-flip-vs-wf_vblank:
>                 pass       -> FAIL       (bsw-nuc-2)
1 frame long inter-flip ts jitter with seq-step=1:
https://bugs.freedesktop.org/show_bug.cgi?id=94294

>                 pass       -> DMESG-WARN (hsw-gt2)
Device suspended during WM programming:
https://bugs.freedesktop.org/show_bug.cgi?id=94349

>                 dmesg-warn -> PASS       (hsw-brixbox)
> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-c:
>                 pass       -> DMESG-WARN (bsw-nuc-2)
CPU hotplug lockdep WARN during GGTT programming:
https://bugs.freedesktop.org/show_bug.cgi?id=94350

> Test pm_rpm:
>         Subgroup basic-pci-d3-state:
>                 pass       -> DMESG-WARN (bsw-nuc-2)
Unclaimed reg access prior to suspending:
https://bugs.freedesktop.org/show_bug.cgi?id=94164

>         Subgroup basic-rte:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
>                 pass       -> DMESG-WARN (snb-dellxps)
Device suspended during WM programming:
https://bugs.freedesktop.org/show_bug.cgi?id=94349

> 
> bdw-
> nuci7        total:194  pass:182  dwarn:0   dfail:0   fail:0   skip:1
> 2 
> bdw-
> ultra        total:194  pass:173  dwarn:0   dfail:0   fail:0   skip:2
> 1 
> bsw-nuc-
> 2        total:194  pass:154  dwarn:2   dfail:0   fail:1   skip:37 
> byt-
> nuc          total:194  pass:155  dwarn:4   dfail:0   fail:0   skip:3
> 5 
> hsw-
> brixbox      total:194  pass:171  dwarn:1   dfail:0   fail:0   skip:2
> 2 
> hsw-
> gt2          total:194  pass:176  dwarn:1   dfail:0   fail:0   skip:1
> 7 
> ilk-
> hp8440p      total:194  pass:131  dwarn:0   dfail:0   fail:0   skip:6
> 3 
> ivb-
> t430s        total:194  pass:169  dwarn:0   dfail:0   fail:0   skip:2
> 5 
> skl-i5k-
> 2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23 
> skl-i7k-
> 2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23 
> skl-
> nuci5        total:194  pass:183  dwarn:0   dfail:0   fail:0   skip:1
> 1 
> snb-
> dellxps      total:194  pass:158  dwarn:2   dfail:0   fail:0   skip:3
> 4 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_1642/
> 
> 851d270da650c16883e87bdaad7ca8b37ccbdea3 drm-intel-nightly: 2016y-
> 03m-18d-11h-13m-55s UTC integration manifest
> e8243dad16bc7ad17f08c3cacf270e3d6e551825 drm/i915: Tune down init
> error message due to failure injection
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: failure for drm/i915: Tune down init error message due to failure injection (rev6)

[Imre Deak]

On Fri, 2016-03-18 at 15:29 +0200, Imre Deak wrote:
> On Fri, 2016-03-18 at 12:38 +0000, Patchwork wrote:
> > == Series Details ==
> > 
> > Series: drm/i915: Tune down init error message due to failure
> > injection (rev6)
> > URL   : https://patchwork.freedesktop.org/series/4577/
> > State : failure
> > 
> > == Summary ==
> > 
> > Series 4577v6 drm/i915: Tune down init error message due to failure
> > injection
> > http://patchwork.freedesktop.org/api/1.0/series/4577/revisions/6/mb
> > ox
> > /
> > 
> > Test drv_module_reload_basic:
> >                 dmesg-warn -> PASS       (bsw-nuc-2)
> >                 dmesg-warn -> PASS       (skl-i5k-2)
> >                 dmesg-warn -> PASS       (skl-nuci5)
> >                 dmesg-warn -> PASS       (hsw-gt2)
> >                 dmesg-warn -> PASS       (bdw-ultra)
> >                 dmesg-warn -> PASS       (skl-i7k-2)
> >                 dmesg-warn -> PASS       (ivb-t430s)
> >                 dmesg-warn -> PASS       (snb-dellxps)
> >                 dmesg-warn -> PASS       (hsw-brixbox)
> >                 dmesg-warn -> PASS       (bdw-nuci7)
> >                 dmesg-warn -> PASS       (ilk-hp8440p)
> > Test gem_mmap_gtt:
> >         Subgroup basic-write-gtt-no-prefault:
> >                 dmesg-warn -> PASS       (bsw-nuc-2)
> > Test kms_flip:
> >         Subgroup basic-flip-vs-wf_vblank:
> >                 pass       -> FAIL       (bsw-nuc-2)
> 1 frame long inter-flip ts jitter with seq-step=1:
> https://bugs.freedesktop.org/show_bug.cgi?id=94294
> 
> >                 pass       -> DMESG-WARN (hsw-gt2)
> Device suspended during WM programming:
> https://bugs.freedesktop.org/show_bug.cgi?id=94349
> 
> >                 dmesg-warn -> PASS       (hsw-brixbox)
> > Test kms_pipe_crc_basic:
> >         Subgroup suspend-read-crc-pipe-c:
> >                 pass       -> DMESG-WARN (bsw-nuc-2)
> CPU hotplug lockdep WARN during GGTT programming:
> https://bugs.freedesktop.org/show_bug.cgi?id=94350
> 
> > Test pm_rpm:
> >         Subgroup basic-pci-d3-state:
> >                 pass       -> DMESG-WARN (bsw-nuc-2)
> Unclaimed reg access prior to suspending:
> https://bugs.freedesktop.org/show_bug.cgi?id=94164
> 
> >         Subgroup basic-rte:
> >                 dmesg-warn -> PASS       (bsw-nuc-2)
> >                 pass       -> DMESG-WARN (snb-dellxps)
> Device suspended during WM programming:
> https://bugs.freedesktop.org/show_bug.cgi?id=94349

I pushed the patch to -dinq, thanks for the review.

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

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Daniel Vetter]

On Fri, Mar 18, 2016 at 11:15:35AM +0200, Imre Deak wrote:
> On Fri, 2016-03-18 at 10:59 +0200, Joonas Lahtinen wrote:
> > On pe, 2016-03-18 at 00:18 +0200, Imre Deak wrote:
> > > On Thu, 2016-03-17 at 22:14 +0000, Chris Wilson wrote:
> > > > 
> > > > On Fri, Mar 18, 2016 at 12:09:30AM +0200, Imre Deak wrote:
> > > > > 
> > > > > On Thu, 2016-03-17 at 21:48 +0000, Chris Wilson wrote:
> > > > > > 
> > > > > > I would also like this to be the preferred
> > > > > > DRM_ERROR reporting mechanism i.e. anytime we emit an ERROR
> > > > > > we
> > > > > > should
> > > > > > be
> > > > > > encouraging the user to file a bug, and do so in a user
> > > > > > friendly
> > > > > > fashion.
> > > > > Ok, but only in i915 I assume. Should we also convert then all
> > > > > DRM_ERROR to dev_err - with an *ERROR* prefix - or still use
> > > > > DRM_ERROR?
> > > > Within i915. I am thinking along the lines that no DRM_ERROR
> > > > should
> > > > exist that doesn't acknowlege that it is a user facing error
> > > > message
> > > > (i.e. written in plain English and is informative, and includes a
> > > > bug
> > > > reporting reference). So i915_report_error() or somesuch.
> > > Ok, will give it a go.
> > 
> > Daniel didn't want i915 debugging/erroring mechanisms to deviate from
> > core DRM. So I guess this would follow in the same category.
> > 
> > I'm all in for structuring a coherent debugging/error message logic
> > and
> > functions for it and then everyone can follow the suit.
> 
> The dev_err/dbg method has obvious advantages, like dynamic debug or
> showing the device instance, so I think that's something we want in any
> case. I don't see a problem with first rolling it out in i915 then
> proposing it for more generic use.

Well that's just silly me trying to apply some pressure into making
something that's compatible with DRM_*. Or well, porting those macros over
to whatever the newfangled fancy thing is. Including keeping drm.debug
alive with some hacks (we can write our own store functions, which could
enable the right stuff with dynamic debugging, if we export a few funcs)
so that the gazillion of howtos out there don't all break.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

On ma, 2016-03-21 at 10:28 +0100, Daniel Vetter wrote:
> On Fri, Mar 18, 2016 at 11:15:35AM +0200, Imre Deak wrote:
> > On Fri, 2016-03-18 at 10:59 +0200, Joonas Lahtinen wrote:
> > > On pe, 2016-03-18 at 00:18 +0200, Imre Deak wrote:
> > > > On Thu, 2016-03-17 at 22:14 +0000, Chris Wilson wrote:
> > > > > 
> > > > > On Fri, Mar 18, 2016 at 12:09:30AM +0200, Imre Deak wrote:
> > > > > > 
> > > > > > On Thu, 2016-03-17 at 21:48 +0000, Chris Wilson wrote:
> > > > > > > 
> > > > > > > I would also like this to be the preferred
> > > > > > > DRM_ERROR reporting mechanism i.e. anytime we emit an
> > > > > > > ERROR
> > > > > > > we
> > > > > > > should
> > > > > > > be
> > > > > > > encouraging the user to file a bug, and do so in a user
> > > > > > > friendly
> > > > > > > fashion.
> > > > > > Ok, but only in i915 I assume. Should we also convert then
> > > > > > all
> > > > > > DRM_ERROR to dev_err - with an *ERROR* prefix - or still
> > > > > > use
> > > > > > DRM_ERROR?
> > > > > Within i915. I am thinking along the lines that no DRM_ERROR
> > > > > should
> > > > > exist that doesn't acknowlege that it is a user facing error
> > > > > message
> > > > > (i.e. written in plain English and is informative, and
> > > > > includes a
> > > > > bug
> > > > > reporting reference). So i915_report_error() or somesuch.
> > > > Ok, will give it a go.
> > > 
> > > Daniel didn't want i915 debugging/erroring mechanisms to deviate
> > > from
> > > core DRM. So I guess this would follow in the same category.
> > > 
> > > I'm all in for structuring a coherent debugging/error message
> > > logic
> > > and
> > > functions for it and then everyone can follow the suit.
> > 
> > The dev_err/dbg method has obvious advantages, like dynamic debug
> > or
> > showing the device instance, so I think that's something we want in
> > any
> > case. I don't see a problem with first rolling it out in i915 then
> > proposing it for more generic use.
> 
> Well that's just silly me trying to apply some pressure into making
> something that's compatible with DRM_*. Or well, porting those macros
> over
> to whatever the newfangled fancy thing is. Including keeping
> drm.debug
> alive with some hacks (we can write our own store functions, which
> could
> enable the right stuff with dynamic debugging, if we export a few
> funcs)
> so that the gazillion of howtos out there don't all break.

Yea, currently we'd output debug messages even in case of drm.debug==0.
I sent a patch that makes __i915_printk() behave the same as
DRM_DEBUG_DRIVER for debug messages. I think on top of that a more
fancy dynamic debug based filtering can be implemented later as you
suggest.

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

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Joonas Lahtinen]

On ma, 2016-03-21 at 17:12 +0200, Imre Deak wrote:
> On ma, 2016-03-21 at 10:28 +0100, Daniel Vetter wrote:
> > 
> > On Fri, Mar 18, 2016 at 11:15:35AM +0200, Imre Deak wrote:
> > > 
> > > On Fri, 2016-03-18 at 10:59 +0200, Joonas Lahtinen wrote:
> > > > 
> > > > On pe, 2016-03-18 at 00:18 +0200, Imre Deak wrote:
> > > > > 
> > > > > On Thu, 2016-03-17 at 22:14 +0000, Chris Wilson wrote:
> > > > > > 
> > > > > > 
> > > > > > On Fri, Mar 18, 2016 at 12:09:30AM +0200, Imre Deak wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On Thu, 2016-03-17 at 21:48 +0000, Chris Wilson wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > I would also like this to be the preferred
> > > > > > > > DRM_ERROR reporting mechanism i.e. anytime we emit an
> > > > > > > > ERROR
> > > > > > > > we
> > > > > > > > should
> > > > > > > > be
> > > > > > > > encouraging the user to file a bug, and do so in a user
> > > > > > > > friendly
> > > > > > > > fashion.
> > > > > > > Ok, but only in i915 I assume. Should we also convert then
> > > > > > > all
> > > > > > > DRM_ERROR to dev_err - with an *ERROR* prefix - or still
> > > > > > > use
> > > > > > > DRM_ERROR?
> > > > > > Within i915. I am thinking along the lines that no DRM_ERROR
> > > > > > should
> > > > > > exist that doesn't acknowlege that it is a user facing error
> > > > > > message
> > > > > > (i.e. written in plain English and is informative, and
> > > > > > includes a
> > > > > > bug
> > > > > > reporting reference). So i915_report_error() or somesuch.
> > > > > Ok, will give it a go.
> > > > Daniel didn't want i915 debugging/erroring mechanisms to deviate
> > > > from
> > > > core DRM. So I guess this would follow in the same category.
> > > > 
> > > > I'm all in for structuring a coherent debugging/error message
> > > > logic
> > > > and
> > > > functions for it and then everyone can follow the suit.
> > > The dev_err/dbg method has obvious advantages, like dynamic debug
> > > or
> > > showing the device instance, so I think that's something we want in
> > > any
> > > case. I don't see a problem with first rolling it out in i915 then
> > > proposing it for more generic use.
> > Well that's just silly me trying to apply some pressure into making
> > something that's compatible with DRM_*. Or well, porting those macros
> > over
> > to whatever the newfangled fancy thing is. Including keeping
> > drm.debug
> > alive with some hacks (we can write our own store functions, which
> > could
> > enable the right stuff with dynamic debugging, if we export a few
> > funcs)
> > so that the gazillion of howtos out there don't all break.
> Yea, currently we'd output debug messages even in case of drm.debug==0.
> I sent a patch that makes __i915_printk() behave the same as
> DRM_DEBUG_DRIVER for debug messages. I think on top of that a more
> fancy dynamic debug based filtering can be implemented later as you
> suggest.
> 

Yeah, when dynamic debugging is disabled drm.debug==0 would produce all
the debug, that's the biggest problem I hit. Over-verbosity.

I have the drm.debug working in dynamic debugging cases already (I
exposed one interface from kernel and tadah).

Problem is only, if we want to have drm.debug doing its filtering thing
for the old and new code in transitino phase when dynamic debugging is
disabled. Then we'll have to go little bit further and do a few #undefs
(but we currently have those, too), because dynamic debugging also
makes itself add the line numbers and file names to allow filtering by
those.

I copy-pasted my changes at the end so you get a rough idea.

Regards, Joonas

> --Imre
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

------------------------------8<-----------------------------------
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 167c8d3..da818a0 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -32,6 +32,7 @@
 #include <linux/moduleparam.h>
 #include <linux/mount.h>
 #include <linux/slab.h>
+#include <linux/dynamic_debug.h>
 #include <drm/drmP.h>
 #include <drm/drm_core.h>
 #include "drm_legacy.h"
@@ -43,8 +44,11 @@ EXPORT_SYMBOL(drm_debug);
 MODULE_AUTHOR(CORE_AUTHOR);
 MODULE_DESCRIPTION(CORE_DESC);
 MODULE_LICENSE("GPL and additional rights");
+
+#ifdef CONFIG_DYNAMIC_DEBUG
 MODULE_PARM_DESC(debug, "Enable debug output");
 module_param_named(debug, drm_debug, int, 0600);
+#endif
 
 static DEFINE_SPINLOCK(drm_minor_lock);
 static struct idr drm_minors_idr;
@@ -61,28 +65,13 @@ void drm_err(const char *format, ...)
 	vaf.fmt = format;
 	vaf.va = &args;
 
-	printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV",
+	printk(KERN_ERR DRM_NAME ": [%ps] *ERROR* %pV",
 	       __builtin_return_address(0), &vaf);
 
 	va_end(args);
 }
 EXPORT_SYMBOL(drm_err);
 
-void drm_ut_debug_printk(const char *function_name, const char *format, ...)
-{
-	struct va_format vaf;
-	va_list args;
-
-	va_start(args, format);
-	vaf.fmt = format;
-	vaf.va = &args;
-
-	printk(KERN_DEBUG "[" DRM_NAME ":%s] %pV", function_name, &vaf);
-
-	va_end(args);
-}
-EXPORT_SYMBOL(drm_ut_debug_printk);
-
 struct drm_master *drm_master_create(struct drm_minor *minor)
 {
 	struct drm_master *master;
@@ -879,10 +868,21 @@ static const struct file_operations drm_stub_fops = {
 	.llseek = noop_llseek,
 };
 
+static void drm_debug_init(void)
+{
+	struct ddebug_query query = { };
+	unsigned int flags = _DPRINTK_FLAGS_PRINT | _DPRINTK_FLAGS_INCL_FUNCNAME;
+	unsigned int mask = ~_DPRINTK_FLAGS_PRINT;
+
+	query.format = DRM_NAME ": " DRM_PREFIX_CORE ": ";
+	ddebug_change(&query, drm_debug & DRM_UT_CORE ? flags : 0, mask);
+}
+
 static int __init drm_core_init(void)
 {
 	int ret = -ENOMEM;
 
+	drm_debug_init();
 	drm_global_init();
 	drm_connector_ida_init();
 	idr_init(&drm_minors_idr);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3c8422c..628f27e 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -130,9 +130,13 @@ struct dma_buf_attachment;
 #define DRM_UT_ATOMIC		0x10
 #define DRM_UT_VBL		0x20
 
-extern __printf(2, 3)
-void drm_ut_debug_printk(const char *function_name,
-			 const char *format, ...);
+#define DRM_PREFIX_CORE		"core"
+#define DRM_PREFIX_DRIVER	"driver"
+#define DRM_PREFIX_KMS		"kms"
+#define DRM_PREFIX_PRIME	"prime"
+#define DRM_PREFIX_ATOMIC	"atomic"
+#define DRM_PREFIX_VBL		"vblank"
+
 extern __printf(1, 2)
 void drm_err(const char *format, ...);
 
@@ -184,48 +188,34 @@ void drm_err(const char *format, ...);
 })
 
 #define DRM_INFO(fmt, ...)				\
-	printk(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
+	printk(KERN_INFO DRM_NAME ": " fmt, ##__VA_ARGS__)
 
 #define DRM_INFO_ONCE(fmt, ...)				\
-	printk_once(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
+	printk_once(KERN_INFO DRM_NAME ": " fmt, ##__VA_ARGS__)
 
 /**
  * Debug output.
  *
  * \param fmt printf() like format string.
- * \param arg arguments
+ * \param args arguments
  */
-#define DRM_DEBUG(fmt, args...)						\
-	do {								\
-		if (unlikely(drm_debug & DRM_UT_CORE))			\
-			drm_ut_debug_printk(__func__, fmt, ##args);	\
-	} while (0)
-
-#define DRM_DEBUG_DRIVER(fmt, args...)					\
-	do {								\
-		if (unlikely(drm_debug & DRM_UT_DRIVER))		\
-			drm_ut_debug_printk(__func__, fmt, ##args);	\
-	} while (0)
-#define DRM_DEBUG_KMS(fmt, args...)					\
-	do {								\
-		if (unlikely(drm_debug & DRM_UT_KMS))			\
-			drm_ut_debug_printk(__func__, fmt, ##args);	\
-	} while (0)
-#define DRM_DEBUG_PRIME(fmt, args...)					\
-	do {								\
-		if (unlikely(drm_debug & DRM_UT_PRIME))			\
-			drm_ut_debug_printk(__func__, fmt, ##args);	\
-	} while (0)
-#define DRM_DEBUG_ATOMIC(fmt, args...)					\
-	do {								\
-		if (unlikely(drm_debug & DRM_UT_ATOMIC))		\
-			drm_ut_debug_printk(__func__, fmt, ##args);	\
-	} while (0)
-#define DRM_DEBUG_VBL(fmt, args...)					\
-	do {								\
-		if (unlikely(drm_debug & DRM_UT_VBL))			\
-			drm_ut_debug_printk(__func__, fmt, ##args);	\
-	} while (0)
+#define DRM_DEBUG(fmt, args...) \
+	pr_debug(DRM_NAME ": " DRM_PREFIX_CORE ": " fmt, ##args)
+
+#define DRM_DEBUG_DRIVER(fmt, args...) \
+	pr_debug(DRM_NAME ": " DRM_PREFIX_DRIVER ": " fmt, ##args)
+
+#define DRM_DEBUG_KMS(fmt, args...) \
+	pr_debug(DRM_NAME ": " DRM_PREFIX_KMS ": " fmt, ##args)
+
+#define DRM_DEBUG_PRIME(fmt, args...) \
+	pr_debug(DRM_NAME ": " DRM_PREFIX_PRIME ": " fmt, ##args)
+
+#define DRM_DEBUG_ATOMIC(fmt, args...) \
+	pr_debug(DRM_NAME ": " DRM_PREFIX_ATOMIC ": " fmt, ##args)
+
+#define DRM_DEBUG_VBL(fmt, args...) \
+	pr_debug(DRM_NAME ": " DRM_PREFIX_VBL ": " fmt, ##args)
 
 /*@}*/
 
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 4f1bbc6..beae2df 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -1,6 +1,14 @@
 #ifndef _DYNAMIC_DEBUG_H
 #define _DYNAMIC_DEBUG_H
 
+struct ddebug_query {
+	const char *filename;
+	const char *module;
+	const char *function;
+	const char *format;
+	unsigned int first_lineno, last_lineno;
+};
+
 /*
  * An instance of this structure is created in a special
  * ELF section at every dynamic debug callsite.  At runtime,
@@ -40,6 +48,8 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
 				const char *modname);
 
 #if defined(CONFIG_DYNAMIC_DEBUG)
+extern int ddebug_change(const struct ddebug_query *query,
+			 unsigned int flags, unsigned int mask);
 extern int ddebug_remove_module(const char *mod_name);
 extern __printf(2, 3)
 void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...);
@@ -111,6 +121,12 @@ do {								\
 #include <linux/string.h>
 #include <linux/errno.h>
 
+static inline int ddebug_change(const struct ddebug_query *query,
+				unsigned int flags, unsigned int mask)
+{
+	return 0;
+}
+
 static inline int ddebug_remove_module(const char *mod)
 {
 	return 0;
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index fe42b6e..cbe7b07 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -47,14 +47,6 @@ struct ddebug_table {
 	struct _ddebug *ddebugs;
 };
 
-struct ddebug_query {
-	const char *filename;
-	const char *module;
-	const char *function;
-	const char *format;
-	unsigned int first_lineno, last_lineno;
-};
-
 struct ddebug_iter {
 	struct ddebug_table *table;
 	unsigned int idx;
@@ -135,8 +127,8 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
  * callsites, normally the same as number of changes.  If verbose,
  * logs the changes.  Takes ddebug_lock.
  */
-static int ddebug_change(const struct ddebug_query *query,
-			unsigned int flags, unsigned int mask)
+int ddebug_change(const struct ddebug_query *query,
+		  unsigned int flags, unsigned int mask)
 {
 	int i;
 	struct ddebug_table *dt;
@@ -203,6 +195,7 @@ static int ddebug_change(const struct ddebug_query *query,
 
 	return nfound;
 }
+EXPORT_SYMBOL(ddebug_change);
 
 /*
  * Split the buffer `buf' into space-separated words.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

[Daniel Vetter]

On Thu, Mar 24, 2016 at 03:11:48PM +0200, Joonas Lahtinen wrote:
> On ma, 2016-03-21 at 17:12 +0200, Imre Deak wrote:
> > On ma, 2016-03-21 at 10:28 +0100, Daniel Vetter wrote:
> > > 
> > > On Fri, Mar 18, 2016 at 11:15:35AM +0200, Imre Deak wrote:
> > > > 
> > > > On Fri, 2016-03-18 at 10:59 +0200, Joonas Lahtinen wrote:
> > > > > 
> > > > > On pe, 2016-03-18 at 00:18 +0200, Imre Deak wrote:
> > > > > > 
> > > > > > On Thu, 2016-03-17 at 22:14 +0000, Chris Wilson wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On Fri, Mar 18, 2016 at 12:09:30AM +0200, Imre Deak wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Thu, 2016-03-17 at 21:48 +0000, Chris Wilson wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > I would also like this to be the preferred
> > > > > > > > > DRM_ERROR reporting mechanism i.e. anytime we emit an
> > > > > > > > > ERROR
> > > > > > > > > we
> > > > > > > > > should
> > > > > > > > > be
> > > > > > > > > encouraging the user to file a bug, and do so in a user
> > > > > > > > > friendly
> > > > > > > > > fashion.
> > > > > > > > Ok, but only in i915 I assume. Should we also convert then
> > > > > > > > all
> > > > > > > > DRM_ERROR to dev_err - with an *ERROR* prefix - or still
> > > > > > > > use
> > > > > > > > DRM_ERROR?
> > > > > > > Within i915. I am thinking along the lines that no DRM_ERROR
> > > > > > > should
> > > > > > > exist that doesn't acknowlege that it is a user facing error
> > > > > > > message
> > > > > > > (i.e. written in plain English and is informative, and
> > > > > > > includes a
> > > > > > > bug
> > > > > > > reporting reference). So i915_report_error() or somesuch.
> > > > > > Ok, will give it a go.
> > > > > Daniel didn't want i915 debugging/erroring mechanisms to deviate
> > > > > from
> > > > > core DRM. So I guess this would follow in the same category.
> > > > > 
> > > > > I'm all in for structuring a coherent debugging/error message
> > > > > logic
> > > > > and
> > > > > functions for it and then everyone can follow the suit.
> > > > The dev_err/dbg method has obvious advantages, like dynamic debug
> > > > or
> > > > showing the device instance, so I think that's something we want in
> > > > any
> > > > case. I don't see a problem with first rolling it out in i915 then
> > > > proposing it for more generic use.
> > > Well that's just silly me trying to apply some pressure into making
> > > something that's compatible with DRM_*. Or well, porting those macros
> > > over
> > > to whatever the newfangled fancy thing is. Including keeping
> > > drm.debug
> > > alive with some hacks (we can write our own store functions, which
> > > could
> > > enable the right stuff with dynamic debugging, if we export a few
> > > funcs)
> > > so that the gazillion of howtos out there don't all break.
> > Yea, currently we'd output debug messages even in case of drm.debug==0.
> > I sent a patch that makes __i915_printk() behave the same as
> > DRM_DEBUG_DRIVER for debug messages. I think on top of that a more
> > fancy dynamic debug based filtering can be implemented later as you
> > suggest.
> > 
> 
> Yeah, when dynamic debugging is disabled drm.debug==0 would produce all
> the debug, that's the biggest problem I hit. Over-verbosity.
> 
> I have the drm.debug working in dynamic debugging cases already (I
> exposed one interface from kernel and tadah).
> 
> Problem is only, if we want to have drm.debug doing its filtering thing
> for the old and new code in transitino phase when dynamic debugging is
> disabled. Then we'll have to go little bit further and do a few #undefs
> (but we currently have those, too), because dynamic debugging also
> makes itself add the line numbers and file names to allow filtering by
> those.
> 
> I copy-pasted my changes at the end so you get a rough idea.

Just very quick pass over it, 2 comments below.
-Danil

> 
> Regards, Joonas
> 
> > --Imre
> -- 
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
> 
> ------------------------------8<-----------------------------------
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 167c8d3..da818a0 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -32,6 +32,7 @@
>  #include <linux/moduleparam.h>
>  #include <linux/mount.h>
>  #include <linux/slab.h>
> +#include <linux/dynamic_debug.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_core.h>
>  #include "drm_legacy.h"
> @@ -43,8 +44,11 @@ EXPORT_SYMBOL(drm_debug);
>  MODULE_AUTHOR(CORE_AUTHOR);
>  MODULE_DESCRIPTION(CORE_DESC);
>  MODULE_LICENSE("GPL and additional rights");
> +
> +#ifdef CONFIG_DYNAMIC_DEBUG
>  MODULE_PARM_DESC(debug, "Enable debug output");
>  module_param_named(debug, drm_debug, int, 0600);
> +#endif
>  
>  static DEFINE_SPINLOCK(drm_minor_lock);
>  static struct idr drm_minors_idr;
> @@ -61,28 +65,13 @@ void drm_err(const char *format, ...)
>  	vaf.fmt = format;
>  	vaf.va = &args;
>  
> -	printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV",
> +	printk(KERN_ERR DRM_NAME ": [%ps] *ERROR* %pV",
>  	       __builtin_return_address(0), &vaf);
>  
>  	va_end(args);
>  }
>  EXPORT_SYMBOL(drm_err);
>  
> -void drm_ut_debug_printk(const char *function_name, const char *format, ...)
> -{
> -	struct va_format vaf;
> -	va_list args;
> -
> -	va_start(args, format);
> -	vaf.fmt = format;
> -	vaf.va = &args;
> -
> -	printk(KERN_DEBUG "[" DRM_NAME ":%s] %pV", function_name, &vaf);
> -
> -	va_end(args);
> -}
> -EXPORT_SYMBOL(drm_ut_debug_printk);
> -
>  struct drm_master *drm_master_create(struct drm_minor *minor)
>  {
>  	struct drm_master *master;
> @@ -879,10 +868,21 @@ static const struct file_operations drm_stub_fops = {
>  	.llseek = noop_llseek,
>  };
>  
> +static void drm_debug_init(void)
> +{
> +	struct ddebug_query query = { };
> +	unsigned int flags = _DPRINTK_FLAGS_PRINT | _DPRINTK_FLAGS_INCL_FUNCNAME;
> +	unsigned int mask = ~_DPRINTK_FLAGS_PRINT;
> +
> +	query.format = DRM_NAME ": " DRM_PREFIX_CORE ": ";
> +	ddebug_change(&query, drm_debug & DRM_UT_CORE ? flags : 0, mask);
> +}

Shouldn't we put this into the store function for the module paramater, so
that changing at runtime still works? Personally I adjust drm.debug a lot
in a bunch of scripts I have lying around, I guess others do the same.

> +
>  static int __init drm_core_init(void)
>  {
>  	int ret = -ENOMEM;
>  
> +	drm_debug_init();
>  	drm_global_init();
>  	drm_connector_ida_init();
>  	idr_init(&drm_minors_idr);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3c8422c..628f27e 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -130,9 +130,13 @@ struct dma_buf_attachment;
>  #define DRM_UT_ATOMIC		0x10
>  #define DRM_UT_VBL		0x20
>  
> -extern __printf(2, 3)
> -void drm_ut_debug_printk(const char *function_name,
> -			 const char *format, ...);
> +#define DRM_PREFIX_CORE		"core"
> +#define DRM_PREFIX_DRIVER	"driver"
> +#define DRM_PREFIX_KMS		"kms"
> +#define DRM_PREFIX_PRIME	"prime"
> +#define DRM_PREFIX_ATOMIC	"atomic"
> +#define DRM_PREFIX_VBL		"vblank"
> +
>  extern __printf(1, 2)
>  void drm_err(const char *format, ...);
>  
> @@ -184,48 +188,34 @@ void drm_err(const char *format, ...);
>  })
>  
>  #define DRM_INFO(fmt, ...)				\
> -	printk(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
> +	printk(KERN_INFO DRM_NAME ": " fmt, ##__VA_ARGS__)
>  
>  #define DRM_INFO_ONCE(fmt, ...)				\
> -	printk_once(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
> +	printk_once(KERN_INFO DRM_NAME ": " fmt, ##__VA_ARGS__)
>  
>  /**
>   * Debug output.
>   *
>   * \param fmt printf() like format string.
> - * \param arg arguments
> + * \param args arguments
>   */
> -#define DRM_DEBUG(fmt, args...)						\
> -	do {								\
> -		if (unlikely(drm_debug & DRM_UT_CORE))			\
> -			drm_ut_debug_printk(__func__, fmt, ##args);	\
> -	} while (0)
> -
> -#define DRM_DEBUG_DRIVER(fmt, args...)					\
> -	do {								\
> -		if (unlikely(drm_debug & DRM_UT_DRIVER))		\
> -			drm_ut_debug_printk(__func__, fmt, ##args);	\
> -	} while (0)
> -#define DRM_DEBUG_KMS(fmt, args...)					\
> -	do {								\
> -		if (unlikely(drm_debug & DRM_UT_KMS))			\
> -			drm_ut_debug_printk(__func__, fmt, ##args);	\
> -	} while (0)
> -#define DRM_DEBUG_PRIME(fmt, args...)					\
> -	do {								\
> -		if (unlikely(drm_debug & DRM_UT_PRIME))			\
> -			drm_ut_debug_printk(__func__, fmt, ##args);	\
> -	} while (0)
> -#define DRM_DEBUG_ATOMIC(fmt, args...)					\
> -	do {								\
> -		if (unlikely(drm_debug & DRM_UT_ATOMIC))		\
> -			drm_ut_debug_printk(__func__, fmt, ##args);	\
> -	} while (0)
> -#define DRM_DEBUG_VBL(fmt, args...)					\
> -	do {								\
> -		if (unlikely(drm_debug & DRM_UT_VBL))			\
> -			drm_ut_debug_printk(__func__, fmt, ##args);	\
> -	} while (0)
> +#define DRM_DEBUG(fmt, args...) \
> +	pr_debug(DRM_NAME ": " DRM_PREFIX_CORE ": " fmt, ##args)
> +
> +#define DRM_DEBUG_DRIVER(fmt, args...) \
> +	pr_debug(DRM_NAME ": " DRM_PREFIX_DRIVER ": " fmt, ##args)
> +
> +#define DRM_DEBUG_KMS(fmt, args...) \
> +	pr_debug(DRM_NAME ": " DRM_PREFIX_KMS ": " fmt, ##args)
> +
> +#define DRM_DEBUG_PRIME(fmt, args...) \
> +	pr_debug(DRM_NAME ": " DRM_PREFIX_PRIME ": " fmt, ##args)
> +
> +#define DRM_DEBUG_ATOMIC(fmt, args...) \
> +	pr_debug(DRM_NAME ": " DRM_PREFIX_ATOMIC ": " fmt, ##args)
> +
> +#define DRM_DEBUG_VBL(fmt, args...) \
> +	pr_debug(DRM_NAME ": " DRM_PREFIX_VBL ": " fmt, ##args)

Yeah, I think we need to have an

#ifndef CONFIG_DYNAMIC_DEBUG
/* old debug macros using drm.debug filtering */
#else
/* new ddebug based macros */
#endif

to address the issues you've raised.

>  
>  /*@}*/
>  
> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> index 4f1bbc6..beae2df 100644
> --- a/include/linux/dynamic_debug.h
> +++ b/include/linux/dynamic_debug.h
> @@ -1,6 +1,14 @@
>  #ifndef _DYNAMIC_DEBUG_H
>  #define _DYNAMIC_DEBUG_H
>  
> +struct ddebug_query {
> +	const char *filename;
> +	const char *module;
> +	const char *function;
> +	const char *format;
> +	unsigned int first_lineno, last_lineno;
> +};
> +
>  /*
>   * An instance of this structure is created in a special
>   * ELF section at every dynamic debug callsite.  At runtime,
> @@ -40,6 +48,8 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
>  				const char *modname);
>  
>  #if defined(CONFIG_DYNAMIC_DEBUG)
> +extern int ddebug_change(const struct ddebug_query *query,
> +			 unsigned int flags, unsigned int mask);
>  extern int ddebug_remove_module(const char *mod_name);
>  extern __printf(2, 3)
>  void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...);
> @@ -111,6 +121,12 @@ do {								\
>  #include <linux/string.h>
>  #include <linux/errno.h>
>  
> +static inline int ddebug_change(const struct ddebug_query *query,
> +				unsigned int flags, unsigned int mask)
> +{
> +	return 0;
> +}
> +
>  static inline int ddebug_remove_module(const char *mod)
>  {
>  	return 0;
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index fe42b6e..cbe7b07 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -47,14 +47,6 @@ struct ddebug_table {
>  	struct _ddebug *ddebugs;
>  };
>  
> -struct ddebug_query {
> -	const char *filename;
> -	const char *module;
> -	const char *function;
> -	const char *format;
> -	unsigned int first_lineno, last_lineno;
> -};
> -
>  struct ddebug_iter {
>  	struct ddebug_table *table;
>  	unsigned int idx;
> @@ -135,8 +127,8 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
>   * callsites, normally the same as number of changes.  If verbose,
>   * logs the changes.  Takes ddebug_lock.
>   */
> -static int ddebug_change(const struct ddebug_query *query,
> -			unsigned int flags, unsigned int mask)
> +int ddebug_change(const struct ddebug_query *query,
> +		  unsigned int flags, unsigned int mask)
>  {
>  	int i;
>  	struct ddebug_table *dt;
> @@ -203,6 +195,7 @@ static int ddebug_change(const struct ddebug_query *query,
>  
>  	return nfound;
>  }
> +EXPORT_SYMBOL(ddebug_change);
>  
>  /*
>   * Split the buffer `buf' into space-separated words.

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx