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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

On Monday 23 Jan 2017 09:48:54 Daniel Vetter wrote:
> 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.

Ok, I got it now. You can't fix the state up at suspend time because you need 
to set the old_state pointers to the state allocated by the .reset() handlers, 
which are called at resume time.

> 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.

Yes, the documentation should be clarified, and include the above explanation 
in some form.

-- 
Regards,

Laurent Pinchart

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