* Re: [PATCH] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers
2026-09-09 9:24 [PATCH] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers Davide Bonatto
@ 2026-09-09 9:38 ` sashiko-bot
2026-09-09 10:43 ` [PATCH v3] " Davide Bonatto
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-09-09 9:38 UTC (permalink / raw)
To: Davide Bonatto; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [Critical] Hardware DMA scans out freed memory when plane is disabled without CRTC disable (Use-After-Free).
- [High] NULL Pointer Dereference during CRTC enable without a primary plane.
- [High] Missing `.prepare_fb` callback breaks implicit synchronization.
- [Medium] Missing plane `.atomic_check` hook bypasses hardware scaling constraints.
Pre-existing issues:
- [High] Sleeping functions called from atomic context.
--
commit 776de1d240335d0996c4eca5f0b2980de524e7c7
Author: Davide Bonatto <bonatto.davide@gmail.com>
drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers
The drm_simple_display_pipe helper is considered legacy/deprecated in
favor of explicitly managing individual plane, CRTC, and encoder objects
using standard atomic helpers.
> diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
> index c93d61ac0bb75..bd24049845350 100644
> --- a/drivers/gpu/drm/tiny/arcpgu.c
> +++ b/drivers/gpu/drm/tiny/arcpgu.c
[ ... ]
> @@ -117,8 +123,8 @@ static const u32 arc_pgu_supported_formats[] = {
>
> static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu)
> {
> - const struct drm_framebuffer *fb = arcpgu->pipe.plane.state->fb;
> - uint32_t pixel_format = fb->format->format;
> + const struct drm_framebuffer *fb = arcpgu->plane.state->fb;
> + u32 pixel_format = fb->format->format;
[Severity: High]
Can this result in a NULL pointer dereference if the CRTC is enabled
without a primary plane?
Because the CRTC .atomic_check hook has been omitted in this patch, the
driver loses the drm_atomic_helper_check_crtc_primary_plane() validation.
Userspace can now submit an atomic commit that enables the CRTC without a
bound primary plane framebuffer. During the commit,
arc_pgu_crtc_atomic_enable() calls arc_pgu_mode_set(), which then calls
arc_pgu_set_pxl_fmt(). If fb is NULL here, dereferencing fb->format->format
will cause a kernel panic.
[ ... ]
> -static void arc_pgu_enable(struct drm_simple_display_pipe *pipe,
> - struct drm_crtc_state *crtc_state,
> - struct drm_plane_state *plane_state)
> +static void arc_pgu_crtc_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> {
> - struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
> + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
>
> arc_pgu_mode_set(arcpgu);
>
> clk_prepare_enable(arcpgu->clk);
[Severity: High]
This isn't a bug introduced by this patch, but does calling
clk_prepare_enable() here violate atomic context rules?
The DRM subsystem guidelines specify that CRTC atomic enable callbacks execute
in atomic context during non-blocking commits, where sleeping is strictly
forbidden. Since clk_prepare_enable() can sleep, it could lead to deadlocks
or system instability.
[ ... ]
> -static void arc_pgu_disable(struct drm_simple_display_pipe *pipe)
> +static void arc_pgu_crtc_atomic_disable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> {
> - struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
> + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
>
> clk_disable_unprepare(arcpgu->clk);
[Severity: High]
This is also a pre-existing issue, but similar to the enable path, is it
safe to call clk_disable_unprepare() here? This function can sleep, which
is unsafe to call during the atomic_disable callback.
[ ... ]
> -static void arc_pgu_update(struct drm_simple_display_pipe *pipe,
> - struct drm_plane_state *state)
> +static void arc_pgu_plane_atomic_update(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> {
> - struct arcpgu_drm_private *arcpgu;
> + struct arcpgu_drm_private *arcpgu = plane_to_arcpgu(plane);
> + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> struct drm_gem_dma_object *gem;
>
> - if (!pipe->plane.state->fb)
> + if (!new_plane_state->fb)
> return;
[Severity: Critical]
Could this early return cause the hardware DMA to scan out freed memory?
Due to the missing CRTC .atomic_check hook, userspace can submit a commit
that disables the primary plane while keeping the CRTC active. If
new_plane_state->fb is NULL, this function returns early without updating
the hardware. Since the CRTC remains enabled, the hardware DMA will
continue scanning from the previously configured buffer address after the
DRM core frees it, leading to memory corruption or an information leak.
[ ... ]
> +static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
> + .mode_valid = arc_pgu_crtc_mode_valid,
> + .atomic_enable = arc_pgu_crtc_atomic_enable,
> + .atomic_disable = arc_pgu_crtc_atomic_disable,
> +};
[Severity: High]
Is the .atomic_check hook intentionally omitted here?
Without it, the driver loses the validation that the simple KMS helper
previously provided to enforce a bound primary plane. This allows an invalid
atomic state to proceed to the commit phase, leading to the NULL pointer
dereference or use-after-free scenarios mentioned above.
[ ... ]
> +static const struct drm_plane_helper_funcs arc_pgu_plane_helper_funcs = {
> + .atomic_update = arc_pgu_plane_atomic_update,
> +};
[Severity: Medium]
Does this structure need an .atomic_check function to validate hardware
scaling constraints?
The simple KMS helper previously enforced DRM_PLANE_NO_SCALING via
drm_atomic_helper_check_plane_state(). Without this validation, the driver
might incorrectly accept atomic commits with unsupported scaling or
positioning constraints from userspace.
[Severity: High]
Will the omission of the .prepare_fb callback break implicit
synchronization?
In the simple KMS helper, this was handled automatically via
drm_gem_plane_helper_prepare_fb(). Without this callback, the DRM core
cannot extract the implicit dma_fence from the GEM object's DMA-buf
reservation, meaning the atomic commit will immediately scan out the
buffer without waiting for GPU rendering to finish.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260909092455.131962-1-bonatto.davide@gmail.com?part=1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v3] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers
2026-09-09 9:24 [PATCH] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers Davide Bonatto
2026-09-09 9:38 ` sashiko-bot
@ 2026-09-09 10:43 ` Davide Bonatto
2026-09-09 11:03 ` sashiko-bot
2026-09-09 12:41 ` [PATCH v4] " Davide Bonatto
2026-09-09 12:58 ` [PATCH v5] " Davide Bonatto
3 siblings, 1 reply; 9+ messages in thread
From: Davide Bonatto @ 2026-09-09 10:43 UTC (permalink / raw)
To: Alexey Brodkin, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter,
open list:DRM DRIVERS, open list
Cc: Davide Bonatto
The drm_simple_display_pipe helper is considered legacy/deprecated in
favor of explicitly managing individual plane, CRTC, and encoder objects
using standard atomic helpers.
Migrate arcpgu to initialize its primary plane, CRTC, and encoder
explicitly, and route mode validation, enable, and disable hooks
through drm_crtc_helper_funcs, and plane updates through
drm_plane_helper_funcs.
Signed-off-by: Davide Bonatto <bonatto.davide@gmail.com>
---
v3:
- Add CRTC .atomic_check hook to validate primary plane presence
and avoid NULL pointer dereference in arc_pgu_set_pxl_fmt().
- Pass framebuffer explicitly to arc_pgu_mode_set() and read plane
state via drm_atomic_get_new_plane_state() in CRTC enable to prevent UAF.
- Keep CRTC global enable bit intact during plane disable to avoid
hardware state desync.
v2:
- Add .prepare_fb callback to support implicit synchronization.
- Add .atomic_check using drm_atomic_helper_check_plane_state()
to validate hardware scaling/clipping constraints.
- Implement arc_pgu_plane_atomic_disable() to stop DMA and clear
buffer address register, preventing hardware DMA scanout UAF.
drivers/gpu/drm/tiny/arcpgu.c | 191 +++++++++++++++++++++++++++-------
1 file changed, 153 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
index c93d61ac0bb7..b66b9562df10 100644
--- a/drivers/gpu/drm/tiny/arcpgu.c
+++ b/drivers/gpu/drm/tiny/arcpgu.c
@@ -5,6 +5,8 @@
* Copyright (C) 2016 Synopsys, Inc. (www.synopsys.com)
*/
+#include "drm/drm_gem_atomic_helper.h"
+
#include <linux/clk.h>
#include <drm/clients/drm_client_setup.h>
@@ -23,6 +25,8 @@
#include <drm/drm_of.h>
#include <drm/drm_probe_helper.h>
#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_bridge.h>
#include <linux/dma-mapping.h>
#include <linux/module.h>
#include <linux/of_reserved_mem.h>
@@ -52,13 +56,17 @@ struct arcpgu_drm_private {
struct drm_device drm;
void __iomem *regs;
struct clk *clk;
- struct drm_simple_display_pipe pipe;
+ struct drm_plane plane;
+ struct drm_crtc crtc;
+ struct drm_encoder encoder;
struct drm_connector sim_conn;
};
#define dev_to_arcpgu(x) container_of(x, struct arcpgu_drm_private, drm)
-#define pipe_to_arcpgu_priv(x) container_of(x, struct arcpgu_drm_private, pipe)
+#define crtc_to_arcpgu_priv(x) container_of(x, struct arcpgu_drm_private, crtc)
+
+#define plane_to_arcpgu(x) container_of(x, struct arcpgu_drm_private, plane)
static inline void arc_pgu_write(struct arcpgu_drm_private *arcpgu,
unsigned int reg, u32 value)
@@ -115,14 +123,19 @@ static const u32 arc_pgu_supported_formats[] = {
DRM_FORMAT_ARGB8888,
};
-static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu)
+static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu,
+ const struct drm_framebuffer *fb)
{
- const struct drm_framebuffer *fb = arcpgu->pipe.plane.state->fb;
- uint32_t pixel_format = fb->format->format;
+ u32 pixel_format;
u32 format = DRM_FORMAT_INVALID;
int i;
u32 reg_ctrl;
+ if (!fb)
+ return;
+
+ pixel_format = fb->format->format;
+
for (i = 0; i < ARRAY_SIZE(arc_pgu_supported_formats); i++) {
if (arc_pgu_supported_formats[i] == pixel_format)
format = arc_pgu_supported_formats[i];
@@ -139,10 +152,10 @@ static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu)
arc_pgu_write(arcpgu, ARCPGU_REG_CTRL, reg_ctrl);
}
-static enum drm_mode_status arc_pgu_mode_valid(struct drm_simple_display_pipe *pipe,
- const struct drm_display_mode *mode)
+static enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
+ const struct drm_display_mode *mode)
{
- struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
+ struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
long rate, clk_rate = mode->clock * 1000;
long diff = clk_rate / 200; /* +-0.5% allowed by HDMI spec */
@@ -153,9 +166,10 @@ static enum drm_mode_status arc_pgu_mode_valid(struct drm_simple_display_pipe *p
return MODE_NOCLOCK;
}
-static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu)
+static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu,
+ const struct drm_framebuffer *fb)
{
- struct drm_display_mode *m = &arcpgu->pipe.crtc.state->adjusted_mode;
+ struct drm_display_mode *m = &arcpgu->crtc.state->adjusted_mode;
u32 val;
arc_pgu_write(arcpgu, ARCPGU_REG_FMT,
@@ -189,54 +203,130 @@ static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu)
arc_pgu_write(arcpgu, ARCPGU_REG_STRIDE, 0);
arc_pgu_write(arcpgu, ARCPGU_REG_START_SET, 1);
- arc_pgu_set_pxl_fmt(arcpgu);
+ arc_pgu_set_pxl_fmt(arcpgu, fb);
clk_set_rate(arcpgu->clk, m->crtc_clock * 1000);
}
-static void arc_pgu_enable(struct drm_simple_display_pipe *pipe,
- struct drm_crtc_state *crtc_state,
- struct drm_plane_state *plane_state)
+static void arc_pgu_crtc_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
{
- struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
+ struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
+ struct drm_plane_state *plane_state =
+ drm_atomic_get_new_plane_state(state, &arcpgu->plane);
+ const struct drm_framebuffer *fb = plane_state ? plane_state->fb : NULL;
- arc_pgu_mode_set(arcpgu);
+ arc_pgu_mode_set(arcpgu, fb);
clk_prepare_enable(arcpgu->clk);
arc_pgu_write(arcpgu, ARCPGU_REG_CTRL,
arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) |
- ARCPGU_CTRL_ENABLE_MASK);
+ ARCPGU_CTRL_ENABLE_MASK);
}
-static void arc_pgu_disable(struct drm_simple_display_pipe *pipe)
+static void arc_pgu_crtc_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
{
- struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
+ struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
clk_disable_unprepare(arcpgu->clk);
arc_pgu_write(arcpgu, ARCPGU_REG_CTRL,
- arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) &
- ~ARCPGU_CTRL_ENABLE_MASK);
+ arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) &
+ ~ARCPGU_CTRL_ENABLE_MASK);
}
-static void arc_pgu_update(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *state)
+static void arc_pgu_plane_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_commit *state)
{
- struct arcpgu_drm_private *arcpgu;
+ struct arcpgu_drm_private *arcpgu = plane_to_arcpgu(plane);
+ struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
struct drm_gem_dma_object *gem;
- if (!pipe->plane.state->fb)
+ if (!new_plane_state->fb)
return;
- arcpgu = pipe_to_arcpgu_priv(pipe);
- gem = drm_fb_dma_get_gem_obj(pipe->plane.state->fb, 0);
+ gem = drm_fb_dma_get_gem_obj(new_plane_state->fb, 0);
arc_pgu_write(arcpgu, ARCPGU_REG_BUF0_ADDR, gem->dma_addr);
}
-static const struct drm_simple_display_pipe_funcs arc_pgu_pipe_funcs = {
- .update = arc_pgu_update,
- .mode_valid = arc_pgu_mode_valid,
- .enable = arc_pgu_enable,
- .disable = arc_pgu_disable,
+static void arc_pgu_plane_atomic_disable(struct drm_plane *plane,
+ struct drm_atomic_commit *state)
+{
+ struct arcpgu_drm_private *arcpgu = plane_to_arcpgu(plane);
+
+ arc_pgu_write(arcpgu, ARCPGU_REG_BUF0_ADDR, 0);
+}
+
+static int arc_pgu_plane_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_commit *state)
+{
+ struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+ struct drm_crtc_state *crtc_state;
+
+ if (!new_plane_state->crtc)
+ return 0;
+
+ crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
+ if (!crtc_state)
+ return -EINVAL;
+
+ return drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, true);
+}
+
+static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
+{
+ struct drm_crtc_state *crct_state = drm_atomic_get_new_crtc_state(state, crtc);
+ struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
+ struct drm_plane_state *plane_state;
+
+ if (!crct_state->enable)
+ return 0;
+
+ plane_state = drm_atomic_get_new_plane_state(state, &arcpgu->plane);
+ if (!plane_state || !plane_state->fb)
+ return -EINVAL;
+
+ return 0;
+}
+
+static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
+ .mode_valid = arc_pgu_crtc_mode_valid,
+ .atomic_enable = arc_pgu_crtc_atomic_enable,
+ .atomic_disable = arc_pgu_crtc_atomic_disable,
+ .atomic_check = arc_pgu_crtc_atomic_check,
+};
+
+static const struct drm_crtc_funcs arc_pgu_crtc_funcs = {
+ .reset = drm_atomic_helper_crtc_reset,
+ .destroy = drm_crtc_cleanup,
+ .set_config = drm_atomic_helper_set_config,
+ .page_flip = drm_atomic_helper_page_flip,
+ .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static const struct drm_plane_helper_funcs arc_pgu_plane_helper_funcs = {
+ .atomic_update = arc_pgu_plane_atomic_update,
+ .prepare_fb = drm_gem_plane_helper_prepare_fb,
+ .atomic_check = arc_pgu_plane_atomic_check,
+ .atomic_disable = arc_pgu_plane_atomic_disable,
+};
+
+static const struct drm_plane_funcs arc_pgu_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .destroy = drm_plane_cleanup,
+ .reset = drm_atomic_helper_plane_reset,
+ .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
+static const struct drm_encoder_funcs arc_pgu_encoder_funcs = {
+ .destroy = drm_encoder_cleanup
};
static const struct drm_mode_config_funcs arcpgu_drm_modecfg_funcs = {
@@ -301,13 +391,38 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
return ret;
}
- ret = drm_simple_display_pipe_init(drm, &arcpgu->pipe, &arc_pgu_pipe_funcs,
- arc_pgu_supported_formats,
- ARRAY_SIZE(arc_pgu_supported_formats),
- NULL, connector);
+ ret = drm_universal_plane_init(drm, &arcpgu->plane, 0,
+ &arc_pgu_plane_funcs, arc_pgu_supported_formats,
+ ARRAY_SIZE(arc_pgu_supported_formats), NULL,
+ DRM_PLANE_TYPE_PRIMARY, NULL);
+
if (ret)
return ret;
+ drm_plane_helper_add(&arcpgu->plane, &arc_pgu_plane_helper_funcs);
+
+ ret = drm_crtc_init_with_planes(drm, &arcpgu->crtc, &arcpgu->plane, NULL,
+ &arc_pgu_crtc_funcs, NULL);
+
+ if (ret)
+ return ret;
+
+ drm_crtc_helper_add(&arcpgu->crtc, &arc_pgu_crtc_helper_funcs);
+
+ ret = drm_encoder_init(drm, &arcpgu->encoder, &arc_pgu_encoder_funcs,
+ DRM_MODE_ENCODER_NONE, NULL);
+
+ if (ret)
+ return ret;
+
+ arcpgu->encoder.possible_crtcs = drm_crtc_mask(&arcpgu->crtc);
+
+ if (connector) {
+ ret = drm_connector_attach_encoder(connector, &arcpgu->encoder);
+ if (ret)
+ return ret;
+ }
+
if (encoder_node) {
/* Locate drm bridge from the hdmi encoder DT node */
struct drm_bridge *bridge __free(drm_bridge_put) =
@@ -315,7 +430,7 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
if (!bridge)
return -EPROBE_DEFER;
- ret = drm_simple_display_pipe_attach_bridge(&arcpgu->pipe, bridge);
+ ret = drm_bridge_attach(&arcpgu->encoder, bridge, NULL, 0);
if (ret)
return ret;
}
@@ -342,7 +457,7 @@ static int arcpgu_show_pxlclock(struct seq_file *m, void *arg)
struct drm_device *drm = node->minor->dev;
struct arcpgu_drm_private *arcpgu = dev_to_arcpgu(drm);
unsigned long clkrate = clk_get_rate(arcpgu->clk);
- unsigned long mode_clock = arcpgu->pipe.crtc.mode.crtc_clock * 1000;
+ unsigned long mode_clock = arcpgu->crtc.mode.crtc_clock * 1000;
seq_printf(m, "hw : %lu\n", clkrate);
seq_printf(m, "mode: %lu\n", mode_clock);
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers
2026-09-09 10:43 ` [PATCH v3] " Davide Bonatto
@ 2026-09-09 11:03 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-09-09 11:03 UTC (permalink / raw)
To: Davide Bonatto; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Incorrect usage of `drm_atomic_get_new_*_state` causes valid partial atomic commits to be rejected with `-EINVAL` and leads to potential hardware misconfiguration.
Pre-existing issues:
- [High] Sleeping clock functions are called from CRTC atomic enable/disable paths, violating atomic context constraints.
- [High] Hardware DMA use-after-free on page flip due to lack of hardware VBLANK synchronization.
--
commit 56c718e0d1deb02575937fad72869dc5fbfabbaa
Author: Davide Bonatto <bonatto.davide@gmail.com>
drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers
The driver is being migrated from the deprecated drm_simple_display_pipe
helper to explicitly managed plane, CRTC, and encoder objects using standard
atomic helpers.
> diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
> index c93d61ac0bb75..b66b9562df10d 100644
> --- a/drivers/gpu/drm/tiny/arcpgu.c
> +++ b/drivers/gpu/drm/tiny/arcpgu.c
[ ... ]
> -static void arc_pgu_enable(struct drm_simple_display_pipe *pipe,
> - struct drm_crtc_state *crtc_state,
> - struct drm_plane_state *plane_state)
> +static void arc_pgu_crtc_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> {
> - struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
> + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> + struct drm_plane_state *plane_state =
> + drm_atomic_get_new_plane_state(state, &arcpgu->plane);
> + const struct drm_framebuffer *fb = plane_state ? plane_state->fb : NULL;
[Severity: High]
Does this skip essential pixel format programming during a CRTC enable if the
plane isn't part of the commit state?
If a userspace compositor requests a CRTC-only DPMS state change,
drm_atomic_get_new_plane_state() will return NULL for the plane state, leading
to a NULL fb and skipping the pixel format programming entirely.
> - arc_pgu_mode_set(arcpgu);
> + arc_pgu_mode_set(arcpgu, fb);
>
> clk_prepare_enable(arcpgu->clk);
[Severity: High]
This is a pre-existing issue, but does this execute a sleeping clock framework
function inside an atomic context?
According to DRM subsystem guidelines, CRTC atomic enable callbacks execute in
an atomic context where sleeping functions are forbidden. Calling
clk_prepare_enable() here can cause kernel warnings and potential deadlocks.
> arc_pgu_write(arcpgu, ARCPGU_REG_CTRL,
> arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) |
> - ARCPGU_CTRL_ENABLE_MASK);
> + ARCPGU_CTRL_ENABLE_MASK);
> }
>
> -static void arc_pgu_disable(struct drm_simple_display_pipe *pipe)
> +static void arc_pgu_crtc_atomic_disable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> {
> - struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
> + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
>
> clk_disable_unprepare(arcpgu->clk);
[Severity: High]
This is a pre-existing issue, but does this execute a sleeping clock framework
function inside an atomic context?
Similar to the enable path, arc_pgu_crtc_atomic_disable() executes in an
atomic context. Calling clk_disable_unprepare() here acquires a mutex and
sleeps, which is not permitted.
[ ... ]
> -static void arc_pgu_update(struct drm_simple_display_pipe *pipe,
> - struct drm_plane_state *state)
> +static void arc_pgu_plane_atomic_update(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> {
> - struct arcpgu_drm_private *arcpgu;
> + struct arcpgu_drm_private *arcpgu = plane_to_arcpgu(plane);
> + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> struct drm_gem_dma_object *gem;
>
> - if (!pipe->plane.state->fb)
> + if (!new_plane_state->fb)
> return;
>
> - arcpgu = pipe_to_arcpgu_priv(pipe);
> - gem = drm_fb_dma_get_gem_obj(pipe->plane.state->fb, 0);
> + gem = drm_fb_dma_get_gem_obj(new_plane_state->fb, 0);
> arc_pgu_write(arcpgu, ARCPGU_REG_BUF0_ADDR, gem->dma_addr);
[Severity: High]
This is a pre-existing issue inherited from the simple display pipe setup,
but can this lead to a hardware DMA use-after-free on page flip?
Because the driver lacks VBLANK interrupts, the new DMA address is committed
to hardware here without a mechanism to delay freeing the previous buffer. The
DRM core simulates a VBLANK and immediately unpins/frees the old memory, but
the physical hardware won't latch this new address until its next real VBLANK.
The hardware could continue scanning from the now-freed memory buffer.
[ ... ]
> +static int arc_pgu_plane_atomic_check(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> +{
> + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> + struct drm_crtc_state *crtc_state;
> +
> + if (!new_plane_state->crtc)
> + return 0;
> +
> + crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
> + if (!crtc_state)
> + return -EINVAL;
[Severity: High]
Does this incorrectly reject valid plane-only updates?
If a userspace compositor requests a non-modesetting atomic update (such as
only updating the plane's FB), the CRTC state is not natively included in the
atomic commit state. drm_atomic_get_new_crtc_state() will return NULL, causing
this function to fail with -EINVAL and breaking userspace display servers.
[ ... ]
> +static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> +{
> + struct drm_crtc_state *crct_state = drm_atomic_get_new_crtc_state(state, crtc);
> + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> + struct drm_plane_state *plane_state;
> +
> + if (!crct_state->enable)
> + return 0;
> +
> + plane_state = drm_atomic_get_new_plane_state(state, &arcpgu->plane);
> + if (!plane_state || !plane_state->fb)
> + return -EINVAL;
[Severity: High]
Will this fail valid CRTC-only updates?
Similar to the plane check, if a userspace compositor requests an update to
a CRTC property (e.g., active state) without including the plane in the
commit request, plane_state will evaluate to NULL and cause the driver to
incorrectly reject the partial atomic commit.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260909104332.160612-1-bonatto.davide@gmail.com?part=1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers
2026-09-09 9:24 [PATCH] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers Davide Bonatto
2026-09-09 9:38 ` sashiko-bot
2026-09-09 10:43 ` [PATCH v3] " Davide Bonatto
@ 2026-09-09 12:41 ` Davide Bonatto
2026-09-09 12:50 ` sashiko-bot
2026-09-09 12:58 ` [PATCH v5] " Davide Bonatto
3 siblings, 1 reply; 9+ messages in thread
From: Davide Bonatto @ 2026-09-09 12:41 UTC (permalink / raw)
To: Alexey Brodkin, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter,
open list:DRM DRIVERS, open list
Cc: Davide Bonatto
The drm_simple_display_pipe helper is considered legacy/deprecated in
favor of explicitly managing individual plane, CRTC, and encoder objects
using standard atomic helpers.
Migrate arcpgu to initialize its primary plane, CRTC, and encoder
explicitly, and route mode validation, enable, and disable hooks
through drm_crtc_helper_funcs, and plane updates through
drm_plane_helper_funcs.
Signed-off-by: Davide Bonatto <bonatto.davide@gmail.com>
---
v4:
- Use drm_atomic_get_plane_state() in CRTC .atomic_check to ensure
the primary plane state is attached and locked, avoiding false -EINVAL
rejections and broken partial atomic commits.
v3:
- Add CRTC .atomic_check hook to validate primary plane presence
and avoid NULL pointer dereference in arc_pgu_set_pxl_fmt().
- Pass framebuffer explicitly to arc_pgu_mode_set() and read plane
state via drm_atomic_get_new_plane_state() in CRTC enable to prevent UAF.
- Keep CRTC global enable bit intact during plane disable to avoid
hardware state desync.
v2:
- Add .prepare_fb callback to support implicit synchronization.
- Add .atomic_check using drm_atomic_helper_check_plane_state()
to validate hardware scaling/clipping constraints.
- Implement arc_pgu_plane_atomic_disable() to stop DMA and clear
buffer address register, preventing hardware DMA scanout UAF.
drivers/gpu/drm/tiny/arcpgu.c | 194 +++++++++++++++++++++++++++-------
1 file changed, 156 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
index c93d61ac0bb7..adda40692733 100644
--- a/drivers/gpu/drm/tiny/arcpgu.c
+++ b/drivers/gpu/drm/tiny/arcpgu.c
@@ -5,6 +5,8 @@
* Copyright (C) 2016 Synopsys, Inc. (www.synopsys.com)
*/
+#include "drm/drm_gem_atomic_helper.h"
+
#include <linux/clk.h>
#include <drm/clients/drm_client_setup.h>
@@ -23,6 +25,8 @@
#include <drm/drm_of.h>
#include <drm/drm_probe_helper.h>
#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_bridge.h>
#include <linux/dma-mapping.h>
#include <linux/module.h>
#include <linux/of_reserved_mem.h>
@@ -52,13 +56,17 @@ struct arcpgu_drm_private {
struct drm_device drm;
void __iomem *regs;
struct clk *clk;
- struct drm_simple_display_pipe pipe;
+ struct drm_plane plane;
+ struct drm_crtc crtc;
+ struct drm_encoder encoder;
struct drm_connector sim_conn;
};
#define dev_to_arcpgu(x) container_of(x, struct arcpgu_drm_private, drm)
-#define pipe_to_arcpgu_priv(x) container_of(x, struct arcpgu_drm_private, pipe)
+#define crtc_to_arcpgu_priv(x) container_of(x, struct arcpgu_drm_private, crtc)
+
+#define plane_to_arcpgu(x) container_of(x, struct arcpgu_drm_private, plane)
static inline void arc_pgu_write(struct arcpgu_drm_private *arcpgu,
unsigned int reg, u32 value)
@@ -115,14 +123,19 @@ static const u32 arc_pgu_supported_formats[] = {
DRM_FORMAT_ARGB8888,
};
-static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu)
+static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu,
+ const struct drm_framebuffer *fb)
{
- const struct drm_framebuffer *fb = arcpgu->pipe.plane.state->fb;
- uint32_t pixel_format = fb->format->format;
+ u32 pixel_format;
u32 format = DRM_FORMAT_INVALID;
int i;
u32 reg_ctrl;
+ if (!fb)
+ return;
+
+ pixel_format = fb->format->format;
+
for (i = 0; i < ARRAY_SIZE(arc_pgu_supported_formats); i++) {
if (arc_pgu_supported_formats[i] == pixel_format)
format = arc_pgu_supported_formats[i];
@@ -139,10 +152,10 @@ static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu)
arc_pgu_write(arcpgu, ARCPGU_REG_CTRL, reg_ctrl);
}
-static enum drm_mode_status arc_pgu_mode_valid(struct drm_simple_display_pipe *pipe,
- const struct drm_display_mode *mode)
+static enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
+ const struct drm_display_mode *mode)
{
- struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
+ struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
long rate, clk_rate = mode->clock * 1000;
long diff = clk_rate / 200; /* +-0.5% allowed by HDMI spec */
@@ -153,9 +166,10 @@ static enum drm_mode_status arc_pgu_mode_valid(struct drm_simple_display_pipe *p
return MODE_NOCLOCK;
}
-static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu)
+static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu,
+ const struct drm_framebuffer *fb)
{
- struct drm_display_mode *m = &arcpgu->pipe.crtc.state->adjusted_mode;
+ struct drm_display_mode *m = &arcpgu->crtc.state->adjusted_mode;
u32 val;
arc_pgu_write(arcpgu, ARCPGU_REG_FMT,
@@ -189,54 +203,133 @@ static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu)
arc_pgu_write(arcpgu, ARCPGU_REG_STRIDE, 0);
arc_pgu_write(arcpgu, ARCPGU_REG_START_SET, 1);
- arc_pgu_set_pxl_fmt(arcpgu);
+ arc_pgu_set_pxl_fmt(arcpgu, fb);
clk_set_rate(arcpgu->clk, m->crtc_clock * 1000);
}
-static void arc_pgu_enable(struct drm_simple_display_pipe *pipe,
- struct drm_crtc_state *crtc_state,
- struct drm_plane_state *plane_state)
+static void arc_pgu_crtc_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
{
- struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
+ struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
+ struct drm_plane_state *plane_state =
+ drm_atomic_get_new_plane_state(state, &arcpgu->plane);
+ const struct drm_framebuffer *fb = plane_state ? plane_state->fb : NULL;
- arc_pgu_mode_set(arcpgu);
+ arc_pgu_mode_set(arcpgu, fb);
clk_prepare_enable(arcpgu->clk);
arc_pgu_write(arcpgu, ARCPGU_REG_CTRL,
arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) |
- ARCPGU_CTRL_ENABLE_MASK);
+ ARCPGU_CTRL_ENABLE_MASK);
}
-static void arc_pgu_disable(struct drm_simple_display_pipe *pipe)
+static void arc_pgu_crtc_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
{
- struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
+ struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
clk_disable_unprepare(arcpgu->clk);
arc_pgu_write(arcpgu, ARCPGU_REG_CTRL,
- arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) &
- ~ARCPGU_CTRL_ENABLE_MASK);
+ arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) &
+ ~ARCPGU_CTRL_ENABLE_MASK);
}
-static void arc_pgu_update(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *state)
+static void arc_pgu_plane_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_commit *state)
{
- struct arcpgu_drm_private *arcpgu;
+ struct arcpgu_drm_private *arcpgu = plane_to_arcpgu(plane);
+ struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
struct drm_gem_dma_object *gem;
- if (!pipe->plane.state->fb)
+ if (!new_plane_state->fb)
return;
- arcpgu = pipe_to_arcpgu_priv(pipe);
- gem = drm_fb_dma_get_gem_obj(pipe->plane.state->fb, 0);
+ gem = drm_fb_dma_get_gem_obj(new_plane_state->fb, 0);
arc_pgu_write(arcpgu, ARCPGU_REG_BUF0_ADDR, gem->dma_addr);
}
-static const struct drm_simple_display_pipe_funcs arc_pgu_pipe_funcs = {
- .update = arc_pgu_update,
- .mode_valid = arc_pgu_mode_valid,
- .enable = arc_pgu_enable,
- .disable = arc_pgu_disable,
+static void arc_pgu_plane_atomic_disable(struct drm_plane *plane,
+ struct drm_atomic_commit *state)
+{
+ struct arcpgu_drm_private *arcpgu = plane_to_arcpgu(plane);
+
+ arc_pgu_write(arcpgu, ARCPGU_REG_BUF0_ADDR, 0);
+}
+
+static int arc_pgu_plane_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_commit *state)
+{
+ struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+ struct drm_crtc_state *crtc_state;
+
+ if (!new_plane_state->crtc)
+ return 0;
+
+ crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
+ if (!crtc_state)
+ return -EINVAL;
+
+ return drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, true);
+}
+
+static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
+{
+ struct drm_crtc_state *crct_state = drm_atomic_get_new_crtc_state(state, crtc);
+ struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
+ struct drm_plane_state *plane_state;
+
+ if (!crct_state->enable)
+ return 0;
+
+ plane_state = drm_atomic_get_new_plane_state(state, &arcpgu->plane);
+ if (IS_ERR(plane_state))
+ return PTR_ERR(plane_state);
+
+ if (!plane_state->fb)
+ return -EINVAL;
+
+ return 0;
+}
+
+static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
+ .mode_valid = arc_pgu_crtc_mode_valid,
+ .atomic_enable = arc_pgu_crtc_atomic_enable,
+ .atomic_disable = arc_pgu_crtc_atomic_disable,
+ .atomic_check = arc_pgu_crtc_atomic_check,
+};
+
+static const struct drm_crtc_funcs arc_pgu_crtc_funcs = {
+ .reset = drm_atomic_helper_crtc_reset,
+ .destroy = drm_crtc_cleanup,
+ .set_config = drm_atomic_helper_set_config,
+ .page_flip = drm_atomic_helper_page_flip,
+ .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static const struct drm_plane_helper_funcs arc_pgu_plane_helper_funcs = {
+ .atomic_update = arc_pgu_plane_atomic_update,
+ .prepare_fb = drm_gem_plane_helper_prepare_fb,
+ .atomic_check = arc_pgu_plane_atomic_check,
+ .atomic_disable = arc_pgu_plane_atomic_disable,
+};
+
+static const struct drm_plane_funcs arc_pgu_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .destroy = drm_plane_cleanup,
+ .reset = drm_atomic_helper_plane_reset,
+ .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
+static const struct drm_encoder_funcs arc_pgu_encoder_funcs = {
+ .destroy = drm_encoder_cleanup
};
static const struct drm_mode_config_funcs arcpgu_drm_modecfg_funcs = {
@@ -301,13 +394,38 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
return ret;
}
- ret = drm_simple_display_pipe_init(drm, &arcpgu->pipe, &arc_pgu_pipe_funcs,
- arc_pgu_supported_formats,
- ARRAY_SIZE(arc_pgu_supported_formats),
- NULL, connector);
+ ret = drm_universal_plane_init(drm, &arcpgu->plane, 0,
+ &arc_pgu_plane_funcs, arc_pgu_supported_formats,
+ ARRAY_SIZE(arc_pgu_supported_formats), NULL,
+ DRM_PLANE_TYPE_PRIMARY, NULL);
+
+ if (ret)
+ return ret;
+
+ drm_plane_helper_add(&arcpgu->plane, &arc_pgu_plane_helper_funcs);
+
+ ret = drm_crtc_init_with_planes(drm, &arcpgu->crtc, &arcpgu->plane, NULL,
+ &arc_pgu_crtc_funcs, NULL);
+
+ if (ret)
+ return ret;
+
+ drm_crtc_helper_add(&arcpgu->crtc, &arc_pgu_crtc_helper_funcs);
+
+ ret = drm_encoder_init(drm, &arcpgu->encoder, &arc_pgu_encoder_funcs,
+ DRM_MODE_ENCODER_NONE, NULL);
+
if (ret)
return ret;
+ arcpgu->encoder.possible_crtcs = drm_crtc_mask(&arcpgu->crtc);
+
+ if (connector) {
+ ret = drm_connector_attach_encoder(connector, &arcpgu->encoder);
+ if (ret)
+ return ret;
+ }
+
if (encoder_node) {
/* Locate drm bridge from the hdmi encoder DT node */
struct drm_bridge *bridge __free(drm_bridge_put) =
@@ -315,7 +433,7 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
if (!bridge)
return -EPROBE_DEFER;
- ret = drm_simple_display_pipe_attach_bridge(&arcpgu->pipe, bridge);
+ ret = drm_bridge_attach(&arcpgu->encoder, bridge, NULL, 0);
if (ret)
return ret;
}
@@ -342,7 +460,7 @@ static int arcpgu_show_pxlclock(struct seq_file *m, void *arg)
struct drm_device *drm = node->minor->dev;
struct arcpgu_drm_private *arcpgu = dev_to_arcpgu(drm);
unsigned long clkrate = clk_get_rate(arcpgu->clk);
- unsigned long mode_clock = arcpgu->pipe.crtc.mode.crtc_clock * 1000;
+ unsigned long mode_clock = arcpgu->crtc.mode.crtc_clock * 1000;
seq_printf(m, "hw : %lu\n", clkrate);
seq_printf(m, "mode: %lu\n", mode_clock);
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v4] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers
2026-09-09 12:41 ` [PATCH v4] " Davide Bonatto
@ 2026-09-09 12:50 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-09-09 12:50 UTC (permalink / raw)
To: Davide Bonatto; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] NULL pointer dereference in `arc_pgu_crtc_atomic_check` due to improper plane state retrieval API.
--
commit 80a9a4e01830c51d120962eff87086ac506b2424
Author: Davide Bonatto <bonatto.davide@gmail.com>
drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers
This commit migrates the arcpgu driver from the deprecated simple display pipe
helpers to explicitly managing individual plane, CRTC, and encoder objects
using standard atomic helpers.
> diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
> index c93d61ac0bb75..adda406927332 100644
> --- a/drivers/gpu/drm/tiny/arcpgu.c
> +++ b/drivers/gpu/drm/tiny/arcpgu.c
[ ... ]
> @@ -189,54 +203,133 @@ static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu)
[ ... ]
> +static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> +{
> + struct drm_crtc_state *crct_state = drm_atomic_get_new_crtc_state(state, crtc);
> + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> + struct drm_plane_state *plane_state;
> +
> + if (!crct_state->enable)
> + return 0;
> +
> + plane_state = drm_atomic_get_new_plane_state(state, &arcpgu->plane);
> + if (IS_ERR(plane_state))
> + return PTR_ERR(plane_state);
> +
[Severity: High]
Can this result in a NULL pointer dereference during partial atomic commits?
When an atomic commit updates a CRTC property but does not modify the plane
and does not trigger a modeset, the plane might not be added to the atomic
state. In that case, drm_atomic_get_new_plane_state() returns NULL, not an
ERR_PTR.
Because IS_ERR(NULL) evaluates to false, the error check is bypassed, and
evaluating !plane_state->fb will dereference the NULL pointer. Should this
use drm_atomic_get_plane_state() instead, which safely attaches the plane to
the commit and returns an ERR_PTR on failure, or alternatively check for
NULL before dereferencing?
> + if (!plane_state->fb)
> + return -EINVAL;
> +
> + return 0;
> +}
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260909124108.168371-1-bonatto.davide@gmail.com?part=1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers
2026-09-09 9:24 [PATCH] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers Davide Bonatto
` (2 preceding siblings ...)
2026-09-09 12:41 ` [PATCH v4] " Davide Bonatto
@ 2026-09-09 12:58 ` Davide Bonatto
2026-09-09 13:22 ` sashiko-bot
2026-09-10 6:18 ` Thomas Zimmermann
3 siblings, 2 replies; 9+ messages in thread
From: Davide Bonatto @ 2026-09-09 12:58 UTC (permalink / raw)
To: Alexey Brodkin, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter,
open list:DRM DRIVERS, open list
Cc: Davide Bonatto
The drm_simple_display_pipe helper is considered legacy/deprecated in
favor of explicitly managing individual plane, CRTC, and encoder objects
using standard atomic helpers.
Migrate arcpgu to initialize its primary plane, CRTC, and encoder
explicitly, and route mode validation, enable, and disable hooks
through drm_crtc_helper_funcs, and plane updates through
drm_plane_helper_funcs.
Signed-off-by: Davide Bonatto <bonatto.davide@gmail.com>
---
v5:
- Fix NULL pointer dereference in arc_pgu_crtc_atomic_check() by checking
crtc_state->plane_mask instead of manually querying plane state.
v4:
- Use drm_atomic_get_plane_state() in CRTC .atomic_check to ensure
the primary plane state is attached and locked, avoiding false -EINVAL
rejections and broken partial atomic commits.
v3:
- Add CRTC .atomic_check hook to validate primary plane presence
and avoid NULL pointer dereference in arc_pgu_set_pxl_fmt().
- Pass framebuffer explicitly to arc_pgu_mode_set() and read plane
state via drm_atomic_get_new_plane_state() in CRTC enable to prevent UAF.
- Keep CRTC global enable bit intact during plane disable to avoid
hardware state desync.
v2:
- Add .prepare_fb callback to support implicit synchronization.
- Add .atomic_check using drm_atomic_helper_check_plane_state()
to validate hardware scaling/clipping constraints.
- Implement arc_pgu_plane_atomic_disable() to stop DMA and clear
buffer address register, preventing hardware DMA scanout UAF.
drivers/gpu/drm/tiny/arcpgu.c | 188 +++++++++++++++++++++++++++-------
1 file changed, 150 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
index c93d61ac0bb7..c6308fee568d 100644
--- a/drivers/gpu/drm/tiny/arcpgu.c
+++ b/drivers/gpu/drm/tiny/arcpgu.c
@@ -5,6 +5,8 @@
* Copyright (C) 2016 Synopsys, Inc. (www.synopsys.com)
*/
+#include "drm/drm_gem_atomic_helper.h"
+
#include <linux/clk.h>
#include <drm/clients/drm_client_setup.h>
@@ -23,6 +25,8 @@
#include <drm/drm_of.h>
#include <drm/drm_probe_helper.h>
#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_bridge.h>
#include <linux/dma-mapping.h>
#include <linux/module.h>
#include <linux/of_reserved_mem.h>
@@ -52,13 +56,17 @@ struct arcpgu_drm_private {
struct drm_device drm;
void __iomem *regs;
struct clk *clk;
- struct drm_simple_display_pipe pipe;
+ struct drm_plane plane;
+ struct drm_crtc crtc;
+ struct drm_encoder encoder;
struct drm_connector sim_conn;
};
#define dev_to_arcpgu(x) container_of(x, struct arcpgu_drm_private, drm)
-#define pipe_to_arcpgu_priv(x) container_of(x, struct arcpgu_drm_private, pipe)
+#define crtc_to_arcpgu_priv(x) container_of(x, struct arcpgu_drm_private, crtc)
+
+#define plane_to_arcpgu(x) container_of(x, struct arcpgu_drm_private, plane)
static inline void arc_pgu_write(struct arcpgu_drm_private *arcpgu,
unsigned int reg, u32 value)
@@ -115,14 +123,19 @@ static const u32 arc_pgu_supported_formats[] = {
DRM_FORMAT_ARGB8888,
};
-static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu)
+static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu,
+ const struct drm_framebuffer *fb)
{
- const struct drm_framebuffer *fb = arcpgu->pipe.plane.state->fb;
- uint32_t pixel_format = fb->format->format;
+ u32 pixel_format;
u32 format = DRM_FORMAT_INVALID;
int i;
u32 reg_ctrl;
+ if (!fb)
+ return;
+
+ pixel_format = fb->format->format;
+
for (i = 0; i < ARRAY_SIZE(arc_pgu_supported_formats); i++) {
if (arc_pgu_supported_formats[i] == pixel_format)
format = arc_pgu_supported_formats[i];
@@ -139,10 +152,10 @@ static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu)
arc_pgu_write(arcpgu, ARCPGU_REG_CTRL, reg_ctrl);
}
-static enum drm_mode_status arc_pgu_mode_valid(struct drm_simple_display_pipe *pipe,
- const struct drm_display_mode *mode)
+static enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
+ const struct drm_display_mode *mode)
{
- struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
+ struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
long rate, clk_rate = mode->clock * 1000;
long diff = clk_rate / 200; /* +-0.5% allowed by HDMI spec */
@@ -153,9 +166,10 @@ static enum drm_mode_status arc_pgu_mode_valid(struct drm_simple_display_pipe *p
return MODE_NOCLOCK;
}
-static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu)
+static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu,
+ const struct drm_framebuffer *fb)
{
- struct drm_display_mode *m = &arcpgu->pipe.crtc.state->adjusted_mode;
+ struct drm_display_mode *m = &arcpgu->crtc.state->adjusted_mode;
u32 val;
arc_pgu_write(arcpgu, ARCPGU_REG_FMT,
@@ -189,54 +203,127 @@ static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu)
arc_pgu_write(arcpgu, ARCPGU_REG_STRIDE, 0);
arc_pgu_write(arcpgu, ARCPGU_REG_START_SET, 1);
- arc_pgu_set_pxl_fmt(arcpgu);
+ arc_pgu_set_pxl_fmt(arcpgu, fb);
clk_set_rate(arcpgu->clk, m->crtc_clock * 1000);
}
-static void arc_pgu_enable(struct drm_simple_display_pipe *pipe,
- struct drm_crtc_state *crtc_state,
- struct drm_plane_state *plane_state)
+static void arc_pgu_crtc_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
{
- struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
+ struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
+ struct drm_plane_state *plane_state =
+ drm_atomic_get_new_plane_state(state, &arcpgu->plane);
+ const struct drm_framebuffer *fb = plane_state ? plane_state->fb : NULL;
- arc_pgu_mode_set(arcpgu);
+ arc_pgu_mode_set(arcpgu, fb);
clk_prepare_enable(arcpgu->clk);
arc_pgu_write(arcpgu, ARCPGU_REG_CTRL,
arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) |
- ARCPGU_CTRL_ENABLE_MASK);
+ ARCPGU_CTRL_ENABLE_MASK);
}
-static void arc_pgu_disable(struct drm_simple_display_pipe *pipe)
+static void arc_pgu_crtc_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
{
- struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
+ struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
clk_disable_unprepare(arcpgu->clk);
arc_pgu_write(arcpgu, ARCPGU_REG_CTRL,
- arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) &
- ~ARCPGU_CTRL_ENABLE_MASK);
+ arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) &
+ ~ARCPGU_CTRL_ENABLE_MASK);
}
-static void arc_pgu_update(struct drm_simple_display_pipe *pipe,
- struct drm_plane_state *state)
+static void arc_pgu_plane_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_commit *state)
{
- struct arcpgu_drm_private *arcpgu;
+ struct arcpgu_drm_private *arcpgu = plane_to_arcpgu(plane);
+ struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
struct drm_gem_dma_object *gem;
- if (!pipe->plane.state->fb)
+ if (!new_plane_state->fb)
return;
- arcpgu = pipe_to_arcpgu_priv(pipe);
- gem = drm_fb_dma_get_gem_obj(pipe->plane.state->fb, 0);
+ gem = drm_fb_dma_get_gem_obj(new_plane_state->fb, 0);
arc_pgu_write(arcpgu, ARCPGU_REG_BUF0_ADDR, gem->dma_addr);
}
-static const struct drm_simple_display_pipe_funcs arc_pgu_pipe_funcs = {
- .update = arc_pgu_update,
- .mode_valid = arc_pgu_mode_valid,
- .enable = arc_pgu_enable,
- .disable = arc_pgu_disable,
+static void arc_pgu_plane_atomic_disable(struct drm_plane *plane,
+ struct drm_atomic_commit *state)
+{
+ struct arcpgu_drm_private *arcpgu = plane_to_arcpgu(plane);
+
+ arc_pgu_write(arcpgu, ARCPGU_REG_BUF0_ADDR, 0);
+}
+
+static int arc_pgu_plane_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_commit *state)
+{
+ struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+ struct drm_crtc_state *crtc_state;
+
+ if (!new_plane_state->crtc)
+ return 0;
+
+ crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
+ if (!crtc_state)
+ return -EINVAL;
+
+ return drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, true);
+}
+
+static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
+{
+ struct drm_crtc_state *crct_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+ if (!crct_state->enable)
+ return 0;
+
+ if (!crct_state->plane_mask)
+ return -EINVAL;
+
+ return 0;
+}
+
+static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
+ .mode_valid = arc_pgu_crtc_mode_valid,
+ .atomic_enable = arc_pgu_crtc_atomic_enable,
+ .atomic_disable = arc_pgu_crtc_atomic_disable,
+ .atomic_check = arc_pgu_crtc_atomic_check,
+};
+
+static const struct drm_crtc_funcs arc_pgu_crtc_funcs = {
+ .reset = drm_atomic_helper_crtc_reset,
+ .destroy = drm_crtc_cleanup,
+ .set_config = drm_atomic_helper_set_config,
+ .page_flip = drm_atomic_helper_page_flip,
+ .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static const struct drm_plane_helper_funcs arc_pgu_plane_helper_funcs = {
+ .atomic_update = arc_pgu_plane_atomic_update,
+ .prepare_fb = drm_gem_plane_helper_prepare_fb,
+ .atomic_check = arc_pgu_plane_atomic_check,
+ .atomic_disable = arc_pgu_plane_atomic_disable,
+};
+
+static const struct drm_plane_funcs arc_pgu_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .destroy = drm_plane_cleanup,
+ .reset = drm_atomic_helper_plane_reset,
+ .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
+static const struct drm_encoder_funcs arc_pgu_encoder_funcs = {
+ .destroy = drm_encoder_cleanup
};
static const struct drm_mode_config_funcs arcpgu_drm_modecfg_funcs = {
@@ -301,13 +388,38 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
return ret;
}
- ret = drm_simple_display_pipe_init(drm, &arcpgu->pipe, &arc_pgu_pipe_funcs,
- arc_pgu_supported_formats,
- ARRAY_SIZE(arc_pgu_supported_formats),
- NULL, connector);
+ ret = drm_universal_plane_init(drm, &arcpgu->plane, 0,
+ &arc_pgu_plane_funcs, arc_pgu_supported_formats,
+ ARRAY_SIZE(arc_pgu_supported_formats), NULL,
+ DRM_PLANE_TYPE_PRIMARY, NULL);
+
if (ret)
return ret;
+ drm_plane_helper_add(&arcpgu->plane, &arc_pgu_plane_helper_funcs);
+
+ ret = drm_crtc_init_with_planes(drm, &arcpgu->crtc, &arcpgu->plane, NULL,
+ &arc_pgu_crtc_funcs, NULL);
+
+ if (ret)
+ return ret;
+
+ drm_crtc_helper_add(&arcpgu->crtc, &arc_pgu_crtc_helper_funcs);
+
+ ret = drm_encoder_init(drm, &arcpgu->encoder, &arc_pgu_encoder_funcs,
+ DRM_MODE_ENCODER_NONE, NULL);
+
+ if (ret)
+ return ret;
+
+ arcpgu->encoder.possible_crtcs = drm_crtc_mask(&arcpgu->crtc);
+
+ if (connector) {
+ ret = drm_connector_attach_encoder(connector, &arcpgu->encoder);
+ if (ret)
+ return ret;
+ }
+
if (encoder_node) {
/* Locate drm bridge from the hdmi encoder DT node */
struct drm_bridge *bridge __free(drm_bridge_put) =
@@ -315,7 +427,7 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
if (!bridge)
return -EPROBE_DEFER;
- ret = drm_simple_display_pipe_attach_bridge(&arcpgu->pipe, bridge);
+ ret = drm_bridge_attach(&arcpgu->encoder, bridge, NULL, 0);
if (ret)
return ret;
}
@@ -342,7 +454,7 @@ static int arcpgu_show_pxlclock(struct seq_file *m, void *arg)
struct drm_device *drm = node->minor->dev;
struct arcpgu_drm_private *arcpgu = dev_to_arcpgu(drm);
unsigned long clkrate = clk_get_rate(arcpgu->clk);
- unsigned long mode_clock = arcpgu->pipe.crtc.mode.crtc_clock * 1000;
+ unsigned long mode_clock = arcpgu->crtc.mode.crtc_clock * 1000;
seq_printf(m, "hw : %lu\n", clkrate);
seq_printf(m, "mode: %lu\n", mode_clock);
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v5] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers
2026-09-09 12:58 ` [PATCH v5] " Davide Bonatto
@ 2026-09-09 13:22 ` sashiko-bot
2026-09-10 6:18 ` Thomas Zimmermann
1 sibling, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-09-09 13:22 UTC (permalink / raw)
To: Davide Bonatto; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing primary plane visibility check allows CRTC to be enabled with disabled plane, causing DMA scanout from physical address 0.
--
commit 0780d3c6b6d7797f539151d0b77405608d2d3ea1
Author: Davide Bonatto <bonatto.davide@gmail.com>
Subject: drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers
The drm_simple_display_pipe helper is considered legacy/deprecated in
favor of explicitly managing individual plane, CRTC, and encoder objects
using standard atomic helpers. Migrate arcpgu to explicitly route mode
validation and atomic hooks through standard helper functions.
> diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
> index c93d61ac0bb75..c6308fee568db 100644
> --- a/drivers/gpu/drm/tiny/arcpgu.c
> +++ b/drivers/gpu/drm/tiny/arcpgu.c
[ ... ]
> @@ -189,54 +203,127 @@ static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu)
[ ... ]
> +static int arc_pgu_plane_atomic_check(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> +{
> + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> + struct drm_crtc_state *crtc_state;
> +
> + if (!new_plane_state->crtc)
> + return 0;
> +
> + crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
> + if (!crtc_state)
> + return -EINVAL;
> +
> + return drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
> + DRM_PLANE_NO_SCALING,
> + DRM_PLANE_NO_SCALING,
> + false, true);
[Severity: High]
Does this code allow the CRTC to remain enabled without a visible primary
plane, potentially causing the DMA controller to scan from physical address 0?
When drm_atomic_helper_check_plane_state() runs, it returns 0 (success)
if the plane is not visible (e.g., when the framebuffer is NULL).
If a userspace application issues an atomic commit that sets the primary
plane's FB_ID to 0 without also disabling the CRTC, the atomic core sets
new_plane_state->fb = NULL and new_plane_state->visible = false.
Could we add a check here verifying that the primary plane is visible when
the CRTC is enabled (e.g., if (crtc_state->enable && !new_plane_state->visible)
return -EINVAL;) to prevent scanning from address 0?
> +}
> +
> +static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> +{
> + struct drm_crtc_state *crct_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> + if (!crct_state->enable)
> + return 0;
> +
> + if (!crct_state->plane_mask)
> + return -EINVAL;
[Severity: High]
Is this check sufficient to guarantee that the primary plane has a valid
framebuffer?
Setting the primary plane's FB_ID to 0 does not change the plane's crtc
mapping, meaning crct_state->plane_mask remains non-zero. This check will
incorrectly approve the commit, allowing it to proceed.
The commit would then invoke arc_pgu_plane_atomic_disable() which writes 0 to
ARCPGU_REG_BUF0_ADDR. Since the CRTC remains enabled, the hardware DMA
controller could begin scanning out pixels from physical address 0, causing bus
errors, IOMMU faults, or memory corruption.
> +
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260909125806.174851-1-bonatto.davide@gmail.com?part=1
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v5] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers
2026-09-09 12:58 ` [PATCH v5] " Davide Bonatto
2026-09-09 13:22 ` sashiko-bot
@ 2026-09-10 6:18 ` Thomas Zimmermann
1 sibling, 0 replies; 9+ messages in thread
From: Thomas Zimmermann @ 2026-09-10 6:18 UTC (permalink / raw)
To: Davide Bonatto, Alexey Brodkin, Maarten Lankhorst, Maxime Ripard,
David Airlie, Simona Vetter, open list:DRM DRIVERS, open list
Please stop spamming this list with patch mails
Am 09.09.26 um 14:58 schrieb Davide Bonatto:
> The drm_simple_display_pipe helper is considered legacy/deprecated in
> favor of explicitly managing individual plane, CRTC, and encoder objects
> using standard atomic helpers.
>
> Migrate arcpgu to initialize its primary plane, CRTC, and encoder
> explicitly, and route mode validation, enable, and disable hooks
> through drm_crtc_helper_funcs, and plane updates through
> drm_plane_helper_funcs.
>
> Signed-off-by: Davide Bonatto <bonatto.davide@gmail.com>
> ---
> v5:
> - Fix NULL pointer dereference in arc_pgu_crtc_atomic_check() by checking
> crtc_state->plane_mask instead of manually querying plane state.
> v4:
> - Use drm_atomic_get_plane_state() in CRTC .atomic_check to ensure
> the primary plane state is attached and locked, avoiding false -EINVAL
> rejections and broken partial atomic commits.
> v3:
> - Add CRTC .atomic_check hook to validate primary plane presence
> and avoid NULL pointer dereference in arc_pgu_set_pxl_fmt().
> - Pass framebuffer explicitly to arc_pgu_mode_set() and read plane
> state via drm_atomic_get_new_plane_state() in CRTC enable to prevent UAF.
> - Keep CRTC global enable bit intact during plane disable to avoid
> hardware state desync.
> v2:
> - Add .prepare_fb callback to support implicit synchronization.
> - Add .atomic_check using drm_atomic_helper_check_plane_state()
> to validate hardware scaling/clipping constraints.
> - Implement arc_pgu_plane_atomic_disable() to stop DMA and clear
> buffer address register, preventing hardware DMA scanout UAF.
>
> drivers/gpu/drm/tiny/arcpgu.c | 188 +++++++++++++++++++++++++++-------
> 1 file changed, 150 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
> index c93d61ac0bb7..c6308fee568d 100644
> --- a/drivers/gpu/drm/tiny/arcpgu.c
> +++ b/drivers/gpu/drm/tiny/arcpgu.c
> @@ -5,6 +5,8 @@
> * Copyright (C) 2016 Synopsys, Inc. (www.synopsys.com)
> */
>
> +#include "drm/drm_gem_atomic_helper.h"
> +
> #include <linux/clk.h>
>
> #include <drm/clients/drm_client_setup.h>
> @@ -23,6 +25,8 @@
> #include <drm/drm_of.h>
> #include <drm/drm_probe_helper.h>
> #include <drm/drm_simple_kms_helper.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_bridge.h>
> #include <linux/dma-mapping.h>
> #include <linux/module.h>
> #include <linux/of_reserved_mem.h>
> @@ -52,13 +56,17 @@ struct arcpgu_drm_private {
> struct drm_device drm;
> void __iomem *regs;
> struct clk *clk;
> - struct drm_simple_display_pipe pipe;
> + struct drm_plane plane;
> + struct drm_crtc crtc;
> + struct drm_encoder encoder;
> struct drm_connector sim_conn;
> };
>
> #define dev_to_arcpgu(x) container_of(x, struct arcpgu_drm_private, drm)
>
> -#define pipe_to_arcpgu_priv(x) container_of(x, struct arcpgu_drm_private, pipe)
> +#define crtc_to_arcpgu_priv(x) container_of(x, struct arcpgu_drm_private, crtc)
> +
> +#define plane_to_arcpgu(x) container_of(x, struct arcpgu_drm_private, plane)
>
> static inline void arc_pgu_write(struct arcpgu_drm_private *arcpgu,
> unsigned int reg, u32 value)
> @@ -115,14 +123,19 @@ static const u32 arc_pgu_supported_formats[] = {
> DRM_FORMAT_ARGB8888,
> };
>
> -static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu)
> +static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu,
> + const struct drm_framebuffer *fb)
> {
> - const struct drm_framebuffer *fb = arcpgu->pipe.plane.state->fb;
> - uint32_t pixel_format = fb->format->format;
> + u32 pixel_format;
> u32 format = DRM_FORMAT_INVALID;
> int i;
> u32 reg_ctrl;
>
> + if (!fb)
> + return;
> +
> + pixel_format = fb->format->format;
> +
> for (i = 0; i < ARRAY_SIZE(arc_pgu_supported_formats); i++) {
> if (arc_pgu_supported_formats[i] == pixel_format)
> format = arc_pgu_supported_formats[i];
> @@ -139,10 +152,10 @@ static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu)
> arc_pgu_write(arcpgu, ARCPGU_REG_CTRL, reg_ctrl);
> }
>
> -static enum drm_mode_status arc_pgu_mode_valid(struct drm_simple_display_pipe *pipe,
> - const struct drm_display_mode *mode)
> +static enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
> + const struct drm_display_mode *mode)
> {
> - struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
> + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> long rate, clk_rate = mode->clock * 1000;
> long diff = clk_rate / 200; /* +-0.5% allowed by HDMI spec */
>
> @@ -153,9 +166,10 @@ static enum drm_mode_status arc_pgu_mode_valid(struct drm_simple_display_pipe *p
> return MODE_NOCLOCK;
> }
>
> -static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu)
> +static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu,
> + const struct drm_framebuffer *fb)
> {
> - struct drm_display_mode *m = &arcpgu->pipe.crtc.state->adjusted_mode;
> + struct drm_display_mode *m = &arcpgu->crtc.state->adjusted_mode;
> u32 val;
>
> arc_pgu_write(arcpgu, ARCPGU_REG_FMT,
> @@ -189,54 +203,127 @@ static void arc_pgu_mode_set(struct arcpgu_drm_private *arcpgu)
> arc_pgu_write(arcpgu, ARCPGU_REG_STRIDE, 0);
> arc_pgu_write(arcpgu, ARCPGU_REG_START_SET, 1);
>
> - arc_pgu_set_pxl_fmt(arcpgu);
> + arc_pgu_set_pxl_fmt(arcpgu, fb);
>
> clk_set_rate(arcpgu->clk, m->crtc_clock * 1000);
> }
>
> -static void arc_pgu_enable(struct drm_simple_display_pipe *pipe,
> - struct drm_crtc_state *crtc_state,
> - struct drm_plane_state *plane_state)
> +static void arc_pgu_crtc_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> {
> - struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
> + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> + struct drm_plane_state *plane_state =
> + drm_atomic_get_new_plane_state(state, &arcpgu->plane);
> + const struct drm_framebuffer *fb = plane_state ? plane_state->fb : NULL;
>
> - arc_pgu_mode_set(arcpgu);
> + arc_pgu_mode_set(arcpgu, fb);
>
> clk_prepare_enable(arcpgu->clk);
> arc_pgu_write(arcpgu, ARCPGU_REG_CTRL,
> arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) |
> - ARCPGU_CTRL_ENABLE_MASK);
> + ARCPGU_CTRL_ENABLE_MASK);
> }
>
> -static void arc_pgu_disable(struct drm_simple_display_pipe *pipe)
> +static void arc_pgu_crtc_atomic_disable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> {
> - struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe);
> + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
>
> clk_disable_unprepare(arcpgu->clk);
> arc_pgu_write(arcpgu, ARCPGU_REG_CTRL,
> - arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) &
> - ~ARCPGU_CTRL_ENABLE_MASK);
> + arc_pgu_read(arcpgu, ARCPGU_REG_CTRL) &
> + ~ARCPGU_CTRL_ENABLE_MASK);
> }
>
> -static void arc_pgu_update(struct drm_simple_display_pipe *pipe,
> - struct drm_plane_state *state)
> +static void arc_pgu_plane_atomic_update(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> {
> - struct arcpgu_drm_private *arcpgu;
> + struct arcpgu_drm_private *arcpgu = plane_to_arcpgu(plane);
> + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> struct drm_gem_dma_object *gem;
>
> - if (!pipe->plane.state->fb)
> + if (!new_plane_state->fb)
> return;
>
> - arcpgu = pipe_to_arcpgu_priv(pipe);
> - gem = drm_fb_dma_get_gem_obj(pipe->plane.state->fb, 0);
> + gem = drm_fb_dma_get_gem_obj(new_plane_state->fb, 0);
> arc_pgu_write(arcpgu, ARCPGU_REG_BUF0_ADDR, gem->dma_addr);
> }
>
> -static const struct drm_simple_display_pipe_funcs arc_pgu_pipe_funcs = {
> - .update = arc_pgu_update,
> - .mode_valid = arc_pgu_mode_valid,
> - .enable = arc_pgu_enable,
> - .disable = arc_pgu_disable,
> +static void arc_pgu_plane_atomic_disable(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> +{
> + struct arcpgu_drm_private *arcpgu = plane_to_arcpgu(plane);
> +
> + arc_pgu_write(arcpgu, ARCPGU_REG_BUF0_ADDR, 0);
> +}
> +
> +static int arc_pgu_plane_atomic_check(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> +{
> + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> + struct drm_crtc_state *crtc_state;
> +
> + if (!new_plane_state->crtc)
> + return 0;
> +
> + crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
> + if (!crtc_state)
> + return -EINVAL;
> +
> + return drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
> + DRM_PLANE_NO_SCALING,
> + DRM_PLANE_NO_SCALING,
> + false, true);
> +}
> +
> +static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> +{
> + struct drm_crtc_state *crct_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> + if (!crct_state->enable)
> + return 0;
> +
> + if (!crct_state->plane_mask)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
> + .mode_valid = arc_pgu_crtc_mode_valid,
> + .atomic_enable = arc_pgu_crtc_atomic_enable,
> + .atomic_disable = arc_pgu_crtc_atomic_disable,
> + .atomic_check = arc_pgu_crtc_atomic_check,
> +};
> +
> +static const struct drm_crtc_funcs arc_pgu_crtc_funcs = {
> + .reset = drm_atomic_helper_crtc_reset,
> + .destroy = drm_crtc_cleanup,
> + .set_config = drm_atomic_helper_set_config,
> + .page_flip = drm_atomic_helper_page_flip,
> + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +};
> +
> +static const struct drm_plane_helper_funcs arc_pgu_plane_helper_funcs = {
> + .atomic_update = arc_pgu_plane_atomic_update,
> + .prepare_fb = drm_gem_plane_helper_prepare_fb,
> + .atomic_check = arc_pgu_plane_atomic_check,
> + .atomic_disable = arc_pgu_plane_atomic_disable,
> +};
> +
> +static const struct drm_plane_funcs arc_pgu_plane_funcs = {
> + .update_plane = drm_atomic_helper_update_plane,
> + .disable_plane = drm_atomic_helper_disable_plane,
> + .destroy = drm_plane_cleanup,
> + .reset = drm_atomic_helper_plane_reset,
> + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> +};
> +
> +static const struct drm_encoder_funcs arc_pgu_encoder_funcs = {
> + .destroy = drm_encoder_cleanup
> };
>
> static const struct drm_mode_config_funcs arcpgu_drm_modecfg_funcs = {
> @@ -301,13 +388,38 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
> return ret;
> }
>
> - ret = drm_simple_display_pipe_init(drm, &arcpgu->pipe, &arc_pgu_pipe_funcs,
> - arc_pgu_supported_formats,
> - ARRAY_SIZE(arc_pgu_supported_formats),
> - NULL, connector);
> + ret = drm_universal_plane_init(drm, &arcpgu->plane, 0,
> + &arc_pgu_plane_funcs, arc_pgu_supported_formats,
> + ARRAY_SIZE(arc_pgu_supported_formats), NULL,
> + DRM_PLANE_TYPE_PRIMARY, NULL);
> +
> if (ret)
> return ret;
>
> + drm_plane_helper_add(&arcpgu->plane, &arc_pgu_plane_helper_funcs);
> +
> + ret = drm_crtc_init_with_planes(drm, &arcpgu->crtc, &arcpgu->plane, NULL,
> + &arc_pgu_crtc_funcs, NULL);
> +
> + if (ret)
> + return ret;
> +
> + drm_crtc_helper_add(&arcpgu->crtc, &arc_pgu_crtc_helper_funcs);
> +
> + ret = drm_encoder_init(drm, &arcpgu->encoder, &arc_pgu_encoder_funcs,
> + DRM_MODE_ENCODER_NONE, NULL);
> +
> + if (ret)
> + return ret;
> +
> + arcpgu->encoder.possible_crtcs = drm_crtc_mask(&arcpgu->crtc);
> +
> + if (connector) {
> + ret = drm_connector_attach_encoder(connector, &arcpgu->encoder);
> + if (ret)
> + return ret;
> + }
> +
> if (encoder_node) {
> /* Locate drm bridge from the hdmi encoder DT node */
> struct drm_bridge *bridge __free(drm_bridge_put) =
> @@ -315,7 +427,7 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
> if (!bridge)
> return -EPROBE_DEFER;
>
> - ret = drm_simple_display_pipe_attach_bridge(&arcpgu->pipe, bridge);
> + ret = drm_bridge_attach(&arcpgu->encoder, bridge, NULL, 0);
> if (ret)
> return ret;
> }
> @@ -342,7 +454,7 @@ static int arcpgu_show_pxlclock(struct seq_file *m, void *arg)
> struct drm_device *drm = node->minor->dev;
> struct arcpgu_drm_private *arcpgu = dev_to_arcpgu(drm);
> unsigned long clkrate = clk_get_rate(arcpgu->clk);
> - unsigned long mode_clock = arcpgu->pipe.crtc.mode.crtc_clock * 1000;
> + unsigned long mode_clock = arcpgu->crtc.mode.crtc_clock * 1000;
>
> seq_printf(m, "hw : %lu\n", clkrate);
> seq_printf(m, "mode: %lu\n", mode_clock);
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
^ permalink raw reply [flat|nested] 9+ messages in thread