Hi Neil,
Le ven., mars 4 2022 at 14:30:46 +0100, Neil Armstrong
<narmstrong@xxxxxxxxxxxx> 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 <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().
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
