[Intel-gfx] [PATCH v3 1/2] drm/i915: Add support for retrying hotplug
Souza, Jose
jose.souza at intel.com
Wed Jul 10 22:17:25 UTC 2019
On Wed, 2019-07-10 at 04:20 -0700, Rodrigo Vivi wrote:
> On Fri, Jun 28, 2019 at 02:39:20PM -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.
> >
> > v2: Handle shared HPD pins (Imre)
> > v3: Rebased
> >
> > 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/display/intel_ddi.c | 12 ++--
> > drivers/gpu/drm/i915/display/intel_dp.c | 12 ++--
> > drivers/gpu/drm/i915/display/intel_hotplug.c | 59 +++++++++++++++-
> > ----
> > drivers/gpu/drm/i915/display/intel_hotplug.h | 5 +-
> > drivers/gpu/drm/i915/display/intel_sdvo.c | 8 ++-
> > drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> > drivers/gpu/drm/i915/i915_drv.h | 3 +-
> > drivers/gpu/drm/i915/intel_drv.h | 11 +++-
> > 8 files changed, 80 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 7925a176f900..53009984e046 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -4081,14 +4081,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);
> >
> > @@ -4110,7 +4112,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/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 4336df46fe78..95d0da9d1bac 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4878,14 +4878,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);
> >
> > @@ -4904,7 +4906,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/display/intel_hotplug.c
> > b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > index ea3de4acc850..3662966d366e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > @@ -112,6 +112,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
> > @@ -266,8 +267,10 @@ static void
> > intel_hpd_irq_storm_reenable_work(struct work_struct *work)
> > intel_runtime_pm_put(&dev_priv->runtime_pm, 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;
> > @@ -279,7 +282,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,
> > @@ -287,7 +290,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)
> > @@ -339,7 +342,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);
> > }
> > }
> >
> > @@ -349,14 +352,16 @@ 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 changed = 0, retry = 0;
> > u32 hpd_event_bits;
> > + u32 hpd_retry_bits;
> >
> > mutex_lock(&dev->mode_config.mutex);
> > DRM_DEBUG_KMS("running encoder hotplug functions\n");
> > @@ -365,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);
> > @@ -373,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_connector,
> > + hpd_event_bits &
> > hpd_bit)) {
> > + case INTEL_HOTPLUG_NOCHANGE:
> > + break;
> > + case INTEL_HOTPLUG_CHANGED:
> > + changed |= hpd_bit;
> > + break;
> > + case INTEL_HOTPLUG_RETRY:
> > + retry |= hpd_bit;
> > + break;
> > + }
> > }
> > }
> > drm_connector_list_iter_end(&conn_iter);
> > @@ -390,6 +410,17 @@ static void i915_hotplug_work_func(struct
> > work_struct *work)
> >
> > if (changed)
> > drm_kms_helper_hotplug_event(dev);
> > +
> > + /* Remove shared HPD pins that have changed */
> > + retry &= ~changed;
> > + 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_DELAY));
> > + }
> > }
> >
> >
> > @@ -516,7 +547,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);
> > }
> >
> > /**
> > @@ -636,7 +667,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,
> > @@ -650,11 +682,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/display/intel_hotplug.h
> > b/drivers/gpu/drm/i915/display/intel_hotplug.h
> > index 805f897dbb7a..b0cd447b7fbc 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hotplug.h
> > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.h
> > @@ -15,8 +15,9 @@ struct intel_connector;
> > struct intel_encoder;
> >
> > 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);
> > void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> > u32 pin_mask, u32 long_mask);
> > void intel_hpd_init(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > index ceda03e5a3d4..2855f14f5746 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > @@ -1893,12 +1893,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
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index eeecdad0e3ca..836709ee9ff7 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -4083,7 +4083,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 7e981b03face..d240997912d0 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -163,7 +163,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;
> > @@ -175,6 +175,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_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 1d58f7ec5d84..0088eb2f510e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -101,14 +101,21 @@ struct intel_fbdev {
> > struct mutex hpd_lock;
> > };
> >
> > +enum intel_hotplug_state {
> > + INTEL_HOTPLUG_NOCHANGE,
> > + INTEL_HOTPLUG_CHANGED,
>
> my only bikeshed would be to keep this symmetric...
> no-change change or preferably unchanged changed.
>
> But everything seems right so let's move
Just sent a new version replacing nochange to unchanged and added your
r-v-b.
Thanks
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>
> > + 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 *);
> > --
> > 2.22.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list