Re: [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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [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? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
