[Intel-gfx] [PATCH] drm/i915: Add a bit of locking to intel_dp_hpd_pulse()

[Daniel Vetter]

On Mon, Aug 04, 2014 at 03:50:34PM +0300, Ville Syrjälä wrote:
> 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älä wrote:
> > > On Thu, Jul 31, 2014 at 08:59:08PM +1000, Dave Airlie wrote:
> > > > On 31 July 2014 17:37, Daniel Vetter <daniel at ffwll.ch> wrote:
> > > > > On Thu, Jul 31, 2014 at 1:49 AM, Dave Airlie <airlied at gmail.com> wrote:
> > > > >> 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.

We grab all modeset locks when calling modeset_setup_hw_state, which means
no one will ever notice the inconsistent state between state readout and
sanitizing it. I think we're safe.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch