On Tue, 01 Oct 2019, Eric Engestrom <eric@xxxxxxxxxxxx> wrote: > 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? 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
