Re: [PATCH RESEND] drm: i915: Don't try detecting sinks on ports already in use

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

On Sun, May 28, 2017 at 09:53:19PM -0300, Gabriel Krisman Bertazi wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > The key problem here is say a race between DP unplug and HDMI plug, and
> > users are evil enough (or common enough) for it to happen.
> >
> > I thought the idea was reasonable though, and perhaps we could make
> > more use of the knowlege of the shared ports to improve detection of
> > common DP/HDMI DDIs (i.e. run detection once for a ddi and have it
> > decide whether it is hdmi or dp).
> 
> Hi Chris and Manasi, thanks for the feedback and sorry for the late
> response.
> 
> I see your point about being racy.  I think this is not an issue in my
> system since it appears that the disconnect of the first connector
> triggers another scheduling of the hotplug_work, that further detects
> the other connector.
> 
> I'm still working on this, but can you provide more details on your
> suggestion about using more knowledge of the shared ports to improve the
> detection of DDIs?  I still don't see how we could prevent this race
> once and for all.
> 
> In the mean time, I wrote another patch with a different way to reduce
> the overhead of attempting to detect connectors of shared ports already
> in use.  My understanding is that this version is no longer racy but it
> doesn't catch every condition where we could avoid detection.  Though,
> it feels like something we want to get upstream.  Can you please take a
> look and let me know?
> 
> I think this also addresses the cloned encoders issue raised by Manasi,
> if I understood it correctly.
> 
> >8
> Subject: [PATCH] drm: i915: Stop further detection on shared pins if a sink is
>  connected
> 
> HPD pins may be shared among connectors but, according to documentation,
> only one connector can be enebled at any given time.  This avoids
> waisting time with attempting to detect further connectors on the same
> pin if the first one was already detected.
> 
> Since this reduces the overhead of the i915_hotplug_work_func, it may
> address the following vblank misses detected by the CI:
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100215
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100558
> 
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_hotplug.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index f1200272a699..913e8523b89d 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -319,6 +319,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	struct drm_connector_list_iter conn_iter;
>  	bool changed = false;
>  	u32 hpd_event_bits;
> +	u32 enc_hpd_pin_mask;
>  
>  	mutex_lock(&dev->mode_config.mutex);
>  	DRM_DEBUG_KMS("running encoder hotplug functions\n");
> @@ -339,13 +340,18 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  		if (!intel_connector->encoder)
>  			continue;
>  		intel_encoder = intel_connector->encoder;
> -		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> +		enc_hpd_pin_mask = (1 << intel_encoder->hpd_pin);
> +		if (hpd_event_bits & enc_hpd_pin_mask) {
>  			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
>  				      connector->name, intel_encoder->hpd_pin);
>  			if (intel_encoder->hot_plug)
>  				intel_encoder->hot_plug(intel_encoder);
> -			if (intel_hpd_irq_event(dev, connector))
> +			if (intel_hpd_irq_event(dev, connector)) {
>  				changed = true;
> +				if (connector->status ==
> +				    connector_status_connected)
> +					hpd_event_bits &= ~(enc_hpd_pin_mask);

This won't work correctly because now it will fail to update
connector->status for the other connector. I admit that being fast
enough to switch between DP<->HDMI directly while the HPD interrupt
is enabled is perhaps a little unlikely, but it can certainly happen
very easily if the interrupt has been disabled.

We alrady have other bugs in that area. Eg. if you manage to switch to a
totally different display without noticing the disconnected state in
between we'll skip sending the uevent. We should really be checking if
the sink changed somehow (eg. EDID changed) and in that case send the
uevent anyway.

I also don't think this can have anything to do with vblank tests
failing, unless there's some other problem involved that somehow
triggers HPDs while the test is doing its measurements.

> +			}
>  		}
>  	}
>  	drm_connector_list_iter_end(&conn_iter);
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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