[Intel-gfx] [PATCH] drm: Add DRM_NOTICE_IF

[Chris Wilson]

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 at 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

-- 
Chris Wilson, Intel Open Source Technology Centre