[PATCH] drm/client: Send hotplug event after registering a client

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann at suse.de> writes:

Hello Thomas,

> Generate a hotplug event after registering a client to allow the
> client to configure its display. Remove the hotplug calls from the
> existing clients for fbdev emulation. This change fixes a concurrency
> bug between registering a client and receiving events from the DRM
> core. The bug is present in the fbdev emulation of all drivers.
>
> The fbdev emulation currently generates a hotplug event before
> registering the client to the device. For each new output, the DRM
> core sends an additional hotplug event to each registered client.
>
> If the DRM core detects first output between sending the artificial
> hotplug and registering the device, the output's hotplug event gets
> lost. If this is the first output, the fbdev console display remains
> dark. This has been observed with amdgpu and fbdev-generic.
>
> Fix this by adding hotplug generation directly to the client's
> register helper drm_client_register(). Registering the client and
> receiving events are serialized by struct drm_device.clientlist_mutex.
> So an output is either configured by the initial hotplug event, or
> the client has already been registered.
>
> The bug was originally added in commit 6e3f17ee73f7 ("drm/fb-helper:
> generic: Call drm_client_add() after setup is done"), in which adding
> a client and receiving a hotplug event switched order. It was hidden,
> as most hardware and drivers have at least on static output configured.
> Other drivers didn't use the internal DRM client or still had struct
> drm_mode_config_funcs.output_poll_changed set. That callback handled
> hotplug events as well. After not setting the callback in amdgpu in
> commit 0e3172bac3f4 ("drm/amdgpu: Don't set struct
> drm_driver.output_poll_changed"), amdgpu did not show a framebuffer
> console if output events got lost. The bug got copy-pasted from
> fbdev-generic into the other fbdev emulation.
>
> Reported-by: Moritz Duge <MoritzDuge at kolahilft.de>
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2649

Aren't you missing a Fixes: for 0e3172bac3f4 too? Since that's the commit
that unmasked the bug for amdgpu, IMO that is the most important to list.

> Fixes: 6e3f17ee73f7 ("drm/fb-helper: generic: Call drm_client_add() after setup is done")
> Fixes: 8ab59da26bc0 ("drm/fb-helper: Move generic fbdev emulation into separate source file")
> Fixes: b79fe9abd58b ("drm/fbdev-dma: Implement fbdev emulation for GEM DMA helpers")
> Fixes: 63c381552f69 ("drm/armada: Implement fbdev emulation as in-kernel client")
> Fixes: 49953b70e7d3 ("drm/exynos: Implement fbdev emulation as in-kernel client")
> Fixes: 8f1aaccb04b7 ("drm/gma500: Implement client-based fbdev emulation")
> Fixes: 940b869c2f2f ("drm/msm: Implement fbdev emulation as in-kernel client")
> Fixes: 9e69bcd88e45 ("drm/omapdrm: Implement fbdev emulation as in-kernel client")
> Fixes: e317a69fe891 ("drm/radeon: Implement client-based fbdev emulation")
> Fixes: 71ec16f45ef8 ("drm/tegra: Implement fbdev emulation as in-kernel client")
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> Tested-by: Moritz Duge <MoritzDuge at kolahilft.de>
> Tested-by: Torsten Krah <krah.tm at gmail.com>
> Tested-by: Paul Schyska <pschyska at gmail.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: David Airlie <airlied at gmail.com>
> Cc: Noralf Trønnes <noralf at tronnes.org>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Maxime Ripard <mripard at kernel.org>
> Cc: Javier Martinez Canillas <javierm at redhat.com>
> Cc: Russell King <linux at armlinux.org.uk>
> Cc: Inki Dae <inki.dae at samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim at samsung.com>
> Cc: Kyungmin Park <kyungmin.park at samsung.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org>
> Cc: Patrik Jakobsson <patrik.r.jakobsson at gmail.com>
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: Abhinav Kumar <quic_abhinavk at quicinc.com>
> Cc: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> Cc: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: "Christian König" <christian.koenig at amd.com>
> Cc: "Pan, Xinhui" <Xinhui.Pan at amd.com>
> Cc: Thierry Reding <thierry.reding at gmail.com>
> Cc: Mikko Perttunen <mperttunen at nvidia.com>
> Cc: dri-devel at lists.freedesktop.org
> Cc: linux-kernel at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-samsung-soc at vger.kernel.org
> Cc: linux-arm-msm at vger.kernel.org
> Cc: freedreno at lists.freedesktop.org
> Cc: amd-gfx at lists.freedesktop.org
> Cc: linux-tegra at vger.kernel.org
> Cc: dri-devel at lists.freedesktop.org
> Cc: <stable at vger.kernel.org> # v5.2+

While it's true that the but was introduced by commit 6e3f17ee73f7 and that
landed in v5.2, I wonder if this patch could even be applied to such olders
Linux versions. Probably in practice it would be at most backported to
v6.2, which is the release that exposed the bug for the amdgpu driver.

Your explanation makes sense to me and the patch looks good.

Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat