| Message ID | 1432137874-20543-4-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 20, 2015 at 06:04:25PM +0200, maarten.lankhorst@linux.intel.com wrote: > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Repeated calls to begin_crtc_commit can cause warnings like this: > [ 169.127746] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:616 > [ 169.127835] in_atomic(): 0, irqs_disabled(): 1, pid: 1947, name: kms_flip > [ 169.127840] 3 locks held by kms_flip/1947: > [ 169.127843] #0: (&dev->mode_config.mutex){+.+.+.}, at: [<ffffffff814774bc>] __drm_modeset_lock_all+0x9c/0x130 > [ 169.127860] #1: (crtc_ww_class_acquire){+.+.+.}, at: [<ffffffff814774cd>] __drm_modeset_lock_all+0xad/0x130 > [ 169.127870] #2: (crtc_ww_class_mutex){+.+.+.}, at: [<ffffffff81477178>] drm_modeset_lock+0x38/0x110 > [ 169.127879] irq event stamp: 665690 > [ 169.127882] hardirqs last enabled at (665689): [<ffffffff817ffdb5>] _raw_spin_unlock_irqrestore+0x55/0x70 > [ 169.127889] hardirqs last disabled at (665690): [<ffffffffc0197a23>] intel_pipe_update_start+0x113/0x5c0 [i915] > [ 169.127936] softirqs last enabled at (665470): [<ffffffff8108a766>] __do_softirq+0x236/0x650 > [ 169.127942] softirqs last disabled at (665465): [<ffffffff8108ae75>] irq_exit+0xc5/0xd0 > [ 169.127951] CPU: 1 PID: 1947 Comm: kms_flip Not tainted 4.1.0-rc4-patser+ #4039 > [ 169.127954] Hardware name: LENOVO 2349AV8/2349AV8, BIOS G1ETA5WW (2.65 ) 04/15/2014 > [ 169.127957] ffff8800c49036f0 ffff8800cde5fa28 ffffffff817f6907 0000000080000001 > [ 169.127964] 0000000000000000 ffff8800cde5fa58 ffffffff810aebed 0000000000000046 > [ 169.127970] ffffffff81c5d518 0000000000000268 0000000000000000 ffff8800cde5fa88 > [ 169.127981] Call Trace: > [ 169.127992] [<ffffffff817f6907>] dump_stack+0x4f/0x7b > [ 169.128001] [<ffffffff810aebed>] ___might_sleep+0x16d/0x270 > [ 169.128008] [<ffffffff810aed38>] __might_sleep+0x48/0x90 > [ 169.128017] [<ffffffff817fc359>] mutex_lock_nested+0x29/0x410 > [ 169.128073] [<ffffffffc01635f0>] ? vgpu_write64+0x220/0x220 [i915] > [ 169.128138] [<ffffffffc017fddf>] ? ironlake_update_primary_plane+0x2ff/0x410 [i915] > [ 169.128198] [<ffffffffc0190e75>] intel_frontbuffer_flush+0x25/0x70 [i915] > [ 169.128253] [<ffffffffc01831ac>] intel_finish_crtc_commit+0x4c/0x180 [i915] > [ 169.128279] [<ffffffffc00784ac>] drm_atomic_helper_commit_planes+0x12c/0x240 [drm_kms_helper] > [ 169.128338] [<ffffffffc0184264>] __intel_set_mode+0x684/0x830 [i915] > [ 169.128378] [<ffffffffc018a84a>] intel_crtc_set_config+0x49a/0x620 [i915] > [ 169.128385] [<ffffffff817fdd39>] ? mutex_unlock+0x9/0x10 > [ 169.128391] [<ffffffff81467b69>] drm_mode_set_config_internal+0x69/0x120 > [ 169.128398] [<ffffffff8119b547>] ? might_fault+0x57/0xb0 > [ 169.128403] [<ffffffff8146bf93>] drm_mode_setcrtc+0x253/0x620 > [ 169.128409] [<ffffffff8145c600>] drm_ioctl+0x1a0/0x6a0 > [ 169.128415] [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50 > [ 169.128424] [<ffffffff811e9ab8>] do_vfs_ioctl+0x2f8/0x530 > [ 169.128429] [<ffffffff810d0fcd>] ? trace_hardirqs_on+0xd/0x10 > [ 169.128435] [<ffffffff812e7676>] ? selinux_file_ioctl+0x56/0x100 > [ 169.128439] [<ffffffff811e9d71>] SyS_ioctl+0x81/0xa0 > [ 169.128445] [<ffffffff81800697>] system_call_fastpath+0x12/0x6f > > Solve it by using the newly introduced drm_atomic_helper_commit_planes_on_crtc. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> This commit looks good, but I think you might want to elaborate on the commit message a bit for people not super familiar with the atomic work. The problem here was that the drm_atomic_helper_commit_planes() helper we were using was basically designed to do begin_crtc_commit(crtc #1) begin_crtc_commit(crtc #2) ... commit all planes finish_crtc_commit(crtc #1) finish_crtc_commit(crtc #2) The problem here is that since our hardware relies on vblank evasion, our CRTC 'begin' function waits until we're out of the danger zone in which register writes might wind up straddling the vblank, then disables interrupts; our 'finish' function re-enables interrupts after the registers have been written. The expectation is that the operations between 'begin' and 'end' must be performed without sleeping (since interrupts are disabled) and should happen as quickly as possible. By clumping all of the 'begin' calls together, we introducing a couple problems: * Subsequent 'begin' invocations might sleep (which is illegal) * The first 'begin' ensured that we were far enough from the vblank that we could write our registers safely and ensure they all fell within the same frame. Adding extra delay waiting for subsequent CRTC's wasn't accounted for and could put us back into the 'danger zone' for CRTC #1. This commit solves the problem by using a new helper that allows an order of operations like: for each crtc { begin_crtc_commit(crtc) // sleep (maybe), then disable interrupts commit planes for this specific CRTC end_crtc_commit(crtc) // reenable interrupts } so that sleeps will only be performed while interrupts are enabled and we can be sure that registers for a CRTC will be written immediately once we know we're in the safe zone. The only other thing I noted is that the CRTC backpointer update in intel_modeset_readout_hw_state() seems somewhat unrelated that might have fit better in one of the earlier patches (but is necessary at this point since otherwise the helper will dereference NULL). You might want to make a quick note about that. Matt > --- > drivers/gpu/drm/i915/intel_atomic.c | 13 +++---------- > drivers/gpu/drm/i915/intel_display.c | 5 +++-- > 2 files changed, 6 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index e93aade0f2dc..83078763ba14 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -151,18 +151,11 @@ int intel_atomic_commit(struct drm_device *dev, > > if (INTEL_INFO(dev)->gen >= 9) > skl_detach_scalers(to_intel_crtc(crtc)); > + > + drm_atomic_helper_commit_planes_on_crtc(crtc_state); > } > > - /* > - * FIXME: The proper sequence here will eventually be: > - * > - * drm_atomic_helper_commit_modeset_disables(dev, state); > - * drm_atomic_helper_commit_planes(dev, state); > - * drm_atomic_helper_commit_modeset_enables(dev, state); > - * > - * once we have full atomic modeset. > - */ > - drm_atomic_helper_commit_planes(dev, state); > + /* FIXME: This function should eventually call __intel_set_mode when needed */ > > drm_atomic_helper_wait_for_vblanks(dev, state); > drm_atomic_helper_cleanup_planes(dev, state); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index adb19c3c9172..81d5358efdde 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12225,10 +12225,10 @@ static int __intel_set_mode(struct drm_atomic_state *state) > > modeset_update_crtc_power_domains(state); > > - drm_atomic_helper_commit_planes(dev, state); > - > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > for_each_crtc_in_state(state, crtc, crtc_state, i) { > + drm_atomic_helper_commit_planes_on_crtc(crtc_state); > + > if (!needs_modeset(crtc->state) || !crtc->state->active) > continue; > > @@ -14572,6 +14572,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > struct intel_plane_state *plane_state; > > memset(crtc->config, 0, sizeof(*crtc->config)); > + crtc->config->base.crtc = &crtc->base; > > crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE; > > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index e93aade0f2dc..83078763ba14 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -151,18 +151,11 @@ int intel_atomic_commit(struct drm_device *dev, if (INTEL_INFO(dev)->gen >= 9) skl_detach_scalers(to_intel_crtc(crtc)); + + drm_atomic_helper_commit_planes_on_crtc(crtc_state); } - /* - * FIXME: The proper sequence here will eventually be: - * - * drm_atomic_helper_commit_modeset_disables(dev, state); - * drm_atomic_helper_commit_planes(dev, state); - * drm_atomic_helper_commit_modeset_enables(dev, state); - * - * once we have full atomic modeset. - */ - drm_atomic_helper_commit_planes(dev, state); + /* FIXME: This function should eventually call __intel_set_mode when needed */ drm_atomic_helper_wait_for_vblanks(dev, state); drm_atomic_helper_cleanup_planes(dev, state); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index adb19c3c9172..81d5358efdde 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12225,10 +12225,10 @@ static int __intel_set_mode(struct drm_atomic_state *state) modeset_update_crtc_power_domains(state); - drm_atomic_helper_commit_planes(dev, state); - /* Now enable the clocks, plane, pipe, and connectors that we set up. */ for_each_crtc_in_state(state, crtc, crtc_state, i) { + drm_atomic_helper_commit_planes_on_crtc(crtc_state); + if (!needs_modeset(crtc->state) || !crtc->state->active) continue; @@ -14572,6 +14572,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) struct intel_plane_state *plane_state; memset(crtc->config, 0, sizeof(*crtc->config)); + crtc->config->base.crtc = &crtc->base; crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;