| Message ID | 20210914212507.177511-3-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/5] drm/i915/display/adlp: Fix PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR calculation | expand |
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. 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. > > This is reproducile on TGL and ADL-P. > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_psr.c | 25 ++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > index 567c7ceef8dba..f8fa30e50e70c 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -1543,6 +1543,28 @@ 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); > +} > + > int intel_psr2_sel_fetch_update(struct intel_atomic_state *state, > struct intel_crtc *crtc) > { > @@ -1611,6 +1633,9 @@ 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 || > (!num_clips && > -- > 2.33.0
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. > > 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. So I can't understand why it is incompatible. > > > > > This is reproducile on TGL and ADL-P. > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_psr.c | 25 ++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > index 567c7ceef8dba..f8fa30e50e70c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -1543,6 +1543,28 @@ 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); > > +} > > + > > int intel_psr2_sel_fetch_update(struct intel_atomic_state *state, > > struct intel_crtc *crtc) > > { > > @@ -1611,6 +1633,9 @@ 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 || > > (!num_clips && > > -- > > 2.33.0 >
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. > 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 |________________|
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 > > ________________| >
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. > > I believe compositors will do a single atomic commit updating cursor and all the other planes into a single commit. No. X obviously doesn't do that. And IIRC chromeos also uses the legacy cursor ioctl for the cursor despite using atomic commits for everything else.
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 believe compositors will do a single atomic commit updating cursor and all the other planes into a single commit. > > No. X obviously doesn't do that. And IIRC chromeos also uses the > legacy cursor ioctl for the cursor despite using atomic commits for > everything else. >
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.
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. >
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) - 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 - bigjoiner also affects the coordinate spaces, so that part too is probably busted
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. > - 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. > - 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().
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. 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. > > > - 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.
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. >
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index 567c7ceef8dba..f8fa30e50e70c 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -1543,6 +1543,28 @@ 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); +} + int intel_psr2_sel_fetch_update(struct intel_atomic_state *state, struct intel_crtc *crtc) { @@ -1611,6 +1633,9 @@ 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 || (!num_clips &&