| Message ID | 1438089264-8482-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/07/15 14:14, Chris Wilson wrote: > Styled after WARN_ON/DRM_ERROR_ON, this prints a mild warning message (a > KERN_NOTICE for significant but mild events) that allows us to insert > interesting events without alarming the user or bug reporting tools. > > For an example I have changed a DRM_ERROR for being unable to set a > performance enhancement in i915. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_lrc.c | 5 ++--- > include/drm/drmP.h | 20 ++++++++++++++++++++ > 2 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 184d5f2dce21..f62cd78f8691 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1902,13 +1902,12 @@ static int gen8_init_rcs_context(struct drm_i915_gem_request *req) > if (ret) > return ret; > > - ret = intel_rcs_context_init_mocs(req); > /* > * Failing to program the MOCS is non-fatal.The system will not > * run at peak performance. So generate an error and carry on. > */ > - if (ret) > - DRM_ERROR("MOCS failed to program: expect performance issues.\n"); > + DRM_NOTICE_IF(intel_rcs_context_init_mocs(req), > + "MOCS failed to program: expect performance issues.\n"); I like the general idea of the macro, but I don't like this style of usage, specifically, embedding a function with side-effects inside the macro call. I'd much prefer ret = intel_rcs_context_init_mocs(req); DRM_NOTICE_IF(ret, "MOCS failed to program: expect performance issues.\n"); I think it's because the shouty MACRO_IN_ALL_CAPS distracts from looking at the details of the boolean thing-being-tested. Or maybe that anything that looks like a JUST_PRINT_A_MESSAGE() call should be optional, so I can delete it or comment it out without making a difference to the rest of the code - and putting important calls inside the macro invocation violates that principle. > return intel_lr_context_render_state_init(req); > } > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index b76af322d812..f2d68d185274 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -181,6 +181,26 @@ void drm_err(const char *format, ...); > }) > > /** > + * Mild warning on assertion-esque failure. > + * > + * \param cond condition on which to *fail* > + * \param fmt printf() like format string. > + * \param arg arguments > + * > + * This is similar to WARN_ON but only prints a NOTICE rather than a warning > + * and the whole stacktrace. It is only intended for mild issues which > + * while significant do not critically impact the user (such as a performance > + * issue). > + */ > +#define DRM_NOTICE_IF(cond, fmt, ...) ({ \ > + bool __cond = !!(cond); \ > + if (unlikely(__cond)) \ > + printk(KERN_NOTICE "[" DRM_NAME ":%s] " fmt, \ > + __func__, ##__VA_ARGS__); \ > + unlikely(__cond); \ > +}) Why DRM_NOTICE_IF() rather than DRM_NOTICE_ON() ? It might actually be a more sensible name but sort of loses the connection with the BUG_ON and WARN_ON macros. .Dave.
On Tue, Jul 28, 2015 at 04:58:27PM +0100, Dave Gordon wrote: > On 28/07/15 14:14, Chris Wilson wrote: > >Styled after WARN_ON/DRM_ERROR_ON, this prints a mild warning message (a > >KERN_NOTICE for significant but mild events) that allows us to insert > >interesting events without alarming the user or bug reporting tools. > > > >For an example I have changed a DRM_ERROR for being unable to set a > >performance enhancement in i915. > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >--- > > drivers/gpu/drm/i915/intel_lrc.c | 5 ++--- > > include/drm/drmP.h | 20 ++++++++++++++++++++ > > 2 files changed, 22 insertions(+), 3 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >index 184d5f2dce21..f62cd78f8691 100644 > >--- a/drivers/gpu/drm/i915/intel_lrc.c > >+++ b/drivers/gpu/drm/i915/intel_lrc.c > >@@ -1902,13 +1902,12 @@ static int gen8_init_rcs_context(struct drm_i915_gem_request *req) > > if (ret) > > return ret; > > > >- ret = intel_rcs_context_init_mocs(req); > > /* > > * Failing to program the MOCS is non-fatal.The system will not > > * run at peak performance. So generate an error and carry on. > > */ > >- if (ret) > >- DRM_ERROR("MOCS failed to program: expect performance issues.\n"); > >+ DRM_NOTICE_IF(intel_rcs_context_init_mocs(req), > >+ "MOCS failed to program: expect performance issues.\n"); > > I like the general idea of the macro, but I don't like this style of > usage, specifically, embedding a function with side-effects inside > the macro call. I'd much prefer > > ret = intel_rcs_context_init_mocs(req); > DRM_NOTICE_IF(ret, "MOCS failed to program: expect performance issues.\n"); > > I think it's because the shouty MACRO_IN_ALL_CAPS distracts from > looking at the details of the boolean thing-being-tested. Or maybe > that anything that looks like a JUST_PRINT_A_MESSAGE() call should > be optional, so I can delete it or comment it out without making a > difference to the rest of the code - and putting important calls > inside the macro invocation violates that principle. On the other hand, if it printed the condition that failed as part of the message, then the function name is much more expressive than ret. I am also not convinced that the latter is more readable than the former in this particular example. > >+#define DRM_NOTICE_IF(cond, fmt, ...) ({ \ > >+ bool __cond = !!(cond); \ > >+ if (unlikely(__cond)) \ > >+ printk(KERN_NOTICE "[" DRM_NAME ":%s] " fmt, \ > >+ __func__, ##__VA_ARGS__); \ > >+ unlikely(__cond); \ > >+}) > > Why DRM_NOTICE_IF() rather than DRM_NOTICE_ON() ? It might actually > be a more sensible name but sort of loses the connection with the > BUG_ON and WARN_ON macros. Because I was trying to avoid having it confused with the BUG_ON and the NOTICE_IF did serve to make it a gentler and milder request to do something. -Chris
On 28/07/15 14:14, Chris Wilson wrote: > Styled after WARN_ON/DRM_ERROR_ON, this prints a mild warning message (a > KERN_NOTICE for significant but mild events) that allows us to insert > interesting events without alarming the user or bug reporting tools. > Wouldn't it be better to opt for DRM_NOTICE_ON to be consistent with the other two ? It sounds fine here, although I am a non-native English speaker, so take it with a grain of salt. Thanks Emil
On 28/07/15 14:14, Chris Wilson wrote: > Styled after WARN_ON/DRM_ERROR_ON, this prints a mild warning message (a > KERN_NOTICE for significant but mild events) that allows us to insert > interesting events without alarming the user or bug reporting tools. > Wouldn't it be better to opt for DRM_NOTICE_ON to be consistent with the other two ? It sounds fine here, although I am a non-native English speaker, so take it with a grain of salt. Thanks Emil
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 184d5f2dce21..f62cd78f8691 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1902,13 +1902,12 @@ static int gen8_init_rcs_context(struct drm_i915_gem_request *req) if (ret) return ret; - ret = intel_rcs_context_init_mocs(req); /* * Failing to program the MOCS is non-fatal.The system will not * run at peak performance. So generate an error and carry on. */ - if (ret) - DRM_ERROR("MOCS failed to program: expect performance issues.\n"); + DRM_NOTICE_IF(intel_rcs_context_init_mocs(req), + "MOCS failed to program: expect performance issues.\n"); return intel_lr_context_render_state_init(req); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index b76af322d812..f2d68d185274 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -181,6 +181,26 @@ void drm_err(const char *format, ...); }) /** + * Mild warning on assertion-esque failure. + * + * \param cond condition on which to *fail* + * \param fmt printf() like format string. + * \param arg arguments + * + * This is similar to WARN_ON but only prints a NOTICE rather than a warning + * and the whole stacktrace. It is only intended for mild issues which + * while significant do not critically impact the user (such as a performance + * issue). + */ +#define DRM_NOTICE_IF(cond, fmt, ...) ({ \ + bool __cond = !!(cond); \ + if (unlikely(__cond)) \ + printk(KERN_NOTICE "[" DRM_NAME ":%s] " fmt, \ + __func__, ##__VA_ARGS__); \ + unlikely(__cond); \ +}) + +/** * Rate limited error output. Like DRM_ERROR() but won't flood the log. * * \param fmt printf() like format string.
Styled after WARN_ON/DRM_ERROR_ON, this prints a mild warning message (a KERN_NOTICE for significant but mild events) that allows us to insert interesting events without alarming the user or bug reporting tools. For an example I have changed a DRM_ERROR for being unable to set a performance enhancement in i915. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_lrc.c | 5 ++--- include/drm/drmP.h | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-)