> -----Original Message----- > From: Inki Dae <[email protected]> > Sent: Monday, November 10, 2025 11:24 AM > To: Hoyoung Lee <[email protected]> > Cc: Seung-Woo Kim <[email protected]>; Kyungmin Park > <[email protected]>; David Airlie <[email protected]>; Simona > Vetter <[email protected]>; Krzysztof Kozlowski <[email protected]>; Alim > Akhtar <[email protected]>; [email protected]; linux- > [email protected]; [email protected]; linux- > [email protected] > Subject: Re: [PATCH 1/3] drm/exynos: plane: Disable fully off-screen > planes instead of zero-sized update > > Thanks for contribution, > > 2025년 9월 29일 (월) 오후 1:29, Hoyoung Lee <[email protected]>님이 작 > 성: > > > > Some configurations require additional actions when all windows are > > disabled to keep DECON operating correctly. Programming a zero-sized > > window in ->atomic_update() leaves the plane logically enabled and can > > bypass those disable semantics. > > > > Treat a fully off-screen plane as not visible and take the explicit > > disable path. > > > > Implementation details: > > - exynos_plane_mode_set(): if computed actual_w/actual_h is zero, mark > > state->visible = false and return early. > > - exynos_plane_atomic_check(): if !visible, skip further checks and > > return 0. > > - exynos_plane_atomic_update(): if !visible, call ->disable_plane(); > > otherwise call ->update_plane(). > > > > No functional change for visible planes; off-screen planes are now > > cleanly disabled, ensuring the disable hooks run consistently. > > > > Signed-off-by: Hoyoung Lee <[email protected]> > > --- > > drivers/gpu/drm/exynos/exynos_drm_plane.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c > > b/drivers/gpu/drm/exynos/exynos_drm_plane.c > > index 7c3aa77186d3..842974154d79 100644 > > --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c > > +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c > > @@ -91,6 +91,11 @@ static void exynos_plane_mode_set(struct > exynos_drm_plane_state *exynos_state) > > actual_w = exynos_plane_get_size(crtc_x, crtc_w, mode->hdisplay); > > actual_h = exynos_plane_get_size(crtc_y, crtc_h, > > mode->vdisplay); > > > > + if (!actual_w || !actual_h) { > > + state->visible = false; > > The state->visible field in the DRM atomic framework is set to true only > when the following conditions are met: > - Both state->crtc and state->fb are present (having only one of them > results in an error). > - src_w/src_h and crtc_w/crtc_h are non-zero. > - The source rectangle does not exceed the framebuffer bounds (e.g., src_x > + src_w <= fb->width). > - Rotation and clipping checks pass successfully. > > However, this patch modifies the state->visible value within vendor- > specific code. Doing so can be problematic because it overrides a field > that is managed by the DRM atomic framework. Even if it currently works, > it may lead to unexpected behavior in the future. > > For example, if the DRM atomic framework sets visible = true after > validating the above conditions and begins processing certain logic, but > the vendor driver later changes it to false, the framework may still > assume the variable remains true, resulting in inconsistent states. > > Turning off a plane when it doesn’t need to be displayed is a good idea I > think. You might consider contributing this behavior upstream so it can be > properly handled within the DRM atomic framework itself. > > Thanks, > Inki Dae > > > + return; > > + } > > + > > if (crtc_x < 0) { > > if (actual_w) > > src_x += ((-crtc_x) * exynos_state->h_ratio) > > >> 16; @@ -244,6 +249,9 @@ static int exynos_plane_atomic_check(struct > drm_plane *plane, > > /* translate state into exynos_state */ > > exynos_plane_mode_set(exynos_state); > > > > + if (!new_plane_state->visible) > > + return 0; > > + > > ret = exynos_drm_plane_check_format(exynos_plane->config, > exynos_state); > > if (ret) > > return ret; > > @@ -263,8 +271,10 @@ static void exynos_plane_atomic_update(struct > drm_plane *plane, > > if (!new_state->crtc) > > return; > > > > - if (exynos_crtc->ops->update_plane) > > + if (new_state->visible && exynos_crtc->ops->update_plane) > > exynos_crtc->ops->update_plane(exynos_crtc, > > exynos_plane); > > + else if (exynos_crtc->ops->disable_plane) > > + exynos_crtc->ops->disable_plane(exynos_crtc, > > + exynos_plane); > > } > > > > static void exynos_plane_atomic_disable(struct drm_plane *plane, > > -- > > 2.34.1 > > > >
Hi Inki, Thanks for the review. I agree that mutating state->visible value in vendor code is risky. Rather than touching the DRM atomic framework or that field, how about we add a driver-private flag in Exynos(e.g. exynos_drm_plane_state->visible) and use that instead? If this approach sounds reasonable to you, I’ll spin a v2 along these lines. BRs, Hoyoung Lee
