Hi Neil,
Le ven., mars 4 2022 at 14:30:46 +0100, Neil Armstrong
<[email protected]> a ?crit :
> Hi,
>
> On 03/03/2022 18:59, H. Nikolaus Schaller wrote:
>> Hi Paul, Neil,
>>
>>> Am 03.03.2022 um 18:20 schrieb Paul Cercueil <[email protected]>:
>>>
>>> Hi Nikolaus,
>>>
>>> [snip]
>>>
>>>>> Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do
>>>>> have access to the main drm_device in the ingenic_drm_bind()
>>>>> function, so you can add it there (with a cleanup function
>>>>> calling drm_kms_helper_poll_fini() registered with
>>>>> drmm_add_action_or_reset()).
>>>> Well, do you really want to mix HPD detection between connector,
>>>> Synopsys bridge and Ingenic DRM core? These are independent...
>>>> Or should be accessed only through the bridge chain pointers.
>>>> IMHO we should keep separate functions separate.
>>>
>>> The drm_kms_helper_poll_init() just says "this DRM device may have
>>> connectors that need to be polled" so it very well fits inside the
>>> main driver, IMHO.
>>
>> As far as I understand, it has the side-effect to always set
>> dev->mode_config.poll_enabled and
>> schedule_delayed_work() for all devices.
>> I am not sure if this is intended for arbitrary ingenic-drm devices.
>> But you know better than me.
>>
>>
>> Hm. But wait, I think I now finally remember why I have proposed it
>> the way it is!
>> It is always better to go back to requirements and find the least
>> invasive solution.
>>
>> - HPD IRQ works and calls dw_hdmi_irq() [as can be shown by adding
>> printk()]
>> - it is just that the udevd is only notified if poll_enabled = true
>> (but no polling takes place!).
>>
>> An earlier version (v4) to fix this proposed to add an explicit call
>> to drm_kms_helper_hotplug_event()
>> in dw_hdmi_irq() but that was rejected a while ago because
>> drm_helper_hpd_irq_event() will already call it:
>>
>> https://www.spinics.net/lists/dri-devel/msg316846.html
>>
>> Since this did not take into account that
>> dev->mode_config.poll_enabled must be set true, I then proposed the
>> enable_poll() mechanism just to set this bit for the ingenic-dw-hdmi
>> specialization.
>>
>> So a HPD event is delivered to the dw-hdmi driver as dw_hdmi_irq()
>> and that calls drm_helper_hpd_irq_event()
>> but not drm_kms_helper_hotplug_event() and user-space is not getting
>> aware.
>>
>> It is all a hack because we mix the dw-hdmi driver which originally
>> did register its own connector
>> with an explicit connector...
>>
>> In summary I now thing that the v4 patch is the simplest and least
>> invasive solution.
>>
>> We neither have to introduce a dw_hdmi_enable_poll() function or
>> call drm_kms_helper_poll_init() anywhere.
>>
>> It is just a single line to add to dw-hdmi. And neither touches
>> ingenic-dw-hdmi nor ingenic-drm-drv.
>>
>> So let's go back to v4 version (just modify commit message to better
>> describe why we have to call
>> drm_kms_helper_hotplug_event() explicitly) and forget about
>> alternatives.
>
> Please don't and add drm_kms_helper_poll_init() from the
> ingenic-drm-drv.c like every other DRM driver.
>
> Adding drm_kms_helper_hotplug_event() in dw-hdmi will impact other
> drivers using dw-hdmi but correctly
> calling drm_kms_helper_poll_init().
From what I understood in Nikolaus' last message, HDMI hotplug is
actually correctly detected, so there's no need for polling. What is
missing is the call to drm_kms_helper_hotplug_event *somewhere*, so
that the information is correctly relayed to userspace.
I think this issue can be fixed by calling
drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c.
Cheers,
-Paul
Hi Paul, Neil,
> Am 04.03.2022 um 17:47 schrieb Paul Cercueil <[email protected]>:
>
> From what I understood in Nikolaus' last message, HDMI hotplug is actually correctly detected, so there's no need for polling. What is missing is the call to drm_kms_helper_hotplug_event *somewhere*, so that the information is correctly relayed to userspace.
Exactly.
As Maxime pointed out it should already be called by drm_helper_hpd_irq_event() in dw_hdmi_irq() but isn't
because mode_config.poll_enabled isn't enabled.
So we can either
a) enable mode_config.poll_enabled so that it is called by drm_helper_hpd_irq_event() or
b) make drm_kms_helper_hotplug_event() being called explicitly in dw_hdmi_irq().
We could guard that by mode_config.poll_enabled to avoid drm_kms_helper_hotplug_event()
being called twice (but I think the "changed" mechanism will take care of).
> I think this issue can be fixed by calling drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c.
I don't see yet how this would solve it, but it may work.
Anyways, this all is a missing feature (sometimes called "bug") of the *dw-hdmi driver* and IMHO
neither of the connector nor the ingenic-drm-drv.
So I think it should not be solved outside dw-hdmi.
BR and thanks,
Nikolaus