From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Add a bit of locking to intel_dp_hpd_pulse() Date: Mon, 4 Aug 2014 15:50:34 +0300 Message-ID: <20140804125034.GF4193@intel.com> References: <1406734764-19234-1-git-send-email-ville.syrjala@linux.intel.com> <20140801115522.GX4193@intel.com> <20140804081050.GK8727@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id E28CB6E3DC for ; Mon, 4 Aug 2014 05:50:38 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140804081050.GK8727@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: Dave Airlie , "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Mon, Aug 04, 2014 at 10:10:50AM +0200, Daniel Vetter wrote: > On Fri, Aug 01, 2014 at 02:55:22PM +0300, Ville Syrj=E4l=E4 wrote: > > On Thu, Jul 31, 2014 at 08:59:08PM +1000, Dave Airlie wrote: > > > On 31 July 2014 17:37, Daniel Vetter wrote: > > > > On Thu, Jul 31, 2014 at 1:49 AM, Dave Airlie wr= ote: > > > >> Daniel, the only way intel_dp->is_mst can get reset is inside this= path. > > > > > > > > Ok, so that one should be safe. Then I guess we can just push the > > > > locking down into the respective non-mst leafs (since atm we do > > > > link-retraining without any locking, which isn't good). And it needs > > > > to be dev->mode_config.mutex, not connection mutex. > > = > > Why that? We can't be doing a modeset w/o connection_mutex so that > > seems like it should be enough. Well, there's also dpms which leaves > > the crtc<->encoder<->connector links intact but that too takes > > connection_mutex currently. > > = > > > = > > > I'd like to know why we do link training at this point though as well, > > > adding locking is required of course, I was just going to wrap the > > > short irq call to the link status check with the lock, but I think it > > > should be possible to push it down further, > > = > > I don't really know why the sink generates the hpd when we turn off the > > port, but that doesn't really matter I think. We need to be prepared for > > hpds at any time. > > = > > intel_dp_check_link_status() just checks if there's a crtc, which there > > is (either the old one or the new one, depending on how far along the > > modeset path we are I guess), and then it just checks > > drm_dp_channel_eq_ok() which says false since the link was turned off, > > and then it proceeds to retrain the link. > > = > > Maybe it should also check crtc->active? Though that itself won't = > > eliminate the problem unless the locking gets fixed somehow. > = > We check encoder->connectors_active, which is equivalent. Only after intel_sanitize_encoder(). Before that we can have !crtc->active && encoder->connectors_active. -- = Ville Syrj=E4l=E4 Intel OTC