Re: [Intel-gfx] [PATCH v2 0/9] drm/print: add and use drm_debug_enabled()
On Tue, 24 Sep 2019, Jani Nikula wrote: > drm/print: move drm_debug variable to drm_print.[ch] > drm/print: add drm_debug_enabled() > drm/etnaviv: use drm_debug_enabled() to check for debug categories > drm/i2c/sil164: use drm_debug_enabled() to check for debug categories > drm/msm: use drm_debug_enabled() to check for debug categories Pushed the above patches to drm-misc-next, thanks for the reviews and acks! The below i915 and amdgpu patches conflict on drm-misc-next, and I haven't received any feedback on nouveau. > drm/i915: use drm_debug_enabled() to check for debug categories > drm/nouveau: use drm_debug_enabled() to check for debug categories > drm/amdgpu: use drm_debug_enabled() to check for debug categories > drm/print: rename drm_debug to __drm_debug to discourage use BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 0/9] drm/print: add and use drm_debug_enabled()
On Tue, 01 Oct 2019, Eric Engestrom wrote: > On Tuesday, 2019-10-01 14:03:55 +0300, Jani Nikula wrote: >> On Thu, 26 Sep 2019, Eric Engestrom 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 >> >> Cc: Alex Deucher >> >> Cc: Christian König >> >> Cc: David (ChunMing) Zhou >> >> Cc: amd-...@lists.freedesktop.org >> >> Cc: Ben Skeggs >> >> Cc: nouv...@lists.freedesktop.org >> >> Cc: Rob Clark >> >> Cc: Sean Paul >> >> Cc: linux-arm-...@vger.kernel.org >> >> Cc: freedr...@lists.freedesktop.org >> >> Cc: Francisco Jerez >> >> Cc: Lucas Stach >> >> Cc: Russell King >> >> Cc: Christian Gmeiner >> >> Cc: etna...@lists.freedesktop.org >> >> >> >> >> >> [1] cover.1568375189.git.jani.nikula@intel.com">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 >> > >> > 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 0/9] drm/print: add and use drm_debug_enabled()
On Tuesday, 2019-10-01 14:03:55 +0300, Jani Nikula wrote: > On Thu, 26 Sep 2019, Eric Engestrom 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 > >> Cc: Alex Deucher > >> Cc: Christian König > >> Cc: David (ChunMing) Zhou > >> Cc: amd-...@lists.freedesktop.org > >> Cc: Ben Skeggs > >> Cc: nouv...@lists.freedesktop.org > >> Cc: Rob Clark > >> Cc: Sean Paul > >> Cc: linux-arm-...@vger.kernel.org > >> Cc: freedr...@lists.freedesktop.org > >> Cc: Francisco Jerez > >> Cc: Lucas Stach > >> Cc: Russell King > >> Cc: Christian Gmeiner > >> Cc: etna...@lists.freedesktop.org > >> > >> > >> [1] cover.1568375189.git.jani.nikula@intel.com">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 > > > > 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 0/9] drm/print: add and use drm_debug_enabled()
On Thu, 26 Sep 2019, Eric Engestrom 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 >> Cc: Alex Deucher >> Cc: Christian König >> Cc: David (ChunMing) Zhou >> Cc: amd-...@lists.freedesktop.org >> Cc: Ben Skeggs >> Cc: nouv...@lists.freedesktop.org >> Cc: Rob Clark >> Cc: Sean Paul >> Cc: linux-arm-...@vger.kernel.org >> Cc: freedr...@lists.freedesktop.org >> Cc: Francisco Jerez >> Cc: Lucas Stach >> Cc: Russell King >> Cc: Christian Gmeiner >> Cc: etna...@lists.freedesktop.org >> >> >> [1] cover.1568375189.git.jani.nikula@intel.com">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 > > 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. 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? BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 0/9] drm/print: add and use drm_debug_enabled()
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 > Cc: Alex Deucher > Cc: Christian König > Cc: David (ChunMing) Zhou > Cc: amd-...@lists.freedesktop.org > Cc: Ben Skeggs > Cc: nouv...@lists.freedesktop.org > Cc: Rob Clark > Cc: Sean Paul > Cc: linux-arm-...@vger.kernel.org > Cc: freedr...@lists.freedesktop.org > Cc: Francisco Jerez > Cc: Lucas Stach > Cc: Russell King > Cc: Christian Gmeiner > Cc: etna...@lists.freedesktop.org > > > [1] cover.1568375189.git.jani.nikula@intel.com">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 Did you check to make sure the `unlikely()` is propagated correctly outside the `drm_debug_enabled()` call? ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 0/9] drm/print: add and use drm_debug_enabled()
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 Cc: Alex Deucher Cc: Christian König Cc: David (ChunMing) Zhou Cc: amd-...@lists.freedesktop.org Cc: Ben Skeggs Cc: nouv...@lists.freedesktop.org Cc: Rob Clark Cc: Sean Paul Cc: linux-arm-...@vger.kernel.org Cc: freedr...@lists.freedesktop.org Cc: Francisco Jerez Cc: Lucas Stach Cc: Russell King Cc: Christian Gmeiner Cc: etna...@lists.freedesktop.org [1] cover.1568375189.git.jani.nikula@intel.com">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/etnaviv: use drm_debug_enabled() to check for debug categories drm/i2c/sil164: use drm_debug_enabled() to check for debug categories drm/i915: use drm_debug_enabled() to check for debug categories drm/msm: use drm_debug_enabled() to check for debug categories drm/nouveau: use drm_debug_enabled() to check for debug categories drm/amdgpu: use drm_debug_enabled() to check for debug categories drm/print: rename drm_debug to __drm_debug to discourage use drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c | 4 ++-- drivers/gpu/drm/drm_atomic_uapi.c| 2 +- drivers/gpu/drm/drm_dp_mst_topology.c| 6 ++--- drivers/gpu/drm/drm_drv.c| 17 --- drivers/gpu/drm/drm_edid.c | 2 +- drivers/gpu/drm/drm_edid_load.c | 2 +- drivers/gpu/drm/drm_mipi_dbi.c | 4 ++-- drivers/gpu/drm/drm_print.c | 23 ++-- drivers/gpu/drm/drm_vblank.c | 6 ++--- drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 8 +++ drivers/gpu/drm/i2c/sil164_drv.c | 2 +- drivers/gpu/drm/i915/display/intel_display.c | 4 ++-- drivers/gpu/drm/i915/display/intel_dp.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 2 +- drivers/gpu/drm/i915/i915_gem.h | 2 +- drivers/gpu/drm/i915/i915_utils.c| 2 +- drivers/gpu/drm/i915/intel_pm.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 ++-- drivers/gpu/drm/nouveau/dispnv50/disp.h | 4 ++-- drivers/gpu/drm/nouveau/nouveau_drv.h| 4 ++-- include/drm/drm_drv.h| 2 -- include/drm/drm_print.h | 8 +++ 22 files changed, 60 insertions(+), 52 deletions(-) -- 2.20.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx