Re: [Freedreno] [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-ker...@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: freedreno@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
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [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-ker...@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: freedreno@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: [Freedreno] [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-ker...@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: freedreno@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