[PATCH v4 2/2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers
Thomas Zimmermann
tzimmermann at suse.de
Wed Sep 23 00:12:02 PDT 2026
Hi
Am 19.09.26 um 16:59 schrieb Ze Huang:
> Convert i.MX LCDC to explicit primary plane, CRTC and encoder objects.
> Keep no-scaling plane check and GEM framebuffer prepare callback from
> simple-KMS path.
>
> Wire the vblank lifecycle explicitly with CRTC vblank callbacks and
> drm_crtc_vblank_on()/drm_crtc_vblank_off(). Use the old CRTC state in
> the disable path for clock unwinding so the clock reference count
> remains paired with the previous active state.
>
> Use the atomic_create_state helpers with
> drm_mode_config_create_initial_state() in probe, and
> drm_crtc_vblank_atomic_flush() for the flush callback.
>
> Signed-off-by: Ze Huang <ze.huang at oss.qualcomm.com>
IDK how well (or if) this driver is still maintained. I'd give it the
benefit of the doubt to make progress with the DRM cleanup. If there's a
bug in this patch, users will report it quickly.
Reviewed-by: Thomas Zimmermann <tzimmermann at suse.de>
Best regards
Thomas
> ---
> drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 250 +++++++++++++++++++++++++-----------
> 1 file changed, 175 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> index e00e52c1c46e..8f0a0d4cf61a 100644
> --- a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> +++ b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> @@ -2,6 +2,7 @@
> // SPDX-FileCopyrightText: 2020 Marian Cichy <M.Cichy at pengutronix.de>
>
> #include <drm/clients/drm_client_setup.h>
> +#include <drm/drm_atomic.h>
> #include <drm/drm_bridge.h>
> #include <drm/drm_bridge_connector.h>
> #include <drm/drm_damage_helper.h>
> @@ -14,10 +15,11 @@
> #include <drm/drm_gem_dma_helper.h>
> #include <drm/drm_gem_framebuffer_helper.h>
> #include <drm/drm_of.h>
> +#include <drm/drm_plane_helper.h>
> #include <drm/drm_print.h>
> #include <drm/drm_probe_helper.h>
> -#include <drm/drm_simple_kms_helper.h>
> #include <drm/drm_vblank.h>
> +#include <drm/drm_vblank_helper.h>
> #include <linux/bitfield.h>
> #include <linux/clk.h>
> #include <linux/dma-mapping.h>
> @@ -101,7 +103,9 @@
>
> struct imx_lcdc {
> struct drm_device drm;
> - struct drm_simple_display_pipe pipe;
> + struct drm_plane plane;
> + struct drm_crtc crtc;
> + struct drm_encoder encoder;
> struct drm_connector *connector;
> void __iomem *base;
>
> @@ -134,14 +138,15 @@ static unsigned int imx_lcdc_get_format(unsigned int drm_format)
> }
> }
>
> -static void imx_lcdc_update_hw_registers(struct drm_simple_display_pipe *pipe,
> - struct drm_plane_state *old_state,
> +static void imx_lcdc_update_hw_registers(struct drm_crtc *crtc,
> + struct drm_crtc_state *old_crtc_state,
> + struct drm_crtc_state *new_crtc_state,
> + struct drm_plane_state *new_state,
> bool mode_set)
> {
> - struct drm_crtc *crtc = &pipe->crtc;
> - struct drm_plane_state *new_state = pipe->plane.state;
> + struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(crtc->dev);
> + const struct drm_display_mode *mode = &new_crtc_state->mode;
> struct drm_framebuffer *fb = new_state->fb;
> - struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
> u32 lpcr, lvcr, lhcr;
> u32 framesize;
> dma_addr_t addr;
> @@ -154,24 +159,24 @@ static void imx_lcdc_update_hw_registers(struct drm_simple_display_pipe *pipe,
> return;
>
> /* Disable PER clock to make register write possible */
> - if (old_state && old_state->crtc && old_state->crtc->enabled)
> + if (old_crtc_state && old_crtc_state->enable)
> clk_disable_unprepare(lcdc->clk_per);
>
> /* Framesize */
> - framesize = FIELD_PREP(IMX21LCDC_LSR_XMAX, crtc->mode.hdisplay >> 4) |
> - FIELD_PREP(IMX21LCDC_LSR_YMAX, crtc->mode.vdisplay);
> + framesize = FIELD_PREP(IMX21LCDC_LSR_XMAX, mode->hdisplay >> 4) |
> + FIELD_PREP(IMX21LCDC_LSR_YMAX, mode->vdisplay);
> writel(framesize, lcdc->base + IMX21LCDC_LSR);
>
> /* HSYNC */
> - lhcr = FIELD_PREP(IMX21LCDC_LHCR_HFPORCH, crtc->mode.hsync_start - crtc->mode.hdisplay - 1) |
> - FIELD_PREP(IMX21LCDC_LHCR_HWIDTH, crtc->mode.hsync_end - crtc->mode.hsync_start - 1) |
> - FIELD_PREP(IMX21LCDC_LHCR_HBPORCH, crtc->mode.htotal - crtc->mode.hsync_end - 3);
> + lhcr = FIELD_PREP(IMX21LCDC_LHCR_HFPORCH, mode->hsync_start - mode->hdisplay - 1) |
> + FIELD_PREP(IMX21LCDC_LHCR_HWIDTH, mode->hsync_end - mode->hsync_start - 1) |
> + FIELD_PREP(IMX21LCDC_LHCR_HBPORCH, mode->htotal - mode->hsync_end - 3);
> writel(lhcr, lcdc->base + IMX21LCDC_LHCR);
>
> /* VSYNC */
> - lvcr = FIELD_PREP(IMX21LCDC_LVCR_VFPORCH, crtc->mode.vsync_start - crtc->mode.vdisplay) |
> - FIELD_PREP(IMX21LCDC_LVCR_VWIDTH, crtc->mode.vsync_end - crtc->mode.vsync_start) |
> - FIELD_PREP(IMX21LCDC_LVCR_VBPORCH, crtc->mode.vtotal - crtc->mode.vsync_end);
> + lvcr = FIELD_PREP(IMX21LCDC_LVCR_VFPORCH, mode->vsync_start - mode->vdisplay) |
> + FIELD_PREP(IMX21LCDC_LVCR_VWIDTH, mode->vsync_end - mode->vsync_start) |
> + FIELD_PREP(IMX21LCDC_LVCR_VBPORCH, mode->vtotal - mode->vsync_end);
> writel(lvcr, lcdc->base + IMX21LCDC_LVCR);
>
> lpcr = readl(lcdc->base + IMX21LCDC_LPCR);
> @@ -183,19 +188,20 @@ static void imx_lcdc_update_hw_registers(struct drm_simple_display_pipe *pipe,
> writel(new_state->fb->pitches[0] / 4, lcdc->base + IMX21LCDC_LVPWR);
>
> /* Enable PER clock */
> - if (new_state->crtc->enabled)
> + if (new_crtc_state->enable)
> clk_prepare_enable(lcdc->clk_per);
> }
>
> -static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
> - struct drm_crtc_state *crtc_state,
> - struct drm_plane_state *plane_state)
> +static void imx_lcdc_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *commit)
> {
> int ret;
> int clk_div;
> int bpp;
> - struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
> - struct drm_display_mode *mode = &pipe->crtc.mode;
> + struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(crtc->dev);
> + struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, crtc);
> + struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, &lcdc->plane);
> + struct drm_display_mode *mode = &cstate->mode;
> struct drm_display_info *disp_info = &lcdc->connector->display_info;
> const int hsync_pol = (mode->flags & DRM_MODE_FLAG_PHSYNC) ? 0 : 1;
> const int vsync_pol = (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : 1;
> @@ -206,7 +212,7 @@ static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
>
> clk_div = DIV_ROUND_CLOSEST_ULL(clk_get_rate(lcdc->clk_per),
> mode->clock * 1000);
> - bpp = imx_lcdc_get_format(plane_state->fb->format->format);
> + bpp = imx_lcdc_get_format(pstate->fb->format->format);
>
> writel(FIELD_PREP(IMX21LCDC_LPCR_PCD, clk_div - 1) |
> FIELD_PREP(IMX21LCDC_LPCR_LPPOL, hsync_pol) |
> @@ -230,40 +236,46 @@ static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
>
> ret = clk_prepare_enable(lcdc->clk_ipg);
> if (ret) {
> - dev_err(pipe->crtc.dev->dev, "Cannot enable ipg clock: %pe\n", ERR_PTR(ret));
> + dev_err(crtc->dev->dev, "Cannot enable ipg clock: %pe\n", ERR_PTR(ret));
> return;
> }
> ret = clk_prepare_enable(lcdc->clk_ahb);
> if (ret) {
> - dev_err(pipe->crtc.dev->dev, "Cannot enable ahb clock: %pe\n", ERR_PTR(ret));
> + dev_err(crtc->dev->dev, "Cannot enable ahb clock: %pe\n", ERR_PTR(ret));
>
> clk_disable_unprepare(lcdc->clk_ipg);
>
> return;
> }
>
> - imx_lcdc_update_hw_registers(pipe, NULL, true);
> + imx_lcdc_update_hw_registers(crtc, NULL, cstate, pstate, true);
>
> /* Enable VBLANK Interrupt */
> writel(INTR_EOF, lcdc->base + IMX21LCDC_LIER);
> +
> + drm_crtc_vblank_on(crtc);
> }
>
> -static void imx_lcdc_pipe_disable(struct drm_simple_display_pipe *pipe)
> +static void imx_lcdc_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *commit)
> {
> - struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
> - struct drm_crtc *crtc = &lcdc->pipe.crtc;
> + struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(commit, crtc);
> + struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
> + struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(crtc->dev);
> struct drm_pending_vblank_event *event;
>
> + drm_crtc_vblank_off(crtc);
> +
> clk_disable_unprepare(lcdc->clk_ahb);
> clk_disable_unprepare(lcdc->clk_ipg);
>
> - if (pipe->crtc.enabled)
> + if (old_crtc_state->enable)
> clk_disable_unprepare(lcdc->clk_per);
>
> spin_lock_irq(&lcdc->drm.event_lock);
> - event = crtc->state->event;
> + event = new_crtc_state->event;
> if (event) {
> - crtc->state->event = NULL;
> + new_crtc_state->event = NULL;
> drm_crtc_send_vblank_event(crtc, event);
> }
> spin_unlock_irq(&lcdc->drm.event_lock);
> @@ -272,66 +284,132 @@ static void imx_lcdc_pipe_disable(struct drm_simple_display_pipe *pipe)
> writel(0, lcdc->base + IMX21LCDC_LIER);
> }
>
> -static int imx_lcdc_pipe_check(struct drm_simple_display_pipe *pipe,
> - struct drm_plane_state *plane_state,
> - struct drm_crtc_state *crtc_state)
> +static int imx_lcdc_crtc_helper_atomic_check(struct drm_crtc *crtc,
> + struct drm_atomic_commit *commit)
> {
> + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
> + struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(commit, crtc);
> const struct drm_display_mode *mode = &crtc_state->mode;
> - const struct drm_display_mode *old_mode = &pipe->crtc.state->mode;
> + const struct drm_display_mode *old_mode = &old_crtc_state->mode;
> + int ret;
> +
> + if (crtc_state->enable) {
> + ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
> + if (ret)
> + return ret;
> + }
>
> - if (mode->hdisplay < LCDC_MIN_XRES || mode->hdisplay > LCDC_MAX_XRES ||
> - mode->vdisplay < LCDC_MIN_YRES || mode->vdisplay > LCDC_MAX_YRES ||
> - mode->hdisplay % 0x10) { /* must be multiple of 16 */
> - drm_err(pipe->crtc.dev, "unsupported display mode (%u x %u)\n",
> + if (crtc_state->enable &&
> + (mode->hdisplay < LCDC_MIN_XRES || mode->hdisplay > LCDC_MAX_XRES ||
> + mode->vdisplay < LCDC_MIN_YRES || mode->vdisplay > LCDC_MAX_YRES ||
> + mode->hdisplay % 0x10)) { /* must be multiple of 16 */
> + drm_err(crtc->dev, "unsupported display mode (%u x %u)\n",
> mode->hdisplay, mode->vdisplay);
> return -EINVAL;
> }
>
> - crtc_state->mode_changed =
> - old_mode->hdisplay != mode->hdisplay ||
> - old_mode->vdisplay != mode->vdisplay;
> + if (old_mode->hdisplay != mode->hdisplay ||
> + old_mode->vdisplay != mode->vdisplay)
> + crtc_state->mode_changed = true;
>
> - return 0;
> + return drm_atomic_add_affected_planes(commit, crtc);
> }
>
> -static void imx_lcdc_pipe_update(struct drm_simple_display_pipe *pipe,
> - struct drm_plane_state *old_state)
> +static void imx_lcdc_plane_helper_atomic_update(struct drm_plane *plane,
> + struct drm_atomic_commit *commit)
> {
> - struct drm_crtc *crtc = &pipe->crtc;
> - struct drm_pending_vblank_event *event = crtc->state->event;
> - struct drm_plane_state *new_state = pipe->plane.state;
> + struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(commit, plane);
> + struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(commit, plane);
> + struct drm_crtc *crtc = new_state->crtc;
> + struct drm_crtc_state *old_crtc_state = NULL;
> + struct drm_crtc_state *new_crtc_state;
> struct drm_framebuffer *fb = new_state->fb;
> struct drm_framebuffer *old_fb = old_state->fb;
> struct drm_crtc *old_crtc = old_state->crtc;
> bool mode_changed = false;
>
> + if (!fb || !crtc)
> + return;
> +
> + if (old_crtc)
> + old_crtc_state = drm_atomic_get_old_crtc_state(commit, old_crtc);
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(commit, crtc);
> + if (!new_crtc_state)
> + return;
> +
> if (old_fb && old_fb->format != fb->format)
> mode_changed = true;
> else if (old_crtc != crtc)
> mode_changed = true;
>
> - if (!drm_atomic_crtc_needs_modeset(crtc->state))
> - imx_lcdc_update_hw_registers(pipe, old_state, mode_changed);
> + if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
> + imx_lcdc_update_hw_registers(crtc, old_crtc_state, new_crtc_state,
> + new_state, mode_changed);
> +}
>
> - if (event) {
> - crtc->state->event = NULL;
> +static int imx_lcdc_plane_helper_atomic_check(struct drm_plane *plane,
> + struct drm_atomic_commit *commit)
> +{
> + struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(commit, plane);
> + struct drm_crtc_state *crtc_state = NULL;
>
> - spin_lock_irq(&crtc->dev->event_lock);
> + if (plane_state->crtc) {
> + crtc_state = drm_atomic_get_crtc_state(commit, plane_state->crtc);
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
> + }
>
> - if (crtc->state->active && drm_crtc_vblank_get(crtc) == 0)
> - drm_crtc_arm_vblank_event(crtc, event);
> - else
> - drm_crtc_send_vblank_event(crtc, event);
> + return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> + DRM_PLANE_NO_SCALING,
> + DRM_PLANE_NO_SCALING,
> + false, false);
> +}
>
> - spin_unlock_irq(&crtc->dev->event_lock);
> - }
> +static const struct drm_plane_helper_funcs imx_lcdc_plane_helper_funcs = {
> + .prepare_fb = drm_gem_plane_helper_prepare_fb,
> + .atomic_check = imx_lcdc_plane_helper_atomic_check,
> + .atomic_update = imx_lcdc_plane_helper_atomic_update,
> +};
> +
> +static const struct drm_plane_funcs imx_lcdc_plane_funcs = {
> + .update_plane = drm_atomic_helper_update_plane,
> + .disable_plane = drm_atomic_helper_disable_plane,
> + .destroy = drm_plane_cleanup,
> + .atomic_create_state = drm_atomic_helper_plane_create_state,
> + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> +};
> +
> +static const struct drm_crtc_helper_funcs imx_lcdc_crtc_helper_funcs = {
> + .atomic_check = imx_lcdc_crtc_helper_atomic_check,
> + .atomic_enable = imx_lcdc_crtc_helper_atomic_enable,
> + .atomic_disable = imx_lcdc_crtc_helper_atomic_disable,
> + .atomic_flush = drm_crtc_vblank_atomic_flush,
> +};
> +
> +static int imx_lcdc_crtc_enable_vblank(struct drm_crtc *crtc)
> +{
> + return 0;
> }
>
> -static const struct drm_simple_display_pipe_funcs imx_lcdc_pipe_funcs = {
> - .enable = imx_lcdc_pipe_enable,
> - .disable = imx_lcdc_pipe_disable,
> - .check = imx_lcdc_pipe_check,
> - .update = imx_lcdc_pipe_update,
> +static void imx_lcdc_crtc_disable_vblank(struct drm_crtc *crtc)
> +{
> +}
> +
> +static const struct drm_crtc_funcs imx_lcdc_crtc_funcs = {
> + .atomic_create_state = drm_atomic_helper_crtc_create_state,
> + .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 = imx_lcdc_crtc_enable_vblank,
> + .disable_vblank = imx_lcdc_crtc_disable_vblank,
> +};
> +
> +static const struct drm_encoder_funcs imx_lcdc_encoder_funcs = {
> + .destroy = drm_encoder_cleanup,
> };
>
> static const struct drm_mode_config_funcs imx_lcdc_mode_config_funcs = {
> @@ -369,7 +447,7 @@ MODULE_DEVICE_TABLE(of, imx_lcdc_of_dev_id);
> static irqreturn_t imx_lcdc_irq_handler(int irq, void *arg)
> {
> struct imx_lcdc *lcdc = arg;
> - struct drm_crtc *crtc = &lcdc->pipe.crtc;
> + struct drm_crtc *crtc = &lcdc->crtc;
> unsigned int status;
>
> status = readl(lcdc->base + IMX21LCDC_LISR);
> @@ -387,6 +465,9 @@ static int imx_lcdc_probe(struct platform_device *pdev)
> struct imx_lcdc *lcdc;
> struct drm_device *drm;
> struct drm_bridge *bridge;
> + struct drm_plane *plane;
> + struct drm_crtc *crtc;
> + struct drm_encoder *encoder;
> int irq;
> int ret;
> struct device *dev = &pdev->dev;
> @@ -428,23 +509,40 @@ static int imx_lcdc_probe(struct platform_device *pdev)
> if (ret)
> return dev_err_probe(dev, ret, "Cannot initialize mode configuration structure\n");
>
> - /* CRTC, Plane, Encoder */
> - ret = drm_simple_display_pipe_init(drm, &lcdc->pipe,
> - &imx_lcdc_pipe_funcs,
> - imx_lcdc_formats,
> - ARRAY_SIZE(imx_lcdc_formats), NULL, NULL);
> + plane = &lcdc->plane;
> + ret = drm_universal_plane_init(drm, plane, 0,
> + &imx_lcdc_plane_funcs,
> + imx_lcdc_formats,
> + ARRAY_SIZE(imx_lcdc_formats),
> + NULL,
> + DRM_PLANE_TYPE_PRIMARY, NULL);
> + if (ret < 0)
> + return dev_err_probe(drm->dev, ret, "Cannot initialize primary plane\n");
> + drm_plane_helper_add(plane, &imx_lcdc_plane_helper_funcs);
> +
> + crtc = &lcdc->crtc;
> + ret = drm_crtc_init_with_planes(drm, crtc, plane, NULL,
> + &imx_lcdc_crtc_funcs, NULL);
> + if (ret < 0)
> + return dev_err_probe(drm->dev, ret, "Cannot initialize CRTC\n");
> + drm_crtc_helper_add(crtc, &imx_lcdc_crtc_helper_funcs);
> +
> + encoder = &lcdc->encoder;
> + ret = drm_encoder_init(drm, encoder, &imx_lcdc_encoder_funcs,
> + DRM_MODE_ENCODER_NONE, NULL);
> if (ret < 0)
> - return dev_err_probe(drm->dev, ret, "Cannot setup simple display pipe\n");
> + return dev_err_probe(drm->dev, ret, "Cannot initialize encoder\n");
> + encoder->possible_crtcs = drm_crtc_mask(crtc);
>
> ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
> if (ret < 0)
> return dev_err_probe(drm->dev, ret, "Failed to initialize vblank\n");
>
> - ret = drm_bridge_attach(&lcdc->pipe.encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> + ret = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> if (ret)
> return dev_err_probe(drm->dev, ret, "Cannot attach bridge\n");
>
> - lcdc->connector = drm_bridge_connector_init(drm, &lcdc->pipe.encoder);
> + lcdc->connector = drm_bridge_connector_init(drm, encoder);
> if (IS_ERR(lcdc->connector))
> return dev_err_probe(drm->dev, PTR_ERR(lcdc->connector), "Cannot init bridge connector\n");
>
> @@ -483,7 +581,9 @@ static int imx_lcdc_probe(struct platform_device *pdev)
> drm->mode_config.funcs = &imx_lcdc_mode_config_funcs;
> drm->mode_config.helper_private = &imx_lcdc_mode_config_helpers;
>
> - drm_mode_config_reset(drm);
> + ret = drm_mode_config_create_initial_state(drm);
> + if (ret)
> + return dev_err_probe(dev, ret, "Cannot create initial state\n");
>
> irq = platform_get_irq(pdev, 0);
> if (irq < 0) {
>
--
--
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