* [PATCH v2 01/14] dt-bindings: display: imx: Add i.MX8qxp/qm PRG binding
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
@ 2025-09-23 2:07 ` Liu Ying
2025-09-23 2:07 ` [PATCH v2 02/14] dt-bindings: display: imx: Add i.MX8qxp/qm DPR channel binding Liu Ying
` (12 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:07 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying
Add bindings for i.MX8qxp/qm Display Prefetch Resolve Gasket.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
Changed commit message to use imperative mood.
---
.../bindings/display/imx/fsl,imx8qxp-prg.yaml | 60 ++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-prg.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-prg.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..3ff46e0d4e73488f8a3ed538830570b1ec71b50c
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-prg.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/imx/fsl,imx8qxp-prg.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale i.MX8qm/qxp Display Prefetch Resolve Gasket
+
+maintainers:
+ - Liu Ying <victor.liu@nxp.com>
+
+description: |
+ The i.MX8qm/qxp Prefetch Resolve Gasket (PRG) is a gasket interface between
+ RTRAM controller and Display Controller. The main function is to convert
+ the AXI interface to the RTRAM interface, which includes re-mapping the
+ ARADDR to a RTRAM address.
+
+properties:
+ compatible:
+ enum:
+ - fsl,imx8qxp-prg
+ - fsl,imx8qm-prg
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: rtram clock
+ - description: apb clock
+
+ clock-names:
+ items:
+ - const: rtram
+ - const: apb
+
+ power-domains:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - power-domains
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/imx8-lpcg.h>
+ #include <dt-bindings/firmware/imx/rsrc.h>
+ prg@56040000 {
+ compatible = "fsl,imx8qxp-prg";
+ reg = <0x56040000 0x10000>;
+ clocks = <&dc0_prg0_lpcg IMX_LPCG_CLK_0>,
+ <&dc0_prg0_lpcg IMX_LPCG_CLK_4>;
+ clock-names = "rtram", "apb";
+ power-domains = <&pd IMX_SC_R_DC_0>;
+ };
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 02/14] dt-bindings: display: imx: Add i.MX8qxp/qm DPR channel binding
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
2025-09-23 2:07 ` [PATCH v2 01/14] dt-bindings: display: imx: Add i.MX8qxp/qm PRG binding Liu Ying
@ 2025-09-23 2:07 ` Liu Ying
2025-09-23 2:07 ` [PATCH v2 03/14] MAINTAINERS: Add i.MX8qxp prefetch engine DT binding files Liu Ying
` (11 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:07 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying
Add bindings for i.MX8qxp/qm Display Prefetch Resolve Channel.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
Changed commit message to use imperative mood.
---
.../bindings/display/imx/fsl,imx8qxp-dprc.yaml | 100 +++++++++++++++++++++
1 file changed, 100 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dprc.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dprc.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..bd94254c1288d75f81662ef12b7f79209efb45f0
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dprc.yaml
@@ -0,0 +1,100 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/imx/fsl,imx8qxp-dprc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale i.MX8qm/qxp Display Prefetch Resolve Channel
+
+maintainers:
+ - Liu Ying <victor.liu@nxp.com>
+
+description: |
+ The i.MX8qm/qxp Display Prefetch Resolve Channel(DPRC) is an engine which
+ fetches display data before the display pipeline needs the data to drive
+ pixels in the active display region. This data is transformed, or resolved,
+ from a variety of tiled buffer formats into linear format, if needed.
+ The DPR works with a double bank memory structure. This memory structure is
+ implemented in the Resolve Tile Memory(RTRAM) and the banks are referred to
+ as A and B. Each bank is either 4 or 8 lines high depending on the source
+ frame buffer format.
+
+properties:
+ compatible:
+ enum:
+ - fsl,imx8qxp-dpr-channel
+ - fsl,imx8qm-dpr-channel
+
+ reg:
+ maxItems: 1
+
+ interrupts-extended:
+ items:
+ - description: DPR wrap interrupt
+ - description: |
+ 'r_rtram_stall' interrupt which indicates relevant i.MX8qm/qxp
+ Prefetch Resolve Gasket(PRG) or PRGs are forcing an underflow
+ condition in the RTRAM.
+
+ interrupt-names:
+ items:
+ - const: dpr_wrap
+ - const: r_rtram_stall
+
+ clocks:
+ items:
+ - description: apb clock
+ - description: b clock
+ - description: rtram clock
+
+ clock-names:
+ items:
+ - const: apb
+ - const: b
+ - const: rtram
+
+ fsl,sc-resource:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: The SCU resource ID associated with this DPRC instance.
+
+ fsl,prgs:
+ $ref: /schemas/types.yaml#/definitions/phandle-array
+ description: |
+ List of phandle which points to PRG or PRGs associated with
+ this DPRC instance.
+
+ power-domains:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts-extended
+ - interrupt-names
+ - clocks
+ - clock-names
+ - fsl,sc-resource
+ - fsl,prgs
+ - power-domains
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/imx8-lpcg.h>
+ #include <dt-bindings/firmware/imx/rsrc.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ dpr-channel@56100000 {
+ compatible = "fsl,imx8qxp-dpr-channel";
+ reg = <0x56100000 0x10000>;
+ interrupts-extended = <&gic GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>,
+ <&dc0_irqsteer 324>;
+ interrupt-names = "dpr_wrap", "r_rtram_stall";
+ clocks = <&dc0_dpr1_lpcg IMX_LPCG_CLK_4>,
+ <&dc0_dpr1_lpcg IMX_LPCG_CLK_5>,
+ <&dc0_rtram1_lpcg IMX_LPCG_CLK_0>;
+ clock-names = "apb", "b", "rtram";
+ fsl,sc-resource = <IMX_SC_R_DC_0_VIDEO0>;
+ fsl,prgs = <&dc0_prg4>, <&dc0_prg5>;
+ power-domains = <&pd IMX_SC_R_DC_0>;
+ };
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 03/14] MAINTAINERS: Add i.MX8qxp prefetch engine DT binding files
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
2025-09-23 2:07 ` [PATCH v2 01/14] dt-bindings: display: imx: Add i.MX8qxp/qm PRG binding Liu Ying
2025-09-23 2:07 ` [PATCH v2 02/14] dt-bindings: display: imx: Add i.MX8qxp/qm DPR channel binding Liu Ying
@ 2025-09-23 2:07 ` Liu Ying
2025-09-23 2:07 ` [PATCH v2 04/14] drm/imx: dc-fu: Fix dimensions Liu Ying
` (10 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:07 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying, Frank Li
Add i.MX8qxp prefetch engine DT binding files to
'DRM DRIVERS FOR FREESCALE IMX8 DISPLAY CONTROLLER' section.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v2:
- Collect Frank's R-b tag.
---
MAINTAINERS | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 0c8281ea4cc64dce7de4e5ae660a24139753eccb..d1051de3ba5d0ca524a3895ea8981c3e89054e11 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8324,6 +8324,8 @@ L: dri-devel@lists.freedesktop.org
S: Maintained
T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
F: Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc*.yaml
+F: Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dprc.yaml
+F: Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-prg.yaml
F: drivers/gpu/drm/imx/dc/
DRM DRIVERS FOR FREESCALE IMX BRIDGE
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 04/14] drm/imx: dc-fu: Fix dimensions
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
` (2 preceding siblings ...)
2025-09-23 2:07 ` [PATCH v2 03/14] MAINTAINERS: Add i.MX8qxp prefetch engine DT binding files Liu Ying
@ 2025-09-23 2:07 ` Liu Ying
2025-09-23 2:07 ` [PATCH v2 05/14] drm/imx: dc-crtc: Disable at boot Liu Ying
` (9 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:07 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying, Alexander Stein, Frank Li
Fix off-by-one issue in LINEWIDTH, LINECOUNT, FRAMEWIDTH and FRAMEHEIGHT
macro definitions. The first two macros are used to set a fetchunit's
source buffer dimension and the other two are used to set a fetchunit's
frame dimension. It appears that display controller itself works ok
without this fix, however, it enters panic mode when prefetch engine(DPRC
and PRGs) attaches to it without this fix.
Fixes: 0e177d5ce01c ("drm/imx: Add i.MX8qxp Display Controller pixel engine")
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v2:
- Collect Alexander's and Frank's R-b tags.
---
drivers/gpu/drm/imx/dc/dc-fu.c | 4 ++--
drivers/gpu/drm/imx/dc/dc-fu.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/imx/dc/dc-fu.c b/drivers/gpu/drm/imx/dc/dc-fu.c
index f94c591c815891468a5c2a940076963eeaa4d51c..b36d3034b8d5962eaab212b6b65ebbb5c29f978c 100644
--- a/drivers/gpu/drm/imx/dc/dc-fu.c
+++ b/drivers/gpu/drm/imx/dc/dc-fu.c
@@ -31,8 +31,8 @@
#define STRIDE(x) FIELD_PREP(STRIDE_MASK, (x) - 1)
/* SOURCEBUFFERDIMENSION */
-#define LINEWIDTH(x) FIELD_PREP(GENMASK(13, 0), (x))
-#define LINECOUNT(x) FIELD_PREP(GENMASK(29, 16), (x))
+#define LINEWIDTH(x) FIELD_PREP(GENMASK(13, 0), (x) - 1)
+#define LINECOUNT(x) FIELD_PREP(GENMASK(29, 16), (x) - 1)
/* LAYEROFFSET */
#define LAYERXOFFSET(x) FIELD_PREP(GENMASK(14, 0), (x))
diff --git a/drivers/gpu/drm/imx/dc/dc-fu.h b/drivers/gpu/drm/imx/dc/dc-fu.h
index e016e1ea5b4e0471cf6627782603e72d0475c4e8..518d1af49f5ae9d4f67da5e6c2e80abd7e962120 100644
--- a/drivers/gpu/drm/imx/dc/dc-fu.h
+++ b/drivers/gpu/drm/imx/dc/dc-fu.h
@@ -38,8 +38,8 @@
#define SOURCEBUFFERENABLE BIT(31)
/* FRAMEDIMENSIONS */
-#define FRAMEWIDTH(x) FIELD_PREP(GENMASK(13, 0), (x))
-#define FRAMEHEIGHT(x) FIELD_PREP(GENMASK(29, 16), (x))
+#define FRAMEWIDTH(x) FIELD_PREP(GENMASK(13, 0), (x) - 1)
+#define FRAMEHEIGHT(x) FIELD_PREP(GENMASK(29, 16), (x) - 1)
/* CONTROL */
#define INPUTSELECT_MASK GENMASK(4, 3)
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 05/14] drm/imx: dc-crtc: Disable at boot
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
` (3 preceding siblings ...)
2025-09-23 2:07 ` [PATCH v2 04/14] drm/imx: dc-fu: Fix dimensions Liu Ying
@ 2025-09-23 2:07 ` Liu Ying
2025-09-23 18:08 ` Frank Li
2025-09-23 2:07 ` [PATCH v2 06/14] drm/imx: dc: Add PRG support Liu Ying
` (8 subsequent siblings)
13 siblings, 1 reply; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:07 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying
CRTC(s) could still be running after the DRM device is unplugged by
calling drm_dev_unplug(), because the CRTC disablement logic is
protected and bypassed by the drm_dev_enter()/drm_dev_exit() pair.
Hence, Pixel Engine's AXI clock use count(managed by Pixel Engine
driver's runtime PM) and pixel clock use count could be inbalanced
after removing and re-installing the driver module. To fix this,
add a helper dc_crtc_disable_at_boot() and call it to properly
disable all CRTCs before advertising DRM device to user-space by
calling drm_dev_register().
Fixes: 711a3b878366 ("drm/imx: Add i.MX8qxp Display Controller KMS")
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
drivers/gpu/drm/imx/dc/dc-crtc.c | 50 ++++++++++++++++++++++++++++++++++++----
drivers/gpu/drm/imx/dc/dc-drv.c | 5 ++++
drivers/gpu/drm/imx/dc/dc-drv.h | 3 +++
3 files changed, 53 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
index 31d3a982deaf7a0390937285c9d5d00100323181..45a87df1ad6a8bd768aa5ed38d6f03f14052b3d7 100644
--- a/drivers/gpu/drm/imx/dc/dc-crtc.c
+++ b/drivers/gpu/drm/imx/dc/dc-crtc.c
@@ -293,6 +293,16 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
dc_crtc_queue_state_event(new_crtc_state);
}
+static inline void __dc_crtc_disable_fg(struct drm_crtc *crtc)
+{
+ struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
+
+ enable_irq(dc_crtc->irq_dec_seqcomplete);
+ dc_fg_disable(dc_crtc->fg);
+ DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
+ disable_irq(dc_crtc->irq_dec_seqcomplete);
+}
+
static void
dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
{
@@ -305,11 +315,7 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
if (!drm_dev_enter(crtc->dev, &idx))
goto out;
- enable_irq(dc_crtc->irq_dec_seqcomplete);
- dc_fg_disable(dc_crtc->fg);
- DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
- disable_irq(dc_crtc->irq_dec_seqcomplete);
-
+ __dc_crtc_disable_fg(crtc);
dc_fg_disable_clock(dc_crtc->fg);
/* request pixel engine power-off as plane is off too */
@@ -337,6 +343,40 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
spin_unlock_irq(&crtc->dev->event_lock);
}
+void dc_crtc_disable_at_boot(struct drm_crtc *crtc)
+{
+ struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev);
+ struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
+ int ret;
+
+ ret = pm_runtime_resume_and_get(dc_crtc->de->dev);
+ if (ret < 0) {
+ dc_crtc_err(crtc, "failed to get DC display engine RPM: %d\n",
+ ret);
+ return;
+ }
+
+ if (!dc_fg_wait_for_frame_index_moving(dc_crtc->fg)) {
+ dc_crtc_dbg(crtc, "FrameGen frame index isn't moving\n");
+ goto out;
+ }
+
+ dc_crtc_dbg(crtc, "disabling at boot\n");
+ __dc_crtc_disable_fg(crtc);
+ dc_fg_disable_clock(dc_crtc->fg);
+
+ if (!dc_drm->pe_clk_axi_disabled) {
+ clk_disable_unprepare(dc_drm->pe->clk_axi);
+ dc_drm->pe_clk_axi_disabled = true;
+ }
+
+out:
+ ret = pm_runtime_put(dc_crtc->de->dev);
+ if (ret < 0)
+ dc_crtc_err(crtc, "failed to put DC display engine RPM: %d\n",
+ ret);
+}
+
static bool dc_crtc_get_scanout_position(struct drm_crtc *crtc,
bool in_vblank_irq,
int *vpos, int *hpos,
diff --git a/drivers/gpu/drm/imx/dc/dc-drv.c b/drivers/gpu/drm/imx/dc/dc-drv.c
index 04f021d2d6cfc93972aa8d9073be24d347152602..f93766b6bfbfae8510db05278d104820ca0719c4 100644
--- a/drivers/gpu/drm/imx/dc/dc-drv.c
+++ b/drivers/gpu/drm/imx/dc/dc-drv.c
@@ -17,6 +17,7 @@
#include <drm/clients/drm_client_setup.h>
#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc.h>
#include <drm/drm_drv.h>
#include <drm/drm_fbdev_dma.h>
#include <drm/drm_fourcc.h>
@@ -96,6 +97,7 @@ static int dc_drm_bind(struct device *dev)
struct dc_priv *priv = dev_get_drvdata(dev);
struct dc_drm_device *dc_drm;
struct drm_device *drm;
+ struct drm_crtc *crtc;
int ret;
dc_drm = devm_drm_dev_alloc(dev, &dc_drm_driver, struct dc_drm_device,
@@ -118,6 +120,9 @@ static int dc_drm_bind(struct device *dev)
if (ret)
return ret;
+ drm_for_each_crtc(crtc, drm)
+ dc_crtc_disable_at_boot(crtc);
+
ret = drm_dev_register(drm, 0);
if (ret) {
dev_err(dev, "failed to register drm device: %d\n", ret);
diff --git a/drivers/gpu/drm/imx/dc/dc-drv.h b/drivers/gpu/drm/imx/dc/dc-drv.h
index eb61b8c7626933adc7688f046139e2167665dad1..68e99ba7cedbca1b8bdc0d8ced7a610a1056bfc7 100644
--- a/drivers/gpu/drm/imx/dc/dc-drv.h
+++ b/drivers/gpu/drm/imx/dc/dc-drv.h
@@ -50,6 +50,8 @@ struct dc_drm_device {
struct dc_pe *pe;
/** @tc: tcon list */
struct dc_tc *tc[DC_DISPLAYS];
+ /** @pe_clk_axi_disabled: disablement flag at boot */
+ bool pe_clk_axi_disabled;
};
struct dc_subdev_info {
@@ -96,6 +98,7 @@ static inline int dc_subdev_get_id(const struct dc_subdev_info *info,
return -EINVAL;
}
+void dc_crtc_disable_at_boot(struct drm_crtc *crtc);
void dc_de_post_bind(struct dc_drm_device *dc_drm);
void dc_pe_post_bind(struct dc_drm_device *dc_drm);
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 05/14] drm/imx: dc-crtc: Disable at boot
2025-09-23 2:07 ` [PATCH v2 05/14] drm/imx: dc-crtc: Disable at boot Liu Ying
@ 2025-09-23 18:08 ` Frank Li
0 siblings, 0 replies; 26+ messages in thread
From: Frank Li @ 2025-09-23 18:08 UTC (permalink / raw)
To: Liu Ying
Cc: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov,
dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel
On Tue, Sep 23, 2025 at 10:07:55AM +0800, Liu Ying wrote:
> CRTC(s) could still be running after the DRM device is unplugged by
> calling drm_dev_unplug(), because the CRTC disablement logic is
> protected and bypassed by the drm_dev_enter()/drm_dev_exit() pair.
> Hence, Pixel Engine's AXI clock use count(managed by Pixel Engine
> driver's runtime PM) and pixel clock use count could be inbalanced
> after removing and re-installing the driver module. To fix this,
> add a helper dc_crtc_disable_at_boot() and call it to properly
> disable all CRTCs before advertising DRM device to user-space by
> calling drm_dev_register().
>
> Fixes: 711a3b878366 ("drm/imx: Add i.MX8qxp Display Controller KMS")
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
> drivers/gpu/drm/imx/dc/dc-crtc.c | 50 ++++++++++++++++++++++++++++++++++++----
> drivers/gpu/drm/imx/dc/dc-drv.c | 5 ++++
> drivers/gpu/drm/imx/dc/dc-drv.h | 3 +++
> 3 files changed, 53 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
> index 31d3a982deaf7a0390937285c9d5d00100323181..45a87df1ad6a8bd768aa5ed38d6f03f14052b3d7 100644
> --- a/drivers/gpu/drm/imx/dc/dc-crtc.c
> +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c
> @@ -293,6 +293,16 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
> dc_crtc_queue_state_event(new_crtc_state);
> }
>
> +static inline void __dc_crtc_disable_fg(struct drm_crtc *crtc)
> +{
> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
> +
> + enable_irq(dc_crtc->irq_dec_seqcomplete);
> + dc_fg_disable(dc_crtc->fg);
> + DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
> + disable_irq(dc_crtc->irq_dec_seqcomplete);
> +}
> +
> static void
> dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
> {
> @@ -305,11 +315,7 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
> if (!drm_dev_enter(crtc->dev, &idx))
> goto out;
>
> - enable_irq(dc_crtc->irq_dec_seqcomplete);
> - dc_fg_disable(dc_crtc->fg);
> - DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
> - disable_irq(dc_crtc->irq_dec_seqcomplete);
> -
> + __dc_crtc_disable_fg(crtc);
> dc_fg_disable_clock(dc_crtc->fg);
>
> /* request pixel engine power-off as plane is off too */
> @@ -337,6 +343,40 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
> spin_unlock_irq(&crtc->dev->event_lock);
> }
>
> +void dc_crtc_disable_at_boot(struct drm_crtc *crtc)
> +{
> + struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev);
> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
> + int ret;
> +
> + ret = pm_runtime_resume_and_get(dc_crtc->de->dev);
> + if (ret < 0) {
> + dc_crtc_err(crtc, "failed to get DC display engine RPM: %d\n",
> + ret);
> + return;
> + }
> +
> + if (!dc_fg_wait_for_frame_index_moving(dc_crtc->fg)) {
> + dc_crtc_dbg(crtc, "FrameGen frame index isn't moving\n");
> + goto out;
> + }
> +
> + dc_crtc_dbg(crtc, "disabling at boot\n");
> + __dc_crtc_disable_fg(crtc);
> + dc_fg_disable_clock(dc_crtc->fg);
> +
> + if (!dc_drm->pe_clk_axi_disabled) {
> + clk_disable_unprepare(dc_drm->pe->clk_axi);
> + dc_drm->pe_clk_axi_disabled = true;
> + }
> +
> +out:
> + ret = pm_runtime_put(dc_crtc->de->dev);
> + if (ret < 0)
> + dc_crtc_err(crtc, "failed to put DC display engine RPM: %d\n",
> + ret);
> +}
> +
> static bool dc_crtc_get_scanout_position(struct drm_crtc *crtc,
> bool in_vblank_irq,
> int *vpos, int *hpos,
> diff --git a/drivers/gpu/drm/imx/dc/dc-drv.c b/drivers/gpu/drm/imx/dc/dc-drv.c
> index 04f021d2d6cfc93972aa8d9073be24d347152602..f93766b6bfbfae8510db05278d104820ca0719c4 100644
> --- a/drivers/gpu/drm/imx/dc/dc-drv.c
> +++ b/drivers/gpu/drm/imx/dc/dc-drv.c
> @@ -17,6 +17,7 @@
>
> #include <drm/clients/drm_client_setup.h>
> #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc.h>
> #include <drm/drm_drv.h>
> #include <drm/drm_fbdev_dma.h>
> #include <drm/drm_fourcc.h>
> @@ -96,6 +97,7 @@ static int dc_drm_bind(struct device *dev)
> struct dc_priv *priv = dev_get_drvdata(dev);
> struct dc_drm_device *dc_drm;
> struct drm_device *drm;
> + struct drm_crtc *crtc;
> int ret;
>
> dc_drm = devm_drm_dev_alloc(dev, &dc_drm_driver, struct dc_drm_device,
> @@ -118,6 +120,9 @@ static int dc_drm_bind(struct device *dev)
> if (ret)
> return ret;
>
> + drm_for_each_crtc(crtc, drm)
> + dc_crtc_disable_at_boot(crtc);
> +
> ret = drm_dev_register(drm, 0);
> if (ret) {
> dev_err(dev, "failed to register drm device: %d\n", ret);
> diff --git a/drivers/gpu/drm/imx/dc/dc-drv.h b/drivers/gpu/drm/imx/dc/dc-drv.h
> index eb61b8c7626933adc7688f046139e2167665dad1..68e99ba7cedbca1b8bdc0d8ced7a610a1056bfc7 100644
> --- a/drivers/gpu/drm/imx/dc/dc-drv.h
> +++ b/drivers/gpu/drm/imx/dc/dc-drv.h
> @@ -50,6 +50,8 @@ struct dc_drm_device {
> struct dc_pe *pe;
> /** @tc: tcon list */
> struct dc_tc *tc[DC_DISPLAYS];
> + /** @pe_clk_axi_disabled: disablement flag at boot */
> + bool pe_clk_axi_disabled;
> };
>
> struct dc_subdev_info {
> @@ -96,6 +98,7 @@ static inline int dc_subdev_get_id(const struct dc_subdev_info *info,
> return -EINVAL;
> }
>
> +void dc_crtc_disable_at_boot(struct drm_crtc *crtc);
> void dc_de_post_bind(struct dc_drm_device *dc_drm);
> void dc_pe_post_bind(struct dc_drm_device *dc_drm);
>
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 06/14] drm/imx: dc: Add PRG support
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
` (4 preceding siblings ...)
2025-09-23 2:07 ` [PATCH v2 05/14] drm/imx: dc-crtc: Disable at boot Liu Ying
@ 2025-09-23 2:07 ` Liu Ying
2025-09-23 2:07 ` [PATCH v2 07/14] drm/imx: dc: Add DPR channel support Liu Ying
` (7 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:07 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying, Frank Li
Display Prefetch Resolve Gasket(PRG) is a part of a FetchUnit's
prefetch engine. It sits between a Display Prefetch Resolve
Channel(DPRC) and a FetchUnit. Add a platform driver to support
the PRG.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v2:
- Manage clocks with bulk interfaces. (Frank)
- Collect Frank's R-b tag.
---
drivers/gpu/drm/imx/dc/Makefile | 2 +-
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 | 308 ++++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/imx/dc/dc-prg.h | 35 +++++
5 files changed, 346 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile
index b9d33c074984a7ee5a6f0876d09bfeee5096264c..e3a06ee3ce1a5117d0a9a00fdf7655ee31be3caf 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-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-drv.c b/drivers/gpu/drm/imx/dc/dc-drv.c
index f93766b6bfbfae8510db05278d104820ca0719c4..9bdcfc5aee976ef77bea6b3f6f3ac5f11249798f 100644
--- a/drivers/gpu/drm/imx/dc/dc-drv.c
+++ b/drivers/gpu/drm/imx/dc/dc-drv.c
@@ -276,6 +276,7 @@ static struct platform_driver * const dc_drivers[] = {
&dc_ic_driver,
&dc_lb_driver,
&dc_pe_driver,
+ &dc_prg_driver,
&dc_tc_driver,
&dc_driver,
};
diff --git a/drivers/gpu/drm/imx/dc/dc-drv.h b/drivers/gpu/drm/imx/dc/dc-drv.h
index 68e99ba7cedbca1b8bdc0d8ced7a610a1056bfc7..557e7d90e4ea8ca2af59027b3152163cf7f9a618 100644
--- a/drivers/gpu/drm/imx/dc/dc-drv.h
+++ b/drivers/gpu/drm/imx/dc/dc-drv.h
@@ -81,6 +81,7 @@ extern struct platform_driver dc_fw_driver;
extern struct platform_driver dc_ic_driver;
extern struct platform_driver dc_lb_driver;
extern struct platform_driver dc_pe_driver;
+extern struct platform_driver dc_prg_driver;
extern struct platform_driver dc_tc_driver;
static inline int dc_subdev_get_id(const struct dc_subdev_info *info,
diff --git a/drivers/gpu/drm/imx/dc/dc-prg.c b/drivers/gpu/drm/imx/dc/dc-prg.c
new file mode 100644
index 0000000000000000000000000000000000000000..f37bff12674ae792dc35a1f27cf754df4c372f20
--- /dev/null
+++ b/drivers/gpu/drm/imx/dc/dc-prg.c
@@ -0,0 +1,308 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2025 NXP
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#include "dc-prg.h"
+
+#define SET 0x4
+#define CLR 0x8
+#define TOG 0xc
+
+#define PRG_CTRL 0x00
+#define BYPASS BIT(0)
+#define SC_DATA_TYPE_8BIT 0
+#define HANDSHAKE_MODE_4LINES 0
+#define SHADOW_LOAD_MODE BIT(5)
+#define DES_DATA_TYPE_MASK GENMASK(17, 16)
+#define DES_DATA_TYPE_32BPP FIELD_PREP(DES_DATA_TYPE_MASK, 0)
+#define DES_DATA_TYPE_24BPP FIELD_PREP(DES_DATA_TYPE_MASK, 1)
+#define DES_DATA_TYPE_16BPP FIELD_PREP(DES_DATA_TYPE_MASK, 2)
+#define DES_DATA_TYPE_8BPP FIELD_PREP(DES_DATA_TYPE_MASK, 3)
+#define SOFTRST BIT(30)
+#define SHADOW_EN BIT(31)
+
+#define PRG_STATUS 0x10
+
+#define PRG_REG_UPDATE 0x20
+#define REG_UPDATE BIT(0)
+
+#define PRG_STRIDE 0x30
+#define STRIDE(n) FIELD_PREP(GENMASK(15, 0), (n) - 1)
+
+#define PRG_HEIGHT 0x40
+#define HEIGHT(n) FIELD_PREP(GENMASK(15, 0), (n) - 1)
+
+#define PRG_BADDR 0x50
+#define PRG_OFFSET 0x60
+
+#define PRG_WIDTH 0x70
+#define WIDTH(n) FIELD_PREP(GENMASK(15, 0), (n) - 1)
+
+#define DPU_PRG_MAX_STRIDE 0x10000
+
+struct dc_prg {
+ struct device *dev;
+ struct regmap *reg;
+ struct list_head list;
+ struct clk_bulk_data *clks;
+ int num_clks;
+};
+
+static DEFINE_MUTEX(dc_prg_list_mutex);
+static LIST_HEAD(dc_prg_list);
+
+static const struct regmap_config dc_prg_regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .fast_io = true,
+ .max_register = PRG_WIDTH + TOG,
+};
+
+static void dc_prg_reset(struct dc_prg *prg)
+{
+ regmap_write(prg->reg, PRG_CTRL + SET, SOFTRST);
+ fsleep(10);
+ regmap_write(prg->reg, PRG_CTRL + CLR, SOFTRST);
+ fsleep(10);
+}
+
+void dc_prg_enable(struct dc_prg *prg)
+{
+ regmap_write(prg->reg, PRG_CTRL + CLR, BYPASS);
+}
+
+void dc_prg_disable(struct dc_prg *prg)
+{
+ regmap_write(prg->reg, PRG_CTRL, BYPASS);
+
+ pm_runtime_put(prg->dev);
+}
+
+void dc_prg_disable_at_boot(struct dc_prg *prg)
+{
+ regmap_write(prg->reg, PRG_CTRL, BYPASS);
+
+ clk_bulk_disable_unprepare(prg->num_clks, prg->clks);
+}
+
+static unsigned int dc_prg_burst_size_fixup(dma_addr_t baddr)
+{
+ unsigned int burst_size;
+
+ burst_size = 1 << __ffs(baddr);
+ burst_size = round_up(burst_size, 8);
+ burst_size = min(burst_size, 128U);
+
+ return burst_size;
+}
+
+static unsigned int
+dc_prg_stride_fixup(unsigned int stride, unsigned int burst_size)
+{
+ return round_up(stride, burst_size);
+}
+
+void dc_prg_configure(struct dc_prg *prg,
+ unsigned int width, unsigned int height,
+ unsigned int stride, unsigned int bits_per_pixel,
+ dma_addr_t baddr, bool start)
+{
+ struct device *dev = prg->dev;
+ unsigned int burst_size;
+ 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;
+ }
+ }
+
+ burst_size = dc_prg_burst_size_fixup(baddr);
+
+ stride = dc_prg_stride_fixup(stride, burst_size);
+
+ regmap_write(prg->reg, PRG_STRIDE, STRIDE(stride));
+ regmap_write(prg->reg, PRG_WIDTH, WIDTH(width));
+ regmap_write(prg->reg, PRG_HEIGHT, HEIGHT(height));
+ regmap_write(prg->reg, PRG_OFFSET, 0);
+ regmap_write(prg->reg, PRG_BADDR, baddr);
+
+ val = SHADOW_LOAD_MODE | SC_DATA_TYPE_8BIT | BYPASS |
+ HANDSHAKE_MODE_4LINES;
+
+ switch (bits_per_pixel) {
+ case 32:
+ val |= DES_DATA_TYPE_32BPP;
+ break;
+ case 24:
+ val |= DES_DATA_TYPE_24BPP;
+ break;
+ case 16:
+ val |= DES_DATA_TYPE_16BPP;
+ break;
+ case 8:
+ val |= DES_DATA_TYPE_8BPP;
+ break;
+ }
+
+ /* no shadow for the first frame */
+ if (!start)
+ val |= SHADOW_EN;
+ regmap_write(prg->reg, PRG_CTRL, val);
+}
+
+void dc_prg_reg_update(struct dc_prg *prg)
+{
+ regmap_write(prg->reg, PRG_REG_UPDATE, REG_UPDATE);
+}
+
+void dc_prg_shadow_enable(struct dc_prg *prg)
+{
+ regmap_write(prg->reg, PRG_CTRL + SET, SHADOW_EN);
+}
+
+bool dc_prg_stride_supported(struct dc_prg *prg,
+ unsigned int stride, dma_addr_t baddr)
+{
+ unsigned int burst_size;
+
+ burst_size = dc_prg_burst_size_fixup(baddr);
+
+ stride = dc_prg_stride_fixup(stride, burst_size);
+
+ if (stride > DPU_PRG_MAX_STRIDE)
+ return false;
+
+ return true;
+}
+
+struct dc_prg *
+dc_prg_lookup_by_phandle(struct device *dev, const char *name, int index)
+{
+ struct device_node *prg_node __free(device_node);
+ struct dc_prg *prg;
+
+ prg_node = of_parse_phandle(dev->of_node, name, index);
+ if (!prg_node)
+ return NULL;
+
+ guard(mutex)(&dc_prg_list_mutex);
+ list_for_each_entry(prg, &dc_prg_list, list) {
+ if (prg_node == prg->dev->of_node)
+ return prg;
+ }
+
+ return NULL;
+}
+
+static int dc_prg_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ void __iomem *base;
+ struct dc_prg *prg;
+ int ret;
+
+ prg = devm_kzalloc(dev, sizeof(*prg), GFP_KERNEL);
+ if (!prg)
+ return -ENOMEM;
+
+ base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ prg->reg = devm_regmap_init_mmio(dev, base, &dc_prg_regmap_config);
+ if (IS_ERR(prg->reg))
+ return PTR_ERR(prg->reg);
+
+ prg->num_clks = devm_clk_bulk_get_all(dev, &prg->clks);
+ if (prg->num_clks < 0)
+ return dev_err_probe(dev, prg->num_clks, "failed to get clocks\n");
+
+ dev_set_drvdata(dev, prg);
+
+ ret = devm_pm_runtime_enable(dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to enable PM runtime\n");
+
+ prg->dev = dev;
+
+ guard(mutex)(&dc_prg_list_mutex);
+ list_add(&prg->list, &dc_prg_list);
+
+ return 0;
+}
+
+static void dc_prg_remove(struct platform_device *pdev)
+{
+ struct dc_prg *prg = dev_get_drvdata(&pdev->dev);
+
+ guard(mutex)(&dc_prg_list_mutex);
+ list_del(&prg->list);
+}
+
+static int dc_prg_runtime_suspend(struct device *dev)
+{
+ struct dc_prg *prg = dev_get_drvdata(dev);
+
+ clk_bulk_disable_unprepare(prg->num_clks, prg->clks);
+
+ return 0;
+}
+
+static int dc_prg_runtime_resume(struct device *dev)
+{
+ struct dc_prg *prg = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_bulk_prepare_enable(prg->num_clks, prg->clks);
+ if (ret) {
+ dev_err(dev, "failed to enable clocks: %d\n", ret);
+ return ret;
+ }
+
+ dc_prg_reset(prg);
+
+ return 0;
+}
+
+static const struct dev_pm_ops dc_prg_pm_ops = {
+ RUNTIME_PM_OPS(dc_prg_runtime_suspend, dc_prg_runtime_resume, NULL)
+};
+
+static const struct of_device_id dc_prg_dt_ids[] = {
+ { .compatible = "fsl,imx8qxp-prg", },
+ { /* sentinel */ }
+};
+
+struct platform_driver dc_prg_driver = {
+ .probe = dc_prg_probe,
+ .remove = dc_prg_remove,
+ .driver = {
+ .name = "imx8-dc-prg",
+ .suppress_bind_attrs = true,
+ .of_match_table = dc_prg_dt_ids,
+ .pm = pm_ptr(&dc_prg_pm_ops),
+ },
+};
diff --git a/drivers/gpu/drm/imx/dc/dc-prg.h b/drivers/gpu/drm/imx/dc/dc-prg.h
new file mode 100644
index 0000000000000000000000000000000000000000..6fd9b050bfa12334720f83ff9ceaf337e3048a54
--- /dev/null
+++ b/drivers/gpu/drm/imx/dc/dc-prg.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright 2025 NXP
+ */
+
+#ifndef __DC_PRG_H__
+#define __DC_PRG_H__
+
+#include <linux/device.h>
+#include <linux/types.h>
+
+struct dc_prg;
+
+void dc_prg_enable(struct dc_prg *prg);
+
+void dc_prg_disable(struct dc_prg *prg);
+
+void dc_prg_disable_at_boot(struct dc_prg *prg);
+
+void dc_prg_configure(struct dc_prg *prg,
+ unsigned int width, unsigned int height,
+ unsigned int stride, unsigned int bits_per_pixel,
+ dma_addr_t baddr, bool start);
+
+void dc_prg_reg_update(struct dc_prg *prg);
+
+void dc_prg_shadow_enable(struct dc_prg *prg);
+
+bool dc_prg_stride_supported(struct dc_prg *prg,
+ unsigned int stride, dma_addr_t baddr);
+
+struct dc_prg *
+dc_prg_lookup_by_phandle(struct device *dev, const char *name, int index);
+
+#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 07/14] drm/imx: dc: Add DPR channel support
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
` (5 preceding siblings ...)
2025-09-23 2:07 ` [PATCH v2 06/14] drm/imx: dc: Add PRG support Liu Ying
@ 2025-09-23 2:07 ` Liu Ying
2025-09-23 18:29 ` Frank Li
2025-09-23 2:07 ` [PATCH v2 08/14] drm/imx: dc: Use TCON operation mode Liu Ying
` (6 subsequent siblings)
13 siblings, 1 reply; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:07 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying
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@nxp.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(-)
diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig
index 415993207f2e3487f09602050fa9284fd0955cc7..507dc9a92d96be225cd9b10968a037dad286b327 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 e3a06ee3ce1a5117d0a9a00fdf7655ee31be3caf..fd5d62783971d575cf18d3e27d742d91dee7623e 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 0000000000000000000000000000000000000000..c6dcc0f820ebd569542df51bb9088dd9e71d24c2
--- /dev/null
+++ b/drivers/gpu/drm/imx/dc/dc-dprc.c
@@ -0,0 +1,465 @@
+// 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/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/firmware/imx/rsrc.h>
+
+#include "dc-dprc.h"
+#include "dc-prg.h"
+
+#define SET 0x4
+#define CLR 0x8
+#define TOG 0xc
+
+#define SYSTEM_CTRL0 0x00
+#define SW_SHADOW_LOAD_SEL BIT(4)
+#define SHADOW_LOAD_EN BIT(3)
+#define REPEAT_EN BIT(2)
+#define SOFT_RESET BIT(1)
+#define RUN_EN BIT(0) /* self-clearing */
+
+#define IRQ_MASK 0x20
+#define IRQ_MASK_STATUS 0x30
+#define IRQ_NONMASK_STATUS 0x40
+#define DPR2RTR_FIFO_LOAD_BUF_RDY_UV_ERROR BIT(7)
+#define DPR2RTR_FIFO_LOAD_BUF_RDY_YRGB_ERROR BIT(6)
+#define DPR2RTR_UV_FIFO_OVFL BIT(5)
+#define DPR2RTR_YRGB_FIFO_OVFL BIT(4)
+#define IRQ_AXI_READ_ERROR BIT(3)
+#define IRQ_DPR_SHADOW_LOADED_MASK BIT(2)
+#define IRQ_DPR_RUN BIT(1)
+#define IRQ_DPR_CRTL_DONE BIT(0)
+#define IRQ_CTRL_MASK GENMASK(2, 0)
+
+#define MODE_CTRL0 0x50
+#define A_COMP_SEL(byte) FIELD_PREP(GENMASK(17, 16), (byte))
+#define R_COMP_SEL(byte) FIELD_PREP(GENMASK(15, 14), (byte))
+#define G_COMP_SEL(byte) FIELD_PREP(GENMASK(13, 12), (byte))
+#define B_COMP_SEL(byte) FIELD_PREP(GENMASK(11, 10), (byte))
+#define PIX_SIZE_32BIT FIELD_PREP(GENMASK(7, 6), 0x2)
+#define LINE4 BIT(1)
+#define BUF2 0
+
+#define FRAME_CTRL0 0x70
+#define PITCH(n) FIELD_PREP(GENMASK(31, 16), (n))
+
+#define FRAME_1P_CTRL0 0x90
+#define FRAME_2P_CTRL0 0xe0
+#define MAX_BYTES_PREQ_MASK GENMASK(2, 0)
+#define BYTE_1K FIELD_PREP(MAX_BYTES_PREQ_MASK, 0x4)
+
+#define FRAME_1P_PIX_X_CTRL 0xa0
+#define NUM_X_PIX_WIDE(n) FIELD_PREP(GENMASK(15, 0), (n))
+
+#define FRAME_1P_PIX_Y_CTRL 0xb0
+#define NUM_Y_PIX_HIGH(n) FIELD_PREP(GENMASK(15, 0), (n))
+
+#define FRAME_1P_BASE_ADDR_CTRL0 0xc0
+
+#define FRAME_PIX_X_ULC_CTRL 0xf0
+#define CROP_ULC_X(n) FIELD_PREP(GENMASK(15, 0), (n))
+
+#define FRAME_PIX_Y_ULC_CTRL 0x100
+#define CROP_ULC_Y(n) FIELD_PREP(GENMASK(15, 0), (n))
+
+#define FRAME_2P_BASE_ADDR_CTRL0 0x110
+
+#define STATUS_CTRL0 0x130
+#define STATUS_CTRL1 0x140
+
+#define RTRAM_CTRL0 0x200
+#define THRES_LOW(n) FIELD_PREP(GENMASK(6, 4), (n))
+#define THRES_HIGH(n) FIELD_PREP(GENMASK(3, 1), (n))
+
+#define DPU_DRPC_MAX_STRIDE 0x10000
+#define DPU_DPRC_MAX_RTRAM_WIDTH 2880
+
+struct dc_dprc {
+ struct device *dev;
+ struct regmap *reg;
+ struct clk_bulk_data *clks;
+ int num_clks;
+ struct imx_sc_ipc *ipc_handle;
+ spinlock_t lock; /* protect IRQ registers */
+ u32 sc_resource;
+ struct dc_prg *prg;
+};
+
+static const struct regmap_range dc_dprc_regmap_write_ranges[] = {
+ regmap_reg_range(SYSTEM_CTRL0, SYSTEM_CTRL0 + TOG),
+ regmap_reg_range(IRQ_MASK, IRQ_MASK + TOG),
+ regmap_reg_range(IRQ_NONMASK_STATUS, MODE_CTRL0 + TOG),
+ regmap_reg_range(FRAME_CTRL0, FRAME_CTRL0 + TOG),
+ regmap_reg_range(FRAME_1P_CTRL0, FRAME_1P_BASE_ADDR_CTRL0 + TOG),
+ regmap_reg_range(FRAME_PIX_X_ULC_CTRL, FRAME_2P_BASE_ADDR_CTRL0 + TOG),
+ regmap_reg_range(STATUS_CTRL0, STATUS_CTRL0 + TOG),
+ regmap_reg_range(RTRAM_CTRL0, RTRAM_CTRL0 + TOG),
+};
+
+static const struct regmap_range dc_dprc_regmap_read_ranges[] = {
+ regmap_reg_range(SYSTEM_CTRL0, SYSTEM_CTRL0 + TOG),
+ regmap_reg_range(IRQ_MASK, IRQ_MASK_STATUS + TOG),
+ regmap_reg_range(MODE_CTRL0, MODE_CTRL0 + TOG),
+ regmap_reg_range(FRAME_CTRL0, FRAME_CTRL0 + TOG),
+ regmap_reg_range(FRAME_1P_CTRL0, FRAME_1P_BASE_ADDR_CTRL0 + TOG),
+ regmap_reg_range(FRAME_PIX_X_ULC_CTRL, FRAME_2P_BASE_ADDR_CTRL0 + TOG),
+ regmap_reg_range(STATUS_CTRL0, STATUS_CTRL1 + TOG),
+ regmap_reg_range(RTRAM_CTRL0, RTRAM_CTRL0 + TOG),
+};
+
+static const struct regmap_access_table dc_dprc_regmap_write_table = {
+ .yes_ranges = dc_dprc_regmap_write_ranges,
+ .n_yes_ranges = ARRAY_SIZE(dc_dprc_regmap_write_ranges),
+};
+
+static const struct regmap_access_table dc_dprc_regmap_read_table = {
+ .yes_ranges = dc_dprc_regmap_read_ranges,
+ .n_yes_ranges = ARRAY_SIZE(dc_dprc_regmap_read_ranges),
+};
+
+static const struct regmap_config dc_dprc_regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .fast_io = true,
+ .wr_table = &dc_dprc_regmap_write_table,
+ .rd_table = &dc_dprc_regmap_read_table,
+ .max_register = RTRAM_CTRL0 + TOG,
+};
+
+static void dc_dprc_set_stream_id(struct dc_dprc *dprc, unsigned int stream_id)
+{
+ int ret;
+
+ ret = imx_sc_misc_set_control(dprc->ipc_handle, dprc->sc_resource,
+ IMX_SC_C_KACHUNK_SEL, stream_id);
+ if (ret)
+ dev_err(dprc->dev, "failed to set KACHUNK_SEL: %d\n", ret);
+}
+
+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));
+
+ 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);
+
+ 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);
+
+ 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 irqreturn_t dc_dprc_wrap_irq_handler(int irq, void *data)
+{
+ struct dc_dprc *dprc = data;
+ struct device *dev = dprc->dev;
+ u32 mask, status;
+
+ scoped_guard(spinlock, &dprc->lock) {
+ /* cache valid IRQ status */
+ regmap_read(dprc->reg, IRQ_MASK, &mask);
+ regmap_read(dprc->reg, IRQ_MASK_STATUS, &status);
+ status &= ~mask;
+
+ /* mask the IRQ(s) being handled */
+ regmap_write(dprc->reg, IRQ_MASK + SET, status);
+
+ /* clear status register */
+ regmap_write(dprc->reg, IRQ_MASK_STATUS, status);
+ }
+
+ if (status & DPR2RTR_FIFO_LOAD_BUF_RDY_UV_ERROR)
+ dev_err(dev, "DPR to RTRAM FIFO load UV buffer ready error\n");
+
+ if (status & DPR2RTR_FIFO_LOAD_BUF_RDY_YRGB_ERROR)
+ dev_err(dev, "DPR to RTRAM FIFO load YRGB buffer ready error\n");
+
+ if (status & DPR2RTR_UV_FIFO_OVFL)
+ dev_err(dev, "DPR to RTRAM FIFO UV FIFO overflow\n");
+
+ if (status & DPR2RTR_YRGB_FIFO_OVFL)
+ dev_err(dev, "DPR to RTRAM FIFO YRGB FIFO overflow\n");
+
+ if (status & IRQ_AXI_READ_ERROR)
+ dev_err(dev, "AXI read error\n");
+
+ if (status & IRQ_DPR_CRTL_DONE)
+ dc_dprc_ctrl_done_handle(dprc);
+
+ return IRQ_HANDLED;
+}
+
+bool dc_dprc_rtram_width_supported(struct dc_dprc *dprc, unsigned int width)
+{
+ 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;
+}
+
+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);
+
+ return 0;
+}
+
+static const struct dev_pm_ops dc_dprc_pm_ops = {
+ RUNTIME_PM_OPS(dc_dprc_runtime_suspend, dc_dprc_runtime_resume, NULL)
+};
+
+static const struct of_device_id dc_dprc_dt_ids[] = {
+ { .compatible = "fsl,imx8qxp-dpr-channel", },
+ { /* sentinel */ }
+};
+
+struct platform_driver dc_dprc_driver = {
+ .probe = dc_dprc_probe,
+ .driver = {
+ .name = "imx8-dc-dpr-channel",
+ .suppress_bind_attrs = true,
+ .of_match_table = dc_dprc_dt_ids,
+ .pm = pm_ptr(&dc_dprc_pm_ops),
+ },
+};
diff --git a/drivers/gpu/drm/imx/dc/dc-dprc.h b/drivers/gpu/drm/imx/dc/dc-dprc.h
new file mode 100644
index 0000000000000000000000000000000000000000..f977858b57fec2f19775a97dc0baf011ca177c0b
--- /dev/null
+++ b/drivers/gpu/drm/imx/dc/dc-dprc.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright 2025 NXP
+ */
+
+#ifndef __DC_DPRC_H__
+#define __DC_DPRC_H__
+
+#include <linux/device.h>
+#include <linux/types.h>
+
+#include <drm/drm_fourcc.h>
+
+struct dc_dprc;
+
+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);
+
+void dc_dprc_disable_repeat_en(struct dc_dprc *dprc);
+
+void dc_dprc_disable(struct dc_dprc *dprc);
+
+void dc_dprc_disable_at_boot(struct dc_dprc *dprc);
+
+bool dc_dprc_rtram_width_supported(struct dc_dprc *dprc, unsigned int 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);
+
+#endif
diff --git a/drivers/gpu/drm/imx/dc/dc-drv.c b/drivers/gpu/drm/imx/dc/dc-drv.c
index 9bdcfc5aee976ef77bea6b3f6f3ac5f11249798f..17b9c4d0953d46be0a2cd276f06298d848fdcbdd 100644
--- a/drivers/gpu/drm/imx/dc/dc-drv.c
+++ b/drivers/gpu/drm/imx/dc/dc-drv.c
@@ -269,6 +269,7 @@ static struct platform_driver dc_driver = {
static struct platform_driver * const dc_drivers[] = {
&dc_cf_driver,
&dc_de_driver,
+ &dc_dprc_driver,
&dc_ed_driver,
&dc_fg_driver,
&dc_fl_driver,
diff --git a/drivers/gpu/drm/imx/dc/dc-drv.h b/drivers/gpu/drm/imx/dc/dc-drv.h
index 557e7d90e4ea8ca2af59027b3152163cf7f9a618..93a8ce4e7c314770b64ccb631628b7e79648c791 100644
--- a/drivers/gpu/drm/imx/dc/dc-drv.h
+++ b/drivers/gpu/drm/imx/dc/dc-drv.h
@@ -74,6 +74,7 @@ int dc_plane_init(struct dc_drm_device *dc_drm, struct dc_plane *dc_plane);
extern struct platform_driver dc_cf_driver;
extern struct platform_driver dc_de_driver;
+extern struct platform_driver dc_dprc_driver;
extern struct platform_driver dc_ed_driver;
extern struct platform_driver dc_fg_driver;
extern struct platform_driver dc_fl_driver;
diff --git a/drivers/gpu/drm/imx/dc/dc-prg.c b/drivers/gpu/drm/imx/dc/dc-prg.c
index f37bff12674ae792dc35a1f27cf754df4c372f20..ff80ec73677ffe643962529dc4b7b2057196a467 100644
--- a/drivers/gpu/drm/imx/dc/dc-prg.c
+++ b/drivers/gpu/drm/imx/dc/dc-prg.c
@@ -19,6 +19,7 @@
#include <linux/pm_runtime.h>
#include <linux/regmap.h>
+#include "dc-dprc.h"
#include "dc-prg.h"
#define SET 0x4
@@ -63,6 +64,7 @@ struct dc_prg {
struct list_head list;
struct clk_bulk_data *clks;
int num_clks;
+ struct dc_dprc *dprc;
};
static DEFINE_MUTEX(dc_prg_list_mutex);
@@ -216,6 +218,16 @@ dc_prg_lookup_by_phandle(struct device *dev, const char *name, int index)
return NULL;
}
+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
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 07/14] drm/imx: dc: Add DPR channel support
2025-09-23 2:07 ` [PATCH v2 07/14] drm/imx: dc: Add DPR channel support Liu Ying
@ 2025-09-23 18:29 ` Frank Li
2025-09-24 6:41 ` Liu Ying
2025-09-24 6:59 ` Liu Ying
0 siblings, 2 replies; 26+ messages in thread
From: Frank Li @ 2025-09-23 18:29 UTC (permalink / raw)
To: Liu Ying
Cc: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov,
dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel
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@nxp.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.
> +
> + 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().
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()
if (start) //look like only need enable when start is true
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.
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 07/14] drm/imx: dc: Add DPR channel support
2025-09-23 18:29 ` Frank Li
@ 2025-09-24 6:41 ` Liu Ying
2025-09-24 15:26 ` Frank Li
2025-09-24 6:59 ` Liu Ying
1 sibling, 1 reply; 26+ messages in thread
From: Liu Ying @ 2025-09-24 6:41 UTC (permalink / raw)
To: Frank Li
Cc: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov,
dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel
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@nxp.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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 07/14] drm/imx: dc: Add DPR channel support
2025-09-24 6:41 ` Liu Ying
@ 2025-09-24 15:26 ` Frank Li
2025-09-25 2:58 ` Liu Ying
0 siblings, 1 reply; 26+ messages in thread
From: Frank Li @ 2025-09-24 15:26 UTC (permalink / raw)
To: Liu Ying
Cc: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov,
dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel
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@nxp.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();.
> >
> > 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.
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?
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 07/14] drm/imx: dc: Add DPR channel support
2025-09-24 15:26 ` Frank Li
@ 2025-09-25 2:58 ` Liu Ying
2025-09-25 3:55 ` Frank Li
0 siblings, 1 reply; 26+ messages in thread
From: Liu Ying @ 2025-09-25 2:58 UTC (permalink / raw)
To: Frank Li
Cc: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov,
dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel
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@nxp.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.
>
>>>
>>> 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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 07/14] drm/imx: dc: Add DPR channel support
2025-09-25 2:58 ` Liu Ying
@ 2025-09-25 3:55 ` Frank Li
2025-09-25 9:09 ` Liu Ying
0 siblings, 1 reply; 26+ messages in thread
From: Frank Li @ 2025-09-25 3:55 UTC (permalink / raw)
To: Liu Ying
Cc: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov,
dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel
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@nxp.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.
>
> >
> >>>
> >>> 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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 07/14] drm/imx: dc: Add DPR channel support
2025-09-25 3:55 ` Frank Li
@ 2025-09-25 9:09 ` Liu Ying
2025-09-25 15:58 ` Frank Li
0 siblings, 1 reply; 26+ messages in thread
From: Liu Ying @ 2025-09-25 9:09 UTC (permalink / raw)
To: Frank Li
Cc: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov,
dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel
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@nxp.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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 07/14] drm/imx: dc: Add DPR channel support
2025-09-25 9:09 ` Liu Ying
@ 2025-09-25 15:58 ` Frank Li
0 siblings, 0 replies; 26+ messages in thread
From: Frank Li @ 2025-09-25 15:58 UTC (permalink / raw)
To: Liu Ying
Cc: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov,
dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel
On Thu, Sep 25, 2025 at 05:09:24PM +0800, Liu Ying wrote:
> 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@nxp.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.
Okay, it is not big issue. we can improve later, especially on going patch
https://lore.kernel.org/all/2399578.ElGaqSPkdT@rafael.j.wysocki/
Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> >
> >>
> >>>
> >>>>>
> >>>>> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 07/14] drm/imx: dc: Add DPR channel support
2025-09-23 18:29 ` Frank Li
2025-09-24 6:41 ` Liu Ying
@ 2025-09-24 6:59 ` Liu Ying
1 sibling, 0 replies; 26+ messages in thread
From: Liu Ying @ 2025-09-24 6:59 UTC (permalink / raw)
To: Frank Li
Cc: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov,
dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel
On 09/23/2025, Frank Li wrote:
> On Tue, Sep 23, 2025 at 10:07:57AM +0800, Liu Ying wrote:
[...]
>> +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().
The problem is that once drm_dev_unplug() is called from dc_drm_unbind()
due to imx8_dc_drm module removal, the RPM status of DC, DPRC and PRG is
"unsupported", which doesn't reflect the fact that the display controller
is running if it is before the removal. When the module is installed
again, these clocks need to be disabled out of the RPM management to
avoid clock enable/prepare count leaks if necessary.
See kerneldoc in drm_drv.c:
/*
* DOC: driver instance overview
...
* Drivers that want to support device unplugging (USB, DT overlay unload) should
* use drm_dev_unplug() instead of drm_dev_unregister(). The driver must protect
* regions that is accessing device resources to prevent use after they're
* released. This is done using drm_dev_enter() and drm_dev_exit(). There is one
* shortcoming however, drm_dev_unplug() marks the drm_device as unplugged before
* drm_atomic_helper_shutdown() is called. This means that if the disable code
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
* paths are protected, they will not run on regular driver module unload,
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
* possibly leaving the hardware enabled.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
*/
>
> 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");
>> +}
--
Regards,
Liu Ying
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 08/14] drm/imx: dc: Use TCON operation mode
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
` (6 preceding siblings ...)
2025-09-23 2:07 ` [PATCH v2 07/14] drm/imx: dc: Add DPR channel support Liu Ying
@ 2025-09-23 2:07 ` Liu Ying
2025-09-23 18:30 ` Frank Li
2025-09-23 2:07 ` [PATCH v2 09/14] drm/imx: dc-ed: Support getting source selection Liu Ying
` (5 subsequent siblings)
13 siblings, 1 reply; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:07 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying
In TCON operation mode, sync signals from FrameGen are ignored, but
a much more customized output timing can be generated by the TCON
module. By using TCON operaton mode, generate KACHUNK signal along
with HSYNC/VSYNC/data enable signals. The KACHUNK signal is used as
a synchronization signal inside the prefetch engine(DPRC + PRG(s),
attached to FetchUnit(s)). Carefully switch TCON bypass mode to TCON
operation mode when CRTC is being enabled so that the prefetch engine
may evade the first dumb frame generated by the display controller.
Since TCON BYPASS bit is controlled by KMS driver when doing atomic
commits, drop the bit setting when initializing TCON. This also
avoids accidentally initializing TCON BYPASS bit to 1 when driver
module removing and re-installing where an upcoming patch would
disable a CRTC at boot in TCON operation mode if needed.
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
drivers/gpu/drm/imx/dc/dc-crtc.c | 28 ++++++++++
drivers/gpu/drm/imx/dc/dc-de.h | 2 +
drivers/gpu/drm/imx/dc/dc-kms.h | 2 +
drivers/gpu/drm/imx/dc/dc-tc.c | 114 +++++++++++++++++++++++++++++++++++++--
4 files changed, 142 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
index 45a87df1ad6a8bd768aa5ed38d6f03f14052b3d7..9e9e86cd5202bcb0bb4d5627dbcefcc3f4e2ead0 100644
--- a/drivers/gpu/drm/imx/dc/dc-crtc.c
+++ b/drivers/gpu/drm/imx/dc/dc-crtc.c
@@ -6,8 +6,10 @@
#include <linux/completion.h>
#include <linux/container_of.h>
#include <linux/interrupt.h>
+#include <linux/irqflags.h>
#include <linux/irqreturn.h>
#include <linux/pm_runtime.h>
+#include <linux/preempt.h>
#include <linux/spinlock.h>
#include <drm/drm_atomic.h>
@@ -68,6 +70,14 @@ do { \
__func__); \
} while (0)
+#define DC_CRTC_WAIT_FOR_FRAMEGEN_FRAME_INDEX_MOVING(fg) \
+do { \
+ if (!dc_fg_wait_for_frame_index_moving(fg)) \
+ dc_crtc_err(crtc, \
+ "%s: FrameGen frame index isn't moving\n", \
+ __func__); \
+} while (0)
+
static inline struct dc_crtc *to_dc_crtc(struct drm_crtc *crtc)
{
return container_of(crtc, struct dc_crtc, base);
@@ -229,6 +239,7 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
struct drm_display_mode *adj = &new_crtc_state->adjusted_mode;
struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
enum dc_link_id cf_link;
+ unsigned long flags;
int idx, ret;
dc_crtc_dbg(crtc, "mode " DRM_MODE_FMT "\n", DRM_MODE_ARG(adj));
@@ -249,6 +260,7 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
enable_irq(dc_crtc->irq_ed_safe_shdload);
dc_fg_cfg_videomode(dc_crtc->fg, adj);
+ dc_tc_cfg_videomode(dc_crtc->tc, adj);
dc_cf_framedimensions(dc_crtc->cf_cont,
adj->crtc_hdisplay, adj->crtc_vdisplay);
@@ -273,7 +285,22 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
dc_ed_pec_sync_trigger(dc_crtc->ed_cont);
dc_ed_pec_sync_trigger(dc_crtc->ed_safe);
dc_fg_shdtokgen(dc_crtc->fg);
+
+ /* Don't relinquish CPU until TCON is set to operation mode. */
+ local_irq_save(flags);
+ preempt_disable();
+
dc_fg_enable(dc_crtc->fg);
+ /*
+ * Turn TCON into operation mode as soon as the first dumb
+ * frame is generated by DC(we don't relinquish CPU to ensure
+ * this). This makes DPR/PRG be able to evade the frame.
+ */
+ DC_CRTC_WAIT_FOR_FRAMEGEN_FRAME_INDEX_MOVING(dc_crtc->fg);
+ dc_tc_set_operation_mode(dc_crtc->tc);
+
+ local_irq_restore(flags);
+ preempt_enable();
DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(ed_safe_shdload_done);
DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(ed_cont_shdload_done);
@@ -561,6 +588,7 @@ int dc_crtc_init(struct dc_drm_device *dc_drm, int crtc_index)
dc_crtc->ed_cont = pe->ed_cont[crtc_index];
dc_crtc->ed_safe = pe->ed_safe[crtc_index];
dc_crtc->fg = de->fg;
+ dc_crtc->tc = de->tc;
dc_crtc->irq_dec_framecomplete = de->irq_framecomplete;
dc_crtc->irq_dec_seqcomplete = de->irq_seqcomplete;
diff --git a/drivers/gpu/drm/imx/dc/dc-de.h b/drivers/gpu/drm/imx/dc/dc-de.h
index 211f3fcc1a9ad642617d3b22e35ea923f75e645b..c39f2ef5eea98c3eb6ae9b5392f9bf9f7e33e7c5 100644
--- a/drivers/gpu/drm/imx/dc/dc-de.h
+++ b/drivers/gpu/drm/imx/dc/dc-de.h
@@ -54,6 +54,8 @@ enum drm_mode_status dc_fg_check_clock(struct dc_fg *fg, int clk_khz);
void dc_fg_init(struct dc_fg *fg);
/* Timing Controller Unit */
+void dc_tc_set_operation_mode(struct dc_tc *tc);
+void dc_tc_cfg_videomode(struct dc_tc *tc, struct drm_display_mode *m);
void dc_tc_init(struct dc_tc *tc);
#endif /* __DC_DISPLAY_ENGINE_H__ */
diff --git a/drivers/gpu/drm/imx/dc/dc-kms.h b/drivers/gpu/drm/imx/dc/dc-kms.h
index cd7860eff986a272f6983ad0f3cc87dbf40c2851..a25d47eebd28792e4b53b4ecc89907ce00430c2c 100644
--- a/drivers/gpu/drm/imx/dc/dc-kms.h
+++ b/drivers/gpu/drm/imx/dc/dc-kms.h
@@ -50,6 +50,8 @@ struct dc_crtc {
struct dc_ed *ed_safe;
/** @fg: framegen */
struct dc_fg *fg;
+ /** @tc: tcon */
+ struct dc_tc *tc;
/**
* @irq_dec_framecomplete:
*
diff --git a/drivers/gpu/drm/imx/dc/dc-tc.c b/drivers/gpu/drm/imx/dc/dc-tc.c
index 0bfd381b2cea15444c399f3ad261e2d061ea1c9f..6f1dc71f1b40cb4d99ca177172bd0066f39e8314 100644
--- a/drivers/gpu/drm/imx/dc/dc-tc.c
+++ b/drivers/gpu/drm/imx/dc/dc-tc.c
@@ -9,11 +9,30 @@
#include <linux/platform_device.h>
#include <linux/regmap.h>
+#include <drm/drm_modes.h>
+
#include "dc-drv.h"
#include "dc-de.h"
#define TCON_CTRL 0x410
-#define CTRL_RST_VAL 0x01401408
+#define SPLITPOSITION_MASK GENMASK(29, 16)
+#define SPLITPOSITION(n) FIELD_PREP(SPLITPOSITION_MASK, (n))
+#define DUAL_SWAP BIT(15)
+#define MINILVDS_OPCODE_MASK GENMASK(14, 12)
+#define MODE_4PAIRS FIELD_PREP(MINILVDS_OPCODE_MASK, 0x1)
+#define LVDS_CLOCK_INV BIT(11)
+#define LVDS_BALANCE BIT(10)
+#define LVDSMODE BIT(9)
+#define ENLVDS BIT(8)
+#define INV_CTRL_MASK GENMASK(7, 4)
+#define BYPASS BIT(3)
+#define TCON_SYNC BIT(2)
+#define CHANNELMODE_MASK GENMASK(1, 0)
+#define CTRL_RST_MASK \
+ (SPLITPOSITION_MASK | DUAL_SWAP | MINILVDS_OPCODE_MASK | \
+ LVDS_CLOCK_INV | LVDS_BALANCE | LVDSMODE | ENLVDS | \
+ INV_CTRL_MASK | TCON_SYNC | CHANNELMODE_MASK)
+#define CTRL_RST_VAL (SPLITPOSITION(0x140) | MODE_4PAIRS | LVDS_BALANCE)
/* red: MAPBIT 29-20, green: MAPBIT 19-10, blue: MAPBIT 9-0 */
#define MAPBIT3_0 0x418
@@ -25,6 +44,16 @@
#define MAPBIT27_24 0x430
#define MAPBIT31_28 0x434
+#define SPGPOSON(n) (0x460 + (n) * 16)
+#define SPGMASKON(n) (0x464 + (n) * 16)
+#define SPGPOSOFF(n) (0x468 + (n) * 16)
+#define SPGMASKOFF(n) (0x46c + (n) * 16)
+#define X(n) FIELD_PREP(GENMASK(30, 16), (n))
+#define Y(n) FIELD_PREP(GENMASK(14, 0), (n))
+
+#define SMXSIGS(n) (0x520 + (n) * 8)
+#define SMXFCTTABLE(n) (0x524 + (n) * 8)
+
static const struct dc_subdev_info dc_tc_info[] = {
{ .reg_start = 0x5618c800, .id = 0, },
{ .reg_start = 0x5618e400, .id = 1, },
@@ -33,6 +62,8 @@ static const struct dc_subdev_info dc_tc_info[] = {
static const struct regmap_range dc_tc_regmap_ranges[] = {
regmap_reg_range(TCON_CTRL, TCON_CTRL),
regmap_reg_range(MAPBIT3_0, MAPBIT31_28),
+ regmap_reg_range(SPGPOSON(0), SPGMASKOFF(4)),
+ regmap_reg_range(SMXSIGS(0), SMXFCTTABLE(3)),
};
static const struct regmap_access_table dc_tc_regmap_access_table = {
@@ -47,7 +78,7 @@ static const struct regmap_config dc_tc_regmap_config = {
.fast_io = true,
.wr_table = &dc_tc_regmap_access_table,
.rd_table = &dc_tc_regmap_access_table,
- .max_register = MAPBIT31_28,
+ .max_register = SMXFCTTABLE(3),
};
/*
@@ -60,10 +91,85 @@ static const u32 dc_tc_mapbit[] = {
0x13121110, 0x03020100, 0x07060504, 0x00000908,
};
+void dc_tc_set_operation_mode(struct dc_tc *tc)
+{
+ regmap_write_bits(tc->reg, TCON_CTRL, BYPASS, 0);
+}
+
+void dc_tc_cfg_videomode(struct dc_tc *tc, struct drm_display_mode *m)
+{
+ int hdisplay, hsync_start, hsync_end;
+ int vdisplay, vsync_start, vsync_end;
+ int y;
+
+ hdisplay = m->hdisplay;
+ vdisplay = m->vdisplay;
+ hsync_start = m->hsync_start;
+ vsync_start = m->vsync_start;
+ hsync_end = m->hsync_end;
+ vsync_end = m->vsync_end;
+
+ /*
+ * Turn TCON into operation mode later after the first dumb frame is
+ * generated by DC. This makes DPR/PRG be able to evade the frame.
+ */
+ regmap_write_bits(tc->reg, TCON_CTRL, BYPASS, BYPASS);
+
+ /* dsp_control[0]: HSYNC */
+ regmap_write(tc->reg, SPGPOSON(0), X(hsync_start));
+ regmap_write(tc->reg, SPGMASKON(0), 0xffff);
+
+ regmap_write(tc->reg, SPGPOSOFF(0), X(hsync_end));
+ regmap_write(tc->reg, SPGMASKOFF(0), 0xffff);
+
+ regmap_write(tc->reg, SMXSIGS(0), 0x2);
+ regmap_write(tc->reg, SMXFCTTABLE(0), 0x1);
+
+ /* dsp_control[1]: VSYNC */
+ regmap_write(tc->reg, SPGPOSON(1), X(hsync_start) | Y(vsync_start - 1));
+ regmap_write(tc->reg, SPGMASKON(1), 0x0);
+
+ regmap_write(tc->reg, SPGPOSOFF(1), X(hsync_start) | Y(vsync_end - 1));
+ regmap_write(tc->reg, SPGMASKOFF(1), 0x0);
+
+ regmap_write(tc->reg, SMXSIGS(1), 0x3);
+ regmap_write(tc->reg, SMXFCTTABLE(1), 0x1);
+
+ /* dsp_control[2]: data enable */
+ /* horizontal */
+ regmap_write(tc->reg, SPGPOSON(2), 0x0);
+ regmap_write(tc->reg, SPGMASKON(2), 0xffff);
+
+ regmap_write(tc->reg, SPGPOSOFF(2), X(hdisplay));
+ regmap_write(tc->reg, SPGMASKOFF(2), 0xffff);
+
+ /* vertical */
+ regmap_write(tc->reg, SPGPOSON(3), 0x0);
+ regmap_write(tc->reg, SPGMASKON(3), 0x7fff0000);
+
+ regmap_write(tc->reg, SPGPOSOFF(3), Y(vdisplay));
+ regmap_write(tc->reg, SPGMASKOFF(3), 0x7fff0000);
+
+ regmap_write(tc->reg, SMXSIGS(2), 0x2c);
+ regmap_write(tc->reg, SMXFCTTABLE(2), 0x8);
+
+ /* dsp_control[3]: KACHUNK */
+ y = vdisplay + 1;
+
+ regmap_write(tc->reg, SPGPOSON(4), X(0x0) | Y(y));
+ regmap_write(tc->reg, SPGMASKON(4), 0x0);
+
+ regmap_write(tc->reg, SPGPOSOFF(4), X(0x20) | Y(y));
+ regmap_write(tc->reg, SPGMASKOFF(4), 0x0);
+
+ regmap_write(tc->reg, SMXSIGS(3), 0x6);
+ regmap_write(tc->reg, SMXFCTTABLE(3), 0x2);
+}
+
void dc_tc_init(struct dc_tc *tc)
{
- /* reset TCON_CTRL to POR default so that TCON works in bypass mode */
- regmap_write(tc->reg, TCON_CTRL, CTRL_RST_VAL);
+ /* reset TCON_CTRL to POR default except for touching BYPASS bit */
+ regmap_write_bits(tc->reg, TCON_CTRL, CTRL_RST_MASK, CTRL_RST_VAL);
/* set format */
regmap_bulk_write(tc->reg, MAPBIT3_0, dc_tc_mapbit,
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 08/14] drm/imx: dc: Use TCON operation mode
2025-09-23 2:07 ` [PATCH v2 08/14] drm/imx: dc: Use TCON operation mode Liu Ying
@ 2025-09-23 18:30 ` Frank Li
0 siblings, 0 replies; 26+ messages in thread
From: Frank Li @ 2025-09-23 18:30 UTC (permalink / raw)
To: Liu Ying
Cc: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov,
dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel
On Tue, Sep 23, 2025 at 10:07:58AM +0800, Liu Ying wrote:
> In TCON operation mode, sync signals from FrameGen are ignored, but
> a much more customized output timing can be generated by the TCON
> module. By using TCON operaton mode, generate KACHUNK signal along
> with HSYNC/VSYNC/data enable signals. The KACHUNK signal is used as
> a synchronization signal inside the prefetch engine(DPRC + PRG(s),
> attached to FetchUnit(s)). Carefully switch TCON bypass mode to TCON
> operation mode when CRTC is being enabled so that the prefetch engine
> may evade the first dumb frame generated by the display controller.
>
> Since TCON BYPASS bit is controlled by KMS driver when doing atomic
> commits, drop the bit setting when initializing TCON. This also
> avoids accidentally initializing TCON BYPASS bit to 1 when driver
> module removing and re-installing where an upcoming patch would
> disable a CRTC at boot in TCON operation mode if needed.
>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
> drivers/gpu/drm/imx/dc/dc-crtc.c | 28 ++++++++++
> drivers/gpu/drm/imx/dc/dc-de.h | 2 +
> drivers/gpu/drm/imx/dc/dc-kms.h | 2 +
> drivers/gpu/drm/imx/dc/dc-tc.c | 114 +++++++++++++++++++++++++++++++++++++--
> 4 files changed, 142 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
> index 45a87df1ad6a8bd768aa5ed38d6f03f14052b3d7..9e9e86cd5202bcb0bb4d5627dbcefcc3f4e2ead0 100644
> --- a/drivers/gpu/drm/imx/dc/dc-crtc.c
> +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c
> @@ -6,8 +6,10 @@
> #include <linux/completion.h>
> #include <linux/container_of.h>
> #include <linux/interrupt.h>
> +#include <linux/irqflags.h>
> #include <linux/irqreturn.h>
> #include <linux/pm_runtime.h>
> +#include <linux/preempt.h>
> #include <linux/spinlock.h>
>
> #include <drm/drm_atomic.h>
> @@ -68,6 +70,14 @@ do { \
> __func__); \
> } while (0)
>
> +#define DC_CRTC_WAIT_FOR_FRAMEGEN_FRAME_INDEX_MOVING(fg) \
> +do { \
> + if (!dc_fg_wait_for_frame_index_moving(fg)) \
> + dc_crtc_err(crtc, \
> + "%s: FrameGen frame index isn't moving\n", \
> + __func__); \
> +} while (0)
> +
> static inline struct dc_crtc *to_dc_crtc(struct drm_crtc *crtc)
> {
> return container_of(crtc, struct dc_crtc, base);
> @@ -229,6 +239,7 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
> struct drm_display_mode *adj = &new_crtc_state->adjusted_mode;
> struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
> enum dc_link_id cf_link;
> + unsigned long flags;
> int idx, ret;
>
> dc_crtc_dbg(crtc, "mode " DRM_MODE_FMT "\n", DRM_MODE_ARG(adj));
> @@ -249,6 +260,7 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
> enable_irq(dc_crtc->irq_ed_safe_shdload);
>
> dc_fg_cfg_videomode(dc_crtc->fg, adj);
> + dc_tc_cfg_videomode(dc_crtc->tc, adj);
>
> dc_cf_framedimensions(dc_crtc->cf_cont,
> adj->crtc_hdisplay, adj->crtc_vdisplay);
> @@ -273,7 +285,22 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
> dc_ed_pec_sync_trigger(dc_crtc->ed_cont);
> dc_ed_pec_sync_trigger(dc_crtc->ed_safe);
> dc_fg_shdtokgen(dc_crtc->fg);
> +
> + /* Don't relinquish CPU until TCON is set to operation mode. */
> + local_irq_save(flags);
> + preempt_disable();
> +
> dc_fg_enable(dc_crtc->fg);
> + /*
> + * Turn TCON into operation mode as soon as the first dumb
> + * frame is generated by DC(we don't relinquish CPU to ensure
> + * this). This makes DPR/PRG be able to evade the frame.
> + */
> + DC_CRTC_WAIT_FOR_FRAMEGEN_FRAME_INDEX_MOVING(dc_crtc->fg);
> + dc_tc_set_operation_mode(dc_crtc->tc);
> +
> + local_irq_restore(flags);
> + preempt_enable();
>
> DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(ed_safe_shdload_done);
> DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(ed_cont_shdload_done);
> @@ -561,6 +588,7 @@ int dc_crtc_init(struct dc_drm_device *dc_drm, int crtc_index)
> dc_crtc->ed_cont = pe->ed_cont[crtc_index];
> dc_crtc->ed_safe = pe->ed_safe[crtc_index];
> dc_crtc->fg = de->fg;
> + dc_crtc->tc = de->tc;
>
> dc_crtc->irq_dec_framecomplete = de->irq_framecomplete;
> dc_crtc->irq_dec_seqcomplete = de->irq_seqcomplete;
> diff --git a/drivers/gpu/drm/imx/dc/dc-de.h b/drivers/gpu/drm/imx/dc/dc-de.h
> index 211f3fcc1a9ad642617d3b22e35ea923f75e645b..c39f2ef5eea98c3eb6ae9b5392f9bf9f7e33e7c5 100644
> --- a/drivers/gpu/drm/imx/dc/dc-de.h
> +++ b/drivers/gpu/drm/imx/dc/dc-de.h
> @@ -54,6 +54,8 @@ enum drm_mode_status dc_fg_check_clock(struct dc_fg *fg, int clk_khz);
> void dc_fg_init(struct dc_fg *fg);
>
> /* Timing Controller Unit */
> +void dc_tc_set_operation_mode(struct dc_tc *tc);
> +void dc_tc_cfg_videomode(struct dc_tc *tc, struct drm_display_mode *m);
> void dc_tc_init(struct dc_tc *tc);
>
> #endif /* __DC_DISPLAY_ENGINE_H__ */
> diff --git a/drivers/gpu/drm/imx/dc/dc-kms.h b/drivers/gpu/drm/imx/dc/dc-kms.h
> index cd7860eff986a272f6983ad0f3cc87dbf40c2851..a25d47eebd28792e4b53b4ecc89907ce00430c2c 100644
> --- a/drivers/gpu/drm/imx/dc/dc-kms.h
> +++ b/drivers/gpu/drm/imx/dc/dc-kms.h
> @@ -50,6 +50,8 @@ struct dc_crtc {
> struct dc_ed *ed_safe;
> /** @fg: framegen */
> struct dc_fg *fg;
> + /** @tc: tcon */
> + struct dc_tc *tc;
> /**
> * @irq_dec_framecomplete:
> *
> diff --git a/drivers/gpu/drm/imx/dc/dc-tc.c b/drivers/gpu/drm/imx/dc/dc-tc.c
> index 0bfd381b2cea15444c399f3ad261e2d061ea1c9f..6f1dc71f1b40cb4d99ca177172bd0066f39e8314 100644
> --- a/drivers/gpu/drm/imx/dc/dc-tc.c
> +++ b/drivers/gpu/drm/imx/dc/dc-tc.c
> @@ -9,11 +9,30 @@
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
>
> +#include <drm/drm_modes.h>
> +
> #include "dc-drv.h"
> #include "dc-de.h"
>
> #define TCON_CTRL 0x410
> -#define CTRL_RST_VAL 0x01401408
> +#define SPLITPOSITION_MASK GENMASK(29, 16)
> +#define SPLITPOSITION(n) FIELD_PREP(SPLITPOSITION_MASK, (n))
> +#define DUAL_SWAP BIT(15)
> +#define MINILVDS_OPCODE_MASK GENMASK(14, 12)
> +#define MODE_4PAIRS FIELD_PREP(MINILVDS_OPCODE_MASK, 0x1)
> +#define LVDS_CLOCK_INV BIT(11)
> +#define LVDS_BALANCE BIT(10)
> +#define LVDSMODE BIT(9)
> +#define ENLVDS BIT(8)
> +#define INV_CTRL_MASK GENMASK(7, 4)
> +#define BYPASS BIT(3)
> +#define TCON_SYNC BIT(2)
> +#define CHANNELMODE_MASK GENMASK(1, 0)
> +#define CTRL_RST_MASK \
> + (SPLITPOSITION_MASK | DUAL_SWAP | MINILVDS_OPCODE_MASK | \
> + LVDS_CLOCK_INV | LVDS_BALANCE | LVDSMODE | ENLVDS | \
> + INV_CTRL_MASK | TCON_SYNC | CHANNELMODE_MASK)
> +#define CTRL_RST_VAL (SPLITPOSITION(0x140) | MODE_4PAIRS | LVDS_BALANCE)
>
> /* red: MAPBIT 29-20, green: MAPBIT 19-10, blue: MAPBIT 9-0 */
> #define MAPBIT3_0 0x418
> @@ -25,6 +44,16 @@
> #define MAPBIT27_24 0x430
> #define MAPBIT31_28 0x434
>
> +#define SPGPOSON(n) (0x460 + (n) * 16)
> +#define SPGMASKON(n) (0x464 + (n) * 16)
> +#define SPGPOSOFF(n) (0x468 + (n) * 16)
> +#define SPGMASKOFF(n) (0x46c + (n) * 16)
> +#define X(n) FIELD_PREP(GENMASK(30, 16), (n))
> +#define Y(n) FIELD_PREP(GENMASK(14, 0), (n))
> +
> +#define SMXSIGS(n) (0x520 + (n) * 8)
> +#define SMXFCTTABLE(n) (0x524 + (n) * 8)
> +
> static const struct dc_subdev_info dc_tc_info[] = {
> { .reg_start = 0x5618c800, .id = 0, },
> { .reg_start = 0x5618e400, .id = 1, },
> @@ -33,6 +62,8 @@ static const struct dc_subdev_info dc_tc_info[] = {
> static const struct regmap_range dc_tc_regmap_ranges[] = {
> regmap_reg_range(TCON_CTRL, TCON_CTRL),
> regmap_reg_range(MAPBIT3_0, MAPBIT31_28),
> + regmap_reg_range(SPGPOSON(0), SPGMASKOFF(4)),
> + regmap_reg_range(SMXSIGS(0), SMXFCTTABLE(3)),
> };
>
> static const struct regmap_access_table dc_tc_regmap_access_table = {
> @@ -47,7 +78,7 @@ static const struct regmap_config dc_tc_regmap_config = {
> .fast_io = true,
> .wr_table = &dc_tc_regmap_access_table,
> .rd_table = &dc_tc_regmap_access_table,
> - .max_register = MAPBIT31_28,
> + .max_register = SMXFCTTABLE(3),
> };
>
> /*
> @@ -60,10 +91,85 @@ static const u32 dc_tc_mapbit[] = {
> 0x13121110, 0x03020100, 0x07060504, 0x00000908,
> };
>
> +void dc_tc_set_operation_mode(struct dc_tc *tc)
> +{
> + regmap_write_bits(tc->reg, TCON_CTRL, BYPASS, 0);
> +}
> +
> +void dc_tc_cfg_videomode(struct dc_tc *tc, struct drm_display_mode *m)
> +{
> + int hdisplay, hsync_start, hsync_end;
> + int vdisplay, vsync_start, vsync_end;
> + int y;
> +
> + hdisplay = m->hdisplay;
> + vdisplay = m->vdisplay;
> + hsync_start = m->hsync_start;
> + vsync_start = m->vsync_start;
> + hsync_end = m->hsync_end;
> + vsync_end = m->vsync_end;
> +
> + /*
> + * Turn TCON into operation mode later after the first dumb frame is
> + * generated by DC. This makes DPR/PRG be able to evade the frame.
> + */
> + regmap_write_bits(tc->reg, TCON_CTRL, BYPASS, BYPASS);
> +
> + /* dsp_control[0]: HSYNC */
> + regmap_write(tc->reg, SPGPOSON(0), X(hsync_start));
> + regmap_write(tc->reg, SPGMASKON(0), 0xffff);
> +
> + regmap_write(tc->reg, SPGPOSOFF(0), X(hsync_end));
> + regmap_write(tc->reg, SPGMASKOFF(0), 0xffff);
> +
> + regmap_write(tc->reg, SMXSIGS(0), 0x2);
> + regmap_write(tc->reg, SMXFCTTABLE(0), 0x1);
> +
> + /* dsp_control[1]: VSYNC */
> + regmap_write(tc->reg, SPGPOSON(1), X(hsync_start) | Y(vsync_start - 1));
> + regmap_write(tc->reg, SPGMASKON(1), 0x0);
> +
> + regmap_write(tc->reg, SPGPOSOFF(1), X(hsync_start) | Y(vsync_end - 1));
> + regmap_write(tc->reg, SPGMASKOFF(1), 0x0);
> +
> + regmap_write(tc->reg, SMXSIGS(1), 0x3);
> + regmap_write(tc->reg, SMXFCTTABLE(1), 0x1);
> +
> + /* dsp_control[2]: data enable */
> + /* horizontal */
> + regmap_write(tc->reg, SPGPOSON(2), 0x0);
> + regmap_write(tc->reg, SPGMASKON(2), 0xffff);
> +
> + regmap_write(tc->reg, SPGPOSOFF(2), X(hdisplay));
> + regmap_write(tc->reg, SPGMASKOFF(2), 0xffff);
> +
> + /* vertical */
> + regmap_write(tc->reg, SPGPOSON(3), 0x0);
> + regmap_write(tc->reg, SPGMASKON(3), 0x7fff0000);
> +
> + regmap_write(tc->reg, SPGPOSOFF(3), Y(vdisplay));
> + regmap_write(tc->reg, SPGMASKOFF(3), 0x7fff0000);
> +
> + regmap_write(tc->reg, SMXSIGS(2), 0x2c);
> + regmap_write(tc->reg, SMXFCTTABLE(2), 0x8);
> +
> + /* dsp_control[3]: KACHUNK */
> + y = vdisplay + 1;
> +
> + regmap_write(tc->reg, SPGPOSON(4), X(0x0) | Y(y));
> + regmap_write(tc->reg, SPGMASKON(4), 0x0);
> +
> + regmap_write(tc->reg, SPGPOSOFF(4), X(0x20) | Y(y));
> + regmap_write(tc->reg, SPGMASKOFF(4), 0x0);
> +
> + regmap_write(tc->reg, SMXSIGS(3), 0x6);
> + regmap_write(tc->reg, SMXFCTTABLE(3), 0x2);
> +}
> +
> void dc_tc_init(struct dc_tc *tc)
> {
> - /* reset TCON_CTRL to POR default so that TCON works in bypass mode */
> - regmap_write(tc->reg, TCON_CTRL, CTRL_RST_VAL);
> + /* reset TCON_CTRL to POR default except for touching BYPASS bit */
> + regmap_write_bits(tc->reg, TCON_CTRL, CTRL_RST_MASK, CTRL_RST_VAL);
>
> /* set format */
> regmap_bulk_write(tc->reg, MAPBIT3_0, dc_tc_mapbit,
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 09/14] drm/imx: dc-ed: Support getting source selection
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
` (7 preceding siblings ...)
2025-09-23 2:07 ` [PATCH v2 08/14] drm/imx: dc: Use TCON operation mode Liu Ying
@ 2025-09-23 2:07 ` Liu Ying
2025-09-23 2:08 ` [PATCH v2 10/14] drm/imx: dc-lb: Support getting secondary input selection Liu Ying
` (4 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:07 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying, Frank Li
Add a helper to get ExtDst source selection. This is needed by
disabling CRTC at boot in an upcoming commit which tries to get
the source selection.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v2:
- Collect Frank's R-b tag.
---
drivers/gpu/drm/imx/dc/dc-ed.c | 26 ++++++++++++++++++++++++++
drivers/gpu/drm/imx/dc/dc-pe.h | 1 +
2 files changed, 27 insertions(+)
diff --git a/drivers/gpu/drm/imx/dc/dc-ed.c b/drivers/gpu/drm/imx/dc/dc-ed.c
index 86ecc22d0a554bf3eced218df0312b513e10e179..b514790bb765329295553f89d16ee1167ef411dd 100644
--- a/drivers/gpu/drm/imx/dc/dc-ed.c
+++ b/drivers/gpu/drm/imx/dc/dc-ed.c
@@ -150,6 +150,32 @@ void dc_ed_pec_src_sel(struct dc_ed *ed, enum dc_link_id src)
}
}
+int dc_ed_pec_src_sel_get(struct dc_ed *ed, enum dc_link_id *src)
+{
+ u32 val;
+
+ regmap_read(ed->reg_pec, PIXENGCFG_DYNAMIC, &val);
+
+ switch (val) {
+ case LINK_ID_NONE:
+ case LINK_ID_CONSTFRAME0:
+ case LINK_ID_CONSTFRAME4:
+ case LINK_ID_CONSTFRAME1:
+ case LINK_ID_CONSTFRAME5:
+ case LINK_ID_FETCHWARP2:
+ case LINK_ID_FETCHLAYER0:
+ case LINK_ID_LAYERBLEND0:
+ case LINK_ID_LAYERBLEND1:
+ case LINK_ID_LAYERBLEND2:
+ case LINK_ID_LAYERBLEND3:
+ *src = val;
+ dev_dbg(ed->dev, "get source selection: 0x%02x\n", val);
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
void dc_ed_pec_sync_trigger(struct dc_ed *ed)
{
regmap_write(ed->reg_pec, PIXENGCFG_TRIGGER, SYNC_TRIGGER);
diff --git a/drivers/gpu/drm/imx/dc/dc-pe.h b/drivers/gpu/drm/imx/dc/dc-pe.h
index f5e01a6eb9e918516adfc89949665fdd2ff1e40e..0b81dc6b251c452e7ca72f1cd12af065bafaa198 100644
--- a/drivers/gpu/drm/imx/dc/dc-pe.h
+++ b/drivers/gpu/drm/imx/dc/dc-pe.h
@@ -85,6 +85,7 @@ void dc_cf_init(struct dc_cf *cf);
/* External Destination Unit */
void dc_ed_pec_src_sel(struct dc_ed *ed, enum dc_link_id src);
+int dc_ed_pec_src_sel_get(struct dc_ed *ed, enum dc_link_id *src);
void dc_ed_pec_sync_trigger(struct dc_ed *ed);
void dc_ed_init(struct dc_ed *ed);
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 10/14] drm/imx: dc-lb: Support getting secondary input selection
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
` (8 preceding siblings ...)
2025-09-23 2:07 ` [PATCH v2 09/14] drm/imx: dc-ed: Support getting source selection Liu Ying
@ 2025-09-23 2:08 ` Liu Ying
2025-09-23 2:08 ` [PATCH v2 11/14] drm/imx: dc-ed: Drop initial source selection Liu Ying
` (3 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:08 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying, Frank Li
Add a helper to get LayerBlend secondary input selection. This is
needed by disabling CRTC at boot in an upcoming commit which tries
to get the selection.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v2:
- Collect Frank's R-b tag.
---
drivers/gpu/drm/imx/dc/dc-lb.c | 21 +++++++++++++++++++++
drivers/gpu/drm/imx/dc/dc-pe.h | 1 +
2 files changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/imx/dc/dc-lb.c b/drivers/gpu/drm/imx/dc/dc-lb.c
index 38f966625d382487e45c2e0c202e15c1417f6e65..b0f3fb818799b58597e8ae41d868338c3fd40800 100644
--- a/drivers/gpu/drm/imx/dc/dc-lb.c
+++ b/drivers/gpu/drm/imx/dc/dc-lb.c
@@ -23,6 +23,8 @@
#define PIXENGCFG_DYNAMIC_SEC_SEL_MASK GENMASK(13, 8)
#define PIXENGCFG_DYNAMIC_SEC_SEL(x) \
FIELD_PREP(PIXENGCFG_DYNAMIC_SEC_SEL_MASK, (x))
+#define PIXENGCFG_DYNAMIC_SEC_SEL_GET(x) \
+ FIELD_GET(PIXENGCFG_DYNAMIC_SEC_SEL_MASK, (x))
#define STATICCONTROL 0x8
#define SHDTOKSEL_MASK GENMASK(4, 3)
@@ -182,6 +184,25 @@ void dc_lb_pec_dynamic_sec_sel(struct dc_lb *lb, enum dc_link_id sec)
dev_warn(lb->dev, "invalid secondary input selection:%d\n", sec);
}
+int dc_lb_pec_dynamic_sec_sel_get(struct dc_lb *lb, enum dc_link_id *sec)
+{
+ u32 val;
+
+ regmap_read(lb->reg_pec, PIXENGCFG_DYNAMIC, &val);
+ val = PIXENGCFG_DYNAMIC_SEC_SEL_GET(val);
+
+ switch (val) {
+ case LINK_ID_NONE:
+ case LINK_ID_FETCHWARP2:
+ case LINK_ID_FETCHLAYER0:
+ *sec = val;
+ dev_dbg(lb->dev, "get secondary input selection: 0x%02x\n", val);
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
void dc_lb_pec_clken(struct dc_lb *lb, enum dc_pec_clken clken)
{
regmap_write_bits(lb->reg_pec, PIXENGCFG_DYNAMIC, CLKEN_MASK,
diff --git a/drivers/gpu/drm/imx/dc/dc-pe.h b/drivers/gpu/drm/imx/dc/dc-pe.h
index 0b81dc6b251c452e7ca72f1cd12af065bafaa198..4e38aead69bce6e9ad4cf2a7cd1fedbaeda60650 100644
--- a/drivers/gpu/drm/imx/dc/dc-pe.h
+++ b/drivers/gpu/drm/imx/dc/dc-pe.h
@@ -93,6 +93,7 @@ void dc_ed_init(struct dc_ed *ed);
enum dc_link_id dc_lb_get_link_id(struct dc_lb *lb);
void dc_lb_pec_dynamic_prim_sel(struct dc_lb *lb, enum dc_link_id prim);
void dc_lb_pec_dynamic_sec_sel(struct dc_lb *lb, enum dc_link_id sec);
+int dc_lb_pec_dynamic_sec_sel_get(struct dc_lb *lb, enum dc_link_id *sec);
void dc_lb_pec_clken(struct dc_lb *lb, enum dc_pec_clken clken);
void dc_lb_mode(struct dc_lb *lb, enum dc_lb_mode mode);
void dc_lb_position(struct dc_lb *lb, int x, int y);
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 11/14] drm/imx: dc-ed: Drop initial source selection
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
` (9 preceding siblings ...)
2025-09-23 2:08 ` [PATCH v2 10/14] drm/imx: dc-lb: Support getting secondary input selection Liu Ying
@ 2025-09-23 2:08 ` Liu Ying
2025-09-23 2:08 ` [PATCH v2 12/14] drm/imx: dc-lb: Drop initial primary and secondary input selections Liu Ying
` (2 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:08 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying, Frank Li
It's unnecessary to set initial ExtDst source selection because KMS
driver would do that when doing atomic commits. Drop the selection.
This is needed as a preparation for reading ExtDst source selection
when trying to disable CRTC at boot in an upcoming commit.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v2:
- Collect Frank's R-b tag.
---
drivers/gpu/drm/imx/dc/dc-ed.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/dc/dc-ed.c b/drivers/gpu/drm/imx/dc/dc-ed.c
index b514790bb765329295553f89d16ee1167ef411dd..1b141ced57b895c24b6358332cbe5e308dad7c01 100644
--- a/drivers/gpu/drm/imx/dc/dc-ed.c
+++ b/drivers/gpu/drm/imx/dc/dc-ed.c
@@ -203,7 +203,6 @@ static inline void dc_ed_disable_gamma_apply(struct dc_ed *ed)
void dc_ed_init(struct dc_ed *ed)
{
- dc_ed_pec_src_sel(ed, LINK_ID_NONE);
dc_ed_pec_enable_shden(ed);
dc_ed_pec_poweron(ed);
dc_ed_pec_sync_mode_single(ed);
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 12/14] drm/imx: dc-lb: Drop initial primary and secondary input selections
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
` (10 preceding siblings ...)
2025-09-23 2:08 ` [PATCH v2 11/14] drm/imx: dc-ed: Drop initial source selection Liu Ying
@ 2025-09-23 2:08 ` Liu Ying
2025-09-23 2:08 ` [PATCH v2 13/14] drm/imx: dc-fu: Get DPR channel Liu Ying
2025-09-23 2:08 ` [PATCH v2 14/14] drm/imx: dc: Use prefetch engine Liu Ying
13 siblings, 0 replies; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:08 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying, Frank Li
It's unnecessary to set initial primary and secondary input selections
because KMS driver would do that when doing atomic commits. Drop the
selections. This is needed as a preparation for reading LayerBlend
secondary input selection when trying to disable CRTC at boot in an
upcoming commit.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v2:
- Collect Frank's R-b tag.
---
drivers/gpu/drm/imx/dc/dc-lb.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/imx/dc/dc-lb.c b/drivers/gpu/drm/imx/dc/dc-lb.c
index b0f3fb818799b58597e8ae41d868338c3fd40800..d0a805469ae79d467c7e71ebf75e1befb80e15f3 100644
--- a/drivers/gpu/drm/imx/dc/dc-lb.c
+++ b/drivers/gpu/drm/imx/dc/dc-lb.c
@@ -254,8 +254,6 @@ int dc_lb_get_id(struct dc_lb *lb)
void dc_lb_init(struct dc_lb *lb)
{
- dc_lb_pec_dynamic_prim_sel(lb, LINK_ID_NONE);
- dc_lb_pec_dynamic_sec_sel(lb, LINK_ID_NONE);
dc_lb_pec_clken(lb, CLKEN_DISABLE);
dc_lb_shdldsel(lb, BOTH);
dc_lb_shdtoksel(lb, BOTH);
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 13/14] drm/imx: dc-fu: Get DPR channel
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
` (11 preceding siblings ...)
2025-09-23 2:08 ` [PATCH v2 12/14] drm/imx: dc-lb: Drop initial primary and secondary input selections Liu Ying
@ 2025-09-23 2:08 ` Liu Ying
2025-09-23 2:08 ` [PATCH v2 14/14] drm/imx: dc: Use prefetch engine Liu Ying
13 siblings, 0 replies; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:08 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying, Frank Li
Add a helper dc_fu_get_dprc() to get DPR channel for FetchUnit.
And, call it from dc_{fl,fw)_bind() to get DPR channels for
FetchLayer and FetchWarp.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v2:
- Collect Frank's R-b tag.
---
drivers/gpu/drm/imx/dc/dc-fl.c | 7 ++++++-
drivers/gpu/drm/imx/dc/dc-fu.c | 18 ++++++++++++++++++
drivers/gpu/drm/imx/dc/dc-fu.h | 5 +++++
drivers/gpu/drm/imx/dc/dc-fw.c | 7 ++++++-
4 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imx/dc/dc-fl.c b/drivers/gpu/drm/imx/dc/dc-fl.c
index 3ce24c72aa13e8aa150c0152a5a1ea3d0fa90b7e..a1835f43da0dc5cf0c83d93f16658b485011e285 100644
--- a/drivers/gpu/drm/imx/dc/dc-fl.c
+++ b/drivers/gpu/drm/imx/dc/dc-fl.c
@@ -101,13 +101,18 @@ static int dc_fl_bind(struct device *dev, struct device *master, void *data)
void __iomem *base_cfg;
struct dc_fl *fl;
struct dc_fu *fu;
- int i, id;
+ int ret, i, id;
fl = devm_kzalloc(dev, sizeof(*fl), GFP_KERNEL);
if (!fl)
return -ENOMEM;
fu = &fl->fu;
+ fu->dev = dev;
+
+ ret = dc_fu_get_dprc(fu);
+ if (ret)
+ return ret;
res_pec = platform_get_resource(pdev, IORESOURCE_MEM, 0);
diff --git a/drivers/gpu/drm/imx/dc/dc-fu.c b/drivers/gpu/drm/imx/dc/dc-fu.c
index b36d3034b8d5962eaab212b6b65ebbb5c29f978c..47d436abb65157de7cab74565e44b199be76de52 100644
--- a/drivers/gpu/drm/imx/dc/dc-fu.c
+++ b/drivers/gpu/drm/imx/dc/dc-fu.c
@@ -10,6 +10,7 @@
#include "dc-fu.h"
#include "dc-pe.h"
+#include "dc-prg.h"
/* STATICCONTROL */
#define SHDLDREQSTICKY_MASK GENMASK(31, 24)
@@ -256,3 +257,20 @@ void dc_fu_common_hw_init(struct dc_fu *fu)
dc_fu_set_pixel_blend_mode(fu, i);
}
}
+
+int dc_fu_get_dprc(struct dc_fu *fu)
+{
+ struct dc_prg *prg;
+
+ prg = dc_prg_lookup_by_phandle(fu->dev, "fsl,prg", 0);
+ if (!prg)
+ return dev_err_probe(fu->dev, -EPROBE_DEFER,
+ "failed to lookup PRG\n");
+
+ fu->dprc = dc_prg_get_dprc(prg);
+ if (!fu->dprc)
+ return dev_err_probe(fu->dev, -EPROBE_DEFER,
+ "failed to get DPR channel\n");
+
+ return 0;
+}
diff --git a/drivers/gpu/drm/imx/dc/dc-fu.h b/drivers/gpu/drm/imx/dc/dc-fu.h
index 518d1af49f5ae9d4f67da5e6c2e80abd7e962120..44b9497e53580589a05bcb180eb2312ea4449da4 100644
--- a/drivers/gpu/drm/imx/dc/dc-fu.h
+++ b/drivers/gpu/drm/imx/dc/dc-fu.h
@@ -8,11 +8,13 @@
#include <linux/bitfield.h>
#include <linux/bits.h>
+#include <linux/device.h>
#include <linux/regmap.h>
#include <linux/types.h>
#include <drm/drm_fourcc.h>
+#include "dc-dprc.h"
#include "dc-pe.h"
#define FRAC_OFFSET 0x28
@@ -99,6 +101,7 @@ struct dc_fu_ops {
};
struct dc_fu {
+ struct device *dev;
struct regmap *reg_pec;
struct regmap *reg_cfg;
char name[21];
@@ -114,6 +117,7 @@ struct dc_fu {
enum dc_link_id link_id;
struct dc_fu_ops ops;
struct dc_lb *lb;
+ struct dc_dprc *dprc;
};
extern const struct dc_fu_ops dc_fu_common_ops;
@@ -123,6 +127,7 @@ void dc_fu_get_pixel_format_shifts(struct dc_fu *fu, u32 format, u32 *shifts);
void dc_fu_shdldreq_sticky(struct dc_fu *fu, u8 layer_mask);
void dc_fu_set_src_bpp(struct dc_fu *fu, enum dc_fu_frac frac, unsigned int bpp);
void dc_fu_common_hw_init(struct dc_fu *fu);
+int dc_fu_get_dprc(struct dc_fu *fu);
const struct dc_fu_ops *dc_fu_get_ops(struct dc_fu *fu);
diff --git a/drivers/gpu/drm/imx/dc/dc-fw.c b/drivers/gpu/drm/imx/dc/dc-fw.c
index acb2d4d9e2ecd6d36dbab050f25b560d9ec9aba5..c2e60741bc871c6976ead53f132b60f19352aab3 100644
--- a/drivers/gpu/drm/imx/dc/dc-fw.c
+++ b/drivers/gpu/drm/imx/dc/dc-fw.c
@@ -131,13 +131,18 @@ static int dc_fw_bind(struct device *dev, struct device *master, void *data)
void __iomem *base_cfg;
struct dc_fw *fw;
struct dc_fu *fu;
- int i, id;
+ int ret, i, id;
fw = devm_kzalloc(dev, sizeof(*fw), GFP_KERNEL);
if (!fw)
return -ENOMEM;
fu = &fw->fu;
+ fu->dev = dev;
+
+ ret = dc_fu_get_dprc(fu);
+ if (ret)
+ return ret;
base_pec = devm_platform_get_and_ioremap_resource(pdev, 0, &res_pec);
if (IS_ERR(base_pec))
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 14/14] drm/imx: dc: Use prefetch engine
2025-09-23 2:07 [PATCH v2 00/14] drm/imx: dc: Use prefetch engine Liu Ying
` (12 preceding siblings ...)
2025-09-23 2:08 ` [PATCH v2 13/14] drm/imx: dc-fu: Get DPR channel Liu Ying
@ 2025-09-23 2:08 ` Liu Ying
2025-09-23 18:32 ` Frank Li
13 siblings, 1 reply; 26+ messages in thread
From: Liu Ying @ 2025-09-23 2:08 UTC (permalink / raw)
To: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
Liu Ying
One prefetch engine consists of one DPR channel and one or two PRGs.
Each PRG handles one planar in a pixel format. Every FetchUnit used
by KMS may attach to a PRG and hence use a prefetch engine. So, to
simplify driver code, always use prefetch engines for FetchUnits in
KMS driver and avoid supporting bypassing them. Aside from configuring
and disabling a prefetch engine along with a FetchUnit for atomic
commits, properly disable the prefetch engine at boot and adapt burst
size/stride fixup requirements from PRG in FetchUnit driver.
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
drivers/gpu/drm/imx/dc/dc-crtc.c | 139 +++++++++++++++++++++++++++++++++++---
drivers/gpu/drm/imx/dc/dc-fu.c | 27 +++++++-
drivers/gpu/drm/imx/dc/dc-fu.h | 2 +-
drivers/gpu/drm/imx/dc/dc-kms.h | 5 ++
drivers/gpu/drm/imx/dc/dc-plane.c | 46 +++++++++++--
5 files changed, 197 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
index 9e9e86cd5202bcb0bb4d5627dbcefcc3f4e2ead0..4c7aab360616cb1c84c31c83f16df703b1c2c6d7 100644
--- a/drivers/gpu/drm/imx/dc/dc-crtc.c
+++ b/drivers/gpu/drm/imx/dc/dc-crtc.c
@@ -25,6 +25,7 @@
#include <drm/drm_vblank.h>
#include "dc-de.h"
+#include "dc-dprc.h"
#include "dc-drv.h"
#include "dc-kms.h"
#include "dc-pe.h"
@@ -204,7 +205,13 @@ dc_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_atomic_state *state)
drm_atomic_get_old_crtc_state(state, crtc);
struct drm_crtc_state *new_crtc_state =
drm_atomic_get_new_crtc_state(state, crtc);
+ struct drm_plane_state *old_plane_state =
+ drm_atomic_get_old_plane_state(state, crtc->primary);
+ struct drm_plane_state *new_plane_state =
+ drm_atomic_get_new_plane_state(state, crtc->primary);
+ struct dc_plane *dc_plane = to_dc_plane(crtc->primary);
struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
+ bool disabling_plane;
int idx;
if (drm_atomic_crtc_needs_modeset(new_crtc_state) ||
@@ -216,13 +223,40 @@ dc_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_atomic_state *state)
enable_irq(dc_crtc->irq_ed_cont_shdload);
- /* flush plane update out to display */
- dc_ed_pec_sync_trigger(dc_crtc->ed_cont);
+ disabling_plane = drm_atomic_plane_disabling(old_plane_state,
+ new_plane_state);
+
+ if (disabling_plane) {
+ unsigned long flags;
+
+ dc_crtc_dbg(crtc, "disabling plane\n");
+
+ /*
+ * Don't relinquish CPU until DPRC REPEAT_EN is disabled and
+ * sync is triggered.
+ */
+ local_irq_save(flags);
+ preempt_disable();
+
+ DC_CRTC_WAIT_FOR_FRAMEGEN_FRAME_INDEX_MOVING(dc_crtc->fg);
+ dc_dprc_disable_repeat_en(dc_plane->fu->dprc);
+ /* flush plane update out to display */
+ dc_ed_pec_sync_trigger(dc_crtc->ed_cont);
+
+ local_irq_restore(flags);
+ preempt_enable();
+ } else {
+ /* flush plane update out to display */
+ dc_ed_pec_sync_trigger(dc_crtc->ed_cont);
+ }
DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(ed_cont_shdload_done);
disable_irq(dc_crtc->irq_ed_cont_shdload);
+ if (disabling_plane)
+ dc_dprc_disable(dc_plane->fu->dprc);
+
DC_CRTC_CHECK_FRAMEGEN_FIFO(dc_crtc->fg);
drm_dev_exit(idx);
@@ -320,14 +354,33 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
dc_crtc_queue_state_event(new_crtc_state);
}
-static inline void __dc_crtc_disable_fg(struct drm_crtc *crtc)
+static inline void
+__dc_crtc_disable_fg_along_with_dprc_repeat_en(struct drm_crtc *crtc)
{
+ struct dc_plane *dc_plane = to_dc_plane(crtc->primary);
struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
+ unsigned long flags;
- enable_irq(dc_crtc->irq_dec_seqcomplete);
+ /* Don't relinquish CPU until DPRC REPEAT_EN is disabled. */
+ local_irq_save(flags);
+ preempt_disable();
+
+ /*
+ * Sync to FrameGen frame index moving so that
+ * FrameGen can be disabled in the next frame.
+ */
+ DC_CRTC_WAIT_FOR_FRAMEGEN_FRAME_INDEX_MOVING(dc_crtc->fg);
dc_fg_disable(dc_crtc->fg);
- DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
- disable_irq(dc_crtc->irq_dec_seqcomplete);
+ /*
+ * There is one frame leftover after FrameGen disablement.
+ * Sync to FrameGen frame index moving so that DPRC REPEAT_EN
+ * can be disabled in the next frame.
+ */
+ DC_CRTC_WAIT_FOR_FRAMEGEN_FRAME_INDEX_MOVING(dc_crtc->fg);
+ dc_dprc_disable_repeat_en(dc_plane->fu->dprc);
+
+ local_irq_restore(flags);
+ preempt_enable();
}
static void
@@ -335,14 +388,29 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
{
struct drm_crtc_state *new_crtc_state =
drm_atomic_get_new_crtc_state(state, crtc);
+ struct drm_crtc_state *old_crtc_state =
+ drm_atomic_get_old_crtc_state(state, crtc);
struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev);
+ struct dc_plane *dc_plane = to_dc_plane(crtc->primary);
struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
int idx, ret;
if (!drm_dev_enter(crtc->dev, &idx))
goto out;
- __dc_crtc_disable_fg(crtc);
+ enable_irq(dc_crtc->irq_dec_seqcomplete);
+
+ if (old_crtc_state->plane_mask)
+ __dc_crtc_disable_fg_along_with_dprc_repeat_en(crtc);
+ else
+ dc_fg_disable(dc_crtc->fg);
+
+ DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
+ disable_irq(dc_crtc->irq_dec_seqcomplete);
+
+ if (old_crtc_state->plane_mask)
+ dc_dprc_disable(dc_plane->fu->dprc);
+
dc_fg_disable_clock(dc_crtc->fg);
/* request pixel engine power-off as plane is off too */
@@ -373,7 +441,10 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
void dc_crtc_disable_at_boot(struct drm_crtc *crtc)
{
struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev);
+ struct dc_plane *dc_plane = to_dc_plane(crtc->primary);
struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
+ enum dc_link_id ed_src, lb_sec;
+ bool disable_dprc = false;
int ret;
ret = pm_runtime_resume_and_get(dc_crtc->de->dev);
@@ -383,13 +454,53 @@ void dc_crtc_disable_at_boot(struct drm_crtc *crtc)
return;
}
+ ret = pm_runtime_resume_and_get(dc_drm->pe->dev);
+ if (ret) {
+ dc_crtc_err(crtc, "failed to get DC pixel engine RPM: %d\n",
+ ret);
+ goto out1;
+ }
+
if (!dc_fg_wait_for_frame_index_moving(dc_crtc->fg)) {
dc_crtc_dbg(crtc, "FrameGen frame index isn't moving\n");
- goto out;
+ goto out2;
+ }
+
+ ret = dc_ed_pec_src_sel_get(dc_crtc->ed_cont, &ed_src);
+ if (ret) {
+ dc_crtc_err(crtc, "failed to get content ExtDst's source\n");
+ goto out2;
+ }
+
+ if (ed_src == LINK_ID_CONSTFRAME0 || ed_src == LINK_ID_CONSTFRAME1)
+ goto disable;
+
+ ret = dc_lb_pec_dynamic_sec_sel_get(dc_plane->lb, &lb_sec);
+ if (ret) {
+ dc_crtc_err(crtc,
+ "failed to get primary plane LayerBlend secondary source\n");
+ goto out2;
}
- dc_crtc_dbg(crtc, "disabling at boot\n");
- __dc_crtc_disable_fg(crtc);
+ disable_dprc = true;
+
+disable:
+ enable_irq(dc_crtc->irq_dec_seqcomplete);
+
+ if (disable_dprc) {
+ dc_crtc_dbg(crtc, "disabling along with DPRC REPEAT_EN at boot\n");
+ __dc_crtc_disable_fg_along_with_dprc_repeat_en(crtc);
+ } else {
+ dc_crtc_dbg(crtc, "disabling at boot\n");
+ dc_fg_disable(dc_crtc->fg);
+ }
+
+ DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
+ disable_irq(dc_crtc->irq_dec_seqcomplete);
+
+ if (disable_dprc)
+ dc_dprc_disable_at_boot(dc_plane->fu->dprc);
+
dc_fg_disable_clock(dc_crtc->fg);
if (!dc_drm->pe_clk_axi_disabled) {
@@ -397,7 +508,13 @@ void dc_crtc_disable_at_boot(struct drm_crtc *crtc)
dc_drm->pe_clk_axi_disabled = true;
}
-out:
+out2:
+ ret = pm_runtime_put(dc_drm->pe->dev);
+ if (ret)
+ dc_crtc_err(crtc, "failed to put DC pixel engine RPM: %d\n",
+ ret);
+
+out1:
ret = pm_runtime_put(dc_crtc->de->dev);
if (ret < 0)
dc_crtc_err(crtc, "failed to put DC display engine RPM: %d\n",
diff --git a/drivers/gpu/drm/imx/dc/dc-fu.c b/drivers/gpu/drm/imx/dc/dc-fu.c
index 47d436abb65157de7cab74565e44b199be76de52..bc439c3520d45f894c0afab5b3d52f2f3309c2e2 100644
--- a/drivers/gpu/drm/imx/dc/dc-fu.c
+++ b/drivers/gpu/drm/imx/dc/dc-fu.c
@@ -124,13 +124,28 @@ static inline void dc_fu_set_numbuffers(struct dc_fu *fu, unsigned int num)
SETNUMBUFFERS_MASK, SETNUMBUFFERS(num));
}
-static void dc_fu_set_burstlength(struct dc_fu *fu, dma_addr_t baddr)
+static unsigned int dc_fu_burst_size_fixup(dma_addr_t baddr)
{
- unsigned int burst_size, burst_length;
+ unsigned int burst_size;
burst_size = 1 << __ffs(baddr);
burst_size = round_up(burst_size, 8);
burst_size = min(burst_size, 128U);
+
+ return burst_size;
+}
+
+static unsigned int
+dc_fu_stride_fixup(unsigned int stride, unsigned int burst_size)
+{
+ return round_up(stride, burst_size);
+}
+
+static void dc_fu_set_burstlength(struct dc_fu *fu, dma_addr_t baddr)
+{
+ unsigned int burst_size, burst_length;
+
+ burst_size = dc_fu_burst_size_fixup(baddr);
burst_length = burst_size / 8;
regmap_write_bits(fu->reg_cfg, BURSTBUFFERMANAGEMENT,
@@ -150,8 +165,14 @@ void dc_fu_set_src_bpp(struct dc_fu *fu, enum dc_fu_frac frac, unsigned int bpp)
}
static void dc_fu_set_src_stride(struct dc_fu *fu, enum dc_fu_frac frac,
- unsigned int stride)
+ unsigned int width, int bpp, dma_addr_t baddr)
{
+ unsigned int burst_size = dc_fu_burst_size_fixup(baddr);
+ unsigned int stride;
+
+ stride = width * (bpp / 8);
+ stride = dc_fu_stride_fixup(stride, burst_size);
+
regmap_write_bits(fu->reg_cfg, fu->reg_sourcebufferattributes[frac],
STRIDE_MASK, STRIDE(stride));
}
diff --git a/drivers/gpu/drm/imx/dc/dc-fu.h b/drivers/gpu/drm/imx/dc/dc-fu.h
index 44b9497e53580589a05bcb180eb2312ea4449da4..09570955a5b92105ef7010f71fa615a1cefc9b7e 100644
--- a/drivers/gpu/drm/imx/dc/dc-fu.h
+++ b/drivers/gpu/drm/imx/dc/dc-fu.h
@@ -87,7 +87,7 @@ struct dc_fu_ops {
void (*set_baseaddress)(struct dc_fu *fu, enum dc_fu_frac frac,
dma_addr_t baddr);
void (*set_src_stride)(struct dc_fu *fu, enum dc_fu_frac frac,
- unsigned int stride);
+ unsigned int width, int bpp, dma_addr_t baddr);
void (*set_src_buf_dimensions)(struct dc_fu *fu, enum dc_fu_frac frac,
int w, int h);
void (*set_fmt)(struct dc_fu *fu, enum dc_fu_frac frac,
diff --git a/drivers/gpu/drm/imx/dc/dc-kms.h b/drivers/gpu/drm/imx/dc/dc-kms.h
index a25d47eebd28792e4b53b4ecc89907ce00430c2c..8b45b21a6f8a7e6e6ed2563499753200bdd42ebc 100644
--- a/drivers/gpu/drm/imx/dc/dc-kms.h
+++ b/drivers/gpu/drm/imx/dc/dc-kms.h
@@ -130,4 +130,9 @@ struct dc_plane {
struct dc_ed *ed;
};
+static inline struct dc_plane *to_dc_plane(struct drm_plane *plane)
+{
+ return container_of(plane, struct dc_plane, base);
+}
+
#endif /* __DC_KMS_H__ */
diff --git a/drivers/gpu/drm/imx/dc/dc-plane.c b/drivers/gpu/drm/imx/dc/dc-plane.c
index d8b946fb90de638da2bf4667307f11b06f4e77f5..8ef754492b2dcb5d986a63f516328f8d2512c7b6 100644
--- a/drivers/gpu/drm/imx/dc/dc-plane.c
+++ b/drivers/gpu/drm/imx/dc/dc-plane.c
@@ -17,6 +17,7 @@
#include <drm/drm_plane_helper.h>
#include <drm/drm_print.h>
+#include "dc-dprc.h"
#include "dc-drv.h"
#include "dc-fu.h"
#include "dc-kms.h"
@@ -44,11 +45,6 @@ static const struct drm_plane_funcs dc_plane_funcs = {
.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
};
-static inline struct dc_plane *to_dc_plane(struct drm_plane *plane)
-{
- return container_of(plane, struct dc_plane, base);
-}
-
static int dc_plane_check_max_source_resolution(struct drm_plane_state *state)
{
int src_h = drm_rect_height(&state->src) >> 16;
@@ -88,6 +84,28 @@ static int dc_plane_check_fb(struct drm_plane_state *state)
return 0;
}
+static int dc_plane_check_dprc(struct drm_plane_state *state)
+{
+ struct dc_plane *dplane = to_dc_plane(state->plane);
+ struct drm_framebuffer *fb = state->fb;
+ dma_addr_t baseaddr = drm_fb_dma_get_gem_addr(fb, state, 0);
+ struct dc_dprc *dprc = dplane->fu->dprc;
+ u32 src_w = drm_rect_width(&state->src) >> 16;
+
+ if (!dc_dprc_rtram_width_supported(dprc, src_w)) {
+ dc_plane_dbg(state->plane, "bad RTRAM width for DPRC\n");
+ return -EINVAL;
+ }
+
+ if (!dc_dprc_stride_supported(dprc, fb->pitches[0], src_w, fb->format,
+ baseaddr)) {
+ dc_plane_dbg(state->plane, "fb bad pitches[0] for DPRC\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int
dc_plane_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state)
{
@@ -123,7 +141,11 @@ dc_plane_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state)
if (ret)
return ret;
- return dc_plane_check_fb(plane_state);
+ ret = dc_plane_check_fb(plane_state);
+ if (ret)
+ return ret;
+
+ return dc_plane_check_dprc(plane_state);
}
static void
@@ -131,6 +153,12 @@ dc_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state)
{
struct drm_plane_state *new_state =
drm_atomic_get_new_plane_state(state, plane);
+ struct drm_plane_state *old_state =
+ drm_atomic_get_old_plane_state(state, plane);
+ struct drm_crtc_state *new_crtc_state =
+ drm_atomic_get_new_crtc_state(state, new_state->crtc);
+ bool needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
+ bool prefetch_start = needs_modeset || !old_state->fb;
struct dc_plane *dplane = to_dc_plane(plane);
struct drm_framebuffer *fb = new_state->fb;
const struct dc_fu_ops *fu_ops;
@@ -152,7 +180,8 @@ dc_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state)
fu_ops->set_layerblend(fu, lb);
fu_ops->set_burstlength(fu, baseaddr);
- fu_ops->set_src_stride(fu, DC_FETCHUNIT_FRAC0, fb->pitches[0]);
+ fu_ops->set_src_stride(fu, DC_FETCHUNIT_FRAC0, src_w,
+ fb->format->cpp[0] * 8, baseaddr);
fu_ops->set_src_buf_dimensions(fu, DC_FETCHUNIT_FRAC0, src_w, src_h);
fu_ops->set_fmt(fu, DC_FETCHUNIT_FRAC0, fb->format);
fu_ops->set_framedimensions(fu, src_w, src_h);
@@ -161,6 +190,9 @@ dc_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state)
dc_plane_dbg(plane, "uses %s\n", fu_ops->get_name(fu));
+ dc_dprc_configure(fu->dprc, new_state->crtc->index, src_w, src_h,
+ fb->pitches[0], fb->format, baseaddr, prefetch_start);
+
dc_lb_pec_dynamic_prim_sel(lb, dc_cf_get_link_id(dplane->cf));
dc_lb_pec_dynamic_sec_sel(lb, fu_ops->get_link_id(fu));
dc_lb_mode(lb, LB_BLEND);
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 14/14] drm/imx: dc: Use prefetch engine
2025-09-23 2:08 ` [PATCH v2 14/14] drm/imx: dc: Use prefetch engine Liu Ying
@ 2025-09-23 18:32 ` Frank Li
0 siblings, 0 replies; 26+ messages in thread
From: Frank Li @ 2025-09-23 18:32 UTC (permalink / raw)
To: Liu Ying
Cc: Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Dmitry Baryshkov,
dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel
On Tue, Sep 23, 2025 at 10:08:04AM +0800, Liu Ying wrote:
> One prefetch engine consists of one DPR channel and one or two PRGs.
> Each PRG handles one planar in a pixel format. Every FetchUnit used
> by KMS may attach to a PRG and hence use a prefetch engine. So, to
> simplify driver code, always use prefetch engines for FetchUnits in
> KMS driver and avoid supporting bypassing them. Aside from configuring
> and disabling a prefetch engine along with a FetchUnit for atomic
> commits, properly disable the prefetch engine at boot and adapt burst
> size/stride fixup requirements from PRG in FetchUnit driver.
>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
> drivers/gpu/drm/imx/dc/dc-crtc.c | 139 +++++++++++++++++++++++++++++++++++---
> drivers/gpu/drm/imx/dc/dc-fu.c | 27 +++++++-
> drivers/gpu/drm/imx/dc/dc-fu.h | 2 +-
> drivers/gpu/drm/imx/dc/dc-kms.h | 5 ++
> drivers/gpu/drm/imx/dc/dc-plane.c | 46 +++++++++++--
> 5 files changed, 197 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
> index 9e9e86cd5202bcb0bb4d5627dbcefcc3f4e2ead0..4c7aab360616cb1c84c31c83f16df703b1c2c6d7 100644
> --- a/drivers/gpu/drm/imx/dc/dc-crtc.c
> +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c
> @@ -25,6 +25,7 @@
> #include <drm/drm_vblank.h>
>
> #include "dc-de.h"
> +#include "dc-dprc.h"
> #include "dc-drv.h"
> #include "dc-kms.h"
> #include "dc-pe.h"
> @@ -204,7 +205,13 @@ dc_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_atomic_state *state)
> drm_atomic_get_old_crtc_state(state, crtc);
> struct drm_crtc_state *new_crtc_state =
> drm_atomic_get_new_crtc_state(state, crtc);
> + struct drm_plane_state *old_plane_state =
> + drm_atomic_get_old_plane_state(state, crtc->primary);
> + struct drm_plane_state *new_plane_state =
> + drm_atomic_get_new_plane_state(state, crtc->primary);
> + struct dc_plane *dc_plane = to_dc_plane(crtc->primary);
> struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
> + bool disabling_plane;
> int idx;
>
> if (drm_atomic_crtc_needs_modeset(new_crtc_state) ||
> @@ -216,13 +223,40 @@ dc_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_atomic_state *state)
>
> enable_irq(dc_crtc->irq_ed_cont_shdload);
>
> - /* flush plane update out to display */
> - dc_ed_pec_sync_trigger(dc_crtc->ed_cont);
> + disabling_plane = drm_atomic_plane_disabling(old_plane_state,
> + new_plane_state);
> +
> + if (disabling_plane) {
> + unsigned long flags;
> +
> + dc_crtc_dbg(crtc, "disabling plane\n");
> +
> + /*
> + * Don't relinquish CPU until DPRC REPEAT_EN is disabled and
> + * sync is triggered.
> + */
> + local_irq_save(flags);
> + preempt_disable();
> +
> + DC_CRTC_WAIT_FOR_FRAMEGEN_FRAME_INDEX_MOVING(dc_crtc->fg);
> + dc_dprc_disable_repeat_en(dc_plane->fu->dprc);
> + /* flush plane update out to display */
> + dc_ed_pec_sync_trigger(dc_crtc->ed_cont);
> +
> + local_irq_restore(flags);
> + preempt_enable();
> + } else {
> + /* flush plane update out to display */
> + dc_ed_pec_sync_trigger(dc_crtc->ed_cont);
> + }
>
> DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(ed_cont_shdload_done);
>
> disable_irq(dc_crtc->irq_ed_cont_shdload);
>
> + if (disabling_plane)
> + dc_dprc_disable(dc_plane->fu->dprc);
> +
> DC_CRTC_CHECK_FRAMEGEN_FIFO(dc_crtc->fg);
>
> drm_dev_exit(idx);
> @@ -320,14 +354,33 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
> dc_crtc_queue_state_event(new_crtc_state);
> }
>
> -static inline void __dc_crtc_disable_fg(struct drm_crtc *crtc)
> +static inline void
> +__dc_crtc_disable_fg_along_with_dprc_repeat_en(struct drm_crtc *crtc)
> {
> + struct dc_plane *dc_plane = to_dc_plane(crtc->primary);
> struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
> + unsigned long flags;
>
> - enable_irq(dc_crtc->irq_dec_seqcomplete);
> + /* Don't relinquish CPU until DPRC REPEAT_EN is disabled. */
> + local_irq_save(flags);
> + preempt_disable();
> +
> + /*
> + * Sync to FrameGen frame index moving so that
> + * FrameGen can be disabled in the next frame.
> + */
> + DC_CRTC_WAIT_FOR_FRAMEGEN_FRAME_INDEX_MOVING(dc_crtc->fg);
> dc_fg_disable(dc_crtc->fg);
> - DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
> - disable_irq(dc_crtc->irq_dec_seqcomplete);
> + /*
> + * There is one frame leftover after FrameGen disablement.
> + * Sync to FrameGen frame index moving so that DPRC REPEAT_EN
> + * can be disabled in the next frame.
> + */
> + DC_CRTC_WAIT_FOR_FRAMEGEN_FRAME_INDEX_MOVING(dc_crtc->fg);
> + dc_dprc_disable_repeat_en(dc_plane->fu->dprc);
> +
> + local_irq_restore(flags);
> + preempt_enable();
> }
>
> static void
> @@ -335,14 +388,29 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
> {
> struct drm_crtc_state *new_crtc_state =
> drm_atomic_get_new_crtc_state(state, crtc);
> + struct drm_crtc_state *old_crtc_state =
> + drm_atomic_get_old_crtc_state(state, crtc);
> struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev);
> + struct dc_plane *dc_plane = to_dc_plane(crtc->primary);
> struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
> int idx, ret;
>
> if (!drm_dev_enter(crtc->dev, &idx))
> goto out;
>
> - __dc_crtc_disable_fg(crtc);
> + enable_irq(dc_crtc->irq_dec_seqcomplete);
> +
> + if (old_crtc_state->plane_mask)
> + __dc_crtc_disable_fg_along_with_dprc_repeat_en(crtc);
> + else
> + dc_fg_disable(dc_crtc->fg);
> +
> + DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
> + disable_irq(dc_crtc->irq_dec_seqcomplete);
> +
> + if (old_crtc_state->plane_mask)
> + dc_dprc_disable(dc_plane->fu->dprc);
> +
> dc_fg_disable_clock(dc_crtc->fg);
>
> /* request pixel engine power-off as plane is off too */
> @@ -373,7 +441,10 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
> void dc_crtc_disable_at_boot(struct drm_crtc *crtc)
> {
> struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev);
> + struct dc_plane *dc_plane = to_dc_plane(crtc->primary);
> struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
> + enum dc_link_id ed_src, lb_sec;
> + bool disable_dprc = false;
> int ret;
>
> ret = pm_runtime_resume_and_get(dc_crtc->de->dev);
> @@ -383,13 +454,53 @@ void dc_crtc_disable_at_boot(struct drm_crtc *crtc)
> return;
> }
>
> + ret = pm_runtime_resume_and_get(dc_drm->pe->dev);
> + if (ret) {
> + dc_crtc_err(crtc, "failed to get DC pixel engine RPM: %d\n",
> + ret);
> + goto out1;
> + }
> +
> if (!dc_fg_wait_for_frame_index_moving(dc_crtc->fg)) {
> dc_crtc_dbg(crtc, "FrameGen frame index isn't moving\n");
> - goto out;
> + goto out2;
> + }
> +
> + ret = dc_ed_pec_src_sel_get(dc_crtc->ed_cont, &ed_src);
> + if (ret) {
> + dc_crtc_err(crtc, "failed to get content ExtDst's source\n");
> + goto out2;
> + }
> +
> + if (ed_src == LINK_ID_CONSTFRAME0 || ed_src == LINK_ID_CONSTFRAME1)
> + goto disable;
> +
> + ret = dc_lb_pec_dynamic_sec_sel_get(dc_plane->lb, &lb_sec);
> + if (ret) {
> + dc_crtc_err(crtc,
> + "failed to get primary plane LayerBlend secondary source\n");
> + goto out2;
> }
>
> - dc_crtc_dbg(crtc, "disabling at boot\n");
> - __dc_crtc_disable_fg(crtc);
> + disable_dprc = true;
> +
> +disable:
> + enable_irq(dc_crtc->irq_dec_seqcomplete);
> +
> + if (disable_dprc) {
> + dc_crtc_dbg(crtc, "disabling along with DPRC REPEAT_EN at boot\n");
> + __dc_crtc_disable_fg_along_with_dprc_repeat_en(crtc);
> + } else {
> + dc_crtc_dbg(crtc, "disabling at boot\n");
> + dc_fg_disable(dc_crtc->fg);
> + }
> +
> + DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
> + disable_irq(dc_crtc->irq_dec_seqcomplete);
> +
> + if (disable_dprc)
> + dc_dprc_disable_at_boot(dc_plane->fu->dprc);
> +
> dc_fg_disable_clock(dc_crtc->fg);
>
> if (!dc_drm->pe_clk_axi_disabled) {
> @@ -397,7 +508,13 @@ void dc_crtc_disable_at_boot(struct drm_crtc *crtc)
> dc_drm->pe_clk_axi_disabled = true;
> }
>
> -out:
> +out2:
> + ret = pm_runtime_put(dc_drm->pe->dev);
> + if (ret)
> + dc_crtc_err(crtc, "failed to put DC pixel engine RPM: %d\n",
> + ret);
> +
> +out1:
> ret = pm_runtime_put(dc_crtc->de->dev);
> if (ret < 0)
> dc_crtc_err(crtc, "failed to put DC display engine RPM: %d\n",
> diff --git a/drivers/gpu/drm/imx/dc/dc-fu.c b/drivers/gpu/drm/imx/dc/dc-fu.c
> index 47d436abb65157de7cab74565e44b199be76de52..bc439c3520d45f894c0afab5b3d52f2f3309c2e2 100644
> --- a/drivers/gpu/drm/imx/dc/dc-fu.c
> +++ b/drivers/gpu/drm/imx/dc/dc-fu.c
> @@ -124,13 +124,28 @@ static inline void dc_fu_set_numbuffers(struct dc_fu *fu, unsigned int num)
> SETNUMBUFFERS_MASK, SETNUMBUFFERS(num));
> }
>
> -static void dc_fu_set_burstlength(struct dc_fu *fu, dma_addr_t baddr)
> +static unsigned int dc_fu_burst_size_fixup(dma_addr_t baddr)
> {
> - unsigned int burst_size, burst_length;
> + unsigned int burst_size;
>
> burst_size = 1 << __ffs(baddr);
> burst_size = round_up(burst_size, 8);
> burst_size = min(burst_size, 128U);
> +
> + return burst_size;
> +}
> +
> +static unsigned int
> +dc_fu_stride_fixup(unsigned int stride, unsigned int burst_size)
> +{
> + return round_up(stride, burst_size);
> +}
> +
> +static void dc_fu_set_burstlength(struct dc_fu *fu, dma_addr_t baddr)
> +{
> + unsigned int burst_size, burst_length;
> +
> + burst_size = dc_fu_burst_size_fixup(baddr);
> burst_length = burst_size / 8;
>
> regmap_write_bits(fu->reg_cfg, BURSTBUFFERMANAGEMENT,
> @@ -150,8 +165,14 @@ void dc_fu_set_src_bpp(struct dc_fu *fu, enum dc_fu_frac frac, unsigned int bpp)
> }
>
> static void dc_fu_set_src_stride(struct dc_fu *fu, enum dc_fu_frac frac,
> - unsigned int stride)
> + unsigned int width, int bpp, dma_addr_t baddr)
> {
> + unsigned int burst_size = dc_fu_burst_size_fixup(baddr);
> + unsigned int stride;
> +
> + stride = width * (bpp / 8);
> + stride = dc_fu_stride_fixup(stride, burst_size);
> +
> regmap_write_bits(fu->reg_cfg, fu->reg_sourcebufferattributes[frac],
> STRIDE_MASK, STRIDE(stride));
> }
> diff --git a/drivers/gpu/drm/imx/dc/dc-fu.h b/drivers/gpu/drm/imx/dc/dc-fu.h
> index 44b9497e53580589a05bcb180eb2312ea4449da4..09570955a5b92105ef7010f71fa615a1cefc9b7e 100644
> --- a/drivers/gpu/drm/imx/dc/dc-fu.h
> +++ b/drivers/gpu/drm/imx/dc/dc-fu.h
> @@ -87,7 +87,7 @@ struct dc_fu_ops {
> void (*set_baseaddress)(struct dc_fu *fu, enum dc_fu_frac frac,
> dma_addr_t baddr);
> void (*set_src_stride)(struct dc_fu *fu, enum dc_fu_frac frac,
> - unsigned int stride);
> + unsigned int width, int bpp, dma_addr_t baddr);
> void (*set_src_buf_dimensions)(struct dc_fu *fu, enum dc_fu_frac frac,
> int w, int h);
> void (*set_fmt)(struct dc_fu *fu, enum dc_fu_frac frac,
> diff --git a/drivers/gpu/drm/imx/dc/dc-kms.h b/drivers/gpu/drm/imx/dc/dc-kms.h
> index a25d47eebd28792e4b53b4ecc89907ce00430c2c..8b45b21a6f8a7e6e6ed2563499753200bdd42ebc 100644
> --- a/drivers/gpu/drm/imx/dc/dc-kms.h
> +++ b/drivers/gpu/drm/imx/dc/dc-kms.h
> @@ -130,4 +130,9 @@ struct dc_plane {
> struct dc_ed *ed;
> };
>
> +static inline struct dc_plane *to_dc_plane(struct drm_plane *plane)
> +{
> + return container_of(plane, struct dc_plane, base);
> +}
> +
> #endif /* __DC_KMS_H__ */
> diff --git a/drivers/gpu/drm/imx/dc/dc-plane.c b/drivers/gpu/drm/imx/dc/dc-plane.c
> index d8b946fb90de638da2bf4667307f11b06f4e77f5..8ef754492b2dcb5d986a63f516328f8d2512c7b6 100644
> --- a/drivers/gpu/drm/imx/dc/dc-plane.c
> +++ b/drivers/gpu/drm/imx/dc/dc-plane.c
> @@ -17,6 +17,7 @@
> #include <drm/drm_plane_helper.h>
> #include <drm/drm_print.h>
>
> +#include "dc-dprc.h"
> #include "dc-drv.h"
> #include "dc-fu.h"
> #include "dc-kms.h"
> @@ -44,11 +45,6 @@ static const struct drm_plane_funcs dc_plane_funcs = {
> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> };
>
> -static inline struct dc_plane *to_dc_plane(struct drm_plane *plane)
> -{
> - return container_of(plane, struct dc_plane, base);
> -}
> -
> static int dc_plane_check_max_source_resolution(struct drm_plane_state *state)
> {
> int src_h = drm_rect_height(&state->src) >> 16;
> @@ -88,6 +84,28 @@ static int dc_plane_check_fb(struct drm_plane_state *state)
> return 0;
> }
>
> +static int dc_plane_check_dprc(struct drm_plane_state *state)
> +{
> + struct dc_plane *dplane = to_dc_plane(state->plane);
> + struct drm_framebuffer *fb = state->fb;
> + dma_addr_t baseaddr = drm_fb_dma_get_gem_addr(fb, state, 0);
> + struct dc_dprc *dprc = dplane->fu->dprc;
> + u32 src_w = drm_rect_width(&state->src) >> 16;
> +
> + if (!dc_dprc_rtram_width_supported(dprc, src_w)) {
> + dc_plane_dbg(state->plane, "bad RTRAM width for DPRC\n");
> + return -EINVAL;
> + }
> +
> + if (!dc_dprc_stride_supported(dprc, fb->pitches[0], src_w, fb->format,
> + baseaddr)) {
> + dc_plane_dbg(state->plane, "fb bad pitches[0] for DPRC\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int
> dc_plane_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state)
> {
> @@ -123,7 +141,11 @@ dc_plane_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state)
> if (ret)
> return ret;
>
> - return dc_plane_check_fb(plane_state);
> + ret = dc_plane_check_fb(plane_state);
> + if (ret)
> + return ret;
> +
> + return dc_plane_check_dprc(plane_state);
> }
>
> static void
> @@ -131,6 +153,12 @@ dc_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state)
> {
> struct drm_plane_state *new_state =
> drm_atomic_get_new_plane_state(state, plane);
> + struct drm_plane_state *old_state =
> + drm_atomic_get_old_plane_state(state, plane);
> + struct drm_crtc_state *new_crtc_state =
> + drm_atomic_get_new_crtc_state(state, new_state->crtc);
> + bool needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> + bool prefetch_start = needs_modeset || !old_state->fb;
> struct dc_plane *dplane = to_dc_plane(plane);
> struct drm_framebuffer *fb = new_state->fb;
> const struct dc_fu_ops *fu_ops;
> @@ -152,7 +180,8 @@ dc_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state)
>
> fu_ops->set_layerblend(fu, lb);
> fu_ops->set_burstlength(fu, baseaddr);
> - fu_ops->set_src_stride(fu, DC_FETCHUNIT_FRAC0, fb->pitches[0]);
> + fu_ops->set_src_stride(fu, DC_FETCHUNIT_FRAC0, src_w,
> + fb->format->cpp[0] * 8, baseaddr);
> fu_ops->set_src_buf_dimensions(fu, DC_FETCHUNIT_FRAC0, src_w, src_h);
> fu_ops->set_fmt(fu, DC_FETCHUNIT_FRAC0, fb->format);
> fu_ops->set_framedimensions(fu, src_w, src_h);
> @@ -161,6 +190,9 @@ dc_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state)
>
> dc_plane_dbg(plane, "uses %s\n", fu_ops->get_name(fu));
>
> + dc_dprc_configure(fu->dprc, new_state->crtc->index, src_w, src_h,
> + fb->pitches[0], fb->format, baseaddr, prefetch_start);
> +
> dc_lb_pec_dynamic_prim_sel(lb, dc_cf_get_link_id(dplane->cf));
> dc_lb_pec_dynamic_sec_sel(lb, fu_ops->get_link_id(fu));
> dc_lb_mode(lb, LB_BLEND);
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread