• Maarten Lankhorst's avatar
    drm/i915: Commit planes on each crtc separately. · d2944cf2
    Maarten Lankhorst authored
    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>
    d2944cf2
intel_atomic.c 11.6 KB