This avois leaking drm_atomic_state internals into the helpers. The only place where this still happens after this patch is drm_atomic_helper_swap_state(). It's unavoidable there, and maybe a good indicator we should actually move that function into drm_atomic.c.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++------------------------ 1 file changed, 16 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ddfa0d120e39..464541c87c40 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -611,7 +611,7 @@ drm_atomic_helper_check_planes(struct drm_device *dev, if (!funcs || !funcs->atomic_check) continue;

- ret = funcs->atomic_check(crtc, state->crtc_states[i]); + ret = funcs->atomic_check(crtc, crtc_state); if (ret) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic driver check failed\n", crtc->base.id, crtc->name); @@ -1249,16 +1249,12 @@ EXPORT_SYMBOL(drm_atomic_helper_commit); int drm_atomic_helper_prepare_planes(struct drm_device *dev, struct drm_atomic_state *state) { - int nplanes = dev->mode_config.num_total_plane; - int ret, i; + struct drm_plane *plane; + struct drm_plane_state *plane_state; + int ret, i, j;

- for (i = 0; i < nplanes; i++) { + for_each_plane_in_state(state, plane, plane_state, i) { const struct drm_plane_helper_funcs *funcs; - struct drm_plane *plane = state->planes[i]; - struct drm_plane_state *plane_state = state->plane_states[i]; - - if (!plane) - continue;

funcs = plane->helper_private;

@@ -1272,12 +1268,10 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev, return 0;

fail: - for (i--; i >= 0; i--) { + for_each_plane_in_state(state, plane, plane_state, j) { const struct drm_plane_helper_funcs *funcs; - struct drm_plane *plane = state->planes[i]; - struct drm_plane_state *plane_state = state->plane_states[i];

- if (!plane) + if (j >= i) continue;

funcs = plane->helper_private; @@ -1564,35 +1558,26 @@ void drm_atomic_helper_swap_state(struct drm_device *dev, struct drm_atomic_state *state) { int i; + struct drm_connector *connector; + struct drm_connector_state *conn_state; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + struct drm_plane *plane; + struct drm_plane_state *plane_state;

- for (i = 0; i < state->num_connector; i++) { - struct drm_connector *connector = state->connectors[i]; - - if (!connector) - continue; - + for_each_connector_in_state(state, connector, conn_state, i) { connector->state->state = state; swap(state->connector_states[i], connector->state); connector->state->state = NULL; }

- for (i = 0; i < dev->mode_config.num_crtc; i++) { - struct drm_crtc *crtc = state->crtcs[i]; - - if (!crtc) - continue; - + for_each_crtc_in_state(state, crtc, crtc_state, i) { crtc->state->state = state; swap(state->crtc_states[i], crtc->state); crtc->state->state = NULL; }

- for (i = 0; i < dev->mode_config.num_total_plane; i++) { - struct drm_plane *plane = state->planes[i]; - - if (!plane) - continue; - + for_each_plane_in_state(state, plane, plane_state, i) { plane->state->state = state; swap(state->plane_states[i], plane->state); plane->state->state = NULL;

We want to hide drm_atomic_state internals better.

Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 8 ++++---- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 20 ++++++++------------ 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index e70a4f33d970..f315c55c1f65 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -288,6 +288,8 @@ static int rcar_du_atomic_commit(struct drm_device *dev, { struct rcar_du_device *rcdu = dev->dev_private; struct rcar_du_commit *commit; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; unsigned int i; int ret;

@@ -309,10 +311,8 @@ static int rcar_du_atomic_commit(struct drm_device *dev, /* Wait until all affected CRTCs have completed previous commits and * mark them as pending. */ - for (i = 0; i < dev->mode_config.num_crtc; ++i) { - if (state->crtcs[i]) - commit->crtcs |= 1 << drm_crtc_index(state->crtcs[i]); - } + for_each_crtc_in_state(state, crtc, crtc_state, i) + commit->crtcs |= 1 << drm_crtc_index(crtc);

spin_lock(&rcdu->commit.wait.lock); ret = wait_event_interruptible_locked(rcdu->commit.wait, diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index d445e67f78e1..bfe31ca870cc 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c @@ -140,18 +140,17 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, bool needs_realloc = false; unsigned int groups = 0; unsigned int i; + struct drm_plane *drm_plane; + struct drm_plane_state *drm_plane_state;

/* Check if hardware planes need to be reallocated. */ - for (i = 0; i < dev->mode_config.num_total_plane; ++i) { + for_each_plane_in_state(state, drm_plane, drm_plane_state, i) { struct rcar_du_plane_state *plane_state; struct rcar_du_plane *plane; unsigned int index;

- if (!state->planes[i]) - continue; - - plane = to_rcar_plane(state->planes[i]); - plane_state = to_rcar_plane_state(state->plane_states[i]); + plane = to_rcar_plane(drm_plane); + plane_state = to_rcar_plane_state(drm_plane_state);

dev_dbg(rcdu->dev, "%s: checking plane (%u,%tu)\n", __func__, plane->group->index, plane - plane->group->planes); @@ -247,18 +246,15 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, }

/* Reallocate hardware planes for each plane that needs it. */ - for (i = 0; i < dev->mode_config.num_total_plane; ++i) { + for_each_plane_in_state(state, drm_plane, drm_plane_state, i) { struct rcar_du_plane_state *plane_state; struct rcar_du_plane *plane; unsigned int crtc_planes; unsigned int free; int idx;

- if (!state->planes[i]) - continue; - - plane = to_rcar_plane(state->planes[i]); - plane_state = to_rcar_plane_state(state->plane_states[i]); + plane = to_rcar_plane(drm_plane); + plane_state = to_rcar_plane_state(drm_plane_state);

dev_dbg(rcdu->dev, "%s: allocating plane (%u,%tu)\n", __func__, plane->group->index, plane - plane->group->planes);

... and use it in msm&vc4. Again just want to encapsulate drm_atomic_state internals a bit.

The const threading is a bit awkward in vc4 since C sucks, but I still think it's worth to enforce this. Eventually I want to make all the obj->state pointers const too, but that's a lot more work ...

Cc: Eric Anholt eric@anholt.net Cc: Rob Clark robdclark@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 10 +++------ drivers/gpu/drm/vc4/vc4_crtc.c | 11 +++------- drivers/gpu/drm/vc4/vc4_drv.h | 2 +- drivers/gpu/drm/vc4/vc4_plane.c | 5 +++-- include/drm/drm_atomic.h | 36 ++++++++++++++++++++++++++++++++ 5 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c index 88fe256c1931..6d4086ee0503 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c @@ -383,19 +383,15 @@ static int mdp5_crtc_atomic_check(struct drm_crtc *crtc, */ hw_cfg = mdp5_cfg_get_hw_config(mdp5_kms->cfg); drm_atomic_crtc_state_for_each_plane(plane, state) { - struct drm_plane_state *pstate; + const struct drm_plane_state *pstate; if (cnt >= (hw_cfg->lm.nb_stages)) { dev_err(dev->dev, "too many planes!\n"); return -EINVAL; }

- pstate = state->state->plane_states[drm_plane_index(plane)]; + pstate = __drm_atomic_get_current_plane_state(state->state, + plane);

- /* plane might not have changed, in which case take - * current state: - */ - if (!pstate) - pstate = plane->state; pstates[cnt].plane = plane; pstates[cnt].state = to_mdp5_plane_state(pstate);

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 904d0754ad78..703bda170105 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -405,14 +405,9 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, return -EINVAL;

drm_atomic_crtc_state_for_each_plane(plane, state) { - struct drm_plane_state *plane_state = - state->state->plane_states[drm_plane_index(plane)]; - - /* plane might not have changed, in which case take - * current state: - */ - if (!plane_state) - plane_state = plane->state; + const struct drm_plane_state *plane_state = + __drm_atomic_get_current_plane_state(state->state, + plane);

dlist_count += vc4_plane_dlist_size(plane_state); } diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 37cac59401d7..c799baabc008 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -469,7 +469,7 @@ int vc4_kms_load(struct drm_device *dev); struct drm_plane *vc4_plane_init(struct drm_device *dev, enum drm_plane_type type); u32 vc4_plane_write_dlist(struct drm_plane *plane, u32 __iomem *dlist); -u32 vc4_plane_dlist_size(struct drm_plane_state *state); +u32 vc4_plane_dlist_size(const struct drm_plane_state *state); void vc4_plane_async_set_fb(struct drm_plane *plane, struct drm_framebuffer *fb);

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 4037b52fde31..5d2c3d9fd17a 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -690,9 +690,10 @@ u32 vc4_plane_write_dlist(struct drm_plane *plane, u32 __iomem *dlist) return vc4_state->dlist_count; }

-u32 vc4_plane_dlist_size(struct drm_plane_state *state) +u32 vc4_plane_dlist_size(const struct drm_plane_state *state) { - struct vc4_plane_state *vc4_state = to_vc4_plane_state(state); + const struct vc4_plane_state *vc4_state = + container_of(state, typeof(*vc4_state), base);

return vc4_state->dlist_count; } diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 92c84e9ab09a..4e97186293be 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -109,6 +109,42 @@ drm_atomic_get_existing_connector_state(struct drm_atomic_state *state, return state->connector_states[index]; }

+/** + * __drm_atomic_get_current_plane_state - get current plane state + * @state: global atomic state object + * @plane: plane to grab + * + * This function returns the plane state for the given plane, either from + * @state, or if the plane isn't part of the atomic state update, from @plane. + * This is useful in atomic check callbacks, when drivers need to peek at, but + * not change, state of other planes, since it avoids threading an error code + * back up the call chain. + * + * WARNING: + * + * Note that this function is in general unsafe since it doesn't check for the + * required locking for access state structures. Drivers must ensure that it is + * save to access the returned state structure through other means. One commone + * example is when planes are fixed to a single CRTC, and the driver knows that + * the CRTC locks is held already. In that case holding the CRTC locks gives a + * read-lock on all planes connected to that CRTC. But if planes can be + * reassigned things get more tricky. In that case it's better to use + * drm_atomic_get_plane_state and wire up full error handling. + * + * Returns: + * + * Read-only pointer to the current plane state. + */ +static inline const struct drm_plane_state * +__drm_atomic_get_current_plane_state(struct drm_atomic_state *state, + struct drm_plane *plane) +{ + if (state->plane_states[drm_plane_index(plane)]) + return state->plane_states[drm_plane_index(plane)]; + + return plane->state; +} + int __must_check drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, struct drm_display_mode *mode);

We want to hide drm_atomic_state internals better.

Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/exynos/exynos_drm_drv.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 2dd820e23b0c..cabc5fd0246d 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -267,6 +267,8 @@ int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, { struct exynos_drm_private *priv = dev->dev_private; struct exynos_atomic_commit *commit; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; int i, ret;

commit = kzalloc(sizeof(*commit), GFP_KERNEL); @@ -288,10 +290,8 @@ int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, /* Wait until all affected CRTCs have completed previous commits and * mark them as pending. */ - for (i = 0; i < dev->mode_config.num_crtc; ++i) { - if (state->crtcs[i]) - commit->crtcs |= 1 << drm_crtc_index(state->crtcs[i]); - } + for_each_crtc_in_state(state, crtc, crtc_state, i) + commit->crtcs |= 1 << drm_crtc_index(crtc);

wait_event(priv->wait, !commit_is_pending(priv, commit->crtcs));

It's kinda pointless to have 2 separate mallocs for these. And when we add more per-plane state in the future it's even more pointless.

Right now there's no such thing planned, but both Gustavo's per-crtc fence patches, and some nonblocking commit helpers I'm playing around with will add more per-crtc stuff. It makes sense to also consolidate planes, just for consistency.

In the future we can use this to store a pointer to the preceeding state, making an atomic update entirely free-standing. This will be needed to be able to queue them up with a depth > 1.

Cc: Gustavo Padovan gustavo@padovan.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic.c | 17 ++++++----------- drivers/gpu/drm/drm_atomic_helper.c | 2 +- drivers/gpu/drm/i915/intel_atomic.c | 2 +- include/drm/drm_atomic.h | 14 +++++++------- include/drm/drm_crtc.h | 11 +++++++---- 5 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index a6395e9654af..68fd99d2fd01 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -47,7 +47,6 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state) kfree(state->crtcs); kfree(state->crtc_states); kfree(state->planes); - kfree(state->plane_states); } EXPORT_SYMBOL(drm_atomic_state_default_release);

@@ -79,10 +78,6 @@ drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state) sizeof(*state->planes), GFP_KERNEL); if (!state->planes) goto fail; - state->plane_states = kcalloc(dev->mode_config.num_total_plane, - sizeof(*state->plane_states), GFP_KERNEL); - if (!state->plane_states) - goto fail;

state->dev = dev;

@@ -163,15 +158,15 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) }

for (i = 0; i < config->num_total_plane; i++) { - struct drm_plane *plane = state->planes[i]; + struct drm_plane *plane = state->planes[i].ptr;

if (!plane) continue;

plane->funcs->atomic_destroy_state(plane, - state->plane_states[i]); - state->planes[i] = NULL; - state->plane_states[i] = NULL; + state->planes[i].state); + state->planes[i].ptr = NULL; + state->planes[i].state = NULL; } } EXPORT_SYMBOL(drm_atomic_state_default_clear); @@ -630,8 +625,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state, if (!plane_state) return ERR_PTR(-ENOMEM);

- state->plane_states[index] = plane_state; - state->planes[index] = plane; + state->planes[index].state = plane_state; + state->planes[index].ptr = plane; plane_state->state = state;

DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n", diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0fb4868fdad6..f70d576f745e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1579,7 +1579,7 @@ void drm_atomic_helper_swap_state(struct drm_device *dev,

for_each_plane_in_state(state, plane, plane_state, i) { plane->state->state = state; - swap(state->plane_states[i], plane->state); + swap(state->planes[i].state, plane->state); plane->state->state = NULL; } } diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 3e6d9ff8840a..7542f5f5db1d 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -191,7 +191,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev,

/* plane scaler case: assign as a plane scaler */ /* find the plane that set the bit as scaler_user */ - plane = drm_state->planes[i]; + plane = drm_state->planes[i].ptr;

/* * to enable/disable hq mode, add planes that are using scaler diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 37478adb6a16..8e616d39353b 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -86,7 +86,7 @@ static inline struct drm_plane_state * drm_atomic_get_existing_plane_state(struct drm_atomic_state *state, struct drm_plane *plane) { - return state->plane_states[drm_plane_index(plane)]; + return state->planes[drm_plane_index(plane)].state; }

/** @@ -139,8 +139,8 @@ static inline const struct drm_plane_state * __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, struct drm_plane *plane) { - if (state->plane_states[drm_plane_index(plane)]) - return state->plane_states[drm_plane_index(plane)]; + if (state->planes[drm_plane_index(plane)].state) + return state->planes[drm_plane_index(plane)].state;

return plane->state; } @@ -191,11 +191,11 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); (__i)++) \ for_each_if (crtc_state)

-#define for_each_plane_in_state(state, plane, plane_state, __i) \ +#define for_each_plane_in_state(__state, plane, plane_state, __i) \ for ((__i) = 0; \ - (__i) < (state)->dev->mode_config.num_total_plane && \ - ((plane) = (state)->planes[__i], \ - (plane_state) = (state)->plane_states[__i], 1); \ + (__i) < (__state)->dev->mode_config.num_total_plane && \ + ((plane) = (__state)->planes[__i].ptr, \ + (plane_state) = (__state)->planes[__i].state, 1); \ (__i)++) \ for_each_if (plane_state) static inline bool diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d4c46bfe4142..c7c2b3fa7179 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1694,6 +1694,11 @@ struct drm_bridge { void *driver_private; };

+struct __drm_planes_state { + struct drm_plane *ptr; + struct drm_plane_state *state; +}; + struct __drm_connnectors_state { struct drm_connector *ptr; struct drm_connector_state *state; @@ -1705,8 +1710,7 @@ struct __drm_connnectors_state { * @allow_modeset: allow full modeset * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics * @legacy_set_config: Disable conflicting encoders instead of failing with -EINVAL. - * @planes: pointer to array of plane pointers - * @plane_states: pointer to array of plane states pointers + * @planes: pointer to array of structures with per-plane data * @crtcs: pointer to array of CRTC pointers * @crtc_states: pointer to array of CRTC states pointers * @num_connector: size of the @connectors and @connector_states arrays @@ -1718,8 +1722,7 @@ struct drm_atomic_state { bool allow_modeset : 1; bool legacy_cursor_update : 1; bool legacy_set_config : 1; - struct drm_plane **planes; - struct drm_plane_state **plane_states; + struct __drm_planes_state *planes; struct drm_crtc **crtcs; struct drm_crtc_state **crtc_states; int num_connector;

From: Gustavo Padovan gustavo.padovan@collabora.co.uk

Now a drm_pending_event can either send a real drm_event or signal a fence, or both. It allow us to signal via fences when the buffer is displayed on the screen. Which in turn means that the previous buffer is not in use anymore and can be freed or sent back to another driver for processing.

v2: Comments from Daniel Vetter - call fence_signal in drm_send_event_locked() - remove unneeded !e->event check

v3: Remove drm_pending_event->destroy to fix a leak when e->file_priv is not set.

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk (v2) Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++------ drivers/gpu/drm/drm_fops.c | 16 +++++++++------- drivers/gpu/drm/nouveau/nouveau_usif.c | 1 - drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 +- include/drm/drmP.h | 3 ++- 5 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 674b2e490aa9..1db198df3014 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1412,7 +1412,8 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit); */

static struct drm_pending_vblank_event *create_vblank_event( - struct drm_device *dev, struct drm_file *file_priv, uint64_t user_data) + struct drm_device *dev, struct drm_file *file_priv, + struct fence *fence, uint64_t user_data) { struct drm_pending_vblank_event *e = NULL; int ret; @@ -1425,12 +1426,17 @@ static struct drm_pending_vblank_event *create_vblank_event( e->event.base.length = sizeof(e->event); e->event.user_data = user_data;

- ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base); - if (ret) { - kfree(e); - return NULL; + if (file_priv) { + ret = drm_event_reserve_init(dev, file_priv, &e->base, + &e->event.base); + if (ret) { + kfree(e); + return NULL; + } }

+ e->base.fence = fence; + return e; }

@@ -1670,7 +1676,8 @@ retry: for_each_crtc_in_state(state, crtc, crtc_state, i) { struct drm_pending_vblank_event *e;

- e = create_vblank_event(dev, file_priv, arg->user_data); + e = create_vblank_event(dev, file_priv, NULL, + arg->user_data); if (!e) { ret = -ENOMEM; goto out; diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 868871068956..4c4b30f7a9f2 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -294,7 +294,7 @@ static void drm_events_release(struct drm_file *file_priv) /* Remove unconsumed events */ list_for_each_entry_safe(e, et, &file_priv->event_list, link) { list_del(&e->link); - e->destroy(e); + kfree(e); }

spin_unlock_irqrestore(&dev->event_lock, flags); @@ -525,7 +525,7 @@ put_back_event: }

ret += length; - e->destroy(e); + kfree(e); } } mutex_unlock(&file_priv->event_read_lock); @@ -602,9 +602,6 @@ int drm_event_reserve_init_locked(struct drm_device *dev, list_add(&p->pending_link, &file_priv->pending_event_list); p->file_priv = file_priv;

- /* we *could* pass this in as arg, but everyone uses kfree: */ - p->destroy = (void (*) (struct drm_pending_event *)) kfree; - return 0; } EXPORT_SYMBOL(drm_event_reserve_init_locked); @@ -667,7 +664,7 @@ void drm_event_cancel_free(struct drm_device *dev, list_del(&p->pending_link); } spin_unlock_irqrestore(&dev->event_lock, flags); - p->destroy(p); + kfree(p); } EXPORT_SYMBOL(drm_event_cancel_free);

@@ -689,8 +686,13 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e) { assert_spin_locked(&dev->event_lock);

+ if (e->fence) { + fence_signal(e->fence); + fence_put(e->fence); + } + if (!e->file_priv) { - e->destroy(e); + kfree(e); return; }

diff --git a/drivers/gpu/drm/nouveau/nouveau_usif.c b/drivers/gpu/drm/nouveau/nouveau_usif.c index 675e9e077a95..08f9c6fa0f7f 100644 --- a/drivers/gpu/drm/nouveau/nouveau_usif.c +++ b/drivers/gpu/drm/nouveau/nouveau_usif.c @@ -212,7 +212,6 @@ usif_notify_get(struct drm_file *f, void *data, u32 size, void *argv, u32 argc) ntfy->p->base.event = &ntfy->p->e.base; ntfy->p->base.file_priv = f; ntfy->p->base.pid = current->pid; - ntfy->p->base.destroy =(void(*)(struct drm_pending_event *))kfree; ntfy->p->e.base.type = DRM_NOUVEAU_EVENT_NVIF; ntfy->p->e.base.length = sizeof(ntfy->p->e.base) + ntfy->reply;

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 1c4d5b5a70a2..5567fb43e674 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -889,7 +889,7 @@ static void vop_crtc_cancel_pending_vblank(struct drm_crtc *crtc, if (e && e->base.file_priv == file_priv) { vop->event = NULL;

- e->base.destroy(&e->base); + kfree(&e->base); file_priv->event_space += sizeof(e->event); } spin_unlock_irqrestore(&drm->event_lock, flags); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index f51fa4328494..51f751d1c8a4 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -57,6 +57,7 @@ #include <linux/types.h> #include <linux/vmalloc.h> #include <linux/workqueue.h> +#include <linux/fence.h>

#include <asm/mman.h> #include <asm/pgalloc.h> @@ -283,12 +284,12 @@ struct drm_ioctl_desc { /* Event queued up for userspace to read */ struct drm_pending_event { struct drm_event *event; + struct fence *fence; struct list_head link; struct list_head pending_link; struct drm_file *file_priv; pid_t pid; /* pid of requester, no guarantee it's valid by the time we deliver the event, for tracing only */ - void (*destroy)(struct drm_pending_event *event); };

/* initial implementaton using a linked list - todo hashtab */

The drm core has a nice ready-made helper for exactly the simple case where it should fire on the next vblank.

Note that arming the vblank event in _begin is probably too early, and might easily result in the vblank firing too early, before the new set of planes are actually disabled. But that's kinda a minor issue compared to just outright hanging userspace.

v2: Be more robust and either arm, when the CRTC is on, or just send the event out right away.

Cc: Carlos Palminha palminha@synopsys.com Cc: Alexey Brodkin abrodkin@synopsys.com Cc: linux-snps-arc@lists.infradead.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/arc/arcpgu_crtc.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c index d5ca0c280e68..c9f183b11df9 100644 --- a/drivers/gpu/drm/arc/arcpgu_crtc.c +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c @@ -145,16 +145,17 @@ static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc, static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, struct drm_crtc_state *state) { - struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); - unsigned long flags; - - if (crtc->state->event) { - struct drm_pending_vblank_event *event = crtc->state->event; + struct drm_pending_vblank_event *event = crtc->state->event;

+ if (event) { crtc->state->event = NULL; - event->pipe = drm_crtc_index(crtc);

- WARN_ON(drm_crtc_vblank_get(crtc) != 0); + spin_lock_irq(&crtc->dev->event_lock); + if (drm_crtc_vblank_get(crtc) == 0) + drm_crtc_arm_vblank_event(crtc, event); + else + drm_crtc_send_vblank_event(crtc, event); + spin_unlock_irq(&crtc->dev->event_lock); } }

Again, just doing them as blocking commits isn't cool.

From a cursory check hdlcd does seem to at least try to handle

drm_pending_vblank_event, so hopefully this works.

Cc: Liviu Dudau Liviu.Dudau@arm.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/arm/hdlcd_drv.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c index 4f909378d581..6d1877238cf0 100644 --- a/drivers/gpu/drm/arm/hdlcd_drv.c +++ b/drivers/gpu/drm/arm/hdlcd_drv.c @@ -112,17 +112,11 @@ static void hdlcd_fb_output_poll_changed(struct drm_device *drm) drm_fbdev_cma_hotplug_event(hdlcd->fbdev); }

-static int hdlcd_atomic_commit(struct drm_device *dev, - struct drm_atomic_state *state, bool nonblock) -{ - return drm_atomic_helper_commit(dev, state, false); -} - static const struct drm_mode_config_funcs hdlcd_mode_config_funcs = { .fb_create = drm_fb_cma_create, .output_poll_changed = hdlcd_fb_output_poll_changed, .atomic_check = drm_atomic_helper_check, - .atomic_commit = hdlcd_atomic_commit, + .atomic_commit = drm_atomic_helper_commit, };

static void hdlcd_setup_mode_config(struct drm_device *drm)

No idea how exactly fsl-du commits hw state changes, but here in flush is probably the safest place.

While at it nuke the dummy functions.

v2: Be more robust and either arm, when the CRTC is on, or just send the event out right away.

Cc: Stefan Agner stefan@agner.ch Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c index 89c0084c2814..706de3278f1c 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c @@ -22,20 +22,21 @@ #include "fsl_dcu_drm_drv.h" #include "fsl_dcu_drm_plane.h"

-static void fsl_dcu_drm_crtc_atomic_begin(struct drm_crtc *crtc, +static void fsl_dcu_drm_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state) { -} + struct drm_pending_vblank_event *event = crtc->state->event;

-static int fsl_dcu_drm_crtc_atomic_check(struct drm_crtc *crtc, - struct drm_crtc_state *state) -{ - return 0; -} + if (event) { + crtc->state->event = NULL;

-static void fsl_dcu_drm_crtc_atomic_flush(struct drm_crtc *crtc, - struct drm_crtc_state *old_crtc_state) -{ + spin_lock_irq(&crtc->dev->event_lock); + if (drm_crtc_vblank_get(crtc) == 0) + drm_crtc_arm_vblank_event(crtc, event); + else + drm_crtc_send_vblank_event(crtc, event); + spin_unlock_irq(&crtc->dev->event_lock); + } }

static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc) @@ -117,8 +118,6 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc) }

static const struct drm_crtc_helper_funcs fsl_dcu_drm_crtc_helper_funcs = { - .atomic_begin = fsl_dcu_drm_crtc_atomic_begin, - .atomic_check = fsl_dcu_drm_crtc_atomic_check, .atomic_flush = fsl_dcu_drm_crtc_atomic_flush, .disable = fsl_dcu_drm_disable_crtc, .enable = fsl_dcu_drm_crtc_enable,

atomic_flush seems to be the right place, right after we commit the plane updates. Again use the fullproof version, since the pipe might be off.

Cc: Boris Brezillon boris.brezillon@free-electrons.com Cc: Maxime Ripard maxime.ripard@free-electrons.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/sun4i/sun4i_crtc.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c index 4182a21f5923..f628b6d8f23f 100644 --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c @@ -51,10 +51,22 @@ static void sun4i_crtc_atomic_flush(struct drm_crtc *crtc, { struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc); struct sun4i_drv *drv = scrtc->drv; + struct drm_pending_vblank_event *event = crtc->state->event;

DRM_DEBUG_DRIVER("Committing plane changes\n");

sun4i_backend_commit(drv->backend); + + if (event) { + crtc->state->event = NULL; + + spin_lock_irq(&crtc->dev->event_lock); + if (drm_crtc_vblank_get(crtc) == 0) + drm_crtc_arm_vblank_event(crtc, event); + else + drm_crtc_send_vblank_event(crtc, event); + spin_unlock_irq(&crtc->dev->event_lock); + } }

static void sun4i_crtc_disable(struct drm_crtc *crtc)

Design ideas:

- split up the actual commit into different phases, and have completions for each of them. This will be useful for the future when we want to interleave phases much more aggressively, for e.g. queue depth > 1. For not it's just a minimal optimization compared to current common nonblocking implementation patterns from drivers, which all stall for the entire commit to complete, including vblank waits and cleanups.

- Extract a separate atomic_commit_hw hook since that's the part most drivers will need to overwrite, hopefully allowing even more shared code.

- Enforce EBUSY seamntics by attaching one of the completions to the flip_done vblank event. Side benefit of forcing atomic drivers using these helpers to implement event handlign at least semi-correct. I'm evil that way ;-)

- Ridiculously modular, as usual.

- The main tracking unit for a commit stays struct drm_atomic_state, and the ownership rules for that are unchanged. Ownership still gets transferred to the driver (and subsequently to the worker) on successful commits. What is added is a small, per-crtc, refcounted structure to track pending commits called struct drm_crtc_commit. No actual state is attached to that though, it's purely for ordering and waiting.

- Dependencies are implicitly handled by assuming that any CRTC part of &drm_atomic_state is a dependency, and that the current commit must wait for any commits to complete on those CRTC. This way drivers can easily add more depencies using drm_atomic_get_crtc_state(), which is very natural since in most case a dependency exists iff there's some bit of state that needs to be cross checked.

Removing depencies is not possible, drivers simply need to be careful to not include every CRTC in a commit if that's not necessary. Which is a good idea anyway, since that also avoids ww_mutex lock contention.

- Queue depth > 1 sees some prep work in this patch by adding a stall paramater to drm_atomic_helper_swap_states(). To be able to push commits entirely free-standing and in a deeper queue through the back-end the driver must not access any obj->state pointers. This means we need to track the old state in drm_atomic_state (much easier with the consolidated arrays), and pass them all explicitly to driver backends (this will be serious amounts of churn).

Once that's done stall can be set to false in swap_states.

Features: Contains bugs because totally untested.

v2: Dont ask for flip_done signalling when the CRTC is off and stays off: Drivers don't handle events in that case. Instead complete right away. This way future commits don't need to have special-case logic, but can keep blocking for the flip_done completion.

v3: Tons of fixes: - Stall for preceeding commit for real, not the current one by accident. - Add WARN_ON in case drivers don't fire the drm event. - Don't double-free drm events.

v4: Make legacy cursor not stall.

v5: Extend the helper hook to cover the entire commit tail. Some drivers need special code for cleanup and vblank waiting, this makes it a bit more useful. Inspired by the rockchip driver.

v6: Add WARN_ON to catch drivers who forget to send out the drm event.

Cc: Tomeu Vizoso tomeu.vizoso@gmail.com Cc: Daniel Stone daniels@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic.c | 22 ++ drivers/gpu/drm/drm_atomic_helper.c | 366 ++++++++++++++++++++++++++++--- drivers/gpu/drm/drm_crtc.c | 3 + drivers/gpu/drm/drm_fops.c | 6 + include/drm/drmP.h | 1 + include/drm/drm_atomic.h | 6 + include/drm/drm_atomic_helper.h | 8 + include/drm/drm_crtc.h | 119 +++++++++- include/drm/drm_modeset_helper_vtables.h | 36 +++ 9 files changed, 534 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 1db198df3014..50aa341ca9ff 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -33,6 +33,20 @@

#include "drm_crtc_internal.h"

+static void crtc_commit_free(struct kref *kref) +{ + struct drm_crtc_commit *commit = + container_of(kref, struct drm_crtc_commit, ref); + + kfree(commit); +} + +void drm_crtc_commit_put(struct drm_crtc_commit *commit) +{ + kref_put(&commit->ref, crtc_commit_free); +} +EXPORT_SYMBOL(drm_crtc_commit_put); + /** * drm_atomic_state_default_release - * release memory initialized by drm_atomic_state_init @@ -148,6 +162,14 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)

crtc->funcs->atomic_destroy_state(crtc, state->crtcs[i].state); + + if (state->crtcs[i].commit) { + kfree(state->crtcs[i].commit->event); + state->crtcs[i].commit->event = NULL; + drm_crtc_commit_put(state->crtcs[i].commit); + } + + state->crtcs[i].commit = NULL; state->crtcs[i].ptr = NULL; state->crtcs[i].state = NULL; } diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index fecbb52cbb85..a5854a31f493 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1113,20 +1113,15 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev, EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);

/** - * drm_atomic_helper_commit - commit validated state object - * @dev: DRM device - * @state: the driver state object - * @nonblocking: whether nonblocking behavior is requested. + * drm_atomic_helper_commit_tail - commit atomic update to hardware + * @state: new modeset state to be committed * - * This function commits a with drm_atomic_helper_check() pre-validated state - * object. This can still fail when e.g. the framebuffer reservation fails. For - * now this doesn't implement nonblocking commits. + * This is the default implemenation for the ->atomic_commit_tail() hook of the + * &drm_mode_config_helper_funcs vtable. * - * Note that right now this function does not support nonblocking commits, hence - * driver writers must implement their own version for now. Also note that the - * default ordering of how the various stages are called is to match the legacy - * modeset helper library closest. One peculiarity of that is that it doesn't - * mesh well with runtime PM at all. + * Note that the default ordering of how the various stages are called is to + * match the legacy modeset helper library closest. One peculiarity of that is + * that it doesn't mesh well with runtime PM at all. * * For drivers supporting runtime PM the recommended sequence is * @@ -1137,6 +1132,70 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks); * drm_atomic_helper_commit_planes(dev, state, true); * * See the kerneldoc entries for these three functions for more details. + */ +void drm_atomic_helper_commit_tail(struct drm_atomic_state *state) +{ + struct drm_device *dev = state->dev; + + drm_atomic_helper_commit_modeset_disables(dev, state); + + drm_atomic_helper_commit_planes(dev, state, false); + + drm_atomic_helper_commit_modeset_enables(dev, state); + + drm_atomic_helper_commit_hw_done(state); + + drm_atomic_helper_wait_for_vblanks(dev, state); + + drm_atomic_helper_cleanup_planes(dev, state); +} +EXPORT_SYMBOL(drm_atomic_helper_commit_tail); + +void commit_tail(struct drm_atomic_state *state) +{ + struct drm_device *dev = state->dev; + struct drm_mode_config_helper_funcs *funcs; + + funcs = dev->mode_config.helper_private; + + drm_atomic_helper_wait_for_fences(dev, state); + + drm_atomic_helper_wait_for_dependencies(state); + + if (funcs && funcs->atomic_commit_tail) + funcs->atomic_commit_tail(state); + else + drm_atomic_helper_commit_tail(state); + + drm_atomic_helper_commit_cleanup_done(state); + + drm_atomic_state_free(state); +} + +static void commit_work(struct work_struct *work) +{ + struct drm_atomic_state *state = container_of(work, + struct drm_atomic_state, + commit_work); + commit_tail(state); +} + +/** + * drm_atomic_helper_commit - commit validated state object + * @dev: DRM device + * @state: the driver state object + * @nonblocking: whether nonblocking behavior is requested. + * + * This function commits a with drm_atomic_helper_check() pre-validated state + * object. This can still fail when e.g. the framebuffer reservation fails. For + * now this doesn't implement nonblocking commits. + * + * Note that right now this function does not support nonblocking commits, hence + * driver writers must implement their own version for now. + * + * Committing the actual hardware state is done through the + * ->atomic_commit_tail() callback of the &drm_mode_config_helper_funcs vtable, + * or it's default implementation drm_atomic_helper_commit_tail(). * * RETURNS * Zero for success or -errno. @@ -1147,8 +1206,11 @@ int drm_atomic_helper_commit(struct drm_device *dev, { int ret;

- if (nonblock) - return -EBUSY; + ret = drm_atomic_helper_setup_commit(state, nonblock); + if (ret) + return ret; + + INIT_WORK(&state->commit_work, commit_work);

ret = drm_atomic_helper_prepare_planes(dev, state); if (ret) @@ -1176,21 +1238,16 @@ int drm_atomic_helper_commit(struct drm_device *dev, * update. Which is important since compositors need to figure out the * composition of the next frame right after having submitted the * current layout. + * + * NOTE: Commit work has multiple phases, first hardware commit, then + * cleanup. We want them to overlap, hence need system_unbound_wq to + * make sure work items don't artifically stall on each another. */

- drm_atomic_helper_wait_for_fences(dev, state); - - drm_atomic_helper_commit_modeset_disables(dev, state); - - drm_atomic_helper_commit_planes(dev, state, false); - - drm_atomic_helper_commit_modeset_enables(dev, state); - - drm_atomic_helper_wait_for_vblanks(dev, state); - - drm_atomic_helper_cleanup_planes(dev, state); - - drm_atomic_state_free(state); + if (nonblock) + queue_work(system_unbound_wq, &state->commit_work); + else + commit_tail(state);

return 0; } @@ -1234,6 +1291,225 @@ EXPORT_SYMBOL(drm_atomic_helper_commit); * being displayed. */

+static int stall_checks(struct drm_crtc *crtc, bool nonblock) +{ + struct drm_crtc_commit *commit, *stall_commit = NULL; + bool completed = true; + int i, ret = 0; + + spin_lock(&crtc->commit_lock); + i = 0; + list_for_each_entry(commit, &crtc->commit_list, commit_entry) { + if (i == 0) { + completed = try_wait_for_completion(&commit->flip_done); + /* Userspace is not allowed to get ahead of the previous + * commit with nonblocking ones. */ + if (!completed && nonblock) { + spin_unlock(&crtc->commit_lock); + return -EBUSY; + } + } else if (i == 1) { + stall_commit = commit; + drm_crtc_commit_get(stall_commit); + } else + break; + + i++; + } + spin_unlock(&crtc->commit_lock); + + if (!stall_commit) + return 0; + + /* We don't want to let commits get ahead of cleanup work too much, + * stalling on 2nd previous commit means triple-buffer won't ever stall. + */ + ret = wait_for_completion_interruptible_timeout(&commit->cleanup_done, + 10*HZ); + if (ret == 0) + DRM_ERROR("[CRTC:%d:%s] cleanup_done timed out\n", + crtc->base.id, crtc->name); + + if (ret == -ERESTARTSYS) + ret = -EINTR; + + drm_crtc_commit_put(stall_commit); + + return ret; +} + +/** + * drm_atomic_helper_setup_commit - setup possible nonblocking commit + * + * Returns: + * + * 0 on success. -EBUSY when userspace schedules nonblocking commits too fast, + * -ENOMEM on allocation failures and -EINTR when a signal is pending. + */ +int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, + bool nonblock) +{ + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + struct drm_crtc_commit *commit; + int i, ret; + + for_each_crtc_in_state(state, crtc, crtc_state, i) { + commit = kzalloc(sizeof(*commit), GFP_KERNEL); + if (!commit) + return -ENOMEM; + + init_completion(&commit->flip_done); + init_completion(&commit->hw_done); + init_completion(&commit->cleanup_done); + INIT_LIST_HEAD(&commit->commit_entry); + kref_init(&commit->ref); + commit->crtc = crtc; + + state->crtcs[i].commit = commit; + + ret = stall_checks(crtc, nonblock); + if (ret) + return ret; + + /* Drivers only send out events when at least either current or + * new CRTC state is active. Complete right away if everything + * stays off. */ + if (!crtc->state->active && !crtc_state->active) { + complete_all(&commit->flip_done); + continue; + } + + /* Legacy cursor updates are fully unsynced. */ + if (state->legacy_cursor_update) { + complete_all(&commit->flip_done); + continue; + } + + if (!crtc_state->event) { + commit->event = kzalloc(sizeof(*commit->event), + GFP_KERNEL); + if (!commit->event) + return -ENOMEM; + + crtc_state->event = commit->event; + } + + crtc_state->event->base.completion = &commit->flip_done; + } + + return 0; +} +EXPORT_SYMBOL(drm_atomic_helper_setup_commit); + + +static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc) +{ + struct drm_crtc_commit *commit; + int i = 0; + + list_for_each_entry(commit, &crtc->commit_list, commit_entry) { + /* skip the first entry, that's the current commit */ + if (i == 1) + return commit; + i++; + } + + return NULL; +} + +void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state) +{ + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + struct drm_crtc_commit *commit; + int i; + long ret; + + for_each_crtc_in_state(state, crtc, crtc_state, i) { + spin_lock(&crtc->commit_lock); + commit = preceeding_commit(crtc); + if (commit) + drm_crtc_commit_get(commit); + spin_unlock(&crtc->commit_lock); + + if (!commit) + continue; + + ret = wait_for_completion_timeout(&commit->hw_done, + 10*HZ); + if (ret == 0) + DRM_ERROR("[CRTC:%d:%s] hw_done timed out\n", + crtc->base.id, crtc->name); + + /* Currently no support for overwriting flips, hence + * stall for previous one to execute completely. */ + ret = wait_for_completion_timeout(&commit->flip_done, + 10*HZ); + if (ret == 0) + DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n", + crtc->base.id, crtc->name); + + drm_crtc_commit_put(commit); + } +} +EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies); + +void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state) +{ + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + struct drm_crtc_commit *commit; + int i; + + for_each_crtc_in_state(state, crtc, crtc_state, i) { + commit = state->crtcs[i].commit; + if (!commit) + continue; + + /* backend must have consumed any event by now */ + WARN_ON(crtc->state->event); + spin_lock(&crtc->commit_lock); + complete_all(&commit->hw_done); + spin_unlock(&crtc->commit_lock); + } +} +EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done); + +void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state) +{ + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + struct drm_crtc_commit *commit; + int i; + long ret; + + for_each_crtc_in_state(state, crtc, crtc_state, i) { + commit = state->crtcs[i].commit; + if (!commit) + continue; + spin_lock(&crtc->commit_lock); + complete_all(&commit->cleanup_done); + + WARN_ON(!try_wait_for_completion(&commit->hw_done)); + + /* commit_list borrows our reference, need to remove before we + * clean up our drm_atomic_state. */ + list_del(&commit->commit_entry); + spin_unlock(&crtc->commit_lock); + + /* We must wait for the vblank event to signal our completion + * before releasing our reference, since the vblank work does + * not hold a reference of its own. */ + ret = wait_for_completion_timeout(&commit->flip_done, + 10*HZ); + if (ret == 0) + DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n", + crtc->base.id, crtc->name); + } +} +EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done); + /** * drm_atomic_helper_prepare_planes - prepare plane resources before commit * @dev: DRM device @@ -1553,17 +1829,44 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes); * * 5. Call drm_atomic_helper_cleanup_planes() with @state, which since step 3 * contains the old state. Also do any other cleanup required with that state. + * + * @stall must be set when nonblocking commits for this driver directly access + * the ->state pointer of &drm_plane, &drm_crtc or &drm_connector. With the + * current atomic helpers this is almost always the case, since the helpers + * don't pass the right state structures to the callbacks. */ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, bool stall) { int i; + long ret; struct drm_connector *connector; struct drm_connector_state *conn_state; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; struct drm_plane *plane; struct drm_plane_state *plane_state; + struct drm_crtc_commit *commit; + + if (stall) { + for_each_crtc_in_state(state, crtc, crtc_state, i) { + spin_lock(&crtc->commit_lock); + commit = preceeding_commit(crtc); + if (commit) + drm_crtc_commit_get(commit); + spin_unlock(&crtc->commit_lock); + + if (!commit) + continue; + + ret = wait_for_completion_timeout(&commit->hw_done, + 10*HZ); + if (ret == 0) + DRM_ERROR("[CRTC:%d:%s] hw_done timed out\n", + crtc->base.id, crtc->name); + drm_crtc_commit_put(commit); + } + }

for_each_connector_in_state(state, connector, conn_state, i) { connector->state->state = state; @@ -1575,6 +1878,15 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, crtc->state->state = state; swap(state->crtcs[i].state, crtc->state); crtc->state->state = NULL; + + if (state->crtcs[i].commit) { + spin_lock(&crtc->commit_lock); + list_add(&state->crtcs[i].commit->commit_entry, + &crtc->commit_list); + spin_unlock(&crtc->commit_lock); + + state->crtcs[i].commit->event = NULL; + } }

for_each_plane_in_state(state, plane, plane_state, i) { diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 88cf22052a69..3e52a6ecf6c0 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -669,6 +669,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, crtc->dev = dev; crtc->funcs = funcs;

+ INIT_LIST_HEAD(&crtc->commit_list); + spin_lock_init(&crtc->commit_lock); + drm_modeset_lock_init(&crtc->mutex); ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); if (ret) diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 4c4b30f7a9f2..5921b203503a 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -686,6 +686,12 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e) { assert_spin_locked(&dev->event_lock);

+ if (e->completion) { + /* ->completion might disappear as soon as it signalled. */ + complete_all(e->completion); + e->completion = NULL; + } + if (e->fence) { fence_signal(e->fence); fence_put(e->fence); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 51f751d1c8a4..781db4f562d4 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -283,6 +283,7 @@ struct drm_ioctl_desc {

/* Event queued up for userspace to read */ struct drm_pending_event { + struct completion *completion; struct drm_event *event; struct fence *fence; struct list_head link; diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 465a1212f4f0..abec2a3f0225 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -30,6 +30,12 @@

#include <drm/drm_crtc.h>

+void drm_crtc_commit_put(struct drm_crtc_commit *commit); +static inline void drm_crtc_commit_get(struct drm_crtc_commit *commit) +{ + kref_get(&commit->ref); +} + struct drm_atomic_state * __must_check drm_atomic_state_alloc(struct drm_device *dev); void drm_atomic_state_clear(struct drm_atomic_state *state); diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 0276447225ed..5fb8c306e16b 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -38,6 +38,7 @@ int drm_atomic_helper_check_planes(struct drm_device *dev, struct drm_atomic_state *state); int drm_atomic_helper_check(struct drm_device *dev, struct drm_atomic_state *state); +void drm_atomic_helper_commit_tail(struct drm_atomic_state *state); int drm_atomic_helper_commit(struct drm_device *dev, struct drm_atomic_state *state, bool nonblock); @@ -74,6 +75,13 @@ void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc, void drm_atomic_helper_swap_state(struct drm_atomic_state *state, bool stall);

+/* nonblocking commit helpers */ +int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, + bool nonblock); +void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state); +void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state); +void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state); + /* implementations for legacy interfaces */ int drm_atomic_helper_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d5d5e343531e..c0edd87fa0ee 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -725,9 +725,6 @@ struct drm_crtc_funcs { * @gamma_store: gamma ramp values * @helper_private: mid-layer private data * @properties: property tracking for this CRTC - * @state: current atomic state for this CRTC - * @acquire_ctx: per-CRTC implicit acquire context used by atomic drivers for - * legacy IOCTLs * * Each CRTC may have one or more connectors associated with it. This structure * allows the CRTC to be controlled. @@ -780,11 +777,37 @@ struct drm_crtc {

struct drm_object_properties properties;

+ /** + * @state: + * + * Current atomic state for this CRTC. + */ struct drm_crtc_state *state;

- /* - * For legacy crtc IOCTLs so that atomic drivers can get at the locking - * acquire context. + /** + * @commit_list: + * + * List of &drm_crtc_commit structures tracking pending commits. + * Protected by @commit_lock. This list doesn't hold its own full + * reference, but burrows it from the ongoing commit. Commit entries + * must be removed from this list once the commit is fully completed, + * but before it's correspoding &drm_atomic_state gets destroyed. + */ + struct list_head commit_list; + + /** + * @commit_lock: + * + * Spinlock to protect @commit_list. + */ + spinlock_t commit_lock; + + /** + * @acquire_ctx: + * + * Per-CRTC implicit acquire context used by atomic drivers for legacy + * IOCTLs, so that atomic drivers can get at the locking acquire + * context. */ struct drm_modeset_acquire_ctx *acquire_ctx; }; @@ -1694,6 +1717,78 @@ struct drm_bridge { void *driver_private; };

+/** + * struct drm_crtc_commit - track modeset commits on a CRTC + */ +struct drm_crtc_commit { + /** + * @crtc: + * + * DRM CRTC for this commit. + */ + struct drm_crtc *crtc; + + /** + * @ref: + * + * Reference count for this structure. Needed to allow blocking on + * completions without the risk of the completion disappearing + * meanwhile. + */ + struct kref ref; + + /** + * @flip_done: + * + * Will be signaled when the hardware has flipped to the new set of + * buffers. Signals at the same time as when the drm event for this + * commit is sent to userspace, or when an out-fence is singalled. Note + * that for most hardware, in most cases this happens after @hw_done is + * signalled. + */ + struct completion flip_done; + + /** + * @hw_done: + * + * Will be signalled when all hw register changes for this commit have + * been written out. Especially when disabling a pipe this can be much + * later than than @flip_done, since that can signal already when the + * screen goes black, whereas to fully shut down a pipe more register + * I/O is required. + * + * Note that this does not need to include separately reference-counted + * resources like backing storage buffer pinning, or runtime pm + * management. + */ + struct completion hw_done; + + /** + * cleanup_done: + * + * Will be signalled after old buffers have been cleaned up again by + * calling drm_atomic_helper_cleanup_planes(). Since this can only + * happen after a vblank wait completed it might be a bit later. This + * completion is useful to throttle updates and avoid hardware updates + * getting ahead of the buffer cleanup too much. + */ + struct completion cleanup_done; + + /** + * @commit_entry: + * + * Entry on the per-CRTC commit_list. Protected by crtc->commit_lock. + */ + struct list_head commit_entry; + + /** + * @event: + * + * &drm_pending_vblank_event pointer to clean up private events. + */ + struct drm_pending_vblank_event *event; +}; + struct __drm_planes_state { struct drm_plane *ptr; struct drm_plane_state *state; @@ -1702,6 +1797,7 @@ struct __drm_planes_state { struct __drm_crtcs_state { struct drm_crtc *ptr; struct drm_crtc_state *state; + struct drm_crtc_commit *commit; };

struct __drm_connnectors_state { @@ -1733,6 +1829,14 @@ struct drm_atomic_state { struct __drm_connnectors_state *connectors;

struct drm_modeset_acquire_ctx *acquire_ctx; + + /** + * @commit_work: + * + * Work item which can be used by the driver or helpers to execute the + * commit without blocking. + */ + struct work_struct commit_work; };

@@ -2074,6 +2178,7 @@ struct drm_mode_config_funcs { * @async_page_flip: does this device support async flips on the primary plane? * @cursor_width: hint to userspace for max cursor width * @cursor_height: hint to userspace for max cursor height + * @helper_private: mid-layer private data * * Core mode resource tracking structure. All CRTC, encoders, and connectors * enumerated by the driver are added here, as are global properties. Some @@ -2193,6 +2298,8 @@ struct drm_mode_config {

/* cursor size */ uint32_t cursor_width, cursor_height; + + struct drm_mode_config_helper_funcs *helper_private; };

/** diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index d4619dc2eecb..145c147705d8 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -925,4 +925,40 @@ static inline void drm_plane_helper_add(struct drm_plane *plane, plane->helper_private = funcs; }

+/** + * struct drm_mode_config_helper_funcs - global modeset helper operations + * + * These helper functions are used by the atomic helpers. + */ +struct drm_mode_config_helper_funcs { + /** + * @atomic_commit_tail: + * + * This hook should commit the given atomic state to the hardware, but + * nothing else. This excludes specifically any prepare and cleanup work + * done by drm_atomic_helper_prepare_planes() and + * drm_atomic_helper_cleanup_planes(). And also any synchronization and + * stalling necessary around the around the hardware commit as done for + * example by drm_atomic_helper_wait_for_fences() or + * drm_atomic_helper_wait_for_vblanks(). + * + * The function is not required to stall for the commit to actually + * complete and for the new buffers to be displayed on the screen. This + * is handled by properly signalling the &drm_pending_vblank_event in each + * &drm_crtc_state through a call to drm_send_vblank_event() or + * drm_arm_vblank_event(), depending upon how the driver works. + * + * When disabling a CRTC otoh this hook _must_ stall for the commit to + * complete. Vblank waits don't work on disabled CRTC, hence the core + * can't take care of this. And it also can't rely on the vblank event, + * since that can be signalled already when the screen shows black, + * which can happen much earlier than the last hardware access needed to + * shut off the display pipeline completely. + * + * This hook is optional, the default implementation is + * drm_atomic_helper_commit_tail(). + */ + void (*atomic_commit_tail)(struct drm_atomic_state *state); +}; + #endif

This is part of what atomic must implement. And it's also required to be able to use the helper nonblocking support.

v2: Always send out the drm event, remove the planes_changed check.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++--- drivers/gpu/drm/i915/intel_sprite.c | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d32274071393..66a254fa2a96 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13731,13 +13731,21 @@ static int intel_atomic_commit(struct drm_device *dev, bool modeset = needs_modeset(crtc->state); struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->state); - bool update_pipe = !modeset && pipe_config->update_pipe;

if (modeset && crtc->state->active) { update_scanline_offset(to_intel_crtc(crtc)); dev_priv->display.crtc_enable(crtc); }

+ /* Complete events for now disable pipes here. */ + if (modeset && !crtc->state->active && crtc->state->event) { + spin_lock_irq(&dev->event_lock); + drm_crtc_send_vblank_event(crtc, crtc->state->event); + spin_unlock_irq(&dev->event_lock); + + crtc->state->event = NULL; + } + if (!modeset) intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));

@@ -13745,8 +13753,7 @@ static int intel_atomic_commit(struct drm_device *dev, drm_atomic_get_existing_plane_state(state, crtc->primary)) intel_fbc_enable(intel_crtc);

- if (crtc->state->active && - (crtc->state->planes_changed || update_pipe)) + if (crtc->state->active) drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);

if (pipe_config->base.active && needs_vblank_wait(pipe_config)) diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 97b1a54eb09f..8864cc983ea2 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -166,6 +166,20 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work

trace_i915_pipe_update_end(crtc, end_vbl_count, scanline_end);

+ /* We're still in the vblank-evade critical section, this can't race. + * Would be slightly nice to just grab the vblank count and arm the + * event outside of the critical section - the spinlock might spin for a + * while ... */ + if (crtc->base.state->event) { + WARN_ON(drm_crtc_vblank_get(&crtc->base) != 0); + + spin_lock(&crtc->base.dev->event_lock); + drm_crtc_arm_vblank_event(&crtc->base, crtc->base.state->event); + spin_unlock(&crtc->base.dev->event_lock); + + crtc->base.state->event = NULL; + } + local_irq_enable();

if (crtc->debug.start_vbl_count &&

So totally untested ...

v2: Fixes from Tomeu.

v3: Send out vblank events correctly when shutting down a crtc for good. This is part of the atomic interface contract.

v4: Properly protect vop->event.

Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Cc: Mark yao mark.yao@rock-chips.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 3 -- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 10 ---- drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 72 ++++------------------------- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 15 +++++- 4 files changed, 22 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index 09a4d429c0f0..2fac6799ceb2 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -145,9 +145,6 @@ static int rockchip_drm_load(struct drm_device *drm_dev, unsigned long flags) if (!private) return -ENOMEM;

- mutex_init(&private->commit.lock); - INIT_WORK(&private->commit.work, rockchip_drm_atomic_work); - drm_dev->dev_private = private;

drm_mode_config_init(drm_dev); diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h index 56f43a364c7f..7684503ff765 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h @@ -43,13 +43,6 @@ struct rockchip_crtc_funcs { void (*cancel_pending_vblank)(struct drm_crtc *crtc, struct drm_file *file_priv); };

-struct rockchip_atomic_commit { - struct work_struct work; - struct drm_atomic_state *state; - struct drm_device *dev; - struct mutex lock; -}; - struct rockchip_crtc_state { struct drm_crtc_state base; int output_type; @@ -68,11 +61,8 @@ struct rockchip_drm_private { struct drm_fb_helper fbdev_helper; struct drm_gem_object *fbdev_bo; const struct rockchip_crtc_funcs *crtc_funcs[ROCKCHIP_MAX_CRTC]; - - struct rockchip_atomic_commit commit; };

-void rockchip_drm_atomic_work(struct work_struct *work); int rockchip_register_crtc_funcs(struct drm_crtc *crtc, const struct rockchip_crtc_funcs *crtc_funcs); void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc); diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index 5ea141dfae5b..9198ee1ddc87 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c @@ -229,87 +229,32 @@ rockchip_atomic_wait_for_complete(struct drm_device *dev, struct drm_atomic_stat }

static void -rockchip_atomic_commit_complete(struct rockchip_atomic_commit *commit) +rockchip_atomic_commit_tail(struct drm_atomic_state *state) { - struct drm_atomic_state *state = commit->state; - struct drm_device *dev = commit->dev; + struct drm_device *dev = state->dev;

- /* - * TODO: do fence wait here. - */ - - /* - * Rockchip crtc support runtime PM, can't update display planes - * when crtc is disabled. - * - * drm_atomic_helper_commit comments detail that: - * For drivers supporting runtime PM the recommended sequence is - * - * drm_atomic_helper_commit_modeset_disables(dev, state); - * - * drm_atomic_helper_commit_modeset_enables(dev, state); - * - * drm_atomic_helper_commit_planes(dev, state, true); - * - * See the kerneldoc entries for these three functions for more details. - */ drm_atomic_helper_commit_modeset_disables(dev, state);

drm_atomic_helper_commit_modeset_enables(dev, state);

drm_atomic_helper_commit_planes(dev, state, true);

+ drm_atomic_helper_commit_hw_done(state); + rockchip_atomic_wait_for_complete(dev, state);

drm_atomic_helper_cleanup_planes(dev, state); - - drm_atomic_state_free(state); -} - -void rockchip_drm_atomic_work(struct work_struct *work) -{ - struct rockchip_atomic_commit *commit = container_of(work, - struct rockchip_atomic_commit, work); - - rockchip_atomic_commit_complete(commit); }

-int rockchip_drm_atomic_commit(struct drm_device *dev, - struct drm_atomic_state *state, - bool nonblock) -{ - struct rockchip_drm_private *private = dev->dev_private; - struct rockchip_atomic_commit *commit = &private->commit; - int ret; - - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) - return ret; - - /* serialize outstanding nonblocking commits */ - mutex_lock(&commit->lock); - flush_work(&commit->work); - - drm_atomic_helper_swap_state(state, true); - - commit->dev = dev; - commit->state = state; - - if (nonblock) - schedule_work(&commit->work); - else - rockchip_atomic_commit_complete(commit); - - mutex_unlock(&commit->lock); - - return 0; -} +struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = { + .atomic_commit_tail = rockchip_atomic_commit_tail, +};

static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = { .fb_create = rockchip_user_fb_create, .output_poll_changed = rockchip_drm_output_poll_changed, .atomic_check = drm_atomic_helper_check, - .atomic_commit = rockchip_drm_atomic_commit, + .atomic_commit = drm_atomic_helper_commit, };

struct drm_framebuffer * @@ -340,4 +285,5 @@ void rockchip_drm_mode_config_init(struct drm_device *dev) dev->mode_config.max_height = 4096;

dev->mode_config.funcs = &rockchip_drm_mode_config_funcs; + dev->mode_config.helper_private = &rockchip_mode_config_helpers; } diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 5567fb43e674..8cde1ea62978 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -553,6 +553,14 @@ static void vop_crtc_disable(struct drm_crtc *crtc) clk_disable(vop->aclk); clk_disable(vop->hclk); pm_runtime_put(vop->dev); + + if (crtc->state->event && !crtc->state->active) { + spin_lock_irq(&crtc->dev->event_lock); + drm_crtc_send_vblank_event(crtc, crtc->state->event); + spin_unlock_irq(&crtc->dev->event_lock); + + crtc->state->event = NULL; + } }

static void vop_plane_destroy(struct drm_plane *plane) @@ -1027,12 +1035,14 @@ static void vop_crtc_atomic_begin(struct drm_crtc *crtc, { struct vop *vop = to_vop(crtc);

+ spin_lock_irq(&crtc->dev->event_lock); if (crtc->state->event) { WARN_ON(drm_crtc_vblank_get(crtc) != 0);

vop->event = crtc->state->event; crtc->state->event = NULL; } + spin_unlock_irq(&crtc->dev->event_lock); }

static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = { @@ -1104,15 +1114,16 @@ static void vop_handle_vblank(struct vop *vop) return; }

+ spin_lock_irqsave(&drm->event_lock, flags); if (vop->event) { - spin_lock_irqsave(&drm->event_lock, flags);

drm_crtc_send_vblank_event(crtc, vop->event); drm_crtc_vblank_put(crtc); vop->event = NULL;

- spin_unlock_irqrestore(&drm->event_lock, flags); } + spin_unlock_irqrestore(&drm->event_lock, flags); + if (!completion_done(&vop->wait_update_complete)) complete(&vop->wait_update_complete); }

Now that the core helpers support nonblocking atomic commits there's no need to invent that wheel separately (instead of fixing the bug in the atomic implementation of virtio, as it should have been done!).

Cc: Gerd Hoffmann kraxel@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/virtio/virtgpu_display.c | 48 ++------------------------------ 1 file changed, 2 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index d4305da88f44..325c6f73814b 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -118,58 +118,13 @@ static int virtio_gpu_crtc_cursor_move(struct drm_crtc *crtc, return 0; }

-static int virtio_gpu_page_flip(struct drm_crtc *crtc, - struct drm_framebuffer *fb, - struct drm_pending_vblank_event *event, - uint32_t flags) -{ - struct virtio_gpu_device *vgdev = crtc->dev->dev_private; - struct virtio_gpu_output *output = - container_of(crtc, struct virtio_gpu_output, crtc); - struct drm_plane *plane = crtc->primary; - struct virtio_gpu_framebuffer *vgfb; - struct virtio_gpu_object *bo; - unsigned long irqflags; - uint32_t handle; - - plane->fb = fb; - vgfb = to_virtio_gpu_framebuffer(plane->fb); - bo = gem_to_virtio_gpu_obj(vgfb->obj); - handle = bo->hw_res_handle; - - DRM_DEBUG("handle 0x%x%s, crtc %dx%d\n", handle, - bo->dumb ? ", dumb" : "", - crtc->mode.hdisplay, crtc->mode.vdisplay); - if (bo->dumb) { - virtio_gpu_cmd_transfer_to_host_2d - (vgdev, handle, 0, - cpu_to_le32(crtc->mode.hdisplay), - cpu_to_le32(crtc->mode.vdisplay), - 0, 0, NULL); - } - virtio_gpu_cmd_set_scanout(vgdev, output->index, handle, - crtc->mode.hdisplay, - crtc->mode.vdisplay, 0, 0); - virtio_gpu_cmd_resource_flush(vgdev, handle, 0, 0, - crtc->mode.hdisplay, - crtc->mode.vdisplay); - - if (event) { - spin_lock_irqsave(&crtc->dev->event_lock, irqflags); - drm_send_vblank_event(crtc->dev, -1, event); - spin_unlock_irqrestore(&crtc->dev->event_lock, irqflags); - } - - return 0; -} - static const struct drm_crtc_funcs virtio_gpu_crtc_funcs = { .cursor_set2 = virtio_gpu_crtc_cursor_set, .cursor_move = virtio_gpu_crtc_cursor_move, .set_config = drm_atomic_helper_set_config, .destroy = drm_crtc_cleanup,

- .page_flip = virtio_gpu_page_flip, + .page_flip = drm_atomic_helper_page_flip, .reset = drm_atomic_helper_crtc_reset, .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, @@ -267,6 +222,7 @@ static void virtio_gpu_crtc_atomic_flush(struct drm_crtc *crtc, spin_lock_irqsave(&crtc->dev->event_lock, flags); if (crtc->state->event) drm_crtc_send_vblank_event(crtc, crtc->state->event); + crtc->state->event = NULL; spin_unlock_irqrestore(&crtc->dev->event_lock, flags); }