Re: [PATCH 5/9] drm/pl111: replace struct drm_simple_display_pipe with regular atomic helpers

[Thomas Zimmermann <tzimmermann@suse.de>]

Am 04.07.26 um 20:31 schrieb Ze Huang:
> Replace PL111 simple display pipe with explicit plane, CRTC and encoder
> objects.
>
> Keep existing hardware programming and vblank behavior, and install GEM
> framebuffer prepare helper explicitly.
>
> Signed-off-by: Ze Huang <ze.huang@oss.qualcomm.com>
> ---
>   drivers/gpu/drm/pl111/pl111_display.c | 174 ++++++++++++++++++++++++++--------
>   drivers/gpu/drm/pl111/pl111_drm.h     |   5 +-
>   drivers/gpu/drm/pl111/pl111_drv.c     |   3 +-
>   3 files changed, 136 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> index 5d10bc5fdf1f..b1bdd4c9dbe6 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c
> @@ -15,6 +15,7 @@
>   #include <linux/media-bus-format.h>
>   #include <linux/of_graph.h>
>   
> +#include <drm/drm_atomic_helper.h>
>   #include <drm/drm_fb_dma_helper.h>
>   #include <drm/drm_fourcc.h>
>   #include <drm/drm_framebuffer.h>
> @@ -37,7 +38,7 @@ irqreturn_t pl111_irq(int irq, void *data)
>   		return IRQ_NONE;
>   
>   	if (irq_stat & CLCD_IRQ_NEXTBASE_UPDATE) {
> -		drm_crtc_handle_vblank(&priv->pipe.crtc);
> +		drm_crtc_handle_vblank(&priv->crtc);
>   
>   		status = IRQ_HANDLED;
>   	}
> @@ -49,10 +50,10 @@ irqreturn_t pl111_irq(int irq, void *data)
>   }
>   
>   static enum drm_mode_status
> -pl111_mode_valid(struct drm_simple_display_pipe *pipe,
> -		 const struct drm_display_mode *mode)
> +pl111_crtc_helper_mode_valid(struct drm_crtc *crtc,
> +			     const struct drm_display_mode *mode)
>   {
> -	struct drm_device *drm = pipe->crtc.dev;
> +	struct drm_device *drm = crtc->dev;
>   	struct pl111_drm_dev_private *priv = drm->dev_private;
>   	u32 cpp = DIV_ROUND_UP(priv->variant->fb_depth, 8);
>   	u64 bw;
> @@ -83,13 +84,35 @@ pl111_mode_valid(struct drm_simple_display_pipe *pipe,
>   	return MODE_OK;
>   }
>   
> -static int pl111_display_check(struct drm_simple_display_pipe *pipe,
> -			       struct drm_plane_state *pstate,
> -			       struct drm_crtc_state *cstate)
> +static int pl111_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;
> +
> +	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);

This is another case of _check_plane_state() running conditionally.

> +	if (ret)
> +		return ret;
> +
> +	if (!pstate->visible)
> +		return 0;
> +
> +	mode = &cstate->mode;
>   
>   	if (mode->hdisplay % 16)
>   		return -EINVAL;
> @@ -117,16 +140,14 @@ static int pl111_display_check(struct drm_simple_display_pipe *pipe,
>   	return 0;
>   }
>   
> -static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
> -				 struct drm_crtc_state *cstate,
> -				 struct drm_plane_state *plane_state)
> +static void pl111_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 pl111_drm_dev_private *priv = drm->dev_private;
> +	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 = priv->plane.state->fb;
>   	struct drm_connector *connector = priv->connector;
>   	struct drm_bridge *bridge = priv->bridge;
>   	bool grayscale = false;
> @@ -355,9 +376,9 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
>   		drm_crtc_vblank_on(crtc);
>   }
>   
> -static void pl111_display_disable(struct drm_simple_display_pipe *pipe)
> +static void pl111_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 pl111_drm_dev_private *priv = drm->dev_private;
>   	u32 cntl;
> @@ -387,17 +408,23 @@ static void pl111_display_disable(struct drm_simple_display_pipe *pipe)
>   	clk_disable_unprepare(priv->clk);
>   }
>   
> -static void pl111_display_update(struct drm_simple_display_pipe *pipe,
> -				 struct drm_plane_state *old_pstate)
> +static void pl111_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 pl111_drm_dev_private *priv = drm->dev_private;
> -	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 pl111_drm_dev_private *priv;
> +	struct drm_pending_vblank_event *event;
>   	struct drm_plane_state *pstate = plane->state;
>   	struct drm_framebuffer *fb = pstate->fb;
>   
> +	if (!crtc)
> +		return;
> +
> +	drm = crtc->dev;
> +	priv = drm->dev_private;
> +	event = crtc->state->event;
> +
>   	if (fb) {
>   		u32 addr = drm_fb_dma_get_gem_addr(fb, pstate, 0);
>   
> @@ -416,9 +443,8 @@ static void pl111_display_update(struct drm_simple_display_pipe *pipe,
>   	}
>   }
>   
> -static int pl111_display_enable_vblank(struct drm_simple_display_pipe *pipe)
> +static int pl111_display_enable_vblank(struct drm_crtc *crtc)
>   {
> -	struct drm_crtc *crtc = &pipe->crtc;
>   	struct drm_device *drm = crtc->dev;
>   	struct pl111_drm_dev_private *priv = drm->dev_private;
>   
> @@ -427,21 +453,63 @@ static int pl111_display_enable_vblank(struct drm_simple_display_pipe *pipe)
>   	return 0;
>   }
>   
> -static void pl111_display_disable_vblank(struct drm_simple_display_pipe *pipe)
> +static void pl111_display_disable_vblank(struct drm_crtc *crtc)
>   {
> -	struct drm_crtc *crtc = &pipe->crtc;
>   	struct drm_device *drm = crtc->dev;
>   	struct pl111_drm_dev_private *priv = drm->dev_private;
>   
>   	writel(0, priv->regs + priv->ienb);
>   }
>   
> -static struct drm_simple_display_pipe_funcs pl111_display_funcs = {
> -	.mode_valid = pl111_mode_valid,
> -	.check = pl111_display_check,
> -	.enable = pl111_display_enable,
> -	.disable = pl111_display_disable,
> -	.update = pl111_display_update,
> +static int pl111_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 struct drm_crtc_funcs pl111_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_crtc_helper_funcs pl111_crtc_helper_funcs = {
> +	.mode_valid	= pl111_crtc_helper_mode_valid,
> +	.atomic_check	= pl111_crtc_helper_atomic_check,
> +	.atomic_enable	= pl111_crtc_helper_atomic_enable,
> +	.atomic_disable	= pl111_crtc_helper_atomic_disable,
> +};
> +
> +static const struct drm_plane_funcs pl111_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 pl111_plane_helper_funcs = {
> +	.prepare_fb	= drm_gem_plane_helper_prepare_fb,
> +	.atomic_check	= pl111_plane_helper_atomic_check,
> +	.atomic_update	= pl111_plane_helper_atomic_update,
> +};
> +
> +static const struct drm_encoder_funcs pl111_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
>   };
>   
>   static int pl111_clk_div_choose_div(struct clk_hw *hw, unsigned long rate,
> @@ -583,18 +651,40 @@ int pl111_display_init(struct drm_device *drm)
>   		return ret;
>   
>   	if (!priv->variant->broken_vblank) {
> -		pl111_display_funcs.enable_vblank = pl111_display_enable_vblank;
> -		pl111_display_funcs.disable_vblank = pl111_display_disable_vblank;
> +		pl111_crtc_funcs.enable_vblank = pl111_display_enable_vblank;
> +		pl111_crtc_funcs.disable_vblank = pl111_display_disable_vblank;
>   	}
>   
> -	ret = drm_simple_display_pipe_init(drm, &priv->pipe,
> -					   &pl111_display_funcs,
> -					   priv->variant->formats,
> -					   priv->variant->nformats,
> -					   NULL,
> -					   priv->connector);
> +	ret = drm_universal_plane_init(drm, &priv->plane, 0,
> +				       &pl111_plane_funcs,
> +				       priv->variant->formats,
> +				       priv->variant->nformats,
> +				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>   	if (ret)
>   		return ret;
>   
> +	drm_plane_helper_add(&priv->plane, &pl111_plane_helper_funcs);
> +
> +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->plane,
> +					NULL, &pl111_crtc_funcs, NULL);
> +	if (ret)
> +		return ret;
> +
> +	drm_crtc_helper_add(&priv->crtc, &pl111_crtc_helper_funcs);
> +
> +	ret = drm_encoder_init(drm, &priv->encoder, &pl111_encoder_funcs,
> +			       DRM_MODE_ENCODER_NONE, NULL);
> +	if (ret)
> +		return ret;
> +
> +	priv->encoder.possible_crtcs = drm_crtc_mask(&priv->crtc);
> +
> +	if (priv->connector) {
> +		ret = drm_connector_attach_encoder(priv->connector,
> +						   &priv->encoder);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/pl111/pl111_drm.h b/drivers/gpu/drm/pl111/pl111_drm.h
> index d1fe756444ee..ec92a5a180a8 100644
> --- a/drivers/gpu/drm/pl111/pl111_drm.h
> +++ b/drivers/gpu/drm/pl111/pl111_drm.h
> @@ -21,7 +21,6 @@
>   #include <drm/drm_encoder.h>
>   #include <drm/drm_gem.h>
>   #include <drm/drm_panel.h>
> -#include <drm/drm_simple_kms_helper.h>
>   
>   /*
>    * CLCD Controller Internal Register addresses
> @@ -135,7 +134,9 @@ struct pl111_drm_dev_private {
>   	struct drm_connector *connector;
>   	struct drm_panel *panel;
>   	struct drm_bridge *bridge;
> -	struct drm_simple_display_pipe pipe;
> +	struct drm_plane plane;
> +	struct drm_crtc crtc;
> +	struct drm_encoder encoder;
>   
>   	void *regs;
>   	u32 memory_bw;
> diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
> index ac7b1d12a0f5..f649c266c33a 100644
> --- a/drivers/gpu/drm/pl111/pl111_drv.c
> +++ b/drivers/gpu/drm/pl111/pl111_drv.c
> @@ -168,8 +168,7 @@ static int pl111_modeset_init(struct drm_device *dev)
>   		goto out_bridge;
>   	}
>   
> -	ret = drm_simple_display_pipe_attach_bridge(&priv->pipe,
> -						    bridge);
> +	ret = drm_bridge_attach(&priv->encoder, bridge, NULL, 0);
>   	if (ret)
>   		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)