Hi Lucas,
> -----Original Message-----
> From: De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>
> Sent: Thursday, June 22, 2023 12:41 PM
> To: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Maarten Lankhorst
> <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Subject: Re: [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.
<snip>
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
</snip>
>
> 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 <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> >---
> > 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) & 0xffffff;
> >@@ -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
> >
