Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[Paul Cercueil]

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