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?
BR, Jani.
-- Jani Nikula, Intel Open Source Graphics Center