Re: [PATCH 1/6] drm/connector: report IRQ_HPD events to drm_connector_oob_hotplug_event()

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hi,

On 20/04/2026 14:45, Dmitry Baryshkov wrote:
> On Mon, Apr 20, 2026 at 02:01:57PM +0300, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 20/04/2026 12:50, Dmitry Baryshkov wrote:
>>> On Mon, Apr 20, 2026 at 07:50:46AM +0300, Tomi Valkeinen wrote:
>>>> Hi,
>>>>
>>>> On 18/04/2026 01:32, Dmitry Baryshkov wrote:
>>>>> On Thu, Apr 16, 2026 at 11:10:03AM +0300, Tomi Valkeinen wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 16/04/2026 02:22, Dmitry Baryshkov wrote:
>>>>>>> The DisplayPort standard defines a special kind of events called IRQ.
>>>>>>> These events are used to notify DP Source about the events on the Sink
>>>>>>> side. It is extremely important for DP MST handling, where the MST
>>>>>>> events are reported through this IRQ.
>>>>>>>
>>>>>>> In case of the USB-C DP AltMode there is no actual HPD pulse, but the
>>>>>>> events are ported through the bits in the AltMode VDOs.
>>>>>>>
>>>>>>> Extend the drm_connector_oob_hotplug_event() interface and report IRQ
>>>>>>> events to the DisplayPort Sink drivers.
>>>>>>>
>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/drm_connector.c          |  4 +++-
>>>>>>>      drivers/usb/typec/altmodes/displayport.c | 12 ++++++++----
>>>>>>>      include/drm/drm_connector.h              |  3 ++-
>>>>>>>      3 files changed, 13 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>>>>>> index 47dc53c4a738..5fdacbd84bd7 100644
>>>>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>>>>> @@ -3510,6 +3510,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
>>>>>>> + * @irq_hpd: HPD pulse detected
>>>>>>>       *
>>>>>>>       * 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
>>>>>>> @@ -3520,7 +3521,8 @@ struct drm_connector *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
>>>>>>>       * a drm_connector reference through calling drm_connector_find_by_fwnode().
>>>>>>>       */
>>>>>>>      void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
>>>>>>> -				     enum drm_connector_status status)
>>>>>>> +				     enum drm_connector_status status,
>>>>>>> +				     bool irq_hpd)
>>>>>> I find the "IRQ HPD" naming always confusing, even if I'm somewhat familiar
>>>>>> with DP, but if someone has mainly worked on HDMI, I'm sure it's even worse.
>>>>>>
>>>>>> Can we define this a bit more precisely? Is 'irq_hpd' only for displayport?
>>>>>> If so, perhaps 'dp_irq_hpd' or 'displayport_irq_hpd'. I might even call it
>>>>>> 'dp_hpd_pulse', but maybe that's not good as the spec talks about HPD pulse
>>>>>> for both short and long ones (although in the kernel doc you just write "HPD
>>>>>> pulse")... The kernel doc could be expanded a bit to make it clear what this
>>>>>> flag indicates.
>>>>>
>>>>> I attempted to stay away from defining a DP-specific flag, keeping it
>>>>> generic enough. HDMI is pretty close (IMO) to requiring separate flag in
>>>>
>>>> If it's not specifically the DP IRQ HPD, then we need to define what it
>>>> means. I tried to think what it would mean with HDMI, but I didn't come up
>>>> with anything.
>>>
>>> I might be mistaken, but I had someting like HEAC HPD / EDID status
>>> changes in mind (or HDCP-triggered HPD status changes). But here I
>>> admit, I hadn't checked if it is actually applicable or not.
>>
>> Possibly, I'm not familiar with those.
>>
>>> Anyway, for e.g. DVI or VGA that means nothing. But, my point really is
>>> to abstain from defining someting as DP-only in the top-level API.
>>
>> I'm fine with that, but then it really has to be defined =).
>>
>>>>> Linux. Likewise I'd rather not use "pulse". The DP AltMode defines a bit
>>>>> in the VDO rather than a pulse.
>>>>>
>>>>> Anyway, if irq_hpd doesn't sound precise enough, what about "bool
>>>>> extra_irq"? This would convey that this is the extra hpd-related IRQ,
>>>>> but it would also be obvious that it's not related to the HPD pin
>>>>> itself.
>>>> We'd still need to define what exactly it means. I think it might be better
>>>> to just define it as the DP IRQ HPD, as then the meaning is clear.
>>>>
>>>> Also, would an enum flags parameter be better than a bool parameter?
>>>
>>> Maybe not enum, but u32 param. Then it can become:
>>>
>>> @extra_status: additional type-specific information provided by the sink
>>> without changing the HPD state
>>>
>>> void drm_connector_oob_hotplug_event(..., u32 extra_status);
>>>
>>> /* DP short HPD pulse or corresponding AltMode flag */
>>> #define DRM_CONNECTOR_OOB_DP_IRQ_HPD BIT(0)
>>> /* DP long HPD pulse, debounced XXX: do we need this? */
>>> #define DRM_CONNECTOR_OOB_DP_REPLUG BIT(1)
>>
>> Why is u32 better than enum? So that we could e.g. pass short values inside
>> the extra_status?
> 
> No, my thought was to be able to define values specific to the
> particular connector types and to be able to combine those values.
> 
> After sending the email I started thinking about the bridged and
> corresponding notifications. There having overlapping values will not
> work becasue bridges in the chanin don't easily know the final connector
> type.

An enum can have overlapping values. I don't think there's much 
difference between u32 and an enum in C. I just like enum because 1) it 
groups the possible values in the header file, and 2) the function 
parameters can use the enum type, making it obvious what flags you are 
supposed to use there.

> I think you are correct here, it should be the enum. With the first
> iteration defined as:
> 
> /**
>   * enum drm_connector_status_extra - additional events sent by the sink
>   * together or in replacement of the HPD status changes
>   /
> enum drm_connector_status_extra {
>    /**
>     * @DRM_CONNECTOR_DP_IRQ_HPD: DisplayPort Sink has sent the
>     * IRQ_HPD (either by the HPD short pulse or via the AltMode event).
>     */
>    DRM_CONNECTOR_DP_IRQ_HPD = BIT(0),
> };
> 
> /**
>    * @extra_status: additional information provided by the sink without
>    * changing the HPD state (or in addition to such a change). It is an
>    * OR of the values defined in the drm_connector_status_extra enum.
>    */
> void drm_connector_oob_hotplug_event(..., u32 extra_status);

Looks good to me, except I'd use "enum drm_connector_status_extra" 
instead of u32 there in the function parameters.

  Tomi