[Intel-gfx] [PATCH] drm/atomic: protect crtc|connector->state with rcu

Mon Mar 20 10:01:59 UTC 2017

Op 20-03-17 om 09:59 schreef Daniel Vetter:
> On Mon, Mar 20, 2017 at 09:38:52AM +0100, Maarten Lankhorst wrote:
>> Op 20-03-17 om 09:18 schreef Daniel Vetter:
>>> On Fri, Mar 17, 2017 at 05:52:32PM +0100, Maarten Lankhorst wrote:
>>>> Op 16-03-17 om 21:15 schreef Daniel Vetter:
>>>>> On Thu, Mar 16, 2017 at 5:09 PM, Maarten Lankhorst
>>>>> <maarten.lankhorst at linux.intel.com> wrote:
>>>>>> Op 16-03-17 om 16:52 schreef Daniel Vetter:
>>>>>>> The vblank code really wants to look at crtc->state without having to
>>>>>>> take a ww_mutex. One option might be to take one of the vblank locks
>>>>>>> right when assigning crtc->state, which would ensure that the vblank
>>>>>>> code doesn't race and access freed memory.
>>>>>> I'm not sure this is the right approach for vblank.
>>>>> It's not, it's just that I've had to resurrect this patch quickly
>>>>> before leaving and accidentally left the vblank stuff in.
>>>>>
>>>>>> crtc->state might not be the same as the current state in case of a nonblocking
>>>>>> modeset that hasn't even disabled the old crtc yet.
>>>>>>> But userspace tends to poke the vblank_ioctl to query the current
>>>>>>> vblank timestamp rather often, and going all in with rcu would help a
>>>>>>> bit. We're not there yet, but also doesn't really hurt.
>>>>>>>
>>>>>>> v2: Maarten needs this to make connector properties atomic, so he can
>>>>>>> peek at state from the various ->mode_valid hooks.
>>>>>>>
>>>>>>> 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        | 26 +++++++++++++++++---------
>>>>>>>  drivers/gpu/drm/drm_atomic_helper.c |  2 +-
>>>>>>>  include/drm/drm_atomic.h            |  5 +++++
>>>>>>>  include/drm/drm_connector.h         | 13 ++++++++++++-
>>>>>>>  include/drm/drm_crtc.h              |  9 ++++++++-
>>>>>>>  5 files changed, 43 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>>>>> index 9b892af7811a..ba11e31ff9ba 100644
>>>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>>>> @@ -213,16 +213,10 @@ void drm_atomic_state_clear(struct drm_atomic_state *state)
>>>>>>>  }
>>>>>>>  EXPORT_SYMBOL(drm_atomic_state_clear);
>>>>>>>
>>>>>>> -/**
>>>>>>> - * __drm_atomic_state_free - free all memory for an atomic state
>>>>>>> - * @ref: This atomic state to deallocate
>>>>>>> - *
>>>>>>> - * This frees all memory associated with an atomic state, including all the
>>>>>>> - * per-object state for planes, crtcs and connectors.
>>>>>>> - */
>>>>>>> -void __drm_atomic_state_free(struct kref *ref)
>>>>>>> +void ___drm_atomic_state_free(struct rcu_head *rhead)
>>>>>>>  {
>>>>>>> -     struct drm_atomic_state *state = container_of(ref, typeof(*state), ref);
>>>>>>> +     struct drm_atomic_state *state =
>>>>>>> +             container_of(rhead, typeof(*state), rhead);
>>>>>>>       struct drm_mode_config *config = &state->dev->mode_config;
>>>>>>>
>>>>>>>       drm_atomic_state_clear(state);
>>>>>>> @@ -236,6 +230,20 @@ void __drm_atomic_state_free(struct kref *ref)
>>>>>>>               kfree(state);
>>>>>>>       }
>>>>>>>  }
>>>>>> whatisRCU.txt:
>>>>>> "This function invokes func(head) after a grace period has elapsed.
>>>>>> This invocation might happen from either softirq or process context,
>>>>>> so the function is not permitted to block."
>>>>>>
>>>>>> Looking at
>>>>>> commit 6f0f02dc56f18760b46dc1bf5b3f7386869d4162
>>>>>> Author: Chris Wilson <chris at chris-wilson.co.uk>
>>>>>> Date:   Mon Jan 23 21:29:39 2017 +0000
>>>>>>
>>>>>>     drm/i915: Move atomic state free from out of fence release
>>>>>>
>>>>>> I would say that drm_atomic_state_free would definitely block..
>>>>>>
>>>>>> The only thing that makes sense IMO is doing kfree_rcu on the object_states.
>>>>>> But I don't think RCU is the answer here, it won't protect you against using
>>>>>> the wrong crtc state.
>>>>>>
>>>>>> I think I would try to use the crtc ww_mutex in the vblank code and serialize it to pending commits, if at all possible.
>>>>> Oops. I guess it should have come with "totally untested". In that
>>>>> case we need a workter which does a synchronize_rcu() before
>>>>> releasing. I don't just want to do a simple kfree_rcu() because by
>>>>> that point we've (partially) destroyed the state alreayd (so it's
>>>>> already unsafe to access, and special ruels are ugly). And doing it
>>>>> here before we release anything in the core would avoid the need for
>>>>> drivers to bother with kfree_rcu().
>>>>>
>>>>> I'll try to respin something less obviously buggy tomorrow :-)
>>>> It will still be buggy tomorrow, since you have no way to know if the current hardware crtc_state is anything like crtc->state.
>>>>
>>>> :(
>>> Maybe I wasnt' clear, so let me retry:
>>>
>>> - this approach doesn't work for vblank and crtc state. Agreed. I'll
>>>   remove all the leftover comments I've forgotten to remove in a hurry.
>>>
>>> - the patch itself is broken, so can't be used for connector->state
>>>   peeking in mode_valid either. That one I'll fix.
>>>
>>> Does that make sense?
>>> -Daniel
>> Yes, but I'm still not completely convinced it's required for connector state to use RCU. During detect()
>> we would take the connection_mutex anyway, so I can probably  do that for mode_valid as well.
> Why do you want to take the connection_mutex there? If we just go with
> taking that everywhere we touch shared state, we might as well push that
> up in the callchain ... With the connector_iter stuff there's no reason at
> all anymore to hold the mode_config.mutex for anything really around
> connector probing.
Lets kill that lock, then. :D
> The only thing we need is that we pass the acquire_ctx down into callars
> somehow, so that the load detect stuff still works.
>
> But my idea was kinda that we'd do the same for probe -> modeset data
> flows like here for the other way round: Just a bunch of READ_ONCE and
> maybe lookup the edid with rcu too. That way it's clear to anybody that
> probe and modeset are entirely not synchronized.

Though I think it's beneficial to lock them. if it is. I'm not sure there
are many usecases for parallel modeset vs probe. And if someone does care,
they can use nonblocking modesets, maybe.

Legacy userspace probably can't do blocking modeset and probe at the same
time, so I don't think it will regress.

~Maarten