On Tuesday, 2019-10-01 14:03:55 +0300, Jani Nikula wrote:
> On Thu, 26 Sep 2019, Eric Engestrom <[email protected]> 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 <[email protected]>
> >> Cc: Alex Deucher <[email protected]>
> >> Cc: Christian König <[email protected]>
> >> Cc: David (ChunMing) Zhou <[email protected]>
> >> Cc: [email protected]
> >> Cc: Ben Skeggs <[email protected]>
> >> Cc: [email protected]
> >> Cc: Rob Clark <[email protected]>
> >> Cc: Sean Paul <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Cc: Francisco Jerez <[email protected]>
> >> Cc: Lucas Stach <[email protected]>
> >> Cc: Russell King <[email protected]>
> >> Cc: Christian Gmeiner <[email protected]>
> >> Cc: [email protected]
> >>
> >>
> >> [1] [email protected]">http://mid.mail-archive.com/[email protected]
> >>
> >> 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 <[email protected]>
> >
> > 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?
>
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
