Re: [PATCH v4 2/5] drm: Add HPD state to drm_connector_oob_hotplug_event()
On Mon, May 02, 2022 at 09:53:13AM -0700, Bjorn Andersson wrote:
> In some implementations, such as the Qualcomm platforms, the display
> driver has no way to query the current HPD state and as such it's
> impossible to distinguish between disconnect and attention events.
>
> Add a parameter to drm_connector_oob_hotplug_event() to pass the HPD
> state.
>
> Also push the test for unchanged state in the displayport altmode driver
> into the i915 driver, to allow other drivers to act upon each update.
>
> Signed-off-by: Bjorn Andersson
Acked-by: Heikki Krogerus
> ---
>
> Changes since v3:
> - Transition to drm_connector_status instead of custom hpd_state
>
> drivers/gpu/drm/drm_connector.c | 6 --
> drivers/gpu/drm/i915/display/intel_dp.c | 17 ++---
> drivers/gpu/drm/i915/i915_drv.h | 3 +++
> drivers/usb/typec/altmodes/displayport.c | 10 +++---
> include/drm/drm_connector.h | 6 --
> 5 files changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 1c48d162c77e..e86c69f0640f 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2794,6 +2794,7 @@ struct drm_connector
> *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> /**
> * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
> connector
> * @connector_fwnode: fwnode_handle to report the event on
> + * @status: hot plug detect logical state
> *
> * On some hardware a hotplug event notification may come from outside the
> display
> * driver / device. An example of this is some USB Type-C setups where the
> hardware
> @@ -2803,7 +2804,8 @@ struct drm_connector
> *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> * This function can be used to report these out-of-band events after
> obtaining
> * a drm_connector reference through calling drm_connector_find_by_fwnode().
> */
> -void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode)
> +void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
> + enum drm_connector_status status)
> {
> struct drm_connector *connector;
>
> @@ -2812,7 +2814,7 @@ void drm_connector_oob_hotplug_event(struct
> fwnode_handle *connector_fwnode)
> return;
>
> if (connector->funcs->oob_hotplug_event)
> - connector->funcs->oob_hotplug_event(connector);
> + connector->funcs->oob_hotplug_event(connector, status);
>
> drm_connector_put(connector);
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index e4a79c11fd25..56cc023f7bbd 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4951,15 +4951,26 @@ static int intel_dp_connector_atomic_check(struct
> drm_connector *conn,
> return intel_modeset_synced_crtcs(state, conn);
> }
>
> -static void intel_dp_oob_hotplug_event(struct drm_connector *connector)
> +static void intel_dp_oob_hotplug_event(struct drm_connector *connector,
> +enum drm_connector_status hpd_state)
> {
> struct intel_encoder *encoder =
> intel_attached_encoder(to_intel_connector(connector));
> struct drm_i915_private *i915 = to_i915(connector->dev);
> + bool hpd_high = hpd_state == connector_status_connected;
> + unsigned int hpd_pin = encoder->hpd_pin;
> + bool need_work = false;
>
> spin_lock_irq(>irq_lock);
> - i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
> + if (hpd_high != test_bit(hpd_pin,
> >hotplug.oob_hotplug_last_state)) {
> + i915->hotplug.event_bits |= BIT(hpd_pin);
> +
> + __assign_bit(hpd_pin, >hotplug.oob_hotplug_last_state,
> hpd_high);
> + need_work = true;
> + }
> spin_unlock_irq(>irq_lock);
> - queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
> +
> + if (need_work)
> + queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
> }
>
> static const struct drm_connector_funcs intel_dp_connector_funcs = {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 24111bf42ce0..96c088bb5522 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -135,6 +135,9 @@ struct i915_hotplug {
> /* Whether or not to count short HPD IRQs in HPD storms */
> u8 hpd_short_storm_enabled;
>
> + /* Last state reported by oob_hotplug_event for each encoder */
> + unsigned long oob_hotplug_last_state;
> +
> /*
>* if we get a HPD irq from DP and a HPD irq from non-DP
>* the non-DP HPD could block the workqueue on a mode config
> diff --git a/drivers/usb/typec/altmodes/displayport.c
> b/drivers/usb/typec/altmodes/displayport.c
> index c1d8c23baa39..9360ca177c7d
Re: [PATCH v4 2/5] drm: Add HPD state to drm_connector_oob_hotplug_event()
Hi,
On 5/16/22 13:25, Heikki Krogerus wrote:
> +Hans
>
> Hans, do you have any comments?
Thanks for the ping, this looks good to me:
Reviewed-by: Hans de Goede
Regards,
Hans
>
> On Mon, May 02, 2022 at 09:53:13AM -0700, Bjorn Andersson wrote:
>> In some implementations, such as the Qualcomm platforms, the display
>> driver has no way to query the current HPD state and as such it's
>> impossible to distinguish between disconnect and attention events.
>>
>> Add a parameter to drm_connector_oob_hotplug_event() to pass the HPD
>> state.
>>
>> Also push the test for unchanged state in the displayport altmode driver
>> into the i915 driver, to allow other drivers to act upon each update.
>>
>> Signed-off-by: Bjorn Andersson
>> ---
>>
>> Changes since v3:
>> - Transition to drm_connector_status instead of custom hpd_state
>>
>> drivers/gpu/drm/drm_connector.c | 6 --
>> drivers/gpu/drm/i915/display/intel_dp.c | 17 ++---
>> drivers/gpu/drm/i915/i915_drv.h | 3 +++
>> drivers/usb/typec/altmodes/displayport.c | 10 +++---
>> include/drm/drm_connector.h | 6 --
>> 5 files changed, 28 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c
>> b/drivers/gpu/drm/drm_connector.c
>> index 1c48d162c77e..e86c69f0640f 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -2794,6 +2794,7 @@ struct drm_connector
>> *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
>> /**
>> * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
>> connector
>> * @connector_fwnode: fwnode_handle to report the event on
>> + * @status: hot plug detect logical state
>> *
>> * On some hardware a hotplug event notification may come from outside the
>> display
>> * driver / device. An example of this is some USB Type-C setups where the
>> hardware
>> @@ -2803,7 +2804,8 @@ struct drm_connector
>> *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
>> * This function can be used to report these out-of-band events after
>> obtaining
>> * a drm_connector reference through calling drm_connector_find_by_fwnode().
>> */
>> -void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode)
>> +void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
>> + enum drm_connector_status status)
>> {
>> struct drm_connector *connector;
>>
>> @@ -2812,7 +2814,7 @@ void drm_connector_oob_hotplug_event(struct
>> fwnode_handle *connector_fwnode)
>> return;
>>
>> if (connector->funcs->oob_hotplug_event)
>> -connector->funcs->oob_hotplug_event(connector);
>> +connector->funcs->oob_hotplug_event(connector, status);
>>
>> drm_connector_put(connector);
>> }
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> b/drivers/gpu/drm/i915/display/intel_dp.c
>> index e4a79c11fd25..56cc023f7bbd 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -4951,15 +4951,26 @@ static int intel_dp_connector_atomic_check(struct
>> drm_connector *conn,
>> return intel_modeset_synced_crtcs(state, conn);
>> }
>>
>> -static void intel_dp_oob_hotplug_event(struct drm_connector *connector)
>> +static void intel_dp_oob_hotplug_event(struct drm_connector *connector,
>> + enum drm_connector_status hpd_state)
>> {
>> struct intel_encoder *encoder =
>> intel_attached_encoder(to_intel_connector(connector));
>> struct drm_i915_private *i915 = to_i915(connector->dev);
>> +bool hpd_high = hpd_state == connector_status_connected;
>> +unsigned int hpd_pin = encoder->hpd_pin;
>> +bool need_work = false;
>>
>> spin_lock_irq(>irq_lock);
>> -i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
>> +if (hpd_high != test_bit(hpd_pin,
>> >hotplug.oob_hotplug_last_state)) {
>> +i915->hotplug.event_bits |= BIT(hpd_pin);
>> +
>> +__assign_bit(hpd_pin, >hotplug.oob_hotplug_last_state,
>> hpd_high);
>> +need_work = true;
>> +}
>> spin_unlock_irq(>irq_lock);
>> -queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
>> +
>> +if (need_work)
>> +queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
>> }
>>
>> static const struct drm_connector_funcs intel_dp_connector_funcs = {
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 24111bf42ce0..96c088bb5522 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -135,6 +135,9 @@ struct i915_hotplug {
>> /* Whether or not to count short HPD IRQs in HPD storms */
>> u8 hpd_short_storm_enabled;
>>
>> +/* Last state reported by oob_hotplug_event for each encoder */
>> +unsigned long oob_hotplug_last_state;
>> +
>> /*
>> * if we get a HPD
Re: [PATCH v4 2/5] drm: Add HPD state to drm_connector_oob_hotplug_event()
+Hans
Hans, do you have any comments?
On Mon, May 02, 2022 at 09:53:13AM -0700, Bjorn Andersson wrote:
> In some implementations, such as the Qualcomm platforms, the display
> driver has no way to query the current HPD state and as such it's
> impossible to distinguish between disconnect and attention events.
>
> Add a parameter to drm_connector_oob_hotplug_event() to pass the HPD
> state.
>
> Also push the test for unchanged state in the displayport altmode driver
> into the i915 driver, to allow other drivers to act upon each update.
>
> Signed-off-by: Bjorn Andersson
> ---
>
> Changes since v3:
> - Transition to drm_connector_status instead of custom hpd_state
>
> drivers/gpu/drm/drm_connector.c | 6 --
> drivers/gpu/drm/i915/display/intel_dp.c | 17 ++---
> drivers/gpu/drm/i915/i915_drv.h | 3 +++
> drivers/usb/typec/altmodes/displayport.c | 10 +++---
> include/drm/drm_connector.h | 6 --
> 5 files changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 1c48d162c77e..e86c69f0640f 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2794,6 +2794,7 @@ struct drm_connector
> *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> /**
> * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
> connector
> * @connector_fwnode: fwnode_handle to report the event on
> + * @status: hot plug detect logical state
> *
> * On some hardware a hotplug event notification may come from outside the
> display
> * driver / device. An example of this is some USB Type-C setups where the
> hardware
> @@ -2803,7 +2804,8 @@ struct drm_connector
> *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> * This function can be used to report these out-of-band events after
> obtaining
> * a drm_connector reference through calling drm_connector_find_by_fwnode().
> */
> -void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode)
> +void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
> + enum drm_connector_status status)
> {
> struct drm_connector *connector;
>
> @@ -2812,7 +2814,7 @@ void drm_connector_oob_hotplug_event(struct
> fwnode_handle *connector_fwnode)
> return;
>
> if (connector->funcs->oob_hotplug_event)
> - connector->funcs->oob_hotplug_event(connector);
> + connector->funcs->oob_hotplug_event(connector, status);
>
> drm_connector_put(connector);
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index e4a79c11fd25..56cc023f7bbd 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4951,15 +4951,26 @@ static int intel_dp_connector_atomic_check(struct
> drm_connector *conn,
> return intel_modeset_synced_crtcs(state, conn);
> }
>
> -static void intel_dp_oob_hotplug_event(struct drm_connector *connector)
> +static void intel_dp_oob_hotplug_event(struct drm_connector *connector,
> +enum drm_connector_status hpd_state)
> {
> struct intel_encoder *encoder =
> intel_attached_encoder(to_intel_connector(connector));
> struct drm_i915_private *i915 = to_i915(connector->dev);
> + bool hpd_high = hpd_state == connector_status_connected;
> + unsigned int hpd_pin = encoder->hpd_pin;
> + bool need_work = false;
>
> spin_lock_irq(>irq_lock);
> - i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
> + if (hpd_high != test_bit(hpd_pin,
> >hotplug.oob_hotplug_last_state)) {
> + i915->hotplug.event_bits |= BIT(hpd_pin);
> +
> + __assign_bit(hpd_pin, >hotplug.oob_hotplug_last_state,
> hpd_high);
> + need_work = true;
> + }
> spin_unlock_irq(>irq_lock);
> - queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
> +
> + if (need_work)
> + queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
> }
>
> static const struct drm_connector_funcs intel_dp_connector_funcs = {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 24111bf42ce0..96c088bb5522 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -135,6 +135,9 @@ struct i915_hotplug {
> /* Whether or not to count short HPD IRQs in HPD storms */
> u8 hpd_short_storm_enabled;
>
> + /* Last state reported by oob_hotplug_event for each encoder */
> + unsigned long oob_hotplug_last_state;
> +
> /*
>* if we get a HPD irq from DP and a HPD irq from non-DP
>* the non-DP HPD could block the workqueue on a mode config
> diff --git a/drivers/usb/typec/altmodes/displayport.c
> b/drivers/usb/typec/altmodes/displayport.c
> index
