dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] Fix DP busy wait and defer disabling overlay plane
@ 2017-02-27 13:14 Philipp Zabel
  2017-02-27 13:14 ` [PATCH v2 1/4] gpu: ipu-v3: remove IRQ dance on DC channel disable Philipp Zabel
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Philipp Zabel @ 2017-02-27 13:14 UTC (permalink / raw)
  To: dri-devel; +Cc: Russell King, Dan MacDonald, kernel

Hi,

second try. This time keeping the IPU_SRM_PRI2 register values intact.

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: call drm_atomic_helper_commit_hw_done after
    drm_atomic_helper_wait_for_vblanks
  drm/imx: add deferred plane disabling

 drivers/gpu/drm/imx/imx-drm-core.c | 11 +++++--
 drivers/gpu/drm/imx/ipuv3-crtc.c   | 22 +++++++++++++-
 drivers/gpu/drm/imx/ipuv3-plane.c  | 24 ++++++++++-----
 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, 74 insertions(+), 81 deletions(-)

-- 
2.11.0

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/4] gpu: ipu-v3: remove IRQ dance on DC channel disable
  2017-02-27 13:14 [PATCH v2 0/4] Fix DP busy wait and defer disabling overlay plane Philipp Zabel
@ 2017-02-27 13:14 ` Philipp Zabel
  2017-02-27 13:14 ` [PATCH v2 2/4] gpu: ipu-v3: add unsynchronised DP channel disabling Philipp Zabel
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Philipp Zabel @ 2017-02-27 13:14 UTC (permalink / raw)
  To: dri-devel; +Cc: Russell King, Dan MacDonald, kernel

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/4] gpu: ipu-v3: add unsynchronised DP channel disabling
  2017-02-27 13:14 [PATCH v2 0/4] Fix DP busy wait and defer disabling overlay plane Philipp Zabel
  2017-02-27 13:14 ` [PATCH v2 1/4] gpu: ipu-v3: remove IRQ dance on DC channel disable Philipp Zabel
@ 2017-02-27 13:14 ` Philipp Zabel
  2017-02-27 13:35   ` Lucas Stach
  2017-02-27 13:14 ` [PATCH v2 3/4] drm/imx: call drm_atomic_helper_commit_hw_done after drm_atomic_helper_wait_for_vblanks Philipp Zabel
  2017-02-27 13:14 ` [PATCH v2 4/4] drm/imx: add deferred plane disabling Philipp Zabel
  3 siblings, 1 reply; 10+ messages in thread
From: Philipp Zabel @ 2017-02-27 13:14 UTC (permalink / raw)
  To: dri-devel; +Cc: Russell King, Dan MacDonald, kernel

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>
---
Changes since v1:
 - Only clear the DP_S_SRM_MODE bitfield in the IPU_SRM_PRI2 register, not
   everything else.
---
 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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/4] drm/imx: call drm_atomic_helper_commit_hw_done after drm_atomic_helper_wait_for_vblanks
  2017-02-27 13:14 [PATCH v2 0/4] Fix DP busy wait and defer disabling overlay plane Philipp Zabel
  2017-02-27 13:14 ` [PATCH v2 1/4] gpu: ipu-v3: remove IRQ dance on DC channel disable Philipp Zabel
  2017-02-27 13:14 ` [PATCH v2 2/4] gpu: ipu-v3: add unsynchronised DP channel disabling Philipp Zabel
@ 2017-02-27 13:14 ` Philipp Zabel
  2017-02-27 16:25   ` Daniel Vetter
  2017-02-27 13:14 ` [PATCH v2 4/4] drm/imx: add deferred plane disabling Philipp Zabel
  3 siblings, 1 reply; 10+ messages in thread
From: Philipp Zabel @ 2017-02-27 13:14 UTC (permalink / raw)
  To: dri-devel; +Cc: Russell King, Dan MacDonald, kernel

Disabling planes will consist of two steps as of the following patch.
First, the DP is asked to stop at the next vblank, and then, after the
vblank the associated IDMAC channel is idle and can be disabled.
To avoid further commits being awoken out of their wait for dependencies
too early, we should report commit_hw_done only after wait_for_vblanks.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index f562cb7964b08..1ed120c181724 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -169,10 +169,10 @@ 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_commit_hw_done(state);
+
 	drm_atomic_helper_cleanup_planes(dev, state);
 }
 
-- 
2.11.0

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 4/4] drm/imx: add deferred plane disabling
  2017-02-27 13:14 [PATCH v2 0/4] Fix DP busy wait and defer disabling overlay plane Philipp Zabel
                   ` (2 preceding siblings ...)
  2017-02-27 13:14 ` [PATCH v2 3/4] drm/imx: call drm_atomic_helper_commit_hw_done after drm_atomic_helper_wait_for_vblanks Philipp Zabel
@ 2017-02-27 13:14 ` Philipp Zabel
  3 siblings, 0 replies; 10+ messages in thread
From: Philipp Zabel @ 2017-02-27 13:14 UTC (permalink / raw)
  To: dri-devel; +Cc: Russell King, Dan MacDonald, kernel

The DP 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 the 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.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-drm-core.c |  7 +++++++
 drivers/gpu/drm/imx/ipuv3-crtc.c   | 22 +++++++++++++++++++++-
 drivers/gpu/drm/imx/ipuv3-plane.c  | 24 +++++++++++++++++-------
 drivers/gpu/drm/imx/ipuv3-plane.h  |  5 +++++
 drivers/gpu/ipu-v3/ipu-dp.c        |  3 ---
 5 files changed, 50 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 1ed120c181724..7cfc52fe33339 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,9 @@ 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;
+	int i;
 
 	drm_atomic_helper_commit_modeset_disables(dev, state);
 
@@ -171,6 +175,9 @@ static void imx_drm_atomic_commit_tail(struct drm_atomic_state *state)
 
 	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);
 
 	drm_atomic_helper_cleanup_planes(dev, 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..96b6299d7fa63 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -172,22 +172,28 @@ 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);
+	}
 }
 
 static void ipu_plane_destroy(struct drm_plane *plane)
@@ -356,7 +362,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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/4] gpu: ipu-v3: add unsynchronised DP channel disabling
  2017-02-27 13:14 ` [PATCH v2 2/4] gpu: ipu-v3: add unsynchronised DP channel disabling Philipp Zabel
@ 2017-02-27 13:35   ` Lucas Stach
  0 siblings, 0 replies; 10+ messages in thread
From: Lucas Stach @ 2017-02-27 13:35 UTC (permalink / raw)
  To: Philipp Zabel; +Cc: kernel, Russell King, Dan MacDonald, dri-devel

Am Montag, den 27.02.2017, 14:14 +0100 schrieb 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>

> ---
> Changes since v1:
>  - Only clear the DP_S_SRM_MODE bitfield in the IPU_SRM_PRI2 register, not
>    everything else.
> ---
>  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);


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/4] drm/imx: call drm_atomic_helper_commit_hw_done after drm_atomic_helper_wait_for_vblanks
  2017-02-27 13:14 ` [PATCH v2 3/4] drm/imx: call drm_atomic_helper_commit_hw_done after drm_atomic_helper_wait_for_vblanks Philipp Zabel
@ 2017-02-27 16:25   ` Daniel Vetter
  2017-02-28  9:59     ` Philipp Zabel
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2017-02-27 16:25 UTC (permalink / raw)
  To: Philipp Zabel; +Cc: kernel, Russell King, Dan MacDonald, dri-devel

On Mon, Feb 27, 2017 at 02:14:57PM +0100, Philipp Zabel wrote:
> Disabling planes will consist of two steps as of the following patch.
> First, the DP is asked to stop at the next vblank, and then, after the
> vblank the associated IDMAC channel is idle and can be disabled.
> To avoid further commits being awoken out of their wait for dependencies
> too early, we should report commit_hw_done only after wait_for_vblanks.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index f562cb7964b08..1ed120c181724 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -169,10 +169,10 @@ 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_commit_hw_done(state);

That gives you a pretty good chance of halfing your refresh rate for async
flips. At least if you're not super careful about things. Did you
double-check this?
-Daniel

> +
>  	drm_atomic_helper_cleanup_planes(dev, state);
>  }
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/4] drm/imx: call drm_atomic_helper_commit_hw_done after drm_atomic_helper_wait_for_vblanks
  2017-02-27 16:25   ` Daniel Vetter
@ 2017-02-28  9:59     ` Philipp Zabel
  2017-02-28 11:45       ` Daniel Vetter
  0 siblings, 1 reply; 10+ messages in thread
From: Philipp Zabel @ 2017-02-28  9:59 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: kernel, Russell King, Dan MacDonald, dri-devel

On Mon, 2017-02-27 at 17:25 +0100, Daniel Vetter wrote:
> On Mon, Feb 27, 2017 at 02:14:57PM +0100, Philipp Zabel wrote:
> > Disabling planes will consist of two steps as of the following patch.
> > First, the DP is asked to stop at the next vblank, and then, after the
> > vblank the associated IDMAC channel is idle and can be disabled.
> > To avoid further commits being awoken out of their wait for dependencies
> > too early, we should report commit_hw_done only after wait_for_vblanks.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > index f562cb7964b08..1ed120c181724 100644
> > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > @@ -169,10 +169,10 @@ 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_commit_hw_done(state);
> 
> That gives you a pretty good chance of halfing your refresh rate for async
> flips. At least if you're not super careful about things. Did you
> double-check this?
> -Daniel

I'm not used to the terminology, does async flips just mean
DRM_MODE_PAGE_FLIP_ASYNC? We can't support that at all, since the
hardware can't flip during hblank. We are not allowed to touch or even
look at the DMA channel parameter memory during the whole frame,
unfortunately.

With sync flips during vblank, which is the only thing we can support,
the only place waiting for the hw_done completion is
drm_atomic_helper_wait_for_dependencies, which goes on to wait for the
same commit's flip_done completion right afterwards. That is completed
by the same vblank IRQ that wait_for_vblanks is also waiting for.

So to be completely correct, we could report hw_done before
wait_for_vblanks only if there are no plane disables pending, and
afterwards otherwise, but I don't think this should make a difference
right now.

regards
Philipp

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/4] drm/imx: call drm_atomic_helper_commit_hw_done after drm_atomic_helper_wait_for_vblanks
  2017-02-28  9:59     ` Philipp Zabel
@ 2017-02-28 11:45       ` Daniel Vetter
  2017-02-28 13:23         ` Philipp Zabel
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2017-02-28 11:45 UTC (permalink / raw)
  To: Philipp Zabel; +Cc: kernel, Russell King, dri-devel, Dan MacDonald

On Tue, Feb 28, 2017 at 10:59:54AM +0100, Philipp Zabel wrote:
> On Mon, 2017-02-27 at 17:25 +0100, Daniel Vetter wrote:
> > On Mon, Feb 27, 2017 at 02:14:57PM +0100, Philipp Zabel wrote:
> > > Disabling planes will consist of two steps as of the following patch.
> > > First, the DP is asked to stop at the next vblank, and then, after the
> > > vblank the associated IDMAC channel is idle and can be disabled.
> > > To avoid further commits being awoken out of their wait for dependencies
> > > too early, we should report commit_hw_done only after wait_for_vblanks.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> > > ---
> > >  drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > > index f562cb7964b08..1ed120c181724 100644
> > > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > > @@ -169,10 +169,10 @@ 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_commit_hw_done(state);
> > 
> > That gives you a pretty good chance of halfing your refresh rate for async
> > flips. At least if you're not super careful about things. Did you
> > double-check this?
> > -Daniel
> 
> I'm not used to the terminology, does async flips just mean
> DRM_MODE_PAGE_FLIP_ASYNC? We can't support that at all, since the
> hardware can't flip during hblank. We are not allowed to touch or even
> look at the DMA channel parameter memory during the whole frame,
> unfortunately.

sorry, I mixed up async with nonblocking. I meant nonblocking.

> With sync flips during vblank, which is the only thing we can support,
> the only place waiting for the hw_done completion is
> drm_atomic_helper_wait_for_dependencies, which goes on to wait for the
> same commit's flip_done completion right afterwards. That is completed
> by the same vblank IRQ that wait_for_vblanks is also waiting for.
> 
> So to be completely correct, we could report hw_done before
> wait_for_vblanks only if there are no plane disables pending, and
> afterwards otherwise, but I don't think this should make a difference
> right now.

if it works, then it's all fine. Just wanted to raise here that this went
wrong before ... The issue is that the _next_ update only waits for
hw_done, not for the entire previous commit work item to complete. So
there's a very real chance that the next nonblocking commit gets delayed,
since you reduce the amount of interleaving they support. It mostly works,
except when you get it wrong, or when you're unlucky about races.

I think a better fix here would be to put the vblank wait into the disable
function of your DP port, to make sure you only incur the vblank delay for
the case that needs it, and not for the usual case of a stream of screen
updates. hw_done means "I stopped touching the hw, the next thread can
jump in", not "the hw is done touching stuff".

Oh and if this is all unclear in the docs, pls submit a patch to clarify
things more.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/4] drm/imx: call drm_atomic_helper_commit_hw_done after drm_atomic_helper_wait_for_vblanks
  2017-02-28 11:45       ` Daniel Vetter
@ 2017-02-28 13:23         ` Philipp Zabel
  0 siblings, 0 replies; 10+ messages in thread
From: Philipp Zabel @ 2017-02-28 13:23 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: kernel, Russell King, Dan MacDonald, dri-devel

On Tue, 2017-02-28 at 12:45 +0100, Daniel Vetter wrote:
> On Tue, Feb 28, 2017 at 10:59:54AM +0100, Philipp Zabel wrote:
> > On Mon, 2017-02-27 at 17:25 +0100, Daniel Vetter wrote:
> > > On Mon, Feb 27, 2017 at 02:14:57PM +0100, Philipp Zabel wrote:
> > > > Disabling planes will consist of two steps as of the following patch.
> > > > First, the DP is asked to stop at the next vblank, and then, after the
> > > > vblank the associated IDMAC channel is idle and can be disabled.
> > > > To avoid further commits being awoken out of their wait for dependencies
> > > > too early, we should report commit_hw_done only after wait_for_vblanks.
> > > > 
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> > > > ---
> > > >  drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > > > index f562cb7964b08..1ed120c181724 100644
> > > > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > > > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > > > @@ -169,10 +169,10 @@ 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_commit_hw_done(state);
> > > 
> > > That gives you a pretty good chance of halfing your refresh rate for async
> > > flips. At least if you're not super careful about things. Did you
> > > double-check this?
> > > -Daniel
> > 
> > I'm not used to the terminology, does async flips just mean
> > DRM_MODE_PAGE_FLIP_ASYNC? We can't support that at all, since the
> > hardware can't flip during hblank. We are not allowed to touch or even
> > look at the DMA channel parameter memory during the whole frame,
> > unfortunately.
> 
> sorry, I mixed up async with nonblocking. I meant nonblocking.

Oh, ok.

> > With sync flips during vblank, which is the only thing we can support,
> > the only place waiting for the hw_done completion is
> > drm_atomic_helper_wait_for_dependencies, which goes on to wait for the
> > same commit's flip_done completion right afterwards. That is completed
> > by the same vblank IRQ that wait_for_vblanks is also waiting for.
> > 
> > So to be completely correct, we could report hw_done before
> > wait_for_vblanks only if there are no plane disables pending, and
> > afterwards otherwise, but I don't think this should make a difference
> > right now.
> 
> if it works, then it's all fine. Just wanted to raise here that this went
> wrong before ... The issue is that the _next_ update only waits for
> hw_done, not for the entire previous commit work item to complete. So
> there's a very real chance that the next nonblocking commit gets delayed,
> since you reduce the amount of interleaving they support. It mostly works,
> except when you get it wrong, or when you're unlucky about races.

I appreciate the hint. We should remove the wait_for_vblanks and
cleanup_planes calls from the commit_tail completely if no plane is
currently being disabled, as there is no cleanup_fb for CMA based planes
anyway.

> I think a better fix here would be to put the vblank wait into the disable
> function of your DP port, to make sure you only incur the vblank delay for
> the case that needs it, and not for the usual case of a stream of screen
> updates.

Sorry for being unclear, DP in this context doesn't mean DisplayPort,
it's just a part of the display pipeline between DMA controller and
display controller that handles colorspace conversion and compositing.
It just happens to be called "display processor".

> hw_done means "I stopped touching the hw, the next thread can
> jump in", not "the hw is done touching stuff".
> 
> Oh and if this is all unclear in the docs, pls submit a patch to clarify
> things more.

Yes, that much is clear. We really have to tell the overlay plane to
please stop at the next vblank, then wait for the vblank, and only then
touch the stopped hardware to finalize disabling the plane.

regards
Philipp

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-02-28 13:23 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-27 13:14 [PATCH v2 0/4] Fix DP busy wait and defer disabling overlay plane Philipp Zabel
2017-02-27 13:14 ` [PATCH v2 1/4] gpu: ipu-v3: remove IRQ dance on DC channel disable Philipp Zabel
2017-02-27 13:14 ` [PATCH v2 2/4] gpu: ipu-v3: add unsynchronised DP channel disabling Philipp Zabel
2017-02-27 13:35   ` Lucas Stach
2017-02-27 13:14 ` [PATCH v2 3/4] drm/imx: call drm_atomic_helper_commit_hw_done after drm_atomic_helper_wait_for_vblanks Philipp Zabel
2017-02-27 16:25   ` Daniel Vetter
2017-02-28  9:59     ` Philipp Zabel
2017-02-28 11:45       ` Daniel Vetter
2017-02-28 13:23         ` Philipp Zabel
2017-02-27 13:14 ` [PATCH v2 4/4] drm/imx: add deferred plane disabling Philipp Zabel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).