diff mbox

[10/37] drm: add per-crtc locks

Message ID 1355317637-16742-11-git-send-email-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter Dec. 12, 2012, 1:06 p.m. UTC
*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(+)

Comments

Ville Syrjälä Dec. 13, 2012, 11:38 a.m. UTC | #1
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.
Daniel Vetter Dec. 13, 2012, 11:54 a.m. UTC | #2
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
Ville Syrjälä Dec. 13, 2012, 2:25 p.m. UTC | #3
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 mbox

Patch

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 */