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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 11, 2016 at 08:43:53PM +0000, Pandiyan, Dhinakaran wrote:
> On Tue, 2016-11-08 at 13:04 +0200, Ville Syrjälä 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@intel.com>
> > > ---
> > >  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);
> > 
> > To help reviewers it might be nice to have a short explanation of the
> > locking situation in the commit message, mainly why is it safe to simply
> > take mode_config.mutex here.
> > 
> 
> I can't convince myself this locking is correct yet.
> drm_dp_send_link_address(), one of the callers of this function is
> recursive.
> drm_dp_send_link_address()->drm_dp_add_port()->drm_dp_send_link_address()

Hmm. I wonder how deep that can get. As in is there an upper bound on
the stack usage...

> 
> I am wondering if hpd_pulse should return IRQ_NONE so that we can have
> ->detect() called from the hotplug work function. For this to work,
> we'll need to change drm_dp_mst_hpd_irq().

Does the topology manager do all the processing in a blocking fashion
from the dig_port_work, or does is schedule additional works and return
before they're done?

If it all happens from the dig_port_work, then I guess it should be
possible to just set some flag from the mst .hotplug() hook and check
that flag after all the processing is done to know whether the schedule
the a full detect work. I don't think you can just return IRQ_NONE from
the short pulse since that would schedule the full hotplug on the main
DP connector, not the MST one. In fact our current hotplug code entirely
skips MST connectors. But you could have a separate work perhaps just
to deal with the MST connectors, or we'd need to redo the main hotplug
handling in some way to handle MST connectors as well.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx