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

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

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

->hpd_pulse() is called from a workqueue via an interrupt so it happens
asynchronously with modesets. Grab the connection_mutex in
intel_dp_hpd_pulse() to avoid disturbing on angoing modeset with
parallel hpd processing.

On my IVB turning off the port during a modeset could result in a
hpd which would then proceed to link train while the modeset was
still happening. Suffice to say that didn't go so well.

Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 87d0489..cd63e74 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4051,6 +4051,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 	DRM_DEBUG_KMS("got hpd irq on port %d - %s\n", intel_dig_port->port,
 		      long_hpd ? "long" : "short");
 
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+
 	if (long_hpd) {
 		if (!ibx_digital_port_connected(dev_priv, intel_dig_port))
 			goto mst_fail;
@@ -4079,6 +4081,7 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			intel_dp_check_link_status(intel_dp);
 		}
 	}
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 	return false;
 mst_fail:
 	/* if we were in MST mode, and device is not there get out of MST mode */
@@ -4087,6 +4090,7 @@ mst_fail:
 		intel_dp->is_mst = false;
 		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
 	}
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 	return true;
 }
 
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dave Airlie]

>
> ->hpd_pulse() is called from a workqueue via an interrupt so it happens
> asynchronously with modesets. Grab the connection_mutex in
> intel_dp_hpd_pulse() to avoid disturbing on angoing modeset with
> parallel hpd processing.

Nope, not right, we need to lock just not that amount of stuff, since MST
won't work anymore, since it relies on hpd irqs while other threads are
holding the locks to talk to the remote devices. I also think we shouldn't be
locking the the irq processing in intel_dp_check_link_status.

> On my IVB turning off the port during a modeset could result in a
> hpd which would then proceed to link train while the modeset was
> still happening. Suffice to say that didn't go so well.

Does this not point to an ordering issue somewhere, we've turned off the port,
but the device thinks the link should be trained but it currently isn't? maybe
intel_dp_check_link_status needs to be smarter about something
but we should be able to process the irqs fine without locking the modesetting
locks. Or maybe we need to know that we don't want the link trained so don't
retrain when the remote device signals a link status problem.

Daniel, the only way intel_dp->is_mst can get reset is inside this path.

Dave.

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

[Daniel Vetter]

On Thu, Jul 31, 2014 at 1:49 AM, Dave Airlie <airlied@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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Dave Airlie]

On 31 July 2014 17:37, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Jul 31, 2014 at 1:49 AM, Dave Airlie <airlied@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.

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'm not sure how vague the spec is on what should happen on HPDs, but
if we drop the port clock we obviously will lose the link, but we
should also know not to be retraining it at that point anyways.

Dave.

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

[Ville Syrjälä]

On Thu, Jul 31, 2014 at 08:59:08PM +1000, Dave Airlie wrote:
> On 31 July 2014 17:37, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Jul 31, 2014 at 1:49 AM, Dave Airlie <airlied@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.

> 
> I'm not sure how vague the spec is on what should happen on HPDs, but
> if we drop the port clock we obviously will lose the link, but we
> should also know not to be retraining it at that point anyways.
> 
> Dave.

-- 
Ville Syrjälä
Intel OTC

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

[Daniel Vetter]

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@ffwll.ch> wrote:
> > > On Thu, Jul 31, 2014 at 1:49 AM, Dave Airlie <airlied@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.
-Daniel

> 
> > 
> > I'm not sure how vague the spec is on what should happen on HPDs, but
> > if we drop the port clock we obviously will lose the link, but we
> > should also know not to be retraining it at that point anyways.
> > 
> > Dave.
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Dave Airlie]

On 4 August 2014 18:10, Daniel Vetter <daniel@ffwll.ch> 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@ffwll.ch> wrote:
>> > > On Thu, Jul 31, 2014 at 1:49 AM, Dave Airlie <airlied@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.
> -Daniel

We still check that. if you mean intel_encoders->connectors_active.

Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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@ffwll.ch> wrote:
> > > > On Thu, Jul 31, 2014 at 1:49 AM, Dave Airlie <airlied@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.

-- 
Ville Syrjälä
Intel OTC

Re: [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@ffwll.ch> wrote:
> > > > > On Thu, Jul 31, 2014 at 1:49 AM, Dave Airlie <airlied@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