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

[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 ;-)

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

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch