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 <helen.koike@xxxxxxxxxxxxx> wrote: >> >> From: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx> >> >> Add support to async updates of cursors by using the new atomic >> interface for that. >> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx> >> [updated for upstream] >> Signed-off-by: Helen Koike <helen.koike@xxxxxxxxxxxxx> >> >> --- >> 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 01ff3c858875..22a70ab6e214 100644 >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c >> @@ -13,6 +13,7 @@ >> */ >> >> #include <drm/drmP.h> >> +#include <drm/drm_atomic.h> >> #include <drm/drm_crtc_helper.h> >> >> #include "rockchip_drm_drv.h" >> @@ -109,6 +110,42 @@ int rockchip_drm_psr_inhibit_put(struct drm_encoder *encoder) >> } >> EXPORT_SYMBOL(rockchip_drm_psr_inhibit_put); >> >> +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); >> +} >> +EXPORT_SYMBOL(rockchip_drm_psr_inhibit_get_state); >> + >> +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); >> +} >> +EXPORT_SYMBOL(rockchip_drm_psr_inhibit_put_state); >> + >> /** >> * rockchip_drm_psr_inhibit_get - acquire PSR inhibit on given encoder >> * @encoder: encoder to obtain the PSR encoder >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h >> index 860c62494496..25350ba3237b 100644 >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h >> @@ -20,6 +20,9 @@ void rockchip_drm_psr_flush_all(struct drm_device *dev); >> int rockchip_drm_psr_inhibit_put(struct drm_encoder *encoder); >> int rockchip_drm_psr_inhibit_get(struct drm_encoder *encoder); >> >> +void rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state); >> +void rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state); >> + >> int rockchip_drm_psr_register(struct drm_encoder *encoder, >> int (*psr_set)(struct drm_encoder *, bool enable)); >> void rockchip_drm_psr_unregister(struct drm_encoder *encoder); >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >> index fb70fb486fbf..176d6e8207ed 100644 >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >> @@ -15,6 +15,7 @@ >> #include <drm/drm.h> >> #include <drm/drmP.h> >> #include <drm/drm_atomic.h> >> +#include <drm/drm_atomic_uapi.h> >> #include <drm/drm_crtc.h> >> #include <drm/drm_crtc_helper.h> >> #include <drm/drm_flip_work.h> >> @@ -819,10 +820,99 @@ static void vop_plane_atomic_update(struct drm_plane *plane, >> spin_unlock(&vop->reg_lock); >> } >> >> +static int vop_plane_atomic_async_check(struct drm_plane *plane, >> + struct drm_plane_state *state) >> +{ >> + struct vop_win *vop_win = to_vop_win(plane); >> + const struct vop_win_data *win = vop_win->data; >> + int min_scale = win->phy->scl ? FRAC_16_16(1, 8) : >> + DRM_PLANE_HELPER_NO_SCALING; >> + int max_scale = win->phy->scl ? FRAC_16_16(8, 1) : >> + DRM_PLANE_HELPER_NO_SCALING; >> + struct drm_crtc_state *crtc_state; >> + int ret; >> + >> + if (plane != state->crtc->cursor) >> + return -EINVAL; >> + >> + if (!plane->state) >> + return -EINVAL; >> + >> + if (!plane->state->fb) >> + return -EINVAL; >> + >> + if (state->state) >> + crtc_state = drm_atomic_get_existing_crtc_state(state->state, >> + state->crtc); >> + else /* Special case for asynchronous cursor updates. */ >> + crtc_state = plane->crtc->state; >> + >> + ret = drm_atomic_helper_check_plane_state(plane->state, >> + crtc_state, >> + min_scale, max_scale, >> + true, true); >> + return ret; >> +} >> + >> +static void vop_crtc_atomic_commit_flush(struct drm_crtc *crtc, >> + struct drm_crtc_state *old_crtc_state) >> +{ >> + struct drm_atomic_state *old_state = old_crtc_state->state; >> + struct drm_plane_state *old_plane_state, *new_plane_state; >> + struct vop *vop = to_vop(crtc); >> + struct drm_plane *plane; >> + int i; >> + >> + for_each_oldnew_plane_in_state(old_state, plane, old_plane_state, >> + new_plane_state, i) { > > Hmm, from what I can see, we're not going through the full atomic > commit sequence, with state flip, so I'm not sure where we would get > the new state here from. > >> + if (!old_plane_state->fb) >> + continue; >> + >> + if (old_plane_state->fb == new_plane_state->fb) >> + continue; >> + >> + drm_framebuffer_get(old_plane_state->fb); >> + WARN_ON(drm_crtc_vblank_get(crtc) != 0); >> + drm_flip_work_queue(&vop->fb_unref_work, old_plane_state->fb); >> + set_bit(VOP_PENDING_FB_UNREF, &vop->pending); >> + } >> +} >> + >> +static void vop_plane_atomic_async_update(struct drm_plane *plane, >> + struct drm_plane_state *new_state) >> +{ >> + struct vop *vop = to_vop(plane->state->crtc); >> + >> + if (vop->crtc.state->state) >> + vop_crtc_atomic_commit_flush(&vop->crtc, vop->crtc.state); > > Since we just operate on one plane here, we could just do like this: > > if (plane->state->fb && plane->state->fb != new_state->fb) { > drm_framebuffer_get(plane->state->fb); > WARN_ON(drm_crtc_vblank_get(crtc) != 0); > drm_flip_work_queue(&vop->fb_unref_work, plane->state->fb); > set_bit(VOP_PENDING_FB_UNREF, &vop->pending); > } > > However, we cannot simply to this here, because it races with the > vblank interrupt. We need to program the hw plane with the new fb > first and trigger the update. This needs all the careful handling that > is done in vop_crtc_atomic_flush() and so my original suggestion to > just call it. vop_crtc_atomic_flush() also updates the crtc->state->event, I don't think we want that. And actually I don't think we have this race condition, please see below > > Of course to call it in its current shape, one needs to have a full > atomic state from a commit, after a flip, but we only have the new > plane state here. Perhaps you could duplicate existing state, update > the desired plane state, flip and then call vop_crtc_atomic_flush()? Could you please clarify your proposal? You mean duplicating plane->state ? I'm trying to see how this would fit it in the code. The drm_atomic_state structure at plate->state->state is actually always NULL (as an async update is applied right away). > > Best regards, > Tomasz > >From your comment in v2: > Isn't this going to drop the old fb reference on the floor without > waiting for the hardware to actually stop scanning out from it? I've been trying to analyze this better, I also got some help from Gustavo Padovan, and I think there is no problem here as the configuration we are doing here will just be taken into consideration by the hardware in the next vblank, so I guess we can set and re-set plane->fb as much as we want. >From the async_update docs: * - Some hw might still scan out the old buffer until the next * vblank, however we let go of the fb references as soon as * we run this hook. For now drivers must implement their own workers * for deferring if needed, until a common solution is created. So the idea is to let the reference go asap, then we shouldn't need an extra drm_framebuffer_get() as done by vop_crtc_atomic_flush(). Does it make sense to you? Unless I am missing something, the patch v2 is essentially correct, if not, then vc4 driver is also broken. Please, let me know what you think and I'll send the v4. Regards, Helen _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
