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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> >>> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> >>> ---
> >>> 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@xxxxxxxxxxxxxxxxxx>
> >> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
