Re: [Intel-gfx] [PATCH 3/4] drm/i915: Make intel_crtc_get_vblank_counter use no trace hw reads
Hi Lucas,
> -Original Message-
> From: De Marchi, Lucas
> Sent: Thursday, June 22, 2023 12:41 PM
> To: Sripada, Radhakrishna
> Cc: intel-gfx@lists.freedesktop.org; Maarten Lankhorst
>
> Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915: Make
> intel_crtc_get_vblank_counter use no trace hw reads
>
> On Tue, Jun 13, 2023 at 02:52:44PM -0700, Radhakrishna Sripada wrote:
> >intel_crtc_get_vblank_counter is used in many places in the display
> >tracing infrastructure. For a clean execution of the tracing assignment,
> >ensure that any necessary HW reads would not further trigger another trace,
> >to prevent nesting of trace events.
>
>
> it's not clear what "nesting" means in this patch series. For me
> "nesting" would be if in the middle of a trace event it triggered
> another trace event. Given our current infra, I don't see how that
> would be possible.
Intel_crtc_get_vblank_counter/intel_get_crtc_scanline is used at many of the
trace events defined in intel_display_trace.h like intel_pipe_{en,dis}able,
intel_pipe_crc
during the assign phase to capture the current vblank and scanline values.
However those
functions indeed use traceable versions of register reads making a nested trace
call.
kworker/u29:0-153 [007] 402.314951: kernel_stack:=>
trace_event_raw_event_i915_reg_rw
=> __intel_get_crtc_scanline
=> intel_get_crtc_scanline
=> trace_event_raw_event_intel_plane_update_noarm
=> intel_plane_update_noarm
=> intel_crtc_planes_update_noarm
=> intel_update_crtc
=> skl_commit_modeset_enables
>
> Do you mean that certain register accesses are being reported twice
> since they are being recorded in 2 different layers like intel_de and
> intel_uncore? If so, can you add in the commit message what is the call
> chain you're seeing? The indirections in intel_de_read_fw() are not so
> easy to follow, but from a quick look I don't see that happening here.
I haven't observed those style of reporting twice.
--Radhakrishna(RK) Sripada
>
> intel_de_read_fw()
>intel_uncore_read_fw()
> __raw_uncore_read32() <-- no trace here
>trace_i915_reg_rw()
>
> What makes intel_de_read_fw() call special in this intel_vblank.c that
> is not the case in all the hundred other places this function is called?
>
> The trace_i915_reg_rw() in intel_de_read_fw() was added exactly because
> __raw_uncore_read32() doesn't trace.
>
> In xe, we should probably override the intel_de_read_fw() with a
> xe-specific function that just leaves the trace out, delegated to
> xe_mmio().
>
>
> Btw, see the comment on top of intel_uncore_read_fw() that nobody reads
> and calls to those "raw" accessors are added, making the i915_reg_rw
> trace almost useless.
>
> $ git grep intel_uncore_read_fw | wc -l
> 65
>
> The _fw() suffix was meant as: you first take the forcewake, then
> you access a bunch of registers, then release the forcewake. The
> non-trace is a bad side effect with no clue on the name of the function,
> just a comment on top of it.
>
> Lucas De Marchi
>
>
> >
> >Suggested-by: Maarten Lankhorst
> >Signed-off-by: Radhakrishna Sripada
> >---
> > drivers/gpu/drm/i915/display/intel_vblank.c | 7 ---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c
> b/drivers/gpu/drm/i915/display/intel_vblank.c
> >index f5659ebd08eb..55f3389fa220 100644
> >--- a/drivers/gpu/drm/i915/display/intel_vblank.c
> >+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> >@@ -103,7 +103,7 @@ u32 i915_get_vblank_counter(struct drm_crtc *crtc)
> > * we get a low value that's stable across two reads of the high
> > * register.
> > */
> >-frame = intel_de_read64_2x32(dev_priv, PIPEFRAMEPIXEL(pipe),
> PIPEFRAME(pipe));
> >+frame = intel_de_read64_2x32_notrace(dev_priv,
> PIPEFRAMEPIXEL(pipe), PIPEFRAME(pipe));
> >
> > pixel = frame & PIPE_PIXEL_MASK;
> > frame = (frame >> PIPE_FRAME_LOW_SHIFT) & 0xff;
> >@@ -125,7 +125,7 @@ u32 g4x_get_vblank_counter(struct drm_crtc *crtc)
> > if (!vblank->max_vblank_count)
> > return 0;
> >
> >-return intel_de_read(dev_priv, PIPE_FRMCOUNT_G4X(pipe));
> >+return intel_de_read_notrace(dev_priv, PIPE_FRMCOUNT_G4X(pipe));
> > }
> >
> > static u32 intel_crtc_scanlines_since_frame_timestamp(struct intel_crtc
> > *crtc)
> >@@ -324,7 +324,8 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc
> *_crtc,
> > *
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Make intel_crtc_get_vblank_counter use no trace hw reads
On Tue, Jun 13, 2023 at 02:52:44PM -0700, Radhakrishna Sripada wrote: intel_crtc_get_vblank_counter is used in many places in the display tracing infrastructure. For a clean execution of the tracing assignment, ensure that any necessary HW reads would not further trigger another trace, to prevent nesting of trace events. it's not clear what "nesting" means in this patch series. For me "nesting" would be if in the middle of a trace event it triggered another trace event. Given our current infra, I don't see how that would be possible. Do you mean that certain register accesses are being reported twice since they are being recorded in 2 different layers like intel_de and intel_uncore? If so, can you add in the commit message what is the call chain you're seeing? The indirections in intel_de_read_fw() are not so easy to follow, but from a quick look I don't see that happening here. intel_de_read_fw() intel_uncore_read_fw() __raw_uncore_read32() <-- no trace here trace_i915_reg_rw() What makes intel_de_read_fw() call special in this intel_vblank.c that is not the case in all the hundred other places this function is called? The trace_i915_reg_rw() in intel_de_read_fw() was added exactly because __raw_uncore_read32() doesn't trace. In xe, we should probably override the intel_de_read_fw() with a xe-specific function that just leaves the trace out, delegated to xe_mmio(). Btw, see the comment on top of intel_uncore_read_fw() that nobody reads and calls to those "raw" accessors are added, making the i915_reg_rw trace almost useless. $ git grep intel_uncore_read_fw | wc -l 65 The _fw() suffix was meant as: you first take the forcewake, then you access a bunch of registers, then release the forcewake. The non-trace is a bad side effect with no clue on the name of the function, just a comment on top of it. Lucas De Marchi Suggested-by: Maarten Lankhorst Signed-off-by: Radhakrishna Sripada --- drivers/gpu/drm/i915/display/intel_vblank.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c index f5659ebd08eb..55f3389fa220 100644 --- a/drivers/gpu/drm/i915/display/intel_vblank.c +++ b/drivers/gpu/drm/i915/display/intel_vblank.c @@ -103,7 +103,7 @@ u32 i915_get_vblank_counter(struct drm_crtc *crtc) * we get a low value that's stable across two reads of the high * register. */ - frame = intel_de_read64_2x32(dev_priv, PIPEFRAMEPIXEL(pipe), PIPEFRAME(pipe)); + frame = intel_de_read64_2x32_notrace(dev_priv, PIPEFRAMEPIXEL(pipe), PIPEFRAME(pipe)); pixel = frame & PIPE_PIXEL_MASK; frame = (frame >> PIPE_FRAME_LOW_SHIFT) & 0xff; @@ -125,7 +125,7 @@ u32 g4x_get_vblank_counter(struct drm_crtc *crtc) if (!vblank->max_vblank_count) return 0; - return intel_de_read(dev_priv, PIPE_FRMCOUNT_G4X(pipe)); + return intel_de_read_notrace(dev_priv, PIPE_FRMCOUNT_G4X(pipe)); } static u32 intel_crtc_scanlines_since_frame_timestamp(struct intel_crtc *crtc) @@ -324,7 +324,8 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc, * We can split this into vertical and horizontal * scanout position. */ - position = (intel_de_read_fw(dev_priv, PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; + position = (intel_de_read_fw_notrace(dev_priv, PIPEFRAMEPIXEL(pipe)) & + PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; /* convert to pixel counts */ vbl_start *= htotal; -- 2.34.1
[Intel-gfx] [PATCH 3/4] drm/i915: Make intel_crtc_get_vblank_counter use no trace hw reads
intel_crtc_get_vblank_counter is used in many places in the display tracing infrastructure. For a clean execution of the tracing assignment, ensure that any necessary HW reads would not further trigger another trace, to prevent nesting of trace events. Suggested-by: Maarten Lankhorst Signed-off-by: Radhakrishna Sripada --- drivers/gpu/drm/i915/display/intel_vblank.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c index f5659ebd08eb..55f3389fa220 100644 --- a/drivers/gpu/drm/i915/display/intel_vblank.c +++ b/drivers/gpu/drm/i915/display/intel_vblank.c @@ -103,7 +103,7 @@ u32 i915_get_vblank_counter(struct drm_crtc *crtc) * we get a low value that's stable across two reads of the high * register. */ - frame = intel_de_read64_2x32(dev_priv, PIPEFRAMEPIXEL(pipe), PIPEFRAME(pipe)); + frame = intel_de_read64_2x32_notrace(dev_priv, PIPEFRAMEPIXEL(pipe), PIPEFRAME(pipe)); pixel = frame & PIPE_PIXEL_MASK; frame = (frame >> PIPE_FRAME_LOW_SHIFT) & 0xff; @@ -125,7 +125,7 @@ u32 g4x_get_vblank_counter(struct drm_crtc *crtc) if (!vblank->max_vblank_count) return 0; - return intel_de_read(dev_priv, PIPE_FRMCOUNT_G4X(pipe)); + return intel_de_read_notrace(dev_priv, PIPE_FRMCOUNT_G4X(pipe)); } static u32 intel_crtc_scanlines_since_frame_timestamp(struct intel_crtc *crtc) @@ -324,7 +324,8 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc, * We can split this into vertical and horizontal * scanout position. */ - position = (intel_de_read_fw(dev_priv, PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; + position = (intel_de_read_fw_notrace(dev_priv, PIPEFRAMEPIXEL(pipe)) & + PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; /* convert to pixel counts */ vbl_start *= htotal; -- 2.34.1
