Make it more clear that post commit return ret is really return 0,

and add a missing drm_atomic_helper_cleanup_planes when drm_atomic_helper_wait_for_fences fails.

Fixes: 839ca903f12e ("drm/nouveau/kms/nv50: transition to atomic interfaces internally") Cc: Ben Skeggs bskeggs@redhat.com Cc: dri-devel@lists.freedesktop.org Cc: nouveau@lists.freedesktop.org Cc: stable@vger.kernel.org # v4.10+ Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/nouveau/nv50_display.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c index 42a85c14aea0..8fb55b96c40f 100644 --- a/drivers/gpu/drm/nouveau/nv50_display.c +++ b/drivers/gpu/drm/nouveau/nv50_display.c @@ -4119,7 +4119,7 @@ nv50_disp_atomic_commit(struct drm_device *dev, if (!nonblock) { ret = drm_atomic_helper_wait_for_fences(dev, state, true); if (ret) - goto done; + goto err_cleanup; }

for_each_plane_in_state(state, plane, plane_state, i) { @@ -4147,7 +4147,7 @@ nv50_disp_atomic_commit(struct drm_device *dev, if (crtc->state->enable) { if (!drm->have_disp_power_ref) { drm->have_disp_power_ref = true; - return ret; + return 0; } active = true; break; @@ -4158,7 +4158,10 @@ nv50_disp_atomic_commit(struct drm_device *dev, pm_runtime_put_autosuspend(dev->dev); drm->have_disp_power_ref = false; } + goto done;

+err_cleanup: + drm_atomic_helper_cleanup_planes(dev, state); done: pm_runtime_put_autosuspend(dev->dev); return ret;

We want to change swap_state to wait indefinitely, but to do this swap_state should wait interruptibly. This requires propagating the error to each driver.

Cc: dri-devel@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org Cc: intel-gfx@lists.freedesktop.org Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 22 ++++++++++++++++------ include/drm/drm_atomic_helper.h | 3 +-- 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 667ec97d4efb..bfb98fbd0e0e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1510,10 +1510,8 @@ int drm_atomic_helper_commit(struct drm_device *dev,

if (!nonblock) { ret = drm_atomic_helper_wait_for_fences(dev, state, true); - if (ret) { - drm_atomic_helper_cleanup_planes(dev, state); - return ret; - } + if (ret) + goto err; }

/* @@ -1522,7 +1520,9 @@ int drm_atomic_helper_commit(struct drm_device *dev, * the software side now. */

- drm_atomic_helper_swap_state(state, true); + ret = drm_atomic_helper_swap_state(state, true); + if (ret) + goto err;

/* * Everything below can be run asynchronously without the need to grab @@ -1551,6 +1551,10 @@ int drm_atomic_helper_commit(struct drm_device *dev, commit_tail(state);

return 0; + +err: + drm_atomic_helper_cleanup_planes(dev, state); + return ret; } EXPORT_SYMBOL(drm_atomic_helper_commit);

@@ -2254,8 +2258,12 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes); * the &drm_plane.state, &drm_crtc.state or &drm_connector.state pointer. With * the current atomic helpers this is almost always the case, since the helpers * don't pass the right state structures to the callbacks. + * + * Returns: + * + * Always returns 0, cannot fail yet. */ -void drm_atomic_helper_swap_state(struct drm_atomic_state *state, +int drm_atomic_helper_swap_state(struct drm_atomic_state *state, bool stall) { int i; @@ -2332,6 +2340,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,

__for_each_private_obj(state, obj, obj_state, i, funcs) funcs->swap_state(obj, &state->private_objs[i].obj_state); + + return 0; } EXPORT_SYMBOL(drm_atomic_helper_swap_state);

diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index dd196cc0afd7..37c9534ff691 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -84,8 +84,7 @@ void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc_state *old_crtc_state, bool atomic);

-void drm_atomic_helper_swap_state(struct drm_atomic_state *state, - bool stall); +int 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,

drm_atomic_helper_swap_state() will be changed to interruptible waiting in the next few commits, so all drivers have to be changed to handling failure.

Atmel tracks pending commits through dc->commit.pending, so it can ignore the changes by setting stall = false. We never return failure in this case, so make failure a BUG_ON.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Boris Brezillon boris.brezillon@free-electrons.com --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c index 516d9547d331..64f54dc7dd68 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c @@ -539,14 +539,13 @@ static int atmel_hlcdc_dc_atomic_commit(struct drm_device *dev, dc->commit.pending = true; spin_unlock(&dc->commit.wait.lock);

- if (ret) { - kfree(commit); - goto error; - } + if (ret) + goto err_free;

- /* Swap the state, this is the point of no return. */ - drm_atomic_helper_swap_state(state, true); + /* We have our own synchronization through the commit lock. */ + BUG_ON(drm_atomic_helper_swap_state(state, false) < 0);

+ /* Swap state succeeded, this is the point of no return. */ drm_atomic_state_get(state); if (async) queue_work(dc->wq, &commit->work); @@ -555,6 +554,8 @@ static int atmel_hlcdc_dc_atomic_commit(struct drm_device *dev,

return 0;

+err_free: + kfree(commit); error: drm_atomic_helper_cleanup_planes(dev, state); return ret;

drm_atomic_helper_swap_state() will be changed to interruptible waiting in the next few commits, so all drivers have to be changed to handling failure.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: CK Hu ck.hu@mediatek.com Cc: Philipp Zabel p.zabel@pengutronix.de Cc: linux-arm-kernel@lists.infradead.org Cc: linux-mediatek@lists.infradead.org --- drivers/gpu/drm/mediatek/mtk_drm_drv.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index 56f802d0a51c..9a130c84c861 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c @@ -109,7 +109,12 @@ static int mtk_atomic_commit(struct drm_device *drm, mutex_lock(&private->commit.lock); flush_work(&private->commit.work);

- drm_atomic_helper_swap_state(state, true); + ret = drm_atomic_helper_swap_state(state, true); + if (ret) { + mutex_unlock(&private->commit.lock); + drm_atomic_helper_cleanup_planes(drm, state); + return ret; + }

drm_atomic_state_get(state); if (async)

drm_atomic_helper_swap_state() will be changed to interruptible waiting in the next few commits, so all drivers have to be changed to handling failure.

MSM has its own busy tracking, which means the swap_state call can be done with stall = false, in which case it should never return an error. Handle failure with BUG_ON for this reason.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Rob Clark robdclark@gmail.com Cc: linux-arm-msm@vger.kernel.org Cc: freedreno@lists.freedesktop.org --- drivers/gpu/drm/msm/msm_atomic.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index 9633a68b14d7..badfa8717317 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -232,20 +232,18 @@ int msm_atomic_commit(struct drm_device *dev, * mark our set of crtc's as busy: */ ret = start_atomic(dev->dev_private, c->crtc_mask); - if (ret) { - kfree(c); - goto error; - } + if (ret) + goto err_free; + + BUG_ON(drm_atomic_helper_swap_state(state, false) < 0);

/* * This is the point of no return - everything below never fails except * when the hw goes bonghits. Which means we can commit the new state on * the software side now. + * + * swap driver private state while still holding state_lock */ - - drm_atomic_helper_swap_state(state, true); - - /* swap driver private state while still holding state_lock */ if (to_kms_state(state)->state) priv->kms->funcs->swap_state(priv->kms, state);

@@ -275,6 +273,8 @@ int msm_atomic_commit(struct drm_device *dev,

return 0;

+err_free: + kfree(c); error: drm_atomic_helper_cleanup_planes(dev, state); return ret;

drm_atomic_helper_swap_state() will be changed to interruptible waiting in the next few commits, so all drivers have to be changed to handling failure.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Thierry Reding thierry.reding@gmail.com Cc: Jonathan Hunter jonathanh@nvidia.com Cc: linux-tegra@vger.kernel.org --- drivers/gpu/drm/tegra/drm.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index ad3d124a972d..3ba659a5940d 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -100,7 +100,12 @@ static int tegra_atomic_commit(struct drm_device *drm, * the software side now. */

- drm_atomic_helper_swap_state(state, true); + err = drm_atomic_helper_swap_state(state, true); + if (err) { + mutex_unlock(&tegra->commit.lock); + drm_atomic_helper_cleanup_planes(drm, state); + return err; + }

drm_atomic_state_get(state); if (nonblock)

drm_atomic_helper_swap_state() will be changed to interruptible waiting in the next few commits, so all drivers have to be changed to handling failure.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Jyri Sarha jsarha@ti.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index d67e18983a7d..049d2f5a1ee4 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -108,7 +108,11 @@ static int tilcdc_commit(struct drm_device *dev, if (ret) return ret;

- drm_atomic_helper_swap_state(state, true); + ret = drm_atomic_helper_swap_state(state, true); + if (ret) { + drm_atomic_helper_cleanup_planes(dev, state); + return ret; + }

/* * Everything below can be run asynchronously without the need to grab