[Intel-gfx] [PATCH] drm/atomic: Call ww_acquire_done after check phase is complete

[Daniel Vetter]

On Thu, Aug 06, 2015 at 06:17:28AM +0200, Maarten Lankhorst wrote:
> Op 06-08-15 om 00:25 schreef Daniel Vetter:
> > On Wed, Aug 5, 2015 at 8:13 PM, Maarten Lankhorst
> > <maarten.lankhorst at linux.intel.com> wrote:
> >> Op 05-08-15 om 17:03 schreef Daniel Vetter:
> >>> On Wed, Aug 5, 2015 at 4:57 PM, Maarten Lankhorst
> >>> <maarten.lankhorst at linux.intel.com> wrote:
> >>>> Op 05-08-15 om 15:08 schreef Daniel Vetter:
> >>>>> We want to make sure that no one tries to acquire more locks and
> >>>>> states, and ww mutexes provide debug facilities for that. So use them.
> >>>>>
> >>>>> Cc: Rob Clark <robdclark at gmail.com>
> >>>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >>>>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/drm_atomic.c | 2 ++
> >>>>>  1 file changed, 2 insertions(+)
> >>>> I like the idea, played with the thought myself, but I think it might need to be slightly less strict for transitional drivers.
> >>> What would blow up? This should only be called fairly late in the
> >>> transition when most of the atomic handling is correctly done. And
> >>> i915 is probably the most extreme example of a conversion, so if it
> >>> works out for us I think everyone else should be fine too.
> >> Might blow up with transitional helpers, though not 100% sure if it would.
> > Transitional helpers don't use the top-level atomic_commit/check entry
> > points and hence don't use this function here at all.
> >
> >> Also if atomic_check returns -EDEADLK you would still say acquire_done, that will definitely blow up in legitimate cases..
> >>
> >> If it doesn't blow up transitional helpers and you fix the -EDEADLK, acked-by. :-)
> > Yeah that needs to be fixed ;-)
> After some sleep I think only when ret == 0 we should call acquire_done.
> >>> Generally drivers only started to do fancy stuff with get_*_state once
> >>> converted to atomic to start exploiting it, not before the transition
> >>> is completed. i915 is different since we have a lot of our own modeset
> >> Should we electrify drm_atomic_get_{*}_state too, to force everyone to use the get_existing_state versions?
> > And I think this is the killer - we unconditionally take the locks
> > again, taking advantage of -EALREADY. But with this patch that will
> > blow and we need to patch up all the existing code to use the
> > get_existing_state functions. That will be a bigger series I guess ...
> Depends, this only happens when the object can not be found in the state the locks are taken for planes and crtc's.
> But it seems for connectors it happens unconditionally.
> > I'll make a note about this and defer this for now. But the
> > get_*_state vs. get_existing_*_state thing will actually make these
> > two functions more distinctive, so I think this patch here will be
> > really useful. But needs driver fixes and kerneldoc updates too.
> I think this is fine; grabbing existing state for crtc's and planes will work and grabbing new state during atomic_commit is a bug anyway.
> The only driver that uses drm_atomic_get_connector_state is i915, which uses it for the setup phase.

Yeah I've done a full audit too, everything seems to be fine.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch