Re: [PATCH v7 08/15] drm/imx: dc: Add DPR channel support

[Laurentiu Palcu]

On Mon, Oct 26, 2026 at 06:05:13PM +0800, Liu Ying wrote:
> Display Prefetch Resolve Channel(DPRC) is a part of a prefetch engine.
> It fetches display data, transforms it to linear format and stores it
> to DPRC's RTRAM.  PRG, as the other part of a prefetch engine, acts as
> a gasket between the RTRAM controller and a FetchUnit.  Add a platform
> driver to support the DPRC.
> 
> Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> Acked-by: Maxime Ripard <mripard@xxxxxxxxxx>
> Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> ---
> v7:
> - Collect Maxime's A-b tag.
> - Fix dc_dprc_regmap_write_ranges[].  (Sashiko AI bot)
> - Fix dc_dprc_wrap_irq_handler() return value.  (Sashiko AI bot)
> - Check runtime PM status in dc_dprc_wrap_irq_handler().  (Sashiko AI bot)
> - Fix DPU_DRPC_MAX_STRIDE macro.  (Sashiko AI bot)
> - Support deferral probe if platform_get_irq_byname() fails.  (Sashiko AI bot)
> - Avoid dangling prg->dprc by calling dc_prg_set_dprc() properly.
>   (Sashiko AI bot)
> - Add dc_dprc_get() helper for consumer device to get DPRC properly.
> - Set use_raw_spinlock flag in regmap configuration to avoid a potential
>   hard lockup because dc_dprc_disable_repeat_en() is supposed to be called
>   with local irq and preempt disabled.
> 
> v6:
> - Include "dc-drv.h" to avoid build warning with C=1.
> 
> v3:
> - Call dc_dprc_enable() only when start == true.  (Frank)
> - Collect Frank's R-b tag.
> 
> v2:
> - Manage clocks with bulk interfaces.  (Frank)
> - Sort variables in probe function in reverse Christmas tree fashion.  (Frank)
> ---
>  drivers/gpu/drm/imx/dc/Kconfig   |   1 +
>  drivers/gpu/drm/imx/dc/Makefile  |   6 +-
>  drivers/gpu/drm/imx/dc/dc-dprc.c | 522 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/imx/dc/dc-dprc.h |  37 +++
>  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  |  12 +
>  drivers/gpu/drm/imx/dc/dc-prg.h  |   4 +
>  8 files changed, 581 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig
> index 415993207f2e..507dc9a92d96 100644
> --- a/drivers/gpu/drm/imx/dc/Kconfig
> +++ b/drivers/gpu/drm/imx/dc/Kconfig
> @@ -1,6 +1,7 @@
>  config DRM_IMX8_DC
>  	tristate "Freescale i.MX8 Display Controller Graphics"
>  	depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST)
> +	depends on IMX_SCU
>  	select DRM_CLIENT_SELECTION
>  	select DRM_GEM_DMA_HELPER
>  	select DRM_KMS_HELPER
> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile
> index e3a06ee3ce1a..fd5d62783971 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-prg.o  \
> -		    dc-plane.o dc-tc.o
> +imx8-dc-drm-objs := dc-cf.o dc-crtc.o dc-de.o dc-dprc.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-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-dprc.c b/drivers/gpu/drm/imx/dc/dc-dprc.c
> new file mode 100644
> index 000000000000..a8ddcd117a52
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/dc/dc-dprc.c
> @@ -0,0 +1,522 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2025 NXP
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/firmware/imx/svc/misc.h>
> +#include <linux/interrupt.h>
> +#include <linux/list.h>
> +#include <linux/module.h>

You can remove the include above. You're not using anything from module.h
AFAIS.

[...]

> +
> +bool dc_dprc_rtram_width_supported(struct dc_dprc *dprc, unsigned int width)

You could probably get rid of dprc as well. You're not using it.

> +{
> +	return width <= DPU_DPRC_MAX_RTRAM_WIDTH;
> +}
> +
> +bool dc_dprc_stride_supported(struct dc_dprc *dprc,
> +			      unsigned int stride, unsigned int width,
> +			      const struct drm_format_info *format,
> +			      dma_addr_t baddr)
> +{
> +	unsigned int prg_stride = width * format->cpp[0];
> +
> +	if (stride > DPU_DRPC_MAX_STRIDE)
> +		return false;
> +
> +	if (!dc_prg_stride_supported(dprc->prg, prg_stride, baddr))
> +		return false;
> +
> +	return true;
> +}
> +
> +struct dc_dprc *dc_dprc_get(struct device *dev, struct dc_dprc *dprc)
> +{
> +	struct dc_dprc *iter;
> +
> +	guard(mutex)(&dc_dprc_list_mutex);
> +	list_for_each_entry(iter, &dc_dprc_list, list) {
> +		if (iter == dprc) {
> +			device_link_add(dev, dprc->dev,
> +					DL_FLAG_AUTOREMOVE_CONSUMER);
> +			return dprc;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +static int dc_dprc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct resource *res;
> +	struct dc_dprc *dprc;
> +	void __iomem *base;
> +	int ret, wrap_irq;
> +
> +	dprc = devm_kzalloc(dev, sizeof(*dprc), GFP_KERNEL);
> +	if (!dprc)
> +		return -ENOMEM;
> +
> +	ret = imx_scu_get_handle(&dprc->ipc_handle);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get SCU ipc handle\n");
> +
> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	dprc->reg = devm_regmap_init_mmio(dev, base, &dc_dprc_regmap_config);
> +	if (IS_ERR(dprc->reg))
> +		return PTR_ERR(dprc->reg);
> +
> +	wrap_irq = platform_get_irq_byname(pdev, "dpr_wrap");
> +	if (wrap_irq < 0)
> +		return dev_err_probe(dev, wrap_irq, "failed to get dpr_wrap IRQ\n");
> +
> +	dprc->num_clks = devm_clk_bulk_get_all(dev, &dprc->clks);
> +	if (dprc->num_clks < 0)
> +		return dev_err_probe(dev, dprc->num_clks, "failed to get clocks\n");
> +
> +	ret = of_property_read_u32(np, "fsl,sc-resource", &dprc->sc_resource);
> +	if (ret) {
> +		dev_err(dev, "failed to get SC resource %d\n", ret);
> +		return ret;

You could probably use 'return dev_err_probe()', to be consistent with the
other places where you used the same.

-- 
Thanks,
Laurentiu