Re: [PATCH 0/4] Fix DP busy wait and defer disabling overlay plane

[Philipp Zabel <p.zabel@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

Hi Dan,

On Sat, 2017-04-01 at 11:50 +0100, Dan MacDonald wrote:
> No such luck.

I am sorry, I forgot to fix the old patches for modular builds. I have
added the updated patches as tags v4.9-ipu-dp-plane-fix-2,
v4.10-ipu-dp-plane-fix-3, and v4.11-ipu-dp-plane-fix. For example:

    git://git.pengutronix.de/git/pza/linux.git tags/v4.11-ipu-dp-plane-fix

The changes are attached as a patch against v4.11-rc1.

regards
Philipp

[-- Attachment #2: v4.11-rc1-ipu-dp-plane-fix.patch --]
[-- Type: text/x-patch, Size: 14956 bytes --]

diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
index f2c9ae822149c..c9e439c82241c 100644
--- a/drivers/gpu/drm/imx/Kconfig
+++ b/drivers/gpu/drm/imx/Kconfig
@@ -31,13 +31,6 @@ config DRM_IMX_LDB
 	  Choose this to enable the internal LVDS Display Bridge (LDB)
 	  found on i.MX53 and i.MX6 processors.
 
-config DRM_IMX_IPUV3
-	tristate
-	depends on DRM_IMX
-	depends on IMX_IPUV3_CORE
-	default y if DRM_IMX=y
-	default m if DRM_IMX=m
-
 config DRM_IMX_HDMI
 	tristate "Freescale i.MX DRM HDMI"
 	select DRM_DW_HDMI
diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
index f3ecd8903d973..16ecef33e0089 100644
--- a/drivers/gpu/drm/imx/Makefile
+++ b/drivers/gpu/drm/imx/Makefile
@@ -1,5 +1,5 @@
 
-imxdrm-objs := imx-drm-core.o
+imxdrm-objs := imx-drm-core.o ipuv3-crtc.o ipuv3-plane.o
 
 obj-$(CONFIG_DRM_IMX) += imxdrm.o
 
@@ -7,6 +7,5 @@ obj-$(CONFIG_DRM_IMX_PARALLEL_DISPLAY) += parallel-display.o
 obj-$(CONFIG_DRM_IMX_TVE) += imx-tve.o
 obj-$(CONFIG_DRM_IMX_LDB) += imx-ldb.o
 
-imx-ipuv3-crtc-objs  := ipuv3-crtc.o ipuv3-plane.o
 obj-$(CONFIG_DRM_IMX_IPUV3)	+= imx-ipuv3-crtc.o
 obj-$(CONFIG_DRM_IMX_HDMI) += dw_hdmi-imx.o
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index f562cb7964b08..056743d26e800 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -30,6 +30,7 @@
 #include <video/imx-ipu-v3.h>
 
 #include "imx-drm.h"
+#include "ipuv3-plane.h"
 
 #define MAX_CRTC	4
 
@@ -160,6 +161,10 @@ static const struct drm_mode_config_funcs imx_drm_mode_config_funcs = {
 static void imx_drm_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state;
+	bool plane_disabling = false;
+	int i;
 
 	drm_atomic_helper_commit_modeset_disables(dev, state);
 
@@ -169,11 +174,20 @@ static void imx_drm_atomic_commit_tail(struct drm_atomic_state *state)
 
 	drm_atomic_helper_commit_modeset_enables(dev, state);
 
-	drm_atomic_helper_commit_hw_done(state);
+	for_each_plane_in_state(state, plane, old_plane_state, i) {
+		if (drm_atomic_plane_disabling(plane, old_plane_state))
+			plane_disabling = true;
+	}
 
-	drm_atomic_helper_wait_for_vblanks(dev, state);
+	if (plane_disabling) {
+		drm_atomic_helper_wait_for_vblanks(dev, state);
 
-	drm_atomic_helper_cleanup_planes(dev, state);
+		for_each_plane_in_state(state, plane, old_plane_state, i)
+			ipu_plane_disable_deferred(plane);
+
+	}
+
+	drm_atomic_helper_commit_hw_done(state);
 }
 
 static struct drm_mode_config_helper_funcs imx_drm_mode_config_helpers = {
@@ -519,7 +533,23 @@ static struct platform_driver imx_drm_pdrv = {
 		.of_match_table = imx_drm_dt_ids,
 	},
 };
-module_platform_driver(imx_drm_pdrv);
+
+static struct platform_driver * const drivers[] = {
+	&imx_drm_pdrv,
+	&ipu_drm_driver,
+};
+
+static int __init imx_drm_init(void)
+{
+	return platform_register_drivers(drivers, ARRAY_SIZE(drivers));
+}
+module_init(imx_drm_init);
+
+static void __exit imx_drm_exit(void)
+{
+	platform_unregister_drivers(drivers, ARRAY_SIZE(drivers));
+}
+module_exit(imx_drm_exit);
 
 MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
 MODULE_DESCRIPTION("i.MX drm driver core");
diff --git a/drivers/gpu/drm/imx/imx-drm.h b/drivers/gpu/drm/imx/imx-drm.h
index 5a91cb16c8fa5..98ee8a4e62971 100644
--- a/drivers/gpu/drm/imx/imx-drm.h
+++ b/drivers/gpu/drm/imx/imx-drm.h
@@ -42,6 +42,8 @@ int imx_drm_init_drm(struct platform_device *pdev,
 		int preferred_bpp);
 int imx_drm_exit_drm(void);
 
+extern struct platform_driver ipu_drm_driver;
+
 void imx_drm_mode_config_init(struct drm_device *drm);
 
 struct drm_gem_cma_object *imx_drm_fb_get_obj(struct drm_framebuffer *fb);
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 6be515a9fb694..19df599d45714 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -60,6 +60,26 @@ static void ipu_crtc_enable(struct drm_crtc *crtc)
 	ipu_di_enable(ipu_crtc->di);
 }
 
+static void ipu_crtc_disable_planes(struct ipu_crtc *ipu_crtc,
+				    struct drm_crtc_state *old_crtc_state)
+{
+	bool disable_partial = false;
+	bool disable_full = false;
+	struct drm_plane *plane;
+
+	drm_atomic_crtc_state_for_each_plane(plane, old_crtc_state) {
+		if (plane == &ipu_crtc->plane[0]->base)
+			disable_full = true;
+		if (&ipu_crtc->plane[1] && plane == &ipu_crtc->plane[1]->base)
+			disable_partial = true;
+	}
+
+	if (disable_partial)
+		ipu_plane_disable(ipu_crtc->plane[1], true);
+	if (disable_full)
+		ipu_plane_disable(ipu_crtc->plane[0], false);
+}
+
 static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
 				    struct drm_crtc_state *old_crtc_state)
 {
@@ -73,7 +93,7 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
 	 * attached IDMACs will be left in undefined state, possibly hanging
 	 * the IPU or even system.
 	 */
-	drm_atomic_helper_disable_planes_on_crtc(old_crtc_state, false);
+	ipu_crtc_disable_planes(ipu_crtc, old_crtc_state);
 	ipu_dc_disable(ipu);
 
 	spin_lock_irq(&crtc->dev->event_lock);
@@ -457,16 +477,10 @@ static int ipu_drm_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct platform_driver ipu_drm_driver = {
+struct platform_driver ipu_drm_driver = {
 	.driver = {
 		.name = "imx-ipuv3-crtc",
 	},
 	.probe = ipu_drm_probe,
 	.remove = ipu_drm_remove,
 };
-module_platform_driver(ipu_drm_driver);
-
-MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
-MODULE_DESCRIPTION(DRIVER_DESC);
-MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:imx-ipuv3-crtc");
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 8b5294d47ceee..5023bbd21dd1e 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -172,23 +172,30 @@ static void ipu_plane_enable(struct ipu_plane *ipu_plane)
 		ipu_dp_enable_channel(ipu_plane->dp);
 }
 
-static int ipu_disable_plane(struct drm_plane *plane)
+void ipu_plane_disable(struct ipu_plane *ipu_plane, bool disable_dp_channel)
 {
-	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
-
 	DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
 
 	ipu_idmac_wait_busy(ipu_plane->ipu_ch, 50);
 
-	if (ipu_plane->dp)
-		ipu_dp_disable_channel(ipu_plane->dp);
+	if (ipu_plane->dp && disable_dp_channel)
+		ipu_dp_disable_channel(ipu_plane->dp, false);
 	ipu_idmac_disable_channel(ipu_plane->ipu_ch);
 	ipu_dmfc_disable_channel(ipu_plane->dmfc);
 	if (ipu_plane->dp)
 		ipu_dp_disable(ipu_plane->ipu);
+}
 
-	return 0;
+void ipu_plane_disable_deferred(struct drm_plane *plane)
+{
+	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
+
+	if (ipu_plane->disabling) {
+		ipu_plane->disabling = false;
+		ipu_plane_disable(ipu_plane, false);
+	}
 }
+EXPORT_SYMBOL_GPL(ipu_plane_disable_deferred);
 
 static void ipu_plane_destroy(struct drm_plane *plane)
 {
@@ -361,7 +368,11 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
 static void ipu_plane_atomic_disable(struct drm_plane *plane,
 				     struct drm_plane_state *old_state)
 {
-	ipu_disable_plane(plane);
+	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
+
+	if (ipu_plane->dp)
+		ipu_dp_disable_channel(ipu_plane->dp, true);
+	ipu_plane->disabling = true;
 }
 
 static void ipu_plane_atomic_update(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.h b/drivers/gpu/drm/imx/ipuv3-plane.h
index 338b88a74eb6e..0e2a723ff9816 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.h
+++ b/drivers/gpu/drm/imx/ipuv3-plane.h
@@ -23,6 +23,8 @@ struct ipu_plane {
 
 	int			dma;
 	int			dp_flow;
+
+	bool			disabling;
 };
 
 struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
@@ -42,4 +44,7 @@ void ipu_plane_put_resources(struct ipu_plane *plane);
 
 int ipu_plane_irq(struct ipu_plane *plane);
 
+void ipu_plane_disable(struct ipu_plane *ipu_plane, bool disable_dp_channel);
+void ipu_plane_disable_deferred(struct drm_plane *plane);
+
 #endif
diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c
index 8368e6f766ee5..8a32ed25a1c29 100644
--- a/drivers/gpu/ipu-v3/ipu-common.c
+++ b/drivers/gpu/ipu-v3/ipu-common.c
@@ -51,15 +51,17 @@ int ipu_get_num(struct ipu_soc *ipu)
 }
 EXPORT_SYMBOL_GPL(ipu_get_num);
 
-void ipu_srm_dp_sync_update(struct ipu_soc *ipu)
+void ipu_srm_dp_update(struct ipu_soc *ipu, bool sync)
 {
 	u32 val;
 
 	val = ipu_cm_read(ipu, IPU_SRM_PRI2);
-	val |= 0x8;
+	val &= ~DP_S_SRM_MODE_MASK;
+	val |= sync ? DP_S_SRM_MODE_NEXT_FRAME :
+		      DP_S_SRM_MODE_NOW;
 	ipu_cm_write(ipu, val, IPU_SRM_PRI2);
 }
-EXPORT_SYMBOL_GPL(ipu_srm_dp_sync_update);
+EXPORT_SYMBOL_GPL(ipu_srm_dp_update);
 
 enum ipu_color_space ipu_drm_fourcc_to_colorspace(u32 drm_fourcc)
 {
diff --git a/drivers/gpu/ipu-v3/ipu-dc.c b/drivers/gpu/ipu-v3/ipu-dc.c
index 659475c1e44ab..7a4b8362dda8f 100644
--- a/drivers/gpu/ipu-v3/ipu-dc.c
+++ b/drivers/gpu/ipu-v3/ipu-dc.c
@@ -112,8 +112,6 @@ struct ipu_dc_priv {
 	struct ipu_dc		channels[IPU_DC_NUM_CHANNELS];
 	struct mutex		mutex;
 	struct completion	comp;
-	int			dc_irq;
-	int			dp_irq;
 	int			use_count;
 };
 
@@ -262,47 +260,13 @@ void ipu_dc_enable_channel(struct ipu_dc *dc)
 }
 EXPORT_SYMBOL_GPL(ipu_dc_enable_channel);
 
-static irqreturn_t dc_irq_handler(int irq, void *dev_id)
-{
-	struct ipu_dc *dc = dev_id;
-	u32 reg;
-
-	reg = readl(dc->base + DC_WR_CH_CONF);
-	reg &= ~DC_WR_CH_CONF_PROG_TYPE_MASK;
-	writel(reg, dc->base + DC_WR_CH_CONF);
-
-	/* The Freescale BSP kernel clears DIx_COUNTER_RELEASE here */
-
-	complete(&dc->priv->comp);
-	return IRQ_HANDLED;
-}
-
 void ipu_dc_disable_channel(struct ipu_dc *dc)
 {
-	struct ipu_dc_priv *priv = dc->priv;
-	int irq;
-	unsigned long ret;
 	u32 val;
 
-	/* TODO: Handle MEM_FG_SYNC differently from MEM_BG_SYNC */
-	if (dc->chno == 1)
-		irq = priv->dc_irq;
-	else if (dc->chno == 5)
-		irq = priv->dp_irq;
-	else
-		return;
-
-	init_completion(&priv->comp);
-	enable_irq(irq);
-	ret = wait_for_completion_timeout(&priv->comp, msecs_to_jiffies(50));
-	disable_irq(irq);
-	if (ret == 0) {
-		dev_warn(priv->dev, "DC stop timeout after 50 ms\n");
-
-		val = readl(dc->base + DC_WR_CH_CONF);
-		val &= ~DC_WR_CH_CONF_PROG_TYPE_MASK;
-		writel(val, dc->base + DC_WR_CH_CONF);
-	}
+	val = readl(dc->base + DC_WR_CH_CONF);
+	val &= ~DC_WR_CH_CONF_PROG_TYPE_MASK;
+	writel(val, dc->base + DC_WR_CH_CONF);
 }
 EXPORT_SYMBOL_GPL(ipu_dc_disable_channel);
 
@@ -389,7 +353,7 @@ int ipu_dc_init(struct ipu_soc *ipu, struct device *dev,
 	struct ipu_dc_priv *priv;
 	static int channel_offsets[] = { 0, 0x1c, 0x38, 0x54, 0x58, 0x5c,
 		0x78, 0, 0x94, 0xb4};
-	int i, ret;
+	int i;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -410,23 +374,6 @@ int ipu_dc_init(struct ipu_soc *ipu, struct device *dev,
 		priv->channels[i].base = priv->dc_reg + channel_offsets[i];
 	}
 
-	priv->dc_irq = ipu_map_irq(ipu, IPU_IRQ_DC_FC_1);
-	if (!priv->dc_irq)
-		return -EINVAL;
-	ret = devm_request_irq(dev, priv->dc_irq, dc_irq_handler, 0, NULL,
-			       &priv->channels[1]);
-	if (ret < 0)
-		return ret;
-	disable_irq(priv->dc_irq);
-	priv->dp_irq = ipu_map_irq(ipu, IPU_IRQ_DP_SF_END);
-	if (!priv->dp_irq)
-		return -EINVAL;
-	ret = devm_request_irq(dev, priv->dp_irq, dc_irq_handler, 0, NULL,
-			       &priv->channels[5]);
-	if (ret < 0)
-		return ret;
-	disable_irq(priv->dp_irq);
-
 	writel(DC_WR_CH_CONF_WORD_SIZE_24 | DC_WR_CH_CONF_DISP_ID_PARALLEL(1) |
 			DC_WR_CH_CONF_PROG_DI_ID,
 			priv->channels[1].base + DC_WR_CH_CONF);
diff --git a/drivers/gpu/ipu-v3/ipu-dp.c b/drivers/gpu/ipu-v3/ipu-dp.c
index 98686edbcdbb0..9b2b3fa479c46 100644
--- a/drivers/gpu/ipu-v3/ipu-dp.c
+++ b/drivers/gpu/ipu-v3/ipu-dp.c
@@ -112,7 +112,7 @@ int ipu_dp_set_global_alpha(struct ipu_dp *dp, bool enable,
 		writel(reg & ~DP_COM_CONF_GWAM, flow->base + DP_COM_CONF);
 	}
 
-	ipu_srm_dp_sync_update(priv->ipu);
+	ipu_srm_dp_update(priv->ipu, true);
 
 	mutex_unlock(&priv->mutex);
 
@@ -127,7 +127,7 @@ int ipu_dp_set_window_pos(struct ipu_dp *dp, u16 x_pos, u16 y_pos)
 
 	writel((x_pos << 16) | y_pos, flow->base + DP_FG_POS);
 
-	ipu_srm_dp_sync_update(priv->ipu);
+	ipu_srm_dp_update(priv->ipu, true);
 
 	return 0;
 }
@@ -207,7 +207,7 @@ int ipu_dp_setup_channel(struct ipu_dp *dp,
 					flow->out_cs, DP_COM_CONF_CSC_DEF_FG);
 	}
 
-	ipu_srm_dp_sync_update(priv->ipu);
+	ipu_srm_dp_update(priv->ipu, true);
 
 	mutex_unlock(&priv->mutex);
 
@@ -247,7 +247,7 @@ int ipu_dp_enable_channel(struct ipu_dp *dp)
 	reg |= DP_COM_CONF_FG_EN;
 	writel(reg, flow->base + DP_COM_CONF);
 
-	ipu_srm_dp_sync_update(priv->ipu);
+	ipu_srm_dp_update(priv->ipu, true);
 
 	mutex_unlock(&priv->mutex);
 
@@ -255,7 +255,7 @@ int ipu_dp_enable_channel(struct ipu_dp *dp)
 }
 EXPORT_SYMBOL_GPL(ipu_dp_enable_channel);
 
-void ipu_dp_disable_channel(struct ipu_dp *dp)
+void ipu_dp_disable_channel(struct ipu_dp *dp, bool sync)
 {
 	struct ipu_flow *flow = to_flow(dp);
 	struct ipu_dp_priv *priv = flow->priv;
@@ -275,10 +275,7 @@ void ipu_dp_disable_channel(struct ipu_dp *dp)
 	writel(reg, flow->base + DP_COM_CONF);
 
 	writel(0, flow->base + DP_FG_POS);
-	ipu_srm_dp_sync_update(priv->ipu);
-
-	if (ipu_idmac_channel_busy(priv->ipu, IPUV3_CHANNEL_MEM_BG_SYNC))
-		ipu_wait_interrupt(priv->ipu, IPU_IRQ_DP_SF_END, 50);
+	ipu_srm_dp_update(priv->ipu, sync);
 
 	mutex_unlock(&priv->mutex);
 }
diff --git a/drivers/gpu/ipu-v3/ipu-prv.h b/drivers/gpu/ipu-v3/ipu-prv.h
index 22e47b68b14a2..285595702ee0f 100644
--- a/drivers/gpu/ipu-v3/ipu-prv.h
+++ b/drivers/gpu/ipu-v3/ipu-prv.h
@@ -75,6 +75,11 @@ struct ipu_soc;
 #define IPU_INT_CTRL(n)		IPU_CM_REG(0x003C + 4 * (n))
 #define IPU_INT_STAT(n)		IPU_CM_REG(0x0200 + 4 * (n))
 
+/* SRM_PRI2 */
+#define DP_S_SRM_MODE_MASK		(0x3 << 3)
+#define DP_S_SRM_MODE_NOW		(0x3 << 3)
+#define DP_S_SRM_MODE_NEXT_FRAME	(0x1 << 3)
+
 /* FS_PROC_FLOW1 */
 #define FS_PRPENC_ROT_SRC_SEL_MASK	(0xf << 0)
 #define FS_PRPENC_ROT_SRC_SEL_ENC		(0x7 << 0)
@@ -215,7 +220,7 @@ static inline void ipu_idmac_write(struct ipu_soc *ipu, u32 value,
 	writel(value, ipu->idmac_reg + offset);
 }
 
-void ipu_srm_dp_sync_update(struct ipu_soc *ipu);
+void ipu_srm_dp_update(struct ipu_soc *ipu, bool sync);
 
 int ipu_module_enable(struct ipu_soc *ipu, u32 mask);
 int ipu_module_disable(struct ipu_soc *ipu, u32 mask);
diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
index 53cd07ccaa4ce..899d2b00ad6d4 100644
--- a/include/video/imx-ipu-v3.h
+++ b/include/video/imx-ipu-v3.h
@@ -300,7 +300,7 @@ struct ipu_dp *ipu_dp_get(struct ipu_soc *ipu, unsigned int flow);
 void ipu_dp_put(struct ipu_dp *);
 int ipu_dp_enable(struct ipu_soc *ipu);
 int ipu_dp_enable_channel(struct ipu_dp *dp);
-void ipu_dp_disable_channel(struct ipu_dp *dp);
+void ipu_dp_disable_channel(struct ipu_dp *dp, bool sync);
 void ipu_dp_disable(struct ipu_soc *ipu);
 int ipu_dp_setup_channel(struct ipu_dp *dp,
 		enum ipu_color_space in, enum ipu_color_space out);

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel