[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