Re: [Intel-gfx] [PATCH v2 5/9] drm/i915/display: Fix glitches when moving cursor with PSR2 selective fetch enabled

[Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>]

Looks good to me.
Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>


On 9/30/21 10:56 AM, Ville Syrjälä wrote:
> On Wed, Sep 29, 2021 at 05:14:05PM -0700, José Roberto de Souza wrote:
>> Legacy cursor APIs are handled by intel_legacy_cursor_update(), that
>> calls drm_atomic_helper_update_plane() when going through the
>> slow/atomic path to update cursor, what was the case for PSR2
>> selective fetch.
>>
>> drm_atomic_helper_update_plane() sets
>> drm_atomic_state->legacy_cursor_update to true when updating the
>> cursor plane, to allow several cursor updates to happen within the
>> same frame, as userspace does that.
>> If drivers waited for a vblank increment at the end of every cursor
>> movement that would cause a visible lag in the cursor.
>>
>> But this optimization do not properly work with PSR2 selective fetch
>> dirt area calculation, for example if within a single frame the cursor
>> had 3 moves the final dirt area programmed to PSR2_MAN_TRK_CTL would
>> be based in the second movement as old state and third movement as new
>> state, not updating the area where cursor was in the first state.
>>
>> So here switching back to the fast path approach in
>> intel_legacy_cursor_update() and handling cursor movements as
>> frontbuffer rendering(psr_force_hw_tracking_exit()), that is not the
>> most optimal for power-savings but is the solution that we have until
>> mailbox style updates is implemented.
>>
>> Also removing the cursor workaround as not it is properly undestand
>> the issue and is know that it will never cover all the cases.
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> 
> Not really familiar with the PSR details to give a full review
> on those parts, but the approach looks OK to me.
> 
> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
>> ---
>>   drivers/gpu/drm/i915/display/intel_cursor.c   |  5 +-
>>   drivers/gpu/drm/i915/display/intel_fbc.c      |  4 +-
>>   .../gpu/drm/i915/display/intel_frontbuffer.h  |  1 +
>>   drivers/gpu/drm/i915/display/intel_psr.c      | 59 +++++--------------
>>   4 files changed, 20 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
>> index 901ad3a4c8c3b..f6dcb5aa63f64 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
>> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
>> @@ -639,8 +639,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>>   	 * FIXME bigjoiner fastpath would be good
>>   	 */
>>   	if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) ||
>> -	    crtc_state->update_pipe || crtc_state->bigjoiner ||
>> -	    crtc_state->enable_psr2_sel_fetch)
>> +	    crtc_state->update_pipe || crtc_state->bigjoiner)
>>   		goto slow;
>>   
>>   	/*
>> @@ -698,7 +697,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>>   		goto out_free;
>>   
>>   	intel_frontbuffer_flush(to_intel_frontbuffer(new_plane_state->hw.fb),
>> -				ORIGIN_FLIP);
>> +				ORIGIN_CURSOR_UPDATE);
>>   	intel_frontbuffer_track(to_intel_frontbuffer(old_plane_state->hw.fb),
>>   				to_intel_frontbuffer(new_plane_state->hw.fb),
>>   				plane->frontbuffer_bit);
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
>> index 46f62fdf9eeeb..77b00e3a92c23 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
>> @@ -1199,7 +1199,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>>   	if (!HAS_FBC(dev_priv))
>>   		return;
>>   
>> -	if (origin == ORIGIN_FLIP)
>> +	if (origin == ORIGIN_FLIP || origin == ORIGIN_CURSOR_UPDATE)
>>   		return;
>>   
>>   	mutex_lock(&fbc->lock);
>> @@ -1224,7 +1224,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>>   
>>   	fbc->busy_bits &= ~frontbuffer_bits;
>>   
>> -	if (origin == ORIGIN_FLIP)
>> +	if (origin == ORIGIN_FLIP || origin == ORIGIN_CURSOR_UPDATE)
>>   		goto out;
>>   
>>   	if (!fbc->busy_bits && fbc->crtc &&
>> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
>> index 4b977c1e4d52b..a88441edc8f94 100644
>> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
>> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
>> @@ -37,6 +37,7 @@ enum fb_op_origin {
>>   	ORIGIN_CS,
>>   	ORIGIN_FLIP,
>>   	ORIGIN_DIRTYFB,
>> +	ORIGIN_CURSOR_UPDATE,
>>   };
>>   
>>   struct intel_frontbuffer {
>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> index 7185801d5deff..e8af39591dfea 100644
>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> @@ -1558,28 +1558,6 @@ static void intel_psr2_sel_fetch_pipe_alignment(const struct intel_crtc_state *c
>>   		drm_warn(&dev_priv->drm, "Missing PSR2 sel fetch alignment with DSC\n");
>>   }
>>   
>> -/*
>> - * FIXME: Not sure why but when moving the cursor fast it causes some artifacts
>> - * of the cursor to be left in the cursor path, adding some pixels above the
>> - * cursor to the damaged area fixes the issue.
>> - */
>> -static void cursor_area_workaround(const struct intel_plane_state *new_plane_state,
>> -				   struct drm_rect *damaged_area,
>> -				   struct drm_rect *pipe_clip)
>> -{
>> -	const struct intel_plane *plane = to_intel_plane(new_plane_state->uapi.plane);
>> -	int height;
>> -
>> -	if (plane->id != PLANE_CURSOR)
>> -		return;
>> -
>> -	height = drm_rect_height(&new_plane_state->uapi.dst) / 2;
>> -	damaged_area->y1 -=  height;
>> -	damaged_area->y1 = max(damaged_area->y1, 0);
>> -
>> -	clip_area_update(pipe_clip, damaged_area);
>> -}
>> -
>>   /*
>>    * TODO: Not clear how to handle planes with negative position,
>>    * also planes are not updated if they have a negative X
>> @@ -1680,9 +1658,6 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
>>   				damaged_area.y2 = new_plane_state->uapi.dst.y2;
>>   				clip_area_update(&pipe_clip, &damaged_area);
>>   			}
>> -
>> -			cursor_area_workaround(new_plane_state, &damaged_area,
>> -					       &pipe_clip);
>>   			continue;
>>   		} else if (new_plane_state->uapi.alpha != old_plane_state->uapi.alpha) {
>>   			/* If alpha changed mark the whole plane area as damaged */
>> @@ -2116,20 +2091,16 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
>>   /*
>>    * When we will be completely rely on PSR2 S/W tracking in future,
>>    * intel_psr_flush() will invalidate and flush the PSR for ORIGIN_FLIP
>> - * event also therefore tgl_dc3co_flush() require to be changed
>> + * event also therefore tgl_dc3co_flush_locked() require to be changed
>>    * accordingly in future.
>>    */
>>   static void
>> -tgl_dc3co_flush(struct intel_dp *intel_dp, unsigned int frontbuffer_bits,
>> -		enum fb_op_origin origin)
>> +tgl_dc3co_flush_locked(struct intel_dp *intel_dp, unsigned int frontbuffer_bits,
>> +		       enum fb_op_origin origin)
>>   {
>> -	mutex_lock(&intel_dp->psr.lock);
>> -
>> -	if (!intel_dp->psr.dc3co_exitline)
>> -		goto unlock;
>> -
>> -	if (!intel_dp->psr.psr2_enabled || !intel_dp->psr.active)
>> -		goto unlock;
>> +	if (!intel_dp->psr.dc3co_exitline || !intel_dp->psr.psr2_enabled ||
>> +	    !intel_dp->psr.active)
>> +		return;
>>   
>>   	/*
>>   	 * At every frontbuffer flush flip event modified delay of delayed work,
>> @@ -2137,14 +2108,11 @@ tgl_dc3co_flush(struct intel_dp *intel_dp, unsigned int frontbuffer_bits,
>>   	 */
>>   	if (!(frontbuffer_bits &
>>   	    INTEL_FRONTBUFFER_ALL_MASK(intel_dp->psr.pipe)))
>> -		goto unlock;
>> +		return;
>>   
>>   	tgl_psr2_enable_dc3co(intel_dp);
>>   	mod_delayed_work(system_wq, &intel_dp->psr.dc3co_work,
>>   			 intel_dp->psr.dc3co_exit_delay);
>> -
>> -unlock:
>> -	mutex_unlock(&intel_dp->psr.lock);
>>   }
>>   
>>   /**
>> @@ -2169,11 +2137,6 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>>   		unsigned int pipe_frontbuffer_bits = frontbuffer_bits;
>>   		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>   
>> -		if (origin == ORIGIN_FLIP) {
>> -			tgl_dc3co_flush(intel_dp, frontbuffer_bits, origin);
>> -			continue;
>> -		}
>> -
>>   		mutex_lock(&intel_dp->psr.lock);
>>   		if (!intel_dp->psr.enabled) {
>>   			mutex_unlock(&intel_dp->psr.lock);
>> @@ -2194,6 +2157,14 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>>   			continue;
>>   		}
>>   
>> +		if (origin == ORIGIN_FLIP ||
>> +		    (origin == ORIGIN_CURSOR_UPDATE &&
>> +		     !intel_dp->psr.psr2_sel_fetch_enabled)) {
>> +			tgl_dc3co_flush_locked(intel_dp, frontbuffer_bits, origin);
>> +			mutex_unlock(&intel_dp->psr.lock);
>> +			continue;
>> +		}
>> +
>>   		/* By definition flush = invalidate + flush */
>>   		if (pipe_frontbuffer_bits)
>>   			psr_force_hw_tracking_exit(intel_dp);
>> -- 
>> 2.33.0
>