Message-ID: <3051b1a2-a6da-4b1a-88ac-f27da47a82e9@nxp.com>
Date: Thu, 25 Sep 2025 17:09:24 +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/24/2025, Frank Li wrote:
> On Thu, Sep 25, 2025 at 10:58:27AM +0800, Liu Ying wrote:
>> On 09/24/2025, Frank Li wrote:
>>> On Wed, Sep 24, 2025 at 02:41:50PM +0800, Liu Ying wrote:
>>>> 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.
>>>>
>>>
>>> Okay,
>>>
>>> dc_dprc_enable()
>>> {
>>> 	...
>>> 	pm_runtime_resume_and_get();
>>> }
>>>
>>> dc_dpdr_disable()
>>> {
>>> 	pm_runtime_put();
>>> }
>>>
>>> dc_dprc_configure()
>>> {
>>> 	pm_runtime_resume_and_get();
>>>
>>> 	if (start)
>>> 		dc_dprc_enable(dprc);
>>>
>>> 	pm_runtime_put();
>>> }
>>>
>>> Look more reasonable for pair get()/put().  after first start, ref count
>>> will not reduce 0 by pm_runtime_put();.
>>
>> Then, as dc_dprc_disable_repeat_en() also accesses DPRC register, it needs
>> the RPM get/put too?  Same for PRG driver APIs, dc_prg_{enable,disable,
>> configure,reg_update,shadow_enable} access PRG registers.  Though adding
>> RPM get/put to all of them should work, I don't see much point in having
>> the get/put dance.
> 
> I don't think need change all.
> 
> you call dc_dprc_configure(start = true) to get pm_runtime_resume_and_get()
> 
> call dc_dpdr_disable() to put run time pm.
> 
> Just change to
> 1. call pm_runtime_resume_and_get() in dc_dprc_enable(), which call only when
>  (start == true)
> 2. call pm_runtime_put() in dc_dpdr_disable() /* you already did it */
> 
> get()/put() pair in dc_dprc_configure() to make sure access register
> before call dc_dprc_enable().
> 
> The whole logic should be the same as what your current code.

I got your whole idea and thought it should work, but my point was that
aside from dc_dprc_configure(), dc_dprc_disable_repeat_en() and those
PRG driver APIs access registers too, so if dc_dprc_configure() needs the
RPM get/put, then all of them need too, i.e. dc_dprc_configure() is not
special.  To make the RPM simpler, I think it makes sense to have
dc_{dprc,prg}_configure() get RPM when start == true and have
dc_{dprc,prg}_disable() put RPM, just like patch 6&7 do.

> 
>>
>>>
>>>>>
>>>>> 	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.
>>>
>>> guard only protect after it.
>>
>> scoped_guard() called by dc_dprc_wrap_irq_handler() protects register access
>> too.
> 
> Sorry, I missed this part. I found at original patch.
> 
> Frank Li
> 
>>
>>>
>>> in dc_dprc_runtime_resume()
>>>
>>> +	/* disable all control IRQs and enable all error IRQs */
>>> +	guard(spinlock_irqsave)(&dprc->lock);
>>> +	regmap_write(dprc->reg, IRQ_MASK, IRQ_CTRL_MASK);
>>> +
>>> +	return 0;
>>>
>>> +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);
>>> +}
>>>
>>> How spin lock protect register access?
>>
>> With the error and control IRQs, dc_dprc_wrap_irq_handler() and
>> dc_dprc_enable_ctrl_done_irq() could have concurrent access to IRQ_MASK
>> registers(though there is SET/CLR/TOG register variants, they have a
>> shared read-out value).
>>
>>>
>>> 1: IRQ_MASK <= IRQ_CTRL_MASK;
>>> 2: IRQ_MASK + CLR <= IRQ_DPR_CRTL_DONE;
>>>
>>> 2 possilbe result:
>>> 	1 happen after 2
>>> 	2 happen after 1
>>>
>>> Frank
>>>
>>>>
>>>>>
>>>>> 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
>>
>>
>> --
>> Regards,
>> Liu Ying


-- 
Regards,
Liu Ying

Powered by blists - more mailing lists