[Intel-gfx] [PATCH v2] drm/i915/dp: Update connector status for DP MST hotplugs

[Daniel Vetter]

On Thu, Nov 17, 2016 at 01:53:31AM +0000, Pandiyan, Dhinakaran wrote:
> On Sun, 2016-11-13 at 11:39 +0100, Daniel Vetter wrote:
> > On Fri, Nov 11, 2016 at 10:21:39PM +0100, Daniel Vetter wrote:
> > > On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote:
> > > > Hotplugging a monitor in DP MST configuration triggers short pulses.
> > > > Although the short pulse handling path ends up in the MST hotplug function,
> > > > we do not perform a detect before sending the hotplug uvent. This leads to
> > > > the connector status not being updated when the userspace calls
> > > > DRM_IOCTL_MODE_GETCONNECTOR.
> > > > 
> > > > So, let's call the connector function ->detect() to update the connector
> > > > status before sending the uevent.
> > > > 
> > > > v2: Update connector status inside mode_config mutex (Ville)
> > > > 
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > 
> > > quick comments from me on irc:
> > > 
> > > <danvet> dhnkrn, really tricky I think
> > > <danvet> dhnkrn, also feels wrong from an entire design pov
> > > <danvet> I'd expect the topology manager could tell us from which exact downstream port the short pulse came
> > > <danvet> which means we should be able to get at that connector without taking the global lock
> > > <danvet> i.e. at least avoid the for_each_connector
> > > <danvet> on the detect itself, I need to think
> > > <danvet> but we might need to essentially postpone the ->detect work to the hotplug worker
> > > <danvet> except that it's not a direct hpd from our chip
> > > <danvet> but a downstream one
> > > <danvet> this is going to be fun
> > > <dhnkrn> Hmm .. Does updating the connector status need the lock or is it the detect() ?
> > > <danvet> yup
> > > <danvet> and we might need to do additional dpcd transactions (e.g. for load detect probing on downstream ports)
> > > <danvet> or at least edid reads
> > > <danvet> both recurse back into dp mst helper code like ville said
> > > <danvet> dhnkrn, so instead of doing the detect work from the hotplug code
> > > <danvet> put them on some list
> > > <danvet> need to be careful in case they are already there
> > > <danvet> list only protected with dedicated lock
> > > <danvet> then launch the hpd worker
> > > <danvet> and walk that list from there, with the mode_config.mutex to do the full-on probe
> > > <danvet> we can't call ->detect from dig_port_work, and walking the entire connector list in there is also not a good idea imo
> > > <danvet> dhnkrn, but that's just my preliminary analysis from like 10 minutes of thinking and checking what ville raised
> > > <danvet> needs more thought probably
> > > <danvet> dhnkrn, to clarify your question: both connector->status and calling ->detect need mode_config.mutex
> > > <danvet> that mutex is what protects output probe state<Vtec234> ccr: i might have udev configured incorrectly then
> > > 
> > > Cheers, Daniel
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++-
> > > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > index 3ffbd69..8e36a96 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> > > >  	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
> > > >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> > > > +	struct intel_connector *intel_connector;
> > > > +	bool changed = false;
> > > > +	enum drm_connector_status old_status;
> > > > +	struct drm_mode_config *mode_config = &dev->mode_config;
> > > > +
> > > > +	mutex_lock(&mode_config->mutex);
> > > > +	for_each_intel_connector(dev, intel_connector) {
> > > > +		struct drm_connector *connector = &(intel_connector->base);
> > > > +
> > > > +		if (intel_connector->mst_port != intel_dp)
> > > > +			continue;
> > > > +
> > > > +		old_status = connector->status;
> > > > +		connector->status = connector->funcs->detect(connector, false);
> > > > +
> > > > +		if (old_status != connector->status)
> > > > +			changed = true;
> > > > +	}
> > > > +	mutex_unlock(&mode_config->mutex);
> > > >  
> > > > -	drm_kms_helper_hotplug_event(dev);
> > 
> > I just realized that this call here will call down into the fbdev helper,
> > which already does the above ->detect calls. At least if there's no
> > compositor running. So I wonder why the deadlock ville pointed out isn't
> > blowing up already ...
> > -Daniel
> > 
> 
> You are right, that call sequence does end up taking the
> mode_config.mutex lock recursively. Are you referring to CI results for
> this patch?

Well I'm confused, since per our discussion here it should blow up, but it
doesn't. What are we missing? CI can't be trusted on this since we can't
(yet) do MST hotplug cycles, hence it doesn't exercise this codepath.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch