diff mbox series

drm/i915/psr2: Do full fetches when doing async flips

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

Commit Message

Souza, Jose Oct. 27, 2021, 6:48 p.m. UTC
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(-)

Comments

Karthik B S Oct. 28, 2021, 5:59 a.m. UTC | #1
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;
>   }
Ville Syrjala Oct. 28, 2021, 1:32 p.m. UTC | #2
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.
Souza, Jose Oct. 28, 2021, 5:02 p.m. UTC | #3
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.
Ville Syrjala Oct. 28, 2021, 5:38 p.m. UTC | #4
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.
Souza, Jose Oct. 28, 2021, 5:43 p.m. UTC | #5
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.
Ville Syrjala Oct. 28, 2021, 5:46 p.m. UTC | #6
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.
Souza, Jose Oct. 28, 2021, 8:18 p.m. UTC | #7
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.
>
Ville Syrjala Oct. 29, 2021, 6:22 a.m. UTC | #8
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.
Souza, Jose Oct. 29, 2021, 10:55 p.m. UTC | #9
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.

>
Souza, Jose Oct. 30, 2021, 12:15 a.m. UTC | #10
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 mbox series

Patch

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;
 }