* [PATCH 0/2] Two patches for pageflips @ 2012-12-17 15:58 Daniel Vetter 2012-12-17 15:58 ` [PATCH 1/2] drm/fb_helper: check whether fbcon is bound Daniel Vetter 2012-12-17 15:59 ` [PATCH 2/2] drm/i915: wake up all pageflip waiters Daniel Vetter 0 siblings, 2 replies; 6+ messages in thread From: Daniel Vetter @ 2012-12-17 15:58 UTC (permalink / raw) To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development Hi all, So in the course of beating the pageflip paths senseless to test the new locking scheme I've hunted down some bugs on g33. Sometimes we seemingly time out waiting for the event - apparently the pageflip irq is lost somewhere. I've tried to dig in, but lost myself completely in the rather fickle vblank mess in drm_irq.c. To not write off the last few days completely, I've figured I'll submit the two sane patches for inclusion - the first should fix fbcon doing stupid things, the second is purely paranoia (I hope at least). Imho longterm we need to seriously look at the current vblank stuff - since it operates on pipe ides instead of modeset crtc objects it's all rather fiddly and easily falls appart. But keeping both ums and kms paths working is a challenge ... Comments highly welcome. Yours, Daniel Daniel Vetter (2): drm/fb_helper: check whether fbcon is bound drm/i915: wake up all pageflip waiters drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++--------- drivers/gpu/drm/i915/intel_display.c | 2 +- 2 files changed, 30 insertions(+), 11 deletions(-) -- 1.7.10.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] drm/fb_helper: check whether fbcon is bound 2012-12-17 15:58 [PATCH 0/2] Two patches for pageflips Daniel Vetter @ 2012-12-17 15:58 ` Daniel Vetter 2012-12-17 15:59 ` [PATCH 2/2] drm/i915: wake up all pageflip waiters Daniel Vetter 1 sibling, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2012-12-17 15:58 UTC (permalink / raw) To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development We need to make sure that the fbcon is still bound when touching the hw, since otherwise we might corrupt the modeset state of kms clients. X mostly works around that with VT switching and setting the VT into raw mode, which disables most fbcon events. Raw kms test programs though don't do that dance, and in the future we might want to aim to abolish CONFIG_VT anyway. So improve preventive measures a bit. To do so, extract the existing logic for handling hotplug events (which X can't block with the current set of tricks) and reuse it for the fbdev blanking helper. Long-term we really need to either scrap this all and only have a OOPS console, or come up with a saner model for device ownership sharing between fbdev/fbcon and kms userspace. And for those wondering whether this check can't be added to fb_set_par e.g. to prevent undue panning - drm initialization locking is too screwed up for that to work out, I'm sorry :( Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_fb_helper.c | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index be0f2d6..0c6e25e 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -305,6 +305,24 @@ void drm_fb_helper_restore(void) } EXPORT_SYMBOL(drm_fb_helper_restore); +static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper) +{ + struct drm_device *dev = fb_helper->dev; + struct drm_crtc *crtc; + int bound = 0, crtcs_bound = 0; + + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + if (crtc->fb) + crtcs_bound++; + if (crtc->fb == fb_helper->fb) + bound++; + } + + if (bound < crtcs_bound) + return false; + return true; +} + #ifdef CONFIG_MAGIC_SYSRQ static void drm_fb_helper_restore_work_fn(struct work_struct *ignored) { @@ -338,6 +356,11 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) * For each CRTC in this fb, turn the connectors on/off. */ drm_modeset_lock_all(dev); + if (!drm_fb_helper_is_bound(fb_helper)) { + drm_modeset_unlock_all(dev); + return; + } + for (i = 0; i < fb_helper->crtc_count; i++) { crtc = fb_helper->crtc_info[i].mode_set.crtc; @@ -702,6 +725,11 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, int i; drm_modeset_lock_all(dev); + if (!drm_fb_helper_is_bound(fb_helper)) { + drm_modeset_unlock_all(dev); + return -EBUSY; + } + for (i = 0; i < fb_helper->crtc_count; i++) { crtc = fb_helper->crtc_info[i].mode_set.crtc; @@ -1369,21 +1397,12 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) struct drm_device *dev = fb_helper->dev; int count = 0; u32 max_width, max_height, bpp_sel; - int bound = 0, crtcs_bound = 0; - struct drm_crtc *crtc; if (!fb_helper->fb) return 0; drm_modeset_lock_all(dev); - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { - if (crtc->fb) - crtcs_bound++; - if (crtc->fb == fb_helper->fb) - bound++; - } - - if (bound < crtcs_bound) { + if (!drm_fb_helper_is_bound(fb_helper)) { fb_helper->delayed_hotplug = true; drm_modeset_unlock_all(dev); return 0; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/i915: wake up all pageflip waiters 2012-12-17 15:58 [PATCH 0/2] Two patches for pageflips Daniel Vetter 2012-12-17 15:58 ` [PATCH 1/2] drm/fb_helper: check whether fbcon is bound Daniel Vetter @ 2012-12-17 15:59 ` Daniel Vetter 2012-12-17 19:24 ` [Intel-gfx] " Chris Wilson 1 sibling, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2012-12-17 15:59 UTC (permalink / raw) To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development Otherwise it seems like we can get stuck with concurrent waiters. Right now this /shouldn't/ be a problem, since all pending pageflip waiters are serialized by the one mode_config.mutex, so there's at most on waiter. But better paranoid than sorry, since this is tricky code. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6056972..f49fd2a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6788,7 +6788,7 @@ static void do_intel_finish_page_flip(struct drm_device *dev, obj = work->old_fb_obj; - wake_up(&dev_priv->pending_flip_queue); + wake_up_all(&dev_priv->pending_flip_queue); queue_work(dev_priv->wq, &work->work); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] drm/i915: wake up all pageflip waiters 2012-12-17 15:59 ` [PATCH 2/2] drm/i915: wake up all pageflip waiters Daniel Vetter @ 2012-12-17 19:24 ` Chris Wilson 2012-12-20 20:24 ` [PATCH] " Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Chris Wilson @ 2012-12-17 19:24 UTC (permalink / raw) To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development On Mon, 17 Dec 2012 16:59:00 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Otherwise it seems like we can get stuck with concurrent waiters. > Right now this /shouldn't/ be a problem, since all pending pageflip > waiters are serialized by the one mode_config.mutex, so there's at > most on waiter. But better paranoid than sorry, since this is tricky > code. Hmm. BUG_ON(waitqueue_active()) before joining the waitqueue? -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] drm/i915: wake up all pageflip waiters 2012-12-17 19:24 ` [Intel-gfx] " Chris Wilson @ 2012-12-20 20:24 ` Daniel Vetter 2013-01-10 20:37 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2012-12-20 20:24 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter Otherwise it seems like we can get stuck with concurrent waiters. Right now this /shouldn't/ be a problem, since all pending pageflip waiters are serialized by the one mode_config.mutex, so there's at most on waiter. But better paranoid than sorry, since this is tricky code. v2: WARN_ON(waitqueue_active) before waiting, as suggested by Chris Wilson. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 390da00..74f3941 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2221,6 +2221,8 @@ intel_finish_fb(struct drm_framebuffer *old_fb) bool was_interruptible = dev_priv->mm.interruptible; int ret; + WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue)); + wait_event(dev_priv->pending_flip_queue, atomic_read(&dev_priv->mm.wedged) || atomic_read(&obj->pending_flip) == 0); @@ -2888,6 +2890,8 @@ static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) if (crtc->fb == NULL) return; + WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue)); + wait_event(dev_priv->pending_flip_queue, !intel_crtc_has_pending_flip(crtc)); @@ -6833,7 +6837,7 @@ static void do_intel_finish_page_flip(struct drm_device *dev, obj = work->old_fb_obj; - wake_up(&dev_priv->pending_flip_queue); + wake_up_all(&dev_priv->pending_flip_queue); queue_work(dev_priv->wq, &work->work); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: wake up all pageflip waiters 2012-12-20 20:24 ` [PATCH] " Daniel Vetter @ 2013-01-10 20:37 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2013-01-10 20:37 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter On Thu, Dec 20, 2012 at 09:24:07PM +0100, Daniel Vetter wrote: > Otherwise it seems like we can get stuck with concurrent waiters. > Right now this /shouldn't/ be a problem, since all pending pageflip > waiters are serialized by the one mode_config.mutex, so there's at > most on waiter. But better paranoid than sorry, since this is tricky > code. > > v2: WARN_ON(waitqueue_active) before waiting, as suggested by Chris > Wilson. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Queued for next, with an irc-ack from Chris. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-01-10 20:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-17 15:58 [PATCH 0/2] Two patches for pageflips Daniel Vetter 2012-12-17 15:58 ` [PATCH 1/2] drm/fb_helper: check whether fbcon is bound Daniel Vetter 2012-12-17 15:59 ` [PATCH 2/2] drm/i915: wake up all pageflip waiters Daniel Vetter 2012-12-17 19:24 ` [Intel-gfx] " Chris Wilson 2012-12-20 20:24 ` [PATCH] " Daniel Vetter 2013-01-10 20:37 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.