Re: [Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state

2016-11-16 Thread Maarten Lankhorst
Op 03-11-16 om 16:11 schreef Ville Syrjälä:
> On Wed, Nov 02, 2016 at 09:28:46AM +0100, Maarten Lankhorst wrote:
>> Op 01-11-16 om 14:41 schreef Ville Syrjälä:
>>> On Tue, Nov 01, 2016 at 02:34:00PM +0100, Maarten Lankhorst wrote:
 Op 01-11-16 om 14:09 schreef Ville Syrjälä:
> On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote:
>> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
>> replace the old for_each_xxx_in_state ones. This is useful for >1 flip
>> depth and getting rid of all xxx->state dereferences.
>>
>> Signed-off-by: Maarten Lankhorst 
>> ---
>>  drivers/gpu/drm/drm_atomic.c |  6 +++
>>  drivers/gpu/drm/drm_atomic_helper.c  | 52 +--
>>  drivers/gpu/drm/i915/intel_display.c | 11 ++---
>>  include/drm/drm_atomic.h | 81 
>> ++--
>>  include/drm/drm_atomic_helper.h  |  3 ++
>>  5 files changed, 142 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 5dd70540219c..120775fcfb70 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state 
>> *state,
>>  return ERR_PTR(-ENOMEM);
>>  
>>  state->crtcs[index].state = crtc_state;
>> +state->crtcs[index].old_state = crtc->state;
>> +state->crtcs[index].new_state = crtc_state;
>>  state->crtcs[index].ptr = crtc;
>>  crtc_state->state = state;
>>  
>> @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state 
>> *state,
>>  
>>  state->planes[index].state = plane_state;
>>  state->planes[index].ptr = plane;
>> +state->planes[index].old_state = plane->state;
>> +state->planes[index].new_state = plane_state;
>>  plane_state->state = state;
>>  
>>  DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
>> @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct 
>> drm_atomic_state *state,
>>  
>>  drm_connector_reference(connector);
>>  state->connectors[index].state = connector_state;
>> +state->connectors[index].old_state = connector->state;
>> +state->connectors[index].new_state = connector_state;
>>  state->connectors[index].ptr = connector;
>>  connector_state->state = state;
>>  
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
>> b/drivers/gpu/drm/drm_atomic_helper.c
>> index 07b432f43b98..fcb6e5b55217 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
>>   *
>>   * See also:
>>   * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
>> - * drm_atomic_helper_resume()
>> + * drm_atomic_helper_resume(), 
>> drm_atomic_helper_commit_duplicated_state()
>>   */
>>  struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device 
>> *dev)
>>  {
>> @@ -2536,6 +2536,52 @@ unlock:
>>  EXPORT_SYMBOL(drm_atomic_helper_suspend);
>>  
>>  /**
>> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state
>> + * @state: duplicated atomic state to commit
>> + * @ctx: pointer to acquire_ctx to use for commit.
>> + * @nonblock: commit the state non-blocking.
>> + *
>> + * The state returned by drm_atomic_helper_duplicate_state() and
>> + * drm_atomic_helper_suspend() is partially invalid, and needs to
>> + * be fixed up before commit.
> So the old_state pointers are potentially invalid because whatever->state
> may have changed since we duplicated the state?
 Yes when you use drm_atomic_helper_duplicate_state, and commit a different 
 state before committing the duplicated state.
 This only happens during suspend/resume and gpu reset.
>>> Should we maybe have something like drm_atomic_refresh_old_state(state)?
>>> Would allow the caller then to check something in the old vs. new state
>>> before committing?
>> iirc that was the first approach I took, but then you shouldn't do anything 
>> with a duplicated state after
>> creating a except commit it, so creating a commit_duplicated_state should be 
>> enough.
>>
>> Can you think of any case where you do the following?
>>
>> Duplicate state
>> commit different state
>> Look at duplicated state, tinker with it, commit it.
> Not really. Oh, and we do still run the thing through the check phase
> when we commit it, don't we? That sounds like it should let the driver
> do whatever it needs to do.
> So my only other concern is just the 'bool nonblock' parameter. It's a
> bit 

Re: [Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state

2016-11-08 Thread Daniel Vetter
On Tue, Nov 01, 2016 at 02:34:00PM +0100, Maarten Lankhorst wrote:
> Op 01-11-16 om 14:09 schreef Ville Syrjälä:
> > On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote:
> >> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
> >> replace the old for_each_xxx_in_state ones. This is useful for >1 flip
> >> depth and getting rid of all xxx->state dereferences.
> >>
> >> Signed-off-by: Maarten Lankhorst 
> >> ---
> >>  drivers/gpu/drm/drm_atomic.c |  6 +++
> >>  drivers/gpu/drm/drm_atomic_helper.c  | 52 +--
> >>  drivers/gpu/drm/i915/intel_display.c | 11 ++---
> >>  include/drm/drm_atomic.h | 81 
> >> ++--
> >>  include/drm/drm_atomic_helper.h  |  3 ++
> >>  5 files changed, 142 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 5dd70540219c..120775fcfb70 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state 
> >> *state,
> >>return ERR_PTR(-ENOMEM);
> >>  
> >>state->crtcs[index].state = crtc_state;
> >> +  state->crtcs[index].old_state = crtc->state;
> >> +  state->crtcs[index].new_state = crtc_state;
> >>state->crtcs[index].ptr = crtc;
> >>crtc_state->state = state;
> >>  
> >> @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state 
> >> *state,
> >>  
> >>state->planes[index].state = plane_state;
> >>state->planes[index].ptr = plane;
> >> +  state->planes[index].old_state = plane->state;
> >> +  state->planes[index].new_state = plane_state;
> >>plane_state->state = state;
> >>  
> >>DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
> >> @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state 
> >> *state,
> >>  
> >>drm_connector_reference(connector);
> >>state->connectors[index].state = connector_state;
> >> +  state->connectors[index].old_state = connector->state;
> >> +  state->connectors[index].new_state = connector_state;
> >>state->connectors[index].ptr = connector;
> >>connector_state->state = state;
> >>  
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> >> b/drivers/gpu/drm/drm_atomic_helper.c
> >> index 07b432f43b98..fcb6e5b55217 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
> >>   *
> >>   * See also:
> >>   * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
> >> - * drm_atomic_helper_resume()
> >> + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state()
> >>   */
> >>  struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
> >>  {
> >> @@ -2536,6 +2536,52 @@ unlock:
> >>  EXPORT_SYMBOL(drm_atomic_helper_suspend);
> >>  
> >>  /**
> >> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state
> >> + * @state: duplicated atomic state to commit
> >> + * @ctx: pointer to acquire_ctx to use for commit.
> >> + * @nonblock: commit the state non-blocking.
> >> + *
> >> + * The state returned by drm_atomic_helper_duplicate_state() and
> >> + * drm_atomic_helper_suspend() is partially invalid, and needs to
> >> + * be fixed up before commit.
> > So the old_state pointers are potentially invalid because whatever->state
> > may have changed since we duplicated the state?
> 
> Yes when you use drm_atomic_helper_duplicate_state, and commit a different 
> state before committing the duplicated state.
> This only happens during suspend/resume and gpu reset.

Kerneldoc should imo mention that suspend/resume is the only case this is
valid, and even then it depends upon the driver. Proper atomic commits
never keep drm_atomic_state around when dropping locks, so this can't
happen with an atomic ioctl. Please also directly cross-reference all
these functions.
-Daniel

> 
> ~Maarten
> 
> ___
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state

2016-11-03 Thread Ville Syrjälä
On Wed, Nov 02, 2016 at 09:28:46AM +0100, Maarten Lankhorst wrote:
> Op 01-11-16 om 14:41 schreef Ville Syrjälä:
> > On Tue, Nov 01, 2016 at 02:34:00PM +0100, Maarten Lankhorst wrote:
> >> Op 01-11-16 om 14:09 schreef Ville Syrjälä:
> >>> On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote:
>  Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
>  replace the old for_each_xxx_in_state ones. This is useful for >1 flip
>  depth and getting rid of all xxx->state dereferences.
> 
>  Signed-off-by: Maarten Lankhorst 
>  ---
>   drivers/gpu/drm/drm_atomic.c |  6 +++
>   drivers/gpu/drm/drm_atomic_helper.c  | 52 +--
>   drivers/gpu/drm/i915/intel_display.c | 11 ++---
>   include/drm/drm_atomic.h | 81 
>  ++--
>   include/drm/drm_atomic_helper.h  |  3 ++
>   5 files changed, 142 insertions(+), 11 deletions(-)
> 
>  diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>  index 5dd70540219c..120775fcfb70 100644
>  --- a/drivers/gpu/drm/drm_atomic.c
>  +++ b/drivers/gpu/drm/drm_atomic.c
>  @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state 
>  *state,
>   return ERR_PTR(-ENOMEM);
>   
>   state->crtcs[index].state = crtc_state;
>  +state->crtcs[index].old_state = crtc->state;
>  +state->crtcs[index].new_state = crtc_state;
>   state->crtcs[index].ptr = crtc;
>   crtc_state->state = state;
>   
>  @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state 
>  *state,
>   
>   state->planes[index].state = plane_state;
>   state->planes[index].ptr = plane;
>  +state->planes[index].old_state = plane->state;
>  +state->planes[index].new_state = plane_state;
>   plane_state->state = state;
>   
>   DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
>  @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct 
>  drm_atomic_state *state,
>   
>   drm_connector_reference(connector);
>   state->connectors[index].state = connector_state;
>  +state->connectors[index].old_state = connector->state;
>  +state->connectors[index].new_state = connector_state;
>   state->connectors[index].ptr = connector;
>   connector_state->state = state;
>   
>  diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
>  b/drivers/gpu/drm/drm_atomic_helper.c
>  index 07b432f43b98..fcb6e5b55217 100644
>  --- a/drivers/gpu/drm/drm_atomic_helper.c
>  +++ b/drivers/gpu/drm/drm_atomic_helper.c
>  @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
>    *
>    * See also:
>    * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
>  - * drm_atomic_helper_resume()
>  + * drm_atomic_helper_resume(), 
>  drm_atomic_helper_commit_duplicated_state()
>    */
>   struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device 
>  *dev)
>   {
>  @@ -2536,6 +2536,52 @@ unlock:
>   EXPORT_SYMBOL(drm_atomic_helper_suspend);
>   
>   /**
>  + * drm_atomic_helper_commit_duplicated_state - commit duplicated state
>  + * @state: duplicated atomic state to commit
>  + * @ctx: pointer to acquire_ctx to use for commit.
>  + * @nonblock: commit the state non-blocking.
>  + *
>  + * The state returned by drm_atomic_helper_duplicate_state() and
>  + * drm_atomic_helper_suspend() is partially invalid, and needs to
>  + * be fixed up before commit.
> >>> So the old_state pointers are potentially invalid because whatever->state
> >>> may have changed since we duplicated the state?
> >> Yes when you use drm_atomic_helper_duplicate_state, and commit a different 
> >> state before committing the duplicated state.
> >> This only happens during suspend/resume and gpu reset.
> > Should we maybe have something like drm_atomic_refresh_old_state(state)?
> > Would allow the caller then to check something in the old vs. new state
> > before committing?
> 
> iirc that was the first approach I took, but then you shouldn't do anything 
> with a duplicated state after
> creating a except commit it, so creating a commit_duplicated_state should be 
> enough.
> 
> Can you think of any case where you do the following?
> 
> Duplicate state
> commit different state
> Look at duplicated state, tinker with it, commit it.

Not really. Oh, and we do still run the thing through the check phase
when we commit it, don't we? That sounds like it should let the driver
do whatever it needs to do.

So my only other concern is just the 'bool nonblock' parameter. It's a
bit funny looking that we pass that in, then branch off to the 

Re: [Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state

2016-11-02 Thread Maarten Lankhorst
Op 01-11-16 om 14:41 schreef Ville Syrjälä:
> On Tue, Nov 01, 2016 at 02:34:00PM +0100, Maarten Lankhorst wrote:
>> Op 01-11-16 om 14:09 schreef Ville Syrjälä:
>>> On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote:
 Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
 replace the old for_each_xxx_in_state ones. This is useful for >1 flip
 depth and getting rid of all xxx->state dereferences.

 Signed-off-by: Maarten Lankhorst 
 ---
  drivers/gpu/drm/drm_atomic.c |  6 +++
  drivers/gpu/drm/drm_atomic_helper.c  | 52 +--
  drivers/gpu/drm/i915/intel_display.c | 11 ++---
  include/drm/drm_atomic.h | 81 
 ++--
  include/drm/drm_atomic_helper.h  |  3 ++
  5 files changed, 142 insertions(+), 11 deletions(-)

 diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
 index 5dd70540219c..120775fcfb70 100644
 --- a/drivers/gpu/drm/drm_atomic.c
 +++ b/drivers/gpu/drm/drm_atomic.c
 @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state 
 *state,
return ERR_PTR(-ENOMEM);
  
state->crtcs[index].state = crtc_state;
 +  state->crtcs[index].old_state = crtc->state;
 +  state->crtcs[index].new_state = crtc_state;
state->crtcs[index].ptr = crtc;
crtc_state->state = state;
  
 @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state 
 *state,
  
state->planes[index].state = plane_state;
state->planes[index].ptr = plane;
 +  state->planes[index].old_state = plane->state;
 +  state->planes[index].new_state = plane_state;
plane_state->state = state;
  
DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
 @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state 
 *state,
  
drm_connector_reference(connector);
state->connectors[index].state = connector_state;
 +  state->connectors[index].old_state = connector->state;
 +  state->connectors[index].new_state = connector_state;
state->connectors[index].ptr = connector;
connector_state->state = state;
  
 diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
 b/drivers/gpu/drm/drm_atomic_helper.c
 index 07b432f43b98..fcb6e5b55217 100644
 --- a/drivers/gpu/drm/drm_atomic_helper.c
 +++ b/drivers/gpu/drm/drm_atomic_helper.c
 @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
   *
   * See also:
   * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
 - * drm_atomic_helper_resume()
 + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state()
   */
  struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
  {
 @@ -2536,6 +2536,52 @@ unlock:
  EXPORT_SYMBOL(drm_atomic_helper_suspend);
  
  /**
 + * drm_atomic_helper_commit_duplicated_state - commit duplicated state
 + * @state: duplicated atomic state to commit
 + * @ctx: pointer to acquire_ctx to use for commit.
 + * @nonblock: commit the state non-blocking.
 + *
 + * The state returned by drm_atomic_helper_duplicate_state() and
 + * drm_atomic_helper_suspend() is partially invalid, and needs to
 + * be fixed up before commit.
>>> So the old_state pointers are potentially invalid because whatever->state
>>> may have changed since we duplicated the state?
>> Yes when you use drm_atomic_helper_duplicate_state, and commit a different 
>> state before committing the duplicated state.
>> This only happens during suspend/resume and gpu reset.
> Should we maybe have something like drm_atomic_refresh_old_state(state)?
> Would allow the caller then to check something in the old vs. new state
> before committing?

iirc that was the first approach I took, but then you shouldn't do anything 
with a duplicated state after
creating a except commit it, so creating a commit_duplicated_state should be 
enough.

Can you think of any case where you do the following?

Duplicate state
commit different state
Look at duplicated state, tinker with it, commit it.

~Maarten

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state

2016-11-01 Thread Ville Syrjälä
On Tue, Nov 01, 2016 at 02:34:00PM +0100, Maarten Lankhorst wrote:
> Op 01-11-16 om 14:09 schreef Ville Syrjälä:
> > On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote:
> >> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
> >> replace the old for_each_xxx_in_state ones. This is useful for >1 flip
> >> depth and getting rid of all xxx->state dereferences.
> >>
> >> Signed-off-by: Maarten Lankhorst 
> >> ---
> >>  drivers/gpu/drm/drm_atomic.c |  6 +++
> >>  drivers/gpu/drm/drm_atomic_helper.c  | 52 +--
> >>  drivers/gpu/drm/i915/intel_display.c | 11 ++---
> >>  include/drm/drm_atomic.h | 81 
> >> ++--
> >>  include/drm/drm_atomic_helper.h  |  3 ++
> >>  5 files changed, 142 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 5dd70540219c..120775fcfb70 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state 
> >> *state,
> >>return ERR_PTR(-ENOMEM);
> >>  
> >>state->crtcs[index].state = crtc_state;
> >> +  state->crtcs[index].old_state = crtc->state;
> >> +  state->crtcs[index].new_state = crtc_state;
> >>state->crtcs[index].ptr = crtc;
> >>crtc_state->state = state;
> >>  
> >> @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state 
> >> *state,
> >>  
> >>state->planes[index].state = plane_state;
> >>state->planes[index].ptr = plane;
> >> +  state->planes[index].old_state = plane->state;
> >> +  state->planes[index].new_state = plane_state;
> >>plane_state->state = state;
> >>  
> >>DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
> >> @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state 
> >> *state,
> >>  
> >>drm_connector_reference(connector);
> >>state->connectors[index].state = connector_state;
> >> +  state->connectors[index].old_state = connector->state;
> >> +  state->connectors[index].new_state = connector_state;
> >>state->connectors[index].ptr = connector;
> >>connector_state->state = state;
> >>  
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> >> b/drivers/gpu/drm/drm_atomic_helper.c
> >> index 07b432f43b98..fcb6e5b55217 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
> >>   *
> >>   * See also:
> >>   * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
> >> - * drm_atomic_helper_resume()
> >> + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state()
> >>   */
> >>  struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
> >>  {
> >> @@ -2536,6 +2536,52 @@ unlock:
> >>  EXPORT_SYMBOL(drm_atomic_helper_suspend);
> >>  
> >>  /**
> >> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state
> >> + * @state: duplicated atomic state to commit
> >> + * @ctx: pointer to acquire_ctx to use for commit.
> >> + * @nonblock: commit the state non-blocking.
> >> + *
> >> + * The state returned by drm_atomic_helper_duplicate_state() and
> >> + * drm_atomic_helper_suspend() is partially invalid, and needs to
> >> + * be fixed up before commit.
> > So the old_state pointers are potentially invalid because whatever->state
> > may have changed since we duplicated the state?
> 
> Yes when you use drm_atomic_helper_duplicate_state, and commit a different 
> state before committing the duplicated state.
> This only happens during suspend/resume and gpu reset.

Should we maybe have something like drm_atomic_refresh_old_state(state)?
Would allow the caller then to check something in the old vs. new state
before committing?

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state

2016-11-01 Thread Maarten Lankhorst
Op 01-11-16 om 14:09 schreef Ville Syrjälä:
> On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote:
>> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
>> replace the old for_each_xxx_in_state ones. This is useful for >1 flip
>> depth and getting rid of all xxx->state dereferences.
>>
>> Signed-off-by: Maarten Lankhorst 
>> ---
>>  drivers/gpu/drm/drm_atomic.c |  6 +++
>>  drivers/gpu/drm/drm_atomic_helper.c  | 52 +--
>>  drivers/gpu/drm/i915/intel_display.c | 11 ++---
>>  include/drm/drm_atomic.h | 81 
>> ++--
>>  include/drm/drm_atomic_helper.h  |  3 ++
>>  5 files changed, 142 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 5dd70540219c..120775fcfb70 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>>  return ERR_PTR(-ENOMEM);
>>  
>>  state->crtcs[index].state = crtc_state;
>> +state->crtcs[index].old_state = crtc->state;
>> +state->crtcs[index].new_state = crtc_state;
>>  state->crtcs[index].ptr = crtc;
>>  crtc_state->state = state;
>>  
>> @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state 
>> *state,
>>  
>>  state->planes[index].state = plane_state;
>>  state->planes[index].ptr = plane;
>> +state->planes[index].old_state = plane->state;
>> +state->planes[index].new_state = plane_state;
>>  plane_state->state = state;
>>  
>>  DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
>> @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state 
>> *state,
>>  
>>  drm_connector_reference(connector);
>>  state->connectors[index].state = connector_state;
>> +state->connectors[index].old_state = connector->state;
>> +state->connectors[index].new_state = connector_state;
>>  state->connectors[index].ptr = connector;
>>  connector_state->state = state;
>>  
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
>> b/drivers/gpu/drm/drm_atomic_helper.c
>> index 07b432f43b98..fcb6e5b55217 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
>>   *
>>   * See also:
>>   * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
>> - * drm_atomic_helper_resume()
>> + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state()
>>   */
>>  struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
>>  {
>> @@ -2536,6 +2536,52 @@ unlock:
>>  EXPORT_SYMBOL(drm_atomic_helper_suspend);
>>  
>>  /**
>> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state
>> + * @state: duplicated atomic state to commit
>> + * @ctx: pointer to acquire_ctx to use for commit.
>> + * @nonblock: commit the state non-blocking.
>> + *
>> + * The state returned by drm_atomic_helper_duplicate_state() and
>> + * drm_atomic_helper_suspend() is partially invalid, and needs to
>> + * be fixed up before commit.
> So the old_state pointers are potentially invalid because whatever->state
> may have changed since we duplicated the state?

Yes when you use drm_atomic_helper_duplicate_state, and commit a different 
state before committing the duplicated state.
This only happens during suspend/resume and gpu reset.

~Maarten

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state

2016-11-01 Thread Ville Syrjälä
On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote:
> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
> replace the old for_each_xxx_in_state ones. This is useful for >1 flip
> depth and getting rid of all xxx->state dereferences.
> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/drm_atomic.c |  6 +++
>  drivers/gpu/drm/drm_atomic_helper.c  | 52 +--
>  drivers/gpu/drm/i915/intel_display.c | 11 ++---
>  include/drm/drm_atomic.h | 81 
> ++--
>  include/drm/drm_atomic_helper.h  |  3 ++
>  5 files changed, 142 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 5dd70540219c..120775fcfb70 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>   return ERR_PTR(-ENOMEM);
>  
>   state->crtcs[index].state = crtc_state;
> + state->crtcs[index].old_state = crtc->state;
> + state->crtcs[index].new_state = crtc_state;
>   state->crtcs[index].ptr = crtc;
>   crtc_state->state = state;
>  
> @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
>  
>   state->planes[index].state = plane_state;
>   state->planes[index].ptr = plane;
> + state->planes[index].old_state = plane->state;
> + state->planes[index].new_state = plane_state;
>   plane_state->state = state;
>  
>   DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
> @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state 
> *state,
>  
>   drm_connector_reference(connector);
>   state->connectors[index].state = connector_state;
> + state->connectors[index].old_state = connector->state;
> + state->connectors[index].new_state = connector_state;
>   state->connectors[index].ptr = connector;
>   connector_state->state = state;
>  
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 07b432f43b98..fcb6e5b55217 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
>   *
>   * See also:
>   * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
> - * drm_atomic_helper_resume()
> + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state()
>   */
>  struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
>  {
> @@ -2536,6 +2536,52 @@ unlock:
>  EXPORT_SYMBOL(drm_atomic_helper_suspend);
>  
>  /**
> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state
> + * @state: duplicated atomic state to commit
> + * @ctx: pointer to acquire_ctx to use for commit.
> + * @nonblock: commit the state non-blocking.
> + *
> + * The state returned by drm_atomic_helper_duplicate_state() and
> + * drm_atomic_helper_suspend() is partially invalid, and needs to
> + * be fixed up before commit.

So the old_state pointers are potentially invalid because whatever->state
may have changed since we duplicated the state?

> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + *
> + * See also:
> + * drm_atomic_helper_suspend()
> + */
> +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
> +   struct drm_modeset_acquire_ctx 
> *ctx,
> +   bool nonblock)
> +{
> + int i;
> + struct drm_plane *plane;
> + struct drm_plane_state *plane_state;
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> +
> + state->acquire_ctx = ctx;
> +
> + for_each_new_plane_in_state(state, plane, plane_state, i)
> + state->planes[i].old_state = plane->state;
> +
> + for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> + state->crtcs[i].old_state = crtc->state;
> +
> + for_each_new_connector_in_state(state, connector, conn_state, i)
> + state->connectors[i].old_state = connector->state;
> +
> + if (nonblock)
> + return drm_atomic_nonblocking_commit(state);
> + else
> + return drm_atomic_commit(state);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state);
> +
> +/**
>   * drm_atomic_helper_resume - subsystem-level resume helper
>   * @dev: DRM device
>   * @state: atomic state to resume to
> @@ -2558,9 +2604,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
>   int err;
>  
>   drm_mode_config_reset(dev);
> +
>   drm_modeset_lock_all(dev);
> - state->acquire_ctx = config->acquire_ctx;
> - err = drm_atomic_commit(state);
> + err = 

[Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state

2016-10-17 Thread Maarten Lankhorst
Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
replace the old for_each_xxx_in_state ones. This is useful for >1 flip
depth and getting rid of all xxx->state dereferences.

Signed-off-by: Maarten Lankhorst 
---
 drivers/gpu/drm/drm_atomic.c |  6 +++
 drivers/gpu/drm/drm_atomic_helper.c  | 52 +--
 drivers/gpu/drm/i915/intel_display.c | 11 ++---
 include/drm/drm_atomic.h | 81 ++--
 include/drm/drm_atomic_helper.h  |  3 ++
 5 files changed, 142 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 5dd70540219c..120775fcfb70 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
return ERR_PTR(-ENOMEM);
 
state->crtcs[index].state = crtc_state;
+   state->crtcs[index].old_state = crtc->state;
+   state->crtcs[index].new_state = crtc_state;
state->crtcs[index].ptr = crtc;
crtc_state->state = state;
 
@@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
 
state->planes[index].state = plane_state;
state->planes[index].ptr = plane;
+   state->planes[index].old_state = plane->state;
+   state->planes[index].new_state = plane_state;
plane_state->state = state;
 
DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
@@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state 
*state,
 
drm_connector_reference(connector);
state->connectors[index].state = connector_state;
+   state->connectors[index].old_state = connector->state;
+   state->connectors[index].new_state = connector_state;
state->connectors[index].ptr = connector;
connector_state->state = state;
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 07b432f43b98..fcb6e5b55217 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
  *
  * See also:
  * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
- * drm_atomic_helper_resume()
+ * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state()
  */
 struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
 {
@@ -2536,6 +2536,52 @@ unlock:
 EXPORT_SYMBOL(drm_atomic_helper_suspend);
 
 /**
+ * drm_atomic_helper_commit_duplicated_state - commit duplicated state
+ * @state: duplicated atomic state to commit
+ * @ctx: pointer to acquire_ctx to use for commit.
+ * @nonblock: commit the state non-blocking.
+ *
+ * The state returned by drm_atomic_helper_duplicate_state() and
+ * drm_atomic_helper_suspend() is partially invalid, and needs to
+ * be fixed up before commit.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ *
+ * See also:
+ * drm_atomic_helper_suspend()
+ */
+int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
+ struct drm_modeset_acquire_ctx 
*ctx,
+ bool nonblock)
+{
+   int i;
+   struct drm_plane *plane;
+   struct drm_plane_state *plane_state;
+   struct drm_connector *connector;
+   struct drm_connector_state *conn_state;
+   struct drm_crtc *crtc;
+   struct drm_crtc_state *crtc_state;
+
+   state->acquire_ctx = ctx;
+
+   for_each_new_plane_in_state(state, plane, plane_state, i)
+   state->planes[i].old_state = plane->state;
+
+   for_each_new_crtc_in_state(state, crtc, crtc_state, i)
+   state->crtcs[i].old_state = crtc->state;
+
+   for_each_new_connector_in_state(state, connector, conn_state, i)
+   state->connectors[i].old_state = connector->state;
+
+   if (nonblock)
+   return drm_atomic_nonblocking_commit(state);
+   else
+   return drm_atomic_commit(state);
+}
+EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state);
+
+/**
  * drm_atomic_helper_resume - subsystem-level resume helper
  * @dev: DRM device
  * @state: atomic state to resume to
@@ -2558,9 +2604,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
int err;
 
drm_mode_config_reset(dev);
+
drm_modeset_lock_all(dev);
-   state->acquire_ctx = config->acquire_ctx;
-   err = drm_atomic_commit(state);
+   err = drm_atomic_helper_commit_duplicated_state(state, 
config->acquire_ctx, false);
drm_modeset_unlock_all(dev);
 
return err;
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index a12e093c54cf..3bd3f6a93c12 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3497,7 +3497,8 @@ static void