[Intel-gfx] [PATCH] drm/atomic: protect crtc|connector->state with rcu
Daniel Vetter
daniel at ffwll.ch
Mon Mar 20 15:08:54 UTC 2017
On Mon, Mar 20, 2017 at 11:01:59AM +0100, Maarten Lankhorst wrote:
> 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
Yeah, that might be the thing to do ... I just would like to have some
consistent rules, and very much preferrably implemented entirely in
helpers.
We probably need to keep it around for legacy drivers (out of abundance of
caution), but could otherwise entirely nerf mode_config.mutex.
Note that it is used in some strange places, e.g. fbdev emulation helpers
use it to protect their datastructures (which really should be fixed, bkls
are evil).
> > 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.
What we might want to keep working is a read-only probe path, for sysfs
and also perhaps for the GetConnectorCached ioctl (num = 1 trickery).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list