Re: [PATCH v3 13/22] drm/i915: Swap planes on each crtc separately.

[Matt Roper <matthew.d.roper@intel.com>]

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

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx