[Intel-gfx] [PATCH 1/2] drm/i915: Add support for retrying hotplug
Souza, Jose
jose.souza at intel.com
Fri Mar 15 22:22:30 UTC 2019
On Fri, 2019-03-15 at 03:39 +0200, Imre Deak wrote:
> On Fri, Mar 15, 2019 at 02:25:36AM +0200, Souza, Jose wrote:
> > On Thu, 2019-03-14 at 18:09 +0200, Imre Deak wrote:
> > > On Tue, Mar 12, 2019 at 05:58:51PM -0700, José Roberto de Souza
> > > wrote:
> > > > From: Imre Deak <imre.deak at intel.com>
> > > >
> > > > There is some scenarios that we are aware that sink probe can
> > > > fail,
> > > > so lets add the infrastructure to let hotplug() hook to request
> > > > another probe after some time.
> > > >
> > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> > > > drivers/gpu/drm/i915/i915_drv.h | 3 +-
> > > > drivers/gpu/drm/i915/intel_ddi.c | 12 +++---
> > > > drivers/gpu/drm/i915/intel_dp.c | 12 +++---
> > > > drivers/gpu/drm/i915/intel_drv.h | 17 +++++++--
> > > > drivers/gpu/drm/i915/intel_hotplug.c | 56
> > > > ++++++++++++++++++++++
> > > > ------
> > > > drivers/gpu/drm/i915/intel_sdvo.c | 8 ++--
> > > > 7 files changed, 79 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index 6a90558de213..f93aab165c7e 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -4289,7 +4289,7 @@ static int i915_hpd_storm_ctl_show(struct
> > > > seq_file *m, void *data)
> > > > */
> > > > synchronize_irq(dev_priv->drm.irq);
> > > > flush_work(&dev_priv->hotplug.dig_port_work);
> > > > - flush_work(&dev_priv->hotplug.hotplug_work);
> > > > + flush_delayed_work(&dev_priv->hotplug.hotplug_work);
> > > >
> > > > seq_printf(m, "Threshold: %d\n", hotplug-
> > > > >hpd_storm_threshold);
> > > > seq_printf(m, "Detected: %s\n",
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index dc63303225fc..f47cec8b7f01 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -156,7 +156,7 @@ enum hpd_pin {
> > > > #define HPD_STORM_DEFAULT_THRESHOLD 50
> > > >
> > > > struct i915_hotplug {
> > > > - struct work_struct hotplug_work;
> > > > + struct delayed_work hotplug_work;
> > > >
> > > > struct {
> > > > unsigned long last_jiffies;
> > > > @@ -168,6 +168,7 @@ struct i915_hotplug {
> > > > } state;
> > > > } stats[HPD_NUM_PINS];
> > > > u32 event_bits;
> > > > + u32 retry_bits;
> > > > struct delayed_work reenable_work;
> > > >
> > > > u32 long_port_mask;
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index 7e3b4e8fdf3a..5b840a5d00cc 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -4053,14 +4053,16 @@ static int intel_hdmi_reset_link(struct
> > > > intel_encoder *encoder,
> > > > return modeset_pipe(&crtc->base, ctx);
> > > > }
> > > >
> > > > -static bool intel_ddi_hotplug(struct intel_encoder *encoder,
> > > > - struct intel_connector
> > > > *connector)
> > > > +static enum intel_hotplug_state
> > > > +intel_ddi_hotplug(struct intel_encoder *encoder,
> > > > + struct intel_connector *connector,
> > > > + bool irq_received)
> > > > {
> > > > struct drm_modeset_acquire_ctx ctx;
> > > > - bool changed;
> > > > + enum intel_hotplug_state state;
> > > > int ret;
> > > >
> > > > - changed = intel_encoder_hotplug(encoder, connector);
> > > > + state = intel_encoder_hotplug(encoder, connector,
> > > > irq_received);
> > > >
> > > > drm_modeset_acquire_init(&ctx, 0);
> > > >
> > > > @@ -4082,7 +4084,7 @@ static bool intel_ddi_hotplug(struct
> > > > intel_encoder *encoder,
> > > > drm_modeset_acquire_fini(&ctx);
> > > > WARN(ret, "Acquiring modeset locks failed with %i\n",
> > > > ret);
> > > >
> > > > - return changed;
> > > > + return state;
> > > > }
> > > >
> > > > static struct intel_connector *
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index f40b3342d82a..f74e72173874 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4738,14 +4738,16 @@ int intel_dp_retrain_link(struct
> > > > intel_encoder *encoder,
> > > > * retrain the link to get a picture. That's in case no
> > > > * userspace component reacted to intermittent HPD dip.
> > > > */
> > > > -static bool intel_dp_hotplug(struct intel_encoder *encoder,
> > > > - struct intel_connector *connector)
> > > > +static enum intel_hotplug_state
> > > > +intel_dp_hotplug(struct intel_encoder *encoder,
> > > > + struct intel_connector *connector,
> > > > + bool irq_received)
> > > > {
> > > > struct drm_modeset_acquire_ctx ctx;
> > > > - bool changed;
> > > > + enum intel_hotplug_state state;
> > > > int ret;
> > > >
> > > > - changed = intel_encoder_hotplug(encoder, connector);
> > > > + state = intel_encoder_hotplug(encoder, connector,
> > > > irq_received);
> > > >
> > > > drm_modeset_acquire_init(&ctx, 0);
> > > >
> > > > @@ -4764,7 +4766,7 @@ static bool intel_dp_hotplug(struct
> > > > intel_encoder *encoder,
> > > > drm_modeset_acquire_fini(&ctx);
> > > > WARN(ret, "Acquiring modeset locks failed with %i\n",
> > > > ret);
> > > >
> > > > - return changed;
> > > > + return state;
> > > > }
> > > >
> > > > static void intel_dp_check_service_irq(struct intel_dp
> > > > *intel_dp)
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 40ebc94b2187..be9fe0268472 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -226,14 +226,21 @@ struct intel_fbdev {
> > > > struct mutex hpd_lock;
> > > > };
> > > >
> > > > +enum intel_hotplug_state {
> > > > + INTEL_HOTPLUG_NOCHANGE,
> > > > + INTEL_HOTPLUG_CHANGED,
> > > > + INTEL_HOTPLUG_RETRY,
> > > > +};
> > > > +
> > > > struct intel_encoder {
> > > > struct drm_encoder base;
> > > >
> > > > enum intel_output_type type;
> > > > enum port port;
> > > > unsigned int cloneable;
> > > > - bool (*hotplug)(struct intel_encoder *encoder,
> > > > - struct intel_connector *connector);
> > > > + enum intel_hotplug_state (*hotplug)(struct
> > > > intel_encoder
> > > > *encoder,
> > > > + struct
> > > > intel_connector
> > > > *connector,
> > > > + bool irq_received);
> > > > enum intel_output_type (*compute_output_type)(struct
> > > > intel_encoder *,
> > > > struct
> > > > intel_crtc_state *,
> > > > struct
> > > > drm_connector_state *);
> > > > @@ -2016,8 +2023,10 @@ int
> > > > intel_dsi_dcs_init_backlight_funcs(struct intel_connector
> > > > *intel_connector);
> > > > void intel_dvo_init(struct drm_i915_private *dev_priv);
> > > > /* intel_hotplug.c */
> > > > void intel_hpd_poll_init(struct drm_i915_private *dev_priv);
> > > > -bool intel_encoder_hotplug(struct intel_encoder *encoder,
> > > > - struct intel_connector *connector);
> > > > +enum intel_hotplug_state
> > > > +intel_encoder_hotplug(struct intel_encoder *encoder,
> > > > + struct intel_connector *connector,
> > > > + bool irq_received);
> > > >
> > > > /* legacy fbdev emulation in intel_fbdev.c */
> > > > #ifdef CONFIG_DRM_FBDEV_EMULATION
> > > > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c
> > > > b/drivers/gpu/drm/i915/intel_hotplug.c
> > > > index b8937c788f03..3eaa06526f6b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_hotplug.c
> > > > +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> > > > @@ -111,6 +111,7 @@ enum hpd_pin intel_hpd_pin_default(struct
> > > > drm_i915_private *dev_priv,
> > > >
> > > > #define HPD_STORM_DETECT_PERIOD 1000
> > > > #define HPD_STORM_REENABLE_DELAY (2 * 60 * 1000)
> > > > +#define HPD_RETRY_DELAY 1000
> > > >
> > > > /**
> > > > * intel_hpd_irq_storm_detect - gather stats and detect HPD
> > > > IRQ
> > > > storm on a pin
> > > > @@ -265,8 +266,10 @@ static void
> > > > intel_hpd_irq_storm_reenable_work(struct work_struct *work)
> > > > intel_runtime_pm_put(dev_priv, wakeref);
> > > > }
> > > >
> > > > -bool intel_encoder_hotplug(struct intel_encoder *encoder,
> > > > - struct intel_connector *connector)
> > > > +enum intel_hotplug_state
> > > > +intel_encoder_hotplug(struct intel_encoder *encoder,
> > > > + struct intel_connector *connector,
> > > > + bool irq_received)
> > > > {
> > > > struct drm_device *dev = connector->base.dev;
> > > > enum drm_connector_status old_status;
> > > > @@ -278,7 +281,7 @@ bool intel_encoder_hotplug(struct
> > > > intel_encoder
> > > > *encoder,
> > > > drm_helper_probe_detect(&connector->base, NULL,
> > > > false);
> > > >
> > > > if (old_status == connector->base.status)
> > > > - return false;
> > > > + return INTEL_HOTPLUG_NOCHANGE;
> > > >
> > > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s
> > > > to
> > > > %s\n",
> > > > connector->base.base.id,
> > > > @@ -286,7 +289,7 @@ bool intel_encoder_hotplug(struct
> > > > intel_encoder
> > > > *encoder,
> > > > drm_get_connector_status_name(old_status)
> > > > ,
> > > > drm_get_connector_status_name(connector-
> > > > > base.status));
> > > >
> > > > - return true;
> > > > + return INTEL_HOTPLUG_CHANGED;
> > > > }
> > > >
> > > > static bool intel_encoder_has_hpd_pulse(struct intel_encoder
> > > > *encoder)
> > > > @@ -338,7 +341,7 @@ static void i915_digport_work_func(struct
> > > > work_struct *work)
> > > > spin_lock_irq(&dev_priv->irq_lock);
> > > > dev_priv->hotplug.event_bits |= old_bits;
> > > > spin_unlock_irq(&dev_priv->irq_lock);
> > > > - schedule_work(&dev_priv->hotplug.hotplug_work);
> > > > + schedule_delayed_work(&dev_priv-
> > > > >hotplug.hotplug_work,
> > > > 0);
> > > > }
> > > > }
> > > >
> > > > @@ -348,14 +351,17 @@ static void i915_digport_work_func(struct
> > > > work_struct *work)
> > > > static void i915_hotplug_work_func(struct work_struct *work)
> > > > {
> > > > struct drm_i915_private *dev_priv =
> > > > - container_of(work, struct drm_i915_private,
> > > > hotplug.hotplug_work);
> > > > + container_of(work, struct drm_i915_private,
> > > > + hotplug.hotplug_work.work);
> > > > struct drm_device *dev = &dev_priv->drm;
> > > > struct intel_connector *intel_connector;
> > > > struct intel_encoder *intel_encoder;
> > > > struct drm_connector *connector;
> > > > struct drm_connector_list_iter conn_iter;
> > > > bool changed = false;
> > > > + u32 retry = 0;
> > > > u32 hpd_event_bits;
> > > > + u32 hpd_retry_bits;
> > > >
> > > > mutex_lock(&dev->mode_config.mutex);
> > > > DRM_DEBUG_KMS("running encoder hotplug functions\n");
> > > > @@ -364,6 +370,8 @@ static void i915_hotplug_work_func(struct
> > > > work_struct *work)
> > > >
> > > > hpd_event_bits = dev_priv->hotplug.event_bits;
> > > > dev_priv->hotplug.event_bits = 0;
> > > > + hpd_retry_bits = dev_priv->hotplug.retry_bits;
> > > > + dev_priv->hotplug.retry_bits = 0;
> > > >
> > > > /* Enable polling for connectors which had HPD IRQ
> > > > storms */
> > > > intel_hpd_irq_storm_switch_to_polling(dev_priv);
> > > > @@ -372,16 +380,29 @@ static void i915_hotplug_work_func(struct
> > > > work_struct *work)
> > > >
> > > > drm_connector_list_iter_begin(dev, &conn_iter);
> > > > drm_for_each_connector_iter(connector, &conn_iter) {
> > > > + u32 hpd_bit;
> > > > +
> > > > intel_connector =
> > > > to_intel_connector(connector);
> > > > if (!intel_connector->encoder)
> > > > continue;
> > > > intel_encoder = intel_connector->encoder;
> > > > - if (hpd_event_bits & (1 << intel_encoder-
> > > > >hpd_pin)) {
> > > > + hpd_bit = BIT(intel_encoder->hpd_pin);
> > > > + if ((hpd_event_bits | hpd_retry_bits) &
> > > > hpd_bit) {
> > > > DRM_DEBUG_KMS("Connector %s (pin %i)
> > > > received
> > > > hotplug event.\n",
> > > > connector->name,
> > > > intel_encoder-
> > > > > hpd_pin);
> > > >
> > > > - changed |= intel_encoder-
> > > > > hotplug(intel_encoder,
> > > > - intel
> > > > _connect
> > > > or);
> > > > + switch (intel_encoder-
> > > > >hotplug(intel_encoder,
> > > > + intel_co
> > > > nnector,
> > > > + hpd_even
> > > > t_bits &
> > > > hpd_bit)) {
> > > > + case INTEL_HOTPLUG_NOCHANGE:
> > > > + break;
> > > > + case INTEL_HOTPLUG_CHANGED:
> > > > + changed = true;
> > > > + break;
> > > > + case INTEL_HOTPLUG_RETRY:
> > > > + retry |= hpd_bit;
> > > > + break;
> > > > + }
> > > > }
> > > > }
> > > > drm_connector_list_iter_end(&conn_iter);
> > > > @@ -389,6 +410,15 @@ static void i915_hotplug_work_func(struct
> > > > work_struct *work)
> > > >
> > > > if (changed)
> > > > drm_kms_helper_hotplug_event(dev);
> > >
> > > Just realized, that this way we'll do hotplug processing for pins
> > > with a
> > > shared encoder (DDI DP/HDMI) twice, even if one encoder's hotplug
> > > hook
> > > signaled a change (and so there can't be any change on other
> > > connectors
> > > sharing this encoder). I suppose preventing calling the hook for
> > > a
> > > pin that
> > > already signalled a change would make sense, but that's a
> > > separate
> > > issue and could still lead to retrying needlessly. So I think we
> > > should
> > > prevent the retry here explicitly:
> > >
> > > retry &= ~changed;
> >
> > I was not aware of this shared hotplug pin, but it will need more
> > than
> > that, if the first port sharing a pin have changed but the second
> > don't
> > it would still mark the pin to retry,
>
> For two connectors sharing an encoder their ports and pins will be
> the same.
> So if you do above:
>
> case INTEL_HOTPLUG_CHANGED:
> changed |= hpd_bit;
> break;
>
> ...
>
> you can just avoid the retries for such changed connectors by
>
> retry &= ~changed;
>
> here.
So lets say that we have: HDMI-1 on enconder_A and DP-1 on encoder_A,
user hotplug HDMI-1 and when iterating over all connectors in
i915_hotplug_work_func() we have this:
// fist iteration
ret = intel_encoder->hotplug->hotplug(encoder, HDMI-1);
//ret = INTEL_HOTPLUG_CHANGED
// second iteration
ret = intel_encoder->hotplug->hotplug(encoder, DP-1);
//ret = INTEL_HOTPLUG_RETRY
so a 'retry &= ~changed;' in 'case INTEL_HOTPLUG_CHANGED:' alone would
not prevent it to set retry_bits;
Or I did not understood correctly this shared encoder and hpd pin?
>
>
> > I will fix that and add the
> > "retry" to the hotplug DP handler of non-ddi platforms.
> >
> > > (making changed a bitmap).
> > >
> > > > +
> > > > + if (retry) {
> > > > + spin_lock_irq(&dev_priv->irq_lock);
> > > > + dev_priv->hotplug.retry_bits |= retry;
> > > > + spin_unlock_irq(&dev_priv->irq_lock);
> > > > +
> > > > + mod_delayed_work(system_wq, &dev_priv-
> > > > > hotplug.hotplug_work,
> > > > + msecs_to_jiffies(HPD_RETRY_DEL
> > > > AY));
> > > > + }
> > > > }
> > > >
> > > >
> > > > @@ -515,7 +545,7 @@ void intel_hpd_irq_handler(struct
> > > > drm_i915_private *dev_priv,
> > > > if (queue_dig)
> > > > queue_work(dev_priv->hotplug.dp_wq, &dev_priv-
> > > > > hotplug.dig_port_work);
> > > > if (queue_hp)
> > > > - schedule_work(&dev_priv->hotplug.hotplug_work);
> > > > + schedule_delayed_work(&dev_priv-
> > > > >hotplug.hotplug_work,
> > > > 0);
> > > > }
> > > >
> > > > /**
> > > > @@ -635,7 +665,8 @@ void intel_hpd_poll_init(struct
> > > > drm_i915_private *dev_priv)
> > > >
> > > > void intel_hpd_init_work(struct drm_i915_private *dev_priv)
> > > > {
> > > > - INIT_WORK(&dev_priv->hotplug.hotplug_work,
> > > > i915_hotplug_work_func);
> > > > + INIT_DELAYED_WORK(&dev_priv->hotplug.hotplug_work,
> > > > + i915_hotplug_work_func);
> > > > INIT_WORK(&dev_priv->hotplug.dig_port_work,
> > > > i915_digport_work_func);
> > > > INIT_WORK(&dev_priv->hotplug.poll_init_work,
> > > > i915_hpd_poll_init_work);
> > > > INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
> > > > @@ -649,11 +680,12 @@ void intel_hpd_cancel_work(struct
> > > > drm_i915_private *dev_priv)
> > > > dev_priv->hotplug.long_port_mask = 0;
> > > > dev_priv->hotplug.short_port_mask = 0;
> > > > dev_priv->hotplug.event_bits = 0;
> > > > + dev_priv->hotplug.retry_bits = 0;
> > > >
> > > > spin_unlock_irq(&dev_priv->irq_lock);
> > > >
> > > > cancel_work_sync(&dev_priv->hotplug.dig_port_work);
> > > > - cancel_work_sync(&dev_priv->hotplug.hotplug_work);
> > > > + cancel_delayed_work_sync(&dev_priv-
> > > > >hotplug.hotplug_work);
> > > > cancel_work_sync(&dev_priv->hotplug.poll_init_work);
> > > > cancel_delayed_work_sync(&dev_priv-
> > > > >hotplug.reenable_work);
> > > > }
> > > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
> > > > b/drivers/gpu/drm/i915/intel_sdvo.c
> > > > index 68f497493d43..05ae7b3704a7 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > > > @@ -1838,12 +1838,14 @@ static void
> > > > intel_sdvo_enable_hotplug(struct intel_encoder *encoder)
> > > > &intel_sdvo->hotplug_active, 2);
> > > > }
> > > >
> > > > -static bool intel_sdvo_hotplug(struct intel_encoder *encoder,
> > > > - struct intel_connector
> > > > *connector)
> > > > +static enum intel_hotplug_state
> > > > +intel_sdvo_hotplug(struct intel_encoder *encoder,
> > > > + struct intel_connector *connector,
> > > > + bool irq_received)
> > > > {
> > > > intel_sdvo_enable_hotplug(encoder);
> > > >
> > > > - return intel_encoder_hotplug(encoder, connector);
> > > > + return intel_encoder_hotplug(encoder, connector,
> > > > irq_received);
> > > > }
> > > >
> > > > static bool
> > > > --
> > > > 2.21.0
> > > >
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20190315/56f92e73/attachment-0001.sig>
More information about the Intel-gfx
mailing list