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

Lucas De Marchi lucas.demarchi at intel.com
Thu Jun 22 19:41:16 UTC 2023


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


More information about the Intel-gfx mailing list