Re: [Intel-gfx] [PATCH 1/2] drm/atomic: Change drm_atomic_helper_swap_state to return an error.

2017-07-03 Thread Daniel Vetter 
On Mon, Jul 03, 2017 at 01:01:55PM +0200, Maarten Lankhorst wrote:
> Op 30-06-17 om 15:56 schreef Daniel Vetter:
> > On Wed, Jun 28, 2017 at 03:28:11PM +0200, Maarten Lankhorst wrote:
> >> 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. All drivers have changes to deal with the
> >> clean up. In order to allow easy reverting, the commit that changes
> >> behavior is separate so someone only has to revert that for testing.
> >>
> >> Nouveau has a small bugfix, if drm_atomic_helper_wait_for_fences
> >> failed cleanup_planes was not called.
> >>
> >> Cc: Boris Brezillon 
> >> Cc: David Airlie 
> >> Cc: Daniel Vetter 
> >> Cc: Jani Nikula 
> >> Cc: Sean Paul 
> >> Cc: CK Hu 
> >> Cc: Philipp Zabel 
> >> Cc: Matthias Brugger 
> >> Cc: Rob Clark 
> >> Cc: Ben Skeggs 
> >> Cc: Thierry Reding 
> >> Cc: Jonathan Hunter 
> >> Cc: Jyri Sarha 
> >> Cc: Tomi Valkeinen 
> >> Cc: Eric Anholt 
> >> Cc: dri-de...@lists.freedesktop.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: intel-...@lists.freedesktop.org
> >> Cc: linux-arm-ker...@lists.infradead.org
> >> Cc: linux-media...@lists.infradead.org
> >> Cc: linux-arm-...@vger.kernel.org
> >> Cc: freedr...@lists.freedesktop.org
> >> Cc: nouv...@lists.freedesktop.org
> >> Cc: linux-te...@vger.kernel.org
> >> Signed-off-by: Maarten Lankhorst 
> > We kinda need to backport this to older kernels, but I don't really see
> > how :( Maybe we should split this up:
> > patch 1: Change to int return type
> > patches 2-(n-1): Driver conversions
> > patch n: __must_check addition
> >
> > That would at least somewhat make this backportable ...
> >
> >> ---
> >>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 14 --
> >>  drivers/gpu/drm/drm_atomic_helper.c  | 18 --
> >>  drivers/gpu/drm/i915/intel_display.c | 10 +-
> >>  drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  7 ++-
> >>  drivers/gpu/drm/msm/msm_atomic.c | 14 +-
> >>  drivers/gpu/drm/nouveau/nv50_display.c   | 10 --
> >>  drivers/gpu/drm/tegra/drm.c  |  7 ++-
> >>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  6 +-
> >>  drivers/gpu/drm/vc4/vc4_kms.c| 21 +
> >>  include/drm/drm_atomic_helper.h  |  4 ++--
> >>  10 files changed, 82 insertions(+), 29 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..d4f787bf1d4a 100644
> >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> >> @@ -544,9 +544,11 @@ static int atmel_hlcdc_dc_atomic_commit(struct 
> >> drm_device *dev,
> >>goto error;
> >>}
> >>  
> >> -  /* Swap the state, this is the point of no return. */
> >> -  drm_atomic_helper_swap_state(state, true);
> > Push the swap_state up over the commit setup (but after the allocation)
> > and there's no more a problem with unrolling.
> This can't be done higher up because of the interruptible wait.
> 
> Unless we change the patch series to move the waiting for hw_done to a 
> separate step and get rid of the stall argument to swap_state once everything 
> is converted. This could be useful for all drivers that have some kind of 
> setup, because we could move the wait up slightly to suit the drivers needs.

right, swap_state (well the swapping part, not the stalling part) must be
done as the last step and can't fail. Might be a reason do split them, but
not sure that's a good idea either. Please disregard my comment ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 1/2] drm/atomic: Change drm_atomic_helper_swap_state to return an error.

2017-07-03 Thread Daniel Vetter 
On Mon, Jul 03, 2017 at 01:01:55PM +0200, Maarten Lankhorst wrote:
> Op 30-06-17 om 15:56 schreef Daniel Vetter:
> > On Wed, Jun 28, 2017 at 03:28:11PM +0200, Maarten Lankhorst wrote:
> >> 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. All drivers have changes to deal with the
> >> clean up. In order to allow easy reverting, the commit that changes
> >> behavior is separate so someone only has to revert that for testing.
> >>
> >> Nouveau has a small bugfix, if drm_atomic_helper_wait_for_fences
> >> failed cleanup_planes was not called.
> >>
> >> Cc: Boris Brezillon 
> >> Cc: David Airlie 
> >> Cc: Daniel Vetter 
> >> Cc: Jani Nikula 
> >> Cc: Sean Paul 
> >> Cc: CK Hu 
> >> Cc: Philipp Zabel 
> >> Cc: Matthias Brugger 
> >> Cc: Rob Clark 
> >> Cc: Ben Skeggs 
> >> Cc: Thierry Reding 
> >> Cc: Jonathan Hunter 
> >> Cc: Jyri Sarha 
> >> Cc: Tomi Valkeinen 
> >> Cc: Eric Anholt 
> >> Cc: dri-de...@lists.freedesktop.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: intel-...@lists.freedesktop.org
> >> Cc: linux-arm-ker...@lists.infradead.org
> >> Cc: linux-media...@lists.infradead.org
> >> Cc: linux-arm-...@vger.kernel.org
> >> Cc: freedr...@lists.freedesktop.org
> >> Cc: nouv...@lists.freedesktop.org
> >> Cc: linux-te...@vger.kernel.org
> >> Signed-off-by: Maarten Lankhorst 
> > We kinda need to backport this to older kernels, but I don't really see
> > how :( Maybe we should split this up:
> > patch 1: Change to int return type
> > patches 2-(n-1): Driver conversions
> > patch n: __must_check addition
> >
> > That would at least somewhat make this backportable ...
> >
> >> ---
> >>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 14 --
> >>  drivers/gpu/drm/drm_atomic_helper.c  | 18 --
> >>  drivers/gpu/drm/i915/intel_display.c | 10 +-
> >>  drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  7 ++-
> >>  drivers/gpu/drm/msm/msm_atomic.c | 14 +-
> >>  drivers/gpu/drm/nouveau/nv50_display.c   | 10 --
> >>  drivers/gpu/drm/tegra/drm.c  |  7 ++-
> >>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  6 +-
> >>  drivers/gpu/drm/vc4/vc4_kms.c| 21 +
> >>  include/drm/drm_atomic_helper.h  |  4 ++--
> >>  10 files changed, 82 insertions(+), 29 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..d4f787bf1d4a 100644
> >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> >> @@ -544,9 +544,11 @@ static int atmel_hlcdc_dc_atomic_commit(struct 
> >> drm_device *dev,
> >>goto error;
> >>}
> >>  
> >> -  /* Swap the state, this is the point of no return. */
> >> -  drm_atomic_helper_swap_state(state, true);
> > Push the swap_state up over the commit setup (but after the allocation)
> > and there's no more a problem with unrolling.
> This can't be done higher up because of the interruptible wait.
> 
> Unless we change the patch series to move the waiting for hw_done to a 
> separate step and get rid of the stall argument to swap_state once everything 
> is converted. This could be useful for all drivers that have some kind of 
> setup, because we could move the wait up slightly to suit the drivers needs.

right, swap_state (well the swapping part, not the stalling part) must be
done as the last step and can't fail. Might be a reason do split them, but
not sure that's a good idea either. Please disregard my comment ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 1/2] drm/atomic: Change drm_atomic_helper_swap_state to return an error.

2017-07-03 Thread Maarten Lankhorst 
Op 30-06-17 om 15:56 schreef Daniel Vetter:
> On Wed, Jun 28, 2017 at 03:28:11PM +0200, Maarten Lankhorst wrote:
>> 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. All drivers have changes to deal with the
>> clean up. In order to allow easy reverting, the commit that changes
>> behavior is separate so someone only has to revert that for testing.
>>
>> Nouveau has a small bugfix, if drm_atomic_helper_wait_for_fences
>> failed cleanup_planes was not called.
>>
>> Cc: Boris Brezillon 
>> Cc: David Airlie 
>> Cc: Daniel Vetter 
>> Cc: Jani Nikula 
>> Cc: Sean Paul 
>> Cc: CK Hu 
>> Cc: Philipp Zabel 
>> Cc: Matthias Brugger 
>> Cc: Rob Clark 
>> Cc: Ben Skeggs 
>> Cc: Thierry Reding 
>> Cc: Jonathan Hunter 
>> Cc: Jyri Sarha 
>> Cc: Tomi Valkeinen 
>> Cc: Eric Anholt 
>> Cc: dri-de...@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: intel-...@lists.freedesktop.org
>> Cc: linux-arm-ker...@lists.infradead.org
>> Cc: linux-media...@lists.infradead.org
>> Cc: linux-arm-...@vger.kernel.org
>> Cc: freedr...@lists.freedesktop.org
>> Cc: nouv...@lists.freedesktop.org
>> Cc: linux-te...@vger.kernel.org
>> Signed-off-by: Maarten Lankhorst 
> We kinda need to backport this to older kernels, but I don't really see
> how :( Maybe we should split this up:
> patch 1: Change to int return type
> patches 2-(n-1): Driver conversions
> patch n: __must_check addition
>
> That would at least somewhat make this backportable ...
>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 14 --
>>  drivers/gpu/drm/drm_atomic_helper.c  | 18 --
>>  drivers/gpu/drm/i915/intel_display.c | 10 +-
>>  drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  7 ++-
>>  drivers/gpu/drm/msm/msm_atomic.c | 14 +-
>>  drivers/gpu/drm/nouveau/nv50_display.c   | 10 --
>>  drivers/gpu/drm/tegra/drm.c  |  7 ++-
>>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  6 +-
>>  drivers/gpu/drm/vc4/vc4_kms.c| 21 +
>>  include/drm/drm_atomic_helper.h  |  4 ++--
>>  10 files changed, 82 insertions(+), 29 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..d4f787bf1d4a 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> @@ -544,9 +544,11 @@ static int atmel_hlcdc_dc_atomic_commit(struct 
>> drm_device *dev,
>>  goto error;
>>  }
>>  
>> -/* Swap the state, this is the point of no return. */
>> -drm_atomic_helper_swap_state(state, true);
> Push the swap_state up over the commit setup (but after the allocation)
> and there's no more a problem with unrolling.
I think just like msm this might also use stall = false safely.
>> diff --git a/drivers/gpu/drm/msm/msm_atomic.c 
>> b/drivers/gpu/drm/msm/msm_atomic.c
>> index 9633a68b14d7..85dd485fdef4 100644
>> --- a/drivers/gpu/drm/msm/msm_atomic.c
>> +++ b/drivers/gpu/drm/msm/msm_atomic.c
>> @@ -232,8 +232,12 @@ 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)
>> +goto err_free;
>> +
>> +ret = drm_atomic_helper_swap_state(state, true);
> Hm, might be simpler to have stall = false (which btw makes your
> __must_check annotation a lie, you only have to check when you stall),
> since start_atomic above already does stall for everything as needed.
True, could we create a separate function for swap_state_and_wait, and kill the 
stall argument?
>>  if (ret) {
>> -kfree(c);
>> +commit_destroy(c);
>>  goto error;
>>  }
>>  
>> @@ -241,11 +245,9 @@ int msm_atomic_commit(struct drm_device *dev,
>>   * 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 +277,8 @@ int msm_atomic_commit(struct drm_device *dev,
>>  
>>  return 0;
>>

Re: [Intel-gfx] [PATCH 1/2] drm/atomic: Change drm_atomic_helper_swap_state to return an error.

2017-07-03 Thread Maarten Lankhorst 
Op 30-06-17 om 15:56 schreef Daniel Vetter:
> On Wed, Jun 28, 2017 at 03:28:11PM +0200, Maarten Lankhorst wrote:
>> 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. All drivers have changes to deal with the
>> clean up. In order to allow easy reverting, the commit that changes
>> behavior is separate so someone only has to revert that for testing.
>>
>> Nouveau has a small bugfix, if drm_atomic_helper_wait_for_fences
>> failed cleanup_planes was not called.
>>
>> Cc: Boris Brezillon 
>> Cc: David Airlie 
>> Cc: Daniel Vetter 
>> Cc: Jani Nikula 
>> Cc: Sean Paul 
>> Cc: CK Hu 
>> Cc: Philipp Zabel 
>> Cc: Matthias Brugger 
>> Cc: Rob Clark 
>> Cc: Ben Skeggs 
>> Cc: Thierry Reding 
>> Cc: Jonathan Hunter 
>> Cc: Jyri Sarha 
>> Cc: Tomi Valkeinen 
>> Cc: Eric Anholt 
>> Cc: dri-de...@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: intel-...@lists.freedesktop.org
>> Cc: linux-arm-ker...@lists.infradead.org
>> Cc: linux-media...@lists.infradead.org
>> Cc: linux-arm-...@vger.kernel.org
>> Cc: freedr...@lists.freedesktop.org
>> Cc: nouv...@lists.freedesktop.org
>> Cc: linux-te...@vger.kernel.org
>> Signed-off-by: Maarten Lankhorst 
> We kinda need to backport this to older kernels, but I don't really see
> how :( Maybe we should split this up:
> patch 1: Change to int return type
> patches 2-(n-1): Driver conversions
> patch n: __must_check addition
>
> That would at least somewhat make this backportable ...
>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 14 --
>>  drivers/gpu/drm/drm_atomic_helper.c  | 18 --
>>  drivers/gpu/drm/i915/intel_display.c | 10 +-
>>  drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  7 ++-
>>  drivers/gpu/drm/msm/msm_atomic.c | 14 +-
>>  drivers/gpu/drm/nouveau/nv50_display.c   | 10 --
>>  drivers/gpu/drm/tegra/drm.c  |  7 ++-
>>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  6 +-
>>  drivers/gpu/drm/vc4/vc4_kms.c| 21 +
>>  include/drm/drm_atomic_helper.h  |  4 ++--
>>  10 files changed, 82 insertions(+), 29 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..d4f787bf1d4a 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> @@ -544,9 +544,11 @@ static int atmel_hlcdc_dc_atomic_commit(struct 
>> drm_device *dev,
>>  goto error;
>>  }
>>  
>> -/* Swap the state, this is the point of no return. */
>> -drm_atomic_helper_swap_state(state, true);
> Push the swap_state up over the commit setup (but after the allocation)
> and there's no more a problem with unrolling.
I think just like msm this might also use stall = false safely.
>> diff --git a/drivers/gpu/drm/msm/msm_atomic.c 
>> b/drivers/gpu/drm/msm/msm_atomic.c
>> index 9633a68b14d7..85dd485fdef4 100644
>> --- a/drivers/gpu/drm/msm/msm_atomic.c
>> +++ b/drivers/gpu/drm/msm/msm_atomic.c
>> @@ -232,8 +232,12 @@ 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)
>> +goto err_free;
>> +
>> +ret = drm_atomic_helper_swap_state(state, true);
> Hm, might be simpler to have stall = false (which btw makes your
> __must_check annotation a lie, you only have to check when you stall),
> since start_atomic above already does stall for everything as needed.
True, could we create a separate function for swap_state_and_wait, and kill the 
stall argument?
>>  if (ret) {
>> -kfree(c);
>> +commit_destroy(c);
>>  goto error;
>>  }
>>  
>> @@ -241,11 +245,9 @@ int msm_atomic_commit(struct drm_device *dev,
>>   * 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 +277,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;
>> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c 
>> b/drivers/gpu/drm/nouveau/nv50_display.c
>> index 42a85c14aea0..fb2c763c88a8 100644
>> --- a/drivers/gpu/drm/nouveau/nv50_display.c
>> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
>> @@ -4119,9 +4119,13

Re: [Intel-gfx] [PATCH 1/2] drm/atomic: Change drm_atomic_helper_swap_state to return an error.

2017-07-03 Thread Maarten Lankhorst 
Op 30-06-17 om 15:56 schreef Daniel Vetter:
> On Wed, Jun 28, 2017 at 03:28:11PM +0200, Maarten Lankhorst wrote:
>> 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. All drivers have changes to deal with the
>> clean up. In order to allow easy reverting, the commit that changes
>> behavior is separate so someone only has to revert that for testing.
>>
>> Nouveau has a small bugfix, if drm_atomic_helper_wait_for_fences
>> failed cleanup_planes was not called.
>>
>> Cc: Boris Brezillon 
>> Cc: David Airlie 
>> Cc: Daniel Vetter 
>> Cc: Jani Nikula 
>> Cc: Sean Paul 
>> Cc: CK Hu 
>> Cc: Philipp Zabel 
>> Cc: Matthias Brugger 
>> Cc: Rob Clark 
>> Cc: Ben Skeggs 
>> Cc: Thierry Reding 
>> Cc: Jonathan Hunter 
>> Cc: Jyri Sarha 
>> Cc: Tomi Valkeinen 
>> Cc: Eric Anholt 
>> Cc: dri-de...@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: intel-...@lists.freedesktop.org
>> Cc: linux-arm-ker...@lists.infradead.org
>> Cc: linux-media...@lists.infradead.org
>> Cc: linux-arm-...@vger.kernel.org
>> Cc: freedr...@lists.freedesktop.org
>> Cc: nouv...@lists.freedesktop.org
>> Cc: linux-te...@vger.kernel.org
>> Signed-off-by: Maarten Lankhorst 
> We kinda need to backport this to older kernels, but I don't really see
> how :( Maybe we should split this up:
> patch 1: Change to int return type
> patches 2-(n-1): Driver conversions
> patch n: __must_check addition
>
> That would at least somewhat make this backportable ...
>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 14 --
>>  drivers/gpu/drm/drm_atomic_helper.c  | 18 --
>>  drivers/gpu/drm/i915/intel_display.c | 10 +-
>>  drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  7 ++-
>>  drivers/gpu/drm/msm/msm_atomic.c | 14 +-
>>  drivers/gpu/drm/nouveau/nv50_display.c   | 10 --
>>  drivers/gpu/drm/tegra/drm.c  |  7 ++-
>>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  6 +-
>>  drivers/gpu/drm/vc4/vc4_kms.c| 21 +
>>  include/drm/drm_atomic_helper.h  |  4 ++--
>>  10 files changed, 82 insertions(+), 29 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..d4f787bf1d4a 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> @@ -544,9 +544,11 @@ static int atmel_hlcdc_dc_atomic_commit(struct 
>> drm_device *dev,
>>  goto error;
>>  }
>>  
>> -/* Swap the state, this is the point of no return. */
>> -drm_atomic_helper_swap_state(state, true);
> Push the swap_state up over the commit setup (but after the allocation)
> and there's no more a problem with unrolling.
This can't be done higher up because of the interruptible wait.

Unless we change the patch series to move the waiting for hw_done to a separate 
step and get rid of the stall argument to swap_state once everything is 
converted. This could be useful for all drivers that have some kind of setup, 
because we could move the wait up slightly to suit the drivers needs.

~Maarten

Re: [Intel-gfx] [PATCH 1/2] drm/atomic: Change drm_atomic_helper_swap_state to return an error.

2017-07-03 Thread Maarten Lankhorst 
Op 30-06-17 om 15:56 schreef Daniel Vetter:
> On Wed, Jun 28, 2017 at 03:28:11PM +0200, Maarten Lankhorst wrote:
>> 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. All drivers have changes to deal with the
>> clean up. In order to allow easy reverting, the commit that changes
>> behavior is separate so someone only has to revert that for testing.
>>
>> Nouveau has a small bugfix, if drm_atomic_helper_wait_for_fences
>> failed cleanup_planes was not called.
>>
>> Cc: Boris Brezillon 
>> Cc: David Airlie 
>> Cc: Daniel Vetter 
>> Cc: Jani Nikula 
>> Cc: Sean Paul 
>> Cc: CK Hu 
>> Cc: Philipp Zabel 
>> Cc: Matthias Brugger 
>> Cc: Rob Clark 
>> Cc: Ben Skeggs 
>> Cc: Thierry Reding 
>> Cc: Jonathan Hunter 
>> Cc: Jyri Sarha 
>> Cc: Tomi Valkeinen 
>> Cc: Eric Anholt 
>> Cc: dri-de...@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: intel-...@lists.freedesktop.org
>> Cc: linux-arm-ker...@lists.infradead.org
>> Cc: linux-media...@lists.infradead.org
>> Cc: linux-arm-...@vger.kernel.org
>> Cc: freedr...@lists.freedesktop.org
>> Cc: nouv...@lists.freedesktop.org
>> Cc: linux-te...@vger.kernel.org
>> Signed-off-by: Maarten Lankhorst 
> We kinda need to backport this to older kernels, but I don't really see
> how :( Maybe we should split this up:
> patch 1: Change to int return type
> patches 2-(n-1): Driver conversions
> patch n: __must_check addition
>
> That would at least somewhat make this backportable ...
>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 14 --
>>  drivers/gpu/drm/drm_atomic_helper.c  | 18 --
>>  drivers/gpu/drm/i915/intel_display.c | 10 +-
>>  drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  7 ++-
>>  drivers/gpu/drm/msm/msm_atomic.c | 14 +-
>>  drivers/gpu/drm/nouveau/nv50_display.c   | 10 --
>>  drivers/gpu/drm/tegra/drm.c  |  7 ++-
>>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  6 +-
>>  drivers/gpu/drm/vc4/vc4_kms.c| 21 +
>>  include/drm/drm_atomic_helper.h  |  4 ++--
>>  10 files changed, 82 insertions(+), 29 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..d4f787bf1d4a 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> @@ -544,9 +544,11 @@ static int atmel_hlcdc_dc_atomic_commit(struct 
>> drm_device *dev,
>>  goto error;
>>  }
>>  
>> -/* Swap the state, this is the point of no return. */
>> -drm_atomic_helper_swap_state(state, true);
> Push the swap_state up over the commit setup (but after the allocation)
> and there's no more a problem with unrolling.
This can't be done higher up because of the interruptible wait.

Unless we change the patch series to move the waiting for hw_done to a separate 
step and get rid of the stall argument to swap_state once everything is 
converted. This could be useful for all drivers that have some kind of setup, 
because we could move the wait up slightly to suit the drivers needs.

~Maarten

Re: [Intel-gfx] [PATCH 1/2] drm/atomic: Change drm_atomic_helper_swap_state to return an error.

2017-06-30 Thread Daniel Vetter 
On Wed, Jun 28, 2017 at 03:28:11PM +0200, Maarten Lankhorst wrote:
> 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. All drivers have changes to deal with the
> clean up. In order to allow easy reverting, the commit that changes
> behavior is separate so someone only has to revert that for testing.
> 
> Nouveau has a small bugfix, if drm_atomic_helper_wait_for_fences
> failed cleanup_planes was not called.
> 
> Cc: Boris Brezillon 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: Jani Nikula 
> Cc: Sean Paul 
> Cc: CK Hu 
> Cc: Philipp Zabel 
> Cc: Matthias Brugger 
> Cc: Rob Clark 
> Cc: Ben Skeggs 
> Cc: Thierry Reding 
> Cc: Jonathan Hunter 
> Cc: Jyri Sarha 
> Cc: Tomi Valkeinen 
> Cc: Eric Anholt 
> Cc: dri-de...@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Cc: intel-...@lists.freedesktop.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-media...@lists.infradead.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Cc: linux-te...@vger.kernel.org
> Signed-off-by: Maarten Lankhorst 

We kinda need to backport this to older kernels, but I don't really see
how :( Maybe we should split this up:
patch 1: Change to int return type
patches 2-(n-1): Driver conversions
patch n: __must_check addition

That would at least somewhat make this backportable ...

> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 14 --
>  drivers/gpu/drm/drm_atomic_helper.c  | 18 --
>  drivers/gpu/drm/i915/intel_display.c | 10 +-
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  7 ++-
>  drivers/gpu/drm/msm/msm_atomic.c | 14 +-
>  drivers/gpu/drm/nouveau/nv50_display.c   | 10 --
>  drivers/gpu/drm/tegra/drm.c  |  7 ++-
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  6 +-
>  drivers/gpu/drm/vc4/vc4_kms.c| 21 +
>  include/drm/drm_atomic_helper.h  |  4 ++--
>  10 files changed, 82 insertions(+), 29 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..d4f787bf1d4a 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -544,9 +544,11 @@ static int atmel_hlcdc_dc_atomic_commit(struct 
> drm_device *dev,
>   goto error;
>   }
>  
> - /* Swap the state, this is the point of no return. */
> - drm_atomic_helper_swap_state(state, true);

Push the swap_state up over the commit setup (but after the allocation)
and there's no more a problem with unrolling.

> + ret = drm_atomic_helper_swap_state(state, true);
> + if (ret)
> + goto err_pending;
>  
> + /* Swap state succeeded, this is the point of no return. */
>   drm_atomic_state_get(state);
>   if (async)
>   queue_work(dc->wq, >work);
> @@ -555,6 +557,14 @@ static int atmel_hlcdc_dc_atomic_commit(struct 
> drm_device *dev,
>  
>   return 0;
>  
> +err_pending:
> + /* Fail the commit, wake up any waiter. */
> + spin_lock(>commit.wait.lock);
> + dc->commit.pending = false;
> + wake_up_all_locked(>commit.wait);
> + spin_unlock(>commit.wait.lock);
> +err_free:
> + kfree(commit);
>  error:
>   drm_atomic_helper_cleanup_planes(dev, state);
>   return ret;
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index f1847d29ba3e..f66b6c45cdd0 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1387,10 +1387,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;
>   }
>  
>   /*
> @@ -1399,7 +1397,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
> @@ -1428,6 +1428,10 @@ int drm_atomic_helper_commit(struct

Re: [Intel-gfx] [PATCH 1/2] drm/atomic: Change drm_atomic_helper_swap_state to return an error.

2017-06-30 Thread Daniel Vetter 
On Wed, Jun 28, 2017 at 03:28:11PM +0200, Maarten Lankhorst wrote:
> 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. All drivers have changes to deal with the
> clean up. In order to allow easy reverting, the commit that changes
> behavior is separate so someone only has to revert that for testing.
> 
> Nouveau has a small bugfix, if drm_atomic_helper_wait_for_fences
> failed cleanup_planes was not called.
> 
> Cc: Boris Brezillon 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: Jani Nikula 
> Cc: Sean Paul 
> Cc: CK Hu 
> Cc: Philipp Zabel 
> Cc: Matthias Brugger 
> Cc: Rob Clark 
> Cc: Ben Skeggs 
> Cc: Thierry Reding 
> Cc: Jonathan Hunter 
> Cc: Jyri Sarha 
> Cc: Tomi Valkeinen 
> Cc: Eric Anholt 
> Cc: dri-de...@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Cc: intel-...@lists.freedesktop.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-media...@lists.infradead.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Cc: linux-te...@vger.kernel.org
> Signed-off-by: Maarten Lankhorst 

We kinda need to backport this to older kernels, but I don't really see
how :( Maybe we should split this up:
patch 1: Change to int return type
patches 2-(n-1): Driver conversions
patch n: __must_check addition

That would at least somewhat make this backportable ...

> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 14 --
>  drivers/gpu/drm/drm_atomic_helper.c  | 18 --
>  drivers/gpu/drm/i915/intel_display.c | 10 +-
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  7 ++-
>  drivers/gpu/drm/msm/msm_atomic.c | 14 +-
>  drivers/gpu/drm/nouveau/nv50_display.c   | 10 --
>  drivers/gpu/drm/tegra/drm.c  |  7 ++-
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  6 +-
>  drivers/gpu/drm/vc4/vc4_kms.c| 21 +
>  include/drm/drm_atomic_helper.h  |  4 ++--
>  10 files changed, 82 insertions(+), 29 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..d4f787bf1d4a 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -544,9 +544,11 @@ static int atmel_hlcdc_dc_atomic_commit(struct 
> drm_device *dev,
>   goto error;
>   }
>  
> - /* Swap the state, this is the point of no return. */
> - drm_atomic_helper_swap_state(state, true);

Push the swap_state up over the commit setup (but after the allocation)
and there's no more a problem with unrolling.

> + ret = drm_atomic_helper_swap_state(state, true);
> + if (ret)
> + goto err_pending;
>  
> + /* Swap state succeeded, this is the point of no return. */
>   drm_atomic_state_get(state);
>   if (async)
>   queue_work(dc->wq, >work);
> @@ -555,6 +557,14 @@ static int atmel_hlcdc_dc_atomic_commit(struct 
> drm_device *dev,
>  
>   return 0;
>  
> +err_pending:
> + /* Fail the commit, wake up any waiter. */
> + spin_lock(>commit.wait.lock);
> + dc->commit.pending = false;
> + wake_up_all_locked(>commit.wait);
> + spin_unlock(>commit.wait.lock);
> +err_free:
> + kfree(commit);
>  error:
>   drm_atomic_helper_cleanup_planes(dev, state);
>   return ret;
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index f1847d29ba3e..f66b6c45cdd0 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1387,10 +1387,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;
>   }
>  
>   /*
> @@ -1399,7 +1397,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
> @@ -1428,6 +1428,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);
>  
> @@ -2137,7 +2141,7 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
>   * the current atomic helpers this is almost always the case, since the 
> helpers
>   *

[PATCH 1/2] drm/atomic: Change drm_atomic_helper_swap_state to return an error.

2017-06-28 Thread Maarten Lankhorst 
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. All drivers have changes to deal with the
clean up. In order to allow easy reverting, the commit that changes
behavior is separate so someone only has to revert that for testing.

Nouveau has a small bugfix, if drm_atomic_helper_wait_for_fences
failed cleanup_planes was not called.

Cc: Boris Brezillon 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Jani Nikula 
Cc: Sean Paul 
Cc: CK Hu 
Cc: Philipp Zabel 
Cc: Matthias Brugger 
Cc: Rob Clark 
Cc: Ben Skeggs 
Cc: Thierry Reding 
Cc: Jonathan Hunter 
Cc: Jyri Sarha 
Cc: Tomi Valkeinen 
Cc: Eric Anholt 
Cc: dri-de...@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: intel-...@lists.freedesktop.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-media...@lists.infradead.org
Cc: linux-arm-...@vger.kernel.org
Cc: freedr...@lists.freedesktop.org
Cc: nouv...@lists.freedesktop.org
Cc: linux-te...@vger.kernel.org
Signed-off-by: Maarten Lankhorst 
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 14 --
 drivers/gpu/drm/drm_atomic_helper.c  | 18 --
 drivers/gpu/drm/i915/intel_display.c | 10 +-
 drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  7 ++-
 drivers/gpu/drm/msm/msm_atomic.c | 14 +-
 drivers/gpu/drm/nouveau/nv50_display.c   | 10 --
 drivers/gpu/drm/tegra/drm.c  |  7 ++-
 drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  6 +-
 drivers/gpu/drm/vc4/vc4_kms.c| 21 +
 include/drm/drm_atomic_helper.h  |  4 ++--
 10 files changed, 82 insertions(+), 29 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..d4f787bf1d4a 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -544,9 +544,11 @@ static int atmel_hlcdc_dc_atomic_commit(struct drm_device 
*dev,
goto error;
}
 
-   /* Swap the state, this is the point of no return. */
-   drm_atomic_helper_swap_state(state, true);
+   ret = drm_atomic_helper_swap_state(state, true);
+   if (ret)
+   goto err_pending;
 
+   /* Swap state succeeded, this is the point of no return. */
drm_atomic_state_get(state);
if (async)
queue_work(dc->wq, >work);
@@ -555,6 +557,14 @@ static int atmel_hlcdc_dc_atomic_commit(struct drm_device 
*dev,
 
return 0;
 
+err_pending:
+   /* Fail the commit, wake up any waiter. */
+   spin_lock(>commit.wait.lock);
+   dc->commit.pending = false;
+   wake_up_all_locked(>commit.wait);
+   spin_unlock(>commit.wait.lock);
+err_free:
+   kfree(commit);
 error:
drm_atomic_helper_cleanup_planes(dev, state);
return ret;
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index f1847d29ba3e..f66b6c45cdd0 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1387,10 +1387,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;
}
 
/*
@@ -1399,7 +1397,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
@@ -1428,6 +1428,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);
 
@@ -2137,7 +2141,7 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
  * 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,
+int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
  bool stall)
 {
int

[PATCH 1/2] drm/atomic: Change drm_atomic_helper_swap_state to return an error.

2017-06-28 Thread Maarten Lankhorst 
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. All drivers have changes to deal with the
clean up. In order to allow easy reverting, the commit that changes
behavior is separate so someone only has to revert that for testing.

Nouveau has a small bugfix, if drm_atomic_helper_wait_for_fences
failed cleanup_planes was not called.

Cc: Boris Brezillon 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Jani Nikula 
Cc: Sean Paul 
Cc: CK Hu 
Cc: Philipp Zabel 
Cc: Matthias Brugger 
Cc: Rob Clark 
Cc: Ben Skeggs 
Cc: Thierry Reding 
Cc: Jonathan Hunter 
Cc: Jyri Sarha 
Cc: Tomi Valkeinen 
Cc: Eric Anholt 
Cc: dri-de...@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: intel-...@lists.freedesktop.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-media...@lists.infradead.org
Cc: linux-arm-...@vger.kernel.org
Cc: freedr...@lists.freedesktop.org
Cc: nouv...@lists.freedesktop.org
Cc: linux-te...@vger.kernel.org
Signed-off-by: Maarten Lankhorst 
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 14 --
 drivers/gpu/drm/drm_atomic_helper.c  | 18 --
 drivers/gpu/drm/i915/intel_display.c | 10 +-
 drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  7 ++-
 drivers/gpu/drm/msm/msm_atomic.c | 14 +-
 drivers/gpu/drm/nouveau/nv50_display.c   | 10 --
 drivers/gpu/drm/tegra/drm.c  |  7 ++-
 drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  6 +-
 drivers/gpu/drm/vc4/vc4_kms.c| 21 +
 include/drm/drm_atomic_helper.h  |  4 ++--
 10 files changed, 82 insertions(+), 29 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..d4f787bf1d4a 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -544,9 +544,11 @@ static int atmel_hlcdc_dc_atomic_commit(struct drm_device 
*dev,
goto error;
}
 
-   /* Swap the state, this is the point of no return. */
-   drm_atomic_helper_swap_state(state, true);
+   ret = drm_atomic_helper_swap_state(state, true);
+   if (ret)
+   goto err_pending;
 
+   /* Swap state succeeded, this is the point of no return. */
drm_atomic_state_get(state);
if (async)
queue_work(dc->wq, >work);
@@ -555,6 +557,14 @@ static int atmel_hlcdc_dc_atomic_commit(struct drm_device 
*dev,
 
return 0;
 
+err_pending:
+   /* Fail the commit, wake up any waiter. */
+   spin_lock(>commit.wait.lock);
+   dc->commit.pending = false;
+   wake_up_all_locked(>commit.wait);
+   spin_unlock(>commit.wait.lock);
+err_free:
+   kfree(commit);
 error:
drm_atomic_helper_cleanup_planes(dev, state);
return ret;
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index f1847d29ba3e..f66b6c45cdd0 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1387,10 +1387,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;
}
 
/*
@@ -1399,7 +1397,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
@@ -1428,6 +1428,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);
 
@@ -2137,7 +2141,7 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
  * 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,
+int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
  bool stall)
 {
int i;
@@ -2214,6 +2218,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, >private_objs[i].obj_state);
+
+   return 0;
 }
 EXPORT_SYMBOL(drm_atomic_helper_swap_state);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c