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