Message-ID: <CAAQKjZMAPtisBhgcZE1QVoUsG8n=4PGzNB9rdVtjxaVnGc0YOA@mail.gmail.com>
Date: Thu, 13 Nov 2025 23:22:55 +0900
From: Inki Dae <daeinki@...il.com>
To: hy_fifty.lee@...sung.com
Cc: Seung-Woo Kim <sw0312.kim@...sung.com>, Kyungmin Park <kyungmin.park@...sung.com>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	Krzysztof Kozlowski <krzk@...nel.org>, Alim Akhtar <alim.akhtar@...sung.com>, 
	dri-devel@...ts.freedesktop.org, linux-arm-kernel@...ts.infradead.org, 
	linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] drm/exynos: plane: Disable fully off-screen planes
 instead of zero-sized update

Hi Hoyoung,

2025년 11월 12일 (수) 오전 11:44, <hy_fifty.lee@...sung.com>님이 작성:
>
> > -----Original Message-----
> > From: Inki Dae <daeinki@...il.com>
> > Sent: Monday, November 10, 2025 11:24 AM
> > To: Hoyoung Lee <hy_fifty.lee@...sung.com>
> > Cc: Seung-Woo Kim <sw0312.kim@...sung.com>; Kyungmin Park
> > <kyungmin.park@...sung.com>; David Airlie <airlied@...il.com>; Simona
> > Vetter <simona@...ll.ch>; Krzysztof Kozlowski <krzk@...nel.org>; Alim
> > Akhtar <alim.akhtar@...sung.com>; dri-devel@...ts.freedesktop.org; linux-
> > arm-kernel@...ts.infradead.org; linux-samsung-soc@...r.kernel.org; linux-
> > kernel@...r.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@...sung.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@...sung.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.
>

For now, it is fine to add a driver-private flag and apply the change
only to Exynos. However, I believe this feature can also be generally
applied to other SoCs whose display controllers support multiple
hardware overlays, not just Exynos. Therefore, it would be ideal if
this could eventually be integrated into the DRM atomic framework so
that other SoCs may also take advantage of it in the future.

Thanks,
Inki Dae

> BRs,
> Hoyoung Lee
>
>
>

Powered by blists - more mailing lists