diff mbox series

[3/4] drm/i915: Make intel_crtc_get_vblank_counter use no trace hw reads

Message ID 20230613215245.1551145-4-radhakrishna.sripada@intel.com (mailing list archive)
State New, archived
Headers show
Series Use non traceable api in display trace code | expand

Commit Message

Sripada, Radhakrishna June 13, 2023, 9:52 p.m. UTC 
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.

Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
---
 drivers/gpu/drm/i915/display/intel_vblank.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Lucas De Marchi June 22, 2023, 7:41 p.m. UTC | #1 
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@linux.intel.com>
>Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@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
>
Sripada, Radhakrishna June 22, 2023, 9:50 p.m. UTC | #2 
Hi Lucas,

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

Patch

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;