Re: [Intel-gfx] [PATCH v2 3/5] drm/i915/display: Workaround cursor left overs with PSR2 selective fetch enabled

["Souza, Jose" <jose.souza@intel.com>]

On Thu, 2021-09-16 at 16:17 +0300, Ville Syrjälä wrote:
> On Wed, Sep 15, 2021 at 06:18:35PM +0000, Souza, Jose wrote:
> > On Wed, 2021-09-15 at 17:58 +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 14, 2021 at 02:25:05PM -0700, José Roberto de Souza wrote:
> > > > 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, so leaving this as a
> > > > workaround until proper fix is found.
> > > 
> > > Have you tried warping the cursor clear across the screen while
> > > a partial update is already pending? I think it will go badly.
> > 
> > You mean move the cursor for example from 0x0 to 500x500 in one frame?
> > It will mark as damaged the previous area and the new one.
> 
> Legacy cursor updates bypass all that stuff so you're not going to
> updating the sel fetch area for the other planes.
> 
> > 
> > > 
> > > In fact I'm thinking the mailbox style legacy cursor updates are just
> > > fundementally incompatible with partial updates since the cursor
> > > can move outside of the already committed update region any time.
> > > Ie. I suspect while the cursor is visible we simply can't do partial
> > > updates.
> > 
> > Probably I did not understand what you want to say, but each cursor update will be in one frame, updating the necessary area.
> 
> The legacy cursor uses mailbox updates so there is no 1:1 relationship
> between actual scanned out frames and cursor ioctl calls. You can
> have umpteen thousand cursor updates per frame.

Not if intel_legacy_cursor_update() is changed to go to the slow path and do one atomic commit for each move.
https://patchwork.freedesktop.org/patch/453192/?series=94522&rev=1

I believe compositors will do a single atomic commit updating cursor and all the other planes into a single commit.

> 
> > So I can't understand why it is incompatible.
> 
> Because all the other planes already got their selective fetch area
> updated earlir, and now the cursor's new update area is something
> totally different. Doesn't the total area need to computed to comprise
> of a single crtc space rectangle essentially?
> 
> So we could start with something like:
> 
> sel fetch area
>  |
>  v
>  ________________
> > ________       |
> > > plane |      |
> > >       |_____ |
> > _________     ||
> >   _   | plane ||
> >  |_|  |_______||
> > ________________|   
>     ^
>     |
> cursor
> 
> Then the cursor moves before the hardware has even latched the previous
> update:
> 
> old sel fetch area
>  |
>  v
>  ________________
> > ________       |
> > > plane |      |    _
> > >       |_____ |   |_|
> > _________     ||    ^
> >       | plane ||    |
> >       |_______||   cursor
> > ________________|    
>