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@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
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.
~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
