Commit d2944cf2 authored by Maarten Lankhorst's avatar Maarten Lankhorst Committed by Jani Nikula

drm/i915: Commit planes on each crtc separately.

This patch is based on the upstream commit 5ac1c4bc and amended
for v4.2 to make sure it works as intended.

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.

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 crtc->config->base.crtc update may seem unrelated, but the helper
will use it to obtain the crtc for the state. Without the update it
will dereference NULL and crash.

Changes since v1:
- Use Matt Roper's commit message.
Signed-off-by: default avatarMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: default avatarMatt Roper <matthew.d.roper@intel.com>
References: https://bugs.freedesktop.org/show_bug.cgi?id=90398Reviewed-by: default avatarAnder Conselvan de Oliveira <conselvan2@gmail.com>
Signed-off-by: default avatarJani Nikula <jani.nikula@intel.com>
parent f0fdc55d
...@@ -129,8 +129,9 @@ int intel_atomic_commit(struct drm_device *dev, ...@@ -129,8 +129,9 @@ int intel_atomic_commit(struct drm_device *dev,
struct drm_atomic_state *state, struct drm_atomic_state *state,
bool async) bool async)
{ {
int ret; struct drm_crtc_state *crtc_state;
int i; struct drm_crtc *crtc;
int ret, i;
if (async) { if (async) {
DRM_DEBUG_KMS("i915 does not yet support async commit\n"); DRM_DEBUG_KMS("i915 does not yet support async commit\n");
...@@ -142,48 +143,18 @@ int intel_atomic_commit(struct drm_device *dev, ...@@ -142,48 +143,18 @@ int intel_atomic_commit(struct drm_device *dev,
return ret; return ret;
/* Point of no return */ /* Point of no return */
drm_atomic_helper_swap_state(dev, state);
/*
* FIXME: The proper sequence here will eventually be:
*
* drm_atomic_helper_swap_state(dev, state)
* drm_atomic_helper_commit_modeset_disables(dev, state);
* drm_atomic_helper_commit_planes(dev, state);
* drm_atomic_helper_commit_modeset_enables(dev, state);
* drm_atomic_helper_wait_for_vblanks(dev, state);
* drm_atomic_helper_cleanup_planes(dev, state);
* drm_atomic_state_free(state);
*
* once we have full atomic modeset. For now, just manually update
* plane states to avoid clobbering good states with dummy states
* while nuclear pageflipping.
*/
for (i = 0; i < dev->mode_config.num_total_plane; i++) {
struct drm_plane *plane = state->planes[i];
if (!plane)
continue;
plane->state->state = state;
swap(state->plane_states[i], plane->state);
plane->state->state = NULL;
}
/* swap crtc_scaler_state */ /* swap crtc_scaler_state */
for (i = 0; i < dev->mode_config.num_crtc; i++) { for_each_crtc_in_state(state, crtc, crtc_state, i) {
struct drm_crtc *crtc = state->crtcs[i]; to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
if (!crtc) {
continue;
}
to_intel_crtc(crtc)->config->scaler_state =
to_intel_crtc_state(state->crtc_states[i])->scaler_state;
if (INTEL_INFO(dev)->gen >= 9) if (INTEL_INFO(dev)->gen >= 9)
skl_detach_scalers(to_intel_crtc(crtc)); skl_detach_scalers(to_intel_crtc(crtc));
drm_atomic_helper_commit_planes_on_crtc(crtc_state);
} }
drm_atomic_helper_commit_planes(dev, state);
drm_atomic_helper_wait_for_vblanks(dev, state); drm_atomic_helper_wait_for_vblanks(dev, state);
drm_atomic_helper_cleanup_planes(dev, state); drm_atomic_helper_cleanup_planes(dev, state);
drm_atomic_state_free(state); drm_atomic_state_free(state);
......
...@@ -12626,17 +12626,17 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, ...@@ -12626,17 +12626,17 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
modeset_update_crtc_power_domains(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. */ /* Now enable the clocks, plane, pipe, and connectors that we set up. */
for_each_crtc_in_state(state, crtc, crtc_state, i) { for_each_crtc_in_state(state, crtc, crtc_state, i) {
if (!needs_modeset(crtc->state) || !crtc->state->enable) if (!needs_modeset(crtc->state) || !crtc->state->enable) {
drm_atomic_helper_commit_planes_on_crtc(crtc_state);
continue; continue;
}
update_scanline_offset(to_intel_crtc(crtc)); update_scanline_offset(to_intel_crtc(crtc));
dev_priv->display.crtc_enable(crtc); dev_priv->display.crtc_enable(crtc);
intel_crtc_enable_planes(crtc); drm_atomic_helper_commit_planes_on_crtc(crtc_state);
} }
/* FIXME: add subpixel order */ /* FIXME: add subpixel order */
...@@ -13269,7 +13269,7 @@ intel_check_primary_plane(struct drm_plane *plane, ...@@ -13269,7 +13269,7 @@ intel_check_primary_plane(struct drm_plane *plane,
if (IS_BROADWELL(dev)) if (IS_BROADWELL(dev))
intel_crtc->atomic.wait_vblank = true; intel_crtc->atomic.wait_vblank = true;
if (crtc_state && !needs_modeset(&crtc_state->base)) if (crtc_state)
intel_crtc->atomic.post_enable_primary = true; intel_crtc->atomic.post_enable_primary = true;
} }
...@@ -15004,6 +15004,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) ...@@ -15004,6 +15004,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
struct intel_plane_state *plane_state; struct intel_plane_state *plane_state;
memset(crtc->config, 0, sizeof(*crtc->config)); memset(crtc->config, 0, sizeof(*crtc->config));
crtc->config->base.crtc = &crtc->base;
crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE; crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment