[v2,0/9] drm/print: add and use drm_debug_enabled()

Message

Jani Nikula Sept. 24, 2019, 12:58 p.m. UTC

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 <[e-mail]>
Cc: Alex Deucher <[e-mail]>
Cc: Christian König <[e-mail]>
Cc: David (ChunMing) Zhou <[e-mail]>
Cc: [e-mail]
Cc: Ben Skeggs <[e-mail]>
Cc: [e-mail]
Cc: Rob Clark <[e-mail]>
Cc: Sean Paul <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: Francisco Jerez <[e-mail]>
Cc: Lucas Stach <[e-mail]>
Cc: Russell King <[e-mail]>
Cc: Christian Gmeiner <[e-mail]>
Cc: [e-mail]


[1] http://mid.mail-archive.com/[e-mail]

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(-)

Comments

Eric Engestrom Sept. 26, 2019, 7:48 a.m. UTC | #1

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 <[e-mail]>
> Cc: Alex Deucher <[e-mail]>
> Cc: Christian König <[e-mail]>
> Cc: David (ChunMing) Zhou <[e-mail]>
> Cc: [e-mail]
> Cc: Ben Skeggs <[e-mail]>
> Cc: [e-mail]
> Cc: Rob Clark <[e-mail]>
> Cc: Sean Paul <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: Francisco Jerez <[e-mail]>
> Cc: Lucas Stach <[e-mail]>
> Cc: Russell King <[e-mail]>
> Cc: Christian Gmeiner <[e-mail]>
> Cc: [e-mail]
> 
> 
> [1] http://mid.mail-archive.com/[e-mail]
> 
> 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 <[e-mail]>

Did you check to make sure the `unlikely()` is propagated correctly
outside the `drm_debug_enabled()` call?

Jani Nikula Oct. 1, 2019, 11:03 a.m. UTC | #2

On Thu, 26 Sep 2019, Eric Engestrom <[e-mail]> 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 <[e-mail]>
>> Cc: Alex Deucher <[e-mail]>
>> Cc: Christian König <[e-mail]>
>> Cc: David (ChunMing) Zhou <[e-mail]>
>> Cc: [e-mail]
>> Cc: Ben Skeggs <[e-mail]>
>> Cc: [e-mail]
>> Cc: Rob Clark <[e-mail]>
>> Cc: Sean Paul <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: Francisco Jerez <[e-mail]>
>> Cc: Lucas Stach <[e-mail]>
>> Cc: Russell King <[e-mail]>
>> Cc: Christian Gmeiner <[e-mail]>
>> Cc: [e-mail]
>> 
>> 
>> [1] http://mid.mail-archive.com/[e-mail]
>> 
>> 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 <[e-mail]>
>
> 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.

Eric Engestrom Oct. 1, 2019, 12:34 p.m. UTC | #3

On Tuesday, 2019-10-01 14:03:55 +0300, Jani Nikula wrote:
> On Thu, 26 Sep 2019, Eric Engestrom <[e-mail]> 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 <[e-mail]>
> >> Cc: Alex Deucher <[e-mail]>
> >> Cc: Christian König <[e-mail]>
> >> Cc: David (ChunMing) Zhou <[e-mail]>
> >> Cc: [e-mail]
> >> Cc: Ben Skeggs <[e-mail]>
> >> Cc: [e-mail]
> >> Cc: Rob Clark <[e-mail]>
> >> Cc: Sean Paul <[e-mail]>
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: Francisco Jerez <[e-mail]>
> >> Cc: Lucas Stach <[e-mail]>
> >> Cc: Russell King <[e-mail]>
> >> Cc: Christian Gmeiner <[e-mail]>
> >> Cc: [e-mail]
> >> 
> >> 
> >> [1] http://mid.mail-archive.com/[e-mail]
> >> 
> >> 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 <[e-mail]>
> >
> > 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

Jani Nikula Oct. 1, 2019, 2:08 p.m. UTC | #4

On Tue, 01 Oct 2019, Eric Engestrom <[e-mail]> wrote:
> On Tuesday, 2019-10-01 14:03:55 +0300, Jani Nikula wrote:
>> On Thu, 26 Sep 2019, Eric Engestrom <[e-mail]> 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 <[e-mail]>
>> >> Cc: Alex Deucher <[e-mail]>
>> >> Cc: Christian König <[e-mail]>
>> >> Cc: David (ChunMing) Zhou <[e-mail]>
>> >> Cc: [e-mail]
>> >> Cc: Ben Skeggs <[e-mail]>
>> >> Cc: [e-mail]
>> >> Cc: Rob Clark <[e-mail]>
>> >> Cc: Sean Paul <[e-mail]>
>> >> Cc: [e-mail]
>> >> Cc: [e-mail]
>> >> Cc: Francisco Jerez <[e-mail]>
>> >> Cc: Lucas Stach <[e-mail]>
>> >> Cc: Russell King <[e-mail]>
>> >> Cc: Christian Gmeiner <[e-mail]>
>> >> Cc: [e-mail]
>> >> 
>> >> 
>> >> [1] http://mid.mail-archive.com/[e-mail]
>> >> 
>> >> 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 <[e-mail]>
>> >
>> > 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 Oct. 2, 2019, 2:11 p.m. UTC | #5

On Tue, 24 Sep 2019, Jani Nikula <[e-mail]> 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.