Message-ID: <eb070dbc-1e8e-437a-b519-69709b3feae4@nxp.com>
Date: Wed, 24 Sep 2025 14:41:50 +0800
From: Liu Ying <victor.liu@....com>
To: Frank Li <Frank.li@....com>
Cc: Philipp Zabel <p.zabel@...gutronix.de>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, Dmitry Baryshkov <lumag@...nel.org>,
 dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 07/14] drm/imx: dc: Add DPR channel support

On 09/23/2025, Frank Li wrote:
> On Tue, Sep 23, 2025 at 10:07:57AM +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.
>>
>> Signed-off-by: Liu Ying <victor.liu@....com>
>> ---
>> 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 | 465 +++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/imx/dc/dc-dprc.h |  35 +++
>>  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, 522 insertions(+), 3 deletions(-)
>>
> ...
>> +
>> +static void dc_dprc_reset(struct dc_dprc *dprc)
>> +{
>> +	regmap_write(dprc->reg, SYSTEM_CTRL0 + SET, SOFT_RESET);
>> +	fsleep(20);
>> +	regmap_write(dprc->reg, SYSTEM_CTRL0 + CLR, SOFT_RESET);
>> +	fsleep(20);
>> +}
>> +
>> +static void dc_dprc_enable(struct dc_dprc *dprc)
>> +{
>> +	dc_prg_enable(dprc->prg);
>> +}
>> +
>> +static void dc_dprc_reg_update(struct dc_dprc *dprc)
>> +{
>> +	dc_prg_reg_update(dprc->prg);
>> +}
>> +
>> +static void dc_dprc_enable_ctrl_done_irq(struct dc_dprc *dprc)
>> +{
>> +	guard(spinlock_irqsave)(&dprc->lock);
>> +	regmap_write(dprc->reg, IRQ_MASK + CLR, IRQ_DPR_CRTL_DONE);
>> +}
>> +
>> +void dc_dprc_configure(struct dc_dprc *dprc, unsigned int stream_id,
>> +		       unsigned int width, unsigned int height,
>> +		       unsigned int stride,
>> +		       const struct drm_format_info *format,
>> +		       dma_addr_t baddr, bool start)
>> +{
>> +	unsigned int prg_stride = width * format->cpp[0];
>> +	unsigned int bpp = format->cpp[0] * 8;
>> +	struct device *dev = dprc->dev;
>> +	unsigned int p1_w, p1_h;
>> +	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;
>> +		}
>> +
>> +		dc_dprc_set_stream_id(dprc, stream_id);
>> +	}
>> +
>> +	p1_w = round_up(width, format->cpp[0] == 2 ? 32 : 16);
>> +	p1_h = round_up(height, 4);
>> +
>> +	regmap_write(dprc->reg, FRAME_CTRL0, PITCH(stride));
>> +	regmap_write(dprc->reg, FRAME_1P_CTRL0, BYTE_1K);
>> +	regmap_write(dprc->reg, FRAME_1P_PIX_X_CTRL, NUM_X_PIX_WIDE(p1_w));
>> +	regmap_write(dprc->reg, FRAME_1P_PIX_Y_CTRL, NUM_Y_PIX_HIGH(p1_h));
>> +	regmap_write(dprc->reg, FRAME_1P_BASE_ADDR_CTRL0, baddr);
>> +	regmap_write(dprc->reg, FRAME_PIX_X_ULC_CTRL, CROP_ULC_X(0));
>> +	regmap_write(dprc->reg, FRAME_PIX_Y_ULC_CTRL, CROP_ULC_Y(0));
>> +
>> +	regmap_write(dprc->reg, RTRAM_CTRL0, THRES_LOW(3) | THRES_HIGH(7));
> 
> Is it okay to access register if start is false since
> pm_runtime_resume_and_get() have not called.

Yes, it is okay, because dc_dprc_configure() is supposed to be called
continously for multiple times(OFC, fine for only once as well).  For
the first time, start is true in order to enable the DPRC.  After the
first time(DPRC is running), it is called with start == false to do
things like page-flip(update frame buffer address).

> 
>> +
>> +	val = LINE4 | BUF2;
>> +	switch (format->format) {
>> +	case DRM_FORMAT_XRGB8888:
>> +		/*
>> +		 * It turns out pixel components are mapped directly
>> +		 * without position change via DPR processing with
>> +		 * the following color component configurations.
>> +		 * Leave the pixel format to be handled by the
>> +		 * display controllers.
>> +		 */
>> +		val |= A_COMP_SEL(3) | R_COMP_SEL(2) |
>> +		       G_COMP_SEL(1) | B_COMP_SEL(0);
>> +		val |= PIX_SIZE_32BIT;
>> +		break;
>> +	default:
>> +		dev_err(dev, "unsupported format 0x%08x\n", format->format);
>> +		return;
>> +	}
>> +	regmap_write(dprc->reg, MODE_CTRL0, val);
>> +
>> +	if (start) {
>> +		/* software shadow load for the first frame */
>> +		val = SW_SHADOW_LOAD_SEL | SHADOW_LOAD_EN;
>> +		regmap_write(dprc->reg, SYSTEM_CTRL0, val);
>> +
>> +		/* and then, run... */
>> +		val |= RUN_EN | REPEAT_EN;
>> +		regmap_write(dprc->reg, SYSTEM_CTRL0, val);
>> +	}
>> +
>> +	dc_prg_configure(dprc->prg, width, height, prg_stride, bpp, baddr, start);
>> +
>> +	dc_dprc_enable(dprc);
>> +
>> +	dc_dprc_reg_update(dprc);
>> +
>> +	if (start)
>> +		dc_dprc_enable_ctrl_done_irq(dprc);
>> +
>> +	dev_dbg(dev, "w: %u, h: %u, s: %u, fmt: 0x%08x\n",
>> +		width, height, stride, format->format);
>> +}
>> +
>> +void dc_dprc_disable_repeat_en(struct dc_dprc *dprc)
>> +{
>> +	regmap_write(dprc->reg, SYSTEM_CTRL0 + CLR, REPEAT_EN);
>> +	dev_dbg(dprc->dev, "disable REPEAT_EN\n");
>> +}
>> +
>> +void dc_dprc_disable(struct dc_dprc *dprc)
>> +{
>> +	dc_prg_disable(dprc->prg);
>> +
>> +	pm_runtime_put(dprc->dev);
> 
> You call pm_runtime_put() in dc_dprc_disable(), but not call
> pm_runtime_resume_and_get() at dc_dprc_enable().

Yes, dc_dprc_configure()(start == true) is designed to get RPM and
dc_dprc_disable() to put RPM.

dc_dprc_enable() just sets PRG to non-bypass mode.

> 
> Is it more reasonable to call pm_runtime_resume_and_get() in dc_dprc_enable()
> 
> dc_dprc_enable()
> {
> 	...
> 	pm_runtime_resume_and_get();
> }
> 
> dc_dprc_configure()
> {
> 	unconditional call
> 	pm_runtime_resume_and_get()
> 	...
> 	pm_runtime_put()

Here, as RPM is put, it's possible to actually disable the power domain,
hence possibly lose all the DPRC configuration done between RPM get and
RPM put.  So, this doesn't make sense.

> 
> 	if (start) //look like only need enable when start is true

I may add this check in next version.

> 		dc_dprc_enable(dprc);
> }
> 
>> +
>> +	dev_dbg(dprc->dev, "disable\n");
>> +}
>> +
>> +void dc_dprc_disable_at_boot(struct dc_dprc *dprc)
>> +{
>> +	dc_prg_disable_at_boot(dprc->prg);
>> +
>> +	clk_bulk_disable_unprepare(dprc->num_clks, dprc->clks);
>> +
> 
> you have runtime functions dc_dprc_runtime_suspend()
> 
> If runtime pm status is correct, needn't call clk_bulk_disable_unprepare().
> 
> Look like call pm_runtime_put() here to let runtime pm management clks.
> 
> otherwise, runtime pm state will not match clock enable/disable state.
> 
>> +	dev_dbg(dprc->dev, "disable at boot\n");
>> +}
>> +
>> +static void dc_dprc_ctrl_done_handle(struct dc_dprc *dprc)
>> +{
>> +	regmap_write(dprc->reg, SYSTEM_CTRL0, REPEAT_EN);
>> +
>> +	dc_prg_shadow_enable(dprc->prg);
>> +
>> +	dev_dbg(dprc->dev, "CTRL done handle\n");
>> +}
>> +
> ...
>> +
>> +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 -ENODEV;
>> +
>> +	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;
>> +	}
>> +
>> +	dprc->prg = dc_prg_lookup_by_phandle(dev, "fsl,prgs", 0);
>> +	if (!dprc->prg)
>> +		return dev_err_probe(dev, -EPROBE_DEFER,
>> +				     "failed to lookup PRG\n");
>> +
>> +	dc_prg_set_dprc(dprc->prg, dprc);
>> +
>> +	dprc->dev = dev;
>> +	spin_lock_init(&dprc->lock);
>> +
>> +	ret = devm_request_irq(dev, wrap_irq, dc_dprc_wrap_irq_handler,
>> +			       IRQF_SHARED, dev_name(dev), dprc);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to request dpr_wrap IRQ(%d): %d\n",
>> +			wrap_irq, ret);
>> +		return ret;
>> +	}
>> +
>> +	dev_set_drvdata(dev, dprc);
>> +
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to enable PM runtime\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static int dc_dprc_runtime_suspend(struct device *dev)
>> +{
>> +	struct dc_dprc *dprc = dev_get_drvdata(dev);
>> +
>> +	clk_bulk_disable_unprepare(dprc->num_clks, dprc->clks);
>> +
>> +	return 0;
>> +}
>> +
>> +static int dc_dprc_runtime_resume(struct device *dev)
>> +{
>> +	struct dc_dprc *dprc = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	ret = clk_bulk_prepare_enable(dprc->num_clks, dprc->clks);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable clocks: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	dc_dprc_reset(dprc);
>> +
>> +	/* disable all control IRQs and enable all error IRQs */
>> +	guard(spinlock_irqsave)(&dprc->lock);
>> +	regmap_write(dprc->reg, IRQ_MASK, IRQ_CTRL_MASK);
> 
> write one 32bit register is atomic, look like needn't spinlock.
> 
> Only other place use dprc->lock is in dc_dprc_enable_ctrl_done_irq(), which
> write 32bit clr register.

No, dc_dprc_wrap_irq_handler() uses the lock to protect register access too,
so it's needed.

> 
> Frank
>> +
>> +	return 0;
>> +}
>> +
> ...
>> +void dc_prg_set_dprc(struct dc_prg *prg, struct dc_dprc *dprc)
>> +{
>> +	prg->dprc = dprc;
>> +}
>> +
>> +struct dc_dprc *dc_prg_get_dprc(struct dc_prg *prg)
>> +{
>> +	return prg->dprc;
>> +}
>> +
>>  static int dc_prg_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> diff --git a/drivers/gpu/drm/imx/dc/dc-prg.h b/drivers/gpu/drm/imx/dc/dc-prg.h
>> index 6fd9b050bfa12334720f83ff9ceaf337e3048a54..f29d154f7de597b9d20d5e71303049f6f8b022d6 100644
>> --- a/drivers/gpu/drm/imx/dc/dc-prg.h
>> +++ b/drivers/gpu/drm/imx/dc/dc-prg.h
>> @@ -32,4 +32,8 @@ bool dc_prg_stride_supported(struct dc_prg *prg,
>>  struct dc_prg *
>>  dc_prg_lookup_by_phandle(struct device *dev, const char *name, int index);
>>
>> +void dc_prg_set_dprc(struct dc_prg *prg, struct dc_dprc *dprc);
>> +
>> +struct dc_dprc *dc_prg_get_dprc(struct dc_prg *prg);
>> +
>>  #endif
>>
>> --
>> 2.34.1
>>


-- 
Regards,
Liu Ying

Powered by blists - more mailing lists