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 :-)
-Daniel
>
>> +
>> +/**
>> + * __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)
>> +{
>> + struct drm_atomic_state *state = container_of(ref, typeof(*state), ref);
>> +
>> + call_rcu(&state->rhead, ___drm_atomic_state_free);
>> +}
>> EXPORT_SYMBOL(__drm_atomic_state_free);
>>
>> /**
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 1c015344d063..b6bb8bad36a8 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -2041,7 +2041,7 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>> new_crtc_state->state = NULL;
>>
>> state->crtcs[i].state = old_crtc_state;
>> - crtc->state = new_crtc_state;
>> + rcu_assign_pointer(crtc->state, new_crtc_state);
>>
>> if (state->crtcs[i].commit) {
>> spin_lock(&crtc->commit_lock);
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index 0147a047878d..65433eec270b 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -28,6 +28,8 @@
>> #ifndef DRM_ATOMIC_H_
>> #define DRM_ATOMIC_H_
>>
>> +#include <linux/rcupdate.h>
>> +
>> #include <drm/drm_crtc.h>
>>
>> /**
>> @@ -188,6 +190,9 @@ struct drm_atomic_state {
>> * commit without blocking.
>> */
>> struct work_struct commit_work;
>> +
>> + /* private: */
>> + struct rcu_head rhead;
>> };
>>
>> void __drm_crtc_commit_free(struct kref *kref);
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index fabb35aba5f6..8e476986a43e 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -603,7 +603,6 @@ struct drm_cmdline_mode {
>> * @bad_edid_counter: track sinks that give us an EDID with invalid checksum
>> * @edid_corrupt: indicates whether the last read EDID was corrupt
>> * @debugfs_entry: debugfs directory for this connector
>> - * @state: current atomic state for this connector
>> * @has_tile: is this connector connected to a tiled monitor
>> * @tile_group: tile group for the connected monitor
>> * @tile_is_single_monitor: whether the tile is one monitor housing
>> @@ -771,6 +770,18 @@ struct drm_connector {
>>
>> struct dentry *debugfs_entry;
>>
>> + /**
>> + * @state:
>> + *
>> + * Current atomic state for this connector. Note that this is protected
>> + * by @mutex, but also by RCU (for the mode validation code, which needs
>> + * to peek at this with only hold &drm_mode_config.mutex).
>> + *
>> + * FIXME:
>> + *
>> + * This isn't annoted with __rcu because fixing up all the drivers is a
>> + * massive amount of work.
>> + */
>> struct drm_connector_state *state;
>>
>> /* DisplayID bits */
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 24dcb121bad4..6470a0012e38 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -747,7 +747,14 @@ struct drm_crtc {
>> /**
>> * @state:
>> *
>> - * Current atomic state for this CRTC.
>> + * Current atomic state for this CRTC. Note that this is protected by
>> + * @mutex, but also by RCU (for the vblank code, which needs to peek at
>> + * this from interrupt context).
>> + *
>> + * FIXME:
>> + *
>> + * This isn't annoted with __rcu because fixing up all the drivers is a
>> + * massive amount of work.
>> */
>> struct drm_crtc_state *state;
>>
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
