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.
-Paul
>
> And maybe this should also be conditional? Maybe not depend on
> compatible = jz4780 but compatible = ci20?
>
> Looks to me to be a quick fix in the wrong place.
>
> Let's fix the CSC issue first.
>
> BR,
> Nikolaus
>
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.
BR,
Nikolaus
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().
Neil
>
> BR,
> Nikolaus