On Tue, Jul 29, 2014 at 11:32:16PM +0200, Daniel Vetter wrote:

In the atomic state we'll have an array of states for crtcs, planes and connectors and need to be able to at them by their index. We already have a drm_crtc_index function so add the missing ones for planes and connectors.

If it later on turns out that the list walking is too expensive we can add the index to the relevant modeset objects.

Rob Clark doesn't like the loops too much, but we can always add an obj->idx parameter later on. And for now reiterating is actually safer since nowadays we have hotpluggable connectors (thanks to DP MST).

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch

drivers/gpu/drm/drm_crtc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 2 ++ 2 files changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 805240b11229..5a494caa8c9a 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -937,6 +937,29 @@ void drm_connector_cleanup(struct drm_connector *connector) EXPORT_SYMBOL(drm_connector_cleanup);

/**

• • drm_plane_index - find the index of a registered CRTC

Looks like some copy/paste that needs an update...your kerneldoc calls the function drm_*PLANE*_index and then talks about CRTC's, but the actual function is for connectors...

• • @plane: CRTC to find index for
• • Given a registered CRTC, return the index of that CRTC within a DRM
• • device's list of CRTCs.
• */

+unsigned int drm_connector_index(struct drm_connector *connector) +{

• unsigned int index = 0;
• struct drm_connector *tmp;
• list_for_each_entry(tmp, &connector->dev->mode_config.connector_list, head) {

• if (tmp == connector)
  

• 	return index;
  

• index++;
  

• }
• BUG();

+} +EXPORT_SYMBOL(drm_connector_index);

+/**

• drm_connector_register - register a connector
• @connector: the connector to register

@@ -1239,6 +1262,29 @@ void drm_plane_cleanup(struct drm_plane *plane) EXPORT_SYMBOL(drm_plane_cleanup);

/**

• • drm_plane_index - find the index of a registered CRTC
• • @plane: CRTC to find index for
• • Given a registered CRTC, return the index of that CRTC within a DRM
• • device's list of CRTCs.
• */

More copy/paste referenecs to CRTC's.

+unsigned int drm_plane_index(struct drm_plane *plane) +{

• unsigned int index = 0;
• struct drm_plane *tmp;
• list_for_each_entry(tmp, &plane->dev->mode_config.plane_list, head) {

• if (tmp == plane)
  

• 	return index;
  

• index++;
  

• }
• BUG();

+} +EXPORT_SYMBOL(drm_plane_index);

+/**

• drm_plane_force_disable - Forcibly disable a plane
• @plane: plane to disable

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index f1105d0da059..4cae44611ab0 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -903,6 +903,7 @@ int drm_connector_register(struct drm_connector *connector); void drm_connector_unregister(struct drm_connector *connector);

extern void drm_connector_cleanup(struct drm_connector *connector); +extern unsigned int drm_connector_index(struct drm_connector *crtc);

^^^^ connector?

/* helper to unplug all connectors from sysfs for device */ extern void drm_connector_unplug_all(struct drm_device *dev);

@@ -942,6 +943,7 @@ extern int drm_plane_init(struct drm_device *dev, const uint32_t *formats, uint32_t format_count, bool is_primary); extern void drm_plane_cleanup(struct drm_plane *plane); +extern unsigned int drm_plane_index(struct drm_plane *crtc);

^^^^ plane?

extern void drm_plane_force_disable(struct drm_plane *plane); extern int drm_crtc_check_viewport(const struct drm_crtc *crtc, int x, int y, -- 2.0.1

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx

So drivers using the atomic interfaces expect that they can acquire additional locks internal to the driver as-needed. Examples would be locks to protect shared state like shared display PLLs.

Unfortunately the legacy ioctls assume that all locking is fully done by the drm core. Now for those paths which grab all locks we already have to keep around an acquire context in dev->mode_config. Helper functions that implement legacy interfaces in terms of atomic support can therefore grab this acquire contexts and reuse it.

The only interfaces left are the cursor and pageflip ioctls. So add functions to grab the crtc lock these need using an acquire context and preserve it for atomic drivers to reuse.

v2: - Fixup comments&kerneldoc. - Drop the WARNING from modeset_lock_all_crtcs since that can be used in legacy paths with crtc locking.

v3: Fix a type on the kerneldoc Dave spotted.

Cc: Dave Airlie airlied@redhat.com Reviewed-by: Dave Airlie airlied@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 8 ++-- drivers/gpu/drm/drm_modeset_lock.c | 84 ++++++++++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 6 +++ include/drm/drm_modeset_lock.h | 5 +++ 4 files changed, 99 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d90374e6a8cb..cb741cd8bfe9 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2714,7 +2714,7 @@ static int drm_mode_cursor_common(struct drm_device *dev, if (crtc->cursor) return drm_mode_cursor_universal(crtc, req, file_priv);

- drm_modeset_lock(&crtc->mutex, NULL); + drm_modeset_lock_crtc(crtc); if (req->flags & DRM_MODE_CURSOR_BO) { if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) { ret = -ENXIO; @@ -2738,7 +2738,7 @@ static int drm_mode_cursor_common(struct drm_device *dev, } } out: - drm_modeset_unlock(&crtc->mutex); + drm_modeset_unlock_crtc(crtc);

return ret;

@@ -4474,7 +4474,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, if (!crtc) return -ENOENT;

- drm_modeset_lock(&crtc->mutex, NULL); + drm_modeset_lock_crtc(crtc); if (crtc->primary->fb == NULL) { /* The framebuffer is currently unbound, presumably * due to a hotplug event, that userspace has not @@ -4558,7 +4558,7 @@ out: drm_framebuffer_unreference(fb); if (old_fb) drm_framebuffer_unreference(old_fb); - drm_modeset_unlock(&crtc->mutex); + drm_modeset_unlock_crtc(crtc);

return ret; } diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c index 73e6534fd0aa..4753c8bd5ab5 100644 --- a/drivers/gpu/drm/drm_modeset_lock.c +++ b/drivers/gpu/drm/drm_modeset_lock.c @@ -130,6 +130,90 @@ void drm_modeset_unlock_all(struct drm_device *dev) EXPORT_SYMBOL(drm_modeset_unlock_all);

/** + * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx + * @crtc: drm crtc + * + * This function locks the given crtc using a hidden acquire context. This is + * necessary so that drivers internally using the atomic interfaces can grab + * further locks with the lock acquire context. + */ +void drm_modeset_lock_crtc(struct drm_crtc *crtc) +{ + struct drm_modeset_acquire_ctx *ctx; + int ret; + + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); + if (WARN_ON(!ctx)) + return; + + drm_modeset_acquire_init(ctx, 0); + +retry: + ret = drm_modeset_lock(&crtc->mutex, ctx); + if (ret) + goto fail; + + WARN_ON(crtc->acquire_ctx); + + /* now we hold the locks, so now that it is safe, stash the + * ctx for drm_modeset_unlock_crtc(): + */ + crtc->acquire_ctx = ctx; + + return; + +fail: + if (ret == -EDEADLK) { + drm_modeset_backoff(ctx); + goto retry; + } +} +EXPORT_SYMBOL(drm_modeset_lock_crtc); + +/** + * drm_modeset_legacy_acquire_ctx - find acquire ctx for legacy ioctls + * crtc: drm crtc + * + * Legacy ioctl operations like cursor updates or page flips only have per-crtc + * locking, and store the acquire ctx in the corresponding crtc. All other + * legacy operations take all locks and use a global acquire context. This + * function grabs the right one. + */ +struct drm_modeset_acquire_ctx * +drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc) +{ + if (crtc->acquire_ctx) + return crtc->acquire_ctx; + + WARN_ON(!crtc->dev->mode_config.acquire_ctx); + + return crtc->dev->mode_config.acquire_ctx; +} +EXPORT_SYMBOL(drm_modeset_legacy_acquire_ctx); + +/** + * drm_modeset_unlock_crtc - drop crtc lock + * @crtc: drm crtc + * + * This drops the crtc lock acquire with drm_modeset_lock_crtc() and all other + * locks acquired through the hidden context. + */ +void drm_modeset_unlock_crtc(struct drm_crtc *crtc) +{ + struct drm_modeset_acquire_ctx *ctx = crtc->acquire_ctx; + + if (WARN_ON(!ctx)) + return; + + crtc->acquire_ctx = NULL; + drm_modeset_drop_locks(ctx); + drm_modeset_acquire_fini(ctx); + + kfree(ctx); +} +EXPORT_SYMBOL(drm_modeset_unlock_crtc); + +/** * drm_warn_on_modeset_not_all_locked - check that all modeset locks are locked * @dev: device * diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 4521c59d15fc..8350afe96fa9 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -371,6 +371,12 @@ struct drm_crtc { void *helper_private;

struct drm_object_properties properties; + + /* + * For legacy crtc ioctls so that atomic drivers can get at the locking + * acquire context. + */ + struct drm_modeset_acquire_ctx *acquire_ctx; };

diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h index cf61e857bc06..d38e1508f11a 100644 --- a/include/drm/drm_modeset_lock.h +++ b/include/drm/drm_modeset_lock.h @@ -120,10 +120,15 @@ int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock, void drm_modeset_unlock(struct drm_modeset_lock *lock);

struct drm_device; +struct drm_crtc;

void drm_modeset_lock_all(struct drm_device *dev); void drm_modeset_unlock_all(struct drm_device *dev); +void drm_modeset_lock_crtc(struct drm_crtc *crtc); +void drm_modeset_unlock_crtc(struct drm_crtc *crtc); void drm_warn_on_modeset_not_all_locked(struct drm_device *dev); +struct drm_modeset_acquire_ctx * +drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc);

int drm_modeset_lock_all_crtcs(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx);

On Tue, Jul 29, 2014 at 04:46:11PM -0700, Matt Roper wrote:

On Tue, Jul 29, 2014 at 11:32:19PM +0200, Daniel Vetter wrote:

...

Atomic implemenations for legacy ioctls must be able to drop locks. Which doesn't cause havoc since we only do that while constructing the new state, so no driver or hardware state change has happened.

The only troubling bit is the fb refcounting the core does - if someone else has snuck in then it might potentially unref an outdated framebuffer. To fix that move the old_fb temporary storage into struct drm_plane for all ioctls, so that the atomic helpers can update it.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch

drivers/gpu/drm/drm_crtc.c | 40 ++++++++++++++++++++++++---------------- include/drm/drm_crtc.h | 8 ++++---- 2 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index c09374038f9a..bacf565449d5 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1200,19 +1200,21 @@ EXPORT_SYMBOL(drm_plane_index); */ void drm_plane_force_disable(struct drm_plane *plane) {

• struct drm_framebuffer *old_fb = plane->fb; int ret;

• if (!old_fb)

• if (!plane->fb) return;

• plane->old_fb = plane->fb; ret = plane->funcs->disable_plane(plane); if (ret) { DRM_ERROR("failed to disable plane with busy fb\n");

• plane->old_fb = NULL;
  

  return; } /* disconnect the plane from the fb and crtc: */

• __drm_framebuffer_unreference(old_fb);

• __drm_framebuffer_unreference(plane->old_fb);
• plane->old_fb = NULL; plane->fb = NULL; plane->crtc = NULL;

} @@ -2188,7 +2190,7 @@ static int setplane_internal(struct drm_plane *plane, uint32_t src_w, uint32_t src_h) { struct drm_device *dev = plane->dev;

• struct drm_framebuffer *old_fb = NULL;

• struct drm_framebuffer *old_fb;

I think there may be cases where old_fb gets unref'd without ever being set if we drop the NULL assignment. E.g., if the possible_crtcs test or the format test fail, we jump down to out and then test the value + unref which could be garbage.

Oops, totally missed that. And somehow also missed the gcc warning about unitialized usage of old_fb - that one was the reason why I've dropped the initializer. Looks like I've failed.

Would it be simpler to just drm_modeset_lock_all() unconditionally at the start of the function and then just unlock after the unrefs at the end of the function so that we don't need a local old_fb?

Yeah considered that and since you're suggesting this too I'll do it. Trying hard to not grab locks for the error case is fairly pointless optimization.

...

int ret = 0; unsigned int fb_width, fb_height; int i; @@ -2196,14 +2198,16 @@ static int setplane_internal(struct drm_plane *plane, /* No fb means shut it down */ if (!fb) { drm_modeset_lock_all(dev);

• old_fb = plane->fb;
  

• plane->old_fb = plane->fb;
  

  ret = plane->funcs->disable_plane(plane); if (!ret) { plane->crtc = NULL; plane->fb = NULL; } else {

• 	old_fb = NULL;
  

• 	plane->old_fb = NULL;
  

  }

• old_fb = plane->old_fb;
  

• plane->old_fb = NULL;
  

  drm_modeset_unlock_all(dev); goto out; }

@@ -2245,7 +2249,7 @@ static int setplane_internal(struct drm_plane *plane, }

drm_modeset_lock_all(dev);

• old_fb = plane->fb;

• plane->old_fb = plane->fb; ret = plane->funcs->update_plane(plane, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, src_x, src_y, src_w, src_h);

@@ -2254,8 +2258,10 @@ static int setplane_internal(struct drm_plane *plane, plane->fb = fb; fb = NULL; } else {

• old_fb = NULL;
  

• plane->old_fb = NULL;
  

  }
• old_fb = plane->old_fb;
• plane->old_fb = NULL; drm_modeset_unlock_all(dev);

out: @@ -2369,7 +2375,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set) * crtcs. Atomic modeset will have saner semantics ... */ list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head)

• tmp->old_fb = tmp->primary->fb;
  

• tmp->primary->old_fb = tmp->primary->fb;
  
  

  fb = set->fb;

@@ -2382,8 +2388,9 @@ int drm_mode_set_config_internal(struct drm_mode_set *set) list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) { if (tmp->primary->fb) drm_framebuffer_reference(tmp->primary->fb);

• if (tmp->old_fb)
  

• 	drm_framebuffer_unreference(tmp->old_fb);
  

• if (tmp->primary->old_fb)
  

• 	drm_framebuffer_unreference(tmp->primary->old_fb);
  

• tmp->primary->old_fb = NULL;
  

  }

  return ret;

@@ -4458,7 +4465,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, { struct drm_mode_crtc_page_flip *page_flip = data; struct drm_crtc *crtc;

• struct drm_framebuffer *fb = NULL, *old_fb = NULL;

• struct drm_framebuffer *fb = NULL; struct drm_pending_vblank_event *e = NULL; unsigned long flags; int ret = -EINVAL;

@@ -4530,7 +4537,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, (void (*) (struct drm_pending_event *)) kfree; }

• old_fb = crtc->primary->fb;

• crtc->primary->old_fb = crtc->primary->fb; ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags); if (ret) { if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {

@@ -4540,7 +4547,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, kfree(e); } /* Keep the old fb, don't unref it. */

• old_fb = NULL;
  

• crtc->primary->old_fb = NULL;
  

  } else { /*
  • Warn if the driver hasn't properly updated the crtc->fb

@@ -4556,8 +4563,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, out: if (fb) drm_framebuffer_unreference(fb);

• if (old_fb)

• drm_framebuffer_unreference(old_fb);
  

• if (crtc->primary->old_fb)

• drm_framebuffer_unreference(crtc->primary->old_fb);
  

• crtc->primary->old_fb = NULL; drm_modeset_unlock_crtc(crtc);

  return ret;

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index b0e30c5526ce..5fffb5c53ba6 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -341,10 +341,6 @@ struct drm_crtc { int cursor_x; int cursor_y;

• /* Temporary tracking of the old fb while a modeset is ongoing. Used

• * by drm_mode_set_config_internal to implement correct refcounting. */
  

• struct drm_framebuffer *old_fb;

• bool enabled;

  /* Requested mode from modesetting. */

@@ -622,6 +618,10 @@ struct drm_plane { struct drm_crtc *crtc; struct drm_framebuffer *fb;

• /* Temporary tracking of the old fb while a modeset is ongoing. Used

• * by drm_mode_set_config_internal to implement correct refcounting. */
  

Might want to update the wording of this comment slightly since it isn't just for drm_mode_set_config_internal (or modesets) anymore.

Good idea, will augment. -Daniel

Matt

...

• struct drm_framebuffer *old_fb;

• const struct drm_plane_funcs *funcs;

  struct drm_object_properties properties;

-- 2.0.1

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795

On Wed, Jul 30, 2014 at 08:56:07AM -0700, Matt Roper wrote:

On Tue, Jul 29, 2014 at 11:32:20PM +0200, Daniel Vetter wrote:

...

In the fbdev code we want to do trylocks only to avoid deadlocks and other ugly issues. Thus far we've only grabbed the overall modeset lock, but that already failed to exclude a pile of potential concurrent operations. With proper atomic support this will be worse.

So add a trylock mode to the modeset locking code which attempts all locks only with trylocks, if possible. We need to track this in the locking functions themselves and can't restrict this to drivers since driver-private w/w mutexes must be treated the same way.

There's still the issue that other driver private locks aren't handled here at all, but well can't have everything. With this we will at least not regress, even once atomic allows lots of concurrent kms activity.

Aside: We should move the acquire context to stack-based allocation in the callers to get rid of that awful WARN_ON(kmalloc_failed) control flow which just blows up when memory is short. But that's material for separate patches.

v2:

• Fix logic inversion fumble in the fb helper.
• Add proper kerneldoc.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch

drivers/gpu/drm/drm_fb_helper.c | 10 +++---- drivers/gpu/drm/drm_modeset_lock.c | 56 ++++++++++++++++++++++++++++++-------- include/drm/drm_modeset_lock.h | 6 ++++ 3 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 3144db9dc0f1..841e039ba028 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -419,11 +419,11 @@ static bool drm_fb_helper_force_kernel_mode(void) if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) continue;

• /* NOTE: we use lockless flag below to avoid grabbing other
  

•  * modeset locks.  So just trylock the underlying mutex
  

•  * directly:
  

• /*
  

•  * NOTE: Use trylock mode to avoid deadlocks and sleeping in
  

•  * panic context.
  

  */

• if (!mutex_trylock(&dev->mode_config.mutex)) {
  

• if (__drm_modeset_lock_all(dev, true) != 0) {
  error = true;
  continue;
  

  }

Can we succeed locking config->mutex and connection_mutex inside __drm_modeset_lock_all(), but then fail to lock one of the CRTC mutexes in drm_modeset_lock_all_crtcs()? If so, __drm_modeset_lock_all() will return -EBUSY, but not drop the locks it acquired, and then it seems like we can return from the function here without ever dropping locks.

Well it's the panic code, so after this the machine is officially dead anyway. The only thing left to do is get the dmesg out with as little risk as possible. -Daniel

On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel Dänzer wrote:

On 30.07.2014 06:32, Daniel Vetter wrote:

...

As usual in both a crtc index and a struct drm_crtc * version.

The function assumes that no one drivers their display below 10Hz, and it will complain if the vblank wait takes longer than that.

v2: Also check dev->max_vblank_counter since some drivers register a fake get_vblank_counter function.

What does that refer to? Can't find any other reference to max_vblank_counter in the patch.

Oops, that was from an intermediate version. The v3: text somehow got lost where I've switched the code from directly calling the ->get_counter callback to using drm_vblank_counter, which will dtrt even when there's not hw counter available. Will augment the commit message.

...

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 0de123afdb34..76024fdde452 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -999,6 +999,51 @@ void drm_crtc_vblank_put(struct drm_crtc *crtc) EXPORT_SYMBOL(drm_crtc_vblank_put);

/**

• • drm_vblank_wait - wait for one vblank
• • @dev: DRM device
• • @crtc: crtc index
• • This waits for one vblank to pass on @crtc, using the irq driver interfaces.
• • It is a failure to call this when the vblank irq for @crtc is disable, e.g.

Spelling: 'disabled'

...

• • due to lack of driver support or because the crtc is off.
• */

+void drm_vblank_wait(struct drm_device *dev, int crtc) +{

• int ret;
• u32 last;
• ret = drm_vblank_get(dev, crtc);
• if (WARN_ON(ret))

• return;
  

• last = drm_vblank_count(dev, crtc);

+#define C (last != drm_vblank_count(dev, crtc))

• ret = wait_event_timeout(dev->vblank[crtc].queue,

• 		 C, msecs_to_jiffies(100));
  

• WARN_ON(ret == 0);

+#undef C

What's the point of the C macro?

Usually the conditions tend to overflow the 80 char limit, so I've adopted that pattern of extracting it into a local #define. I think it'll fit here, so will roll in.

...

• drm_vblank_put(dev, crtc);

+} +EXPORT_SYMBOL(drm_vblank_wait);

+/**

• • drm_crtc_vblank_wait - wait for one vblank
• • @crtc: DRM crtc
• • This waits for one vblank to pass on @crtc, using the irq driver interfaces.
• • It is a failure to call this when the vblank irq for @crtc is disable, e.g.

Same typo as above.

...

• • due to lack of driver support or because the crtc is off.
• */

+void drm_crtc_vblank_wait(struct drm_crtc *crtc) +{

• drm_vblank_wait(crtc->dev, drm_crtc_index(crtc));

+} +EXPORT_SYMBOL(drm_crtc_vblank_wait);

+/**

Maybe the function names should be *_vblank_wait_next() or something to clarify the purpose and reduce potential confusion versus drm_wait_vblank().

Yeah that name is just transferred from the i915 driver. What about drm_wait_one_vblank()/drm_crtc_wait_one_vblank()? At least to my ear vblank_wait_next sounds backwards.

Looks good to me other than that.

If you're ok with the name suggestion I'll polish & resend, thanks for the comments. -Daniel

As usual in both a crtc index and a struct drm_crtc * version.

The function assumes that no one drivers their display below 10Hz, and it will complain if the vblank wait takes longer than that.

v2: Also check dev->max_vblank_counter since some drivers register a fake get_vblank_counter function.

v3: Use drm_vblank_count instead of calling the low-level ->get_vblank_counter callback. That way we'll get the sw-cooked counter for platforms without proper vblank support and so can ditch the max_vblank_counter check again.

v4: Review from Michel Dänzer: - Restore lost notes about v3: - Spelling in kerneldoc. - Inline wait_event condition. - s/vblank_wait/wait_one_vblank/

Cc: Michel Dänzer michel@daenzer.net Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_irq.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ include/drm/drmP.h | 2 ++ 2 files changed, 46 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 0de123afdb34..2605a5923fdb 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -999,6 +999,50 @@ void drm_crtc_vblank_put(struct drm_crtc *crtc) EXPORT_SYMBOL(drm_crtc_vblank_put);

/** + * drm_wait_one_vblank - wait for one vblank + * @dev: DRM device + * @crtc: crtc index + * + * This waits for one vblank to pass on @crtc, using the irq driver interfaces. + * It is a failure to call this when the vblank irq for @crtc is disabled, e.g. + * due to lack of driver support or because the crtc is off. + */ +void drm_wait_one_vblank(struct drm_device *dev, int crtc) +{ + int ret; + u32 last; + + ret = drm_vblank_get(dev, crtc); + if (WARN_ON(ret)) + return; + + last = drm_vblank_count(dev, crtc); + + ret = wait_event_timeout(dev->vblank[crtc].queue, + last != drm_vblank_count(dev, crtc), + msecs_to_jiffies(100)); + + WARN_ON(ret == 0); + + drm_vblank_put(dev, crtc); +} +EXPORT_SYMBOL(drm_wait_one_vblank); + +/** + * drm_crtc_wait_one_vblank - wait for one vblank + * @crtc: DRM crtc + * + * This waits for one vblank to pass on @crtc, using the irq driver interfaces. + * It is a failure to call this when the vblank irq for @crtc is disabled, e.g. + * due to lack of driver support or because the crtc is off. + */ +void drm_crtc_wait_one_vblank(struct drm_crtc *crtc) +{ + drm_wait_one_vblank(crtc->dev, drm_crtc_index(crtc)); +} +EXPORT_SYMBOL(drm_crtc_wait_one_vblank); + +/** * drm_vblank_off - disable vblank events on a CRTC * @dev: DRM device * @crtc: CRTC in question diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 06a673894c47..ee4c321455e8 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1355,6 +1355,8 @@ extern int drm_vblank_get(struct drm_device *dev, int crtc); extern void drm_vblank_put(struct drm_device *dev, int crtc); extern int drm_crtc_vblank_get(struct drm_crtc *crtc); extern void drm_crtc_vblank_put(struct drm_crtc *crtc); +extern void drm_wait_one_vblank(struct drm_device *dev, int crtc); +extern void drm_crtc_wait_one_vblank(struct drm_crtc *crtc); extern void drm_vblank_off(struct drm_device *dev, int crtc); extern void drm_vblank_on(struct drm_device *dev, int crtc); extern void drm_crtc_vblank_off(struct drm_crtc *crtc);

This has the upside that it will no longer steal interrupts from the interrutp handler on pre-g4x. Furthermore this will now scream properly on all platforms if we don't have hw counters enabled.

v2: Adjust to the new names.

Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/intel_display.c | 41 +----------------------------------- 1 file changed, 1 insertion(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1edfd1ae5b37..540fc762179f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -891,17 +891,6 @@ enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv, return intel_crtc->config.cpu_transcoder; }

-static void g4x_wait_for_vblank(struct drm_device *dev, int pipe) -{ - struct drm_i915_private *dev_priv = dev->dev_private; - u32 frame, frame_reg = PIPE_FRMCOUNT_GM45(pipe); - - frame = I915_READ(frame_reg); - - if (wait_for(I915_READ_NOTRACE(frame_reg) != frame, 50)) - WARN(1, "vblank wait timed out\n"); -} - /** * intel_wait_for_vblank - wait for vblank on a given pipe * @dev: drm device @@ -912,35 +901,7 @@ static void g4x_wait_for_vblank(struct drm_device *dev, int pipe) */ void intel_wait_for_vblank(struct drm_device *dev, int pipe) { - struct drm_i915_private *dev_priv = dev->dev_private; - int pipestat_reg = PIPESTAT(pipe); - - if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) { - g4x_wait_for_vblank(dev, pipe); - return; - } - - /* Clear existing vblank status. Note this will clear any other - * sticky status fields as well. - * - * This races with i915_driver_irq_handler() with the result - * that either function could miss a vblank event. Here it is not - * fatal, as we will either wait upon the next vblank interrupt or - * timeout. Generally speaking intel_wait_for_vblank() is only - * called during modeset at which time the GPU should be idle and - * should *not* be performing page flips and thus not waiting on - * vblanks... - * Currently, the result of us stealing a vblank from the irq - * handler is that a single frame will be skipped during swapbuffers. - */ - I915_WRITE(pipestat_reg, - I915_READ(pipestat_reg) | PIPE_VBLANK_INTERRUPT_STATUS); - - /* Wait for vblank interrupt bit to set */ - if (wait_for(I915_READ(pipestat_reg) & - PIPE_VBLANK_INTERRUPT_STATUS, - 50)) - DRM_DEBUG_KMS("vblank wait timed out\n"); + drm_wait_one_vblank(dev, pipe); }

static bool pipe_dsl_stopped(struct drm_device *dev, enum pipe pipe)