Hi Tomasz,
cc'ing Helen as she will continue the work.
Missatge de Tomasz Figa tfiga@chromium.org del dia dc., 14 de nov. 2018 a les 9:49:
Hi Enric,
On Fri, Sep 28, 2018 at 5:27 PM Enric Balletbo i Serra enric.balletbo@collabora.com wrote:
Add support to async updates of cursors by using the new atomic interface for that.
Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com
Hi,
This is a second version of the patch to add async-plane update support to the Rockchip driver.
I'm really sorry for the super late reply. I couldn't get cycles to look at this earlier. :(
No problem, many thanks for looking at it.
The patch was tested on a Samsung Chromebook Plus in two ways.
- Running all igt kms_cursor_legacy and kms_atomic tests and see that
there is no regression after the patch. Note that before the patch, the following igt tests failed: - basic-flip-before-cursor-legacy - basic-flip-before-cursor-atomic - cursor-vs-flip-legacy - cursor-vs-flip-toggle - flip-vs-cursor-atomic - flip-vs-cursor-crc-atomic - flip-vs-cursor-crc-legacy - flip-vs-cursor-legacy - flip-vs-cursor-crc-atomic - flip-vs-cursor-crc-legacy
Are the last 2 tests repeated?
Right, are repeated sorry.
With the patch applied only two tests don't fail (these two areexpexted to not pass right now): - flip-vs-cursor-crc-atomic - flip-vs-cursor-crc-legac
Did you mean "don't pass"?
Yes, the only two tests that don't pass are the cursor-crc ones.
You can check full IGT test report here: - Before the patch: https://people.collabora.com/~eballetbo/tests/igt/samsung-chromebook-plus/igt-1.23/4.19-rc5/index.html - With the patch applied: https://people.collabora.com/~eballetbo/tests/igt/samsung-chromebook-plus/igt-1.23/4.19-rc5-async-update/index.html
- Running weston using the atomic API.
Best regards, Enric
Changes in v2:
- 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 | 62 +++++++++++++++++++++ 4 files changed, 102 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index 7b6f7227d476..aec9a997de13 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c @@ -128,24 +128,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);-}
uint32_t rockchip_drm_get_vblank_ns(struct drm_display_mode *mode) { uint64_t vblank_time = mode->vtotal - mode->vdisplay; @@ -156,24 +138,6 @@ uint32_t rockchip_drm_get_vblank_ns(struct drm_display_mode *mode) return vblank_time; }
-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 79d00d861a31..1635485955d3 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 6642c2b9bb3c..682ce40166f7 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -819,10 +819,72 @@ 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_plane_atomic_async_update(struct drm_plane *plane,
struct drm_plane_state *new_state)+{
struct vop *vop = to_vop(plane->state->crtc);plane->state->crtc_x = new_state->crtc_x;plane->state->crtc_y = new_state->crtc_y;plane->state->crtc_h = new_state->crtc_h;plane->state->crtc_w = new_state->crtc_w;plane->state->src_x = new_state->src_x;plane->state->src_y = new_state->src_y;plane->state->src_h = new_state->src_h;plane->state->src_w = new_state->src_w;drm_atomic_set_fb_for_plane(plane->state, new_state->fb);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?
Hmm, yes you've reason. Thanks for pointing it.
I believe we need to do something similar to what's already in vop_crtc_commit_flush(), but not called on this async path https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/dri...
Perhaps it would be an option to call vop_crtc_commit_flush() from here?
We'll look at this for the third version, thanks!
Best regards, Tomasz
if (vop->is_enabled) {rockchip_drm_psr_inhibit_get_state(new_state->state);vop_plane_atomic_update(plane, plane->state);spin_lock(&vop->reg_lock);vop_cfg_done(vop);spin_unlock(&vop->reg_lock);rockchip_drm_psr_inhibit_put_state(new_state->state);}+}
static const struct drm_plane_helper_funcs plane_helper_funcs = { .atomic_check = vop_plane_atomic_check, .atomic_update = vop_plane_atomic_update, .atomic_disable = vop_plane_atomic_disable,
.atomic_async_check = vop_plane_atomic_async_check,.atomic_async_update = vop_plane_atomic_async_update,};
static const struct drm_plane_funcs vop_plane_funcs = {
2.19.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel