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 <[email protected]>
>>>> ---
>>>> 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.
>
>>>
>>> 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.
>
> 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
