| Message ID | 1458229571-15123-1-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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 >
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
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
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
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
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
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
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
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
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
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
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
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; }
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(-)