Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1692323 > unrolled thread

Re: [Intel-gfx] [PATCH] drm/i915: Synchronize connectors states when switching from poll to irq

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [Intel-gfx] [PATCH] drm/i915: Synchronize connectors states  when switching from poll to irq "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> - 2017-07-20 03:10 +0200
    Re: [Intel-gfx] [PATCH] drm/i915: Synchronize connectors states  when switching from poll to irq Paul Kocialkowski <paul.kocialkowski@linux.intel.com> - 2017-07-20 16:40 +0200

#1692323 — Re: [Intel-gfx] [PATCH] drm/i915: Synchronize connectors states when switching from poll to irq

On Mon, 2017-06-26 at 15:32 +0300, Paul Kocialkowski wrote:
> After detecting an IRQ storm, hotplug detection will switch from
> irq-based detection to poll-based detection. After a short delay or
> when resetting storm detection from debugfs, detection will switch
> back to being irq-based.
> 
> However, it may occur that polling does not have enough time to detect
> the current connector state when that second switch takes place. 

Some questions so that I understand this better. How short would this
have to be for detect to not complete? Is there a way I can easily test
this scenario?

> Thus,
> this sets the appropriate hotplug event bits for the concerned
> connectors and schedules the hotplug work, that will ensure the
> connectors states are in sync when switching back to irq.
> 

Not sure I understand this correctly, looks like you are setting the
event_bits even if there was no irq during the polling interval. Is that
right?


> Without this, no irq will be triggered and the hpd change will be lost.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hotplug.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index f1200272a699..29f55480b0bb 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -218,9 +218,13 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
>  			struct intel_connector *intel_connector = to_intel_connector(connector);
>  
>  			if (intel_connector->encoder->hpd_pin == i) {
> -				if (connector->polled != intel_connector->polled)
> +				if (connector->polled != intel_connector->polled) {
>  					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
>  							 connector->name);
> +
> +					dev_priv->hotplug.event_bits |= (1 << i);
> +				}
> +
>  				connector->polled = intel_connector->polled;
>  				if (!connector->polled)
>  					connector->polled = DRM_CONNECTOR_POLL_HPD;
> @@ -232,6 +236,8 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
>  		dev_priv->display.hpd_irq_setup(dev_priv);
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  
> +	schedule_work(&dev_priv->hotplug.hotplug_work);

How does this work with DP connectors? From what I understand, the
event_bits are set for DP based on the return value from
intel_dp_hpd_pulse(). But, doesn't this patch set the bits
unconditionally?

> +
>  	intel_runtime_pm_put(dev_priv);
>  }
>  

[toc] | [next] | [standalone]

#1692986

On Thu, 2017-07-20 at 01:04 +0000, Pandiyan, Dhinakaran wrote:
> On Mon, 2017-06-26 at 15:32 +0300, Paul Kocialkowski wrote:
> > After detecting an IRQ storm, hotplug detection will switch from
> > irq-based detection to poll-based detection. After a short delay or
> > when resetting storm detection from debugfs, detection will switch
> > back to being irq-based.
> > 
> > However, it may occur that polling does not have enough time to
> > detect
> > the current connector state when that second switch takes place. 
> 
> Some questions so that I understand this better. How short would this
> have to be for detect to not complete? Is there a way I can easily
> test this scenario?

Well, it doesn't really matter, the point is that it may happen that the
connector's hpd line changes in the time window between the last poll
(that happens every 100ms) and the moment the irq is enabled back. This
time window, however long it may be, always exists and it may happen
that this is exactly when the hpd event occurs.

It's possible to test this by triggering an hpd storm, then triggering a
regular hpd toggle and directly disabling polling mode via debugfs.
I was able to do this with a chamelium and did see the problem take
place.

> > Thus,
> > this sets the appropriate hotplug event bits for the concerned
> > connectors and schedules the hotplug work, that will ensure the
> > connectors states are in sync when switching back to irq.
> 
> Not sure I understand this correctly, looks like you are setting the
> event_bits even if there was no irq during the polling interval. Is
> that right?

Yes, you are perfectly right. it is necessary to do this because the
event will not be detected either by polling (happening too late) nor by
the irq (happening too early). So we must trigger the hotplug work to
make it check whether the connectors states changed.

> > Without this, no irq will be triggered and the hpd change will be
> > lost.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hotplug.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c
> > b/drivers/gpu/drm/i915/intel_hotplug.c
> > index f1200272a699..29f55480b0bb 100644
> > --- a/drivers/gpu/drm/i915/intel_hotplug.c
> > +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> > @@ -218,9 +218,13 @@ static void
> > intel_hpd_irq_storm_reenable_work(struct work_struct *work)
> >  			struct intel_connector *intel_connector =
> > to_intel_connector(connector);
> >  
> >  			if (intel_connector->encoder->hpd_pin == i)
> > {
> > -				if (connector->polled !=
> > intel_connector->polled)
> > +				if (connector->polled !=
> > intel_connector->polled) {
> >  					DRM_DEBUG_DRIVER("Reenablin
> > g HPD on connector %s\n",
> >  							 connector-
> > >name);
> > +
> > +					dev_priv-
> > >hotplug.event_bits |= (1 << i);
> > +				}
> > +
> >  				connector->polled =
> > intel_connector->polled;
> >  				if (!connector->polled)
> >  					connector->polled =
> > DRM_CONNECTOR_POLL_HPD;
> > @@ -232,6 +236,8 @@ static void
> > intel_hpd_irq_storm_reenable_work(struct work_struct *work)
> >  		dev_priv->display.hpd_irq_setup(dev_priv);
> >  	spin_unlock_irq(&dev_priv->irq_lock);
> >  
> > +	schedule_work(&dev_priv->hotplug.hotplug_work);
> 
> How does this work with DP connectors? From what I understand, the
> event_bits are set for DP based on the return value from
> intel_dp_hpd_pulse(). But, doesn't this patch set the bits
> unconditionally?

It works the same as other connectors, nothing specific there. The
hotplug work will read the connector's state and issue a uevent if its
value has changed.

> > +
> >  	intel_runtime_pm_put(dev_priv);
> >  }
> >  
-- 
Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web