linux-kernel - Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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