| Message ID | 20250124105625.822459-12-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PSR DSB support | expand |
On Fri, Jan 24, 2025 at 12:56:22PM +0200, Jouni Högander wrote: > PIPEDSL is reading as 0 when in SRDENT(PSR1) or DEEP_SLEEP(PSR2). On > wake-up scanline counting starts from vblank_start - 1. We don't know if > wake-up is already ongoing when evasion starts. In worst case PIPEDSL could > start reading valid value right after checking the scanline. In this > scenario we wouldn't have enough time to write all registers. To tackle > this evade scanline 0 as well. As a drawback we have 1 frame delay in flip > when waking up. > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c > index bb77ded8bf726..914f0be4491c4 100644 > --- a/drivers/gpu/drm/i915/display/intel_dsb.c > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c > @@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct intel_atomic_state *state, > int latency = intel_usecs_to_scanlines(&crtc_state->hw.adjusted_mode, 20); > int start, end; > > + /* > + * PIPEDSL is reading as 0 when in SRDENT(PSR1) or DEEP_SLEEP(PSR2). On > + * wake-up scanline counting starts from vblank_start - 1. We don't know > + * if wake-up is already ongoing when evasion starts. In worst case > + * PIPEDSL could start reading valid value right after checking the > + * scanline. In this scenario we wouldn't have enough time to write all > + * registers. To tackle this evade scanline 0 as well. As a drawback we > + * have 1 frame delay in flip when waking up. > + */ > + if (crtc_state->has_psr && !crtc_state->has_panel_replay) What's the deal with panel replay here? > + intel_dsb_wait_scanline_out(state, dsb, 0, 0); This needs to be a raw intel_dsb_emit_wait_dsl(dsb, DSB_OPCODE_WAIT_DSL_OUT, 0, 0) because we need to evade the hw scanline 0. What the software thinks is scanline 0 is a bit different (see scanline_offset). > + > if (pre_commit_is_vrr_active(state, crtc)) { > int vblank_delay = intel_vrr_vblank_delay(crtc_state); > > -- > 2.43.0
On Fri, 2025-01-24 at 13:39 +0200, Ville Syrjälä wrote: > On Fri, Jan 24, 2025 at 12:56:22PM +0200, Jouni Högander wrote: > > PIPEDSL is reading as 0 when in SRDENT(PSR1) or DEEP_SLEEP(PSR2). > > On > > wake-up scanline counting starts from vblank_start - 1. We don't > > know if > > wake-up is already ongoing when evasion starts. In worst case > > PIPEDSL could > > start reading valid value right after checking the scanline. In > > this > > scenario we wouldn't have enough time to write all registers. To > > tackle > > this evade scanline 0 as well. As a drawback we have 1 frame delay > > in flip > > when waking up. > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c > > b/drivers/gpu/drm/i915/display/intel_dsb.c > > index bb77ded8bf726..914f0be4491c4 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c > > @@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct > > intel_atomic_state *state, > > int latency = intel_usecs_to_scanlines(&crtc_state- > > >hw.adjusted_mode, 20); > > int start, end; > > > > + /* > > + * PIPEDSL is reading as 0 when in SRDENT(PSR1) or > > DEEP_SLEEP(PSR2). On > > + * wake-up scanline counting starts from vblank_start - 1. > > We don't know > > + * if wake-up is already ongoing when evasion starts. In > > worst case > > + * PIPEDSL could start reading valid value right after > > checking the > > + * scanline. In this scenario we wouldn't have enough time > > to write all > > + * registers. To tackle this evade scanline 0 as well. As > > a drawback we > > + * have 1 frame delay in flip when waking up. > > + */ > > + if (crtc_state->has_psr && !crtc_state->has_panel_replay) > > What's the deal with panel replay here? I couldn't see PIPEDSL returning 0 when using Panel Replay. Even on same setup with PSR I saw it, but with PR I couldn't see. > > > + intel_dsb_wait_scanline_out(state, dsb, 0, 0); > > This needs to be a raw > intel_dsb_emit_wait_dsl(dsb, DSB_OPCODE_WAIT_DSL_OUT, 0, 0) > because we need to evade the hw scanline 0. What the software > thinks is scanline 0 is a bit different (see scanline_offset). Ok, I will change this. BR, Jouni Högander > > > + > > if (pre_commit_is_vrr_active(state, crtc)) { > > int vblank_delay = > > intel_vrr_vblank_delay(crtc_state); > > > > -- > > 2.43.0 >
On Fri, Jan 24, 2025 at 11:57:10AM +0000, Hogander, Jouni wrote: > On Fri, 2025-01-24 at 13:39 +0200, Ville Syrjälä wrote: > > On Fri, Jan 24, 2025 at 12:56:22PM +0200, Jouni Högander wrote: > > > PIPEDSL is reading as 0 when in SRDENT(PSR1) or DEEP_SLEEP(PSR2). > > > On > > > wake-up scanline counting starts from vblank_start - 1. We don't > > > know if > > > wake-up is already ongoing when evasion starts. In worst case > > > PIPEDSL could > > > start reading valid value right after checking the scanline. In > > > this > > > scenario we wouldn't have enough time to write all registers. To > > > tackle > > > this evade scanline 0 as well. As a drawback we have 1 frame delay > > > in flip > > > when waking up. > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c > > > b/drivers/gpu/drm/i915/display/intel_dsb.c > > > index bb77ded8bf726..914f0be4491c4 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c > > > @@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct > > > intel_atomic_state *state, > > > int latency = intel_usecs_to_scanlines(&crtc_state- > > > >hw.adjusted_mode, 20); > > > int start, end; > > > > > > + /* > > > + * PIPEDSL is reading as 0 when in SRDENT(PSR1) or > > > DEEP_SLEEP(PSR2). On > > > + * wake-up scanline counting starts from vblank_start - 1. > > > We don't know > > > + * if wake-up is already ongoing when evasion starts. In > > > worst case > > > + * PIPEDSL could start reading valid value right after > > > checking the > > > + * scanline. In this scenario we wouldn't have enough time > > > to write all > > > + * registers. To tackle this evade scanline 0 as well. As > > > a drawback we > > > + * have 1 frame delay in flip when waking up. > > > + */ > > > + if (crtc_state->has_psr && !crtc_state->has_panel_replay) > > > > What's the deal with panel replay here? > > I couldn't see PIPEDSL returning 0 when using Panel Replay. Even on > same setup with PSR I saw it, but with PR I couldn't see. Hmm. Are you sure it's reaching DC5/6? Though I suppose it's possible that panel replay (unlike PSR) does pretty much everything from the DMC's DC state handler, so as soon as you read the register it restores things fully enough that you get the vblank_start-1 response...
On Fri, 2025-01-24 at 14:37 +0200, Ville Syrjälä wrote: > On Fri, Jan 24, 2025 at 11:57:10AM +0000, Hogander, Jouni wrote: > > On Fri, 2025-01-24 at 13:39 +0200, Ville Syrjälä wrote: > > > On Fri, Jan 24, 2025 at 12:56:22PM +0200, Jouni Högander wrote: > > > > PIPEDSL is reading as 0 when in SRDENT(PSR1) or > > > > DEEP_SLEEP(PSR2). > > > > On > > > > wake-up scanline counting starts from vblank_start - 1. We > > > > don't > > > > know if > > > > wake-up is already ongoing when evasion starts. In worst case > > > > PIPEDSL could > > > > start reading valid value right after checking the scanline. In > > > > this > > > > scenario we wouldn't have enough time to write all registers. > > > > To > > > > tackle > > > > this evade scanline 0 as well. As a drawback we have 1 frame > > > > delay > > > > in flip > > > > when waking up. > > > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c > > > > b/drivers/gpu/drm/i915/display/intel_dsb.c > > > > index bb77ded8bf726..914f0be4491c4 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c > > > > @@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct > > > > intel_atomic_state *state, > > > > int latency = intel_usecs_to_scanlines(&crtc_state- > > > > > hw.adjusted_mode, 20); > > > > int start, end; > > > > > > > > + /* > > > > + * PIPEDSL is reading as 0 when in SRDENT(PSR1) or > > > > DEEP_SLEEP(PSR2). On > > > > + * wake-up scanline counting starts from vblank_start > > > > - 1. > > > > We don't know > > > > + * if wake-up is already ongoing when evasion starts. > > > > In > > > > worst case > > > > + * PIPEDSL could start reading valid value right after > > > > checking the > > > > + * scanline. In this scenario we wouldn't have enough > > > > time > > > > to write all > > > > + * registers. To tackle this evade scanline 0 as well. > > > > As > > > > a drawback we > > > > + * have 1 frame delay in flip when waking up. > > > > + */ > > > > + if (crtc_state->has_psr && !crtc_state- > > > > >has_panel_replay) > > > > > > What's the deal with panel replay here? > > > > I couldn't see PIPEDSL returning 0 when using Panel Replay. Even on > > same setup with PSR I saw it, but with PR I couldn't see. > > Hmm. Are you sure it's reaching DC5/6? > > Though I suppose it's possible that panel replay (unlike PSR) > does pretty much everything from the DMC's DC state handler, > so as soon as you read the register it restores things fully > enough that you get the vblank_start-1 response... Maybe we just add that evasion without checking panel replay. It shouldn't be too expensive anyways. What do you think? BR, Jouni Högander >
On Fri, Jan 24, 2025 at 12:41:11PM +0000, Hogander, Jouni wrote: > On Fri, 2025-01-24 at 14:37 +0200, Ville Syrjälä wrote: > > On Fri, Jan 24, 2025 at 11:57:10AM +0000, Hogander, Jouni wrote: > > > On Fri, 2025-01-24 at 13:39 +0200, Ville Syrjälä wrote: > > > > On Fri, Jan 24, 2025 at 12:56:22PM +0200, Jouni Högander wrote: > > > > > PIPEDSL is reading as 0 when in SRDENT(PSR1) or > > > > > DEEP_SLEEP(PSR2). > > > > > On > > > > > wake-up scanline counting starts from vblank_start - 1. We > > > > > don't > > > > > know if > > > > > wake-up is already ongoing when evasion starts. In worst case > > > > > PIPEDSL could > > > > > start reading valid value right after checking the scanline. In > > > > > this > > > > > scenario we wouldn't have enough time to write all registers. > > > > > To > > > > > tackle > > > > > this evade scanline 0 as well. As a drawback we have 1 frame > > > > > delay > > > > > in flip > > > > > when waking up. > > > > > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++ > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c > > > > > b/drivers/gpu/drm/i915/display/intel_dsb.c > > > > > index bb77ded8bf726..914f0be4491c4 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c > > > > > @@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct > > > > > intel_atomic_state *state, > > > > > int latency = intel_usecs_to_scanlines(&crtc_state- > > > > > > hw.adjusted_mode, 20); > > > > > int start, end; > > > > > > > > > > + /* > > > > > + * PIPEDSL is reading as 0 when in SRDENT(PSR1) or > > > > > DEEP_SLEEP(PSR2). On > > > > > + * wake-up scanline counting starts from vblank_start > > > > > - 1. > > > > > We don't know > > > > > + * if wake-up is already ongoing when evasion starts. > > > > > In > > > > > worst case > > > > > + * PIPEDSL could start reading valid value right after > > > > > checking the > > > > > + * scanline. In this scenario we wouldn't have enough > > > > > time > > > > > to write all > > > > > + * registers. To tackle this evade scanline 0 as well. > > > > > As > > > > > a drawback we > > > > > + * have 1 frame delay in flip when waking up. > > > > > + */ > > > > > + if (crtc_state->has_psr && !crtc_state- > > > > > >has_panel_replay) > > > > > > > > What's the deal with panel replay here? > > > > > > I couldn't see PIPEDSL returning 0 when using Panel Replay. Even on > > > same setup with PSR I saw it, but with PR I couldn't see. > > > > Hmm. Are you sure it's reaching DC5/6? > > > > Though I suppose it's possible that panel replay (unlike PSR) > > does pretty much everything from the DMC's DC state handler, > > so as soon as you read the register it restores things fully > > enough that you get the vblank_start-1 response... > > Maybe we just add that evasion without checking panel replay. It > shouldn't be too expensive anyways. What do you think? Yeah, that seems fine to me. But I still think you should try to double check that it really reaches DC6 with panel replay despite PIPDSL not getting reset, so that we at least understand a bit better what is actually happening.
On Fri, 2025-01-24 at 14:49 +0200, Ville Syrjälä wrote: > On Fri, Jan 24, 2025 at 12:41:11PM +0000, Hogander, Jouni wrote: > > On Fri, 2025-01-24 at 14:37 +0200, Ville Syrjälä wrote: > > > On Fri, Jan 24, 2025 at 11:57:10AM +0000, Hogander, Jouni wrote: > > > > On Fri, 2025-01-24 at 13:39 +0200, Ville Syrjälä wrote: > > > > > On Fri, Jan 24, 2025 at 12:56:22PM +0200, Jouni Högander > > > > > wrote: > > > > > > PIPEDSL is reading as 0 when in SRDENT(PSR1) or > > > > > > DEEP_SLEEP(PSR2). > > > > > > On > > > > > > wake-up scanline counting starts from vblank_start - 1. We > > > > > > don't > > > > > > know if > > > > > > wake-up is already ongoing when evasion starts. In worst > > > > > > case > > > > > > PIPEDSL could > > > > > > start reading valid value right after checking the > > > > > > scanline. In > > > > > > this > > > > > > scenario we wouldn't have enough time to write all > > > > > > registers. > > > > > > To > > > > > > tackle > > > > > > this evade scanline 0 as well. As a drawback we have 1 > > > > > > frame > > > > > > delay > > > > > > in flip > > > > > > when waking up. > > > > > > > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++ > > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c > > > > > > b/drivers/gpu/drm/i915/display/intel_dsb.c > > > > > > index bb77ded8bf726..914f0be4491c4 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c > > > > > > @@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct > > > > > > intel_atomic_state *state, > > > > > > int latency = > > > > > > intel_usecs_to_scanlines(&crtc_state- > > > > > > > hw.adjusted_mode, 20); > > > > > > int start, end; > > > > > > > > > > > > + /* > > > > > > + * PIPEDSL is reading as 0 when in SRDENT(PSR1) or > > > > > > DEEP_SLEEP(PSR2). On > > > > > > + * wake-up scanline counting starts from > > > > > > vblank_start > > > > > > - 1. > > > > > > We don't know > > > > > > + * if wake-up is already ongoing when evasion > > > > > > starts. > > > > > > In > > > > > > worst case > > > > > > + * PIPEDSL could start reading valid value right > > > > > > after > > > > > > checking the > > > > > > + * scanline. In this scenario we wouldn't have > > > > > > enough > > > > > > time > > > > > > to write all > > > > > > + * registers. To tackle this evade scanline 0 as > > > > > > well. > > > > > > As > > > > > > a drawback we > > > > > > + * have 1 frame delay in flip when waking up. > > > > > > + */ > > > > > > + if (crtc_state->has_psr && !crtc_state- > > > > > > > has_panel_replay) > > > > > > > > > > What's the deal with panel replay here? > > > > > > > > I couldn't see PIPEDSL returning 0 when using Panel Replay. > > > > Even on > > > > same setup with PSR I saw it, but with PR I couldn't see. > > > > > > Hmm. Are you sure it's reaching DC5/6? > > > > > > Though I suppose it's possible that panel replay (unlike PSR) > > > does pretty much everything from the DMC's DC state handler, > > > so as soon as you read the register it restores things fully > > > enough that you get the vblank_start-1 response... > > > > Maybe we just add that evasion without checking panel replay. It > > shouldn't be too expensive anyways. What do you think? > > Yeah, that seems fine to me. > > But I still think you should try to double check that it > really reaches DC6 with panel replay despite PIPDSL not > getting reset, so that we at least understand a bit better > what is actually happening. Here are my observations: 1. TGL/PSR2, no DMC firmware : PIPEDSL is reading as vblank - 1 when in DEEP_SLEEP 2. TGL/PSR2, DMC firmware loaded: PIPEDSL is reading as 0 when in DEEP_SLEEP 3. TGL/PSR2, DMC fimware loaded , DC states disabled : PIPEDSL reading is running all the time 4. LNL/Panel Replay, no DMC firmware : PIPEDSL reading is running all the time 5. LNL/Panel Replay, DMC firmware loaded : PIPEDSL is reading as vblank - 1 when in SLEEP 6. LNL/Panel Replay, DMC firmware loaded, DC state disabled : PIPEDSL reading is running all the time Results are differing. Based on this: Current evation is handling cases where PIPEDSL reading is running or vblank - 1. PIPEDSL reading as 0 is handled by this adding scanline 0 evation. PSR2 commits not using DSB currently might be actually affected by your concern. I.e. wake-up is already ongoing when performing vblank evation. PSR1 shouldn't have this problem as there we are waiting PSR state to become idle. Maybe we should add scanline 0 there as well? Not related to this patch though. BR, Jouni Högander >
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c index bb77ded8bf726..914f0be4491c4 100644 --- a/drivers/gpu/drm/i915/display/intel_dsb.c +++ b/drivers/gpu/drm/i915/display/intel_dsb.c @@ -528,6 +528,18 @@ void intel_dsb_vblank_evade(struct intel_atomic_state *state, int latency = intel_usecs_to_scanlines(&crtc_state->hw.adjusted_mode, 20); int start, end; + /* + * PIPEDSL is reading as 0 when in SRDENT(PSR1) or DEEP_SLEEP(PSR2). On + * wake-up scanline counting starts from vblank_start - 1. We don't know + * if wake-up is already ongoing when evasion starts. In worst case + * PIPEDSL could start reading valid value right after checking the + * scanline. In this scenario we wouldn't have enough time to write all + * registers. To tackle this evade scanline 0 as well. As a drawback we + * have 1 frame delay in flip when waking up. + */ + if (crtc_state->has_psr && !crtc_state->has_panel_replay) + intel_dsb_wait_scanline_out(state, dsb, 0, 0); + if (pre_commit_is_vrr_active(state, crtc)) { int vblank_delay = intel_vrr_vblank_delay(crtc_state);
PIPEDSL is reading as 0 when in SRDENT(PSR1) or DEEP_SLEEP(PSR2). On wake-up scanline counting starts from vblank_start - 1. We don't know if wake-up is already ongoing when evasion starts. In worst case PIPEDSL could start reading valid value right after checking the scanline. In this scenario we wouldn't have enough time to write all registers. To tackle this evade scanline 0 as well. As a drawback we have 1 frame delay in flip when waking up. Signed-off-by: Jouni Högander <jouni.hogander@intel.com> --- drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)