| Message ID | 20170316155235.19971-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@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@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. > + > +/** > + * __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; >
On Thu, Mar 16, 2017 at 5:09 PM, Maarten Lankhorst <maarten.lankhorst@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@linux.intel.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@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@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 :-) -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; >> > >
Op 16-03-17 om 21:15 schreef Daniel Vetter: > On Thu, Mar 16, 2017 at 5:09 PM, Maarten Lankhorst > <maarten.lankhorst@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@linux.intel.com> >>> Signed-off-by: Daniel Vetter <daniel.vetter@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@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. :(
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@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@linux.intel.com> > >>> Signed-off-by: Daniel Vetter <daniel.vetter@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@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
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@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@linux.intel.com> >>>>> Signed-off-by: Daniel Vetter <daniel.vetter@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@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. ~Maarten
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@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@linux.intel.com> > >>>>> Signed-off-by: Daniel Vetter <daniel.vetter@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@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. 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. -Daniel
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@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@linux.intel.com> >>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@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@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
On Mon, Mar 20, 2017 at 11:01:59AM +0100, Maarten Lankhorst wrote: > Op 20-03-17 om 09:59 schreef Daniel Vetter: > > 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. It can happen - just consider two clients, such as system-logind walking sysfs. Delaying most modesets would not be an issue, except where the modeset is being uses to e.g. changes strides before in the middle of a pageflip sequence. I would welcome kms_flip flip/set/vblank-vs-probe, and definitely should add it to legacy-cursor and legacy-plane as well. -Chris
Op 20-03-17 om 11:22 schreef Chris Wilson: > On Mon, Mar 20, 2017 at 11:01:59AM +0100, Maarten Lankhorst wrote: >> Op 20-03-17 om 09:59 schreef Daniel Vetter: >>> 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. > It can happen - just consider two clients, such as system-logind walking > sysfs. Delaying most modesets would not be an issue, except where the > modeset is being uses to e.g. changes strides before in the middle of a > pageflip sequence. With atomic pageflip, this is no longer required on i915. :) > I would welcome kms_flip flip/set/vblank-vs-probe, and definitely should > add it to legacy-cursor and legacy-plane as well. Could be useful, the normal paths should never take connection_mutex anyway, so either way it should pass. ~Maarten
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@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@linux.intel.com> > >>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@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@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
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); } } + +/** + * __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;
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. 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@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@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(-)