Re: [PATCH v7 07/15] drm/imx: dc: Add PRG support

[Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>]

On Mon, Oct 26, 2026 at 06:05:12PM +0800, Liu Ying wrote:
> Display Prefetch Resolve Gasket(PRG) is a part of a FetchUnit's
> prefetch engine.  It sits between a Display Prefetch Resolve
> Channel(DPRC) and a FetchUnit.  Add a platform driver to support
> the PRG.
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Acked-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
> v7:
> - Collect Maxime's A-b tag.
> - Return 128B burst size from dc_prg_burst_size_fixup() when baddr == 0.
>   This avoids undefined behavior of __ffs(0).  (Sashiko AI bot)
> - Initialize and declare prg_node at the same time.  (Sashiko AI bot)
> - Add device link between PRG's consumer and PRG.  (Sashiko AI bot)
> 
> v6:
> - Include "dc-drv.h" to avoid build warning with C=1.
> - Fix PRG BYPASS programming.
> 
> v4:
> - Sort bits and bitfields in descending order for register PRG_CTRL.
> 
> v2:
> - Manage clocks with bulk interfaces.  (Frank)
> - Collect Frank's R-b tag.
> ---
>  drivers/gpu/drm/imx/dc/Makefile |   2 +-
>  drivers/gpu/drm/imx/dc/dc-drv.c |   1 +
>  drivers/gpu/drm/imx/dc/dc-drv.h |   1 +
>  drivers/gpu/drm/imx/dc/dc-prg.c | 314 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/imx/dc/dc-prg.h |  35 +++++
>  5 files changed, 352 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile
> index b9d33c074984..e3a06ee3ce1a 100644
> --- a/drivers/gpu/drm/imx/dc/Makefile
> +++ b/drivers/gpu/drm/imx/dc/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  imx8-dc-drm-objs := dc-cf.o dc-crtc.o dc-de.o dc-drv.o dc-ed.o dc-fg.o dc-fl.o \
> -		    dc-fu.o dc-fw.o dc-ic.o dc-kms.o dc-lb.o dc-pe.o \
> +		    dc-fu.o dc-fw.o dc-ic.o dc-kms.o dc-lb.o dc-pe.o dc-prg.o  \
>  		    dc-plane.o dc-tc.o
>  
>  obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o
> diff --git a/drivers/gpu/drm/imx/dc/dc-drv.c b/drivers/gpu/drm/imx/dc/dc-drv.c
> index 09387d62cc0b..1d01f896e427 100644
> --- a/drivers/gpu/drm/imx/dc/dc-drv.c
> +++ b/drivers/gpu/drm/imx/dc/dc-drv.c
> @@ -351,6 +351,7 @@ static struct platform_driver * const dc_drivers[] = {
>  	&dc_ic_driver,
>  	&dc_lb_driver,
>  	&dc_pe_driver,
> +	&dc_prg_driver,
>  	&dc_tc_driver,
>  	&dc_driver,
>  };
> diff --git a/drivers/gpu/drm/imx/dc/dc-drv.h b/drivers/gpu/drm/imx/dc/dc-drv.h
> index 1ec9304329eb..27ef2c552549 100644
> --- a/drivers/gpu/drm/imx/dc/dc-drv.h
> +++ b/drivers/gpu/drm/imx/dc/dc-drv.h
> @@ -81,6 +81,7 @@ extern struct platform_driver dc_fw_driver;
>  extern struct platform_driver dc_ic_driver;
>  extern struct platform_driver dc_lb_driver;
>  extern struct platform_driver dc_pe_driver;
> +extern struct platform_driver dc_prg_driver;
>  extern struct platform_driver dc_tc_driver;
>  
>  static inline int dc_subdev_get_id(const struct dc_subdev_info *info,
> diff --git a/drivers/gpu/drm/imx/dc/dc-prg.c b/drivers/gpu/drm/imx/dc/dc-prg.c
> new file mode 100644
> index 000000000000..43df5683b24d
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dc/dc-prg.c
> @@ -0,0 +1,314 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2025 NXP
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>

This header can be removed.

> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/module.h>

This one too.

[...]

> +
> +void dc_prg_disable(struct dc_prg *prg)
> +{
> +	regmap_write(prg->reg, PRG_CTRL, BYPASS);
> +
> +	pm_runtime_put(prg->dev);

I have a question about this. I see Sashiko also flagged an issue regarding the
unconditional put when the pm_runtime_resume_and_get() fails in
dc_prg_configure(). The resume is, probably, unlikely to fail. But, I see a
situation where the dc_prg_configure() is never called: when the plane is
invisible. The dc_prg_disable() would still be called in flush() when the plane
is disabled, would it not?

> +}
> +
> +void dc_prg_disable_at_boot(struct dc_prg *prg)
> +{
> +	regmap_write(prg->reg, PRG_CTRL, BYPASS);
> +
> +	clk_bulk_disable_unprepare(prg->num_clks, prg->clks);
> +}
> +
> +static unsigned int dc_prg_burst_size_fixup(dma_addr_t baddr)
> +{
> +	unsigned int burst_size;
> +
> +	if (baddr == 0)
> +		return 128U;
> +
> +	burst_size = 1 << __ffs(baddr);
> +	burst_size = round_up(burst_size, 8);
> +	burst_size = min(burst_size, 128U);
> +
> +	return burst_size;
> +}
> +
> +static unsigned int
> +dc_prg_stride_fixup(unsigned int stride, unsigned int burst_size)
> +{
> +	return round_up(stride, burst_size);
> +}
> +
> +void dc_prg_configure(struct dc_prg *prg,
> +		      unsigned int width, unsigned int height,
> +		      unsigned int stride, unsigned int bits_per_pixel,
> +		      dma_addr_t baddr, bool start)
> +{
> +	struct device *dev = prg->dev;
> +	unsigned int burst_size;
> +	u32 val;
> +	int ret;
> +
> +	if (start) {
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to get RPM: %d\n", ret);
> +			return;
> +		}
> +	}
> +
> +	burst_size = dc_prg_burst_size_fixup(baddr);
> +
> +	stride = dc_prg_stride_fixup(stride, burst_size);
> +
> +	regmap_write(prg->reg, PRG_STRIDE, STRIDE(stride));
> +	regmap_write(prg->reg, PRG_WIDTH, WIDTH(width));
> +	regmap_write(prg->reg, PRG_HEIGHT, HEIGHT(height));
> +	regmap_write(prg->reg, PRG_OFFSET, 0);
> +	regmap_write(prg->reg, PRG_BADDR, baddr);
> +
> +	val = SHADOW_LOAD_MODE | SC_DATA_TYPE_8BIT | HANDSHAKE_MODE_4LINES;
> +
> +	switch (bits_per_pixel) {
> +	case 32:
> +		val |= DES_DATA_TYPE_32BPP;
> +		break;
> +	case 24:
> +		val |= DES_DATA_TYPE_24BPP;
> +		break;
> +	case 16:
> +		val |= DES_DATA_TYPE_16BPP;
> +		break;
> +	case 8:
> +		val |= DES_DATA_TYPE_8BPP;
> +		break;
> +	}
> +
> +	/* no shadow for the first frame */
> +	val |= start ? BYPASS : SHADOW_EN;
> +
> +	regmap_write(prg->reg, PRG_CTRL, val);
> +}
> +
> +void dc_prg_reg_update(struct dc_prg *prg)
> +{
> +	regmap_write(prg->reg, PRG_REG_UPDATE, REG_UPDATE);
> +}
> +
> +void dc_prg_shadow_enable(struct dc_prg *prg)
> +{
> +	regmap_write(prg->reg, PRG_CTRL + SET, SHADOW_EN);
> +}
> +
> +bool dc_prg_stride_supported(struct dc_prg *prg,

It looks like you're not using prg. Can it be removed?

-- 
Thanks,
Laurentiu