From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lyude Paul Subject: Re: [Intel-gfx] [PATCH] drm/i915: Discard previous atomic state on resume if connectors change Date: Tue, 03 May 2016 11:08:43 -0400 Message-ID: <1462288123.7721.1.camel@redhat.com> References: <1462284220-14930-1-git-send-email-cpaul@redhat.com> <20160503142903.GL14148@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20160503142903.GL14148@phenom.ffwll.local> Sender: stable-owner@vger.kernel.org To: Daniel Vetter , David Airlie Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org, "open list:INTEL DRM DRIVERS excluding Poulsbo, Moorestow..., linux-kernel@vger.kernel.org open list" , Daniel Vetter List-Id: intel-gfx@lists.freedesktop.org Yeah airlied said the same thing. This patch is more intended for just = 4.6 since the refcounting patch isn't very likely to get into 4.6. On Tue, 2016-05-03 at 16:29 +0200, Daniel Vetter wrote: > On Tue, May 03, 2016 at 10:03:40AM -0400, Lyude wrote: > >=20 > > If an MST device is disconnected while the machine is suspended, th= e > > number of connectors will change as well after we call > > intel_dp_mst_resume(). This means that any previous atomic state we= had > > before suspending is no longer valid, since it'll still be pointing= to > > missing connectors. We need to check for this before committing the > > state, otherwise we'll kernel panic on resume whenever if any MST > > display was disconnected before we started resuming: > >=20 > > BUG: unable to handle kernel NULL pointer dereference at 0000000000= 000008 > > IP: [] drm_atomic_helper_check_modeset+0x29f/0xb4= 0 > > [drm_kms_helper] > > Call Trace: > > =C2=A0[] intel_atomic_check+0x34/0x1180 [i915] > > =C2=A0[] ? mark_held_locks+0x6f/0xa0 > > =C2=A0[] ? trace_hardirqs_on_caller+0x129/0x1b0 > > =C2=A0[] drm_atomic_check_only+0x192/0x620 [drm] > > =C2=A0[] ? pci_pm_thaw+0x21/0x90 > > =C2=A0[] drm_atomic_commit+0x17/0x60 [drm] > > =C2=A0[] intel_display_resume+0xbd/0x160 [i915] > > =C2=A0[] ? pci_pm_thaw+0x90/0x90 > > =C2=A0[] i915_drm_resume+0xd8/0x160 [i915] > > =C2=A0[] i915_pm_resume+0x25/0x30 [i915] > > =C2=A0[] pci_pm_resume+0x64/0xa0 > > =C2=A0[] dpm_run_callback+0x90/0x190 > > =C2=A0[] device_resume+0xd5/0x1f0 > > =C2=A0[] async_resume+0x1d/0x50 > > =C2=A0[] async_run_entry_fn+0x48/0x150 > > =C2=A0[] process_one_work+0x1e9/0x5c0 > > =C2=A0[] ? process_one_work+0x166/0x5c0 > > =C2=A0[] worker_thread+0x48/0x4e0 > > =C2=A0[] ? process_one_work+0x5c0/0x5c0 > > =C2=A0[] kthread+0xe4/0x100 > > =C2=A0[] ret_from_fork+0x22/0x50 > > =C2=A0[] ? kthread_create_on_node+0x200/0x200 > >=20 > > Cc: stable@vger.kernel.org > > Signed-off-by: Lyude > This should be addressed by the connector refcounting fixes Dave Airl= ie > has for 4.7 (not all merged yet though). Can you please retest with t= hose? > -Daniel >=20 > >=20 > > --- > > =C2=A0drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++ > > =C2=A01 file changed, 11 insertions(+) > >=20 > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 6e0d828..252c06c 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -15945,6 +15945,17 @@ void intel_display_resume(struct drm_devic= e *dev) > > =C2=A0 dev_priv->modeset_restore_state =3D NULL; > > =C2=A0 > > =C2=A0 /* > > + =C2=A0* With MST, the number of connectors can change between sus= pend > > and > > + =C2=A0* resume, which means that the state we want to restore mig= ht now > > be > > + =C2=A0* impossible to use since it'll be pointing to non-existant > > + =C2=A0* connectors. > > + =C2=A0*/ > > + if (state->num_connector !=3D dev->mode_config.num_connector) { > > + drm_atomic_state_free(state); > > + state =3D NULL; > > + } > > + > > + /* > > =C2=A0 =C2=A0* This is a cludge because with real atomic modeset > > mode_config.mutex > > =C2=A0 =C2=A0* won't be taken. Unfortunately some probed state like > > =C2=A0 =C2=A0* audio_codec_enable is still protected by mode_config= =2Emutex, so > > lock > > --=C2=A0 > > 2.5.5 > >=20 > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx