Re: [PATCH v2 0/9] drm/print: add and use drm_debug_enabled()

[Jani Nikula <jani.nikula-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Thu, 26 Sep 2019, Eric Engestrom <eric@engestrom.ch> wrote:
> On Tuesday, 2019-09-24 15:58:56 +0300, Jani Nikula wrote:
>> Hi all, v2 of [1], a little refactoring around drm_debug access to
>> abstract it better. There shouldn't be any functional changes.
>> 
>> I'd appreciate acks for merging the lot via drm-misc. If there are any
>> objections to that, we'll need to postpone the last patch until
>> everything has been merged and converted in drm-next.
>> 
>> BR,
>> Jani.
>> 
>> Cc: Eric Engestrom <eric.engestrom@intel.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: David (ChunMing) Zhou <David1.Zhou@amd.com>
>> Cc: amd-gfx@lists.freedesktop.org
>> Cc: Ben Skeggs <bskeggs@redhat.com>
>> Cc: nouveau@lists.freedesktop.org
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Sean Paul <sean@poorly.run>
>> Cc: linux-arm-msm@vger.kernel.org
>> Cc: freedreno@lists.freedesktop.org
>> Cc: Francisco Jerez <currojerez@riseup.net>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
>> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
>> Cc: etnaviv@lists.freedesktop.org
>> 
>> 
>> [1] http://mid.mail-archive.com/cover.1568375189.git.jani.nikula@intel.com
>> 
>> Jani Nikula (9):
>>   drm/print: move drm_debug variable to drm_print.[ch]
>>   drm/print: add drm_debug_enabled()
>>   drm/i915: use drm_debug_enabled() to check for debug categories
>>   drm/print: rename drm_debug to __drm_debug to discourage use
>
> The above four patches are:
> Reviewed-by: Eric Engestrom <eric@engestrom.ch>
>
> Did you check to make sure the `unlikely()` is propagated correctly
> outside the `drm_debug_enabled()` call?

I did now.

Having drm_debug_enabled() as a macro vs. as an inline function does not
seem to make a difference, so I think the inline is clearly preferrable.

However, for example

	unlikely(foo && drm_debug & DRM_UT_DP)

does produce code different from

	(foo && drm_debug_enabled(DRM_UT_DP))

indicating that the unlikely() within drm_debug_enabled() does not
propagate to the whole condition. It's possible to retain the same
assembly output with

	(unlikely(foo) && drm_debug_enabled(DRM_UT_DP))

but it's unclear to me whether this is really worth it, either
readability or performance wise.

Thoughts?

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau