[Intel-gfx] [PATCH v3 1/7] drm/atomic: Add new iterators over all state, v3.

[Daniel Vetter]

On Thu, Jan 19, 2017 at 12:56:29AM +0200, Laurent Pinchart wrote:
> On Tuesday 17 Jan 2017 08:41:03 Maarten Lankhorst wrote:
> > Op 17-01-17 om 00:11 schreef Laurent Pinchart:
> > > On Monday 16 Jan 2017 10:37:38 Maarten Lankhorst wrote:
> > >> @@ -2512,6 +2518,47 @@ struct drm_atomic_state
> > >> *drm_atomic_helper_suspend(struct drm_device *dev)
> > >> 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.
> > >> + *
> > >> + * 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.
> > > 
> > > Can't you fix the state in drm_atomic_helper_suspend() to avoid the need
> > > for this function ?
> > 
> > We would have to fix up other areas that duplicate state too, like i915
> > suspend and load detect. This means moving it to a helper.
> > 
> > It was my original approach, but Daniel preferred this approach.
> 
> We have to fix it somewhere, that's for sure. I think it's better to fix it as 
> close as possible to state duplication, instead of carrying a state that we 
> know is invalid and fix it at the last minute. The drawback of this approach 
> is that the window during which the state will be invalid is much larger, 
> which could cause bugs later when new code will try to touch that state.
> 
> Daniel, is there a specific reason why you prefer this approach ?

You can't fix it in suspend? The issue is that on resume, we have reset
states (to reflect the hw state of everything being off), so we can only
do this on commit. That holds in general for the duplicate, do something
nasty, restore state pattern.

And then I think a dedicated helper to commit duplicated state makes
sense. The kernel-doc might need a bit of work to explain this better
though. I think it should explain what exactly is partially invalid with
duplicated states.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch