Re: [PATCH 2/2] drm/i915: Commit planes on each crtc separately.

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 12 Aug 2015, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote:
> For both patches,
>
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

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@linux.intel.com>
>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> Cc: stable@vger.kernel.org #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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx