Message-ID: <6f112e86-9a16-4a36-912a-d295abc4647b@nxp.com>
Date: Tue, 3 Feb 2026 17:54:18 +0800
From: Liu Ying <victor.liu@....com>
To: Marek Vasut <marek.vasut@...lbox.org>, Liu Ying <victor.liu@....nxp.com>,
 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>
Cc: dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, Frank Li <Frank.Li@....com>
Subject: Re: [PATCH v4 07/14] drm/imx: dc: Add DPR channel support

On Tue, Jan 06, 2026 at 02:36:18AM +0100, Marek Vasut wrote:
> On 11/14/25 11:03 AM, Liu Ying wrote:
> 
> Hello Liu,

Hello Marek,

> 
> sorry for my late reply.
> 
>>>>>>>> +++ 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
>>>>>>> Can the SCU dependency be made optional,
>>>>>>
>>>>>> I don't think this can be done.  If you grep 'depends on IMX_SCU' in
>>>>>> kernel, you may find a handful of existing dependancies.
>>>>>
>>>>> Sure, I do not dispute this part.
>>>>>
>>>>> But the SCU dependency can be contained in a component of this driver,
>>>>> which is not used by MX95, and used only by MX8Q . Then there will be
>>>>> no problem.
>>>>
>>>> Which component?  You mean PRG and DPRC?
>>>>
>>>> If we add something like CONFIG_DRM_IMX8_DC_PRG and make CONFIG_DRM_IMX8_DC_PRG
>>>> depend on SCU, then should we make CONFIG_DRM_IMX8_DC depend on
>>>> CONFIG_DRM_IMX8_DC_PRG?  That's yet another dependency.
>>>
>>> I would say, if possible, put the SCU-dependent parts behind
>>> CONFIG_DRM_IMX8_DC_PRG symbol, and make that symbol configurable via
>>> Kconfig . Users of iMX95-only can turn it off, generic kernel config
>>> should keep it on.
>>
>> Both i.MX8 and i.MX95 would use arm64 defconfig.  Most distros if not all
>> would use that to generate a single kernel image + modules for i.MX8 and
>> i.MX95.  We can't presume that __most__ users would change the configs
>> for i.MX95.
> 
> That wasn't my point. My point was, make it possible to compile the PRG
> parts out if they aren't needed (and with them, SCU support). If I build
> the kernel for iMX95, I don't need iMX8Q support in, so I can turn MX8Q
> support off alongside the SCU parts. The clean way to do that is to keep
> the SCU-dependent parts in dedicated file(s), which may or may not be
> compiled in. For generic kernel builds, they should be compiled in and
> decide at runtime, whether or not they should be used of course.

A concern is that this allows users to disable PRG and DPRC for i.MX8QXP
by configuring something like CONFIG_DRM_IMX8_DC_PREFETCH=n, which means
the driver needs to support 2 cases where either the prefetch engines are
used or bypassed.  I wouldn't say it's not doable, but it makes the driver
a lot more complicated due to different control flows w/wo the prefetch
engine.  Perhaps, one day we'll support the bypass case when we have to,
like, add multiple fractional layers support or warping support.  But,
for now, I think it makes sense to start small and treat the prefetch
engines as always being present/available.

Also, when the bypass case comes to i.MX8QXP blit engine, we need to
somehow tell userspace about the prefetch engines' presence/availability
by UAPI design which supposes to be as simple as possible, not the other
way around...

I'd suggest to add the bypass case support over time in the future if needed:
1) As of now, always use prefetch engines.
2) If needed, let the driver decide to bypass the always presenting prefetch
   engines if it has to.
3) Add config like CONFIG_DRM_IMX8_DC_PREFETCH to allow user to disable
   prefetch engines, which sounds like a bit over-engineering to me though.

> 
> [...]
> 
>>>>>> Like I replied to your i.MX95 DC patch series's cover letter, SCU accesses
>>>>>> registers via Cortex-M core instead of Cortex-A core IIUC.  I really don't
>>>>>> know how to abstract IMX_SCU out, especially via regmap.
>>>>>
>>>>> The simplest way would be to use regmap_config .reg_read and .reg_write ,
>>>>> if there is no better way.
>>>>
>>>> Can you shed more light on this?  Any examples?
>>>
>>> I'll just reply to this part, because that is probably the most relevant
>>> to this whole conversation.
>>>
>>> git grep '\.reg_write' drivers -> drivers/hwmon/aspeed-pwm-tacho.c as a
>>> simple example.
>>>
>>> Then such a reg_write implementation can do:
>>>
>>> if (SCU)
>>>    use SCU accessor
>>> else
>>>    use writel() or so
>>
>> Thanks for the example, it makes regmap idea a bit more clear.  But, how
>> would you pass the SCU flag to .reg_write?  I hope i.MX95 code path won't
>> see any information about SCU.
> 
> You could have one regmap for MX8Q and one regmap for MX95. For the register
> IO in the driver which uses regmap, either regmap is still only a generic
> regmap, no matter what the hardware accessor in that regmap are. Therefore
> most of the code can be generic (use generic regmap) and only the probe code
> decides which regmap (MX8Q SCU regmap or MX95 regmap) is going to be used by
> the driver.

I'm a bit confused.

Considering below 3 points, how would you design regmap(s)?
1) i.MX95 display controller only has MMIO registers.
2) i.MX8QXP display controller only has MMIO registers.
3) i.MX8QXP prefetch engine(DPRC + PRG) has MMIO registers and needs SCU
   accessor(Cortex-M core) to control registers in SCU.

> 
>> Also, IMO, wrapping SCU with regmap is abusing the regmap API.
>> How would you pass the resource, ctrl and val parameters to .reg_write?
>>
>> int imx_sc_misc_set_control(struct imx_sc_ipc *ipc, u32 resource,
>>                              u8 ctrl, u32 val)
>>
>> Regarding the 'else' clause, I don't think we can use writel(), because
>> SCU accesses registers via Cortex-M core, not Cortex-A core(I mentioned
>> this before).  I don't see anything we can put under the 'else' clause.
> 
> You could have one regmap definition for MX8Q/SCU and one for MX95, and
> at probe time, select which of those is going to be used. That could work
> I think ?

Sorry, I don't understand the idea of "one regmap definition for MX8Q/SCU
and one for MX95".

Also, if the idea is ok, care to anwer my previous question about "How would
you pass the resource, ctrl and val parameters to .reg_write"?

int imx_sc_misc_set_control(struct imx_sc_ipc *ipc, u32 resource,
			    u8 ctrl, u32 val)

> 
>>>>>>> so iMX95 support can be added into the driver easily too ?
>>>>>>
>>>>>> Like I replied to your i.MX95 DC patch series, I think i.MX95 DC support
>>>>>> can be in drivers/gpu/drm/imx/dc, but it should be in a separate module
>>>>>> (something like imx95_dc_drm) plus an additional common module(like
>>>>>> imx_dc_drm_common).
>>>>> This design part is something I do not fully understand. Sure, it can be
>>>>> two modules, but in the end, the result would have to be capable of being
>>>>> compiled into single kernel binary if both modules would be =y in Kconfig
>>>>> anyway.
>>>>
>>>> This is something like imx8qm_ldb, imx8qxp_ldb and imx_ldb_helper modules -
>>>> DRM_IMX8QM_LDB and DRM_IMX8QXP_LDB select DRM_IMX_LDB_HELPER.
>>>>
>>>> Note you may make CONFIG_DRM_IMX8QM_LDB=y and CONFIG_DRM_IMX8QXP_LDB=m with
>>>> CONFIG_DRM_IMX_LDB_HELPER=y.
>>> Do we have to make it this complicated right from the start ?
>>
>> I don't think imx_dc_drm_common would make it complicated, but instead it
>> makes drivers simple and easy to maintain.
>>
>>> Maybe we can start simple, with one module, and then split it up if it turns
>>> out to be unsuitable ?
>>
>> Hmm, maybe it's time to split it up now, because:
>> 1) Your below patch has already been adding DB to CRTC function.
>>     See dc_db_shdtokgen() in dc_crtc_atomic_enable(), which certainly breaks
>>     i.MX8 DC.  To avoid that, you'll add branches to the atomic enable
>>     callback, which is something I don't want.
> 
> Can we have separate atomic_enable() callback for MX8Q and MX95 for example ?

Technically, yes.  But it doesn't worth complicating the single imx8-dc
driver, considering that the callcack can be in a separate driver and the
code for supporting separate DRM drivers is fairly limited.

> 
>> https://patchwork.freedesktop.org/patch/680085/?series=155784&rev=1
>>
>> 2) This patch series is trying to add i.MX8 DC prefetch engine support.
>>     i.MX95 really doesn't embed the prefetch engine.  And I hope i.MX95 code
>>     path doesn't need to know the prefetch engine/SCU existence or non-existence.
> I think the prefetch code can be contained in a single file and called only
> if the SoC is MX8Q, that shouldn't be difficult to do ?

Considering the prefetch engine bypass case for i.MX8QXP and keeping the
single imx8-dc driver as simple as possilbe, I'd say I'm not a fan of that.

-- 
Regards,
Liu Ying

Powered by blists - more mailing lists