diff mbox series

[v4,12/13] drm/i915/display: Ensure we have "Frame Change" event in DSB commit

Message ID 20250124105625.822459-13-jouni.hogander@intel.com (mailing list archive)
State New
Headers show
Series PSR DSB support | expand

Commit Message

Hogander, Jouni Jan. 24, 2025, 10:56 a.m. UTC 
We may have commit which doesn't have any non-arming plane register
writes. In that case there aren't "Frame Change" event before DSB vblank
evasion which hangs as PIPEDSL register is reading as 0 when PSR state is
SRDENT(PSR1) or DEEP_SLEEP(PSR2). Handle this by adding dummy write
triggering the "Frame Change" event.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Ville Syrjälä Jan. 24, 2025, 11:43 a.m. UTC | #1 
On Fri, Jan 24, 2025 at 12:56:23PM +0200, Jouni Högander wrote:
> We may have commit which doesn't have any non-arming plane register
> writes. In that case there aren't "Frame Change" event before DSB vblank
> evasion which hangs as PIPEDSL register is reading as 0 when PSR state is
> SRDENT(PSR1) or DEEP_SLEEP(PSR2). Handle this by adding dummy write
> triggering the "Frame Change" event.
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a189aa437d972..cd7e960bd29f1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7666,6 +7666,7 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
>  		intel_atomic_get_old_crtc_state(state, crtc);
>  	struct intel_crtc_state *new_crtc_state =
>  		intel_atomic_get_new_crtc_state(state, crtc);
> +	struct intel_display *display = to_intel_display(crtc);
>  
>  	if (!new_crtc_state->hw.active)
>  		return;
> @@ -7708,6 +7709,15 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
>  		intel_crtc_planes_update_noarm(new_crtc_state->dsb_commit,
>  					       state, crtc);
>  
> +		/*
> +		 * Ensure we have "Frame Change" event when PSR state is
> +		 * SRDENT(PSR1) or DEEP_SLEEP(PSR2). Otherwise DSB vblank
> +		 * evasion hangs as PIPEDSL is reading as 0.
> +		 */
> +		if (new_crtc_state->has_psr && !new_crtc_state->has_panel_replay)
> +			intel_de_write_dsb(display, new_crtc_state->dsb_commit,
> +					   CURSURFLIVE(display, crtc->pipe), 0);

I don't really want to see the low level detais right here.
So we should probably should stuff this into some kind of 
intel_dsb_ensure_psr_frame_change() function or something
along those lines.

> +
>  		intel_dsb_vblank_evade(state, new_crtc_state->dsb_commit);
>  
>  		if (intel_crtc_needs_color_update(new_crtc_state))
> -- 
> 2.43.0
Ville Syrjälä Jan. 24, 2025, 11:57 a.m. UTC | #2 
On Fri, Jan 24, 2025 at 01:43:25PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 12:56:23PM +0200, Jouni Högander wrote:
> > We may have commit which doesn't have any non-arming plane register
> > writes. In that case there aren't "Frame Change" event before DSB vblank
> > evasion which hangs as PIPEDSL register is reading as 0 when PSR state is
> > SRDENT(PSR1) or DEEP_SLEEP(PSR2). Handle this by adding dummy write
> > triggering the "Frame Change" event.
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index a189aa437d972..cd7e960bd29f1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7666,6 +7666,7 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
> >  		intel_atomic_get_old_crtc_state(state, crtc);
> >  	struct intel_crtc_state *new_crtc_state =
> >  		intel_atomic_get_new_crtc_state(state, crtc);
> > +	struct intel_display *display = to_intel_display(crtc);
> >  
> >  	if (!new_crtc_state->hw.active)
> >  		return;
> > @@ -7708,6 +7709,15 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
> >  		intel_crtc_planes_update_noarm(new_crtc_state->dsb_commit,
> >  					       state, crtc);
> >  
> > +		/*
> > +		 * Ensure we have "Frame Change" event when PSR state is
> > +		 * SRDENT(PSR1) or DEEP_SLEEP(PSR2). Otherwise DSB vblank
> > +		 * evasion hangs as PIPEDSL is reading as 0.
> > +		 */
> > +		if (new_crtc_state->has_psr && !new_crtc_state->has_panel_replay)
> > +			intel_de_write_dsb(display, new_crtc_state->dsb_commit,
> > +					   CURSURFLIVE(display, crtc->pipe), 0);
> 
> I don't really want to see the low level detais right here.
> So we should probably should stuff this into some kind of 
> intel_dsb_ensure_psr_frame_change() function or something
> along those lines.

Oh, and I guess this really should be using
intel_pre_commit_crtc_state() as well. I suppose it doesn't
actually matter if we skip commits that change PSR stuff anyway,
but at some point the plan is to attempt full fastsets with
the DSB (if actually possible).

> 
> > +
> >  		intel_dsb_vblank_evade(state, new_crtc_state->dsb_commit);
> >  
> >  		if (intel_crtc_needs_color_update(new_crtc_state))
> > -- 
> > 2.43.0
> 
> -- 
> Ville Syrjälä
> Intel
Hogander, Jouni Jan. 24, 2025, 12:10 p.m. UTC | #3 
On Fri, 2025-01-24 at 13:43 +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 12:56:23PM +0200, Jouni Högander wrote:
> > We may have commit which doesn't have any non-arming plane register
> > writes. In that case there aren't "Frame Change" event before DSB
> > vblank
> > evasion which hangs as PIPEDSL register is reading as 0 when PSR
> > state is
> > SRDENT(PSR1) or DEEP_SLEEP(PSR2). Handle this by adding dummy write
> > triggering the "Frame Change" event.
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index a189aa437d972..cd7e960bd29f1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7666,6 +7666,7 @@ static void intel_atomic_dsb_finish(struct
> > intel_atomic_state *state,
> >  		intel_atomic_get_old_crtc_state(state, crtc);
> >  	struct intel_crtc_state *new_crtc_state =
> >  		intel_atomic_get_new_crtc_state(state, crtc);
> > +	struct intel_display *display = to_intel_display(crtc);
> >  
> >  	if (!new_crtc_state->hw.active)
> >  		return;
> > @@ -7708,6 +7709,15 @@ static void intel_atomic_dsb_finish(struct
> > intel_atomic_state *state,
> >  		intel_crtc_planes_update_noarm(new_crtc_state-
> > >dsb_commit,
> >  					       state, crtc);
> >  
> > +		/*
> > +		 * Ensure we have "Frame Change" event when PSR
> > state is
> > +		 * SRDENT(PSR1) or DEEP_SLEEP(PSR2). Otherwise DSB
> > vblank
> > +		 * evasion hangs as PIPEDSL is reading as 0.
> > +		 */
> > +		if (new_crtc_state->has_psr && !new_crtc_state-
> > >has_panel_replay)
> > +			intel_de_write_dsb(display,
> > new_crtc_state->dsb_commit,
> > +					   CURSURFLIVE(display,
> > crtc->pipe), 0);
> 
> I don't really want to see the low level detais right here.
> So we should probably should stuff this into some kind of 
> intel_dsb_ensure_psr_frame_change() function or something
> along those lines.

Ok, I will write such function.

BR,

Jouni Högander
> 
> > +
> >  		intel_dsb_vblank_evade(state, new_crtc_state-
> > >dsb_commit);
> >  
> >  		if (intel_crtc_needs_color_update(new_crtc_state))
> > -- 
> > 2.43.0
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index a189aa437d972..cd7e960bd29f1 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7666,6 +7666,7 @@  static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
 		intel_atomic_get_old_crtc_state(state, crtc);
 	struct intel_crtc_state *new_crtc_state =
 		intel_atomic_get_new_crtc_state(state, crtc);
+	struct intel_display *display = to_intel_display(crtc);
 
 	if (!new_crtc_state->hw.active)
 		return;
@@ -7708,6 +7709,15 @@  static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
 		intel_crtc_planes_update_noarm(new_crtc_state->dsb_commit,
 					       state, crtc);
 
+		/*
+		 * Ensure we have "Frame Change" event when PSR state is
+		 * SRDENT(PSR1) or DEEP_SLEEP(PSR2). Otherwise DSB vblank
+		 * evasion hangs as PIPEDSL is reading as 0.
+		 */
+		if (new_crtc_state->has_psr && !new_crtc_state->has_panel_replay)
+			intel_de_write_dsb(display, new_crtc_state->dsb_commit,
+					   CURSURFLIVE(display, crtc->pipe), 0);
+
 		intel_dsb_vblank_evade(state, new_crtc_state->dsb_commit);
 
 		if (intel_crtc_needs_color_update(new_crtc_state))