On Tuesday, 2019-10-01 14:03:55 +0300, Jani Nikula wrote: > On Thu, 26 Sep 2019, Eric Engestrom <eric@xxxxxxxxxxxx> 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@xxxxxxxxx> > >> Cc: Alex Deucher <alexander.deucher@xxxxxxx> > >> Cc: Christian König <christian.koenig@xxxxxxx> > >> Cc: David (ChunMing) Zhou <David1.Zhou@xxxxxxx> > >> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx > >> Cc: Ben Skeggs <bskeggs@xxxxxxxxxx> > >> Cc: nouveau@xxxxxxxxxxxxxxxxxxxxx > >> Cc: Rob Clark <robdclark@xxxxxxxxx> > >> Cc: Sean Paul <sean@xxxxxxxxxx> > >> Cc: linux-arm-msm@xxxxxxxxxxxxxxx > >> Cc: freedreno@xxxxxxxxxxxxxxxxxxxxx > >> Cc: Francisco Jerez <currojerez@xxxxxxxxxx> > >> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx> > >> Cc: Russell King <linux+etnaviv@xxxxxxxxxxxxxxx> > >> Cc: Christian Gmeiner <christian.gmeiner@xxxxxxxxx> > >> Cc: etnaviv@xxxxxxxxxxxxxxxxxxxxx > >> > >> > >> [1] http://mid.mail-archive.com/cover.1568375189.git.jani.nikula@xxxxxxxxx > >> > >> 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@xxxxxxxxxxxx> > > > > 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 dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
