Hi,
On 03/03/2022 18:59, H. Nikolaus Schaller wrote:
Hi Paul, Neil,
Am 03.03.2022 um 18:20 schrieb Paul Cercueil <p...@crapouillou.net>:
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
