lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-Id: <VYB88R.ATGIVGZ13PFM1@crapouillou.net> Date: Fri, 04 Mar 2022 16:47:19 +0000 From: Paul Cercueil <paul@...pouillou.net> To: Neil Armstrong <narmstrong@...libre.com> Cc: "H. Nikolaus Schaller" <hns@...delico.com>, Paul Boddie <paul@...die.org.uk>, Andrzej Hajda <andrzej.hajda@...el.com>, Maxime Ripard <maxime@...no.tech>, Jonas Karlman <jonas@...boo.se>, David Airlie <airlied@...ux.ie>, Robert Foss <robert.foss@...aro.org>, linux-mips <linux-mips@...r.kernel.org>, dri-devel <dri-devel@...ts.freedesktop.org>, linux-kernel <linux-kernel@...r.kernel.org>, Kieran Bingham <kieran.bingham+renesas@...asonboard.com>, Jernej Skrabec <jernej.skrabec@...il.com>, Discussions about the Letux Kernel <letux-kernel@...nphoenux.org>, Laurent Pinchart <Laurent.pinchart@...asonboard.com> Subject: Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll() Hi Neil, Le ven., mars 4 2022 at 14:30:46 +0100, Neil Armstrong <narmstrong@...libre.com> 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@...pouillou.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(). 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
Powered by blists - more mailing lists