On Wed, 2021-09-22 at 16:41 +0300, Ville Syrjälä wrote: > On Tue, Sep 21, 2021 at 10:37:53PM +0000, Souza, Jose wrote: > > On Tue, 2021-09-21 at 16:35 +0300, Ville Syrjälä wrote: > > > On Fri, Sep 17, 2021 at 09:33:59PM +0000, Souza, Jose wrote: > > > > On Fri, 2021-09-17 at 20:49 +0300, Ville Syrjälä wrote: > > > > > 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. > > > > > > > > Thanks for pointing out state->legacy_cursor_update and yes setting it to false makes causes the cursor to lag. > > > > > > > > > > > > > > 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. > > > > > > > > I have tested it in a TGL and ADL-P, will try to get some gen9 to try it. > > > > Other than that I don't know what could this big difference between our setups. > > > > I'm using Mate like you with 'enable software compositing window manager' disabled. > > > > > > Not sure. > > > > > > BTW another thing I noticed is that the sel_fetch coordinate calculation > > > code seems super confused: > > > - it seems to do operations between coordinates that don't even live in > > > the same coordinate space (eg. drm_rect_intersect(&clip, &src) where > > > clip is the straight userspace damage coordinates but src is > > > PLANE_SURF relative plane source coordinates) > > > > On the first for_each_oldnew_intel_plane_in_state() it calculates the plane damaged area and then in the last 3 lines converts it to pipe coordinate > > space. > > The second for_each_oldnew_intel_plane_in_state() takes the pipe coordinate space damaged area and sets new_plane_state->psr2_sel_fetch_area with the > > plane coordinate space damaged area. > > There are many many coordinate spaces we use: > - relative to user fb origin: userspace provided dirtyfb and plane src > coordinates (drm_plane_state_src()) > - relative to start of gem obj: used temporarily during some calculations > - relative to start of vma: used temporarily during some calculations > (also actually what intel_plane_fence_y_offset() gives you) > - relative to PLANE_SURF: plane_state->uapi.src > - relative to user crtc origin: drm_plane_state_dest() > - relative to pipe origin: plane_state->uapi.dst > > The sel_fetch code is now doing operations between coordinates from > different coordinate spaces AFAICS. It only uses this 3: - plane damaged: that is in the same coordinate space as plane_state->uapi.src - plane_state->uapi.src - plane_state->uapi.dst There is only conversions between src to dst and dst to src. > > My gut feeling is that we want to do these calculations alongside the > rest of the plane coordinate calcs in the plane code. That way we can > just work forwards from userspace coords all the way to PLANE_SURF > relative coords for both cases. Trying to do these sel fetch > calculations after the fact means we're going to have to work both > forwards and backwards at the same time, which doesn't sounds all that > nice to me. But I've not spent a huge amount of time thinking about > this so not 100% sure. > > What we need is basically something like kms_big_fb but with sel fetch > in mind. That could test all the interesting cases where we either use > remapping or just hit the standard panning cases where PLANE_SURF does > not match the fb origin. > > > > > > - no checks for plane scaling that I can see but it still assumes it can > > > just assume a 1:1 relationship between src and dst coordinates > > > > My understanding is that intel_atomic_plane_check_clipping() will adjust src to match dst width and height. > > To do partial scaled updates correctly we'd need to have sub-pixel > coordinates for the src or else you'll get a visible seam when the > update region doesn't exactly terminate on a pixel boundary. And > actually even if it did land on a pixel boundary you'd still get > the seam unless we could instruct the hardware to filter across > the edge. Don't think we can even do that with the currect hardware. Actually scaling is not supported by the feature, so we are missing a check in intel_psr2_sel_fetch_config_valid() to disable it in this cases. BSpec: 55229 Not supported with plane or pipe scaling. Software calculations to account for extra lines of scaler filter input and adjusted scale factor and filter phase are too complicated. > > > > > > - bigjoiner also affects the coordinate spaces, so that part too is probably > > > busted > > > > > > > I don't think there is a commercial available eDP panel that would require bigjoiner. > > We could definitely rule PSR2 out if such case shows up by adding a check in intel_psr2_sel_fetch_config_valid(). > > > > Yeah, if bigjoiner turns out to complicate the calculations to much we > could just reject the combo. Not entirely sure it's a significant > complication though. >
