Re: [PATCH] drm: Add DRM_NOTICE_IF

[Dave Gordon <david.s.gordon@intel.com>]

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.

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