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@xxxxxxxxxxxxxxxxxx>
---
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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
