Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.21\))
Subject: Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce
 dw_hdmi_enable_poll()
From:   "H. Nikolaus Schaller" <hns@goldelico.com>
In-Reply-To: <RUI68R.Z009SPJAAD8N1@crapouillou.net>
Date:   Thu, 3 Mar 2022 18:59:47 +0100
Cc:     Paul Boddie <paul@boddie.org.uk>, Daniel Vetter <daniel@ffwll.ch>,
        Jonas Karlman <jonas@kwiboo.se>,
        David Airlie <airlied@linux.ie>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        linux-mips <linux-mips@vger.kernel.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>,
        Robert Foss <robert.foss@linaro.org>,
        Andrzej Hajda <andrzej.hajda@intel.com>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Discussions about the Letux Kernel 
        <letux-kernel@openphoenux.org>, Maxime Ripard <maxime@cerno.tech>
Content-Transfer-Encoding: quoted-printable
Message-Id: <F0F8F36B-3A0A-476C-8C7D-566255C629C6@goldelico.com>
References: <cover.1645895582.git.hns@goldelico.com>
 <e54838849f80454b863f9f5634dd10f79ef7bb8f.1645895582.git.hns@goldelico.com>
 <983e9064-17ad-e646-f37d-ca9173ba0967@baylibre.com>
 <C8AE9A7A-E288-4637-ACAD-40CD33CD5F8C@goldelico.com>
 <3E620AF4-402E-45EA-9D92-92EAEA9647F5@goldelico.com>
 <SHH68R.Z3J9KSY0GQVA2@crapouillou.net>
 <ABC1BD09-383B-4499-B034-340CE88725B3@goldelico.com>
 <RUI68R.Z009SPJAAD8N1@crapouillou.net>
To:     Paul Cercueil <paul@crapouillou.net>,
        Neil Armstrong <narmstrong@baylibre.com>
Precedence: bulk

Hi Paul, Neil,

> Am 03.03.2022 um 18:20 schrieb Paul Cercueil <paul@crapouillou.net>:
>=20
> Hi Nikolaus,
>=20
> [snip]
>=20
>>> 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.
>=20
> 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 =3D 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=