On Tue, Feb 25, 2025 at 06:25:01PM +0200, Jani Nikula wrote:
> On Tue, 25 Feb 2025, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> > Add support for suspending the IRQ handling on the HPD pin of a given
> > encoder, handling IRQs that arrived while in the suspended state after
> > resuming the IRQ handling. This will be used by a follow-up change,
> > which suspends/resumes the IRQ handling around DP link training.
> >
> > This is similar to the intel_hpd_disable/enable() functionality, by also
> > handling encoders/ports with a pulse handler (i.e. also
> > suspending/resuming the short/long pulse handling) and handling the IRQs
> > arrived in the suspended state after the handling is resumed (vs. just
> > dropping such IRQs).
> >
> > v2:
> > - Handle encoders without a port assigned to them.
> > - Fix clearing IRQs from intel_hotplug::short_port_mask.
> >
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
>
> I think the terms suspend/resume are too loaded to be used for something
> that isn't about system or runtime suspend/resume. It's guaranteed to
> mislead people.
>
> Maybe:
>
> - pause/continue
> - deactivate/activate
> - block/unblock
Right, these functions aren't related to the device suspend/resume
handling, so better to use separate names. Can rename them to
intel_hpd_block/unblock().
> ?
>
>
> BR,
> Jani.
>
>
> > ---
> > .../gpu/drm/i915/display/intel_display_core.h | 1 +
> > drivers/gpu/drm/i915/display/intel_hotplug.c | 209 +++++++++++++++---
> > drivers/gpu/drm/i915/display/intel_hotplug.h | 2 +
> > 3 files changed, 187 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
> > index 554870d2494b3..152ee4dfcbf38 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> > @@ -159,6 +159,7 @@ struct intel_hotplug {
> > struct {
> > unsigned long last_jiffies;
> > int count;
> > + int suspend_count;
> > enum {
> > HPD_ENABLED = 0,
> > HPD_DISABLED = 1,
> > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > index ab8e71c4b0f86..85c56b8add8b0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > @@ -349,19 +349,69 @@ static bool intel_encoder_has_hpd_pulse(struct intel_encoder *encoder)
> > enc_to_dig_port(encoder)->hpd_pulse != NULL;
> > }
> >
> > +static u32 hpd_mask_to_ports(struct intel_display *display, u32 hpd_mask)
> > +{
> > + struct intel_encoder *encoder;
> > + u32 ports = 0;
> > +
> > + for_each_intel_encoder(display->drm, encoder) {
> > + if (encoder->port == PORT_NONE)
> > + continue;
> > +
> > + if (BIT(encoder->hpd_pin) & hpd_mask)
> > + ports |= BIT(encoder->port);
> > + }
> > +
> > + return ports;
> > +}
> > +
> > +static u32 get_suspended_hpds(struct intel_display *display)
> > +{
> > + struct drm_i915_private *i915 = to_i915(display->drm);
> > + enum hpd_pin pin;
> > + u32 hpds = 0;
> > +
> > + lockdep_assert_held(&i915->irq_lock);
> > +
> > + for_each_hpd_pin(pin) {
> > + if (display->hotplug.stats[pin].suspend_count)
> > + hpds |= BIT(pin);
> > + }
> > +
> > + return hpds;
> > +}
> > +
> > +/*
> > + * Get the mask of all the ports using a suspended HPD pin, accounting for
> > + * ports sharing their - suspended - HPD pin with another port.
> > + */
> > +static u32 get_suspended_hpd_ports(struct intel_display *display)
> > +{
> > + struct drm_i915_private *i915 = to_i915(display->drm);
> > +
> > + lockdep_assert_held(&i915->irq_lock);
> > + return hpd_mask_to_ports(display, get_suspended_hpds(display));
> > +}
> > +
> > static void i915_digport_work_func(struct work_struct *work)
> > {
> > - struct drm_i915_private *dev_priv =
> > - container_of(work, struct drm_i915_private, display.hotplug.dig_port_work);
> > + struct intel_display *display =
> > + container_of(work, struct intel_display, hotplug.dig_port_work);
> > + struct drm_i915_private *dev_priv = to_i915(display->drm);
> > + struct intel_hotplug *hotplug = &display->hotplug;
> > u32 long_port_mask, short_port_mask;
> > struct intel_encoder *encoder;
> > + u32 suspended_ports;
> > u32 old_bits = 0;
> >
> > spin_lock_irq(&dev_priv->irq_lock);
> > - long_port_mask = dev_priv->display.hotplug.long_port_mask;
> > - dev_priv->display.hotplug.long_port_mask = 0;
> > - short_port_mask = dev_priv->display.hotplug.short_port_mask;
> > - dev_priv->display.hotplug.short_port_mask = 0;
> > +
> > + suspended_ports = get_suspended_hpd_ports(display);
> > + long_port_mask = hotplug->long_port_mask & ~suspended_ports;
> > + hotplug->long_port_mask &= ~long_port_mask;
> > + short_port_mask = hotplug->short_port_mask & ~suspended_ports;
> > + hotplug->short_port_mask &= ~short_port_mask;
> > +
> > spin_unlock_irq(&dev_priv->irq_lock);
> >
> > for_each_intel_encoder(&dev_priv->drm, encoder) {
> > @@ -406,13 +456,17 @@ static void i915_digport_work_func(struct work_struct *work)
> > */
> > void intel_hpd_trigger_irq(struct intel_digital_port *dig_port)
> > {
> > - struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > + struct intel_display *display = to_intel_display(dig_port);
> > + struct drm_i915_private *i915 = to_i915(display->drm);
> > + struct intel_hotplug *hotplug = &display->hotplug;
> >
> > spin_lock_irq(&i915->irq_lock);
> > - i915->display.hotplug.short_port_mask |= BIT(dig_port->base.port);
> > - spin_unlock_irq(&i915->irq_lock);
> >
> > - queue_work(i915->display.hotplug.dp_wq, &i915->display.hotplug.dig_port_work);
> > + hotplug->short_port_mask |= BIT(dig_port->base.port);
> > + if (!(BIT(dig_port->base.port) & get_suspended_hpd_ports(display)))
> > + queue_work(hotplug->dp_wq, &hotplug->dig_port_work);
> > +
> > + spin_unlock_irq(&i915->irq_lock);
> > }
> >
> > /*
> > @@ -420,9 +474,10 @@ void intel_hpd_trigger_irq(struct intel_digital_port *dig_port)
> > */
> > static void i915_hotplug_work_func(struct work_struct *work)
> > {
> > - struct drm_i915_private *dev_priv =
> > - container_of(work, struct drm_i915_private,
> > - display.hotplug.hotplug_work.work);
> > + struct intel_display *display =
> > + container_of(work, struct intel_display, hotplug.hotplug_work.work);
> > + struct drm_i915_private *dev_priv = to_i915(display->drm);
> > + struct intel_hotplug *hotplug = &display->hotplug;
> > struct drm_connector_list_iter conn_iter;
> > struct intel_connector *connector;
> > u32 changed = 0, retry = 0;
> > @@ -430,16 +485,18 @@ static void i915_hotplug_work_func(struct work_struct *work)
> > u32 hpd_retry_bits;
> > struct drm_connector *first_changed_connector = NULL;
> > int changed_connectors = 0;
> > + u32 suspended_hpds;
> >
> > mutex_lock(&dev_priv->drm.mode_config.mutex);
> > drm_dbg_kms(&dev_priv->drm, "running encoder hotplug functions\n");
> >
> > spin_lock_irq(&dev_priv->irq_lock);
> >
> > - hpd_event_bits = dev_priv->display.hotplug.event_bits;
> > - dev_priv->display.hotplug.event_bits = 0;
> > - hpd_retry_bits = dev_priv->display.hotplug.retry_bits;
> > - dev_priv->display.hotplug.retry_bits = 0;
> > + suspended_hpds = get_suspended_hpds(display);
> > + hpd_event_bits = hotplug->event_bits & ~suspended_hpds;
> > + hotplug->event_bits &= ~hpd_event_bits;
> > + hpd_retry_bits = hotplug->retry_bits & ~suspended_hpds;
> > + hotplug->retry_bits &= ~hpd_retry_bits;
> >
> > /* Enable polling for connectors which had HPD IRQ storms */
> > intel_hpd_irq_storm_switch_to_polling(dev_priv);
> > @@ -538,11 +595,13 @@ static void i915_hotplug_work_func(struct work_struct *work)
> > void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> > u32 pin_mask, u32 long_mask)
> > {
> > + struct intel_display *display = to_intel_display(&dev_priv->drm);
> > struct intel_encoder *encoder;
> > bool storm_detected = false;
> > bool queue_dig = false, queue_hp = false;
> > u32 long_hpd_pulse_mask = 0;
> > u32 short_hpd_pulse_mask = 0;
> > + u32 suspended_hpds;
> > enum hpd_pin pin;
> >
> > if (!pin_mask)
> > @@ -550,6 +609,8 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> >
> > spin_lock(&dev_priv->irq_lock);
> >
> > + suspended_hpds = get_suspended_hpds(display);
> > +
> > /*
> > * Determine whether ->hpd_pulse() exists for each pin, and
> > * whether we have a short or a long pulse. This is needed
> > @@ -573,7 +634,9 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> > "digital hpd on [ENCODER:%d:%s] - %s\n",
> > encoder->base.base.id, encoder->base.name,
> > long_hpd ? "long" : "short");
> > - queue_dig = true;
> > +
> > + if (!(BIT(pin) & suspended_hpds))
> > + queue_dig = true;
> >
> > if (long_hpd) {
> > long_hpd_pulse_mask |= BIT(pin);
> > @@ -617,7 +680,9 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> > } else {
> > dev_priv->display.hotplug.event_bits |= BIT(pin);
> > long_hpd = true;
> > - queue_hp = true;
> > +
> > + if (!(BIT(pin) & suspended_hpds))
> > + queue_hp = true;
> > }
> >
> > if (intel_hpd_irq_storm_detect(dev_priv, pin, long_hpd)) {
> > @@ -915,11 +980,15 @@ static bool cancel_all_detection_work(struct drm_i915_private *i915)
> >
> > void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
> > {
> > + struct intel_display *display = to_intel_display(&dev_priv->drm);
> > +
> > if (!HAS_DISPLAY(dev_priv))
> > return;
> >
> > spin_lock_irq(&dev_priv->irq_lock);
> >
> > + drm_WARN_ON(display->drm, get_suspended_hpds(display));
> > +
> > dev_priv->display.hotplug.long_port_mask = 0;
> > dev_priv->display.hotplug.short_port_mask = 0;
> > dev_priv->display.hotplug.event_bits = 0;
> > @@ -966,19 +1035,22 @@ void intel_hpd_enable(struct drm_i915_private *dev_priv, enum hpd_pin pin)
> >
> > static void queue_work_for_missed_irqs(struct drm_i915_private *i915)
> > {
> > - bool queue_work = false;
> > + struct intel_display *display = to_intel_display(&i915->drm);
> > + struct intel_hotplug *hotplug = &display->hotplug;
> > + bool queue_hp_work = false;
> > + u32 suspended_hpds;
> > enum hpd_pin pin;
> >
> > lockdep_assert_held(&i915->irq_lock);
> >
> > - if (i915->display.hotplug.event_bits ||
> > - i915->display.hotplug.retry_bits)
> > - queue_work = true;
> > + suspended_hpds = get_suspended_hpds(display);
> > + if ((hotplug->event_bits | hotplug->retry_bits) & ~suspended_hpds)
> > + queue_hp_work = true;
> >
> > for_each_hpd_pin(pin) {
> > switch (i915->display.hotplug.stats[pin].state) {
> > case HPD_MARK_DISABLED:
> > - queue_work = true;
> > + queue_hp_work = true;
> > break;
> > case HPD_DISABLED:
> > case HPD_ENABLED:
> > @@ -988,10 +1060,97 @@ static void queue_work_for_missed_irqs(struct drm_i915_private *i915)
> > }
> > }
> >
> > - if (queue_work)
> > + if ((hotplug->long_port_mask | hotplug->short_port_mask) &
> > + ~hpd_mask_to_ports(display, suspended_hpds))
> > + queue_work(hotplug->dp_wq, &hotplug->dig_port_work);
> > +
> > + if (queue_hp_work)
> > queue_delayed_detection_work(i915, &i915->display.hotplug.hotplug_work, 0);
> > }
> >
> > +/**
> > + * intel_hpd_suspend - Suspend handling of HPD IRQs on an HPD pin
> > + * @encoder: Encoder to suspend the HPD handling for
> > + *
> > + * Suspends the handling of HPD IRQs on the HPD pin of @encoder.
> > + *
> > + * On return:
> > + * - It's guaranteed that the suspended encoders' HPD pulse handler
> > + * (via intel_digital_port::hpd_pulse()) is not running.
> > + * - The hotplug event handling (via intel_encoder::hotplug()) of an
> > + * HPD IRQ pending at the time this function is called may be still
> > + * running.
> > + * - Detection on the encoder's connector (via
> > + * drm_connector_helper_funcs::detect_ctx(),
> > + * drm_connector_funcs::detect()) remains allowed, for instance as part of
> > + * userspace connector probing, or DRM core's connector polling.
> > + *
> > + * The call must be followed by calling intel_hpd_resume().
> > + *
> > + * Note that the handling of HPD IRQs for another encoder using the same HPD
> > + * pin as that of @encoder will be also suspended.
> > + */
> > +void intel_hpd_suspend(struct intel_encoder *encoder)
> > +{
> > + struct intel_display *display = to_intel_display(encoder);
> > + struct drm_i915_private *i915 = to_i915(display->drm);
> > + struct intel_hotplug *hotplug = &display->hotplug;
> > + bool do_flush = false;
> > +
> > + if (encoder->hpd_pin == HPD_NONE)
> > + return;
> > +
> > + spin_lock_irq(&i915->irq_lock);
> > +
> > + hotplug->stats[encoder->hpd_pin].suspend_count++;
> > +
> > + if (hotplug->stats[encoder->hpd_pin].suspend_count == 1 &&
> > + intel_encoder_has_hpd_pulse(encoder))
> > + do_flush = true;
> > +
> > + spin_unlock_irq(&i915->irq_lock);
> > +
> > + if (do_flush)
> > + flush_work(&hotplug->dig_port_work);
> > +}
> > +
> > +static void resume_hpd(struct intel_encoder *encoder)
> > +{
> > + struct intel_display *display = to_intel_display(encoder);
> > + struct drm_i915_private *i915 = to_i915(display->drm);
> > + struct intel_hotplug *hotplug = &display->hotplug;
> > +
> > + lockdep_assert_held(&i915->irq_lock);
> > +
> > + drm_WARN_ON(display->drm, hotplug->stats[encoder->hpd_pin].suspend_count == 0);
> > + hotplug->stats[encoder->hpd_pin].suspend_count--;
> > +
> > + if (hotplug->stats[encoder->hpd_pin].suspend_count == 0)
> > + queue_work_for_missed_irqs(i915);
> > +}
> > +
> > +/**
> > + * intel_hpd_resume - Resume handling of HPD IRQs on an HPD pin
> > + * @encoder: Encoder to resume the HPD handling for
> > + *
> > + * Resume the handling of HPD IRQs on the HPD pin of @encoder, which was
> > + * previously suspended by intel_hpd_suspend(). Any HPD IRQ raised on the
> > + * HPD pin while it was suspended will be handled for @encoder and for any
> > + * other encoder sharing the same HPD pin.
> > + */
> > +void intel_hpd_resume(struct intel_encoder *encoder)
> > +{
> > + struct intel_display *display = to_intel_display(encoder);
> > + struct drm_i915_private *i915 = to_i915(display->drm);
> > +
> > + if (encoder->hpd_pin == HPD_NONE)
> > + return;
> > +
> > + spin_lock_irq(&i915->irq_lock);
> > + resume_hpd(encoder);
> > + spin_unlock_irq(&i915->irq_lock);
> > +}
> > +
> > void intel_hpd_enable_detection_work(struct drm_i915_private *i915)
> > {
> > spin_lock_irq(&i915->irq_lock);
> > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.h b/drivers/gpu/drm/i915/display/intel_hotplug.h
> > index d6986902b0545..5180731def7cc 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hotplug.h
> > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.h
> > @@ -28,6 +28,8 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
> > enum hpd_pin intel_hpd_pin_default(enum port port);
> > bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
> > void intel_hpd_enable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
> > +void intel_hpd_suspend(struct intel_encoder *encoder);
> > +void intel_hpd_resume(struct intel_encoder *encoder);
> > void intel_hpd_debugfs_register(struct drm_i915_private *i915);
> >
> > void intel_hpd_enable_detection_work(struct drm_i915_private *i915);
>
> --
> Jani Nikula, Intel
