RE: [PATCH 1/3] drm/exynos: plane: Disable fully off-screen planes instead of zero-sized update

[<hy_fifty.lee@samsung.com>]

> -----Original Message-----
> From: Inki Dae <daeinki@gmail.com>
> Sent: Monday, November 10, 2025 11:24 AM
> To: Hoyoung Lee <hy_fifty.lee@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>; Kyungmin Park
> <kyungmin.park@samsung.com>; David Airlie <airlied@gmail.com>; Simona
> Vetter <simona@ffwll.ch>; Krzysztof Kozlowski <krzk@kernel.org>; Alim
> Akhtar <alim.akhtar@samsung.com>; dri-devel@lists.freedesktop.org; linux-
> arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; linux-
> kernel@vger.kernel.org
> 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 <hy_fifty.lee@samsung.com>님이 작
> 성:
> >
> > 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 <hy_fifty.lee@samsung.com>
> > ---
> >  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