| Message ID | 20211027184855.108731-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/psr2: Do full fetches when doing async flips | expand |
On 10/28/2021 12:18 AM, José Roberto de Souza wrote: > Async flips are not supported by selective fetch and we had a check > for that but that check was only executed when doing modesets. > So moving this check to the page flip path, so it can be properly > handled. > > This fix a failure in kms_async_flips@test-cursor. > > Cc: Mika Kahola <mika.kahola@intel.com> > Cc: Jouni Hogander <jouni.hogander@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> Reviewed-by: Karthik B S <karthik.b.s@intel.com> > --- > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > index 8d08e3cf08c1f..ce6850ed72c60 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -729,12 +729,6 @@ static bool intel_psr2_sel_fetch_config_valid(struct intel_dp *intel_dp, > return false; > } > > - if (crtc_state->uapi.async_flip) { > - drm_dbg_kms(&dev_priv->drm, > - "PSR2 sel fetch not enabled, async flip enabled\n"); > - return false; > - } > - > /* Wa_14010254185 Wa_14010103792 */ > if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) { > drm_dbg_kms(&dev_priv->drm, > @@ -1592,6 +1586,8 @@ static bool psr2_sel_fetch_pipe_state_supported(const struct intel_crtc_state *c > { > if (crtc_state->scaler_state.scaler_id >= 0) > return false; > + if (crtc_state->uapi.async_flip) > + return false; > > return true; > }
On Wed, Oct 27, 2021 at 11:48:55AM -0700, José Roberto de Souza wrote: > Async flips are not supported by selective fetch and we had a check > for that but that check was only executed when doing modesets. > So moving this check to the page flip path, so it can be properly > handled. > > This fix a failure in kms_async_flips@test-cursor. > > Cc: Mika Kahola <mika.kahola@intel.com> > Cc: Jouni Hogander <jouni.hogander@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > index 8d08e3cf08c1f..ce6850ed72c60 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -729,12 +729,6 @@ static bool intel_psr2_sel_fetch_config_valid(struct intel_dp *intel_dp, > return false; > } > > - if (crtc_state->uapi.async_flip) { > - drm_dbg_kms(&dev_priv->drm, > - "PSR2 sel fetch not enabled, async flip enabled\n"); > - return false; > - } > - > /* Wa_14010254185 Wa_14010103792 */ > if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) { > drm_dbg_kms(&dev_priv->drm, > @@ -1592,6 +1586,8 @@ static bool psr2_sel_fetch_pipe_state_supported(const struct intel_crtc_state *c > { > if (crtc_state->scaler_state.scaler_id >= 0) > return false; > + if (crtc_state->uapi.async_flip) > + return false; This looks dodgy. Pretty sure we can't turn off this thing during an async flip. So I think the correct short term fix is to not do async flips with psr2 enabled. The longer term fix would involve using the same approach Stan is preparing for the async flip watermark tweaking, which is to convert the first async flip into a sync flip.
On Thu, 2021-10-28 at 16:32 +0300, Ville Syrjälä wrote: > On Wed, Oct 27, 2021 at 11:48:55AM -0700, José Roberto de Souza wrote: > > Async flips are not supported by selective fetch and we had a check > > for that but that check was only executed when doing modesets. > > So moving this check to the page flip path, so it can be properly > > handled. > > > > This fix a failure in kms_async_flips@test-cursor. > > > > Cc: Mika Kahola <mika.kahola@intel.com> > > Cc: Jouni Hogander <jouni.hogander@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------ > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > index 8d08e3cf08c1f..ce6850ed72c60 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -729,12 +729,6 @@ static bool intel_psr2_sel_fetch_config_valid(struct intel_dp *intel_dp, > > return false; > > } > > > > - if (crtc_state->uapi.async_flip) { > > - drm_dbg_kms(&dev_priv->drm, > > - "PSR2 sel fetch not enabled, async flip enabled\n"); > > - return false; > > - } > > - > > /* Wa_14010254185 Wa_14010103792 */ > > if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) { > > drm_dbg_kms(&dev_priv->drm, > > @@ -1592,6 +1586,8 @@ static bool psr2_sel_fetch_pipe_state_supported(const struct intel_crtc_state *c > > { > > if (crtc_state->scaler_state.scaler_id >= 0) > > return false; > > + if (crtc_state->uapi.async_flip) > > + return false; > > This looks dodgy. Pretty sure we can't turn off this thing during > an async flip. So I think the correct short term fix is to not do > async flips with psr2 enabled. The longer term fix would involve > using the same approach Stan is preparing for the async flip > watermark tweaking, which is to convert the first async flip into > a sync flip. > It is not turning PSR2 off, it is telling hardware to fetch the whole memory of all planes and send the whole screen to panel instead of fetching and sending a smaller area for this frame, we also do that when a plane is moved to a negative coordinate.
On Thu, Oct 28, 2021 at 05:02:41PM +0000, Souza, Jose wrote: > On Thu, 2021-10-28 at 16:32 +0300, Ville Syrjälä wrote: > > On Wed, Oct 27, 2021 at 11:48:55AM -0700, José Roberto de Souza wrote: > > > Async flips are not supported by selective fetch and we had a check > > > for that but that check was only executed when doing modesets. > > > So moving this check to the page flip path, so it can be properly > > > handled. > > > > > > This fix a failure in kms_async_flips@test-cursor. > > > > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > Cc: Jouni Hogander <jouni.hogander@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------ > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > > index 8d08e3cf08c1f..ce6850ed72c60 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -729,12 +729,6 @@ static bool intel_psr2_sel_fetch_config_valid(struct intel_dp *intel_dp, > > > return false; > > > } > > > > > > - if (crtc_state->uapi.async_flip) { > > > - drm_dbg_kms(&dev_priv->drm, > > > - "PSR2 sel fetch not enabled, async flip enabled\n"); > > > - return false; > > > - } > > > - > > > /* Wa_14010254185 Wa_14010103792 */ > > > if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) { > > > drm_dbg_kms(&dev_priv->drm, > > > @@ -1592,6 +1586,8 @@ static bool psr2_sel_fetch_pipe_state_supported(const struct intel_crtc_state *c > > > { > > > if (crtc_state->scaler_state.scaler_id >= 0) > > > return false; > > > + if (crtc_state->uapi.async_flip) > > > + return false; > > > > This looks dodgy. Pretty sure we can't turn off this thing during > > an async flip. So I think the correct short term fix is to not do > > async flips with psr2 enabled. The longer term fix would involve > > using the same approach Stan is preparing for the async flip > > watermark tweaking, which is to convert the first async flip into > > a sync flip. > > > > It is not turning PSR2 off, it is telling hardware to fetch the whole memory of all planes and send the whole screen to panel instead of fetching and > sending a smaller area for this frame, we also do that when a plane is moved to a negative coordinate. Doesn't matter. Whatever register is rsponsible for this is presumably latched at the next vblank which is after the async flip already happened.
On Thu, 2021-10-28 at 20:38 +0300, Ville Syrjälä wrote: > On Thu, Oct 28, 2021 at 05:02:41PM +0000, Souza, Jose wrote: > > On Thu, 2021-10-28 at 16:32 +0300, Ville Syrjälä wrote: > > > On Wed, Oct 27, 2021 at 11:48:55AM -0700, José Roberto de Souza wrote: > > > > Async flips are not supported by selective fetch and we had a check > > > > for that but that check was only executed when doing modesets. > > > > So moving this check to the page flip path, so it can be properly > > > > handled. > > > > > > > > This fix a failure in kms_async_flips@test-cursor. > > > > > > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > > Cc: Jouni Hogander <jouni.hogander@intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------ > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > > > index 8d08e3cf08c1f..ce6850ed72c60 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > @@ -729,12 +729,6 @@ static bool intel_psr2_sel_fetch_config_valid(struct intel_dp *intel_dp, > > > > return false; > > > > } > > > > > > > > - if (crtc_state->uapi.async_flip) { > > > > - drm_dbg_kms(&dev_priv->drm, > > > > - "PSR2 sel fetch not enabled, async flip enabled\n"); > > > > - return false; > > > > - } > > > > - > > > > /* Wa_14010254185 Wa_14010103792 */ > > > > if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) { > > > > drm_dbg_kms(&dev_priv->drm, > > > > @@ -1592,6 +1586,8 @@ static bool psr2_sel_fetch_pipe_state_supported(const struct intel_crtc_state *c > > > > { > > > > if (crtc_state->scaler_state.scaler_id >= 0) > > > > return false; > > > > + if (crtc_state->uapi.async_flip) > > > > + return false; > > > > > > This looks dodgy. Pretty sure we can't turn off this thing during > > > an async flip. So I think the correct short term fix is to not do > > > async flips with psr2 enabled. The longer term fix would involve > > > using the same approach Stan is preparing for the async flip > > > watermark tweaking, which is to convert the first async flip into > > > a sync flip. > > > > > > > It is not turning PSR2 off, it is telling hardware to fetch the whole memory of all planes and send the whole screen to panel instead of fetching and > > sending a smaller area for this frame, we also do that when a plane is moved to a negative coordinate. > > Doesn't matter. Whatever register is rsponsible for this is presumably latched > at the next vblank which is after the async flip already happened. > That is exactly what BSpec 55229 asks us to do: Not supported with async flips. The plane size and position cannot be changed with async flips, so selective fetch cannot be used. Software must output a full frame for async flips.
On Thu, Oct 28, 2021 at 05:43:51PM +0000, Souza, Jose wrote: > On Thu, 2021-10-28 at 20:38 +0300, Ville Syrjälä wrote: > > On Thu, Oct 28, 2021 at 05:02:41PM +0000, Souza, Jose wrote: > > > On Thu, 2021-10-28 at 16:32 +0300, Ville Syrjälä wrote: > > > > On Wed, Oct 27, 2021 at 11:48:55AM -0700, José Roberto de Souza wrote: > > > > > Async flips are not supported by selective fetch and we had a check > > > > > for that but that check was only executed when doing modesets. > > > > > So moving this check to the page flip path, so it can be properly > > > > > handled. > > > > > > > > > > This fix a failure in kms_async_flips@test-cursor. > > > > > > > > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > > > Cc: Jouni Hogander <jouni.hogander@intel.com> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------ > > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > index 8d08e3cf08c1f..ce6850ed72c60 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > @@ -729,12 +729,6 @@ static bool intel_psr2_sel_fetch_config_valid(struct intel_dp *intel_dp, > > > > > return false; > > > > > } > > > > > > > > > > - if (crtc_state->uapi.async_flip) { > > > > > - drm_dbg_kms(&dev_priv->drm, > > > > > - "PSR2 sel fetch not enabled, async flip enabled\n"); > > > > > - return false; > > > > > - } > > > > > - > > > > > /* Wa_14010254185 Wa_14010103792 */ > > > > > if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) { > > > > > drm_dbg_kms(&dev_priv->drm, > > > > > @@ -1592,6 +1586,8 @@ static bool psr2_sel_fetch_pipe_state_supported(const struct intel_crtc_state *c > > > > > { > > > > > if (crtc_state->scaler_state.scaler_id >= 0) > > > > > return false; > > > > > + if (crtc_state->uapi.async_flip) > > > > > + return false; > > > > > > > > This looks dodgy. Pretty sure we can't turn off this thing during > > > > an async flip. So I think the correct short term fix is to not do > > > > async flips with psr2 enabled. The longer term fix would involve > > > > using the same approach Stan is preparing for the async flip > > > > watermark tweaking, which is to convert the first async flip into > > > > a sync flip. > > > > > > > > > > It is not turning PSR2 off, it is telling hardware to fetch the whole memory of all planes and send the whole screen to panel instead of fetching and > > > sending a smaller area for this frame, we also do that when a plane is moved to a negative coordinate. > > > > Doesn't matter. Whatever register is rsponsible for this is presumably latched > > at the next vblank which is after the async flip already happened. > > > > That is exactly what BSpec 55229 asks us to do: > > Not supported with async flips. The plane size and position cannot be changed with async flips, so selective fetch cannot be used. Software must > output a full frame for async flips. It doesn't tell us to do it wrong.
On Thu, 2021-10-28 at 20:46 +0300, Ville Syrjälä wrote: > On Thu, Oct 28, 2021 at 05:43:51PM +0000, Souza, Jose wrote: > > On Thu, 2021-10-28 at 20:38 +0300, Ville Syrjälä wrote: > > > On Thu, Oct 28, 2021 at 05:02:41PM +0000, Souza, Jose wrote: > > > > On Thu, 2021-10-28 at 16:32 +0300, Ville Syrjälä wrote: > > > > > On Wed, Oct 27, 2021 at 11:48:55AM -0700, José Roberto de Souza wrote: > > > > > > Async flips are not supported by selective fetch and we had a check > > > > > > for that but that check was only executed when doing modesets. > > > > > > So moving this check to the page flip path, so it can be properly > > > > > > handled. > > > > > > > > > > > > This fix a failure in kms_async_flips@test-cursor. > > > > > > > > > > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > > > > Cc: Jouni Hogander <jouni.hogander@intel.com> > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------ > > > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > > index 8d08e3cf08c1f..ce6850ed72c60 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > > @@ -729,12 +729,6 @@ static bool intel_psr2_sel_fetch_config_valid(struct intel_dp *intel_dp, > > > > > > return false; > > > > > > } > > > > > > > > > > > > - if (crtc_state->uapi.async_flip) { > > > > > > - drm_dbg_kms(&dev_priv->drm, > > > > > > - "PSR2 sel fetch not enabled, async flip enabled\n"); > > > > > > - return false; > > > > > > - } > > > > > > - > > > > > > /* Wa_14010254185 Wa_14010103792 */ > > > > > > if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) { > > > > > > drm_dbg_kms(&dev_priv->drm, > > > > > > @@ -1592,6 +1586,8 @@ static bool psr2_sel_fetch_pipe_state_supported(const struct intel_crtc_state *c > > > > > > { > > > > > > if (crtc_state->scaler_state.scaler_id >= 0) > > > > > > return false; > > > > > > + if (crtc_state->uapi.async_flip) > > > > > > + return false; > > > > > > > > > > This looks dodgy. Pretty sure we can't turn off this thing during > > > > > an async flip. So I think the correct short term fix is to not do > > > > > async flips with psr2 enabled. The longer term fix would involve > > > > > using the same approach Stan is preparing for the async flip > > > > > watermark tweaking, which is to convert the first async flip into > > > > > a sync flip. You mean do something like this? diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 3b5a8e971343f..7d29f8c9de0da 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -7710,7 +7710,8 @@ static void kill_bigjoiner_slave(struct intel_atomic_state *state, static int intel_atomic_check_async(struct intel_atomic_state *state, struct intel_crtc *crtc) { struct drm_i915_private *i915 = to_i915(state->base.dev); - const struct intel_crtc_state *old_crtc_state, *new_crtc_state; + const struct intel_crtc_state *old_crtc_state; + struct intel_crtc_state *new_crtc_state; const struct intel_plane_state *new_plane_state, *old_plane_state; struct intel_plane *plane; int i; @@ -7718,6 +7719,12 @@ static int intel_atomic_check_async(struct intel_atomic_state *state, struct int old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); + if (new_crtc_state->enable_psr2_sel_fetch) { + drm_dbg_kms(&i915->drm, "PSR2 selective fetch not compatible with async flip, doing a sync flip instead\n"); + new_crtc_state->uapi.async_flip = false; + return 0; + } + if (intel_crtc_needs_modeset(new_crtc_state)) { drm_dbg_kms(&i915->drm, "Modeset Required. Async flip not supported\n"); return -EINVAL; diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index ce6850ed72c60..00e69421b9648 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -1586,8 +1586,6 @@ static bool psr2_sel_fetch_pipe_state_supported(const struct intel_crtc_state *c { if (crtc_state->scaler_state.scaler_id >= 0) return false; - if (crtc_state->uapi.async_flip) - return false; return true; } This is causing other kms_async_flips tests to fail, the ones that checks the async flip time interval. > > > > > > > > > > > > > It is not turning PSR2 off, it is telling hardware to fetch the whole memory of all planes and send the whole screen to panel instead of fetching and > > > > sending a smaller area for this frame, we also do that when a plane is moved to a negative coordinate. > > > > > > Doesn't matter. Whatever register is rsponsible for this is presumably latched > > > at the next vblank which is after the async flip already happened. > > > > > > > That is exactly what BSpec 55229 asks us to do: > > > > Not supported with async flips. The plane size and position cannot be changed with async flips, so selective fetch cannot be used. Software must > > output a full frame for async flips. > > It doesn't tell us to do it wrong. >
On Thu, Oct 28, 2021 at 08:18:48PM +0000, Souza, Jose wrote: > On Thu, 2021-10-28 at 20:46 +0300, Ville Syrjälä wrote: > > On Thu, Oct 28, 2021 at 05:43:51PM +0000, Souza, Jose wrote: > > > On Thu, 2021-10-28 at 20:38 +0300, Ville Syrjälä wrote: > > > > On Thu, Oct 28, 2021 at 05:02:41PM +0000, Souza, Jose wrote: > > > > > On Thu, 2021-10-28 at 16:32 +0300, Ville Syrjälä wrote: > > > > > > On Wed, Oct 27, 2021 at 11:48:55AM -0700, José Roberto de Souza wrote: > > > > > > > Async flips are not supported by selective fetch and we had a check > > > > > > > for that but that check was only executed when doing modesets. > > > > > > > So moving this check to the page flip path, so it can be properly > > > > > > > handled. > > > > > > > > > > > > > > This fix a failure in kms_async_flips@test-cursor. > > > > > > > > > > > > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > > > > > Cc: Jouni Hogander <jouni.hogander@intel.com> > > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------ > > > > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > > > index 8d08e3cf08c1f..ce6850ed72c60 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > > > @@ -729,12 +729,6 @@ static bool intel_psr2_sel_fetch_config_valid(struct intel_dp *intel_dp, > > > > > > > return false; > > > > > > > } > > > > > > > > > > > > > > - if (crtc_state->uapi.async_flip) { > > > > > > > - drm_dbg_kms(&dev_priv->drm, > > > > > > > - "PSR2 sel fetch not enabled, async flip enabled\n"); > > > > > > > - return false; > > > > > > > - } > > > > > > > - > > > > > > > /* Wa_14010254185 Wa_14010103792 */ > > > > > > > if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) { > > > > > > > drm_dbg_kms(&dev_priv->drm, > > > > > > > @@ -1592,6 +1586,8 @@ static bool psr2_sel_fetch_pipe_state_supported(const struct intel_crtc_state *c > > > > > > > { > > > > > > > if (crtc_state->scaler_state.scaler_id >= 0) > > > > > > > return false; > > > > > > > + if (crtc_state->uapi.async_flip) > > > > > > > + return false; > > > > > > > > > > > > This looks dodgy. Pretty sure we can't turn off this thing during > > > > > > an async flip. So I think the correct short term fix is to not do > > > > > > async flips with psr2 enabled. The longer term fix would involve > > > > > > using the same approach Stan is preparing for the async flip > > > > > > watermark tweaking, which is to convert the first async flip into > > > > > > a sync flip. > > You mean do something like this? > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 3b5a8e971343f..7d29f8c9de0da 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -7710,7 +7710,8 @@ static void kill_bigjoiner_slave(struct intel_atomic_state *state, > static int intel_atomic_check_async(struct intel_atomic_state *state, struct intel_crtc *crtc) > { > struct drm_i915_private *i915 = to_i915(state->base.dev); > - const struct intel_crtc_state *old_crtc_state, *new_crtc_state; > + const struct intel_crtc_state *old_crtc_state; > + struct intel_crtc_state *new_crtc_state; > const struct intel_plane_state *new_plane_state, *old_plane_state; > struct intel_plane *plane; > int i; > @@ -7718,6 +7719,12 @@ static int intel_atomic_check_async(struct intel_atomic_state *state, struct int > old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > > + if (new_crtc_state->enable_psr2_sel_fetch) { > + drm_dbg_kms(&i915->drm, "PSR2 selective fetch not compatible with async flip, doing a sync flip instead\n"); > + new_crtc_state->uapi.async_flip = false; > + return 0; > + } It should just return -EINVAL here. And I'd put the somewhere after the needs_modeset/hw.active checks to keep things in some kind of reasonable order.
On Fri, 2021-10-29 at 09:22 +0300, Ville Syrjälä wrote: > On Thu, Oct 28, 2021 at 08:18:48PM +0000, Souza, Jose wrote: > > On Thu, 2021-10-28 at 20:46 +0300, Ville Syrjälä wrote: > > > On Thu, Oct 28, 2021 at 05:43:51PM +0000, Souza, Jose wrote: > > > > On Thu, 2021-10-28 at 20:38 +0300, Ville Syrjälä wrote: > > > > > On Thu, Oct 28, 2021 at 05:02:41PM +0000, Souza, Jose wrote: > > > > > > On Thu, 2021-10-28 at 16:32 +0300, Ville Syrjälä wrote: > > > > > > > On Wed, Oct 27, 2021 at 11:48:55AM -0700, José Roberto de Souza wrote: > > > > > > > > Async flips are not supported by selective fetch and we had a check > > > > > > > > for that but that check was only executed when doing modesets. > > > > > > > > So moving this check to the page flip path, so it can be properly > > > > > > > > handled. > > > > > > > > > > > > > > > > This fix a failure in kms_async_flips@test-cursor. > > > > > > > > > > > > > > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > > > > > > Cc: Jouni Hogander <jouni.hogander@intel.com> > > > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > > > > --- > > > > > > > > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------ > > > > > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > > > > index 8d08e3cf08c1f..ce6850ed72c60 100644 > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > > > > @@ -729,12 +729,6 @@ static bool intel_psr2_sel_fetch_config_valid(struct intel_dp *intel_dp, > > > > > > > > return false; > > > > > > > > } > > > > > > > > > > > > > > > > - if (crtc_state->uapi.async_flip) { > > > > > > > > - drm_dbg_kms(&dev_priv->drm, > > > > > > > > - "PSR2 sel fetch not enabled, async flip enabled\n"); > > > > > > > > - return false; > > > > > > > > - } > > > > > > > > - > > > > > > > > /* Wa_14010254185 Wa_14010103792 */ > > > > > > > > if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) { > > > > > > > > drm_dbg_kms(&dev_priv->drm, > > > > > > > > @@ -1592,6 +1586,8 @@ static bool psr2_sel_fetch_pipe_state_supported(const struct intel_crtc_state *c > > > > > > > > { > > > > > > > > if (crtc_state->scaler_state.scaler_id >= 0) > > > > > > > > return false; > > > > > > > > + if (crtc_state->uapi.async_flip) > > > > > > > > + return false; > > > > > > > > > > > > > > This looks dodgy. Pretty sure we can't turn off this thing during > > > > > > > an async flip. So I think the correct short term fix is to not do > > > > > > > async flips with psr2 enabled. The longer term fix would involve > > > > > > > using the same approach Stan is preparing for the async flip > > > > > > > watermark tweaking, which is to convert the first async flip into > > > > > > > a sync flip. > > > > You mean do something like this? > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 3b5a8e971343f..7d29f8c9de0da 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -7710,7 +7710,8 @@ static void kill_bigjoiner_slave(struct intel_atomic_state *state, > > static int intel_atomic_check_async(struct intel_atomic_state *state, struct intel_crtc *crtc) > > { > > struct drm_i915_private *i915 = to_i915(state->base.dev); > > - const struct intel_crtc_state *old_crtc_state, *new_crtc_state; > > + const struct intel_crtc_state *old_crtc_state; > > + struct intel_crtc_state *new_crtc_state; > > const struct intel_plane_state *new_plane_state, *old_plane_state; > > struct intel_plane *plane; > > int i; > > @@ -7718,6 +7719,12 @@ static int intel_atomic_check_async(struct intel_atomic_state *state, struct int > > old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > > new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > > > > + if (new_crtc_state->enable_psr2_sel_fetch) { > > + drm_dbg_kms(&i915->drm, "PSR2 selective fetch not compatible with async flip, doing a sync flip instead\n"); > > + new_crtc_state->uapi.async_flip = false; > > + return 0; > > + } > > It should just return -EINVAL here. And I'd put the somewhere after the > needs_modeset/hw.active checks to keep things in some kind of > reasonable order. Okay, easy do that but that would not cause any issues for desktop environments? We advertise async flip capability but state will always be rejected when PSR2 is enabled. Will also need to switch to PSR1 or skip all kms_async_flip tests when PSR2 selective fetch is enabled. >
On Fri, 2021-10-29 at 22:55 +0000, Souza, Jose wrote: > On Fri, 2021-10-29 at 09:22 +0300, Ville Syrjälä wrote: > > On Thu, Oct 28, 2021 at 08:18:48PM +0000, Souza, Jose wrote: > > > On Thu, 2021-10-28 at 20:46 +0300, Ville Syrjälä wrote: > > > > On Thu, Oct 28, 2021 at 05:43:51PM +0000, Souza, Jose wrote: > > > > > On Thu, 2021-10-28 at 20:38 +0300, Ville Syrjälä wrote: > > > > > > On Thu, Oct 28, 2021 at 05:02:41PM +0000, Souza, Jose wrote: > > > > > > > On Thu, 2021-10-28 at 16:32 +0300, Ville Syrjälä wrote: > > > > > > > > On Wed, Oct 27, 2021 at 11:48:55AM -0700, José Roberto de Souza wrote: > > > > > > > > > Async flips are not supported by selective fetch and we had a check > > > > > > > > > for that but that check was only executed when doing modesets. > > > > > > > > > So moving this check to the page flip path, so it can be properly > > > > > > > > > handled. > > > > > > > > > > > > > > > > > > This fix a failure in kms_async_flips@test-cursor. > > > > > > > > > > > > > > > > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > > > > > > > Cc: Jouni Hogander <jouni.hogander@intel.com> > > > > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > > > > > --- > > > > > > > > > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------ > > > > > > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > > > > > index 8d08e3cf08c1f..ce6850ed72c60 100644 > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > > > > > @@ -729,12 +729,6 @@ static bool intel_psr2_sel_fetch_config_valid(struct intel_dp *intel_dp, > > > > > > > > > return false; > > > > > > > > > } > > > > > > > > > > > > > > > > > > - if (crtc_state->uapi.async_flip) { > > > > > > > > > - drm_dbg_kms(&dev_priv->drm, > > > > > > > > > - "PSR2 sel fetch not enabled, async flip enabled\n"); > > > > > > > > > - return false; > > > > > > > > > - } > > > > > > > > > - > > > > > > > > > /* Wa_14010254185 Wa_14010103792 */ > > > > > > > > > if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) { > > > > > > > > > drm_dbg_kms(&dev_priv->drm, > > > > > > > > > @@ -1592,6 +1586,8 @@ static bool psr2_sel_fetch_pipe_state_supported(const struct intel_crtc_state *c > > > > > > > > > { > > > > > > > > > if (crtc_state->scaler_state.scaler_id >= 0) > > > > > > > > > return false; > > > > > > > > > + if (crtc_state->uapi.async_flip) > > > > > > > > > + return false; > > > > > > > > > > > > > > > > This looks dodgy. Pretty sure we can't turn off this thing during > > > > > > > > an async flip. So I think the correct short term fix is to not do > > > > > > > > async flips with psr2 enabled. The longer term fix would involve > > > > > > > > using the same approach Stan is preparing for the async flip > > > > > > > > watermark tweaking, which is to convert the first async flip into > > > > > > > > a sync flip. > > > > > > You mean do something like this? > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index 3b5a8e971343f..7d29f8c9de0da 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -7710,7 +7710,8 @@ static void kill_bigjoiner_slave(struct intel_atomic_state *state, > > > static int intel_atomic_check_async(struct intel_atomic_state *state, struct intel_crtc *crtc) > > > { > > > struct drm_i915_private *i915 = to_i915(state->base.dev); > > > - const struct intel_crtc_state *old_crtc_state, *new_crtc_state; > > > + const struct intel_crtc_state *old_crtc_state; > > > + struct intel_crtc_state *new_crtc_state; > > > const struct intel_plane_state *new_plane_state, *old_plane_state; > > > struct intel_plane *plane; > > > int i; > > > @@ -7718,6 +7719,12 @@ static int intel_atomic_check_async(struct intel_atomic_state *state, struct int > > > old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > > > new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); > > > > > > + if (new_crtc_state->enable_psr2_sel_fetch) { > > > + drm_dbg_kms(&i915->drm, "PSR2 selective fetch not compatible with async flip, doing a sync flip instead\n"); > > > + new_crtc_state->uapi.async_flip = false; > > > + return 0; > > > + } > > > > It should just return -EINVAL here. And I'd put the somewhere after the > > needs_modeset/hw.active checks to keep things in some kind of > > reasonable order. > > Okay, easy do that but that would not cause any issues for desktop environments? > We advertise async flip capability but state will always be rejected when PSR2 is enabled. > > Will also need to switch to PSR1 or skip all kms_async_flip tests when PSR2 selective fetch is enabled. Please disregard the above comment and this whole patch. Did more debug and found better ways to solve all async flip + PSR problems. IGT patches will change the failure to skip as we don't have a way around it: https://patchwork.freedesktop.org/series/96439/ Kernel patch fixing possible corruptions with async flip: https://patchwork.freedesktop.org/series/96440/ > > > >
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index 8d08e3cf08c1f..ce6850ed72c60 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -729,12 +729,6 @@ static bool intel_psr2_sel_fetch_config_valid(struct intel_dp *intel_dp, return false; } - if (crtc_state->uapi.async_flip) { - drm_dbg_kms(&dev_priv->drm, - "PSR2 sel fetch not enabled, async flip enabled\n"); - return false; - } - /* Wa_14010254185 Wa_14010103792 */ if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) { drm_dbg_kms(&dev_priv->drm, @@ -1592,6 +1586,8 @@ static bool psr2_sel_fetch_pipe_state_supported(const struct intel_crtc_state *c { if (crtc_state->scaler_state.scaler_id >= 0) return false; + if (crtc_state->uapi.async_flip) + return false; return true; }
Async flips are not supported by selective fetch and we had a check for that but that check was only executed when doing modesets. So moving this check to the page flip path, so it can be properly handled. This fix a failure in kms_async_flips@test-cursor. Cc: Mika Kahola <mika.kahola@intel.com> Cc: Jouni Hogander <jouni.hogander@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)