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

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 01 Oct 2019, Eric Engestrom <eric@engestrom.ch> wrote:
> On Tuesday, 2019-10-01 14:03:55 +0300, Jani Nikula wrote:
>> 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.
>
> Agreed :)
>
>> 
>> 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?
>
> That kind of code only happens 2 times, both in
> drivers/gpu/drm/drm_dp_mst_topology.c (in patch 2/9), right?
>
> I think your suggestion is the right thing to do here:
>
> -   if (unlikely(ret && drm_debug & DRM_UT_DP)) {
> +   if (unlikely(ret) && drm_debug_enabled(DRM_UT_DP)) {
>
> It doesn't really cost much in readability (especially compared to what
> it was before), and whether it's important performance wise I couldn't
> tell, but I think it's best to keep the code optimised as it was before
> unless there's a reason to drop it.
>
> Lyude might know more since she wrote 2f015ec6eab69301fdcf5, if you want
> to ping her?

Just ended up sending the updated version with what you suggest and I
agree with; pedantically the change should be a separate patch anyway.

Thanks for your inputs.

BR,
Jani.


>
>> 
>> BR,
>> Jani.
>> 
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center