Hi,
On 03/03/2022 18:59, H. Nikolaus Schaller wrote:
Hi Paul, Neil,
Am 03.03.2022 um 18:20 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>:
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().
Neil
BR,
Nikolaus
