[PATCH 0/3] drm/exynos: KMS cleanups for off-screen planes and mode_config

[PATCH 0/3] drm/exynos: KMS cleanups for off-screen planes and mode_config

[Hoyoung Lee]

Hi all,

This small series makes Exynos KMS a bit more robust and better aligned
with common DRM layering:

- Patch 1 treats fully off-screen planes as not visible and routes them
through the explicit disable path instead of programming zero-sized
dimensions in ->atomic_update(). This ensures the proper disable
semantics run when a plane contributes nothing to the frame, and avoids
keeping a logically enabled window around.

- Patch 2 converts mode-config initialization to
drmm_mode_config_init(), tying the lifetime to drm_device and dropping
the now-unnecessary manual drm_mode_config_cleanup() in error/unbind
paths.

- Patch 3 moves the device-wide mode_config setup (funcs/helpers and
limits) from exynos_drm_fb.c to exynos_drm_drv.c and calls
drm_mode_config_init() from inside exynos_drm_mode_config_init().
Historically Exynos put this in fb.c when the driver grew around fbdev
helpers; today it obscures ownership and ordering. Placing it in drv.c
matches other vendors, makes the init order explicit (before creating
CRTC/planes/connectors and binding components), and centralizes device-
level policy in the core driver.

No userspace ABI changes. Comments and reviews are very welcome.

Thanks,
Hoyoung Lee.

Hoyoung Lee (3):
  drm/exynos: plane: Disable fully off-screen planes instead of
    zero-sized update
  drm/exynos: Convert to drmm_mode_config_init() and drop manual cleanup
  drm/exynos: Move mode_config setup from fb.c to drv.c

 drivers/gpu/drm/exynos/exynos_drm_drv.c   | 49 +++++++++++++++++++----
 drivers/gpu/drm/exynos/exynos_drm_fb.c    | 34 +---------------
 drivers/gpu/drm/exynos/exynos_drm_fb.h    |  7 +++-
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 12 +++++-
 4 files changed, 60 insertions(+), 42 deletions(-)

--
2.34.1

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

[Hoyoung Lee]

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;
+		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

[PATCH 2/3] drm/exynos: Convert to drmm_mode_config_init() and drop manual cleanup

[Hoyoung Lee]

Switch mode-config initialization to drmm_mode_config_init() so that the
lifetime is tied to drm_device. Remove explicit drm_mode_config_cleanup()
from error and unbind paths since cleanup is now managed by DRM.

No functional change intended.

Signed-off-by: Hoyoung Lee <hy_fifty.lee@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 6cc7bf77bcac..1aea71778ab1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -257,7 +257,7 @@ static int exynos_drm_bind(struct device *dev)
 	dev_set_drvdata(dev, drm);
 	drm->dev_private = (void *)private;
 
-	drm_mode_config_init(drm);
+	drmm_mode_config_init(drm);
 
 	exynos_drm_mode_config_init(drm);
 
@@ -297,7 +297,6 @@ static int exynos_drm_bind(struct device *dev)
 err_unbind_all:
 	component_unbind_all(drm->dev, drm);
 err_mode_config_cleanup:
-	drm_mode_config_cleanup(drm);
 	exynos_drm_cleanup_dma(drm);
 	kfree(private);
 	dev_set_drvdata(dev, NULL);
@@ -317,7 +316,6 @@ static void exynos_drm_unbind(struct device *dev)
 	drm_atomic_helper_shutdown(drm);
 
 	component_unbind_all(drm->dev, drm);
-	drm_mode_config_cleanup(drm);
 	exynos_drm_cleanup_dma(drm);
 
 	kfree(drm->dev_private);
-- 
2.34.1

[PATCH 3/3] drm/exynos: Move mode_config setup from fb.c to drv.c

[Hoyoung Lee]

Relocate exynos_drm_mode_config_init() and the mode_config funcs/helpers
from exynos_drm_fb.c to exynos_drm_drv.c, and invoke
drm_mode_config_init() from inside exynos_drm_mode_config_init().

Rationale: resolve the historical fb.c placement, align with common DRM
layering (mode_config is device-wide policy that belongs in the core
driver), and make initialization order explicit before creating KMS
objects and binding components.

No functional change intended.

Signed-off-by: Hoyoung Lee <hy_fifty.lee@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 47 ++++++++++++++++++++++---
 drivers/gpu/drm/exynos/exynos_drm_fb.c  | 34 ++----------------
 drivers/gpu/drm/exynos/exynos_drm_fb.h  |  7 ++--
 3 files changed, 49 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 1aea71778ab1..6362cd417a4e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -233,6 +233,43 @@ static struct component_match *exynos_drm_match_add(struct device *dev)
 	return match ?: ERR_PTR(-ENODEV);
 }
 
+static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = {
+	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
+static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
+	.fb_create = exynos_user_fb_create,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static int exynos_drm_mode_config_init(struct drm_device *dev)
+{
+	int ret;
+
+	ret = drmm_mode_config_init(dev);
+	if (ret)
+		return ret;
+
+	dev->mode_config.min_width = 0;
+	dev->mode_config.min_height = 0;
+
+	/*
+	 * set max width and height as default value(4096x4096).
+	 * this value would be used to check framebuffer size limitation
+	 * at drm_mode_addfb().
+	 */
+	dev->mode_config.max_width = 4096;
+	dev->mode_config.max_height = 4096;
+
+	dev->mode_config.funcs = &exynos_drm_mode_config_funcs;
+	dev->mode_config.helper_private = &exynos_drm_mode_config_helpers;
+
+	dev->mode_config.normalize_zpos = true;
+
+	return 0;
+}
+
 static int exynos_drm_bind(struct device *dev)
 {
 	struct exynos_drm_private *private;
@@ -257,9 +294,9 @@ static int exynos_drm_bind(struct device *dev)
 	dev_set_drvdata(dev, drm);
 	drm->dev_private = (void *)private;
 
-	drmm_mode_config_init(drm);
-
-	exynos_drm_mode_config_init(drm);
+	ret = exynos_drm_mode_config_init(drm);
+	if (ret)
+		goto err_free_private;
 
 	/* setup possible_clones. */
 	clone_mask = 0;
@@ -272,7 +309,7 @@ static int exynos_drm_bind(struct device *dev)
 	/* Try to bind all sub drivers. */
 	ret = component_bind_all(drm->dev, drm);
 	if (ret)
-		goto err_mode_config_cleanup;
+		goto err_free_private;
 
 	ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
 	if (ret)
@@ -296,7 +333,7 @@ static int exynos_drm_bind(struct device *dev)
 	drm_kms_helper_poll_fini(drm);
 err_unbind_all:
 	component_unbind_all(drm->dev, drm);
-err_mode_config_cleanup:
+err_free_private:
 	exynos_drm_cleanup_dma(drm);
 	kfree(private);
 	dev_set_drvdata(dev, NULL);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index ddd73e7f26a3..c118a079d308 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -8,8 +8,7 @@
  *	Seung-Woo Kim <sw0312.kim@samsung.com>
  */
 
-#include <drm/drm_atomic.h>
-#include <drm/drm_atomic_helper.h>
+#include <drm/drm_modeset_helper.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_fourcc.h>
@@ -93,7 +92,7 @@ exynos_drm_framebuffer_init(struct drm_device *dev,
 	return ERR_PTR(ret);
 }
 
-static struct drm_framebuffer *
+struct drm_framebuffer *
 exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
 		      const struct drm_format_info *info,
 		      const struct drm_mode_fb_cmd2 *mode_cmd)
@@ -150,32 +149,3 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index)
 	exynos_gem = to_exynos_gem(fb->obj[index]);
 	return exynos_gem->dma_addr + fb->offsets[index];
 }
-
-static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = {
-	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
-};
-
-static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
-	.fb_create = exynos_user_fb_create,
-	.atomic_check = drm_atomic_helper_check,
-	.atomic_commit = drm_atomic_helper_commit,
-};
-
-void exynos_drm_mode_config_init(struct drm_device *dev)
-{
-	dev->mode_config.min_width = 0;
-	dev->mode_config.min_height = 0;
-
-	/*
-	 * set max width and height as default value(4096x4096).
-	 * this value would be used to check framebuffer size limitation
-	 * at drm_mode_addfb().
-	 */
-	dev->mode_config.max_width = 4096;
-	dev->mode_config.max_height = 4096;
-
-	dev->mode_config.funcs = &exynos_drm_mode_config_funcs;
-	dev->mode_config.helper_private = &exynos_drm_mode_config_helpers;
-
-	dev->mode_config.normalize_zpos = true;
-}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.h b/drivers/gpu/drm/exynos/exynos_drm_fb.h
index fdc6cb40cc9c..0c79ce5d4a8d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.h
@@ -19,8 +19,11 @@ exynos_drm_framebuffer_init(struct drm_device *dev,
 			    struct exynos_drm_gem **exynos_gem,
 			    int count);
 
-dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index);
+struct drm_framebuffer *
+exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
+		      const struct drm_format_info *info,
+		      const struct drm_mode_fb_cmd2 *mode_cmd);
 
-void exynos_drm_mode_config_init(struct drm_device *dev);
+dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index);
 
 #endif
-- 
2.34.1

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

[Inki Dae]

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
>
>

Re: [PATCH 2/3] drm/exynos: Convert to drmm_mode_config_init() and drop manual cleanup

[Inki Dae]

2025년 9월 29일 (월) 오후 1:54, Hoyoung Lee <hy_fifty.lee@samsung.com>님이 작성:
>
> Switch mode-config initialization to drmm_mode_config_init() so that the
> lifetime is tied to drm_device. Remove explicit drm_mode_config_cleanup()
> from error and unbind paths since cleanup is now managed by DRM.
>
> No functional change intended.
>
> Signed-off-by: Hoyoung Lee <hy_fifty.lee@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 6cc7bf77bcac..1aea71778ab1 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -257,7 +257,7 @@ static int exynos_drm_bind(struct device *dev)
>         dev_set_drvdata(dev, drm);
>         drm->dev_private = (void *)private;
>
> -       drm_mode_config_init(drm);
> +       drmm_mode_config_init(drm);
>
>         exynos_drm_mode_config_init(drm);
>
> @@ -297,7 +297,6 @@ static int exynos_drm_bind(struct device *dev)
>  err_unbind_all:
>         component_unbind_all(drm->dev, drm);
>  err_mode_config_cleanup:
> -       drm_mode_config_cleanup(drm);

In the current implementation, there is a potential dereference issue
because the private object may be freed before to_dma_dev(dev) is
called.
When drmm_mode_config_init() is invoked, it registers
drm_mode_config_cleanup() as a managed action. This means that the
cleanup function will be automatically executed later when
drm_dev_put() is called.

The problem arises when drm_dev_put() is called without explicitly
invoking drm_mode_config_cleanup() first, as in the original code. In
that case, the managed cleanup is performed later, which allows
to_dma_dev(dev) to be called after the private object has already been
released.

For reference, the following sequence may occur internally when
drm_mode_config_cleanup() is executed:
1. drm_mode_config_cleanup() is called.
2. During the cleanup of FBs, planes, CRTCs, encoders, and connectors,
framebuffers or GEM objects may be released.
3. At this point, Exynos-specific code could invoke to_dma_dev(dev).

Therefore, the private object must remain valid until
drm_mode_config_cleanup() completes.
It would be safer to adjust the code so that kfree(private) is
performed after drm_dev_put(drm) to ensure the private data remains
available during cleanup.

Thanks,
Inki Dae

>         exynos_drm_cleanup_dma(drm);
>         kfree(private);
>         dev_set_drvdata(dev, NULL);
> @@ -317,7 +316,6 @@ static void exynos_drm_unbind(struct device *dev)
>         drm_atomic_helper_shutdown(drm);
>
>         component_unbind_all(drm->dev, drm);
> -       drm_mode_config_cleanup(drm);

Ditto.

>         exynos_drm_cleanup_dma(drm);
>
>         kfree(drm->dev_private);
> --
> 2.34.1
>
>

Re: [PATCH 3/3] drm/exynos: Move mode_config setup from fb.c to drv.c

[Inki Dae]

2025년 9월 29일 (월) 오후 1:29, Hoyoung Lee <hy_fifty.lee@samsung.com>님이 작성:
>
> Relocate exynos_drm_mode_config_init() and the mode_config funcs/helpers
> from exynos_drm_fb.c to exynos_drm_drv.c, and invoke
> drm_mode_config_init() from inside exynos_drm_mode_config_init().
>
> Rationale: resolve the historical fb.c placement, align with common DRM
> layering (mode_config is device-wide policy that belongs in the core
> driver), and make initialization order explicit before creating KMS
> objects and binding components.
>
> No functional change intended.

This patch looks fine to me.
However, since the second patch should be applied first, please repost
them as v2.

Thanks,
Inki Dae

>
> Signed-off-by: Hoyoung Lee <hy_fifty.lee@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c | 47 ++++++++++++++++++++++---
>  drivers/gpu/drm/exynos/exynos_drm_fb.c  | 34 ++----------------
>  drivers/gpu/drm/exynos/exynos_drm_fb.h  |  7 ++--
>  3 files changed, 49 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 1aea71778ab1..6362cd417a4e 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -233,6 +233,43 @@ static struct component_match *exynos_drm_match_add(struct device *dev)
>         return match ?: ERR_PTR(-ENODEV);
>  }
>
> +static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = {
> +       .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> +};
> +
> +static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
> +       .fb_create = exynos_user_fb_create,
> +       .atomic_check = drm_atomic_helper_check,
> +       .atomic_commit = drm_atomic_helper_commit,
> +};
> +
> +static int exynos_drm_mode_config_init(struct drm_device *dev)
> +{
> +       int ret;
> +
> +       ret = drmm_mode_config_init(dev);
> +       if (ret)
> +               return ret;
> +
> +       dev->mode_config.min_width = 0;
> +       dev->mode_config.min_height = 0;
> +
> +       /*
> +        * set max width and height as default value(4096x4096).
> +        * this value would be used to check framebuffer size limitation
> +        * at drm_mode_addfb().
> +        */
> +       dev->mode_config.max_width = 4096;
> +       dev->mode_config.max_height = 4096;
> +
> +       dev->mode_config.funcs = &exynos_drm_mode_config_funcs;
> +       dev->mode_config.helper_private = &exynos_drm_mode_config_helpers;
> +
> +       dev->mode_config.normalize_zpos = true;
> +
> +       return 0;
> +}
> +
>  static int exynos_drm_bind(struct device *dev)
>  {
>         struct exynos_drm_private *private;
> @@ -257,9 +294,9 @@ static int exynos_drm_bind(struct device *dev)
>         dev_set_drvdata(dev, drm);
>         drm->dev_private = (void *)private;
>
> -       drmm_mode_config_init(drm);
> -
> -       exynos_drm_mode_config_init(drm);
> +       ret = exynos_drm_mode_config_init(drm);
> +       if (ret)
> +               goto err_free_private;
>
>         /* setup possible_clones. */
>         clone_mask = 0;
> @@ -272,7 +309,7 @@ static int exynos_drm_bind(struct device *dev)
>         /* Try to bind all sub drivers. */
>         ret = component_bind_all(drm->dev, drm);
>         if (ret)
> -               goto err_mode_config_cleanup;
> +               goto err_free_private;
>
>         ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
>         if (ret)
> @@ -296,7 +333,7 @@ static int exynos_drm_bind(struct device *dev)
>         drm_kms_helper_poll_fini(drm);
>  err_unbind_all:
>         component_unbind_all(drm->dev, drm);
> -err_mode_config_cleanup:
> +err_free_private:
>         exynos_drm_cleanup_dma(drm);
>         kfree(private);
>         dev_set_drvdata(dev, NULL);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> index ddd73e7f26a3..c118a079d308 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -8,8 +8,7 @@
>   *     Seung-Woo Kim <sw0312.kim@samsung.com>
>   */
>
> -#include <drm/drm_atomic.h>
> -#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_modeset_helper.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_framebuffer.h>
>  #include <drm/drm_fourcc.h>
> @@ -93,7 +92,7 @@ exynos_drm_framebuffer_init(struct drm_device *dev,
>         return ERR_PTR(ret);
>  }
>
> -static struct drm_framebuffer *
> +struct drm_framebuffer *
>  exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
>                       const struct drm_format_info *info,
>                       const struct drm_mode_fb_cmd2 *mode_cmd)
> @@ -150,32 +149,3 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index)
>         exynos_gem = to_exynos_gem(fb->obj[index]);
>         return exynos_gem->dma_addr + fb->offsets[index];
>  }
> -
> -static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = {
> -       .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> -};
> -
> -static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
> -       .fb_create = exynos_user_fb_create,
> -       .atomic_check = drm_atomic_helper_check,
> -       .atomic_commit = drm_atomic_helper_commit,
> -};
> -
> -void exynos_drm_mode_config_init(struct drm_device *dev)
> -{
> -       dev->mode_config.min_width = 0;
> -       dev->mode_config.min_height = 0;
> -
> -       /*
> -        * set max width and height as default value(4096x4096).
> -        * this value would be used to check framebuffer size limitation
> -        * at drm_mode_addfb().
> -        */
> -       dev->mode_config.max_width = 4096;
> -       dev->mode_config.max_height = 4096;
> -
> -       dev->mode_config.funcs = &exynos_drm_mode_config_funcs;
> -       dev->mode_config.helper_private = &exynos_drm_mode_config_helpers;
> -
> -       dev->mode_config.normalize_zpos = true;
> -}
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.h b/drivers/gpu/drm/exynos/exynos_drm_fb.h
> index fdc6cb40cc9c..0c79ce5d4a8d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.h
> @@ -19,8 +19,11 @@ exynos_drm_framebuffer_init(struct drm_device *dev,
>                             struct exynos_drm_gem **exynos_gem,
>                             int count);
>
> -dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index);
> +struct drm_framebuffer *
> +exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> +                     const struct drm_format_info *info,
> +                     const struct drm_mode_fb_cmd2 *mode_cmd);
>
> -void exynos_drm_mode_config_init(struct drm_device *dev);
> +dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index);
>
>  #endif
> --
> 2.34.1
>
>

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

[hy_fifty.lee]

> -----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

RE: [PATCH 2/3] drm/exynos: Convert to drmm_mode_config_init() and drop manual cleanup

[hy_fifty.lee]

> -----Original Message-----
> From: Inki Dae <daeinki@gmail.com>
> Sent: Monday, November 10, 2025 2:22 PM
> 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 2/3] drm/exynos: Convert to drmm_mode_config_init()
> and drop manual cleanup
> 
> 2025년 9월 29일 (월) 오후 1:54, Hoyoung Lee <hy_fifty.lee@samsung.com>님이 작
> 성:
> >
> > Switch mode-config initialization to drmm_mode_config_init() so that
> > the lifetime is tied to drm_device. Remove explicit
> > drm_mode_config_cleanup() from error and unbind paths since cleanup is
> now managed by DRM.
> >
> > No functional change intended.
> >
> > Signed-off-by: Hoyoung Lee <hy_fifty.lee@samsung.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > index 6cc7bf77bcac..1aea71778ab1 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > @@ -257,7 +257,7 @@ static int exynos_drm_bind(struct device *dev)
> >         dev_set_drvdata(dev, drm);
> >         drm->dev_private = (void *)private;
> >
> > -       drm_mode_config_init(drm);
> > +       drmm_mode_config_init(drm);
> >
> >         exynos_drm_mode_config_init(drm);
> >
> > @@ -297,7 +297,6 @@ static int exynos_drm_bind(struct device *dev)
> >  err_unbind_all:
> >         component_unbind_all(drm->dev, drm);
> >  err_mode_config_cleanup:
> > -       drm_mode_config_cleanup(drm);
> 
> In the current implementation, there is a potential dereference issue
> because the private object may be freed before to_dma_dev(dev) is called.
> When drmm_mode_config_init() is invoked, it registers
> drm_mode_config_cleanup() as a managed action. This means that the cleanup
> function will be automatically executed later when
> drm_dev_put() is called.
> 
> The problem arises when drm_dev_put() is called without explicitly
> invoking drm_mode_config_cleanup() first, as in the original code. In that
> case, the managed cleanup is performed later, which allows
> to_dma_dev(dev) to be called after the private object has already been
> released.
> 
> For reference, the following sequence may occur internally when
> drm_mode_config_cleanup() is executed:
> 1. drm_mode_config_cleanup() is called.
> 2. During the cleanup of FBs, planes, CRTCs, encoders, and connectors,
> framebuffers or GEM objects may be released.
> 3. At this point, Exynos-specific code could invoke to_dma_dev(dev).
> 
> Therefore, the private object must remain valid until
> drm_mode_config_cleanup() completes.
> It would be safer to adjust the code so that kfree(private) is performed
> after drm_dev_put(drm) to ensure the private data remains available during
> cleanup.
> 
> Thanks,
> Inki Dae
> 
> >         exynos_drm_cleanup_dma(drm);
> >         kfree(private);
> >         dev_set_drvdata(dev, NULL);
> > @@ -317,7 +316,6 @@ static void exynos_drm_unbind(struct device *dev)
> >         drm_atomic_helper_shutdown(drm);
> >
> >         component_unbind_all(drm->dev, drm);
> > -       drm_mode_config_cleanup(drm);
> 
> Ditto.
> 
> >         exynos_drm_cleanup_dma(drm);
> >
> >         kfree(drm->dev_private);
> > --
> > 2.34.1
> >
> >

Hi, Inki
Thanks for the review and for pointing out the to_dma_dev() path

If I understand you correctly, fine with using DRMM, but kfree(priv) should occur after drm_dev_put(drm)
That would mean releasing the drm_device first and freeing dev_private afterwards.
Of course, we will also need to adjust the probe() error-unwind (err_free) order accordingly.
Do you anticipate any side effects from this ordering change? I’d appreciate your thoughts.

BRs,
Hoyoung Lee

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

[Inki Dae]

Hi Hoyoung,

2025년 11월 12일 (수) 오전 11:44, <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.
>

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
>
>
>

Re: [PATCH 2/3] drm/exynos: Convert to drmm_mode_config_init() and drop manual cleanup

[Inki Dae]

2025년 11월 12일 (수) 오후 12:03, <hy_fifty.lee@samsung.com>님이 작성:
>
> > -----Original Message-----
> > From: Inki Dae <daeinki@gmail.com>
> > Sent: Monday, November 10, 2025 2:22 PM
> > 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 2/3] drm/exynos: Convert to drmm_mode_config_init()
> > and drop manual cleanup
> >
> > 2025년 9월 29일 (월) 오후 1:54, Hoyoung Lee <hy_fifty.lee@samsung.com>님이 작
> > 성:
> > >
> > > Switch mode-config initialization to drmm_mode_config_init() so that
> > > the lifetime is tied to drm_device. Remove explicit
> > > drm_mode_config_cleanup() from error and unbind paths since cleanup is
> > now managed by DRM.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Hoyoung Lee <hy_fifty.lee@samsung.com>
> > > ---
> > >  drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > > b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > > index 6cc7bf77bcac..1aea71778ab1 100644
> > > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > > @@ -257,7 +257,7 @@ static int exynos_drm_bind(struct device *dev)
> > >         dev_set_drvdata(dev, drm);
> > >         drm->dev_private = (void *)private;
> > >
> > > -       drm_mode_config_init(drm);
> > > +       drmm_mode_config_init(drm);
> > >
> > >         exynos_drm_mode_config_init(drm);
> > >
> > > @@ -297,7 +297,6 @@ static int exynos_drm_bind(struct device *dev)
> > >  err_unbind_all:
> > >         component_unbind_all(drm->dev, drm);
> > >  err_mode_config_cleanup:
> > > -       drm_mode_config_cleanup(drm);
> >
> > In the current implementation, there is a potential dereference issue
> > because the private object may be freed before to_dma_dev(dev) is called.
> > When drmm_mode_config_init() is invoked, it registers
> > drm_mode_config_cleanup() as a managed action. This means that the cleanup
> > function will be automatically executed later when
> > drm_dev_put() is called.
> >
> > The problem arises when drm_dev_put() is called without explicitly
> > invoking drm_mode_config_cleanup() first, as in the original code. In that
> > case, the managed cleanup is performed later, which allows
> > to_dma_dev(dev) to be called after the private object has already been
> > released.
> >
> > For reference, the following sequence may occur internally when
> > drm_mode_config_cleanup() is executed:
> > 1. drm_mode_config_cleanup() is called.
> > 2. During the cleanup of FBs, planes, CRTCs, encoders, and connectors,
> > framebuffers or GEM objects may be released.
> > 3. At this point, Exynos-specific code could invoke to_dma_dev(dev).
> >
> > Therefore, the private object must remain valid until
> > drm_mode_config_cleanup() completes.
> > It would be safer to adjust the code so that kfree(private) is performed
> > after drm_dev_put(drm) to ensure the private data remains available during
> > cleanup.
> >
> > Thanks,
> > Inki Dae
> >
> > >         exynos_drm_cleanup_dma(drm);
> > >         kfree(private);
> > >         dev_set_drvdata(dev, NULL);
> > > @@ -317,7 +316,6 @@ static void exynos_drm_unbind(struct device *dev)
> > >         drm_atomic_helper_shutdown(drm);
> > >
> > >         component_unbind_all(drm->dev, drm);
> > > -       drm_mode_config_cleanup(drm);
> >
> > Ditto.
> >
> > >         exynos_drm_cleanup_dma(drm);
> > >
> > >         kfree(drm->dev_private);
> > > --
> > > 2.34.1
> > >
> > >
>
> Hi, Inki
> Thanks for the review and for pointing out the to_dma_dev() path
>
> If I understand you correctly, fine with using DRMM, but kfree(priv) should occur after drm_dev_put(drm)
> That would mean releasing the drm_device first and freeing dev_private afterwards.
> Of course, we will also need to adjust the probe() error-unwind (err_free) order accordingly.
> Do you anticipate any side effects from this ordering change? I’d appreciate your thoughts.
>

Yes, that is correct. Also, changing the order of the cleanup steps
should not introduce any issues, because this simply restores the
original sequence that the code previously followed. In fact, the
current patch is, strictly speaking, altering the existing behavior I
think.

Thanks,
Inki Dae

> BRs,
> Hoyoung Lee
>
>