For both patches,
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@xxxxxxxxx>
On Tue, 2015-08-11 at 12:31 +0200, Maarten Lankhorst wrote:
> This patch is based on the upstream commit 5ac1c4bcf073ad 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: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx #v4.2
> References: https://bugs.freedesktop.org/show_bug.cgi?id=90398
> ---
> drivers/gpu/drm/i915/intel_atomic.c | 45 +++++++-----------------------------
> drivers/gpu/drm/i915/intel_display.c | 11 +++++----
> 2 files changed, 14 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 7ed8033aae60..8e35e0d013df 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -129,8 +129,9 @@ int intel_atomic_commit(struct drm_device *dev,
> struct drm_atomic_state *state,
> bool async)
> {
> - int ret;
> - int i;
> + struct drm_crtc_state *crtc_state;
> + struct drm_crtc *crtc;
> + int ret, i;
>
> if (async) {
> DRM_DEBUG_KMS("i915 does not yet support async commit\n");
> @@ -142,48 +143,18 @@ int intel_atomic_commit(struct drm_device *dev,
> return ret;
>
> /* Point of no return */
> -
> - /*
> - * 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;
> - }
> + drm_atomic_helper_swap_state(dev, state);
>
> /* swap crtc_scaler_state */
> - for (i = 0; i < dev->mode_config.num_crtc; i++) {
> - struct drm_crtc *crtc = state->crtcs[i];
> - if (!crtc) {
> - continue;
> - }
> -
> - to_intel_crtc(crtc)->config->scaler_state =
> - to_intel_crtc_state(state->crtc_states[i])->scaler_state;
> + for_each_crtc_in_state(state, crtc, crtc_state, i) {
> + to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
>
> if (INTEL_INFO(dev)->gen >= 9)
> 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_cleanup_planes(dev, state);
> drm_atomic_state_free(state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c2579ded0c36..b920f88ccff8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12624,17 +12624,17 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>
> 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) {
> - 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;
> + }
>
> update_scanline_offset(to_intel_crtc(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 */
> @@ -13267,7 +13267,7 @@ intel_check_primary_plane(struct drm_plane *plane,
> if (IS_BROADWELL(dev))
> intel_crtc->atomic.wait_vblank = true;
>
> - if (crtc_state && !needs_modeset(&crtc_state->base))
> + if (crtc_state)
> intel_crtc->atomic.post_enable_primary = true;
> }
>
> @@ -15002,6 +15002,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;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
