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

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

[Philipp Zabel]

Hi,

third try. This time I've removed the drm_atomic_helper_wait_for_vblanks
call from imx_drm_commit_tail unless there are planes to be disabled.
Also the drm_atomic_helper_cleanup_planes call is removed, as that is
a no-op for CMA framebuffer based drivers.

This series fixes an issue with the IPU DC/DP/IDMAC disable sequence. The
interrupt waiting code didn't work as expected, sometimes causing busy waits
longer than the timeout in drm_atomic_helper_wait_for_vblanks, which would
cause crashes similar to the reported "imxdrm issue on SABRE Lite" [1].

[1] http://www.spinics.net/lists/dri-devel/msg132485.html

I could only reproduce the error when the overlay plane was involved, using
weston with the atomic modeset patchset to trigger it, so I'm not sure if this
fixes the issue above, too.

regards
Philipp

Lucas Stach (1):
  gpu: ipu-v3: remove IRQ dance on DC channel disable

Philipp Zabel (3):
  gpu: ipu-v3: add unsynchronised DP channel disabling
  drm/imx: don't wait for vblank and stop calling cleanup_planes in
    commit_tail
  drm/imx: add deferred plane disabling

 drivers/gpu/drm/imx/imx-drm-core.c | 20 +++++++++++--
 drivers/gpu/drm/imx/ipuv3-crtc.c   | 22 +++++++++++++-
 drivers/gpu/drm/imx/ipuv3-plane.c  | 25 +++++++++++-----
 drivers/gpu/drm/imx/ipuv3-plane.h  |  5 ++++
 drivers/gpu/ipu-v3/ipu-common.c    |  8 +++--
 drivers/gpu/ipu-v3/ipu-dc.c        | 61 +++-----------------------------------
 drivers/gpu/ipu-v3/ipu-dp.c        | 15 ++++------
 drivers/gpu/ipu-v3/ipu-prv.h       |  7 ++++-
 include/video/imx-ipu-v3.h         |  2 +-
 9 files changed, 83 insertions(+), 82 deletions(-)

-- 
2.11.0

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

[PATCH v3 1/4] gpu: ipu-v3: remove IRQ dance on DC channel disable

[Philipp Zabel]

From: Lucas Stach <l.stach@pengutronix.de>

This has never worked properly, as the IRQ got retriggered immediately
on unmask. Remove the IRQ wait dance, as it is apparently safe to disable
the DC channel at any point in time.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/ipu-v3/ipu-dc.c | 61 +++------------------------------------------
 1 file changed, 4 insertions(+), 57 deletions(-)

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);
-- 
2.11.0

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

[PATCH v3 2/4] gpu: ipu-v3: add unsynchronised DP channel disabling

[Philipp Zabel]

When disabling the foreground DP channel during a modeset, the DC is
already disabled without waiting for end of frame. There is no reason
to wait for a frame boundary before updating the DP registers in that
case.
Add support to apply updates immediately. No functional changes, yet.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/imx/ipuv3-plane.c |  2 +-
 drivers/gpu/ipu-v3/ipu-common.c   |  8 +++++---
 drivers/gpu/ipu-v3/ipu-dp.c       | 12 ++++++------
 drivers/gpu/ipu-v3/ipu-prv.h      |  7 ++++++-
 include/video/imx-ipu-v3.h        |  2 +-
 5 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 24819c9c36400..55991d46ced50 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -181,7 +181,7 @@ static int ipu_disable_plane(struct drm_plane *plane)
 	ipu_idmac_wait_busy(ipu_plane->ipu_ch, 50);
 
 	if (ipu_plane->dp)
-		ipu_dp_disable_channel(ipu_plane->dp);
+		ipu_dp_disable_channel(ipu_plane->dp, true);
 	ipu_idmac_disable_channel(ipu_plane->ipu_ch);
 	ipu_dmfc_disable_channel(ipu_plane->dmfc);
 	if (ipu_plane->dp)
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-dp.c b/drivers/gpu/ipu-v3/ipu-dp.c
index 98686edbcdbb0..0e09c98248a0d 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,7 +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);
+	ipu_srm_dp_update(priv->ipu, sync);
 
 	if (ipu_idmac_channel_busy(priv->ipu, IPUV3_CHANNEL_MEM_BG_SYNC))
 		ipu_wait_interrupt(priv->ipu, IPU_IRQ_DP_SF_END, 50);
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);
-- 
2.11.0

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

[PATCH v3 3/4] drm/imx: don't wait for vblank and stop calling cleanup_planes in commit_tail

[Philipp Zabel]

drm_atomic_helper_cleanup_planes only calls the cleanup_fb plane
helpers, which we don't implement as a CMA framebuffer based driver.
There is no reason to wait for vblanks in commit_tail only to do nothing
afterwards.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v2:
 - New patch that removes wait_for_vblanks and cleanup_planes instead
   of reordering commit_hw_done and wait_for_vblanks.
---
 drivers/gpu/drm/imx/imx-drm-core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index f562cb7964b08..0a5e4fbb906bf 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -170,10 +170,6 @@ 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);
-
-	drm_atomic_helper_wait_for_vblanks(dev, state);
-
-	drm_atomic_helper_cleanup_planes(dev, state);
 }
 
 static struct drm_mode_config_helper_funcs imx_drm_mode_config_helpers = {
-- 
2.11.0

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

[PATCH v3 4/4] drm/imx: add deferred plane disabling

[Philipp Zabel]

The DP (display processor) channel disable code tried to busy wait for
the DP sync flow end interrupt status bit when disabling the partial
plane without a full modeset. That never worked reliably, and it was
disabled completely by the recent "gpu: ipu-v3: remove IRQ dance on DC
channel disable" patch, causing ipu_wait_interrupt to always time out
after 50 ms, which in turn would trigger a timeout in
drm_atomic_helper_wait_for_vblanks.

This patch changes ipu_plane_atomic_disable to only queue a DP channel
register update at the next frame boundary and set a flag, which can be
done without any waiting whatsoever. The imx_drm_atomic_commit_tail then
calls a new ipu_plane_disable_deferred function that does the actual
IDMAC teardown of the planes that are flagged for deferred disabling,
after waiting for the vblank.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v2:
 - Add missing export for ipu_plane_disable_deferred
 - Check if there are any planes to be disabled and only then wait for
   vblanks and call the deferred disable function
---
 drivers/gpu/drm/imx/imx-drm-core.c | 18 ++++++++++++++++++
 drivers/gpu/drm/imx/ipuv3-crtc.c   | 22 +++++++++++++++++++++-
 drivers/gpu/drm/imx/ipuv3-plane.c  | 25 ++++++++++++++++++-------
 drivers/gpu/drm/imx/ipuv3-plane.h  |  5 +++++
 drivers/gpu/ipu-v3/ipu-dp.c        |  3 ---
 5 files changed, 62 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 0a5e4fbb906bf..94f9c25e1c67b 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 *plane_state;
+	bool plane_disabling = false;
+	int i;
 
 	drm_atomic_helper_commit_modeset_disables(dev, state);
 
@@ -169,6 +174,19 @@ static void imx_drm_atomic_commit_tail(struct drm_atomic_state *state)
 
 	drm_atomic_helper_commit_modeset_enables(dev, state);
 
+	for_each_plane_in_state(state, plane, plane_state, i) {
+		if (drm_atomic_plane_disabling(plane, plane_state))
+			plane_disabling = true;
+	}
+
+	if (plane_disabling) {
+		drm_atomic_helper_wait_for_vblanks(dev, state);
+
+		for_each_plane_in_state(state, plane, plane_state, i)
+			ipu_plane_disable_deferred(plane);
+
+	}
+
 	drm_atomic_helper_commit_hw_done(state);
 }
 
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 6be515a9fb694..0f15f11f26e0c 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);
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 55991d46ced50..a37735298615e 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, true);
+	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)
 {
@@ -356,7 +363,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-dp.c b/drivers/gpu/ipu-v3/ipu-dp.c
index 0e09c98248a0d..9b2b3fa479c46 100644
--- a/drivers/gpu/ipu-v3/ipu-dp.c
+++ b/drivers/gpu/ipu-v3/ipu-dp.c
@@ -277,9 +277,6 @@ void ipu_dp_disable_channel(struct ipu_dp *dp, bool sync)
 	writel(0, flow->base + DP_FG_POS);
 	ipu_srm_dp_update(priv->ipu, sync);
 
-	if (ipu_idmac_channel_busy(priv->ipu, IPUV3_CHANNEL_MEM_BG_SYNC))
-		ipu_wait_interrupt(priv->ipu, IPU_IRQ_DP_SF_END, 50);
-
 	mutex_unlock(&priv->mutex);
 }
 EXPORT_SYMBOL_GPL(ipu_dp_disable_channel);
-- 
2.11.0

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

Re: [PATCH v3 1/4] gpu: ipu-v3: remove IRQ dance on DC channel disable

[Dan MacDonald]

I've finished doing the bulk of my house moving and have time to test
these patches now. I'm presuming they've not made it into 4.10(.1)
have they?

On Tue, Feb 28, 2017 at 2:18 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> From: Lucas Stach <l.stach@pengutronix.de>
>
> This has never worked properly, as the IRQ got retriggered immediately
> on unmask. Remove the IRQ wait dance, as it is apparently safe to disable
> the DC channel at any point in time.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/ipu-v3/ipu-dc.c | 61 +++------------------------------------------
>  1 file changed, 4 insertions(+), 57 deletions(-)
>
> 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);
> --
> 2.11.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/4] drm/imx: don't wait for vblank and stop calling cleanup_planes in commit_tail

[Lucas Stach]

Am Dienstag, den 28.02.2017, 15:18 +0100 schrieb Philipp Zabel:
> drm_atomic_helper_cleanup_planes only calls the cleanup_fb plane
> helpers, which we don't implement as a CMA framebuffer based driver.
> There is no reason to wait for vblanks in commit_tail only to do nothing
> afterwards.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> Changes since v2:
>  - New patch that removes wait_for_vblanks and cleanup_planes instead
>    of reordering commit_hw_done and wait_for_vblanks.
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index f562cb7964b08..0a5e4fbb906bf 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -170,10 +170,6 @@ 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);
> -
> -	drm_atomic_helper_wait_for_vblanks(dev, state);
> -
> -	drm_atomic_helper_cleanup_planes(dev, state);
>  }
>  
>  static struct drm_mode_config_helper_funcs imx_drm_mode_config_helpers = {


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

Re: [PATCH v3 4/4] drm/imx: add deferred plane disabling

[Lucas Stach]

Am Dienstag, den 28.02.2017, 15:18 +0100 schrieb Philipp Zabel:
> The DP (display processor) channel disable code tried to busy wait for
> the DP sync flow end interrupt status bit when disabling the partial
> plane without a full modeset. That never worked reliably, and it was
> disabled completely by the recent "gpu: ipu-v3: remove IRQ dance on DC
> channel disable" patch, causing ipu_wait_interrupt to always time out
> after 50 ms, which in turn would trigger a timeout in
> drm_atomic_helper_wait_for_vblanks.
> 
> This patch changes ipu_plane_atomic_disable to only queue a DP channel
> register update at the next frame boundary and set a flag, which can be
> done without any waiting whatsoever. The imx_drm_atomic_commit_tail then
> calls a new ipu_plane_disable_deferred function that does the actual
> IDMAC teardown of the planes that are flagged for deferred disabling,
> after waiting for the vblank.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> Changes since v2:
>  - Add missing export for ipu_plane_disable_deferred
>  - Check if there are any planes to be disabled and only then wait for
>    vblanks and call the deferred disable function
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c | 18 ++++++++++++++++++
>  drivers/gpu/drm/imx/ipuv3-crtc.c   | 22 +++++++++++++++++++++-
>  drivers/gpu/drm/imx/ipuv3-plane.c  | 25 ++++++++++++++++++-------
>  drivers/gpu/drm/imx/ipuv3-plane.h  |  5 +++++
>  drivers/gpu/ipu-v3/ipu-dp.c        |  3 ---
>  5 files changed, 62 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 0a5e4fbb906bf..94f9c25e1c67b 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 *plane_state;
> +	bool plane_disabling = false;
> +	int i;
>  
>  	drm_atomic_helper_commit_modeset_disables(dev, state);
>  
> @@ -169,6 +174,19 @@ static void imx_drm_atomic_commit_tail(struct drm_atomic_state *state)
>  
>  	drm_atomic_helper_commit_modeset_enables(dev, state);
>  
> +	for_each_plane_in_state(state, plane, plane_state, i) {
> +		if (drm_atomic_plane_disabling(plane, plane_state))
> +			plane_disabling = true;
> +	}
> +
> +	if (plane_disabling) {
> +		drm_atomic_helper_wait_for_vblanks(dev, state);
> +
> +		for_each_plane_in_state(state, plane, plane_state, i)
> +			ipu_plane_disable_deferred(plane);
> +
> +	}
> +
>  	drm_atomic_helper_commit_hw_done(state);
>  }
>  
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index 6be515a9fb694..0f15f11f26e0c 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);
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index 55991d46ced50..a37735298615e 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, true);
> +	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)
>  {
> @@ -356,7 +363,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-dp.c b/drivers/gpu/ipu-v3/ipu-dp.c
> index 0e09c98248a0d..9b2b3fa479c46 100644
> --- a/drivers/gpu/ipu-v3/ipu-dp.c
> +++ b/drivers/gpu/ipu-v3/ipu-dp.c
> @@ -277,9 +277,6 @@ void ipu_dp_disable_channel(struct ipu_dp *dp, bool sync)
>  	writel(0, flow->base + DP_FG_POS);
>  	ipu_srm_dp_update(priv->ipu, sync);
>  
> -	if (ipu_idmac_channel_busy(priv->ipu, IPUV3_CHANNEL_MEM_BG_SYNC))
> -		ipu_wait_interrupt(priv->ipu, IPU_IRQ_DP_SF_END, 50);
> -
>  	mutex_unlock(&priv->mutex);
>  }
>  EXPORT_SYMBOL_GPL(ipu_dp_disable_channel);


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