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

[Dmitry Baryshkov]

On 10/07/2023 12:10, Thomas Zimmermann wrote:
> 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@xxxxxxxxxxxx>
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2649
> 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@xxxxxxx>
> Tested-by: Moritz Duge <MoritzDuge@xxxxxxxxxxxx>
> Tested-by: Torsten Krah <krah.tm@xxxxxxxxx>
> Tested-by: Paul Schyska <pschyska@xxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: David Airlie <airlied@xxxxxxxxx>
> Cc: Noralf Trønnes <noralf@xxxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> Cc: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> Cc: Russell King <linux@xxxxxxxxxxxxxxx>
> Cc: Inki Dae <inki.dae@xxxxxxxxxxx>
> Cc: Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx>
> Cc: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Cc: Patrik Jakobsson <patrik.r.jakobsson@xxxxxxxxx>
> Cc: Rob Clark <robdclark@xxxxxxxxx>
> Cc: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Cc: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Cc: "Christian König" <christian.koenig@xxxxxxx>
> Cc: "Pan, Xinhui" <Xinhui.Pan@xxxxxxx>
> Cc: Thierry Reding <thierry.reding@xxxxxxxxx>
> Cc: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-samsung-soc@xxxxxxxxxxxxxxx
> Cc: linux-arm-msm@xxxxxxxxxxxxxxx
> Cc: freedreno@xxxxxxxxxxxxxxxxxxxxx
> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: linux-tegra@xxxxxxxxxxxxxxx
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: <stable@xxxxxxxxxxxxxxx> # v5.2+
> ---
>   drivers/gpu/drm/armada/armada_fbdev.c     |  4 ----
>   drivers/gpu/drm/drm_client.c              | 21 +++++++++++++++++++++
>   drivers/gpu/drm/drm_fbdev_dma.c           |  4 ----
>   drivers/gpu/drm/drm_fbdev_generic.c       |  4 ----
>   drivers/gpu/drm/exynos/exynos_drm_fbdev.c |  4 ----
>   drivers/gpu/drm/gma500/fbdev.c            |  4 ----
>   drivers/gpu/drm/msm/msm_fbdev.c           |  4 ----

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> # msm

>   drivers/gpu/drm/omapdrm/omap_fbdev.c      |  4 ----
>   drivers/gpu/drm/radeon/radeon_fbdev.c     |  4 ----
>   drivers/gpu/drm/tegra/fbdev.c             |  4 ----
>   10 files changed, 21 insertions(+), 36 deletions(-)

BTW: As you have been clearing this area. I see that significant amount 
of DRM drivers use exactly the same code for msm_fbdev_client_funcs and 
for the significant part of foo_fbdev_setup(). Do you have any plans for 
moving that into a library / generic code? If not, I can take a look at 
crafting the patch.

--
With best wishes
Dmitry