[PATCH 4/9] drm/mcde: replace struct drm_simple_display_pipe with regular atomic helpers
Thomas Zimmermann
tzimmermann at suse.de
Wed Jul 8 06:02:14 PDT 2026
Hi
Am 04.07.26 um 20:31 schrieb Ze Huang:
> Convert MCDE to explicit plane, CRTC and encoder objects.
>
> Keep FIFO, event and framebuffer update sequencing intact, and install
> GEM framebuffer prepare callback explicitly.
>
> Signed-off-by: Ze Huang <ze.huang at oss.qualcomm.com>
> ---
> drivers/gpu/drm/mcde/mcde_display.c | 162 +++++++++++++++++++++++++++---------
> drivers/gpu/drm/mcde/mcde_drm.h | 6 +-
> drivers/gpu/drm/mcde/mcde_drv.c | 3 +-
> 3 files changed, 129 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/mcde/mcde_display.c b/drivers/gpu/drm/mcde/mcde_display.c
> index 257a6e84dd58..4d86fa5030eb 100644
> --- a/drivers/gpu/drm/mcde/mcde_display.c
> +++ b/drivers/gpu/drm/mcde/mcde_display.c
> @@ -10,6 +10,7 @@
> #include <linux/regulator/consumer.h>
> #include <linux/media-bus-format.h>
>
> +#include <drm/drm_atomic_helper.h>
> #include <drm/drm_device.h>
> #include <drm/drm_fb_dma_helper.h>
> #include <drm/drm_fourcc.h>
> @@ -18,7 +19,6 @@
> #include <drm/drm_gem_dma_helper.h>
> #include <drm/drm_mipi_dsi.h>
> #include <drm/drm_print.h>
> -#include <drm/drm_simple_kms_helper.h>
> #include <drm/drm_bridge.h>
> #include <drm/drm_vblank.h>
> #include <video/mipi_display.h>
> @@ -132,7 +132,7 @@ void mcde_display_irq(struct mcde *mcde)
> writel(mispp, mcde->regs + MCDE_RISPP);
>
> if (vblank)
> - drm_crtc_handle_vblank(&mcde->pipe.crtc);
> + drm_crtc_handle_vblank(&mcde->crtc);
>
> if (misovl)
> dev_info(mcde->dev, "some stray overlay IRQ %08x\n", misovl);
> @@ -157,13 +157,35 @@ void mcde_display_disable_irqs(struct mcde *mcde)
> writel(0xFFFFFFFF, mcde->regs + MCDE_RISCHNL);
> }
>
> -static int mcde_display_check(struct drm_simple_display_pipe *pipe,
> - struct drm_plane_state *pstate,
> - struct drm_crtc_state *cstate)
> +static int mcde_plane_helper_atomic_check(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> {
> - const struct drm_display_mode *mode = &cstate->mode;
> - struct drm_framebuffer *old_fb = pipe->plane.state->fb;
> + struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(state, plane);
> + struct drm_crtc *crtc = pstate->crtc;
> + struct drm_crtc_state *cstate;
> + const struct drm_display_mode *mode;
> + struct drm_framebuffer *old_fb = plane->state->fb;
> struct drm_framebuffer *fb = pstate->fb;
> + int ret;
> +
> + if (!crtc)
> + return 0;
Your planes' atomic_check functions should always run
drm_atomic_helper_check_plane_state() first. Otherwise, the plane state
will be incorrect.
If there is no crtc, simply pass NULL for the CRTC state. I'd advise to
duplicate the pattern at [1] from lines 487 to 498. After
_check_plane_state() ran, the atomic_check can do additional tests.
If not looked over all the other patches for this problem, but this
comment would apply to all of them.
[1]
https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L487
> +
> + cstate = drm_atomic_get_new_crtc_state(state, crtc);
> + if (!cstate)
> + return 0;
> +
> + ret = drm_atomic_helper_check_plane_state(pstate, cstate,
> + DRM_PLANE_NO_SCALING,
> + DRM_PLANE_NO_SCALING,
> + false, false);
> + if (ret)
> + return ret;
> +
> + if (!pstate->visible)
> + return 0;
> +
> + mode = &cstate->mode;
>
> if (fb) {
> u32 offset = drm_fb_dma_get_gem_addr(fb, pstate, 0);
> @@ -1149,16 +1171,14 @@ static void mcde_setup_dsi(struct mcde *mcde, const struct drm_display_mode *mod
> *dsi_formatter_frame = formatter_frame;
> }
>
> -static void mcde_display_enable(struct drm_simple_display_pipe *pipe,
> - struct drm_crtc_state *cstate,
> - struct drm_plane_state *plane_state)
> +static void mcde_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> {
> - struct drm_crtc *crtc = &pipe->crtc;
> - struct drm_plane *plane = &pipe->plane;
> struct drm_device *drm = crtc->dev;
> struct mcde *mcde = to_mcde(drm);
> + struct drm_crtc_state *cstate = crtc->state;
> const struct drm_display_mode *mode = &cstate->mode;
> - struct drm_framebuffer *fb = plane->state->fb;
> + struct drm_framebuffer *fb = mcde->plane.state->fb;
> u32 format = fb->format->format;
> int dsi_pkt_size;
> int fifo_wtrmrk;
> @@ -1298,9 +1318,9 @@ static void mcde_display_enable(struct drm_simple_display_pipe *pipe,
> dev_info(drm->dev, "MCDE display is enabled\n");
> }
>
> -static void mcde_display_disable(struct drm_simple_display_pipe *pipe)
> +static void mcde_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> {
> - struct drm_crtc *crtc = &pipe->crtc;
> struct drm_device *drm = crtc->dev;
> struct mcde *mcde = to_mcde(drm);
> struct drm_pending_vblank_event *event;
> @@ -1381,17 +1401,23 @@ static void mcde_set_extsrc(struct mcde *mcde, u32 buffer_address)
> writel(buffer_address + mcde->stride, mcde->regs + MCDE_EXTSRCXA1);
> }
>
> -static void mcde_display_update(struct drm_simple_display_pipe *pipe,
> - struct drm_plane_state *old_pstate)
> +static void mcde_plane_helper_atomic_update(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> {
> - struct drm_crtc *crtc = &pipe->crtc;
> - struct drm_device *drm = crtc->dev;
> - struct mcde *mcde = to_mcde(drm);
> - struct drm_pending_vblank_event *event = crtc->state->event;
> - struct drm_plane *plane = &pipe->plane;
> + struct drm_crtc *crtc = plane->state->crtc;
> + struct drm_device *drm;
> + struct mcde *mcde;
> + struct drm_pending_vblank_event *event;
> struct drm_plane_state *pstate = plane->state;
> struct drm_framebuffer *fb = pstate->fb;
>
> + if (!crtc)
> + return;
The helper first does vblank handling and then handles visibility by
testing "if (fb)". No need for this test.
> +
> + drm = crtc->dev;
> + mcde = to_mcde(drm);
> + event = crtc->state->event;
> +
And this needs to handle !crtc without returning.
> /*
> * Handle any pending event first, we need to arm the vblank
And the next block handled vblanks, which is not the right place. That's
a preexisting issue. Vblank handling is better done in the crtc's
atomic_flush.
Best regards
Thomas
> * interrupt before sending any update to the display so we don't
> @@ -1443,9 +1469,8 @@ static void mcde_display_update(struct drm_simple_display_pipe *pipe,
> }
> }
>
> -static int mcde_display_enable_vblank(struct drm_simple_display_pipe *pipe)
> +static int mcde_crtc_enable_vblank(struct drm_crtc *crtc)
> {
> - struct drm_crtc *crtc = &pipe->crtc;
> struct drm_device *drm = crtc->dev;
> struct mcde *mcde = to_mcde(drm);
> u32 val;
> @@ -1462,9 +1487,8 @@ static int mcde_display_enable_vblank(struct drm_simple_display_pipe *pipe)
> return 0;
> }
>
> -static void mcde_display_disable_vblank(struct drm_simple_display_pipe *pipe)
> +static void mcde_crtc_disable_vblank(struct drm_crtc *crtc)
> {
> - struct drm_crtc *crtc = &pipe->crtc;
> struct drm_device *drm = crtc->dev;
> struct mcde *mcde = to_mcde(drm);
>
> @@ -1474,13 +1498,56 @@ static void mcde_display_disable_vblank(struct drm_simple_display_pipe *pipe)
> writel(0xFFFFFFFF, mcde->regs + MCDE_RISPP);
> }
>
> -static struct drm_simple_display_pipe_funcs mcde_display_funcs = {
> - .check = mcde_display_check,
> - .enable = mcde_display_enable,
> - .disable = mcde_display_disable,
> - .update = mcde_display_update,
> - .enable_vblank = mcde_display_enable_vblank,
> - .disable_vblank = mcde_display_disable_vblank,
> +static int mcde_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_commit *state)
> +{
> + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> + int ret;
> +
> + if (!crtc_state->enable)
> + goto out;
> +
> + ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
> + if (ret)
> + return ret;
> +
> +out:
> + return drm_atomic_add_affected_planes(state, crtc);
> +}
> +
> +static const struct drm_crtc_funcs mcde_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,
> + .enable_vblank = mcde_crtc_enable_vblank,
> + .disable_vblank = mcde_crtc_disable_vblank,
> +};
> +
> +static const struct drm_crtc_helper_funcs mcde_crtc_helper_funcs = {
> + .atomic_check = mcde_crtc_helper_atomic_check,
> + .atomic_enable = mcde_crtc_helper_atomic_enable,
> + .atomic_disable = mcde_crtc_helper_atomic_disable,
> +};
> +
> +static const struct drm_plane_funcs mcde_plane_funcs = {
> + .update_plane = drm_atomic_helper_update_plane,
> + .disable_plane = drm_atomic_helper_disable_plane,
> + .reset = drm_atomic_helper_plane_reset,
> + .destroy = drm_plane_cleanup,
> + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> +};
> +
> +static const struct drm_plane_helper_funcs mcde_plane_helper_funcs = {
> + .prepare_fb = drm_gem_plane_helper_prepare_fb,
> + .atomic_check = mcde_plane_helper_atomic_check,
> + .atomic_update = mcde_plane_helper_atomic_update,
> +};
> +
> +static const struct drm_encoder_funcs mcde_encoder_funcs = {
> + .destroy = drm_encoder_cleanup,
> };
>
> int mcde_display_init(struct drm_device *drm)
> @@ -1510,11 +1577,30 @@ int mcde_display_init(struct drm_device *drm)
> if (ret)
> return ret;
>
> - ret = drm_simple_display_pipe_init(drm, &mcde->pipe,
> - &mcde_display_funcs,
> - formats, ARRAY_SIZE(formats),
> - NULL,
> - mcde->connector);
> + ret = drm_universal_plane_init(drm, &mcde->plane, 0,
> + &mcde_plane_funcs,
> + formats, ARRAY_SIZE(formats),
> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> + if (ret)
> + return ret;
> +
> + drm_plane_helper_add(&mcde->plane, &mcde_plane_helper_funcs);
> +
> + ret = drm_crtc_init_with_planes(drm, &mcde->crtc, &mcde->plane,
> + NULL, &mcde_crtc_funcs, NULL);
> + if (ret)
> + return ret;
> +
> + drm_crtc_helper_add(&mcde->crtc, &mcde_crtc_helper_funcs);
> +
> + ret = drm_encoder_init(drm, &mcde->encoder, &mcde_encoder_funcs,
> + DRM_MODE_ENCODER_NONE, NULL);
> + if (ret)
> + return ret;
> +
> + mcde->encoder.possible_crtcs = drm_crtc_mask(&mcde->crtc);
> +
> + ret = drm_connector_attach_encoder(mcde->connector, &mcde->encoder);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/mcde/mcde_drm.h b/drivers/gpu/drm/mcde/mcde_drm.h
> index ecb70b4b737c..6123afb1e3b8 100644
> --- a/drivers/gpu/drm/mcde/mcde_drm.h
> +++ b/drivers/gpu/drm/mcde/mcde_drm.h
> @@ -4,7 +4,7 @@
> * Parts of this file were based on the MCDE driver by Marcus Lorentzon
> * (C) ST-Ericsson SA 2013
> */
> -#include <drm/drm_simple_kms_helper.h>
> +#include <drm/drm_encoder.h>
>
> #ifndef _MCDE_DRM_H_
> #define _MCDE_DRM_H_
> @@ -72,7 +72,9 @@ struct mcde {
> struct drm_panel *panel;
> struct drm_bridge *bridge;
> struct drm_connector *connector;
> - struct drm_simple_display_pipe pipe;
> + struct drm_plane plane;
> + struct drm_crtc crtc;
> + struct drm_encoder encoder;
> struct mipi_dsi_device *mdsi;
> bool dpi_output;
> s16 stride;
> diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c
> index 5f2c462bad7e..401cf8ab83bc 100644
> --- a/drivers/gpu/drm/mcde/mcde_drv.c
> +++ b/drivers/gpu/drm/mcde/mcde_drv.c
> @@ -186,8 +186,7 @@ static int mcde_modeset_init(struct drm_device *drm)
> }
>
> /* Attach the bridge. */
> - ret = drm_simple_display_pipe_attach_bridge(&mcde->pipe,
> - mcde->bridge);
> + ret = drm_bridge_attach(&mcde->encoder, mcde->bridge, NULL, 0);
> if (ret) {
> dev_err(drm->dev, "failed to attach display output bridge\n");
> return ret;
>
--
--
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)
More information about the linux-arm-kernel
mailing list