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

[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 <maarten.lankhorst at linux.intel.com>
>>>>>> ---
>>>>>>  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 nonblocking
> vs. blocking commits, which then just pass another bool parameter to the
> commit hook. Should we have block vs. nonblocking variants of this
> function as well or just remove drm_atomic_nonblocking_commit() and have
> callers pass the parameter straight from the beginning?
>
> drm_atomic_commit() and drm_atomic_nonblocking_commit() could also use
> some code deduplication I think. Should we decice to keep them around.

I could have done the check and commit manually, but would rather use the dedicated atomic commands for it.
However nowhere we end up using the nonblocking version, so I think I will remove the nonblock parameter
and leave the decision on what to do with nonblocking commit for another day. :)

~Maarten