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

[Ville Syrjälä]

On Fri, Sep 17, 2021 at 05:02:21PM +0000, Souza, Jose wrote:
> On Fri, 2021-09-17 at 16:04 +0300, Ville Syrjälä wrote:
> > On Thu, Sep 16, 2021 at 05:09:08PM +0000, Souza, Jose wrote:
> > > 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
> > 
> > That's not going to fly. The whole reason for the legacy cursor thing is
> > that X likes to do thousands of cursor updates per frame.
> 
> From user experience perspective there is no issues in converting to atomic commit, those 3 videos that I shared with you have this conversion. 

I don't know what you've tested but the legacy cursor fastpath is very
much needed. We've have numerous bug reports whenever it has
accidentally regressed, and I've witnessed the carnage myself as well.
Hmm, I guess you didn't actually disable it fully. To do that you
would have to clear state->legacy_cursor_update explicitly somewhere.

Either way I just retested the earlier patches just with the nonblocking
commit for dirtyfb hacked in, and I left the cursor code using the
half fast path you made it take. The user experience is still as bad
as before. Just moving the mouse around makes glxgears stutter, and the
reported fps drops to ~400 from that alone. And doing anything more
involved like moving windows around is still a total fail.

-- 
Ville Syrjälä
Intel