Re: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic.
Michael Zoran writes: > On Fri, 2018-11-23 at 11:27 +0900, Tomasz Figa wrote: >> >> The point here is not about setting and resetting the plane->fb >> pointer. It's about what happens inside >> drm_atomic_set_fb_for_plane(). >> >> It calls drm_framebuffer_get() for the new fb and >> drm_framebuffer_put() for the old fb. In result, if the fb changes, >> the old fb, which had its reference count incremented in the atomic >> commit that set it to the plane before, has its reference count >> decremented. Moreover, if the new reference count becomes 0, >> drm_framebuffer_put() will immediately free the buffer. >> >> Freeing a buffer when the hardware is still scanning out of it isn't >> a >> good idea, is it? > > No, it's not. But the board I submitted the patch for doesn't have > anything like hot swapable ram. The ram access is still going to work, > just it might display something it shouldn't. Say for example if that > frame buffer got reused by somethig else and filled with new data in > the very small window. > > But yes, I agree the best solution would be to not release the buffer > until the next vblank. > > Perhaps a good solution would be for the DRM api to have the concept of > a deferred release? Meaning if the put() call just added the frame > buffer to a list that DRM core could walk during the vblank. That > might be better then every single driver trying to work up a custom > solution. > >> The vc4 driver seems to be able to program the hardware to switch the >> scanout to the new buffer immediately: >> >> https://elixir.bootlin.com/linux/v4.20-rc3/source/drivers/gpu/drm/vc4/vc4_plane.c#L794 >> >> Although I wonder if there isn't still a tiny race there - the >> hardware may have just started refilling the FIFO from the old >> address. Still, if the FIFO is small, the FIFO refill operation may >> be >> much shorter than it takes for the kernel code to actually free the >> buffer. Eric and Michael, could you confirm? >> > > I don't have those boards anymore, and I don't have access to any > technical documentation on the GPU so I can't really add much here. > Eric can probably provide the best information. I don't think I understood my scanout hardware well enough when I started on the async update stuff for rpi. vc4 probably needs to wait until the HW starts scanning out a new line before letting the old BO get freed. signature.asc Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic.
Hi Tomasz,
On 11/23/18 12:27 AM, Tomasz Figa wrote:
Hi Helen,
On Fri, Nov 23, 2018 at 8:31 AM Helen Koike wrote:
Hi Tomasz,
On 11/20/18 4:48 AM, Tomasz Figa wrote:
Hi Helen,
On Tue, Nov 20, 2018 at 4:08 AM Helen Koike wrote:
From: Enric Balletbo i Serra
Add support to async updates of cursors by using the new atomic
interface for that.
Signed-off-by: Enric Balletbo i Serra
[updated for upstream]
Signed-off-by: Helen Koike
---
Hello,
This is the third version of the async-plane update suport to the
Rockchip driver.
Thanks for a quick respin. Please see my comments inline. (I'll try to
be better at responding from now on...)
I tested running igt kms_cursor_legacy and kms_atomic tests using a 96Boards
Ficus.
Note that before the patch, the following igt tests failed:
basic-flip-before-cursor-atomic
basic-flip-before-cursor-legacy
cursor-vs-flip-atomic
cursor-vs-flip-legacy
cursor-vs-flip-toggle
flip-vs-cursor-atomic
flip-vs-cursor-busy-crc-atomic
flip-vs-cursor-busy-crc-legacy
flip-vs-cursor-crc-atomic
flip-vs-cursor-crc-legacy
flip-vs-cursor-legacy
Full log: https://people.collabora.com/~koike/results-4.20/html/
Now with the patch applied the following were fixed:
basic-flip-before-cursor-atomic
basic-flip-before-cursor-legacy
flip-vs-cursor-atomic
flip-vs-cursor-legacy
Full log: https://people.collabora.com/~koike/results-4.20-async/html/
Could you also test modetest, with the -C switch to test the legacy
cursor API? I remember it triggering crashes due to synchronization
issues easily.
Sure. I tested with
$ modetest -M rockchip -s 37:1920x1080 -C
I also vary the mode but I couldn't trigger any crashes.
Tomasz, as you mentined in v2 about waiting the hardware before updating
the framebuffer, now I call the loop you pointed out in the async path,
was that what you had in mind? Or do you think I would make sense to
call the vop_crtc_atomic_flush() instead of just exposing that loop?
Thanks
Helen
Changes in v3:
- Rebased on top of drm-misc
- Fix missing include in rockchip_drm_vop.c
- New function vop_crtc_atomic_commit_flush
Changes in v2:
- v2: https://patchwork.freedesktop.org/patch/254180/
- Change the framebuffer as well to cover jumpy cursor when hovering
text boxes or hyperlink. (Tomasz)
- Use the PSR inhibit mechanism when accessing VOP hardware instead of
PSR flushing (Tomasz)
Changes in v1:
- Rebased on top of drm-misc
- In async_check call drm_atomic_helper_check_plane_state to check that
the desired plane is valid and update various bits of derived state
(clipped coordinates etc.)
- In async_check allow to configure new scaling in the fast path.
- In async_update force to flush all registered PSR encoders.
- In async_update call atomic_update directly.
- In async_update call vop_cfg_done needed to set the vop registers and take
effect.
drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 36 ---
drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 37 +++
drivers/gpu/drm/rockchip/rockchip_drm_psr.h | 3 +
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 108 +---
4 files changed, 131 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index ea18cb2a76c0..08bec50d9c5d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -127,42 +127,6 @@ rockchip_user_fb_create(struct drm_device *dev, struct
drm_file *file_priv,
return ERR_PTR(ret);
}
-static void
-rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state)
-{
- struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
- struct drm_encoder *encoder;
- u32 encoder_mask = 0;
- int i;
-
- for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
- encoder_mask |= crtc_state->encoder_mask;
- encoder_mask |= crtc->state->encoder_mask;
- }
-
- drm_for_each_encoder_mask(encoder, state->dev, encoder_mask)
- rockchip_drm_psr_inhibit_get(encoder);
-}
-
-static void
-rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state)
-{
- struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
- struct drm_encoder *encoder;
- u32 encoder_mask = 0;
- int i;
-
- for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
- encoder_mask |= crtc_state->encoder_mask;
- encoder_mask |= crtc->state->encoder_mask;
- }
-
- drm_for_each_encoder_mask(encoder, state->dev, encoder_mask)
- rockchip_drm_psr_inhibit_put(encoder);
-}
-
static void
rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
{
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
index
Re: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic.
Hi Gustavo,
On Tue, Nov 27, 2018 at 8:54 AM Gustavo Padovan
wrote:
>
> Hi Tomasz,
>
> On 11/23/18 12:27 AM, Tomasz Figa wrote:
> > Hi Helen,
> >
> > On Fri, Nov 23, 2018 at 8:31 AM Helen Koike
> > wrote:
> >> Hi Tomasz,
> >>
> >> On 11/20/18 4:48 AM, Tomasz Figa wrote:
> >>> Hi Helen,
> >>>
> >>> On Tue, Nov 20, 2018 at 4:08 AM Helen Koike
> >>> wrote:
> From: Enric Balletbo i Serra
>
> Add support to async updates of cursors by using the new atomic
> interface for that.
>
> Signed-off-by: Enric Balletbo i Serra
> [updated for upstream]
> Signed-off-by: Helen Koike
>
> ---
> Hello,
>
> This is the third version of the async-plane update suport to the
> Rockchip driver.
>
> >>> Thanks for a quick respin. Please see my comments inline. (I'll try to
> >>> be better at responding from now on...)
> >>>
> I tested running igt kms_cursor_legacy and kms_atomic tests using a
> 96Boards Ficus.
>
> Note that before the patch, the following igt tests failed:
>
> basic-flip-before-cursor-atomic
> basic-flip-before-cursor-legacy
> cursor-vs-flip-atomic
> cursor-vs-flip-legacy
> cursor-vs-flip-toggle
> flip-vs-cursor-atomic
> flip-vs-cursor-busy-crc-atomic
> flip-vs-cursor-busy-crc-legacy
> flip-vs-cursor-crc-atomic
> flip-vs-cursor-crc-legacy
> flip-vs-cursor-legacy
>
> Full log: https://people.collabora.com/~koike/results-4.20/html/
>
> Now with the patch applied the following were fixed:
> basic-flip-before-cursor-atomic
> basic-flip-before-cursor-legacy
> flip-vs-cursor-atomic
> flip-vs-cursor-legacy
>
> Full log: https://people.collabora.com/~koike/results-4.20-async/html/
> >>> Could you also test modetest, with the -C switch to test the legacy
> >>> cursor API? I remember it triggering crashes due to synchronization
> >>> issues easily.
> >> Sure. I tested with
> >> $ modetest -M rockchip -s 37:1920x1080 -C
> >>
> >> I also vary the mode but I couldn't trigger any crashes.
> >>
> Tomasz, as you mentined in v2 about waiting the hardware before updating
> the framebuffer, now I call the loop you pointed out in the async path,
> was that what you had in mind? Or do you think I would make sense to
> call the vop_crtc_atomic_flush() instead of just exposing that loop?
>
> Thanks
> Helen
>
> Changes in v3:
> - Rebased on top of drm-misc
> - Fix missing include in rockchip_drm_vop.c
> - New function vop_crtc_atomic_commit_flush
>
> Changes in v2:
> - v2: https://patchwork.freedesktop.org/patch/254180/
> - Change the framebuffer as well to cover jumpy cursor when hovering
> text boxes or hyperlink. (Tomasz)
> - Use the PSR inhibit mechanism when accessing VOP hardware instead of
> PSR flushing (Tomasz)
>
> Changes in v1:
> - Rebased on top of drm-misc
> - In async_check call drm_atomic_helper_check_plane_state to check that
> the desired plane is valid and update various bits of derived state
> (clipped coordinates etc.)
> - In async_check allow to configure new scaling in the fast path.
> - In async_update force to flush all registered PSR encoders.
> - In async_update call atomic_update directly.
> - In async_update call vop_cfg_done needed to set the vop registers and
> take effect.
>
> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 36 ---
> drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 37 +++
> drivers/gpu/drm/rockchip/rockchip_drm_psr.h | 3 +
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 108 +---
> 4 files changed, 131 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index ea18cb2a76c0..08bec50d9c5d 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -127,42 +127,6 @@ rockchip_user_fb_create(struct drm_device *dev,
> struct drm_file *file_priv,
> return ERR_PTR(ret);
> }
>
> -static void
> -rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state)
> -{
> - struct drm_crtc *crtc;
> - struct drm_crtc_state *crtc_state;
> - struct drm_encoder *encoder;
> - u32 encoder_mask = 0;
> - int i;
> -
> - for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
> - encoder_mask |= crtc_state->encoder_mask;
> - encoder_mask |= crtc->state->encoder_mask;
> - }
> -
> -
Re: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic.
On Mon, Nov 26, 2018 at 10:41:02PM +0100, Boris Brezillon wrote: > On Mon, 26 Nov 2018 12:36:03 -0800 > Eric Anholt wrote: > > > Michael Zoran writes: > > > > > On Fri, 2018-11-23 at 11:27 +0900, Tomasz Figa wrote: > > >> > > >> The point here is not about setting and resetting the plane->fb > > >> pointer. It's about what happens inside > > >> drm_atomic_set_fb_for_plane(). > > >> > > >> It calls drm_framebuffer_get() for the new fb and > > >> drm_framebuffer_put() for the old fb. In result, if the fb changes, > > >> the old fb, which had its reference count incremented in the atomic > > >> commit that set it to the plane before, has its reference count > > >> decremented. Moreover, if the new reference count becomes 0, > > >> drm_framebuffer_put() will immediately free the buffer. > > >> > > >> Freeing a buffer when the hardware is still scanning out of it isn't > > >> a > > >> good idea, is it? > > > > > > No, it's not. But the board I submitted the patch for doesn't have > > > anything like hot swapable ram. The ram access is still going to work, > > > just it might display something it shouldn't. Say for example if that > > > frame buffer got reused by somethig else and filled with new data in > > > the very small window. > > > > > > But yes, I agree the best solution would be to not release the buffer > > > until the next vblank. > > > > > > Perhaps a good solution would be for the DRM api to have the concept of > > > a deferred release? Meaning if the put() call just added the frame > > > buffer to a list that DRM core could walk during the vblank. That > > > might be better then every single driver trying to work up a custom > > > solution. > > > > > >> The vc4 driver seems to be able to program the hardware to switch the > > >> scanout to the new buffer immediately: > > >> > > >> https://elixir.bootlin.com/linux/v4.20-rc3/source/drivers/gpu/drm/vc4/vc4_plane.c#L794 > > >> > > >> Although I wonder if there isn't still a tiny race there - the > > >> hardware may have just started refilling the FIFO from the old > > >> address. Still, if the FIFO is small, the FIFO refill operation may > > >> be > > >> much shorter than it takes for the kernel code to actually free the > > >> buffer. Eric and Michael, could you confirm? > > >> > > > > > > I don't have those boards anymore, and I don't have access to any > > > technical documentation on the GPU so I can't really add much here. > > > Eric can probably provide the best information. > > > > I don't think I understood my scanout hardware well enough when I > > started on the async update stuff for rpi. vc4 probably needs to wait > > until the HW starts scanning out a new line before letting the old BO > > get freed. > > That's also my understanding. Note that even if the BO is freed before > the HVS has finished generating a line it shouldn't crash, because > accesses are done through DMA. Might be a security issue though if we > start re-using the memory for something else while it's still being > accessed. > > The solution would be to wait for the EOL (End Of Line) interrupt in > the async update path, but I'm not sure this is allowed. For subsuming async page_flip into the async atomic commit stuff we need completion events anyway. Firing those from the EOL (or some other hw-dependent interrupt, could also be the next vblank) should be hard to arrange for drivers. And the heleprs could then easily offload the cleanup_planes work to a worker and delay it until after the event has fired. Like it already does for normal atomic commits. But yeah right now that's all missing. The biggest issue here is figuring out what these events should look like, and how generic userspace needs to use them. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic.
On Mon, 26 Nov 2018 12:36:03 -0800 Eric Anholt wrote: > Michael Zoran writes: > > > On Fri, 2018-11-23 at 11:27 +0900, Tomasz Figa wrote: > >> > >> The point here is not about setting and resetting the plane->fb > >> pointer. It's about what happens inside > >> drm_atomic_set_fb_for_plane(). > >> > >> It calls drm_framebuffer_get() for the new fb and > >> drm_framebuffer_put() for the old fb. In result, if the fb changes, > >> the old fb, which had its reference count incremented in the atomic > >> commit that set it to the plane before, has its reference count > >> decremented. Moreover, if the new reference count becomes 0, > >> drm_framebuffer_put() will immediately free the buffer. > >> > >> Freeing a buffer when the hardware is still scanning out of it isn't > >> a > >> good idea, is it? > > > > No, it's not. But the board I submitted the patch for doesn't have > > anything like hot swapable ram. The ram access is still going to work, > > just it might display something it shouldn't. Say for example if that > > frame buffer got reused by somethig else and filled with new data in > > the very small window. > > > > But yes, I agree the best solution would be to not release the buffer > > until the next vblank. > > > > Perhaps a good solution would be for the DRM api to have the concept of > > a deferred release? Meaning if the put() call just added the frame > > buffer to a list that DRM core could walk during the vblank. That > > might be better then every single driver trying to work up a custom > > solution. > > > >> The vc4 driver seems to be able to program the hardware to switch the > >> scanout to the new buffer immediately: > >> > >> https://elixir.bootlin.com/linux/v4.20-rc3/source/drivers/gpu/drm/vc4/vc4_plane.c#L794 > >> > >> Although I wonder if there isn't still a tiny race there - the > >> hardware may have just started refilling the FIFO from the old > >> address. Still, if the FIFO is small, the FIFO refill operation may > >> be > >> much shorter than it takes for the kernel code to actually free the > >> buffer. Eric and Michael, could you confirm? > >> > > > > I don't have those boards anymore, and I don't have access to any > > technical documentation on the GPU so I can't really add much here. > > Eric can probably provide the best information. > > I don't think I understood my scanout hardware well enough when I > started on the async update stuff for rpi. vc4 probably needs to wait > until the HW starts scanning out a new line before letting the old BO > get freed. That's also my understanding. Note that even if the BO is freed before the HVS has finished generating a line it shouldn't crash, because accesses are done through DMA. Might be a security issue though if we start re-using the memory for something else while it's still being accessed. The solution would be to wait for the EOL (End Of Line) interrupt in the async update path, but I'm not sure this is allowed. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic.
On Fri, 2018-11-23 at 11:27 +0900, Tomasz Figa wrote: > > The point here is not about setting and resetting the plane->fb > pointer. It's about what happens inside > drm_atomic_set_fb_for_plane(). > > It calls drm_framebuffer_get() for the new fb and > drm_framebuffer_put() for the old fb. In result, if the fb changes, > the old fb, which had its reference count incremented in the atomic > commit that set it to the plane before, has its reference count > decremented. Moreover, if the new reference count becomes 0, > drm_framebuffer_put() will immediately free the buffer. > > Freeing a buffer when the hardware is still scanning out of it isn't > a > good idea, is it? No, it's not. But the board I submitted the patch for doesn't have anything like hot swapable ram. The ram access is still going to work, just it might display something it shouldn't. Say for example if that frame buffer got reused by somethig else and filled with new data in the very small window. But yes, I agree the best solution would be to not release the buffer until the next vblank. Perhaps a good solution would be for the DRM api to have the concept of a deferred release? Meaning if the put() call just added the frame buffer to a list that DRM core could walk during the vblank. That might be better then every single driver trying to work up a custom solution. > The vc4 driver seems to be able to program the hardware to switch the > scanout to the new buffer immediately: > > https://elixir.bootlin.com/linux/v4.20-rc3/source/drivers/gpu/drm/vc4/vc4_plane.c#L794 > > Although I wonder if there isn't still a tiny race there - the > hardware may have just started refilling the FIFO from the old > address. Still, if the FIFO is small, the FIFO refill operation may > be > much shorter than it takes for the kernel code to actually free the > buffer. Eric and Michael, could you confirm? > I don't have those boards anymore, and I don't have access to any technical documentation on the GPU so I can't really add much here. Eric can probably provide the best information. I submitted the patch because I was working on arm64 support for fun and was becomming very annoyed by desktop lockups for long periods of time on the desktop enviroment of my choice due to the driver being flooded with curser animation updates. I sent the patch to Eric who was kind enough to review it and suggest some improvements. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic.
Hi Michael, On Fri, Nov 23, 2018 at 1:58 PM Michael Zoran wrote: > > On Fri, 2018-11-23 at 11:27 +0900, Tomasz Figa wrote: > > > > The point here is not about setting and resetting the plane->fb > > pointer. It's about what happens inside > > drm_atomic_set_fb_for_plane(). > > > > It calls drm_framebuffer_get() for the new fb and > > drm_framebuffer_put() for the old fb. In result, if the fb changes, > > the old fb, which had its reference count incremented in the atomic > > commit that set it to the plane before, has its reference count > > decremented. Moreover, if the new reference count becomes 0, > > drm_framebuffer_put() will immediately free the buffer. > > > > Freeing a buffer when the hardware is still scanning out of it isn't > > a > > good idea, is it? > > No, it's not. But the board I submitted the patch for doesn't have > anything like hot swapable ram. The ram access is still going to work, > just it might display something it shouldn't. Say for example if that > frame buffer got reused by somethig else and filled with new data in > the very small window. Thanks for a quick reply! To clarify, on the Rockchip platform this patch is for (and many other arm/arm64 SoCs) the display controller is behind an IOMMU. Freeing the buffer would mean unmapping the related IOVAs from the IOMMU. If the hardware is still scanning out from the unmapped addresses, it would cause IOMMU page faults. We don't have any good IOMMU page fault handling in the kernel, so on most platforms that would likely end up stalling the display controller completely (on Rockchip it does). > > But yes, I agree the best solution would be to not release the buffer > until the next vblank. > > Perhaps a good solution would be for the DRM api to have the concept of > a deferred release? Meaning if the put() call just added the frame > buffer to a list that DRM core could walk during the vblank. That > might be better then every single driver trying to work up a custom > solution. Agreed. Best regards, Tomasz ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] drm/rockchip: update cursors asynchronously through atomic.
Hi Helen,
On Fri, Nov 23, 2018 at 8:31 AM Helen Koike wrote:
>
> Hi Tomasz,
>
> On 11/20/18 4:48 AM, Tomasz Figa wrote:
> > Hi Helen,
> >
> > On Tue, Nov 20, 2018 at 4:08 AM Helen Koike
> > wrote:
> >>
> >> From: Enric Balletbo i Serra
> >>
> >> Add support to async updates of cursors by using the new atomic
> >> interface for that.
> >>
> >> Signed-off-by: Enric Balletbo i Serra
> >> [updated for upstream]
> >> Signed-off-by: Helen Koike
> >>
> >> ---
> >> Hello,
> >>
> >> This is the third version of the async-plane update suport to the
> >> Rockchip driver.
> >>
> >
> > Thanks for a quick respin. Please see my comments inline. (I'll try to
> > be better at responding from now on...)
> >
> >> I tested running igt kms_cursor_legacy and kms_atomic tests using a
> >> 96Boards Ficus.
> >>
> >> Note that before the patch, the following igt tests failed:
> >>
> >> basic-flip-before-cursor-atomic
> >> basic-flip-before-cursor-legacy
> >> cursor-vs-flip-atomic
> >> cursor-vs-flip-legacy
> >> cursor-vs-flip-toggle
> >> flip-vs-cursor-atomic
> >> flip-vs-cursor-busy-crc-atomic
> >> flip-vs-cursor-busy-crc-legacy
> >> flip-vs-cursor-crc-atomic
> >> flip-vs-cursor-crc-legacy
> >> flip-vs-cursor-legacy
> >>
> >> Full log: https://people.collabora.com/~koike/results-4.20/html/
> >>
> >> Now with the patch applied the following were fixed:
> >> basic-flip-before-cursor-atomic
> >> basic-flip-before-cursor-legacy
> >> flip-vs-cursor-atomic
> >> flip-vs-cursor-legacy
> >>
> >> Full log: https://people.collabora.com/~koike/results-4.20-async/html/
> >
> > Could you also test modetest, with the -C switch to test the legacy
> > cursor API? I remember it triggering crashes due to synchronization
> > issues easily.
>
> Sure. I tested with
> $ modetest -M rockchip -s 37:1920x1080 -C
>
> I also vary the mode but I couldn't trigger any crashes.
>
> >
> >>
> >> Tomasz, as you mentined in v2 about waiting the hardware before updating
> >> the framebuffer, now I call the loop you pointed out in the async path,
> >> was that what you had in mind? Or do you think I would make sense to
> >> call the vop_crtc_atomic_flush() instead of just exposing that loop?
> >>
> >> Thanks
> >> Helen
> >>
> >> Changes in v3:
> >> - Rebased on top of drm-misc
> >> - Fix missing include in rockchip_drm_vop.c
> >> - New function vop_crtc_atomic_commit_flush
> >>
> >> Changes in v2:
> >> - v2: https://patchwork.freedesktop.org/patch/254180/
> >> - Change the framebuffer as well to cover jumpy cursor when hovering
> >> text boxes or hyperlink. (Tomasz)
> >> - Use the PSR inhibit mechanism when accessing VOP hardware instead of
> >> PSR flushing (Tomasz)
> >>
> >> Changes in v1:
> >> - Rebased on top of drm-misc
> >> - In async_check call drm_atomic_helper_check_plane_state to check that
> >> the desired plane is valid and update various bits of derived state
> >> (clipped coordinates etc.)
> >> - In async_check allow to configure new scaling in the fast path.
> >> - In async_update force to flush all registered PSR encoders.
> >> - In async_update call atomic_update directly.
> >> - In async_update call vop_cfg_done needed to set the vop registers and
> >> take effect.
> >>
> >> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 36 ---
> >> drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 37 +++
> >> drivers/gpu/drm/rockchip/rockchip_drm_psr.h | 3 +
> >> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 108 +---
> >> 4 files changed, 131 insertions(+), 53 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> >> b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> >> index ea18cb2a76c0..08bec50d9c5d 100644
> >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> >> @@ -127,42 +127,6 @@ rockchip_user_fb_create(struct drm_device *dev,
> >> struct drm_file *file_priv,
> >> return ERR_PTR(ret);
> >> }
> >>
> >> -static void
> >> -rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state)
> >> -{
> >> - struct drm_crtc *crtc;
> >> - struct drm_crtc_state *crtc_state;
> >> - struct drm_encoder *encoder;
> >> - u32 encoder_mask = 0;
> >> - int i;
> >> -
> >> - for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
> >> - encoder_mask |= crtc_state->encoder_mask;
> >> - encoder_mask |= crtc->state->encoder_mask;
> >> - }
> >> -
> >> - drm_for_each_encoder_mask(encoder, state->dev, encoder_mask)
> >> - rockchip_drm_psr_inhibit_get(encoder);
> >> -}
> >> -
> >> -static void
> >> -rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state)
> >> -{
> >> - struct drm_crtc *crtc;
> >> - struct drm_crtc_state *crtc_state;
> >> - struct drm_encoder *encoder;
> >> - u32 encoder_mask = 0;
> >> - int
