| Message ID | 1355317637-16742-11-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 12, 2012 at 02:06:50PM +0100, Daniel Vetter wrote: > *drumroll* > > The basic idea is to protect per-crtc state which can change without > touching the output configuration with separate mutexes, i.e. all the > input side state to a crtc like framebuffers, cursor settings or plane > configuration. Holding such a crtc lock gives a read-lock on all the > other crtc state which can be changed by e.g. a modeset. > > All non-crtc state is still protected by the mode_config mutex. > Callers that need to change modeset state of a crtc (e.g. dpms or > set_mode) need to grab both the mode_config lock and nested within any > crtc locks. > > Note that since there can only ever be one holder of the mode_config > lock we can grab the subordinate crtc locks in any order (if we need > to grab more than one of them). Lockdep can handle such nesting with > the mutex_lock_nest_lock call correctly. > > With this functions that only touch connectors/encoders but not crtcs > only need to take the mode_config lock. The biggest such case is the > output probing, which means that we can now pageflip and move cursors > while the output probe code is reading an edid. > > Most cases neatly fall into the three buckets: > - Only touches connectors and similar output state and so only needs > the mode_config lock. > - Touches the global configuration and so needs all locks. > - Only touches the crtc input side and so only needs the crtc lock. > > But a few cases that need special consideration: > > - Load detection which requires a crtc. The mode_config lock already > prevents a modeset change, so we can use any unused crtc as we like > to do load detection. The only thing to consider is that such > temporary state changes don't leak out to userspace through ioctls > that only take the crtc look (like a pageflip). Hence the load > detect code needs to grab the crtc of any output pipes it touches > (but only if it touches state used by the pageflip or cursor > ioctls). > > - Atomic pageflip when moving planes. The first case is sane hw, where > planes have a fixed association with crtcs - nothing needs to be > done there. More insane^Wflexible hw needs to have plane->crtc > mapping which is separately protect with a lock that nests within > the crtc lock. If the plane is unused we can just assign it to the > current crtc and continue. But if a plane is already in use by > another crtc we can't just reassign it. > > Two solution present themselves: Either go back to a slow-path which > takes all modeset locks, potentially incure quite a hefty delay. Or > simply disallowing such changes in one atomic pageflip - in general > the vblanks of two crtcs are not synced, so there's no sane way to > atomically flip such plane changes accross more than one crtc. I'd > heavily favour the later approach, going as far as mandating it as > part of the ABI of such a new a nuclear pageflip. Agreed. Just disallow moving an enabled plane between CRTCs. First disable on the old CRTC, then enable on the new one. Two ioctls required to complete the operation. I think everyone should be able to live with that restriction. > And if we _really_ want such semantics, we can always get them by > introducing another pageflip mutex between the mode_config.mutex and > the individual crtc locks. Pageflips crossing more than one crtc > would then need to take that lock first, to lock out concurrent > multi-crtc pageflips. I'm actually concerned with multi CRTC page flips, not for moving planes between CRTCs, but mainly for updating content on genlocked displays atomically. We need to avoid deadlocks between multiple CRTC locks. Trying to take the CRTC locks in the same order would be a solution, but since user space can send the props in any order, it's going to require extra of work. Another lock as you suggest could work. But I suppose the atomic ioctl would need another flag to signal that the kernel needs to grab the multi CRTC lock in the beginning. The downside is that modeset would need to take that lock too, and then you end up in the same situation where a modeset operation on one display will disturb animations on other displays. > - Optimized global modeset operations: We could just take the > mode_config lock and then lazily lock all crtc which are affected by > a modeset operation. This has the advantage that pageflip could > continue unhampered on unaffected crtc. But if e.g. global resources > like plls need to be reassigned and so affect unrelated crtcs we can > still do that - nested locking works in any order. Right. In my atomic ioctl this could be done in the beginning by checking the non-block flag. If the flag is not set, then grab the global lock, and you can lock each CRTC when you see that you need to touch them. I would need to change my code to refuse any change to modeset state when the non-block flag is set. Currently I'm doing that check as late as possible, so that that user space can still send modeset related props, but if they don't end up changing anything in the modeset state, I can continue with the non-blocking operation. But you can argue that userspace is being silly if it sends such things, and we can just refuse it early on.
On Thu, Dec 13, 2012 at 12:38 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> And if we _really_ want such semantics, we can always get them by >> introducing another pageflip mutex between the mode_config.mutex and >> the individual crtc locks. Pageflips crossing more than one crtc >> would then need to take that lock first, to lock out concurrent >> multi-crtc pageflips. > > I'm actually concerned with multi CRTC page flips, not for moving planes > between CRTCs, but mainly for updating content on genlocked displays > atomically. We need to avoid deadlocks between multiple CRTC locks. Trying > to take the CRTC locks in the same order would be a solution, but since > user space can send the props in any order, it's going to require extra > of work. Ordering CRTC locks should also work - modeset_lock_all takes them always in the same order, and as long as you only take a single crtc nested within the modeset lock that's still ok (e.g. the load-detect code). We don't have many CRTCs, so even the dumbest sort will be fast enough. A bit of work will be required to make lockdep happy. But we can achieve this by nesting CRTCs within a fake lock encapsulated by the lock/unlock helper functions. -Daniel
On Thu, Dec 13, 2012 at 12:54:44PM +0100, Daniel Vetter wrote: > On Thu, Dec 13, 2012 at 12:38 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > >> And if we _really_ want such semantics, we can always get them by > >> introducing another pageflip mutex between the mode_config.mutex and > >> the individual crtc locks. Pageflips crossing more than one crtc > >> would then need to take that lock first, to lock out concurrent > >> multi-crtc pageflips. > > > > I'm actually concerned with multi CRTC page flips, not for moving planes > > between CRTCs, but mainly for updating content on genlocked displays > > atomically. We need to avoid deadlocks between multiple CRTC locks. Trying > > to take the CRTC locks in the same order would be a solution, but since > > user space can send the props in any order, it's going to require extra > > of work. > > Ordering CRTC locks should also work - modeset_lock_all takes them > always in the same order, and as long as you only take a single crtc > nested within the modeset lock that's still ok (e.g. the load-detect > code). We don't have many CRTCs, so even the dumbest sort will be fast > enough. A bit of work will be required to make lockdep happy. But we > can achieve this by nesting CRTCs within a fake lock encapsulated by > the lock/unlock helper functions. Yeah it would mean pre-processing the object ID list in the atomic ioctl. Currently there is at most num_crtc+num_plane object IDs in the list, assuming userspace didn't send any duplicates. For each of those we'd need to take the CRTC lock. So a bit of a change to my code, but not too bad I suppose. But this also has to handle planes that can move between CRTCs, so it's not quite as simple as that. Maybe grab the lock for plane->crtc, and once you have the lock re-check that plane->crtc didn't change before we got the lock. We also need to change things so that plane->crtc can never be NULL. Currently when a plane is disabled, we set plane->crtc to NULL, but since we need that information for taking the lock, and to prevent two guys from accessing the same disabled plane, we can no longer allow that.
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 5d223af..91e8068 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -46,7 +46,12 @@ */ void drm_modeset_lock_all(struct drm_device *dev) { + struct drm_crtc *crtc; + mutex_lock(&dev->mode_config.mutex); + + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) + mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex); } EXPORT_SYMBOL(drm_modeset_lock_all); @@ -56,6 +61,11 @@ EXPORT_SYMBOL(drm_modeset_lock_all); */ void drm_modeset_unlock_all(struct drm_device *dev) { + struct drm_crtc *crtc; + + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) + mutex_unlock(&crtc->mutex); + mutex_unlock(&dev->mode_config.mutex); } EXPORT_SYMBOL(drm_modeset_unlock_all); @@ -451,6 +461,8 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, crtc->invert_dimensions = false; drm_modeset_lock_all(dev); + mutex_init(&crtc->mutex); + mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex); ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); if (ret) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index b487922..f1296ce 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -390,6 +390,15 @@ struct drm_crtc { struct drm_device *dev; struct list_head head; + /** + * crtc mutex + * + * This provides a read lock for the overall crtc state (mode, dpms + * state, ...) and a write lock for everything which can be update + * without a full modeset (fb, cursor data, ...) + */ + struct mutex mutex; + struct drm_mode_object base; /* framebuffer the connector is currently bound to */
*drumroll* The basic idea is to protect per-crtc state which can change without touching the output configuration with separate mutexes, i.e. all the input side state to a crtc like framebuffers, cursor settings or plane configuration. Holding such a crtc lock gives a read-lock on all the other crtc state which can be changed by e.g. a modeset. All non-crtc state is still protected by the mode_config mutex. Callers that need to change modeset state of a crtc (e.g. dpms or set_mode) need to grab both the mode_config lock and nested within any crtc locks. Note that since there can only ever be one holder of the mode_config lock we can grab the subordinate crtc locks in any order (if we need to grab more than one of them). Lockdep can handle such nesting with the mutex_lock_nest_lock call correctly. With this functions that only touch connectors/encoders but not crtcs only need to take the mode_config lock. The biggest such case is the output probing, which means that we can now pageflip and move cursors while the output probe code is reading an edid. Most cases neatly fall into the three buckets: - Only touches connectors and similar output state and so only needs the mode_config lock. - Touches the global configuration and so needs all locks. - Only touches the crtc input side and so only needs the crtc lock. But a few cases that need special consideration: - Load detection which requires a crtc. The mode_config lock already prevents a modeset change, so we can use any unused crtc as we like to do load detection. The only thing to consider is that such temporary state changes don't leak out to userspace through ioctls that only take the crtc look (like a pageflip). Hence the load detect code needs to grab the crtc of any output pipes it touches (but only if it touches state used by the pageflip or cursor ioctls). - Atomic pageflip when moving planes. The first case is sane hw, where planes have a fixed association with crtcs - nothing needs to be done there. More insane^Wflexible hw needs to have plane->crtc mapping which is separately protect with a lock that nests within the crtc lock. If the plane is unused we can just assign it to the current crtc and continue. But if a plane is already in use by another crtc we can't just reassign it. Two solution present themselves: Either go back to a slow-path which takes all modeset locks, potentially incure quite a hefty delay. Or simply disallowing such changes in one atomic pageflip - in general the vblanks of two crtcs are not synced, so there's no sane way to atomically flip such plane changes accross more than one crtc. I'd heavily favour the later approach, going as far as mandating it as part of the ABI of such a new a nuclear pageflip. And if we _really_ want such semantics, we can always get them by introducing another pageflip mutex between the mode_config.mutex and the individual crtc locks. Pageflips crossing more than one crtc would then need to take that lock first, to lock out concurrent multi-crtc pageflips. - Optimized global modeset operations: We could just take the mode_config lock and then lazily lock all crtc which are affected by a modeset operation. This has the advantage that pageflip could continue unhampered on unaffected crtc. But if e.g. global resources like plls need to be reassigned and so affect unrelated crtcs we can still do that - nested locking works in any order. This patch just adds the locks and takes them in drm_modeset_lock_all, no real locking changes yet. v2: Need to initialize the new lock in crtc_init and lock it righ away, for otherwise the modeset_unlock_all below will try to unlock a not-locked mutex. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_crtc.c | 12 ++++++++++++ include/drm/drm_crtc.h | 9 +++++++++ 2 files changed, 21 insertions(+)