From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [Intel-gfx] [PATCH v2 07/12] drm/msm: Handle drm_atomic_helper_swap_state failure Date: Fri, 14 Jul 2017 17:02:15 +0200 Message-ID: <20170714150215.sghkd4bagx4nqwzm@phenom.ffwll.local> References: <20170711143314.2148-1-maarten.lankhorst@linux.intel.com> <20170711143314.2148-8-maarten.lankhorst@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mail-wm0-f65.google.com ([74.125.82.65]:34168 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754031AbdGNPCU (ORCPT ); Fri, 14 Jul 2017 11:02:20 -0400 Received: by mail-wm0-f65.google.com with SMTP id p204so11307560wmg.1 for ; Fri, 14 Jul 2017 08:02:19 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20170711143314.2148-8-maarten.lankhorst@linux.intel.com> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Maarten Lankhorst Cc: dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, intel-gfx@lists.freedesktop.org, freedreno@lists.freedesktop.org On Tue, Jul 11, 2017 at 04:33:09PM +0200, Maarten Lankhorst wrote: > 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 > Cc: Rob Clark > 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); Hm, not sure we want to do this, makes switching msm to the nonblocking helpers a bit more tricky. And the got err_free thing looks like leftovers from an old version. But it's all correct. Reviewed-by: Daniel Vetter > > /* > * 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; > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch