On Wed, 12 Aug 2015, Ander Conselvan De Oliveira <conselvan2@xxxxxxxxx> wrote:
> For both patches,
>
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@xxxxxxxxx>
Both pushed to drm-intel-fixes, thanks for the patches and review.
BR,
Jani.
>
> 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;
>>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
