[Intel-gfx] [PATCH 3/4] drm/i915: Make intel_crtc_get_vblank_counter use no trace hw reads

[Sripada, Radhakrishna]

Hi Lucas,

> -----Original Message-----
> From: De Marchi, Lucas <lucas.demarchi at intel.com>
> Sent: Thursday, June 22, 2023 12:41 PM
> To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Maarten Lankhorst
> <maarten.lankhorst at linux.intel.com>
> 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.

<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 at linux.intel.com>
> >Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> >---
> > 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
> >