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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
