[PATCH 00/14] imx drm atomic mode setting conversion

[PATCH 00/14] imx drm atomic mode setting conversion

[Liu Ying]

Hi,

This patch set converts imx drm into atomic mode setting.
It takes 3 phases to achieve the goal as recommended.
This patch set may apply to Philipp Zabel's open git branch imx-drm/fixes.

Liu Ying (14):
  drm/imx: ipuv3-plane: Constify ipu_plane_funcs
  drm/imx: plane: Don't set plane->crtc in ipu_plane_update()
  drm: imx: ipuv3 plane: Check different types of plane separately
  gpu: ipu-v3: ipu-dmfc: Use static DMFC FIFO allocation mechanism
  drm/crtc_helper: Disable and reenable primary plane in
    drm_helper_crtc_mode_set
  drm/imx: atomic phase 1: Use transitional atomic CRTC and plane
    helpers
  drm/imx: atomic phase 2 step 1: Wire up state ->reset, ->duplicate and
    ->destroy
  drm/imx: atomic phase 2 step 2: Track plane_state->fb correctly in
    ->page_flip
  drm/imx: atomic phase 3 step 1: Atomic updates for planes
  drm/atomic-helper: Disable planes when suspending
  drm/imx: atomic phase 3 step 2: Use atomic configuration
  drm/imx: atomic phase 3 step 3: Legacy callback fixups
  drm/imx: atomic phase 3 step 4: Use generic atomic page flip
  drm/imx: atomic phase 3 step 5: Advertise DRIVER_ATOMIC

 drivers/gpu/drm/bridge/dw-hdmi.c       |  19 +-
 drivers/gpu/drm/drm_atomic_helper.c    |  18 +-
 drivers/gpu/drm/drm_crtc_helper.c      |   9 +
 drivers/gpu/drm/imx/dw_hdmi-imx.c      |  22 +-
 drivers/gpu/drm/imx/imx-drm-core.c     | 192 +++++++++---
 drivers/gpu/drm/imx/imx-drm.h          |  11 +-
 drivers/gpu/drm/imx/imx-ldb.c          | 129 ++++----
 drivers/gpu/drm/imx/imx-tve.c          |  75 ++---
 drivers/gpu/drm/imx/ipuv3-crtc.c       | 331 +++++---------------
 drivers/gpu/drm/imx/ipuv3-plane.c      | 538 +++++++++++++++++++--------------
 drivers/gpu/drm/imx/ipuv3-plane.h      |  14 +-
 drivers/gpu/drm/imx/parallel-display.c |  74 ++---
 drivers/gpu/ipu-v3/ipu-dc.c            |   5 +-
 drivers/gpu/ipu-v3/ipu-di.c            |   3 -
 drivers/gpu/ipu-v3/ipu-dmfc.c          | 213 +------------
 include/video/imx-ipu-v3.h             |   3 -
 16 files changed, 720 insertions(+), 936 deletions(-)

-- 
2.7.4

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

[PATCH 01/14] drm/imx: ipuv3-plane: Constify ipu_plane_funcs

[Liu Ying]

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 681ec6e..157032d 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -461,7 +461,7 @@ static void ipu_plane_destroy(struct drm_plane *plane)
 	kfree(ipu_plane);
 }
 
-static struct drm_plane_funcs ipu_plane_funcs = {
+static const struct drm_plane_funcs ipu_plane_funcs = {
 	.update_plane	= ipu_update_plane,
 	.disable_plane	= ipu_disable_plane,
 	.destroy	= ipu_plane_destroy,
-- 
2.7.4

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

[PATCH 02/14] drm/imx: plane: Don't set plane->crtc in ipu_plane_update()

[Liu Ying]

Since the drm core sets plane->crtc correctly, we don't need to do that.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 157032d..e6ec8eb 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -428,7 +428,6 @@ static int ipu_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (crtc != plane->crtc)
 		dev_dbg(plane->dev->dev, "crtc change: %p -> %p\n",
 				plane->crtc, crtc);
-	plane->crtc = crtc;
 
 	if (!ipu_plane->enabled)
 		ipu_plane_enable(ipu_plane);
-- 
2.7.4

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

[PATCH 03/14] drm: imx: ipuv3 plane: Check different types of plane separately

[Liu Ying]

The IPUv3 primary plane doesn't support partial off screen.
So, this patch separates plane check logics for primary plane and overlay
plane and adds more limitations on the primary plane.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 72 ++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index e6ec8eb..8f91b2e 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -190,44 +190,54 @@ int ipu_plane_mode_set(struct ipu_plane *ipu_plane, struct drm_crtc *crtc,
 		       uint32_t src_x, uint32_t src_y,
 		       uint32_t src_w, uint32_t src_h, bool interlaced)
 {
-	struct device *dev = ipu_plane->base.dev->dev;
+	struct drm_plane plane = ipu_plane->base;
+	struct device *dev = plane.dev->dev;
 	int ret;
 
 	/* no scaling */
 	if (src_w != crtc_w || src_h != crtc_h)
 		return -EINVAL;
 
-	/* clip to crtc bounds */
-	if (crtc_x < 0) {
-		if (-crtc_x > crtc_w)
+	if (plane.type == DRM_PLANE_TYPE_PRIMARY) {
+		/* full plane doesn't support partial off screen */
+		if (crtc_x || crtc_y || crtc_w != mode->hdisplay ||
+			crtc_h != mode->vdisplay)
 			return -EINVAL;
-		src_x += -crtc_x;
-		src_w -= -crtc_x;
-		crtc_w -= -crtc_x;
-		crtc_x = 0;
-	}
-	if (crtc_y < 0) {
-		if (-crtc_y > crtc_h)
-			return -EINVAL;
-		src_y += -crtc_y;
-		src_h -= -crtc_y;
-		crtc_h -= -crtc_y;
-		crtc_y = 0;
-	}
-	if (crtc_x + crtc_w > mode->hdisplay) {
-		if (crtc_x > mode->hdisplay)
-			return -EINVAL;
-		crtc_w = mode->hdisplay - crtc_x;
-		src_w = crtc_w;
-	}
-	if (crtc_y + crtc_h > mode->vdisplay) {
-		if (crtc_y > mode->vdisplay)
+
+		/* full plane minimum width is 13 pixels */
+		if (crtc_w < 13)
 			return -EINVAL;
-		crtc_h = mode->vdisplay - crtc_y;
-		src_h = crtc_h;
-	}
-	/* full plane minimum width is 13 pixels */
-	if (crtc_w < 13 && (ipu_plane->dp_flow != IPU_DP_FLOW_SYNC_FG))
+	} else if (plane.type == DRM_PLANE_TYPE_OVERLAY) {
+		/* clip to crtc bounds */
+		if (crtc_x < 0) {
+			if (-crtc_x > crtc_w)
+				return -EINVAL;
+			src_x += -crtc_x;
+			src_w -= -crtc_x;
+			crtc_w -= -crtc_x;
+			crtc_x = 0;
+		}
+		if (crtc_y < 0) {
+			if (-crtc_y > crtc_h)
+				return -EINVAL;
+			src_y += -crtc_y;
+			src_h -= -crtc_y;
+			crtc_h -= -crtc_y;
+			crtc_y = 0;
+		}
+		if (crtc_x + crtc_w > mode->hdisplay) {
+			if (crtc_x > mode->hdisplay)
+				return -EINVAL;
+			crtc_w = mode->hdisplay - crtc_x;
+			src_w = crtc_w;
+		}
+		if (crtc_y + crtc_h > mode->vdisplay) {
+			if (crtc_y > mode->vdisplay)
+				return -EINVAL;
+			crtc_h = mode->vdisplay - crtc_y;
+			src_h = crtc_h;
+		}
+	} else
 		return -EINVAL;
 	if (crtc_h < 2)
 		return -EINVAL;
@@ -238,7 +248,7 @@ int ipu_plane_mode_set(struct ipu_plane *ipu_plane, struct drm_crtc *crtc,
 	 */
 	if (ipu_plane->enabled) {
 		if (src_w != ipu_plane->w || src_h != ipu_plane->h ||
-		    fb->pixel_format != ipu_plane->base.fb->pixel_format)
+		    fb->pixel_format != plane.fb->pixel_format)
 			return -EINVAL;
 
 		return ipu_plane_set_base(ipu_plane, fb, src_x, src_y);
-- 
2.7.4

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

[PATCH 04/14] gpu: ipu-v3: ipu-dmfc: Use static DMFC FIFO allocation mechanism

[Liu Ying]

For normal video modes such as 1280x720p@60Hz and 1920x1080p@60Hz,
we always get 2 slots for a plane by using the current existing
dynamic DMFC FIFO allocation mechanism.  So, let's change to use
the static one to simplify the code.  This also makes it easier to
implement the atomic mode setting as we don't need to handle
allocation failure cases then.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/ipuv3-plane.c |  26 -----
 drivers/gpu/ipu-v3/ipu-dmfc.c     | 213 ++------------------------------------
 include/video/imx-ipu-v3.h        |   3 -
 3 files changed, 7 insertions(+), 235 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 8f91b2e..30cedbb 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -51,24 +51,6 @@ int ipu_plane_irq(struct ipu_plane *ipu_plane)
 				     IPU_IRQ_EOF);
 }
 
-static int calc_vref(struct drm_display_mode *mode)
-{
-	unsigned long htotal, vtotal;
-
-	htotal = mode->htotal;
-	vtotal = mode->vtotal;
-
-	if (!htotal || !vtotal)
-		return 60;
-
-	return DIV_ROUND_UP(mode->clock * 1000, vtotal * htotal);
-}
-
-static inline int calc_bandwidth(int width, int height, unsigned int vref)
-{
-	return width * height * vref;
-}
-
 int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
 		       int x, int y)
 {
@@ -290,14 +272,6 @@ int ipu_plane_mode_set(struct ipu_plane *ipu_plane, struct drm_crtc *crtc,
 		}
 	}
 
-	ret = ipu_dmfc_alloc_bandwidth(ipu_plane->dmfc,
-			calc_bandwidth(crtc_w, crtc_h,
-				       calc_vref(mode)), 64);
-	if (ret) {
-		dev_err(dev, "allocating dmfc bandwidth failed with %d\n", ret);
-		return ret;
-	}
-
 	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, crtc_w);
 
 	ipu_cpmem_zero(ipu_plane->ipu_ch);
diff --git a/drivers/gpu/ipu-v3/ipu-dmfc.c b/drivers/gpu/ipu-v3/ipu-dmfc.c
index 837b1ec2..42705bb 100644
--- a/drivers/gpu/ipu-v3/ipu-dmfc.c
+++ b/drivers/gpu/ipu-v3/ipu-dmfc.c
@@ -45,17 +45,6 @@
 #define DMFC_DP_CHAN_6B_24		16
 #define DMFC_DP_CHAN_6F_29		24
 
-#define DMFC_FIFO_SIZE_64		(3 << 3)
-#define DMFC_FIFO_SIZE_128		(2 << 3)
-#define DMFC_FIFO_SIZE_256		(1 << 3)
-#define DMFC_FIFO_SIZE_512		(0 << 3)
-
-#define DMFC_SEGMENT(x)			((x & 0x7) << 0)
-#define DMFC_BURSTSIZE_128		(0 << 6)
-#define DMFC_BURSTSIZE_64		(1 << 6)
-#define DMFC_BURSTSIZE_32		(2 << 6)
-#define DMFC_BURSTSIZE_16		(3 << 6)
-
 struct dmfc_channel_data {
 	int		ipu_channel;
 	unsigned long	channel_reg;
@@ -104,9 +93,6 @@ struct ipu_dmfc_priv;
 
 struct dmfc_channel {
 	unsigned			slots;
-	unsigned			slotmask;
-	unsigned			segment;
-	int				burstsize;
 	struct ipu_soc			*ipu;
 	struct ipu_dmfc_priv		*priv;
 	const struct dmfc_channel_data	*data;
@@ -117,7 +103,6 @@ struct ipu_dmfc_priv {
 	struct device *dev;
 	struct dmfc_channel channels[DMFC_NUM_CHANNELS];
 	struct mutex mutex;
-	unsigned long bandwidth_per_slot;
 	void __iomem *base;
 	int use_count;
 };
@@ -172,184 +157,6 @@ void ipu_dmfc_disable_channel(struct dmfc_channel *dmfc)
 }
 EXPORT_SYMBOL_GPL(ipu_dmfc_disable_channel);
 
-static int ipu_dmfc_setup_channel(struct dmfc_channel *dmfc, int slots,
-		int segment, int burstsize)
-{
-	struct ipu_dmfc_priv *priv = dmfc->priv;
-	u32 val, field;
-
-	dev_dbg(priv->dev,
-			"dmfc: using %d slots starting from segment %d for IPU channel %d\n",
-			slots, segment, dmfc->data->ipu_channel);
-
-	switch (slots) {
-	case 1:
-		field = DMFC_FIFO_SIZE_64;
-		break;
-	case 2:
-		field = DMFC_FIFO_SIZE_128;
-		break;
-	case 4:
-		field = DMFC_FIFO_SIZE_256;
-		break;
-	case 8:
-		field = DMFC_FIFO_SIZE_512;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	switch (burstsize) {
-	case 16:
-		field |= DMFC_BURSTSIZE_16;
-		break;
-	case 32:
-		field |= DMFC_BURSTSIZE_32;
-		break;
-	case 64:
-		field |= DMFC_BURSTSIZE_64;
-		break;
-	case 128:
-		field |= DMFC_BURSTSIZE_128;
-		break;
-	}
-
-	field |= DMFC_SEGMENT(segment);
-
-	val = readl(priv->base + dmfc->data->channel_reg);
-
-	val &= ~(0xff << dmfc->data->shift);
-	val |= field << dmfc->data->shift;
-
-	writel(val, priv->base + dmfc->data->channel_reg);
-
-	dmfc->slots = slots;
-	dmfc->segment = segment;
-	dmfc->burstsize = burstsize;
-	dmfc->slotmask = ((1 << slots) - 1) << segment;
-
-	return 0;
-}
-
-static int dmfc_bandwidth_to_slots(struct ipu_dmfc_priv *priv,
-		unsigned long bandwidth)
-{
-	int slots = 1;
-
-	while (slots * priv->bandwidth_per_slot < bandwidth)
-		slots *= 2;
-
-	return slots;
-}
-
-static int dmfc_find_slots(struct ipu_dmfc_priv *priv, int slots)
-{
-	unsigned slotmask_need, slotmask_used = 0;
-	int i, segment = 0;
-
-	slotmask_need = (1 << slots) - 1;
-
-	for (i = 0; i < DMFC_NUM_CHANNELS; i++)
-		slotmask_used |= priv->channels[i].slotmask;
-
-	while (slotmask_need <= 0xff) {
-		if (!(slotmask_used & slotmask_need))
-			return segment;
-
-		slotmask_need <<= 1;
-		segment++;
-	}
-
-	return -EBUSY;
-}
-
-void ipu_dmfc_free_bandwidth(struct dmfc_channel *dmfc)
-{
-	struct ipu_dmfc_priv *priv = dmfc->priv;
-	int i;
-
-	dev_dbg(priv->dev, "dmfc: freeing %d slots starting from segment %d\n",
-			dmfc->slots, dmfc->segment);
-
-	mutex_lock(&priv->mutex);
-
-	if (!dmfc->slots)
-		goto out;
-
-	dmfc->slotmask = 0;
-	dmfc->slots = 0;
-	dmfc->segment = 0;
-
-	for (i = 0; i < DMFC_NUM_CHANNELS; i++)
-		priv->channels[i].slotmask = 0;
-
-	for (i = 0; i < DMFC_NUM_CHANNELS; i++) {
-		if (priv->channels[i].slots > 0) {
-			priv->channels[i].segment =
-				dmfc_find_slots(priv, priv->channels[i].slots);
-			priv->channels[i].slotmask =
-				((1 << priv->channels[i].slots) - 1) <<
-				priv->channels[i].segment;
-		}
-	}
-
-	for (i = 0; i < DMFC_NUM_CHANNELS; i++) {
-		if (priv->channels[i].slots > 0)
-			ipu_dmfc_setup_channel(&priv->channels[i],
-					priv->channels[i].slots,
-					priv->channels[i].segment,
-					priv->channels[i].burstsize);
-	}
-out:
-	mutex_unlock(&priv->mutex);
-}
-EXPORT_SYMBOL_GPL(ipu_dmfc_free_bandwidth);
-
-int ipu_dmfc_alloc_bandwidth(struct dmfc_channel *dmfc,
-		unsigned long bandwidth_pixel_per_second, int burstsize)
-{
-	struct ipu_dmfc_priv *priv = dmfc->priv;
-	int slots = dmfc_bandwidth_to_slots(priv, bandwidth_pixel_per_second);
-	int segment = -1, ret = 0;
-
-	dev_dbg(priv->dev, "dmfc: trying to allocate %ldMpixel/s for IPU channel %d\n",
-			bandwidth_pixel_per_second / 1000000,
-			dmfc->data->ipu_channel);
-
-	ipu_dmfc_free_bandwidth(dmfc);
-
-	mutex_lock(&priv->mutex);
-
-	if (slots > 8) {
-		ret = -EBUSY;
-		goto out;
-	}
-
-	/* For the MEM_BG channel, first try to allocate twice the slots */
-	if (dmfc->data->ipu_channel == IPUV3_CHANNEL_MEM_BG_SYNC)
-		segment = dmfc_find_slots(priv, slots * 2);
-	else if (slots < 2)
-		/* Always allocate at least 128*4 bytes (2 slots) */
-		slots = 2;
-
-	if (segment >= 0)
-		slots *= 2;
-	else
-		segment = dmfc_find_slots(priv, slots);
-	if (segment < 0) {
-		ret = -EBUSY;
-		goto out;
-	}
-
-	ipu_dmfc_setup_channel(dmfc, slots, segment, burstsize);
-
-out:
-	mutex_unlock(&priv->mutex);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(ipu_dmfc_alloc_bandwidth);
-
 void ipu_dmfc_config_wait4eot(struct dmfc_channel *dmfc, int width)
 {
 	struct ipu_dmfc_priv *priv = dmfc->priv;
@@ -384,7 +191,6 @@ EXPORT_SYMBOL_GPL(ipu_dmfc_get);
 
 void ipu_dmfc_put(struct dmfc_channel *dmfc)
 {
-	ipu_dmfc_free_bandwidth(dmfc);
 }
 EXPORT_SYMBOL_GPL(ipu_dmfc_put);
 
@@ -412,20 +218,15 @@ int ipu_dmfc_init(struct ipu_soc *ipu, struct device *dev, unsigned long base,
 		priv->channels[i].priv = priv;
 		priv->channels[i].ipu = ipu;
 		priv->channels[i].data = &dmfcdata[i];
-	}
-
-	writel(0x0, priv->base + DMFC_WR_CHAN);
-	writel(0x0, priv->base + DMFC_DP_CHAN);
 
-	/*
-	 * We have a total bandwidth of clkrate * 4pixel divided
-	 * into 8 slots.
-	 */
-	priv->bandwidth_per_slot = clk_get_rate(ipu_clk) * 4 / 8;
-
-	dev_dbg(dev, "dmfc: 8 slots with %ldMpixel/s bandwidth each\n",
-			priv->bandwidth_per_slot / 1000000);
+		if (dmfcdata[i].ipu_channel == IPUV3_CHANNEL_MEM_BG_SYNC ||
+		    dmfcdata[i].ipu_channel == IPUV3_CHANNEL_MEM_FG_SYNC ||
+		    dmfcdata[i].ipu_channel == IPUV3_CHANNEL_MEM_DC_SYNC)
+			priv->channels[i].slots = 2;
+	}
 
+	writel(0x00000050, priv->base + DMFC_WR_CHAN);
+	writel(0x00005654, priv->base + DMFC_DP_CHAN);
 	writel(0x202020f6, priv->base + DMFC_WR_CHAN_DEF);
 	writel(0x2020f6f6, priv->base + DMFC_DP_CHAN_DEF);
 	writel(0x00000003, priv->base + DMFC_GENERAL1);
diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
index 3a2a794..7adeaae 100644
--- a/include/video/imx-ipu-v3.h
+++ b/include/video/imx-ipu-v3.h
@@ -235,9 +235,6 @@ int ipu_di_init_sync_panel(struct ipu_di *, struct ipu_di_signal_cfg *sig);
 struct dmfc_channel;
 int ipu_dmfc_enable_channel(struct dmfc_channel *dmfc);
 void ipu_dmfc_disable_channel(struct dmfc_channel *dmfc);
-int ipu_dmfc_alloc_bandwidth(struct dmfc_channel *dmfc,
-		unsigned long bandwidth_mbs, int burstsize);
-void ipu_dmfc_free_bandwidth(struct dmfc_channel *dmfc);
 void ipu_dmfc_config_wait4eot(struct dmfc_channel *dmfc, int width);
 struct dmfc_channel *ipu_dmfc_get(struct ipu_soc *ipu, int ipuv3_channel);
 void ipu_dmfc_put(struct dmfc_channel *dmfc);
-- 
2.7.4

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

[PATCH 05/14] drm/crtc_helper: Disable and reenable primary plane in drm_helper_crtc_mode_set

[Liu Ying]

Since CRTC has already been disabled in crtc_funcs->prepare(), it doesn't hurt
to disable the primary plane in drm_helper_crtc_mode_set() before enabling it
in drm_helper_crtc_mode_set_base().  This makes those who reject active plane
update in plane_funcs->atomic_check() happy.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/drm_crtc_helper.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 79555d2..7fabcd7 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -1013,6 +1013,15 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
 			goto out;
 	}
 
+	/*
+	 * It doesn't hurt to disable primary plane here since crtc is off
+	 * and we'll enable it again in drm_helper_crtc_mode_set_base()
+	 * below soon.
+	 */
+	ret = drm_plane_helper_disable(crtc->primary);
+	if (ret)
+		goto out;
+
 	swap(crtc->state, crtc_state);
 
 	crtc_funcs->mode_set_nofb(crtc);
-- 
2.7.4

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

[PATCH 06/14] drm/imx: atomic phase 1: Use transitional atomic CRTC and plane helpers

[Liu Ying]

Use the drm_plane_helper_update/disable() and drm_helper_crtc_mode_set()
transistional atomic helpers.  The crtc->mode_set_nofb callback is added
so that the primary plane is no longer tied to the CRTC.  Check/update
logics are separated to make sure crtc->mode_set_nofb and plane->atomic_update
are always successful.  Also, some necessary logics are tweaked for a smooth
transition.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/ipuv3-crtc.c  | 173 +++++++------
 drivers/gpu/drm/imx/ipuv3-plane.c | 516 +++++++++++++++++++++++---------------
 drivers/gpu/drm/imx/ipuv3-plane.h |  14 +-
 drivers/gpu/ipu-v3/ipu-dc.c       |   5 +-
 drivers/gpu/ipu-v3/ipu-di.c       |   3 -
 5 files changed, 400 insertions(+), 311 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index b2c30b8..11fad87 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -72,7 +72,7 @@ struct ipu_crtc {
 
 #define to_ipu_crtc(x) container_of(x, struct ipu_crtc, base)
 
-static void ipu_fb_enable(struct ipu_crtc *ipu_crtc)
+static void ipu_crtc_enable(struct ipu_crtc *ipu_crtc)
 {
 	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
 
@@ -80,16 +80,19 @@ static void ipu_fb_enable(struct ipu_crtc *ipu_crtc)
 		return;
 
 	ipu_dc_enable(ipu);
-	ipu_plane_enable(ipu_crtc->plane[0]);
 	/* Start DC channel and DI after IDMAC */
 	ipu_dc_enable_channel(ipu_crtc->dc);
 	ipu_di_enable(ipu_crtc->di);
-	drm_crtc_vblank_on(&ipu_crtc->base);
-
 	ipu_crtc->enabled = 1;
+
+	/*
+	 * In order not to be warned on enabling vblank failure,
+	 * we should call drm_crtc_vblank_on() after ->enabled is set to 1.
+	 */
+	drm_crtc_vblank_on(&ipu_crtc->base);
 }
 
-static void ipu_fb_disable(struct ipu_crtc *ipu_crtc)
+static void ipu_crtc_disable(struct ipu_crtc *ipu_crtc)
 {
 	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
 
@@ -99,11 +102,10 @@ static void ipu_fb_disable(struct ipu_crtc *ipu_crtc)
 	/* Stop DC channel and DI before IDMAC */
 	ipu_dc_disable_channel(ipu_crtc->dc);
 	ipu_di_disable(ipu_crtc->di);
-	ipu_plane_disable(ipu_crtc->plane[0]);
 	ipu_dc_disable(ipu);
-	drm_crtc_vblank_off(&ipu_crtc->base);
-
 	ipu_crtc->enabled = 0;
+
+	drm_crtc_vblank_off(&ipu_crtc->base);
 }
 
 static void ipu_crtc_dpms(struct drm_crtc *crtc, int mode)
@@ -114,12 +116,12 @@ static void ipu_crtc_dpms(struct drm_crtc *crtc, int mode)
 
 	switch (mode) {
 	case DRM_MODE_DPMS_ON:
-		ipu_fb_enable(ipu_crtc);
+		ipu_crtc_enable(ipu_crtc);
 		break;
 	case DRM_MODE_DPMS_STANDBY:
 	case DRM_MODE_DPMS_SUSPEND:
 	case DRM_MODE_DPMS_OFF:
-		ipu_fb_disable(ipu_crtc);
+		ipu_crtc_disable(ipu_crtc);
 		break;
 	}
 }
@@ -233,77 +235,6 @@ static const struct drm_crtc_funcs ipu_crtc_funcs = {
 	.page_flip = ipu_page_flip,
 };
 
-static int ipu_crtc_mode_set(struct drm_crtc *crtc,
-			       struct drm_display_mode *orig_mode,
-			       struct drm_display_mode *mode,
-			       int x, int y,
-			       struct drm_framebuffer *old_fb)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_encoder *encoder;
-	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
-	struct ipu_di_signal_cfg sig_cfg = {};
-	unsigned long encoder_types = 0;
-	int ret;
-
-	dev_dbg(ipu_crtc->dev, "%s: mode->hdisplay: %d\n", __func__,
-			mode->hdisplay);
-	dev_dbg(ipu_crtc->dev, "%s: mode->vdisplay: %d\n", __func__,
-			mode->vdisplay);
-
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
-		if (encoder->crtc == crtc)
-			encoder_types |= BIT(encoder->encoder_type);
-
-	dev_dbg(ipu_crtc->dev, "%s: attached to encoder types 0x%lx\n",
-		__func__, encoder_types);
-
-	/*
-	 * If we have DAC or LDB, then we need the IPU DI clock to be
-	 * the same as the LDB DI clock. For TVDAC, derive the IPU DI
-	 * clock from 27 MHz TVE_DI clock, but allow to divide it.
-	 */
-	if (encoder_types & (BIT(DRM_MODE_ENCODER_DAC) |
-			     BIT(DRM_MODE_ENCODER_LVDS)))
-		sig_cfg.clkflags = IPU_DI_CLKMODE_SYNC | IPU_DI_CLKMODE_EXT;
-	else if (encoder_types & BIT(DRM_MODE_ENCODER_TVDAC))
-		sig_cfg.clkflags = IPU_DI_CLKMODE_EXT;
-	else
-		sig_cfg.clkflags = 0;
-
-	sig_cfg.enable_pol = 1;
-	sig_cfg.clk_pol = 0;
-	sig_cfg.bus_format = ipu_crtc->bus_format;
-	sig_cfg.v_to_h_sync = 0;
-	sig_cfg.hsync_pin = ipu_crtc->di_hsync_pin;
-	sig_cfg.vsync_pin = ipu_crtc->di_vsync_pin;
-
-	drm_display_mode_to_videomode(mode, &sig_cfg.mode);
-
-	ret = ipu_dc_init_sync(ipu_crtc->dc, ipu_crtc->di,
-			       mode->flags & DRM_MODE_FLAG_INTERLACE,
-			       ipu_crtc->bus_format, mode->hdisplay);
-	if (ret) {
-		dev_err(ipu_crtc->dev,
-				"initializing display controller failed with %d\n",
-				ret);
-		return ret;
-	}
-
-	ret = ipu_di_init_sync_panel(ipu_crtc->di, &sig_cfg);
-	if (ret) {
-		dev_err(ipu_crtc->dev,
-				"initializing panel failed with %d\n", ret);
-		return ret;
-	}
-
-	return ipu_plane_mode_set(ipu_crtc->plane[0], crtc, mode,
-				  crtc->primary->fb,
-				  0, 0, mode->hdisplay, mode->vdisplay,
-				  x, y, mode->hdisplay, mode->vdisplay,
-				  mode->flags & DRM_MODE_FLAG_INTERLACE);
-}
-
 static void ipu_crtc_handle_pageflip(struct ipu_crtc *ipu_crtc)
 {
 	unsigned long flags;
@@ -327,8 +258,7 @@ static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
 	if (ipu_crtc->flip_state == IPU_FLIP_SUBMITTED) {
 		struct ipu_plane *plane = ipu_crtc->plane[0];
 
-		ipu_plane_set_base(plane, ipu_crtc->base.primary->fb,
-				   plane->x, plane->y);
+		ipu_plane_set_base(plane, ipu_crtc->base.primary->fb);
 		ipu_crtc_handle_pageflip(ipu_crtc);
 		queue_work(ipu_crtc->flip_queue,
 			   &ipu_crtc->flip_work->unref_work);
@@ -352,6 +282,9 @@ static bool ipu_crtc_mode_fixup(struct drm_crtc *crtc,
 	if (ret)
 		return false;
 
+	if ((vm.vsync_len == 0) || (vm.hsync_len == 0))
+		return false;
+
 	drm_display_mode_from_videomode(&vm, adjusted_mode);
 
 	return true;
@@ -361,28 +294,93 @@ static void ipu_crtc_prepare(struct drm_crtc *crtc)
 {
 	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
 
-	ipu_fb_disable(ipu_crtc);
+	ipu_crtc_disable(ipu_crtc);
 }
 
 static void ipu_crtc_commit(struct drm_crtc *crtc)
 {
 	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
 
-	ipu_fb_enable(ipu_crtc);
+	ipu_crtc_enable(ipu_crtc);
+}
+
+static int ipu_crtc_atomic_check(struct drm_crtc *crtc,
+				 struct drm_crtc_state *state)
+{
+	return 0;
+}
+
+static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_encoder *encoder;
+	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
+	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
+	struct ipu_di_signal_cfg sig_cfg = {};
+	unsigned long encoder_types = 0;
+
+	dev_dbg(ipu_crtc->dev, "%s: mode->hdisplay: %d\n", __func__,
+			mode->hdisplay);
+	dev_dbg(ipu_crtc->dev, "%s: mode->vdisplay: %d\n", __func__,
+			mode->vdisplay);
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
+		if (encoder->crtc == crtc)
+			encoder_types |= BIT(encoder->encoder_type);
+
+	dev_dbg(ipu_crtc->dev, "%s: attached to encoder types 0x%lx\n",
+		__func__, encoder_types);
+
+	/*
+	 * If we have DAC or LDB, then we need the IPU DI clock to be
+	 * the same as the LDB DI clock. For TVDAC, derive the IPU DI
+	 * clock from 27 MHz TVE_DI clock, but allow to divide it.
+	 */
+	if (encoder_types & (BIT(DRM_MODE_ENCODER_DAC) |
+			     BIT(DRM_MODE_ENCODER_LVDS)))
+		sig_cfg.clkflags = IPU_DI_CLKMODE_SYNC | IPU_DI_CLKMODE_EXT;
+	else if (encoder_types & BIT(DRM_MODE_ENCODER_TVDAC))
+		sig_cfg.clkflags = IPU_DI_CLKMODE_EXT;
+	else
+		sig_cfg.clkflags = 0;
+
+	sig_cfg.enable_pol = 1;
+	sig_cfg.clk_pol = 0;
+	sig_cfg.bus_format = ipu_crtc->bus_format;
+	sig_cfg.v_to_h_sync = 0;
+	sig_cfg.hsync_pin = ipu_crtc->di_hsync_pin;
+	sig_cfg.vsync_pin = ipu_crtc->di_vsync_pin;
+
+	drm_display_mode_to_videomode(mode, &sig_cfg.mode);
+
+	ipu_dc_init_sync(ipu_crtc->dc, ipu_crtc->di,
+			 mode->flags & DRM_MODE_FLAG_INTERLACE,
+			 ipu_crtc->bus_format, mode->hdisplay);
+	ipu_di_init_sync_panel(ipu_crtc->di, &sig_cfg);
 }
 
 static const struct drm_crtc_helper_funcs ipu_helper_funcs = {
 	.dpms = ipu_crtc_dpms,
 	.mode_fixup = ipu_crtc_mode_fixup,
-	.mode_set = ipu_crtc_mode_set,
+	.mode_set = drm_helper_crtc_mode_set,
+	.mode_set_nofb = ipu_crtc_mode_set_nofb,
 	.prepare = ipu_crtc_prepare,
 	.commit = ipu_crtc_commit,
+	.atomic_check = ipu_crtc_atomic_check,
 };
 
 static int ipu_enable_vblank(struct drm_crtc *crtc)
 {
 	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
 
+	/*
+	 * ->commit is done after ->mode_set in drm_crtc_helper_set_mode(),
+	 * so waiting for vblank in drm_plane_helper_commit() will timeout.
+	 * Check the state here to avoid the waiting.
+	 */
+	if (!ipu_crtc->enabled)
+		return -EINVAL;
+
 	enable_irq(ipu_crtc->irq);
 
 	return 0;
@@ -506,6 +504,8 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 	/* Only enable IRQ when we actually need it to trigger work. */
 	disable_irq(ipu_crtc->irq);
 
+	ipu_plane_put_resources(ipu_crtc->plane[0]);
+
 	ipu_crtc->flip_queue = create_singlethread_workqueue("ipu-crtc-flip");
 
 	return 0;
@@ -550,7 +550,6 @@ static void ipu_drm_unbind(struct device *dev, struct device *master,
 	imx_drm_remove_crtc(ipu_crtc->imx_crtc);
 
 	destroy_workqueue(ipu_crtc->flip_queue);
-	ipu_plane_put_resources(ipu_crtc->plane[0]);
 	ipu_put_resources(ipu_crtc);
 }
 
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 30cedbb..4511b84 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -16,6 +16,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_plane_helper.h>
 
 #include "video/imx-ipu-v3.h"
 #include "ipuv3-plane.h"
@@ -51,12 +52,15 @@ int ipu_plane_irq(struct ipu_plane *ipu_plane)
 				     IPU_IRQ_EOF);
 }
 
-int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
-		       int x, int y)
+int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb)
 {
-	struct drm_gem_cma_object *cma_obj[3];
-	unsigned long eba, ubo, vbo;
+	struct drm_gem_cma_object *cma_obj[3], *old_cma_obj[3];
+	struct drm_plane_state *state = ipu_plane->base.state;
+	struct drm_framebuffer *old_fb = state->fb;
+	unsigned long eba, ubo, vbo, old_eba, old_ubo, old_vbo;
 	int active, i;
+	int x = state->src_x >> 16;
+	int y = state->src_y >> 16;
 
 	for (i = 0; i < drm_format_num_planes(fb->pixel_format); i++) {
 		cma_obj[i] = drm_fb_cma_get_gem_obj(fb, i);
@@ -66,6 +70,14 @@ int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
 		}
 	}
 
+	for (i = 0; i < drm_format_num_planes(old_fb->pixel_format); i++) {
+		old_cma_obj[i] = drm_fb_cma_get_gem_obj(old_fb, i);
+		if (!old_cma_obj[i]) {
+			DRM_DEBUG_KMS("plane %d entry is null.\n", i);
+			return -EFAULT;
+		}
+	}
+
 	eba = cma_obj[0]->paddr + fb->offsets[0] +
 	      fb->pitches[0] * y + (fb->bits_per_pixel >> 3) * x;
 
@@ -79,13 +91,11 @@ int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
 		return -EINVAL;
 	}
 
-	if (ipu_plane->enabled && fb->pitches[0] != ipu_plane->stride[0]) {
+	if (fb->pitches[0] != old_fb->pitches[0]) {
 		DRM_DEBUG_KMS("pitches must not change while plane is enabled.\n");
 		return -EINVAL;
 	}
 
-	ipu_plane->stride[0] = fb->pitches[0];
-
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_YUV420:
 	case DRM_FORMAT_YVU420:
@@ -102,6 +112,14 @@ int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
 		vbo = cma_obj[2]->paddr + fb->offsets[2] +
 		      fb->pitches[2] * y / 2 + x / 2 - eba;
 
+		old_eba = old_cma_obj[0]->paddr + old_fb->offsets[0] +
+		      old_fb->pitches[0] * y +
+		      (old_fb->bits_per_pixel >> 3) * x;
+		old_ubo = old_cma_obj[1]->paddr + old_fb->offsets[1] +
+		      old_fb->pitches[1] * y / 2 + x / 2 - old_eba;
+		old_vbo = old_cma_obj[2]->paddr + old_fb->offsets[2] +
+		      old_fb->pitches[2] * y / 2 + x / 2 - old_eba;
+
 		if ((ubo & 0x7) || (vbo & 0x7)) {
 			DRM_DEBUG_KMS("U/V buffer offsets must be a multiple of 8.\n");
 			return -EINVAL;
@@ -112,8 +130,7 @@ int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
 			return -EINVAL;
 		}
 
-		if (ipu_plane->enabled && ((ipu_plane->u_offset != ubo) ||
-					   (ipu_plane->v_offset != vbo))) {
+		if (old_ubo != ubo || old_vbo != vbo) {
 			DRM_DEBUG_KMS("U/V buffer offsets must not change while plane is enabled.\n");
 			return -EINVAL;
 		}
@@ -128,16 +145,11 @@ int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
 			return -EINVAL;
 		}
 
-		if (ipu_plane->enabled &&
-		    (ipu_plane->stride[1] != fb->pitches[1])) {
+		if (old_fb->pitches[1] != fb->pitches[1]) {
 			DRM_DEBUG_KMS("U/V pitches must not change while plane is enabled.\n");
 			return -EINVAL;
 		}
 
-		ipu_plane->u_offset = ubo;
-		ipu_plane->v_offset = vbo;
-		ipu_plane->stride[1] = fb->pitches[1];
-
 		dev_dbg(ipu_plane->base.dev->dev,
 			"phys = %pad %pad %pad, x = %d, y = %d",
 			&cma_obj[0]->paddr, &cma_obj[1]->paddr,
@@ -149,165 +161,111 @@ int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
 		break;
 	}
 
-	if (ipu_plane->enabled) {
-		active = ipu_idmac_get_current_buffer(ipu_plane->ipu_ch);
-		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, !active, eba);
-		ipu_idmac_select_buffer(ipu_plane->ipu_ch, !active);
-	} else {
-		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
-		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 1, eba);
-	}
-
-	/* cache offsets for subsequent pageflips */
-	ipu_plane->x = x;
-	ipu_plane->y = y;
+	active = ipu_idmac_get_current_buffer(ipu_plane->ipu_ch);
+	ipu_cpmem_set_buffer(ipu_plane->ipu_ch, !active, eba);
+	ipu_idmac_select_buffer(ipu_plane->ipu_ch, !active);
 
 	return 0;
 }
 
-int ipu_plane_mode_set(struct ipu_plane *ipu_plane, struct drm_crtc *crtc,
-		       struct drm_display_mode *mode,
-		       struct drm_framebuffer *fb, int crtc_x, int crtc_y,
-		       unsigned int crtc_w, unsigned int crtc_h,
-		       uint32_t src_x, uint32_t src_y,
-		       uint32_t src_w, uint32_t src_h, bool interlaced)
+static inline unsigned long
+drm_plane_state_to_eba(struct drm_plane_state *state)
 {
-	struct drm_plane plane = ipu_plane->base;
-	struct device *dev = plane.dev->dev;
-	int ret;
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_gem_cma_object *cma_obj;
 
-	/* no scaling */
-	if (src_w != crtc_w || src_h != crtc_h)
-		return -EINVAL;
+	cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
+	BUG_ON(!cma_obj);
 
-	if (plane.type == DRM_PLANE_TYPE_PRIMARY) {
-		/* full plane doesn't support partial off screen */
-		if (crtc_x || crtc_y || crtc_w != mode->hdisplay ||
-			crtc_h != mode->vdisplay)
-			return -EINVAL;
+	return cma_obj->paddr + fb->offsets[0] +
+	       fb->pitches[0] * (state->src_y >> 16) +
+	       (fb->bits_per_pixel >> 3) * (state->src_x >> 16);
+}
 
-		/* full plane minimum width is 13 pixels */
-		if (crtc_w < 13)
-			return -EINVAL;
-	} else if (plane.type == DRM_PLANE_TYPE_OVERLAY) {
-		/* clip to crtc bounds */
-		if (crtc_x < 0) {
-			if (-crtc_x > crtc_w)
-				return -EINVAL;
-			src_x += -crtc_x;
-			src_w -= -crtc_x;
-			crtc_w -= -crtc_x;
-			crtc_x = 0;
-		}
-		if (crtc_y < 0) {
-			if (-crtc_y > crtc_h)
-				return -EINVAL;
-			src_y += -crtc_y;
-			src_h -= -crtc_y;
-			crtc_h -= -crtc_y;
-			crtc_y = 0;
-		}
-		if (crtc_x + crtc_w > mode->hdisplay) {
-			if (crtc_x > mode->hdisplay)
-				return -EINVAL;
-			crtc_w = mode->hdisplay - crtc_x;
-			src_w = crtc_w;
-		}
-		if (crtc_y + crtc_h > mode->vdisplay) {
-			if (crtc_y > mode->vdisplay)
-				return -EINVAL;
-			crtc_h = mode->vdisplay - crtc_y;
-			src_h = crtc_h;
-		}
-	} else
-		return -EINVAL;
-	if (crtc_h < 2)
-		return -EINVAL;
+static inline unsigned long
+drm_plane_state_to_ubo(struct drm_plane_state *state)
+{
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_gem_cma_object *cma_obj;
+	unsigned long eba = drm_plane_state_to_eba(state);
 
-	/*
-	 * since we cannot touch active IDMAC channels, we do not support
-	 * resizing the enabled plane or changing its format
-	 */
-	if (ipu_plane->enabled) {
-		if (src_w != ipu_plane->w || src_h != ipu_plane->h ||
-		    fb->pixel_format != plane.fb->pixel_format)
-			return -EINVAL;
+	cma_obj = drm_fb_cma_get_gem_obj(fb, 1);
+	BUG_ON(!cma_obj);
 
-		return ipu_plane_set_base(ipu_plane, fb, src_x, src_y);
-	}
+	return cma_obj->paddr + fb->offsets[1] +
+	       fb->pitches[1] * (state->src_y >> 16) / 2 +
+	       (state->src_x >> 16) / 2 - eba;
+}
 
-	switch (ipu_plane->dp_flow) {
-	case IPU_DP_FLOW_SYNC_BG:
-		ret = ipu_dp_setup_channel(ipu_plane->dp,
-				IPUV3_COLORSPACE_RGB,
-				IPUV3_COLORSPACE_RGB);
-		if (ret) {
-			dev_err(dev,
-				"initializing display processor failed with %d\n",
-				ret);
-			return ret;
-		}
-		ipu_dp_set_global_alpha(ipu_plane->dp, true, 0, true);
-		break;
-	case IPU_DP_FLOW_SYNC_FG:
-		ipu_dp_setup_channel(ipu_plane->dp,
-				ipu_drm_fourcc_to_colorspace(fb->pixel_format),
-				IPUV3_COLORSPACE_UNKNOWN);
-		ipu_dp_set_window_pos(ipu_plane->dp, crtc_x, crtc_y);
-		/* Enable local alpha on partial plane */
-		switch (fb->pixel_format) {
-		case DRM_FORMAT_ARGB1555:
-		case DRM_FORMAT_ABGR1555:
-		case DRM_FORMAT_RGBA5551:
-		case DRM_FORMAT_BGRA5551:
-		case DRM_FORMAT_ARGB4444:
-		case DRM_FORMAT_ARGB8888:
-		case DRM_FORMAT_ABGR8888:
-		case DRM_FORMAT_RGBA8888:
-		case DRM_FORMAT_BGRA8888:
-			ipu_dp_set_global_alpha(ipu_plane->dp, false, 0, false);
-			break;
-		default:
+static inline unsigned long
+drm_plane_state_to_vbo(struct drm_plane_state *state)
+{
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_gem_cma_object *cma_obj;
+	unsigned long eba = drm_plane_state_to_eba(state);
+
+	cma_obj = drm_fb_cma_get_gem_obj(fb, 2);
+	BUG_ON(!cma_obj);
+
+	return cma_obj->paddr + fb->offsets[2] +
+	       fb->pitches[2] * (state->src_y >> 16) / 2 +
+	       (state->src_x >> 16) / 2 - eba;
+}
+
+void ipu_plane_atomic_set_base(struct ipu_plane *ipu_plane,
+			       struct drm_plane_state *old_state)
+{
+	struct drm_plane *plane = &ipu_plane->base;
+	struct drm_plane_state *state = plane->state;
+	struct drm_framebuffer *fb = state->fb;
+	unsigned long eba, ubo, vbo;
+	int active;
+
+	eba = drm_plane_state_to_eba(state);
+
+	switch (fb->pixel_format) {
+	case DRM_FORMAT_YUV420:
+	case DRM_FORMAT_YVU420:
+		if (old_state->fb)
 			break;
-		}
-	}
 
-	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, crtc_w);
+		/*
+		 * Multiplanar formats have to meet the following restrictions:
+		 * - The (up to) three plane addresses are EBA, EBA+UBO, EBA+VBO
+		 * - EBA, UBO and VBO are a multiple of 8
+		 * - UBO and VBO are unsigned and not larger than 0xfffff8
+		 * - Only EBA may be changed while scanout is active
+		 * - The strides of U and V planes must be identical.
+		 */
+		ubo = drm_plane_state_to_ubo(state);
+		vbo = drm_plane_state_to_vbo(state);
 
-	ipu_cpmem_zero(ipu_plane->ipu_ch);
-	ipu_cpmem_set_resolution(ipu_plane->ipu_ch, src_w, src_h);
-	ret = ipu_cpmem_set_fmt(ipu_plane->ipu_ch, fb->pixel_format);
-	if (ret < 0) {
-		dev_err(dev, "unsupported pixel format 0x%08x\n",
-			fb->pixel_format);
-		return ret;
-	}
-	ipu_cpmem_set_high_priority(ipu_plane->ipu_ch);
-	ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1);
-	ipu_cpmem_set_stride(ipu_plane->ipu_ch, fb->pitches[0]);
+		if (fb->pixel_format == DRM_FORMAT_YUV420)
+			ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
+						      fb->pitches[1], ubo, vbo);
+		else
+			ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
+						      fb->pitches[1], vbo, ubo);
 
-	ret = ipu_plane_set_base(ipu_plane, fb, src_x, src_y);
-	if (ret < 0)
-		return ret;
-	if (interlaced)
-		ipu_cpmem_interlaced_scan(ipu_plane->ipu_ch, fb->pitches[0]);
-
-	if (fb->pixel_format == DRM_FORMAT_YUV420) {
-		ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
-					      ipu_plane->stride[1],
-					      ipu_plane->u_offset,
-					      ipu_plane->v_offset);
-	} else if (fb->pixel_format == DRM_FORMAT_YVU420) {
-		ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
-					      ipu_plane->stride[1],
-					      ipu_plane->v_offset,
-					      ipu_plane->u_offset);
-	}
+		dev_dbg(ipu_plane->base.dev->dev,
+			"phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo,
+			state->src_x >> 16, state->src_y >> 16);
+		break;
+	default:
+		dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d",
+			eba, state->src_x >> 16, state->src_y >> 16);
 
-	ipu_plane->w = src_w;
-	ipu_plane->h = src_h;
+		break;
+	}
 
-	return 0;
+	if (old_state->fb) {
+		active = ipu_idmac_get_current_buffer(ipu_plane->ipu_ch);
+		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, !active, eba);
+		ipu_idmac_select_buffer(ipu_plane->ipu_ch, !active);
+	} else {
+		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
+		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 1, eba);
+	}
 }
 
 void ipu_plane_put_resources(struct ipu_plane *ipu_plane)
@@ -362,14 +320,10 @@ void ipu_plane_enable(struct ipu_plane *ipu_plane)
 	ipu_idmac_enable_channel(ipu_plane->ipu_ch);
 	if (ipu_plane->dp)
 		ipu_dp_enable_channel(ipu_plane->dp);
-
-	ipu_plane->enabled = true;
 }
 
 void ipu_plane_disable(struct ipu_plane *ipu_plane)
 {
-	ipu_plane->enabled = false;
-
 	ipu_idmac_wait_busy(ipu_plane->ipu_ch, 50);
 
 	if (ipu_plane->dp)
@@ -380,53 +334,13 @@ void ipu_plane_disable(struct ipu_plane *ipu_plane)
 		ipu_dp_disable(ipu_plane->ipu);
 }
 
-/*
- * drm_plane API
- */
-
-static int ipu_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
-			    struct drm_framebuffer *fb, int crtc_x, int crtc_y,
-			    unsigned int crtc_w, unsigned int crtc_h,
-			    uint32_t src_x, uint32_t src_y,
-			    uint32_t src_w, uint32_t src_h)
-{
-	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
-	int ret = 0;
-
-	DRM_DEBUG_KMS("plane - %p\n", plane);
-
-	if (!ipu_plane->enabled)
-		ret = ipu_plane_get_resources(ipu_plane);
-	if (ret < 0)
-		return ret;
-
-	ret = ipu_plane_mode_set(ipu_plane, crtc, &crtc->hwmode, fb,
-			crtc_x, crtc_y, crtc_w, crtc_h,
-			src_x >> 16, src_y >> 16, src_w >> 16, src_h >> 16,
-			false);
-	if (ret < 0) {
-		ipu_plane_put_resources(ipu_plane);
-		return ret;
-	}
-
-	if (crtc != plane->crtc)
-		dev_dbg(plane->dev->dev, "crtc change: %p -> %p\n",
-				plane->crtc, crtc);
-
-	if (!ipu_plane->enabled)
-		ipu_plane_enable(ipu_plane);
-
-	return 0;
-}
-
 static int ipu_disable_plane(struct drm_plane *plane)
 {
 	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
 
 	DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
 
-	if (ipu_plane->enabled)
-		ipu_plane_disable(ipu_plane);
+	ipu_plane_disable(ipu_plane);
 
 	ipu_plane_put_resources(ipu_plane);
 
@@ -445,11 +359,203 @@ static void ipu_plane_destroy(struct drm_plane *plane)
 }
 
 static const struct drm_plane_funcs ipu_plane_funcs = {
-	.update_plane	= ipu_update_plane,
-	.disable_plane	= ipu_disable_plane,
+	.update_plane	= drm_plane_helper_update,
+	.disable_plane	= drm_plane_helper_disable,
 	.destroy	= ipu_plane_destroy,
 };
 
+static int ipu_plane_atomic_check(struct drm_plane *plane,
+				  struct drm_plane_state *state)
+{
+	struct drm_plane_state *old_state = plane->state;
+	struct drm_crtc_state *crtc_state;
+	struct device *dev = plane->dev->dev;
+	struct drm_crtc *crtc;
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_framebuffer *old_fb = old_state->fb;
+	unsigned long eba, ubo, vbo, old_ubo, old_vbo;
+
+	/* Ok to disable */
+	if (!fb)
+		return old_fb ? 0 : -EINVAL;
+
+	/* CRTC should be enabled */
+	drm_for_each_crtc(crtc, plane->dev) {
+		if (drm_crtc_mask(crtc) == plane->possible_crtcs) {
+			if (!crtc->enabled)
+				return -EINVAL;
+			break;
+		}
+	}
+
+	/* no scaling */
+	if (state->src_w >> 16 != state->crtc_w ||
+	    state->src_h >> 16 != state->crtc_h)
+		return -EINVAL;
+
+	crtc_state = state->crtc->state;
+
+	switch (plane->type) {
+	case DRM_PLANE_TYPE_PRIMARY:
+		/* full plane doesn't support partial off screen */
+		if (state->crtc_x || state->crtc_y ||
+		    state->crtc_w != crtc_state->adjusted_mode.hdisplay ||
+		    state->crtc_h != crtc_state->adjusted_mode.vdisplay)
+			return -EINVAL;
+
+		/* full plane minimum width is 13 pixels */
+		if (state->crtc_w < 13)
+			return -EINVAL;
+		break;
+	case DRM_PLANE_TYPE_OVERLAY:
+		if (state->crtc_x < 0 || state->crtc_y < 0)
+			return -EINVAL;
+
+		if (state->crtc_x + state->crtc_w >
+		    crtc_state->adjusted_mode.hdisplay)
+			return -EINVAL;
+		if (state->crtc_y + state->crtc_h >
+		    crtc_state->adjusted_mode.vdisplay)
+			return -EINVAL;
+		break;
+	default:
+		dev_warn(dev, "Unsupported plane type\n");
+		return -EINVAL;
+	}
+
+	if (state->crtc_h < 2)
+		return -EINVAL;
+
+	/*
+	 * since we cannot touch active IDMAC channels, we do not support
+	 * resizing the enabled plane or changing its format
+	 */
+	if (old_fb && (state->src_w != old_state->src_w ||
+			      state->src_h != old_state->src_h ||
+			      fb->pixel_format != old_fb->pixel_format))
+		return -EINVAL;
+
+	eba = drm_plane_state_to_eba(state);
+
+	if (eba & 0x7)
+		return -EINVAL;
+
+	if (fb->pitches[0] < 1 || fb->pitches[0] > 16384)
+		return -EINVAL;
+
+	if (old_fb && fb->pitches[0] != old_fb->pitches[0])
+		return -EINVAL;
+
+	switch (fb->pixel_format) {
+	case DRM_FORMAT_YUV420:
+	case DRM_FORMAT_YVU420:
+		/*
+		 * Multiplanar formats have to meet the following restrictions:
+		 * - The (up to) three plane addresses are EBA, EBA+UBO, EBA+VBO
+		 * - EBA, UBO and VBO are a multiple of 8
+		 * - UBO and VBO are unsigned and not larger than 0xfffff8
+		 * - Only EBA may be changed while scanout is active
+		 * - The strides of U and V planes must be identical.
+		 */
+		ubo = drm_plane_state_to_ubo(state);
+		vbo = drm_plane_state_to_vbo(state);
+
+		if ((ubo & 0x7) || (vbo & 0x7))
+			return -EINVAL;
+
+		if ((ubo > 0xfffff8) || (vbo > 0xfffff8))
+			return -EINVAL;
+
+		if (old_fb) {
+			old_ubo = drm_plane_state_to_ubo(old_state);
+			old_vbo = drm_plane_state_to_vbo(old_state);
+			if (ubo != old_ubo || vbo != old_vbo)
+				return -EINVAL;
+		}
+
+		if (fb->pitches[1] != fb->pitches[2])
+			return -EINVAL;
+
+		if (fb->pitches[1] < 1 || fb->pitches[1] > 16384)
+			return -EINVAL;
+
+		if (old_fb && old_fb->pitches[1] != fb->pitches[1])
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void ipu_plane_atomic_disable(struct drm_plane *plane,
+				     struct drm_plane_state *old_state)
+{
+	ipu_disable_plane(plane);
+}
+
+static void ipu_plane_atomic_update(struct drm_plane *plane,
+				    struct drm_plane_state *old_state)
+{
+	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
+	struct drm_plane_state *state = plane->state;
+	enum ipu_color_space ics;
+
+	if (old_state->fb) {
+		ipu_plane_atomic_set_base(ipu_plane, old_state);
+		return;
+	}
+
+	ipu_plane_get_resources(ipu_plane);
+
+	switch (ipu_plane->dp_flow) {
+	case IPU_DP_FLOW_SYNC_BG:
+		ipu_dp_setup_channel(ipu_plane->dp,
+					IPUV3_COLORSPACE_RGB,
+					IPUV3_COLORSPACE_RGB);
+		ipu_dp_set_global_alpha(ipu_plane->dp, true, 0, true);
+		break;
+	case IPU_DP_FLOW_SYNC_FG:
+		ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format);
+		ipu_dp_setup_channel(ipu_plane->dp, ics,
+					IPUV3_COLORSPACE_UNKNOWN);
+		ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x,
+					state->crtc_y);
+		/* Enable local alpha on partial plane */
+		switch (state->fb->pixel_format) {
+		case DRM_FORMAT_ARGB1555:
+		case DRM_FORMAT_ABGR1555:
+		case DRM_FORMAT_RGBA5551:
+		case DRM_FORMAT_BGRA5551:
+		case DRM_FORMAT_ARGB4444:
+		case DRM_FORMAT_ARGB8888:
+		case DRM_FORMAT_ABGR8888:
+		case DRM_FORMAT_RGBA8888:
+		case DRM_FORMAT_BGRA8888:
+			ipu_dp_set_global_alpha(ipu_plane->dp, false, 0, false);
+			break;
+		default:
+			break;
+		}
+	}
+
+	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w);
+
+	ipu_cpmem_zero(ipu_plane->ipu_ch);
+	ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16,
+					state->src_h >> 16);
+	ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format);
+	ipu_cpmem_set_high_priority(ipu_plane->ipu_ch);
+	ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1);
+	ipu_cpmem_set_stride(ipu_plane->ipu_ch, state->fb->pitches[0]);
+	ipu_plane_atomic_set_base(ipu_plane, old_state);
+	ipu_plane_enable(ipu_plane);
+}
+
+static const struct drm_plane_helper_funcs ipu_plane_helper_funcs = {
+	.atomic_check = ipu_plane_atomic_check,
+	.atomic_disable = ipu_plane_atomic_disable,
+	.atomic_update = ipu_plane_atomic_update,
+};
+
 struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
 				 int dma, int dp, unsigned int possible_crtcs,
 				 enum drm_plane_type type)
@@ -480,5 +586,7 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
 		return ERR_PTR(ret);
 	}
 
+	drm_plane_helper_add(&ipu_plane->base, &ipu_plane_helper_funcs);
+
 	return ipu_plane;
 }
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.h b/drivers/gpu/drm/imx/ipuv3-plane.h
index 4448fd4..26b9c59 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.h
+++ b/drivers/gpu/drm/imx/ipuv3-plane.h
@@ -23,17 +23,6 @@ struct ipu_plane {
 
 	int			dma;
 	int			dp_flow;
-
-	int			x;
-	int			y;
-	int			w;
-	int			h;
-
-	unsigned int		u_offset;
-	unsigned int		v_offset;
-	unsigned int		stride[2];
-
-	bool			enabled;
 };
 
 struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
@@ -50,8 +39,7 @@ int ipu_plane_mode_set(struct ipu_plane *plane, struct drm_crtc *crtc,
 
 void ipu_plane_enable(struct ipu_plane *plane);
 void ipu_plane_disable(struct ipu_plane *plane);
-int ipu_plane_set_base(struct ipu_plane *plane, struct drm_framebuffer *fb,
-		       int x, int y);
+int ipu_plane_set_base(struct ipu_plane *plane, struct drm_framebuffer *fb);
 
 int ipu_plane_get_resources(struct ipu_plane *plane);
 void ipu_plane_put_resources(struct ipu_plane *plane);
diff --git a/drivers/gpu/ipu-v3/ipu-dc.c b/drivers/gpu/ipu-v3/ipu-dc.c
index 2f29780..cd72dad 100644
--- a/drivers/gpu/ipu-v3/ipu-dc.c
+++ b/drivers/gpu/ipu-v3/ipu-dc.c
@@ -178,10 +178,7 @@ int ipu_dc_init_sync(struct ipu_dc *dc, struct ipu_di *di, bool interlaced,
 	dc->di = ipu_di_get_num(di);
 
 	map = ipu_bus_format_to_map(bus_format);
-	if (map < 0) {
-		dev_dbg(priv->dev, "IPU_DISP: No MAP\n");
-		return map;
-	}
+	BUG_ON(map < 0);
 
 	/*
 	 * In interlaced mode we need more counters to create the asymmetric
diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
index 359268e..a8d87dd 100644
--- a/drivers/gpu/ipu-v3/ipu-di.c
+++ b/drivers/gpu/ipu-v3/ipu-di.c
@@ -572,9 +572,6 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 	dev_dbg(di->ipu->dev, "disp %d: panel size = %d x %d\n",
 		di->id, sig->mode.hactive, sig->mode.vactive);
 
-	if ((sig->mode.vsync_len == 0) || (sig->mode.hsync_len == 0))
-		return -EINVAL;
-
 	dev_dbg(di->ipu->dev, "Clocks: IPU %luHz DI %luHz Needed %luHz\n",
 		clk_get_rate(di->clk_ipu),
 		clk_get_rate(di->clk_di),
-- 
2.7.4

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

[PATCH 07/14] drm/imx: atomic phase 2 step 1: Wire up state ->reset, ->duplicate and ->destroy

[Liu Ying]

Wire up CRTCs', planes' and connectors' ->reset, ->duplicate and ->destroy state
hooks to use the default implementations from the atomic helper library.
The helpers track each DRM object state.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c       | 19 +++----------------
 drivers/gpu/drm/imx/imx-drm-core.c     |  2 ++
 drivers/gpu/drm/imx/imx-ldb.c          |  4 ++++
 drivers/gpu/drm/imx/imx-tve.c          |  4 ++++
 drivers/gpu/drm/imx/ipuv3-crtc.c       |  4 ++++
 drivers/gpu/drm/imx/ipuv3-plane.c      |  4 ++++
 drivers/gpu/drm/imx/parallel-display.c |  4 ++++
 7 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 9795b72..6541b3c 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1508,14 +1508,6 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
 	mutex_unlock(&hdmi->mutex);
 }
 
-static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
-	.dpms = drm_helper_connector_dpms,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.detect = dw_hdmi_connector_detect,
-	.destroy = dw_hdmi_connector_destroy,
-	.force = dw_hdmi_connector_force,
-};
-
 static const struct drm_connector_funcs dw_hdmi_atomic_connector_funcs = {
 	.dpms = drm_atomic_helper_connector_dpms,
 	.fill_modes = drm_helper_probe_single_connector_modes,
@@ -1650,14 +1642,9 @@ static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi)
 	drm_connector_helper_add(&hdmi->connector,
 				 &dw_hdmi_connector_helper_funcs);
 
-	if (drm_core_check_feature(drm, DRIVER_ATOMIC))
-		drm_connector_init(drm, &hdmi->connector,
-				   &dw_hdmi_atomic_connector_funcs,
-				   DRM_MODE_CONNECTOR_HDMIA);
-	else
-		drm_connector_init(drm, &hdmi->connector,
-				   &dw_hdmi_connector_funcs,
-				   DRM_MODE_CONNECTOR_HDMIA);
+	drm_connector_init(drm, &hdmi->connector,
+			   &dw_hdmi_atomic_connector_funcs,
+			   DRM_MODE_CONNECTOR_HDMIA);
 
 	drm_mode_connector_attach_encoder(&hdmi->connector, encoder);
 
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 05229b9..e15f887 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -283,6 +283,8 @@ static int imx_drm_driver_load(struct drm_device *drm, unsigned long flags)
 		}
 	}
 
+	drm_mode_config_reset(drm);
+
 	/*
 	 * All components are now initialised, so setup the fb helper.
 	 * The fb helper takes copies of key hardware information, so the
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index a58eee5..8f17b64 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -17,6 +17,7 @@
 #include <linux/clk.h>
 #include <linux/component.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_of.h>
@@ -357,6 +358,9 @@ static const struct drm_connector_funcs imx_ldb_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = imx_ldb_connector_detect,
 	.destroy = imx_drm_connector_destroy,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static const struct drm_connector_helper_funcs imx_ldb_connector_helper_funcs = {
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index ae7a9fb..9b45deb 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -23,6 +23,7 @@
 #include <linux/spinlock.h>
 #include <linux/videodev2.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <video/imx-ipu-v3.h>
@@ -358,6 +359,9 @@ static const struct drm_connector_funcs imx_tve_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = imx_tve_connector_detect,
 	.destroy = imx_drm_connector_destroy,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static const struct drm_connector_helper_funcs imx_tve_connector_helper_funcs = {
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 11fad87..bd3b99e 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -18,6 +18,7 @@
 #include <linux/device.h>
 #include <linux/platform_device.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <linux/fb.h>
 #include <linux/clk.h>
@@ -233,6 +234,9 @@ static const struct drm_crtc_funcs ipu_crtc_funcs = {
 	.set_config = drm_crtc_helper_set_config,
 	.destroy = drm_crtc_cleanup,
 	.page_flip = ipu_page_flip,
+	.reset = drm_atomic_helper_crtc_reset,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
 static void ipu_crtc_handle_pageflip(struct ipu_crtc *ipu_crtc)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 4511b84..347b581 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -14,6 +14,7 @@
  */
 
 #include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_plane_helper.h>
@@ -362,6 +363,9 @@ static const struct drm_plane_funcs ipu_plane_funcs = {
 	.update_plane	= drm_plane_helper_update,
 	.disable_plane	= drm_plane_helper_disable,
 	.destroy	= ipu_plane_destroy,
+	.reset		= drm_atomic_helper_plane_reset,
+	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
+	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
 };
 
 static int ipu_plane_atomic_check(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 363e2c7..252c0ea 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -16,6 +16,7 @@
 #include <linux/component.h>
 #include <linux/module.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_panel.h>
@@ -146,6 +147,9 @@ static const struct drm_connector_funcs imx_pd_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = imx_pd_connector_detect,
 	.destroy = imx_drm_connector_destroy,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static const struct drm_connector_helper_funcs imx_pd_connector_helper_funcs = {
-- 
2.7.4

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

[PATCH 08/14] drm/imx: atomic phase 2 step 2: Track plane_state->fb correctly in ->page_flip

[Liu Ying]

Use drm_atomic_set_fb_for_plane() in the legacy ->page_flip path to track
the pointer plane_state->fb correctly.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/ipuv3-crtc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index bd3b99e..6ef6c96 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -18,6 +18,7 @@
 #include <linux/device.h>
 #include <linux/platform_device.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <linux/fb.h>
@@ -218,6 +219,9 @@ static int ipu_page_flip(struct drm_crtc *crtc,
 		ipu_crtc->flip_state = IPU_FLIP_SUBMITTED;
 	}
 
+	if (crtc->primary->state)
+		drm_atomic_set_fb_for_plane(crtc->primary->state, fb);
+
 	return 0;
 
 free_flip_work:
-- 
2.7.4

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

[PATCH 09/14] drm/imx: atomic phase 3 step 1: Atomic updates for planes

[Liu Ying]

This patch switches the update/disable_plane callbacks to their atomic version.
Also, use the default atomic helpers to implement the atomic_check/commit
callbacks for mode configuration.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 3 +++
 drivers/gpu/drm/imx/ipuv3-plane.c  | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index e15f887..7e058ea 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_gem_cma_helper.h>
@@ -205,6 +206,8 @@ static void imx_drm_output_poll_changed(struct drm_device *drm)
 static const struct drm_mode_config_funcs imx_drm_mode_config_funcs = {
 	.fb_create = drm_fb_cma_create,
 	.output_poll_changed = imx_drm_output_poll_changed,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
 };
 
 /*
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 347b581..8419cc1 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -360,8 +360,8 @@ static void ipu_plane_destroy(struct drm_plane *plane)
 }
 
 static const struct drm_plane_funcs ipu_plane_funcs = {
-	.update_plane	= drm_plane_helper_update,
-	.disable_plane	= drm_plane_helper_disable,
+	.update_plane	= drm_atomic_helper_update_plane,
+	.disable_plane	= drm_atomic_helper_disable_plane,
 	.destroy	= ipu_plane_destroy,
 	.reset		= drm_atomic_helper_plane_reset,
 	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
-- 
2.7.4

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

[PATCH 10/14] drm/atomic-helper: Disable planes when suspending

[Liu Ying]

We should disable planes explicitly when suspending.
Especially, this is meaningful for those display controllers which
don't support active planes without relevant CRTCs being enabled.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4befe25..5331d95 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1967,7 +1967,7 @@ commit:
  *
  * Loops through all connectors, finding those that aren't turned off and then
  * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
- * that they are connected to.
+ * that they are connected to.  The relevant planes are deactivated as well.
  *
  * This is used for example in suspend/resume to disable all currently active
  * functions when suspending.
@@ -1997,6 +1997,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
 	drm_for_each_connector(conn, dev) {
 		struct drm_crtc *crtc = conn->state->crtc;
 		struct drm_crtc_state *crtc_state;
+		struct drm_plane *plane;
 
 		if (!crtc || conn->dpms != DRM_MODE_DPMS_ON)
 			continue;
@@ -2008,6 +2009,21 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
 		}
 
 		crtc_state->active = false;
+
+		drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
+			struct drm_plane_state *plane_state;
+
+			plane_state = drm_atomic_get_plane_state(state, plane);
+			if (IS_ERR(plane_state)) {
+				err = PTR_ERR(plane_state);
+				goto free;
+			}
+
+			err = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+			if (err != 0)
+				goto free;
+			drm_atomic_set_fb_for_plane(plane_state, NULL);
+		}
 	}
 
 	err = drm_atomic_commit(state);
-- 
2.7.4

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

[PATCH 11/14] drm/imx: atomic phase 3 step 2: Use atomic configuration

[Liu Ying]

Replacing drm_crtc_helper_set_config() by drm_atomic_helper_set_config()
and converting the suspend/resume operations to atomic makes us be able
to use atomic configurations.  All of these allows us to remove the
crtc_funcs->mode_set callback as it is no longer used.  Also, we may remove
all the encoders' ->prepare callbacks as they can be replaced by ->disable.
In consequence, the bus_format, di_vsync_pin and di_hsync_pin settings are
moved from ->prepare to structure imx_encoder.  Furthermore, switching to
the pure atomic version of set_config callback means that we may implement
CRTC/plane atomic checks by using the new CRTC/plane states instead of the
legacy ones and we may remove the private ipu_crtc->enabled state which was
left there for the transitional atomic helpers in phase 1.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/dw_hdmi-imx.c      |  18 +++---
 drivers/gpu/drm/imx/imx-drm-core.c     |  49 ++++-----------
 drivers/gpu/drm/imx/imx-drm.h          |  11 +++-
 drivers/gpu/drm/imx/imx-ldb.c          | 109 +++++++++++++++------------------
 drivers/gpu/drm/imx/imx-tve.c          |  56 +++++++----------
 drivers/gpu/drm/imx/ipuv3-crtc.c       |  63 +++++--------------
 drivers/gpu/drm/imx/ipuv3-plane.c      |  23 +++----
 drivers/gpu/drm/imx/parallel-display.c |  56 ++++++++---------
 8 files changed, 159 insertions(+), 226 deletions(-)

diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index a24631fd..5f64674 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -22,9 +22,11 @@
 
 #include "imx-drm.h"
 
+#define imx_enc_to_imx_hdmi(x) container_of(x, struct imx_hdmi, imx_encoder)
+
 struct imx_hdmi {
 	struct device *dev;
-	struct drm_encoder encoder;
+	struct imx_drm_encoder imx_encoder;
 	struct regmap *regmap;
 };
 
@@ -117,7 +119,8 @@ static void dw_hdmi_imx_encoder_mode_set(struct drm_encoder *encoder,
 
 static void dw_hdmi_imx_encoder_commit(struct drm_encoder *encoder)
 {
-	struct imx_hdmi *hdmi = container_of(encoder, struct imx_hdmi, encoder);
+	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
+	struct imx_hdmi *hdmi = imx_enc_to_imx_hdmi(imx_encoder);
 	int mux = drm_of_encoder_active_port_id(hdmi->dev->of_node, encoder);
 
 	regmap_update_bits(hdmi->regmap, IOMUXC_GPR3,
@@ -125,14 +128,8 @@ static void dw_hdmi_imx_encoder_commit(struct drm_encoder *encoder)
 			   mux << IMX6Q_GPR3_HDMI_MUX_CTL_SHIFT);
 }
 
-static void dw_hdmi_imx_encoder_prepare(struct drm_encoder *encoder)
-{
-	imx_drm_set_bus_format(encoder, MEDIA_BUS_FMT_RGB888_1X24);
-}
-
 static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs = {
 	.mode_set   = dw_hdmi_imx_encoder_mode_set,
-	.prepare    = dw_hdmi_imx_encoder_prepare,
 	.commit     = dw_hdmi_imx_encoder_commit,
 	.disable    = dw_hdmi_imx_encoder_disable,
 };
@@ -215,7 +212,10 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	match = of_match_node(dw_hdmi_imx_dt_ids, pdev->dev.of_node);
 	plat_data = match->data;
 	hdmi->dev = &pdev->dev;
-	encoder = &hdmi->encoder;
+	encoder = &hdmi->imx_encoder.encoder;
+	hdmi->imx_encoder.bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+	hdmi->imx_encoder.di_hsync_pin = 2;
+	hdmi->imx_encoder.di_vsync_pin = 3;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 7e058ea..2fa04a0 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -42,6 +42,7 @@ struct imx_drm_device {
 	struct imx_drm_crtc			*crtc[MAX_CRTC];
 	unsigned int				pipes;
 	struct drm_fbdev_cma			*fbhelper;
+	struct drm_atomic_state			*state;
 };
 
 struct imx_drm_crtc {
@@ -86,42 +87,6 @@ static int imx_drm_driver_unload(struct drm_device *drm)
 	return 0;
 }
 
-static struct imx_drm_crtc *imx_drm_find_crtc(struct drm_crtc *crtc)
-{
-	struct imx_drm_device *imxdrm = crtc->dev->dev_private;
-	unsigned i;
-
-	for (i = 0; i < MAX_CRTC; i++)
-		if (imxdrm->crtc[i] && imxdrm->crtc[i]->crtc == crtc)
-			return imxdrm->crtc[i];
-
-	return NULL;
-}
-
-int imx_drm_set_bus_format_pins(struct drm_encoder *encoder, u32 bus_format,
-		int hsync_pin, int vsync_pin)
-{
-	struct imx_drm_crtc_helper_funcs *helper;
-	struct imx_drm_crtc *imx_crtc;
-
-	imx_crtc = imx_drm_find_crtc(encoder->crtc);
-	if (!imx_crtc)
-		return -EINVAL;
-
-	helper = &imx_crtc->imx_drm_helper_funcs;
-	if (helper->set_interface_pix_fmt)
-		return helper->set_interface_pix_fmt(encoder->crtc,
-					bus_format, hsync_pin, vsync_pin);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(imx_drm_set_bus_format_pins);
-
-int imx_drm_set_bus_format(struct drm_encoder *encoder, u32 bus_format)
-{
-	return imx_drm_set_bus_format_pins(encoder, bus_format, 2, 3);
-}
-EXPORT_SYMBOL_GPL(imx_drm_set_bus_format);
-
 int imx_drm_crtc_vblank_get(struct imx_drm_crtc *imx_drm_crtc)
 {
 	return drm_crtc_vblank_get(imx_drm_crtc->crtc);
@@ -501,6 +466,7 @@ static int imx_drm_platform_remove(struct platform_device *pdev)
 static int imx_drm_suspend(struct device *dev)
 {
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
+	struct imx_drm_device *imxdrm;
 
 	/* The drm_dev is NULL before .load hook is called */
 	if (drm_dev == NULL)
@@ -508,17 +474,26 @@ static int imx_drm_suspend(struct device *dev)
 
 	drm_kms_helper_poll_disable(drm_dev);
 
+	imxdrm = drm_dev->dev_private;
+	imxdrm->state = drm_atomic_helper_suspend(drm_dev);
+	if (IS_ERR(imxdrm->state)) {
+		drm_kms_helper_poll_enable(drm_dev);
+		return PTR_ERR(imxdrm->state);
+	}
+
 	return 0;
 }
 
 static int imx_drm_resume(struct device *dev)
 {
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
+	struct imx_drm_device *imx_drm;
 
 	if (drm_dev == NULL)
 		return 0;
 
-	drm_helper_resume_force_mode(drm_dev);
+	imx_drm = drm_dev->dev_private;
+	drm_atomic_helper_resume(drm_dev, imx_drm->state);
 	drm_kms_helper_poll_enable(drm_dev);
 
 	return 0;
diff --git a/drivers/gpu/drm/imx/imx-drm.h b/drivers/gpu/drm/imx/imx-drm.h
index b0241b9..5ecc1dd 100644
--- a/drivers/gpu/drm/imx/imx-drm.h
+++ b/drivers/gpu/drm/imx/imx-drm.h
@@ -15,11 +15,18 @@ struct platform_device;
 
 unsigned int imx_drm_crtc_id(struct imx_drm_crtc *crtc);
 
+struct imx_drm_encoder {
+	struct drm_encoder			encoder;
+	int					bus_format;
+	int					di_hsync_pin;
+	int					di_vsync_pin;
+};
+
+#define enc_to_imx_enc(x) container_of(x, struct imx_drm_encoder, encoder)
+
 struct imx_drm_crtc_helper_funcs {
 	int (*enable_vblank)(struct drm_crtc *crtc);
 	void (*disable_vblank)(struct drm_crtc *crtc);
-	int (*set_interface_pix_fmt)(struct drm_crtc *crtc,
-			u32 bus_format, int hsync_pin, int vsync_pin);
 	const struct drm_crtc_helper_funcs *crtc_helper_funcs;
 	const struct drm_crtc_funcs *crtc_funcs;
 };
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 8f17b64..d7d1e31 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -50,14 +50,15 @@
 #define LDB_BGREF_RMODE_INT		(1 << 15)
 
 #define con_to_imx_ldb_ch(x) container_of(x, struct imx_ldb_channel, connector)
-#define enc_to_imx_ldb_ch(x) container_of(x, struct imx_ldb_channel, encoder)
+#define imx_enc_to_imx_ldb_ch(x)	\
+			container_of(x, struct imx_ldb_channel, imx_encoder)
 
 struct imx_ldb;
 
 struct imx_ldb_channel {
 	struct imx_ldb *ldb;
 	struct drm_connector connector;
-	struct drm_encoder encoder;
+	struct imx_drm_encoder imx_encoder;
 	struct drm_panel *panel;
 	struct device_node *child;
 	int chno;
@@ -65,7 +66,6 @@ struct imx_ldb_channel {
 	int edid_len;
 	struct drm_display_mode mode;
 	int mode_valid;
-	int bus_format;
 };
 
 struct bus_mux {
@@ -102,8 +102,8 @@ static int imx_ldb_connector_get_modes(struct drm_connector *connector)
 		struct drm_display_info *di = &connector->display_info;
 
 		num_modes = imx_ldb_ch->panel->funcs->get_modes(imx_ldb_ch->panel);
-		if (!imx_ldb_ch->bus_format && di->num_bus_formats)
-			imx_ldb_ch->bus_format = di->bus_formats[0];
+		if (!imx_ldb_ch->imx_encoder.bus_format && di->num_bus_formats)
+			imx_ldb_ch->imx_encoder.bus_format = di->bus_formats[0];
 		if (num_modes > 0)
 			return num_modes;
 	}
@@ -134,7 +134,7 @@ static struct drm_encoder *imx_ldb_connector_best_encoder(
 {
 	struct imx_ldb_channel *imx_ldb_ch = con_to_imx_ldb_ch(connector);
 
-	return &imx_ldb_ch->encoder;
+	return &imx_ldb_ch->imx_encoder.encoder;
 }
 
 static void imx_ldb_encoder_dpms(struct drm_encoder *encoder, int mode)
@@ -169,45 +169,10 @@ static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
 			chno);
 }
 
-static void imx_ldb_encoder_prepare(struct drm_encoder *encoder)
-{
-	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
-	struct imx_ldb *ldb = imx_ldb_ch->ldb;
-	int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
-	u32 bus_format;
-
-	switch (imx_ldb_ch->bus_format) {
-	default:
-		dev_warn(ldb->dev,
-			 "could not determine data mapping, default to 18-bit \"spwg\"\n");
-		/* fallthrough */
-	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
-		bus_format = MEDIA_BUS_FMT_RGB666_1X18;
-		break;
-	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
-		bus_format = MEDIA_BUS_FMT_RGB888_1X24;
-		if (imx_ldb_ch->chno == 0 || dual)
-			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24;
-		if (imx_ldb_ch->chno == 1 || dual)
-			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24;
-		break;
-	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
-		bus_format = MEDIA_BUS_FMT_RGB888_1X24;
-		if (imx_ldb_ch->chno == 0 || dual)
-			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24 |
-					 LDB_BIT_MAP_CH0_JEIDA;
-		if (imx_ldb_ch->chno == 1 || dual)
-			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24 |
-					 LDB_BIT_MAP_CH1_JEIDA;
-		break;
-	}
-
-	imx_drm_set_bus_format(encoder, bus_format);
-}
-
 static void imx_ldb_encoder_commit(struct drm_encoder *encoder)
 {
-	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
+	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
+	struct imx_ldb_channel *imx_ldb_ch = imx_enc_to_imx_ldb_ch(imx_encoder);
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
 	int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
 	int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
@@ -255,7 +220,8 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
 			 struct drm_display_mode *orig_mode,
 			 struct drm_display_mode *mode)
 {
-	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
+	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
+	struct imx_ldb_channel *imx_ldb_ch = imx_enc_to_imx_ldb_ch(imx_encoder);
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
 	int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
 	unsigned long serial_clk;
@@ -298,7 +264,8 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
 
 static void imx_ldb_encoder_disable(struct drm_encoder *encoder)
 {
-	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
+	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
+	struct imx_ldb_channel *imx_ldb_ch = imx_enc_to_imx_ldb_ch(imx_encoder);
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
 	int mux, ret;
 
@@ -374,7 +341,6 @@ static const struct drm_encoder_funcs imx_ldb_encoder_funcs = {
 
 static const struct drm_encoder_helper_funcs imx_ldb_encoder_helper_funcs = {
 	.dpms = imx_ldb_encoder_dpms,
-	.prepare = imx_ldb_encoder_prepare,
 	.commit = imx_ldb_encoder_commit,
 	.mode_set = imx_ldb_encoder_mode_set,
 	.disable = imx_ldb_encoder_disable,
@@ -401,7 +367,7 @@ static int imx_ldb_register(struct drm_device *drm,
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
 	int ret;
 
-	ret = imx_drm_encoder_parse_of(drm, &imx_ldb_ch->encoder,
+	ret = imx_drm_encoder_parse_of(drm, &imx_ldb_ch->imx_encoder.encoder,
 				       imx_ldb_ch->child);
 	if (ret)
 		return ret;
@@ -416,10 +382,10 @@ static int imx_ldb_register(struct drm_device *drm,
 			return ret;
 	}
 
-	drm_encoder_helper_add(&imx_ldb_ch->encoder,
+	drm_encoder_helper_add(&imx_ldb_ch->imx_encoder.encoder,
 			&imx_ldb_encoder_helper_funcs);
-	drm_encoder_init(drm, &imx_ldb_ch->encoder, &imx_ldb_encoder_funcs,
-			 DRM_MODE_ENCODER_LVDS, NULL);
+	drm_encoder_init(drm, &imx_ldb_ch->imx_encoder.encoder,
+			 &imx_ldb_encoder_funcs, DRM_MODE_ENCODER_LVDS, NULL);
 
 	drm_connector_helper_add(&imx_ldb_ch->connector,
 			&imx_ldb_connector_helper_funcs);
@@ -430,7 +396,7 @@ static int imx_ldb_register(struct drm_device *drm,
 		drm_panel_attach(imx_ldb_ch->panel, &imx_ldb_ch->connector);
 
 	drm_mode_connector_attach_encoder(&imx_ldb_ch->connector,
-			&imx_ldb_ch->encoder);
+			&imx_ldb_ch->imx_encoder.encoder);
 
 	return 0;
 }
@@ -558,6 +524,7 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 	for_each_child_of_node(np, child) {
 		struct imx_ldb_channel *channel;
 		struct device_node *port;
+		int bus_format;
 
 		ret = of_property_read_u32(child, "reg", &i);
 		if (ret || i < 0 || i > 1)
@@ -609,21 +576,46 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 				channel->mode_valid = 1;
 		}
 
-		channel->bus_format = of_get_bus_format(dev, child);
-		if (channel->bus_format == -EINVAL) {
+		bus_format = of_get_bus_format(dev, child);
+		if (bus_format == -EINVAL) {
 			/*
 			 * If no bus format was specified in the device tree,
 			 * we can still get it from the connected panel later.
 			 */
 			if (channel->panel && channel->panel->funcs &&
 			    channel->panel->funcs->get_modes)
-				channel->bus_format = 0;
+				bus_format = 0;
 		}
-		if (channel->bus_format < 0) {
+		if (bus_format < 0) {
 			dev_err(dev, "could not determine data mapping: %d\n",
-				channel->bus_format);
-			return channel->bus_format;
+				bus_format);
+			return bus_format;
 		}
+		switch (bus_format) {
+		case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
+			bus_format = MEDIA_BUS_FMT_RGB666_1X18;
+			break;
+		case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
+			bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+			if (i == 0 || dual)
+				imx_ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24;
+			if (i == 1 || dual)
+				imx_ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24;
+			break;
+		case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
+			bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+			if (i == 0 || dual)
+				imx_ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24 |
+						     LDB_BIT_MAP_CH0_JEIDA;
+			if (i == 1 || dual)
+				imx_ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24 |
+						     LDB_BIT_MAP_CH1_JEIDA;
+			break;
+		}
+		channel->imx_encoder.bus_format = bus_format;
+
+		channel->imx_encoder.di_hsync_pin = 2;
+		channel->imx_encoder.di_vsync_pin = 3;
 
 		ret = imx_ldb_register(drm, channel);
 		if (ret)
@@ -648,7 +640,8 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
 			continue;
 
 		channel->connector.funcs->destroy(&channel->connector);
-		channel->encoder.funcs->destroy(&channel->encoder);
+		channel->imx_encoder.encoder.funcs->destroy(
+					&channel->imx_encoder.encoder);
 
 		kfree(channel->edid);
 	}
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index 9b45deb..82a1edd 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -99,7 +99,7 @@
 #define TVE_TVDAC_TEST_MODE_MASK	(0x7 << 0)
 
 #define con_to_tve(x) container_of(x, struct imx_tve, connector)
-#define enc_to_tve(x) container_of(x, struct imx_tve, encoder)
+#define imx_enc_to_tve(x) container_of(x, struct imx_tve, imx_encoder)
 
 enum {
 	TVE_MODE_TVOUT,
@@ -108,7 +108,7 @@ enum {
 
 struct imx_tve {
 	struct drm_connector connector;
-	struct drm_encoder encoder;
+	struct imx_drm_encoder imx_encoder;
 	struct device *dev;
 	spinlock_t lock;	/* register lock */
 	bool enabled;
@@ -121,8 +121,6 @@ struct imx_tve {
 	struct clk *di_sel_clk;
 	struct clk_hw clk_hw_di;
 	struct clk *di_clk;
-	int vsync_pin;
-	int hsync_pin;
 };
 
 static void tve_lock(void *__tve)
@@ -273,12 +271,13 @@ static struct drm_encoder *imx_tve_connector_best_encoder(
 {
 	struct imx_tve *tve = con_to_tve(connector);
 
-	return &tve->encoder;
+	return &tve->imx_encoder.encoder;
 }
 
 static void imx_tve_encoder_dpms(struct drm_encoder *encoder, int mode)
 {
-	struct imx_tve *tve = enc_to_tve(encoder);
+	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
+	struct imx_tve *tve = imx_enc_to_tve(imx_encoder);
 	int ret;
 
 	ret = regmap_update_bits(tve->regmap, TVE_COM_CONF_REG,
@@ -287,28 +286,12 @@ static void imx_tve_encoder_dpms(struct drm_encoder *encoder, int mode)
 		dev_err(tve->dev, "failed to disable TVOUT: %d\n", ret);
 }
 
-static void imx_tve_encoder_prepare(struct drm_encoder *encoder)
-{
-	struct imx_tve *tve = enc_to_tve(encoder);
-
-	tve_disable(tve);
-
-	switch (tve->mode) {
-	case TVE_MODE_VGA:
-		imx_drm_set_bus_format_pins(encoder, MEDIA_BUS_FMT_GBR888_1X24,
-					    tve->hsync_pin, tve->vsync_pin);
-		break;
-	case TVE_MODE_TVOUT:
-		imx_drm_set_bus_format(encoder, MEDIA_BUS_FMT_YUV8_1X24);
-		break;
-	}
-}
-
 static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
 				     struct drm_display_mode *orig_mode,
 				     struct drm_display_mode *mode)
 {
-	struct imx_tve *tve = enc_to_tve(encoder);
+	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
+	struct imx_tve *tve = imx_enc_to_tve(imx_encoder);
 	unsigned long rounded_rate;
 	unsigned long rate;
 	int div = 1;
@@ -342,14 +325,16 @@ static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
 
 static void imx_tve_encoder_commit(struct drm_encoder *encoder)
 {
-	struct imx_tve *tve = enc_to_tve(encoder);
+	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
+	struct imx_tve *tve = imx_enc_to_tve(imx_encoder);
 
 	tve_enable(tve);
 }
 
 static void imx_tve_encoder_disable(struct drm_encoder *encoder)
 {
-	struct imx_tve *tve = enc_to_tve(encoder);
+	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
+	struct imx_tve *tve = imx_enc_to_tve(imx_encoder);
 
 	tve_disable(tve);
 }
@@ -376,7 +361,6 @@ static const struct drm_encoder_funcs imx_tve_encoder_funcs = {
 
 static const struct drm_encoder_helper_funcs imx_tve_encoder_helper_funcs = {
 	.dpms = imx_tve_encoder_dpms,
-	.prepare = imx_tve_encoder_prepare,
 	.mode_set = imx_tve_encoder_mode_set,
 	.commit = imx_tve_encoder_commit,
 	.disable = imx_tve_encoder_disable,
@@ -497,13 +481,14 @@ static int imx_tve_register(struct drm_device *drm, struct imx_tve *tve)
 	encoder_type = tve->mode == TVE_MODE_VGA ?
 				DRM_MODE_ENCODER_DAC : DRM_MODE_ENCODER_TVDAC;
 
-	ret = imx_drm_encoder_parse_of(drm, &tve->encoder,
+	ret = imx_drm_encoder_parse_of(drm, &tve->imx_encoder.encoder,
 				       tve->dev->of_node);
 	if (ret)
 		return ret;
 
-	drm_encoder_helper_add(&tve->encoder, &imx_tve_encoder_helper_funcs);
-	drm_encoder_init(drm, &tve->encoder, &imx_tve_encoder_funcs,
+	drm_encoder_helper_add(&tve->imx_encoder.encoder,
+			       &imx_tve_encoder_helper_funcs);
+	drm_encoder_init(drm, &tve->imx_encoder.encoder, &imx_tve_encoder_funcs,
 			 encoder_type, NULL);
 
 	drm_connector_helper_add(&tve->connector,
@@ -511,7 +496,8 @@ static int imx_tve_register(struct drm_device *drm, struct imx_tve *tve)
 	drm_connector_init(drm, &tve->connector, &imx_tve_connector_funcs,
 			   DRM_MODE_CONNECTOR_VGA);
 
-	drm_mode_connector_attach_encoder(&tve->connector, &tve->encoder);
+	drm_mode_connector_attach_encoder(&tve->connector,
+					  &tve->imx_encoder.encoder);
 
 	return 0;
 }
@@ -589,7 +575,7 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 
 	if (tve->mode == TVE_MODE_VGA) {
 		ret = of_property_read_u32(np, "fsl,hsync-pin",
-					   &tve->hsync_pin);
+					   &tve->imx_encoder.di_hsync_pin);
 
 		if (ret < 0) {
 			dev_err(dev, "failed to get vsync pin\n");
@@ -597,12 +583,14 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 		}
 
 		ret |= of_property_read_u32(np, "fsl,vsync-pin",
-					    &tve->vsync_pin);
+					    &tve->imx_encoder.di_vsync_pin);
 
 		if (ret < 0) {
 			dev_err(dev, "failed to get vsync pin\n");
 			return ret;
 		}
+
+		tve->imx_encoder.bus_format = MEDIA_BUS_FMT_GBR888_1X24;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -691,7 +679,7 @@ static void imx_tve_unbind(struct device *dev, struct device *master,
 	struct imx_tve *tve = dev_get_drvdata(dev);
 
 	tve->connector.funcs->destroy(&tve->connector);
-	tve->encoder.funcs->destroy(&tve->encoder);
+	tve->imx_encoder.encoder.funcs->destroy(&tve->imx_encoder.encoder);
 
 	if (!IS_ERR(tve->dac_reg))
 		regulator_disable(tve->dac_reg);
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 6ef6c96..3a3b67c 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -62,14 +62,10 @@ struct ipu_crtc {
 
 	struct ipu_dc		*dc;
 	struct ipu_di		*di;
-	int			enabled;
 	enum ipu_flip_status	flip_state;
 	struct workqueue_struct *flip_queue;
 	struct ipu_flip_work	*flip_work;
 	int			irq;
-	u32			bus_format;
-	int			di_hsync_pin;
-	int			di_vsync_pin;
 };
 
 #define to_ipu_crtc(x) container_of(x, struct ipu_crtc, base)
@@ -78,19 +74,10 @@ static void ipu_crtc_enable(struct ipu_crtc *ipu_crtc)
 {
 	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
 
-	if (ipu_crtc->enabled)
-		return;
-
 	ipu_dc_enable(ipu);
 	/* Start DC channel and DI after IDMAC */
 	ipu_dc_enable_channel(ipu_crtc->dc);
 	ipu_di_enable(ipu_crtc->di);
-	ipu_crtc->enabled = 1;
-
-	/*
-	 * In order not to be warned on enabling vblank failure,
-	 * we should call drm_crtc_vblank_on() after ->enabled is set to 1.
-	 */
 	drm_crtc_vblank_on(&ipu_crtc->base);
 }
 
@@ -98,15 +85,10 @@ static void ipu_crtc_disable(struct ipu_crtc *ipu_crtc)
 {
 	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
 
-	if (!ipu_crtc->enabled)
-		return;
-
 	/* Stop DC channel and DI before IDMAC */
 	ipu_dc_disable_channel(ipu_crtc->dc);
 	ipu_di_disable(ipu_crtc->di);
 	ipu_dc_disable(ipu);
-	ipu_crtc->enabled = 0;
-
 	drm_crtc_vblank_off(&ipu_crtc->base);
 }
 
@@ -235,7 +217,7 @@ put_vblank:
 }
 
 static const struct drm_crtc_funcs ipu_crtc_funcs = {
-	.set_config = drm_crtc_helper_set_config,
+	.set_config = drm_atomic_helper_set_config,
 	.destroy = drm_crtc_cleanup,
 	.page_flip = ipu_page_flip,
 	.reset = drm_atomic_helper_crtc_reset,
@@ -315,6 +297,11 @@ static void ipu_crtc_commit(struct drm_crtc *crtc)
 static int ipu_crtc_atomic_check(struct drm_crtc *crtc,
 				 struct drm_crtc_state *state)
 {
+	u32 primary_plane_mask = 1 << drm_plane_index(crtc->primary);
+
+	if (state->active && (primary_plane_mask & state->plane_mask) == 0)
+		return -EINVAL;
+
 	return 0;
 }
 
@@ -322,6 +309,7 @@ static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_encoder *encoder;
+	struct imx_drm_encoder *imx_encoder = NULL;
 	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
 	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
 	struct ipu_di_signal_cfg sig_cfg = {};
@@ -332,9 +320,12 @@ static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	dev_dbg(ipu_crtc->dev, "%s: mode->vdisplay: %d\n", __func__,
 			mode->vdisplay);
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
-		if (encoder->crtc == crtc)
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
+		if (encoder->crtc == crtc) {
 			encoder_types |= BIT(encoder->encoder_type);
+			imx_encoder = enc_to_imx_enc(encoder);
+		}
+	}
 
 	dev_dbg(ipu_crtc->dev, "%s: attached to encoder types 0x%lx\n",
 		__func__, encoder_types);
@@ -354,23 +345,22 @@ static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 
 	sig_cfg.enable_pol = 1;
 	sig_cfg.clk_pol = 0;
-	sig_cfg.bus_format = ipu_crtc->bus_format;
+	sig_cfg.bus_format = imx_encoder->bus_format;
 	sig_cfg.v_to_h_sync = 0;
-	sig_cfg.hsync_pin = ipu_crtc->di_hsync_pin;
-	sig_cfg.vsync_pin = ipu_crtc->di_vsync_pin;
+	sig_cfg.hsync_pin = imx_encoder->di_hsync_pin;
+	sig_cfg.vsync_pin = imx_encoder->di_vsync_pin;
 
 	drm_display_mode_to_videomode(mode, &sig_cfg.mode);
 
 	ipu_dc_init_sync(ipu_crtc->dc, ipu_crtc->di,
 			 mode->flags & DRM_MODE_FLAG_INTERLACE,
-			 ipu_crtc->bus_format, mode->hdisplay);
+			 imx_encoder->bus_format, mode->hdisplay);
 	ipu_di_init_sync_panel(ipu_crtc->di, &sig_cfg);
 }
 
 static const struct drm_crtc_helper_funcs ipu_helper_funcs = {
 	.dpms = ipu_crtc_dpms,
 	.mode_fixup = ipu_crtc_mode_fixup,
-	.mode_set = drm_helper_crtc_mode_set,
 	.mode_set_nofb = ipu_crtc_mode_set_nofb,
 	.prepare = ipu_crtc_prepare,
 	.commit = ipu_crtc_commit,
@@ -381,14 +371,6 @@ static int ipu_enable_vblank(struct drm_crtc *crtc)
 {
 	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
 
-	/*
-	 * ->commit is done after ->mode_set in drm_crtc_helper_set_mode(),
-	 * so waiting for vblank in drm_plane_helper_commit() will timeout.
-	 * Check the state here to avoid the waiting.
-	 */
-	if (!ipu_crtc->enabled)
-		return -EINVAL;
-
 	enable_irq(ipu_crtc->irq);
 
 	return 0;
@@ -401,22 +383,9 @@ static void ipu_disable_vblank(struct drm_crtc *crtc)
 	disable_irq_nosync(ipu_crtc->irq);
 }
 
-static int ipu_set_interface_pix_fmt(struct drm_crtc *crtc,
-		u32 bus_format, int hsync_pin, int vsync_pin)
-{
-	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
-
-	ipu_crtc->bus_format = bus_format;
-	ipu_crtc->di_hsync_pin = hsync_pin;
-	ipu_crtc->di_vsync_pin = vsync_pin;
-
-	return 0;
-}
-
 static const struct imx_drm_crtc_helper_funcs ipu_crtc_helper_funcs = {
 	.enable_vblank = ipu_enable_vblank,
 	.disable_vblank = ipu_disable_vblank,
-	.set_interface_pix_fmt = ipu_set_interface_pix_fmt,
 	.crtc_funcs = &ipu_crtc_funcs,
 	.crtc_helper_funcs = &ipu_helper_funcs,
 };
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 8419cc1..b2a413f 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -14,6 +14,7 @@
  */
 
 #include <drm/drmP.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
@@ -374,31 +375,31 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
 	struct drm_plane_state *old_state = plane->state;
 	struct drm_crtc_state *crtc_state;
 	struct device *dev = plane->dev->dev;
-	struct drm_crtc *crtc;
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_framebuffer *old_fb = old_state->fb;
 	unsigned long eba, ubo, vbo, old_ubo, old_vbo;
 
 	/* Ok to disable */
 	if (!fb)
-		return old_fb ? 0 : -EINVAL;
+		return 0;
+
+	if (!state->crtc)
+		return -EINVAL;
+
+	crtc_state =
+		drm_atomic_get_existing_crtc_state(state->state, state->crtc);
+	if (WARN_ON(!crtc_state))
+		return -EINVAL;
 
 	/* CRTC should be enabled */
-	drm_for_each_crtc(crtc, plane->dev) {
-		if (drm_crtc_mask(crtc) == plane->possible_crtcs) {
-			if (!crtc->enabled)
-				return -EINVAL;
-			break;
-		}
-	}
+	if (!crtc_state->enable)
+		return -EINVAL;
 
 	/* no scaling */
 	if (state->src_w >> 16 != state->crtc_w ||
 	    state->src_h >> 16 != state->crtc_h)
 		return -EINVAL;
 
-	crtc_state = state->crtc->state;
-
 	switch (plane->type) {
 	case DRM_PLANE_TYPE_PRIMARY:
 		/* full plane doesn't support partial off screen */
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 252c0ea..2367f0a 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -27,15 +27,15 @@
 #include "imx-drm.h"
 
 #define con_to_imxpd(x) container_of(x, struct imx_parallel_display, connector)
-#define enc_to_imxpd(x) container_of(x, struct imx_parallel_display, encoder)
+#define imx_enc_to_imxpd(x)	\
+		container_of(x, struct imx_parallel_display, imx_encoder)
 
 struct imx_parallel_display {
 	struct drm_connector connector;
-	struct drm_encoder encoder;
+	struct imx_drm_encoder imx_encoder;
 	struct device *dev;
 	void *edid;
 	int edid_len;
-	u32 bus_format;
 	int mode_valid;
 	struct drm_display_mode mode;
 	struct drm_panel *panel;
@@ -58,8 +58,8 @@ static int imx_pd_connector_get_modes(struct drm_connector *connector)
 		struct drm_display_info *di = &connector->display_info;
 
 		num_modes = imxpd->panel->funcs->get_modes(imxpd->panel);
-		if (!imxpd->bus_format && di->num_bus_formats)
-			imxpd->bus_format = di->bus_formats[0];
+		if (!imxpd->imx_encoder.bus_format && di->num_bus_formats)
+			imxpd->imx_encoder.bus_format = di->bus_formats[0];
 		if (num_modes > 0)
 			return num_modes;
 	}
@@ -100,12 +100,13 @@ static struct drm_encoder *imx_pd_connector_best_encoder(
 {
 	struct imx_parallel_display *imxpd = con_to_imxpd(connector);
 
-	return &imxpd->encoder;
+	return &imxpd->imx_encoder.encoder;
 }
 
 static void imx_pd_encoder_dpms(struct drm_encoder *encoder, int mode)
 {
-	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
+	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
+	struct imx_parallel_display *imxpd = imx_enc_to_imxpd(imx_encoder);
 
 	if (mode != DRM_MODE_DPMS_ON)
 		drm_panel_disable(imxpd->panel);
@@ -113,16 +114,10 @@ static void imx_pd_encoder_dpms(struct drm_encoder *encoder, int mode)
 		drm_panel_enable(imxpd->panel);
 }
 
-static void imx_pd_encoder_prepare(struct drm_encoder *encoder)
-{
-	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
-
-	imx_drm_set_bus_format(encoder, imxpd->bus_format);
-}
-
 static void imx_pd_encoder_commit(struct drm_encoder *encoder)
 {
-	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
+	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
+	struct imx_parallel_display *imxpd = imx_enc_to_imxpd(imx_encoder);
 
 	drm_panel_prepare(imxpd->panel);
 	drm_panel_enable(imxpd->panel);
@@ -136,7 +131,8 @@ static void imx_pd_encoder_mode_set(struct drm_encoder *encoder,
 
 static void imx_pd_encoder_disable(struct drm_encoder *encoder)
 {
-	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
+	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
+	struct imx_parallel_display *imxpd = imx_enc_to_imxpd(imx_encoder);
 
 	drm_panel_disable(imxpd->panel);
 	drm_panel_unprepare(imxpd->panel);
@@ -163,7 +159,6 @@ static const struct drm_encoder_funcs imx_pd_encoder_funcs = {
 
 static const struct drm_encoder_helper_funcs imx_pd_encoder_helper_funcs = {
 	.dpms = imx_pd_encoder_dpms,
-	.prepare = imx_pd_encoder_prepare,
 	.commit = imx_pd_encoder_commit,
 	.mode_set = imx_pd_encoder_mode_set,
 	.disable = imx_pd_encoder_disable,
@@ -174,7 +169,7 @@ static int imx_pd_register(struct drm_device *drm,
 {
 	int ret;
 
-	ret = imx_drm_encoder_parse_of(drm, &imxpd->encoder,
+	ret = imx_drm_encoder_parse_of(drm, &imxpd->imx_encoder.encoder,
 				       imxpd->dev->of_node);
 	if (ret)
 		return ret;
@@ -186,9 +181,10 @@ static int imx_pd_register(struct drm_device *drm,
 	 */
 	imxpd->connector.dpms = DRM_MODE_DPMS_OFF;
 
-	drm_encoder_helper_add(&imxpd->encoder, &imx_pd_encoder_helper_funcs);
-	drm_encoder_init(drm, &imxpd->encoder, &imx_pd_encoder_funcs,
-			 DRM_MODE_ENCODER_NONE, NULL);
+	drm_encoder_helper_add(&imxpd->imx_encoder.encoder,
+			       &imx_pd_encoder_helper_funcs);
+	drm_encoder_init(drm, &imxpd->imx_encoder.encoder,
+			 &imx_pd_encoder_funcs, DRM_MODE_ENCODER_NONE, NULL);
 
 	drm_connector_helper_add(&imxpd->connector,
 			&imx_pd_connector_helper_funcs);
@@ -198,7 +194,8 @@ static int imx_pd_register(struct drm_device *drm,
 	if (imxpd->panel)
 		drm_panel_attach(imxpd->panel, &imxpd->connector);
 
-	drm_mode_connector_attach_encoder(&imxpd->connector, &imxpd->encoder);
+	drm_mode_connector_attach_encoder(&imxpd->connector,
+					  &imxpd->imx_encoder.encoder);
 
 	return 0;
 }
@@ -210,7 +207,7 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	struct device_node *port;
 	const u8 *edidp;
 	struct imx_parallel_display *imxpd;
-	int ret;
+	int ret, bus_format = 0;
 	const char *fmt;
 
 	imxpd = devm_kzalloc(dev, sizeof(*imxpd), GFP_KERNEL);
@@ -224,14 +221,17 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	ret = of_property_read_string(np, "interface-pix-fmt", &fmt);
 	if (!ret) {
 		if (!strcmp(fmt, "rgb24"))
-			imxpd->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+			bus_format = MEDIA_BUS_FMT_RGB888_1X24;
 		else if (!strcmp(fmt, "rgb565"))
-			imxpd->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
+			bus_format = MEDIA_BUS_FMT_RGB565_1X16;
 		else if (!strcmp(fmt, "bgr666"))
-			imxpd->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
+			bus_format = MEDIA_BUS_FMT_RGB666_1X18;
 		else if (!strcmp(fmt, "lvds666"))
-			imxpd->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
+			bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
 	}
+	imxpd->imx_encoder.bus_format = bus_format;
+	imxpd->imx_encoder.di_hsync_pin = 2;
+	imxpd->imx_encoder.di_vsync_pin = 3;
 
 	/* port@1 is the output port */
 	port = of_graph_get_port_by_id(np, 1);
@@ -264,7 +264,7 @@ static void imx_pd_unbind(struct device *dev, struct device *master,
 {
 	struct imx_parallel_display *imxpd = dev_get_drvdata(dev);
 
-	imxpd->encoder.funcs->destroy(&imxpd->encoder);
+	imxpd->imx_encoder.encoder.funcs->destroy(&imxpd->imx_encoder.encoder);
 	imxpd->connector.funcs->destroy(&imxpd->connector);
 
 	kfree(imxpd->edid);
-- 
2.7.4

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

[PATCH 12/14] drm/imx: atomic phase 3 step 3: Legacy callback fixups

[Liu Ying]

Now that we can use atomic configurations, all the legacy callbacks
of CRTCs, encoders and connectors can be switched to the atomic version.
For the imx-ldb driver, there is a clock parent setting mismatch bewteen
->enable and ->disable after the switch, so a fixup is added.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/dw_hdmi-imx.c      |  4 ++--
 drivers/gpu/drm/imx/imx-ldb.c          | 16 ++++++-------
 drivers/gpu/drm/imx/imx-tve.c          | 19 +++------------
 drivers/gpu/drm/imx/ipuv3-crtc.c       | 43 +++++-----------------------------
 drivers/gpu/drm/imx/parallel-display.c | 18 +++-----------
 5 files changed, 22 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index 5f64674..5f1d437 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -117,7 +117,7 @@ static void dw_hdmi_imx_encoder_mode_set(struct drm_encoder *encoder,
 {
 }
 
-static void dw_hdmi_imx_encoder_commit(struct drm_encoder *encoder)
+static void dw_hdmi_imx_encoder_enable(struct drm_encoder *encoder)
 {
 	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
 	struct imx_hdmi *hdmi = imx_enc_to_imx_hdmi(imx_encoder);
@@ -130,7 +130,7 @@ static void dw_hdmi_imx_encoder_commit(struct drm_encoder *encoder)
 
 static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs = {
 	.mode_set   = dw_hdmi_imx_encoder_mode_set,
-	.commit     = dw_hdmi_imx_encoder_commit,
+	.enable     = dw_hdmi_imx_encoder_enable,
 	.disable    = dw_hdmi_imx_encoder_disable,
 };
 
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index d7d1e31..4194150 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -137,10 +137,6 @@ static struct drm_encoder *imx_ldb_connector_best_encoder(
 	return &imx_ldb_ch->imx_encoder.encoder;
 }
 
-static void imx_ldb_encoder_dpms(struct drm_encoder *encoder, int mode)
-{
-}
-
 static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
 		unsigned long serial_clk, unsigned long di_clk)
 {
@@ -169,7 +165,7 @@ static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
 			chno);
 }
 
-static void imx_ldb_encoder_commit(struct drm_encoder *encoder)
+static void imx_ldb_encoder_enable(struct drm_encoder *encoder)
 {
 	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
 	struct imx_ldb_channel *imx_ldb_ch = imx_enc_to_imx_ldb_ch(imx_encoder);
@@ -180,8 +176,13 @@ static void imx_ldb_encoder_commit(struct drm_encoder *encoder)
 	drm_panel_prepare(imx_ldb_ch->panel);
 
 	if (dual) {
+		clk_set_parent(ldb->clk_sel[mux], ldb->clk[0]);
+		clk_set_parent(ldb->clk_sel[mux], ldb->clk[1]);
+
 		clk_prepare_enable(ldb->clk[0]);
 		clk_prepare_enable(ldb->clk[1]);
+	} else {
+		clk_set_parent(ldb->clk_sel[mux], ldb->clk[imx_ldb_ch->chno]);
 	}
 
 	if (imx_ldb_ch == &ldb->channel[0] || dual) {
@@ -321,7 +322,7 @@ static void imx_ldb_encoder_disable(struct drm_encoder *encoder)
 }
 
 static const struct drm_connector_funcs imx_ldb_connector_funcs = {
-	.dpms = drm_helper_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = imx_ldb_connector_detect,
 	.destroy = imx_drm_connector_destroy,
@@ -340,9 +341,8 @@ static const struct drm_encoder_funcs imx_ldb_encoder_funcs = {
 };
 
 static const struct drm_encoder_helper_funcs imx_ldb_encoder_helper_funcs = {
-	.dpms = imx_ldb_encoder_dpms,
-	.commit = imx_ldb_encoder_commit,
 	.mode_set = imx_ldb_encoder_mode_set,
+	.enable = imx_ldb_encoder_enable,
 	.disable = imx_ldb_encoder_disable,
 };
 
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index 82a1edd..ed19a7f 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -274,18 +274,6 @@ static struct drm_encoder *imx_tve_connector_best_encoder(
 	return &tve->imx_encoder.encoder;
 }
 
-static void imx_tve_encoder_dpms(struct drm_encoder *encoder, int mode)
-{
-	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
-	struct imx_tve *tve = imx_enc_to_tve(imx_encoder);
-	int ret;
-
-	ret = regmap_update_bits(tve->regmap, TVE_COM_CONF_REG,
-				 TVE_TV_OUT_MODE_MASK, TVE_TV_OUT_DISABLE);
-	if (ret < 0)
-		dev_err(tve->dev, "failed to disable TVOUT: %d\n", ret);
-}
-
 static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
 				     struct drm_display_mode *orig_mode,
 				     struct drm_display_mode *mode)
@@ -323,7 +311,7 @@ static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
 		dev_err(tve->dev, "failed to set configuration: %d\n", ret);
 }
 
-static void imx_tve_encoder_commit(struct drm_encoder *encoder)
+static void imx_tve_encoder_enable(struct drm_encoder *encoder)
 {
 	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
 	struct imx_tve *tve = imx_enc_to_tve(imx_encoder);
@@ -340,7 +328,7 @@ static void imx_tve_encoder_disable(struct drm_encoder *encoder)
 }
 
 static const struct drm_connector_funcs imx_tve_connector_funcs = {
-	.dpms = drm_helper_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = imx_tve_connector_detect,
 	.destroy = imx_drm_connector_destroy,
@@ -360,9 +348,8 @@ static const struct drm_encoder_funcs imx_tve_encoder_funcs = {
 };
 
 static const struct drm_encoder_helper_funcs imx_tve_encoder_helper_funcs = {
-	.dpms = imx_tve_encoder_dpms,
 	.mode_set = imx_tve_encoder_mode_set,
-	.commit = imx_tve_encoder_commit,
+	.enable = imx_tve_encoder_enable,
 	.disable = imx_tve_encoder_disable,
 };
 
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 3a3b67c..4d1b911 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -70,8 +70,9 @@ struct ipu_crtc {
 
 #define to_ipu_crtc(x) container_of(x, struct ipu_crtc, base)
 
-static void ipu_crtc_enable(struct ipu_crtc *ipu_crtc)
+static void ipu_crtc_enable(struct drm_crtc *crtc)
 {
+	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
 	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
 
 	ipu_dc_enable(ipu);
@@ -81,8 +82,9 @@ static void ipu_crtc_enable(struct ipu_crtc *ipu_crtc)
 	drm_crtc_vblank_on(&ipu_crtc->base);
 }
 
-static void ipu_crtc_disable(struct ipu_crtc *ipu_crtc)
+static void ipu_crtc_disable(struct drm_crtc *crtc)
 {
+	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
 	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
 
 	/* Stop DC channel and DI before IDMAC */
@@ -92,24 +94,6 @@ static void ipu_crtc_disable(struct ipu_crtc *ipu_crtc)
 	drm_crtc_vblank_off(&ipu_crtc->base);
 }
 
-static void ipu_crtc_dpms(struct drm_crtc *crtc, int mode)
-{
-	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
-
-	dev_dbg(ipu_crtc->dev, "%s mode: %d\n", __func__, mode);
-
-	switch (mode) {
-	case DRM_MODE_DPMS_ON:
-		ipu_crtc_enable(ipu_crtc);
-		break;
-	case DRM_MODE_DPMS_STANDBY:
-	case DRM_MODE_DPMS_SUSPEND:
-	case DRM_MODE_DPMS_OFF:
-		ipu_crtc_disable(ipu_crtc);
-		break;
-	}
-}
-
 static void ipu_flip_unref_work_func(struct work_struct *__work)
 {
 	struct ipu_flip_work *work =
@@ -280,20 +264,6 @@ static bool ipu_crtc_mode_fixup(struct drm_crtc *crtc,
 	return true;
 }
 
-static void ipu_crtc_prepare(struct drm_crtc *crtc)
-{
-	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
-
-	ipu_crtc_disable(ipu_crtc);
-}
-
-static void ipu_crtc_commit(struct drm_crtc *crtc)
-{
-	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
-
-	ipu_crtc_enable(ipu_crtc);
-}
-
 static int ipu_crtc_atomic_check(struct drm_crtc *crtc,
 				 struct drm_crtc_state *state)
 {
@@ -359,12 +329,11 @@ static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 }
 
 static const struct drm_crtc_helper_funcs ipu_helper_funcs = {
-	.dpms = ipu_crtc_dpms,
 	.mode_fixup = ipu_crtc_mode_fixup,
 	.mode_set_nofb = ipu_crtc_mode_set_nofb,
-	.prepare = ipu_crtc_prepare,
-	.commit = ipu_crtc_commit,
 	.atomic_check = ipu_crtc_atomic_check,
+	.disable = ipu_crtc_disable,
+	.enable = ipu_crtc_enable,
 };
 
 static int ipu_enable_vblank(struct drm_crtc *crtc)
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 2367f0a..999e771 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -103,18 +103,7 @@ static struct drm_encoder *imx_pd_connector_best_encoder(
 	return &imxpd->imx_encoder.encoder;
 }
 
-static void imx_pd_encoder_dpms(struct drm_encoder *encoder, int mode)
-{
-	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
-	struct imx_parallel_display *imxpd = imx_enc_to_imxpd(imx_encoder);
-
-	if (mode != DRM_MODE_DPMS_ON)
-		drm_panel_disable(imxpd->panel);
-	else
-		drm_panel_enable(imxpd->panel);
-}
-
-static void imx_pd_encoder_commit(struct drm_encoder *encoder)
+static void imx_pd_encoder_enable(struct drm_encoder *encoder)
 {
 	struct imx_drm_encoder *imx_encoder = enc_to_imx_enc(encoder);
 	struct imx_parallel_display *imxpd = imx_enc_to_imxpd(imx_encoder);
@@ -139,7 +128,7 @@ static void imx_pd_encoder_disable(struct drm_encoder *encoder)
 }
 
 static const struct drm_connector_funcs imx_pd_connector_funcs = {
-	.dpms = drm_helper_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = imx_pd_connector_detect,
 	.destroy = imx_drm_connector_destroy,
@@ -158,9 +147,8 @@ static const struct drm_encoder_funcs imx_pd_encoder_funcs = {
 };
 
 static const struct drm_encoder_helper_funcs imx_pd_encoder_helper_funcs = {
-	.dpms = imx_pd_encoder_dpms,
-	.commit = imx_pd_encoder_commit,
 	.mode_set = imx_pd_encoder_mode_set,
+	.enable = imx_pd_encoder_enable,
 	.disable = imx_pd_encoder_disable,
 };
 
-- 
2.7.4

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

[PATCH 13/14] drm/imx: atomic phase 3 step 4: Use generic atomic page flip

[Liu Ying]

To support generic atomic page flip, this patch customizes ->atomic_commit
for async commits.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 137 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/imx/ipuv3-crtc.c   | 156 ++-----------------------------------
 2 files changed, 144 insertions(+), 149 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 2fa04a0..cb521cb 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -15,10 +15,14 @@
  */
 #include <linux/component.h>
 #include <linux/device.h>
+#include <linux/dma-buf.h>
 #include <linux/fb.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/reservation.h>
+#include <linux/wait.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_crtc_helper.h>
@@ -48,6 +52,14 @@ struct imx_drm_device {
 struct imx_drm_crtc {
 	struct drm_crtc				*crtc;
 	struct imx_drm_crtc_helper_funcs	imx_drm_helper_funcs;
+	wait_queue_head_t			commit_wait;
+	bool					commit_pending;
+};
+
+struct imx_drm_commit {
+	struct work_struct work;
+	struct drm_device *dev;
+	struct drm_atomic_state *state;
 };
 
 #if IS_ENABLED(CONFIG_DRM_FBDEV_EMULATION)
@@ -168,11 +180,132 @@ static void imx_drm_output_poll_changed(struct drm_device *drm)
 	drm_fbdev_cma_hotplug_event(imxdrm->fbhelper);
 }
 
+static void imx_drm_atomic_complete(struct imx_drm_commit *commit, bool async)
+{
+	struct drm_device *dev = commit->dev;
+	struct imx_drm_device *imxdrm = dev->dev_private;
+	struct imx_drm_crtc *imx_crtc;
+	struct drm_atomic_state *old_state = commit->state;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_crtc_state;
+	struct drm_plane_state *plane_state;
+	struct drm_gem_cma_object *cma_obj;
+	struct fence *excl;
+	unsigned shared_count;
+	struct fence **shared;
+	unsigned int i, j;
+	int ret;
+
+	/* Wait for fences. */
+	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+		if (crtc->state->event) {
+			plane_state = crtc->primary->state;
+			cma_obj = drm_fb_cma_get_gem_obj(plane_state->fb, 0);
+			if (cma_obj->base.dma_buf) {
+				ret = reservation_object_get_fences_rcu(
+					cma_obj->base.dma_buf->resv, &excl,
+					&shared_count, &shared);
+				if (unlikely(ret))
+					DRM_ERROR("failed to get fences "
+						  "for buffer\n");
+
+				if (excl) {
+					fence_wait(excl, false);
+					fence_put(excl);
+				}
+				for (j = 0; j < shared_count; i++) {
+					fence_wait(shared[j], false);
+					fence_put(shared[j]);
+				}
+			}
+		}
+	}
+
+	/* Apply the atomic update. */
+	drm_atomic_helper_commit_modeset_disables(dev, old_state);
+	drm_atomic_helper_commit_modeset_enables(dev, old_state);
+	drm_atomic_helper_commit_planes(dev, old_state, false);
+	drm_atomic_helper_wait_for_vblanks(dev, old_state);
+	drm_atomic_helper_cleanup_planes(dev, old_state);
+
+	if (async)
+		for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+			imx_crtc = imxdrm->crtc[i];
+
+			/* Complete the commit, wake up any waiter. */
+			spin_lock(&imx_crtc->commit_wait.lock);
+			imx_crtc->commit_pending = false;
+			wake_up_all_locked(&imx_crtc->commit_wait);
+			spin_unlock(&imx_crtc->commit_wait.lock);
+		}
+
+	drm_atomic_state_free(old_state);
+
+	kfree(commit);
+}
+
+static void imx_drm_atomic_work(struct work_struct *work)
+{
+	struct imx_drm_commit *commit =
+		container_of(work, struct imx_drm_commit, work);
+
+	imx_drm_atomic_complete(commit, true);
+}
+
+static int imx_drm_atomic_commit(struct drm_device *dev,
+				 struct drm_atomic_state *state, bool async)
+{
+	struct imx_drm_device *imxdrm = dev->dev_private;
+	struct imx_drm_crtc *imx_crtc;
+	struct imx_drm_commit *commit;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	unsigned int i;
+	int ret;
+
+	commit = kzalloc(sizeof(*commit), GFP_KERNEL);
+	if (commit == NULL)
+		return -ENOMEM;
+
+	commit->dev = dev;
+	commit->state = state;
+
+	if (async) {
+		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+			imx_crtc = imxdrm->crtc[i];
+
+			spin_lock(&imx_crtc->commit_wait.lock);
+			ret = wait_event_interruptible_locked(
+					imx_crtc->commit_wait,
+					!imx_crtc->commit_pending);
+			if (ret == 0)
+				imx_crtc->commit_pending = true;
+			spin_unlock(&imx_crtc->commit_wait.lock);
+
+			if (ret) {
+				kfree(commit);
+				return ret;
+			}
+		}
+
+		INIT_WORK(&commit->work, imx_drm_atomic_work);
+	}
+
+	drm_atomic_helper_swap_state(dev, state);
+
+	if (async)
+		schedule_work(&commit->work);
+	else
+		imx_drm_atomic_complete(commit, async);
+
+	return 0;
+}
+
 static const struct drm_mode_config_funcs imx_drm_mode_config_funcs = {
 	.fb_create = drm_fb_cma_create,
 	.output_poll_changed = imx_drm_output_poll_changed,
 	.atomic_check = drm_atomic_helper_check,
-	.atomic_commit = drm_atomic_helper_commit,
+	.atomic_commit = imx_drm_atomic_commit,
 };
 
 /*
@@ -315,6 +448,8 @@ int imx_drm_add_crtc(struct drm_device *drm, struct drm_crtc *crtc,
 	imx_drm_crtc->imx_drm_helper_funcs = *imx_drm_helper_funcs;
 	imx_drm_crtc->crtc = crtc;
 
+	init_waitqueue_head(&imx_drm_crtc->commit_wait);
+
 	crtc->port = port;
 
 	imxdrm->crtc[imxdrm->pipes++] = imx_drm_crtc;
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 4d1b911..dcbeb56 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -24,8 +24,6 @@
 #include <linux/fb.h>
 #include <linux/clk.h>
 #include <linux/errno.h>
-#include <linux/reservation.h>
-#include <linux/dma-buf.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 
@@ -35,23 +33,6 @@
 
 #define DRIVER_DESC		"i.MX IPUv3 Graphics"
 
-enum ipu_flip_status {
-	IPU_FLIP_NONE,
-	IPU_FLIP_PENDING,
-	IPU_FLIP_SUBMITTED,
-};
-
-struct ipu_flip_work {
-	struct work_struct		unref_work;
-	struct drm_gem_object		*bo;
-	struct drm_pending_vblank_event *page_flip_event;
-	struct work_struct		fence_work;
-	struct ipu_crtc			*crtc;
-	struct fence			*excl;
-	unsigned			shared_count;
-	struct fence			**shared;
-};
-
 struct ipu_crtc {
 	struct device		*dev;
 	struct drm_crtc		base;
@@ -62,9 +43,6 @@ struct ipu_crtc {
 
 	struct ipu_dc		*dc;
 	struct ipu_di		*di;
-	enum ipu_flip_status	flip_state;
-	struct workqueue_struct *flip_queue;
-	struct ipu_flip_work	*flip_work;
 	int			irq;
 };
 
@@ -94,150 +72,35 @@ static void ipu_crtc_disable(struct drm_crtc *crtc)
 	drm_crtc_vblank_off(&ipu_crtc->base);
 }
 
-static void ipu_flip_unref_work_func(struct work_struct *__work)
-{
-	struct ipu_flip_work *work =
-			container_of(__work, struct ipu_flip_work, unref_work);
-
-	drm_gem_object_unreference_unlocked(work->bo);
-	kfree(work);
-}
-
-static void ipu_flip_fence_work_func(struct work_struct *__work)
-{
-	struct ipu_flip_work *work =
-			container_of(__work, struct ipu_flip_work, fence_work);
-	int i;
-
-	/* wait for all fences attached to the FB obj to signal */
-	if (work->excl) {
-		fence_wait(work->excl, false);
-		fence_put(work->excl);
-	}
-	for (i = 0; i < work->shared_count; i++) {
-		fence_wait(work->shared[i], false);
-		fence_put(work->shared[i]);
-	}
-
-	work->crtc->flip_state = IPU_FLIP_SUBMITTED;
-}
-
-static int ipu_page_flip(struct drm_crtc *crtc,
-		struct drm_framebuffer *fb,
-		struct drm_pending_vblank_event *event,
-		uint32_t page_flip_flags)
-{
-	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
-	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
-	struct ipu_flip_work *flip_work;
-	int ret;
-
-	if (ipu_crtc->flip_state != IPU_FLIP_NONE)
-		return -EBUSY;
-
-	ret = imx_drm_crtc_vblank_get(ipu_crtc->imx_crtc);
-	if (ret) {
-		dev_dbg(ipu_crtc->dev, "failed to acquire vblank counter\n");
-		list_del(&event->base.link);
-
-		return ret;
-	}
-
-	flip_work = kzalloc(sizeof *flip_work, GFP_KERNEL);
-	if (!flip_work) {
-		ret = -ENOMEM;
-		goto put_vblank;
-	}
-	INIT_WORK(&flip_work->unref_work, ipu_flip_unref_work_func);
-	flip_work->page_flip_event = event;
-
-	/* get BO backing the old framebuffer and take a reference */
-	flip_work->bo = &drm_fb_cma_get_gem_obj(crtc->primary->fb, 0)->base;
-	drm_gem_object_reference(flip_work->bo);
-
-	ipu_crtc->flip_work = flip_work;
-	/*
-	 * If the object has a DMABUF attached, we need to wait on its fences
-	 * if there are any.
-	 */
-	if (cma_obj->base.dma_buf) {
-		INIT_WORK(&flip_work->fence_work, ipu_flip_fence_work_func);
-		flip_work->crtc = ipu_crtc;
-
-		ret = reservation_object_get_fences_rcu(
-				cma_obj->base.dma_buf->resv, &flip_work->excl,
-				&flip_work->shared_count, &flip_work->shared);
-
-		if (unlikely(ret)) {
-			DRM_ERROR("failed to get fences for buffer\n");
-			goto free_flip_work;
-		}
-
-		/* No need to queue the worker if the are no fences */
-		if (!flip_work->excl && !flip_work->shared_count) {
-			ipu_crtc->flip_state = IPU_FLIP_SUBMITTED;
-		} else {
-			ipu_crtc->flip_state = IPU_FLIP_PENDING;
-			queue_work(ipu_crtc->flip_queue,
-				   &flip_work->fence_work);
-		}
-	} else {
-		ipu_crtc->flip_state = IPU_FLIP_SUBMITTED;
-	}
-
-	if (crtc->primary->state)
-		drm_atomic_set_fb_for_plane(crtc->primary->state, fb);
-
-	return 0;
-
-free_flip_work:
-	drm_gem_object_unreference_unlocked(flip_work->bo);
-	kfree(flip_work);
-	ipu_crtc->flip_work = NULL;
-put_vblank:
-	imx_drm_crtc_vblank_put(ipu_crtc->imx_crtc);
-
-	return ret;
-}
-
 static const struct drm_crtc_funcs ipu_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = drm_crtc_cleanup,
-	.page_flip = ipu_page_flip,
+	.page_flip = drm_atomic_helper_page_flip,
 	.reset = drm_atomic_helper_crtc_reset,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
-static void ipu_crtc_handle_pageflip(struct ipu_crtc *ipu_crtc)
+static void ipu_crtc_handle_pageflip(struct drm_crtc *crtc)
 {
+	struct drm_device *drm = crtc->dev;
 	unsigned long flags;
-	struct drm_device *drm = ipu_crtc->base.dev;
-	struct ipu_flip_work *work = ipu_crtc->flip_work;
 
 	spin_lock_irqsave(&drm->event_lock, flags);
-	if (work->page_flip_event)
-		drm_crtc_send_vblank_event(&ipu_crtc->base,
-					   work->page_flip_event);
-	imx_drm_crtc_vblank_put(ipu_crtc->imx_crtc);
+	drm_crtc_send_vblank_event(crtc, crtc->state->event);
+	crtc->state->event = NULL;
 	spin_unlock_irqrestore(&drm->event_lock, flags);
 }
 
 static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
 {
 	struct ipu_crtc *ipu_crtc = dev_id;
+	struct drm_crtc *crtc = &ipu_crtc->base;
 
 	imx_drm_handle_vblank(ipu_crtc->imx_crtc);
 
-	if (ipu_crtc->flip_state == IPU_FLIP_SUBMITTED) {
-		struct ipu_plane *plane = ipu_crtc->plane[0];
-
-		ipu_plane_set_base(plane, ipu_crtc->base.primary->fb);
-		ipu_crtc_handle_pageflip(ipu_crtc);
-		queue_work(ipu_crtc->flip_queue,
-			   &ipu_crtc->flip_work->unref_work);
-		ipu_crtc->flip_state = IPU_FLIP_NONE;
-	}
+	if (crtc->state->event)
+		ipu_crtc_handle_pageflip(crtc);
 
 	return IRQ_HANDLED;
 }
@@ -452,8 +315,6 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 
 	ipu_plane_put_resources(ipu_crtc->plane[0]);
 
-	ipu_crtc->flip_queue = create_singlethread_workqueue("ipu-crtc-flip");
-
 	return 0;
 
 err_put_plane_res:
@@ -495,7 +356,6 @@ static void ipu_drm_unbind(struct device *dev, struct device *master,
 
 	imx_drm_remove_crtc(ipu_crtc->imx_crtc);
 
-	destroy_workqueue(ipu_crtc->flip_queue);
 	ipu_put_resources(ipu_crtc);
 }
 
-- 
2.7.4

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

[PATCH 14/14] drm/imx: atomic phase 3 step 5: Advertise DRIVER_ATOMIC

[Liu Ying]

With all the beforehand phases and steps done, we can adverstise DRIVER_ATOMIC.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index cb521cb..5f6ba2f 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -512,7 +512,8 @@ static const struct drm_ioctl_desc imx_drm_ioctls[] = {
 };
 
 static struct drm_driver imx_drm_driver = {
-	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME,
+	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
+				  DRIVER_ATOMIC,
 	.load			= imx_drm_driver_load,
 	.unload			= imx_drm_driver_unload,
 	.lastclose		= imx_drm_driver_lastclose,
-- 
2.7.4

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

Re: [PATCH 05/14] drm/crtc_helper: Disable and reenable primary plane in drm_helper_crtc_mode_set

[Daniel Vetter]

On Tue, May 24, 2016 at 06:10:44PM +0800, Liu Ying wrote:
> Since CRTC has already been disabled in crtc_funcs->prepare(), it doesn't hurt
> to disable the primary plane in drm_helper_crtc_mode_set() before enabling it
> in drm_helper_crtc_mode_set_base().  This makes those who reject active plane
> update in plane_funcs->atomic_check() happy.

How/where exactly do you blow up?
> 
> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> ---
>  drivers/gpu/drm/drm_crtc_helper.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 79555d2..7fabcd7 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -1013,6 +1013,15 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
>  			goto out;
>  	}
>  
> +	/*
> +	 * It doesn't hurt to disable primary plane here since crtc is off
> +	 * and we'll enable it again in drm_helper_crtc_mode_set_base()
> +	 * below soon.
> +	 */
> +	ret = drm_plane_helper_disable(crtc->primary);

You can't call this helper from crtc helpers. drm_plane_helper_disable
assumes that a) you've already extracted universal plane support for the
primary plane by registering a proper drm_plane for it b) that you're
driver is at least partially using atomic hooks already.

Both assumptions are wrong for all current users of this function.
drm_helper_crtc_mode_set() is purely a legacy helper for legecy drivers.
-Daniel

> +	if (ret)
> +		goto out;
> +
>  	swap(crtc->state, crtc_state);
>  
>  	crtc_funcs->mode_set_nofb(crtc);
> -- 
> 2.7.4
> 
> _______________________________________________
> 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

Re: [PATCH 10/14] drm/atomic-helper: Disable planes when suspending

[Daniel Vetter]

On Tue, May 24, 2016 at 06:10:49PM +0800, Liu Ying wrote:
> We should disable planes explicitly when suspending.
> Especially, this is meaningful for those display controllers which
> don't support active planes without relevant CRTCs being enabled.
> 
> Signed-off-by: Liu Ying <gnuiyl@gmail.com>

Recommended way is to call drm_atomic_helper_disable_planes_on_crtc in
your crtc's ->disable() callback if your hw needs this. This is a general
problem (test e.g. dpms), not just an issue in suspend code.

Also unsetting the planes from state has a semantic meaning: It unpins the
backing storage, which is definitely not what we want for suspend/resume.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 4befe25..5331d95 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1967,7 +1967,7 @@ commit:
>   *
>   * Loops through all connectors, finding those that aren't turned off and then
>   * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
> - * that they are connected to.
> + * that they are connected to.  The relevant planes are deactivated as well.
>   *
>   * This is used for example in suspend/resume to disable all currently active
>   * functions when suspending.
> @@ -1997,6 +1997,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
>  	drm_for_each_connector(conn, dev) {
>  		struct drm_crtc *crtc = conn->state->crtc;
>  		struct drm_crtc_state *crtc_state;
> +		struct drm_plane *plane;
>  
>  		if (!crtc || conn->dpms != DRM_MODE_DPMS_ON)
>  			continue;
> @@ -2008,6 +2009,21 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
>  		}
>  
>  		crtc_state->active = false;
> +
> +		drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
> +			struct drm_plane_state *plane_state;
> +
> +			plane_state = drm_atomic_get_plane_state(state, plane);
> +			if (IS_ERR(plane_state)) {
> +				err = PTR_ERR(plane_state);
> +				goto free;
> +			}
> +
> +			err = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> +			if (err != 0)
> +				goto free;
> +			drm_atomic_set_fb_for_plane(plane_state, NULL);
> +		}
>  	}
>  
>  	err = drm_atomic_commit(state);
> -- 
> 2.7.4
> 
> _______________________________________________
> 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

Re: [PATCH 13/14] drm/imx: atomic phase 3 step 4: Use generic atomic page flip

[Daniel Vetter]

On Tue, May 24, 2016 at 06:10:52PM +0800, Liu Ying wrote:
> To support generic atomic page flip, this patch customizes ->atomic_commit
> for async commits.

It's now called nonblocking instead of async. Please run s/async/nonblock/
over this patch.
-Daniel

> 
> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c | 137 +++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/imx/ipuv3-crtc.c   | 156 ++-----------------------------------
>  2 files changed, 144 insertions(+), 149 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 2fa04a0..cb521cb 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -15,10 +15,14 @@
>   */
>  #include <linux/component.h>
>  #include <linux/device.h>
> +#include <linux/dma-buf.h>
>  #include <linux/fb.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/reservation.h>
> +#include <linux/wait.h>
>  #include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_crtc_helper.h>
> @@ -48,6 +52,14 @@ struct imx_drm_device {
>  struct imx_drm_crtc {
>  	struct drm_crtc				*crtc;
>  	struct imx_drm_crtc_helper_funcs	imx_drm_helper_funcs;
> +	wait_queue_head_t			commit_wait;
> +	bool					commit_pending;
> +};
> +
> +struct imx_drm_commit {
> +	struct work_struct work;
> +	struct drm_device *dev;
> +	struct drm_atomic_state *state;
>  };
>  
>  #if IS_ENABLED(CONFIG_DRM_FBDEV_EMULATION)
> @@ -168,11 +180,132 @@ static void imx_drm_output_poll_changed(struct drm_device *drm)
>  	drm_fbdev_cma_hotplug_event(imxdrm->fbhelper);
>  }
>  
> +static void imx_drm_atomic_complete(struct imx_drm_commit *commit, bool async)
> +{
> +	struct drm_device *dev = commit->dev;
> +	struct imx_drm_device *imxdrm = dev->dev_private;
> +	struct imx_drm_crtc *imx_crtc;
> +	struct drm_atomic_state *old_state = commit->state;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *old_crtc_state;
> +	struct drm_plane_state *plane_state;
> +	struct drm_gem_cma_object *cma_obj;
> +	struct fence *excl;
> +	unsigned shared_count;
> +	struct fence **shared;
> +	unsigned int i, j;
> +	int ret;
> +
> +	/* Wait for fences. */
> +	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> +		if (crtc->state->event) {
> +			plane_state = crtc->primary->state;
> +			cma_obj = drm_fb_cma_get_gem_obj(plane_state->fb, 0);
> +			if (cma_obj->base.dma_buf) {
> +				ret = reservation_object_get_fences_rcu(
> +					cma_obj->base.dma_buf->resv, &excl,
> +					&shared_count, &shared);
> +				if (unlikely(ret))
> +					DRM_ERROR("failed to get fences "
> +						  "for buffer\n");
> +
> +				if (excl) {
> +					fence_wait(excl, false);
> +					fence_put(excl);
> +				}
> +				for (j = 0; j < shared_count; i++) {
> +					fence_wait(shared[j], false);
> +					fence_put(shared[j]);
> +				}
> +			}
> +		}
> +	}
> +
> +	/* Apply the atomic update. */
> +	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> +	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> +	drm_atomic_helper_commit_planes(dev, old_state, false);
> +	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> +	drm_atomic_helper_cleanup_planes(dev, old_state);
> +
> +	if (async)
> +		for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> +			imx_crtc = imxdrm->crtc[i];
> +
> +			/* Complete the commit, wake up any waiter. */
> +			spin_lock(&imx_crtc->commit_wait.lock);
> +			imx_crtc->commit_pending = false;
> +			wake_up_all_locked(&imx_crtc->commit_wait);
> +			spin_unlock(&imx_crtc->commit_wait.lock);
> +		}
> +
> +	drm_atomic_state_free(old_state);
> +
> +	kfree(commit);
> +}
> +
> +static void imx_drm_atomic_work(struct work_struct *work)
> +{
> +	struct imx_drm_commit *commit =
> +		container_of(work, struct imx_drm_commit, work);
> +
> +	imx_drm_atomic_complete(commit, true);
> +}
> +
> +static int imx_drm_atomic_commit(struct drm_device *dev,
> +				 struct drm_atomic_state *state, bool async)
> +{
> +	struct imx_drm_device *imxdrm = dev->dev_private;
> +	struct imx_drm_crtc *imx_crtc;
> +	struct imx_drm_commit *commit;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	unsigned int i;
> +	int ret;
> +
> +	commit = kzalloc(sizeof(*commit), GFP_KERNEL);
> +	if (commit == NULL)
> +		return -ENOMEM;
> +
> +	commit->dev = dev;
> +	commit->state = state;
> +
> +	if (async) {
> +		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +			imx_crtc = imxdrm->crtc[i];
> +
> +			spin_lock(&imx_crtc->commit_wait.lock);
> +			ret = wait_event_interruptible_locked(
> +					imx_crtc->commit_wait,
> +					!imx_crtc->commit_pending);
> +			if (ret == 0)
> +				imx_crtc->commit_pending = true;
> +			spin_unlock(&imx_crtc->commit_wait.lock);
> +
> +			if (ret) {
> +				kfree(commit);
> +				return ret;
> +			}
> +		}
> +
> +		INIT_WORK(&commit->work, imx_drm_atomic_work);
> +	}
> +
> +	drm_atomic_helper_swap_state(dev, state);
> +
> +	if (async)
> +		schedule_work(&commit->work);
> +	else
> +		imx_drm_atomic_complete(commit, async);
> +
> +	return 0;
> +}
> +
>  static const struct drm_mode_config_funcs imx_drm_mode_config_funcs = {
>  	.fb_create = drm_fb_cma_create,
>  	.output_poll_changed = imx_drm_output_poll_changed,
>  	.atomic_check = drm_atomic_helper_check,
> -	.atomic_commit = drm_atomic_helper_commit,
> +	.atomic_commit = imx_drm_atomic_commit,
>  };
>  
>  /*
> @@ -315,6 +448,8 @@ int imx_drm_add_crtc(struct drm_device *drm, struct drm_crtc *crtc,
>  	imx_drm_crtc->imx_drm_helper_funcs = *imx_drm_helper_funcs;
>  	imx_drm_crtc->crtc = crtc;
>  
> +	init_waitqueue_head(&imx_drm_crtc->commit_wait);
> +
>  	crtc->port = port;
>  
>  	imxdrm->crtc[imxdrm->pipes++] = imx_drm_crtc;
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index 4d1b911..dcbeb56 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -24,8 +24,6 @@
>  #include <linux/fb.h>
>  #include <linux/clk.h>
>  #include <linux/errno.h>
> -#include <linux/reservation.h>
> -#include <linux/dma-buf.h>
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_fb_cma_helper.h>
>  
> @@ -35,23 +33,6 @@
>  
>  #define DRIVER_DESC		"i.MX IPUv3 Graphics"
>  
> -enum ipu_flip_status {
> -	IPU_FLIP_NONE,
> -	IPU_FLIP_PENDING,
> -	IPU_FLIP_SUBMITTED,
> -};
> -
> -struct ipu_flip_work {
> -	struct work_struct		unref_work;
> -	struct drm_gem_object		*bo;
> -	struct drm_pending_vblank_event *page_flip_event;
> -	struct work_struct		fence_work;
> -	struct ipu_crtc			*crtc;
> -	struct fence			*excl;
> -	unsigned			shared_count;
> -	struct fence			**shared;
> -};
> -
>  struct ipu_crtc {
>  	struct device		*dev;
>  	struct drm_crtc		base;
> @@ -62,9 +43,6 @@ struct ipu_crtc {
>  
>  	struct ipu_dc		*dc;
>  	struct ipu_di		*di;
> -	enum ipu_flip_status	flip_state;
> -	struct workqueue_struct *flip_queue;
> -	struct ipu_flip_work	*flip_work;
>  	int			irq;
>  };
>  
> @@ -94,150 +72,35 @@ static void ipu_crtc_disable(struct drm_crtc *crtc)
>  	drm_crtc_vblank_off(&ipu_crtc->base);
>  }
>  
> -static void ipu_flip_unref_work_func(struct work_struct *__work)
> -{
> -	struct ipu_flip_work *work =
> -			container_of(__work, struct ipu_flip_work, unref_work);
> -
> -	drm_gem_object_unreference_unlocked(work->bo);
> -	kfree(work);
> -}
> -
> -static void ipu_flip_fence_work_func(struct work_struct *__work)
> -{
> -	struct ipu_flip_work *work =
> -			container_of(__work, struct ipu_flip_work, fence_work);
> -	int i;
> -
> -	/* wait for all fences attached to the FB obj to signal */
> -	if (work->excl) {
> -		fence_wait(work->excl, false);
> -		fence_put(work->excl);
> -	}
> -	for (i = 0; i < work->shared_count; i++) {
> -		fence_wait(work->shared[i], false);
> -		fence_put(work->shared[i]);
> -	}
> -
> -	work->crtc->flip_state = IPU_FLIP_SUBMITTED;
> -}
> -
> -static int ipu_page_flip(struct drm_crtc *crtc,
> -		struct drm_framebuffer *fb,
> -		struct drm_pending_vblank_event *event,
> -		uint32_t page_flip_flags)
> -{
> -	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> -	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
> -	struct ipu_flip_work *flip_work;
> -	int ret;
> -
> -	if (ipu_crtc->flip_state != IPU_FLIP_NONE)
> -		return -EBUSY;
> -
> -	ret = imx_drm_crtc_vblank_get(ipu_crtc->imx_crtc);
> -	if (ret) {
> -		dev_dbg(ipu_crtc->dev, "failed to acquire vblank counter\n");
> -		list_del(&event->base.link);
> -
> -		return ret;
> -	}
> -
> -	flip_work = kzalloc(sizeof *flip_work, GFP_KERNEL);
> -	if (!flip_work) {
> -		ret = -ENOMEM;
> -		goto put_vblank;
> -	}
> -	INIT_WORK(&flip_work->unref_work, ipu_flip_unref_work_func);
> -	flip_work->page_flip_event = event;
> -
> -	/* get BO backing the old framebuffer and take a reference */
> -	flip_work->bo = &drm_fb_cma_get_gem_obj(crtc->primary->fb, 0)->base;
> -	drm_gem_object_reference(flip_work->bo);
> -
> -	ipu_crtc->flip_work = flip_work;
> -	/*
> -	 * If the object has a DMABUF attached, we need to wait on its fences
> -	 * if there are any.
> -	 */
> -	if (cma_obj->base.dma_buf) {
> -		INIT_WORK(&flip_work->fence_work, ipu_flip_fence_work_func);
> -		flip_work->crtc = ipu_crtc;
> -
> -		ret = reservation_object_get_fences_rcu(
> -				cma_obj->base.dma_buf->resv, &flip_work->excl,
> -				&flip_work->shared_count, &flip_work->shared);
> -
> -		if (unlikely(ret)) {
> -			DRM_ERROR("failed to get fences for buffer\n");
> -			goto free_flip_work;
> -		}
> -
> -		/* No need to queue the worker if the are no fences */
> -		if (!flip_work->excl && !flip_work->shared_count) {
> -			ipu_crtc->flip_state = IPU_FLIP_SUBMITTED;
> -		} else {
> -			ipu_crtc->flip_state = IPU_FLIP_PENDING;
> -			queue_work(ipu_crtc->flip_queue,
> -				   &flip_work->fence_work);
> -		}
> -	} else {
> -		ipu_crtc->flip_state = IPU_FLIP_SUBMITTED;
> -	}
> -
> -	if (crtc->primary->state)
> -		drm_atomic_set_fb_for_plane(crtc->primary->state, fb);
> -
> -	return 0;
> -
> -free_flip_work:
> -	drm_gem_object_unreference_unlocked(flip_work->bo);
> -	kfree(flip_work);
> -	ipu_crtc->flip_work = NULL;
> -put_vblank:
> -	imx_drm_crtc_vblank_put(ipu_crtc->imx_crtc);
> -
> -	return ret;
> -}
> -
>  static const struct drm_crtc_funcs ipu_crtc_funcs = {
>  	.set_config = drm_atomic_helper_set_config,
>  	.destroy = drm_crtc_cleanup,
> -	.page_flip = ipu_page_flip,
> +	.page_flip = drm_atomic_helper_page_flip,
>  	.reset = drm_atomic_helper_crtc_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>  };
>  
> -static void ipu_crtc_handle_pageflip(struct ipu_crtc *ipu_crtc)
> +static void ipu_crtc_handle_pageflip(struct drm_crtc *crtc)
>  {
> +	struct drm_device *drm = crtc->dev;
>  	unsigned long flags;
> -	struct drm_device *drm = ipu_crtc->base.dev;
> -	struct ipu_flip_work *work = ipu_crtc->flip_work;
>  
>  	spin_lock_irqsave(&drm->event_lock, flags);
> -	if (work->page_flip_event)
> -		drm_crtc_send_vblank_event(&ipu_crtc->base,
> -					   work->page_flip_event);
> -	imx_drm_crtc_vblank_put(ipu_crtc->imx_crtc);
> +	drm_crtc_send_vblank_event(crtc, crtc->state->event);
> +	crtc->state->event = NULL;
>  	spin_unlock_irqrestore(&drm->event_lock, flags);
>  }
>  
>  static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
>  {
>  	struct ipu_crtc *ipu_crtc = dev_id;
> +	struct drm_crtc *crtc = &ipu_crtc->base;
>  
>  	imx_drm_handle_vblank(ipu_crtc->imx_crtc);
>  
> -	if (ipu_crtc->flip_state == IPU_FLIP_SUBMITTED) {
> -		struct ipu_plane *plane = ipu_crtc->plane[0];
> -
> -		ipu_plane_set_base(plane, ipu_crtc->base.primary->fb);
> -		ipu_crtc_handle_pageflip(ipu_crtc);
> -		queue_work(ipu_crtc->flip_queue,
> -			   &ipu_crtc->flip_work->unref_work);
> -		ipu_crtc->flip_state = IPU_FLIP_NONE;
> -	}
> +	if (crtc->state->event)
> +		ipu_crtc_handle_pageflip(crtc);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -452,8 +315,6 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
>  
>  	ipu_plane_put_resources(ipu_crtc->plane[0]);
>  
> -	ipu_crtc->flip_queue = create_singlethread_workqueue("ipu-crtc-flip");
> -
>  	return 0;
>  
>  err_put_plane_res:
> @@ -495,7 +356,6 @@ static void ipu_drm_unbind(struct device *dev, struct device *master,
>  
>  	imx_drm_remove_crtc(ipu_crtc->imx_crtc);
>  
> -	destroy_workqueue(ipu_crtc->flip_queue);
>  	ipu_put_resources(ipu_crtc);
>  }
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> 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

Re: [PATCH 00/14] imx drm atomic mode setting conversion

[Philipp Zabel]

Hi Liu,

Am Dienstag, den 24.05.2016, 18:10 +0800 schrieb Liu Ying:
> Hi,
> 
> This patch set converts imx drm into atomic mode setting.
> It takes 3 phases to achieve the goal as recommended.
> This patch set may apply to Philipp Zabel's open git branch imx-drm/fixes.

Thank you for the rework! Could I ask you to rebase this onto
imx-drm/next?
I have applied the first two patches, and I have some superficial
comments for some of the later patches at this time. Still have to look
at the rest in detail.

best regards
Philipp

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

Re: [PATCH 03/14] drm: imx: ipuv3 plane: Check different types of plane separately

[Philipp Zabel]

Am Dienstag, den 24.05.2016, 18:10 +0800 schrieb Liu Ying:
> The IPUv3 primary plane doesn't support partial off screen.
> So, this patch separates plane check logics for primary plane and overlay
> plane and adds more limitations on the primary plane.
> 
> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> ---
>  drivers/gpu/drm/imx/ipuv3-plane.c | 72 ++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index e6ec8eb..8f91b2e 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -190,44 +190,54 @@ int ipu_plane_mode_set(struct ipu_plane *ipu_plane, struct drm_crtc *crtc,
>  		       uint32_t src_x, uint32_t src_y,
>  		       uint32_t src_w, uint32_t src_h, bool interlaced)
>  {
> -	struct device *dev = ipu_plane->base.dev->dev;
> +	struct drm_plane plane = ipu_plane->base;

Why make a copy of the drm_plane here?

> +	struct device *dev = plane.dev->dev;
>  	int ret;
>  
>  	/* no scaling */
>  	if (src_w != crtc_w || src_h != crtc_h)
>  		return -EINVAL;
>  
> -	/* clip to crtc bounds */
> -	if (crtc_x < 0) {
> -		if (-crtc_x > crtc_w)
> +	if (plane.type == DRM_PLANE_TYPE_PRIMARY) {
> +		/* full plane doesn't support partial off screen */
> +		if (crtc_x || crtc_y || crtc_w != mode->hdisplay ||
> +			crtc_h != mode->vdisplay)

As long as the requested plane is large enough to cover the whole base
plane, we can fix the crtc_x/y/w/h up by clipping to the base plane
boundaries. There is no need to return -EINVAL here as long as the IDMAC
is capable to start reading at src_x/y = -crtc_x/y.

regards
Philipp

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

Re: [PATCH 04/14] gpu: ipu-v3: ipu-dmfc: Use static DMFC FIFO allocation mechanism

[Philipp Zabel]

Am Dienstag, den 24.05.2016, 18:10 +0800 schrieb Liu Ying:
> For normal video modes such as 1280x720p@60Hz and 1920x1080p@60Hz,

I'm very happy to drop all this code if possible, but the stated reason
is not good enough. Rather we should look at the worst case and make
sure that 2 FIFO slots are enough for 180 MPixel/s on i.MX5 and enough
for 240 MPixel/s on i.MX6.
According to the current heuristic (total bandwidth = clkrate * 4 pixels
with 8 slots) that should be true.

regards
Philipp

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

Re: [PATCH 06/14] drm/imx: atomic phase 1: Use transitional atomic CRTC and plane helpers

[Philipp Zabel]

Am Dienstag, den 24.05.2016, 18:10 +0800 schrieb Liu Ying:
> Use the drm_plane_helper_update/disable() and drm_helper_crtc_mode_set()
> transistional atomic helpers.  The crtc->mode_set_nofb callback is added
> so that the primary plane is no longer tied to the CRTC.  Check/update
> logics are separated to make sure crtc->mode_set_nofb and plane->atomic_update
> are always successful.  Also, some necessary logics are tweaked for a smooth
> transition.
> 
> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> ---
>  drivers/gpu/drm/imx/ipuv3-crtc.c  | 173 +++++++------
>  drivers/gpu/drm/imx/ipuv3-plane.c | 516 +++++++++++++++++++++++---------------
>  drivers/gpu/drm/imx/ipuv3-plane.h |  14 +-
>  drivers/gpu/ipu-v3/ipu-dc.c       |   5 +-
>  drivers/gpu/ipu-v3/ipu-di.c       |   3 -
>  5 files changed, 400 insertions(+), 311 deletions(-)
> 
[...]
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index 30cedbb..4511b84 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -16,6 +16,7 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_plane_helper.h>
>  
>  #include "video/imx-ipu-v3.h"
>  #include "ipuv3-plane.h"
> @@ -51,12 +52,15 @@ int ipu_plane_irq(struct ipu_plane *ipu_plane)
>  				     IPU_IRQ_EOF);
>  }
>  
> -int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
> -		       int x, int y)
> +int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb)
>  {
> -	struct drm_gem_cma_object *cma_obj[3];
> -	unsigned long eba, ubo, vbo;
> +	struct drm_gem_cma_object *cma_obj[3], *old_cma_obj[3];
> +	struct drm_plane_state *state = ipu_plane->base.state;
> +	struct drm_framebuffer *old_fb = state->fb;
> +	unsigned long eba, ubo, vbo, old_eba, old_ubo, old_vbo;

It looks to me like eba, ubo, vbo are good candidates to be put into a
custom ipu_plane_state structure during check.

regards
Philipp

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

Re: [PATCH 07/14] drm/imx: atomic phase 2 step 1: Wire up state ->reset, ->duplicate and ->destroy

[Philipp Zabel]

Am Dienstag, den 24.05.2016, 18:10 +0800 schrieb Liu Ying:
> Wire up CRTCs', planes' and connectors' ->reset, ->duplicate and ->destroy state
> hooks to use the default implementations from the atomic helper library.
> The helpers track each DRM object state.
> 
> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c       | 19 +++----------------
>  drivers/gpu/drm/imx/imx-drm-core.c     |  2 ++
>  drivers/gpu/drm/imx/imx-ldb.c          |  4 ++++
>  drivers/gpu/drm/imx/imx-tve.c          |  4 ++++
>  drivers/gpu/drm/imx/ipuv3-crtc.c       |  4 ++++
>  drivers/gpu/drm/imx/ipuv3-plane.c      |  4 ++++
>  drivers/gpu/drm/imx/parallel-display.c |  4 ++++
>  7 files changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 9795b72..6541b3c 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1508,14 +1508,6 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
>  	mutex_unlock(&hdmi->mutex);
>  }
>  
> -static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
> -	.dpms = drm_helper_connector_dpms,
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.detect = dw_hdmi_connector_detect,
> -	.destroy = dw_hdmi_connector_destroy,
> -	.force = dw_hdmi_connector_force,
> -};
> -
>  static const struct drm_connector_funcs dw_hdmi_atomic_connector_funcs = {

Might as well remove the _atomic_ form the structure's name.

regards
Philipp

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

Re: [PATCH 07/14] drm/imx: atomic phase 2 step 1: Wire up state ->reset, ->duplicate and ->destroy

[Ying Liu]

On Tue, May 24, 2016 at 10:23 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Dienstag, den 24.05.2016, 18:10 +0800 schrieb Liu Ying:
>> Wire up CRTCs', planes' and connectors' ->reset, ->duplicate and ->destroy state
>> hooks to use the default implementations from the atomic helper library.
>> The helpers track each DRM object state.
>>
>> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
>> ---
>>  drivers/gpu/drm/bridge/dw-hdmi.c       | 19 +++----------------
>>  drivers/gpu/drm/imx/imx-drm-core.c     |  2 ++
>>  drivers/gpu/drm/imx/imx-ldb.c          |  4 ++++
>>  drivers/gpu/drm/imx/imx-tve.c          |  4 ++++
>>  drivers/gpu/drm/imx/ipuv3-crtc.c       |  4 ++++
>>  drivers/gpu/drm/imx/ipuv3-plane.c      |  4 ++++
>>  drivers/gpu/drm/imx/parallel-display.c |  4 ++++
>>  7 files changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
>> index 9795b72..6541b3c 100644
>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>> @@ -1508,14 +1508,6 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
>>       mutex_unlock(&hdmi->mutex);
>>  }
>>
>> -static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
>> -     .dpms = drm_helper_connector_dpms,
>> -     .fill_modes = drm_helper_probe_single_connector_modes,
>> -     .detect = dw_hdmi_connector_detect,
>> -     .destroy = dw_hdmi_connector_destroy,
>> -     .force = dw_hdmi_connector_force,
>> -};
>> -
>>  static const struct drm_connector_funcs dw_hdmi_atomic_connector_funcs = {
>
> Might as well remove the _atomic_ form the structure's name.

Good point.  Will do.

Regards,
Liu Ying

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

Re: [PATCH 06/14] drm/imx: atomic phase 1: Use transitional atomic CRTC and plane helpers

[Ying Liu]

On Tue, May 24, 2016 at 10:23 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Dienstag, den 24.05.2016, 18:10 +0800 schrieb Liu Ying:
>> Use the drm_plane_helper_update/disable() and drm_helper_crtc_mode_set()
>> transistional atomic helpers.  The crtc->mode_set_nofb callback is added
>> so that the primary plane is no longer tied to the CRTC.  Check/update
>> logics are separated to make sure crtc->mode_set_nofb and plane->atomic_update
>> are always successful.  Also, some necessary logics are tweaked for a smooth
>> transition.
>>
>> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
>> ---
>>  drivers/gpu/drm/imx/ipuv3-crtc.c  | 173 +++++++------
>>  drivers/gpu/drm/imx/ipuv3-plane.c | 516 +++++++++++++++++++++++---------------
>>  drivers/gpu/drm/imx/ipuv3-plane.h |  14 +-
>>  drivers/gpu/ipu-v3/ipu-dc.c       |   5 +-
>>  drivers/gpu/ipu-v3/ipu-di.c       |   3 -
>>  5 files changed, 400 insertions(+), 311 deletions(-)
>>
> [...]
>> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
>> index 30cedbb..4511b84 100644
>> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
>> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
>> @@ -16,6 +16,7 @@
>>  #include <drm/drmP.h>
>>  #include <drm/drm_fb_cma_helper.h>
>>  #include <drm/drm_gem_cma_helper.h>
>> +#include <drm/drm_plane_helper.h>
>>
>>  #include "video/imx-ipu-v3.h"
>>  #include "ipuv3-plane.h"
>> @@ -51,12 +52,15 @@ int ipu_plane_irq(struct ipu_plane *ipu_plane)
>>                                    IPU_IRQ_EOF);
>>  }
>>
>> -int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb,
>> -                    int x, int y)
>> +int ipu_plane_set_base(struct ipu_plane *ipu_plane, struct drm_framebuffer *fb)
>>  {
>> -     struct drm_gem_cma_object *cma_obj[3];
>> -     unsigned long eba, ubo, vbo;
>> +     struct drm_gem_cma_object *cma_obj[3], *old_cma_obj[3];
>> +     struct drm_plane_state *state = ipu_plane->base.state;
>> +     struct drm_framebuffer *old_fb = state->fb;
>> +     unsigned long eba, ubo, vbo, old_eba, old_ubo, old_vbo;
>
> It looks to me like eba, ubo, vbo are good candidates to be put into a
> custom ipu_plane_state structure during check.

Yes for the first impression. I'll think about that more.
If the required change is too invasive and you accept, I'd like to
customize ipu_plane_state with another patch(set) after this one
is landed.

Regards,
Liu Ying

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

Re: [PATCH 03/14] drm: imx: ipuv3 plane: Check different types of plane separately

[Ying Liu]

On Tue, May 24, 2016 at 10:20 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Dienstag, den 24.05.2016, 18:10 +0800 schrieb Liu Ying:
>> The IPUv3 primary plane doesn't support partial off screen.
>> So, this patch separates plane check logics for primary plane and overlay
>> plane and adds more limitations on the primary plane.
>>
>> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
>> ---
>>  drivers/gpu/drm/imx/ipuv3-plane.c | 72 ++++++++++++++++++++++-----------------
>>  1 file changed, 41 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
>> index e6ec8eb..8f91b2e 100644
>> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
>> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
>> @@ -190,44 +190,54 @@ int ipu_plane_mode_set(struct ipu_plane *ipu_plane, struct drm_crtc *crtc,
>>                      uint32_t src_x, uint32_t src_y,
>>                      uint32_t src_w, uint32_t src_h, bool interlaced)
>>  {
>> -     struct device *dev = ipu_plane->base.dev->dev;
>> +     struct drm_plane plane = ipu_plane->base;
>
> Why make a copy of the drm_plane here?
>
>> +     struct device *dev = plane.dev->dev;
>>       int ret;
>>
>>       /* no scaling */
>>       if (src_w != crtc_w || src_h != crtc_h)
>>               return -EINVAL;
>>
>> -     /* clip to crtc bounds */
>> -     if (crtc_x < 0) {
>> -             if (-crtc_x > crtc_w)
>> +     if (plane.type == DRM_PLANE_TYPE_PRIMARY) {
>> +             /* full plane doesn't support partial off screen */
>> +             if (crtc_x || crtc_y || crtc_w != mode->hdisplay ||
>> +                     crtc_h != mode->vdisplay)
>
> As long as the requested plane is large enough to cover the whole base
> plane, we can fix the crtc_x/y/w/h up by clipping to the base plane
> boundaries. There is no need to return -EINVAL here as long as the IDMAC
> is capable to start reading at src_x/y = -crtc_x/y.

For the primary plane, we really need to look at mode->v/hdisplay
to make sure no partial off screen happens(plane partial off CRTC).
The clip is tricky, so for primary plane, I just take the simple way
to avoid the clip.

Eventually, we'll use atomic check to simply accept or reject
the user's request instead of changing the state to do clip.
This makes the driver simpler.

Regards,
Liu Ying

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

Re: [PATCH 00/14] imx drm atomic mode setting conversion

[Ying Liu]

On Tue, May 24, 2016 at 10:19 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Liu,
>
> Am Dienstag, den 24.05.2016, 18:10 +0800 schrieb Liu Ying:
>> Hi,
>>
>> This patch set converts imx drm into atomic mode setting.
>> It takes 3 phases to achieve the goal as recommended.
>> This patch set may apply to Philipp Zabel's open git branch imx-drm/fixes.
>
> Thank you for the rework! Could I ask you to rebase this onto
> imx-drm/next?

With some comments addressed, I have a local version which is
based on imx-drm/next.  Will send it out after it is mature.

> I have applied the first two patches, and I have some superficial
> comments for some of the later patches at this time. Still have to look
> at the rest in detail.

Thanks for the review.

Regards,
Liu Ying

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

Re: [PATCH 13/14] drm/imx: atomic phase 3 step 4: Use generic atomic page flip

[Ying Liu]

On Tue, May 24, 2016 at 7:11 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, May 24, 2016 at 06:10:52PM +0800, Liu Ying wrote:
>> To support generic atomic page flip, this patch customizes ->atomic_commit
>> for async commits.
>
> It's now called nonblocking instead of async. Please run s/async/nonblock/
> over this patch.

Ok. Will do.

Regards,
Liu Ying

> -Daniel
>
>>
>> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
>> ---
>>  drivers/gpu/drm/imx/imx-drm-core.c | 137 +++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/imx/ipuv3-crtc.c   | 156 ++-----------------------------------
>>  2 files changed, 144 insertions(+), 149 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
>> index 2fa04a0..cb521cb 100644
>> --- a/drivers/gpu/drm/imx/imx-drm-core.c
>> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
>> @@ -15,10 +15,14 @@
>>   */
>>  #include <linux/component.h>
>>  #include <linux/device.h>
>> +#include <linux/dma-buf.h>
>>  #include <linux/fb.h>
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/reservation.h>
>> +#include <linux/wait.h>
>>  #include <drm/drmP.h>
>> +#include <drm/drm_atomic.h>
>>  #include <drm/drm_atomic_helper.h>
>>  #include <drm/drm_fb_helper.h>
>>  #include <drm/drm_crtc_helper.h>
>> @@ -48,6 +52,14 @@ struct imx_drm_device {
>>  struct imx_drm_crtc {
>>       struct drm_crtc                         *crtc;
>>       struct imx_drm_crtc_helper_funcs        imx_drm_helper_funcs;
>> +     wait_queue_head_t                       commit_wait;
>> +     bool                                    commit_pending;
>> +};
>> +
>> +struct imx_drm_commit {
>> +     struct work_struct work;
>> +     struct drm_device *dev;
>> +     struct drm_atomic_state *state;
>>  };
>>
>>  #if IS_ENABLED(CONFIG_DRM_FBDEV_EMULATION)
>> @@ -168,11 +180,132 @@ static void imx_drm_output_poll_changed(struct drm_device *drm)
>>       drm_fbdev_cma_hotplug_event(imxdrm->fbhelper);
>>  }
>>
>> +static void imx_drm_atomic_complete(struct imx_drm_commit *commit, bool async)
>> +{
>> +     struct drm_device *dev = commit->dev;
>> +     struct imx_drm_device *imxdrm = dev->dev_private;
>> +     struct imx_drm_crtc *imx_crtc;
>> +     struct drm_atomic_state *old_state = commit->state;
>> +     struct drm_crtc *crtc;
>> +     struct drm_crtc_state *old_crtc_state;
>> +     struct drm_plane_state *plane_state;
>> +     struct drm_gem_cma_object *cma_obj;
>> +     struct fence *excl;
>> +     unsigned shared_count;
>> +     struct fence **shared;
>> +     unsigned int i, j;
>> +     int ret;
>> +
>> +     /* Wait for fences. */
>> +     for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
>> +             if (crtc->state->event) {
>> +                     plane_state = crtc->primary->state;
>> +                     cma_obj = drm_fb_cma_get_gem_obj(plane_state->fb, 0);
>> +                     if (cma_obj->base.dma_buf) {
>> +                             ret = reservation_object_get_fences_rcu(
>> +                                     cma_obj->base.dma_buf->resv, &excl,
>> +                                     &shared_count, &shared);
>> +                             if (unlikely(ret))
>> +                                     DRM_ERROR("failed to get fences "
>> +                                               "for buffer\n");
>> +
>> +                             if (excl) {
>> +                                     fence_wait(excl, false);
>> +                                     fence_put(excl);
>> +                             }
>> +                             for (j = 0; j < shared_count; i++) {
>> +                                     fence_wait(shared[j], false);
>> +                                     fence_put(shared[j]);
>> +                             }
>> +                     }
>> +             }
>> +     }
>> +
>> +     /* Apply the atomic update. */
>> +     drm_atomic_helper_commit_modeset_disables(dev, old_state);
>> +     drm_atomic_helper_commit_modeset_enables(dev, old_state);
>> +     drm_atomic_helper_commit_planes(dev, old_state, false);
>> +     drm_atomic_helper_wait_for_vblanks(dev, old_state);
>> +     drm_atomic_helper_cleanup_planes(dev, old_state);
>> +
>> +     if (async)
>> +             for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
>> +                     imx_crtc = imxdrm->crtc[i];
>> +
>> +                     /* Complete the commit, wake up any waiter. */
>> +                     spin_lock(&imx_crtc->commit_wait.lock);
>> +                     imx_crtc->commit_pending = false;
>> +                     wake_up_all_locked(&imx_crtc->commit_wait);
>> +                     spin_unlock(&imx_crtc->commit_wait.lock);
>> +             }
>> +
>> +     drm_atomic_state_free(old_state);
>> +
>> +     kfree(commit);
>> +}
>> +
>> +static void imx_drm_atomic_work(struct work_struct *work)
>> +{
>> +     struct imx_drm_commit *commit =
>> +             container_of(work, struct imx_drm_commit, work);
>> +
>> +     imx_drm_atomic_complete(commit, true);
>> +}
>> +
>> +static int imx_drm_atomic_commit(struct drm_device *dev,
>> +                              struct drm_atomic_state *state, bool async)
>> +{
>> +     struct imx_drm_device *imxdrm = dev->dev_private;
>> +     struct imx_drm_crtc *imx_crtc;
>> +     struct imx_drm_commit *commit;
>> +     struct drm_crtc *crtc;
>> +     struct drm_crtc_state *crtc_state;
>> +     unsigned int i;
>> +     int ret;
>> +
>> +     commit = kzalloc(sizeof(*commit), GFP_KERNEL);
>> +     if (commit == NULL)
>> +             return -ENOMEM;
>> +
>> +     commit->dev = dev;
>> +     commit->state = state;
>> +
>> +     if (async) {
>> +             for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +                     imx_crtc = imxdrm->crtc[i];
>> +
>> +                     spin_lock(&imx_crtc->commit_wait.lock);
>> +                     ret = wait_event_interruptible_locked(
>> +                                     imx_crtc->commit_wait,
>> +                                     !imx_crtc->commit_pending);
>> +                     if (ret == 0)
>> +                             imx_crtc->commit_pending = true;
>> +                     spin_unlock(&imx_crtc->commit_wait.lock);
>> +
>> +                     if (ret) {
>> +                             kfree(commit);
>> +                             return ret;
>> +                     }
>> +             }
>> +
>> +             INIT_WORK(&commit->work, imx_drm_atomic_work);
>> +     }
>> +
>> +     drm_atomic_helper_swap_state(dev, state);
>> +
>> +     if (async)
>> +             schedule_work(&commit->work);
>> +     else
>> +             imx_drm_atomic_complete(commit, async);
>> +
>> +     return 0;
>> +}
>> +
>>  static const struct drm_mode_config_funcs imx_drm_mode_config_funcs = {
>>       .fb_create = drm_fb_cma_create,
>>       .output_poll_changed = imx_drm_output_poll_changed,
>>       .atomic_check = drm_atomic_helper_check,
>> -     .atomic_commit = drm_atomic_helper_commit,
>> +     .atomic_commit = imx_drm_atomic_commit,
>>  };
>>
>>  /*
>> @@ -315,6 +448,8 @@ int imx_drm_add_crtc(struct drm_device *drm, struct drm_crtc *crtc,
>>       imx_drm_crtc->imx_drm_helper_funcs = *imx_drm_helper_funcs;
>>       imx_drm_crtc->crtc = crtc;
>>
>> +     init_waitqueue_head(&imx_drm_crtc->commit_wait);
>> +
>>       crtc->port = port;
>>
>>       imxdrm->crtc[imxdrm->pipes++] = imx_drm_crtc;
>> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
>> index 4d1b911..dcbeb56 100644
>> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
>> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
>> @@ -24,8 +24,6 @@
>>  #include <linux/fb.h>
>>  #include <linux/clk.h>
>>  #include <linux/errno.h>
>> -#include <linux/reservation.h>
>> -#include <linux/dma-buf.h>
>>  #include <drm/drm_gem_cma_helper.h>
>>  #include <drm/drm_fb_cma_helper.h>
>>
>> @@ -35,23 +33,6 @@
>>
>>  #define DRIVER_DESC          "i.MX IPUv3 Graphics"
>>
>> -enum ipu_flip_status {
>> -     IPU_FLIP_NONE,
>> -     IPU_FLIP_PENDING,
>> -     IPU_FLIP_SUBMITTED,
>> -};
>> -
>> -struct ipu_flip_work {
>> -     struct work_struct              unref_work;
>> -     struct drm_gem_object           *bo;
>> -     struct drm_pending_vblank_event *page_flip_event;
>> -     struct work_struct              fence_work;
>> -     struct ipu_crtc                 *crtc;
>> -     struct fence                    *excl;
>> -     unsigned                        shared_count;
>> -     struct fence                    **shared;
>> -};
>> -
>>  struct ipu_crtc {
>>       struct device           *dev;
>>       struct drm_crtc         base;
>> @@ -62,9 +43,6 @@ struct ipu_crtc {
>>
>>       struct ipu_dc           *dc;
>>       struct ipu_di           *di;
>> -     enum ipu_flip_status    flip_state;
>> -     struct workqueue_struct *flip_queue;
>> -     struct ipu_flip_work    *flip_work;
>>       int                     irq;
>>  };
>>
>> @@ -94,150 +72,35 @@ static void ipu_crtc_disable(struct drm_crtc *crtc)
>>       drm_crtc_vblank_off(&ipu_crtc->base);
>>  }
>>
>> -static void ipu_flip_unref_work_func(struct work_struct *__work)
>> -{
>> -     struct ipu_flip_work *work =
>> -                     container_of(__work, struct ipu_flip_work, unref_work);
>> -
>> -     drm_gem_object_unreference_unlocked(work->bo);
>> -     kfree(work);
>> -}
>> -
>> -static void ipu_flip_fence_work_func(struct work_struct *__work)
>> -{
>> -     struct ipu_flip_work *work =
>> -                     container_of(__work, struct ipu_flip_work, fence_work);
>> -     int i;
>> -
>> -     /* wait for all fences attached to the FB obj to signal */
>> -     if (work->excl) {
>> -             fence_wait(work->excl, false);
>> -             fence_put(work->excl);
>> -     }
>> -     for (i = 0; i < work->shared_count; i++) {
>> -             fence_wait(work->shared[i], false);
>> -             fence_put(work->shared[i]);
>> -     }
>> -
>> -     work->crtc->flip_state = IPU_FLIP_SUBMITTED;
>> -}
>> -
>> -static int ipu_page_flip(struct drm_crtc *crtc,
>> -             struct drm_framebuffer *fb,
>> -             struct drm_pending_vblank_event *event,
>> -             uint32_t page_flip_flags)
>> -{
>> -     struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
>> -     struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
>> -     struct ipu_flip_work *flip_work;
>> -     int ret;
>> -
>> -     if (ipu_crtc->flip_state != IPU_FLIP_NONE)
>> -             return -EBUSY;
>> -
>> -     ret = imx_drm_crtc_vblank_get(ipu_crtc->imx_crtc);
>> -     if (ret) {
>> -             dev_dbg(ipu_crtc->dev, "failed to acquire vblank counter\n");
>> -             list_del(&event->base.link);
>> -
>> -             return ret;
>> -     }
>> -
>> -     flip_work = kzalloc(sizeof *flip_work, GFP_KERNEL);
>> -     if (!flip_work) {
>> -             ret = -ENOMEM;
>> -             goto put_vblank;
>> -     }
>> -     INIT_WORK(&flip_work->unref_work, ipu_flip_unref_work_func);
>> -     flip_work->page_flip_event = event;
>> -
>> -     /* get BO backing the old framebuffer and take a reference */
>> -     flip_work->bo = &drm_fb_cma_get_gem_obj(crtc->primary->fb, 0)->base;
>> -     drm_gem_object_reference(flip_work->bo);
>> -
>> -     ipu_crtc->flip_work = flip_work;
>> -     /*
>> -      * If the object has a DMABUF attached, we need to wait on its fences
>> -      * if there are any.
>> -      */
>> -     if (cma_obj->base.dma_buf) {
>> -             INIT_WORK(&flip_work->fence_work, ipu_flip_fence_work_func);
>> -             flip_work->crtc = ipu_crtc;
>> -
>> -             ret = reservation_object_get_fences_rcu(
>> -                             cma_obj->base.dma_buf->resv, &flip_work->excl,
>> -                             &flip_work->shared_count, &flip_work->shared);
>> -
>> -             if (unlikely(ret)) {
>> -                     DRM_ERROR("failed to get fences for buffer\n");
>> -                     goto free_flip_work;
>> -             }
>> -
>> -             /* No need to queue the worker if the are no fences */
>> -             if (!flip_work->excl && !flip_work->shared_count) {
>> -                     ipu_crtc->flip_state = IPU_FLIP_SUBMITTED;
>> -             } else {
>> -                     ipu_crtc->flip_state = IPU_FLIP_PENDING;
>> -                     queue_work(ipu_crtc->flip_queue,
>> -                                &flip_work->fence_work);
>> -             }
>> -     } else {
>> -             ipu_crtc->flip_state = IPU_FLIP_SUBMITTED;
>> -     }
>> -
>> -     if (crtc->primary->state)
>> -             drm_atomic_set_fb_for_plane(crtc->primary->state, fb);
>> -
>> -     return 0;
>> -
>> -free_flip_work:
>> -     drm_gem_object_unreference_unlocked(flip_work->bo);
>> -     kfree(flip_work);
>> -     ipu_crtc->flip_work = NULL;
>> -put_vblank:
>> -     imx_drm_crtc_vblank_put(ipu_crtc->imx_crtc);
>> -
>> -     return ret;
>> -}
>> -
>>  static const struct drm_crtc_funcs ipu_crtc_funcs = {
>>       .set_config = drm_atomic_helper_set_config,
>>       .destroy = drm_crtc_cleanup,
>> -     .page_flip = ipu_page_flip,
>> +     .page_flip = drm_atomic_helper_page_flip,
>>       .reset = drm_atomic_helper_crtc_reset,
>>       .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>>       .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>  };
>>
>> -static void ipu_crtc_handle_pageflip(struct ipu_crtc *ipu_crtc)
>> +static void ipu_crtc_handle_pageflip(struct drm_crtc *crtc)
>>  {
>> +     struct drm_device *drm = crtc->dev;
>>       unsigned long flags;
>> -     struct drm_device *drm = ipu_crtc->base.dev;
>> -     struct ipu_flip_work *work = ipu_crtc->flip_work;
>>
>>       spin_lock_irqsave(&drm->event_lock, flags);
>> -     if (work->page_flip_event)
>> -             drm_crtc_send_vblank_event(&ipu_crtc->base,
>> -                                        work->page_flip_event);
>> -     imx_drm_crtc_vblank_put(ipu_crtc->imx_crtc);
>> +     drm_crtc_send_vblank_event(crtc, crtc->state->event);
>> +     crtc->state->event = NULL;
>>       spin_unlock_irqrestore(&drm->event_lock, flags);
>>  }
>>
>>  static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
>>  {
>>       struct ipu_crtc *ipu_crtc = dev_id;
>> +     struct drm_crtc *crtc = &ipu_crtc->base;
>>
>>       imx_drm_handle_vblank(ipu_crtc->imx_crtc);
>>
>> -     if (ipu_crtc->flip_state == IPU_FLIP_SUBMITTED) {
>> -             struct ipu_plane *plane = ipu_crtc->plane[0];
>> -
>> -             ipu_plane_set_base(plane, ipu_crtc->base.primary->fb);
>> -             ipu_crtc_handle_pageflip(ipu_crtc);
>> -             queue_work(ipu_crtc->flip_queue,
>> -                        &ipu_crtc->flip_work->unref_work);
>> -             ipu_crtc->flip_state = IPU_FLIP_NONE;
>> -     }
>> +     if (crtc->state->event)
>> +             ipu_crtc_handle_pageflip(crtc);
>>
>>       return IRQ_HANDLED;
>>  }
>> @@ -452,8 +315,6 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
>>
>>       ipu_plane_put_resources(ipu_crtc->plane[0]);
>>
>> -     ipu_crtc->flip_queue = create_singlethread_workqueue("ipu-crtc-flip");
>> -
>>       return 0;
>>
>>  err_put_plane_res:
>> @@ -495,7 +356,6 @@ static void ipu_drm_unbind(struct device *dev, struct device *master,
>>
>>       imx_drm_remove_crtc(ipu_crtc->imx_crtc);
>>
>> -     destroy_workqueue(ipu_crtc->flip_queue);
>>       ipu_put_resources(ipu_crtc);
>>  }
>>
>> --
>> 2.7.4
>>
>> _______________________________________________
>> 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

Re: [PATCH 10/14] drm/atomic-helper: Disable planes when suspending

[Ying Liu]

On Tue, May 24, 2016 at 7:00 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, May 24, 2016 at 06:10:49PM +0800, Liu Ying wrote:
>> We should disable planes explicitly when suspending.
>> Especially, this is meaningful for those display controllers which
>> don't support active planes without relevant CRTCs being enabled.
>>
>> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
>
> Recommended way is to call drm_atomic_helper_disable_planes_on_crtc in
> your crtc's ->disable() callback if your hw needs this. This is a general
> problem (test e.g. dpms), not just an issue in suspend code.

It looks legacy fbdev unblank operation fails after call that function
in ->disable(). I made the change on top of this patch set.
If you have any idea, please help out here.

Regards,
Liu Ying

>
> Also unsetting the planes from state has a semantic meaning: It unpins the
> backing storage, which is definitely not what we want for suspend/resume.
> -Daniel
>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 4befe25..5331d95 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1967,7 +1967,7 @@ commit:
>>   *
>>   * Loops through all connectors, finding those that aren't turned off and then
>>   * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
>> - * that they are connected to.
>> + * that they are connected to.  The relevant planes are deactivated as well.
>>   *
>>   * This is used for example in suspend/resume to disable all currently active
>>   * functions when suspending.
>> @@ -1997,6 +1997,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
>>       drm_for_each_connector(conn, dev) {
>>               struct drm_crtc *crtc = conn->state->crtc;
>>               struct drm_crtc_state *crtc_state;
>> +             struct drm_plane *plane;
>>
>>               if (!crtc || conn->dpms != DRM_MODE_DPMS_ON)
>>                       continue;
>> @@ -2008,6 +2009,21 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
>>               }
>>
>>               crtc_state->active = false;
>> +
>> +             drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
>> +                     struct drm_plane_state *plane_state;
>> +
>> +                     plane_state = drm_atomic_get_plane_state(state, plane);
>> +                     if (IS_ERR(plane_state)) {
>> +                             err = PTR_ERR(plane_state);
>> +                             goto free;
>> +                     }
>> +
>> +                     err = drm_atomic_set_crtc_for_plane(plane_state, NULL);
>> +                     if (err != 0)
>> +                             goto free;
>> +                     drm_atomic_set_fb_for_plane(plane_state, NULL);
>> +             }
>>       }
>>
>>       err = drm_atomic_commit(state);
>> --
>> 2.7.4
>>
>> _______________________________________________
>> 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

Re: [PATCH 05/14] drm/crtc_helper: Disable and reenable primary plane in drm_helper_crtc_mode_set

[Ying Liu]

On Tue, May 24, 2016 at 6:57 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, May 24, 2016 at 06:10:44PM +0800, Liu Ying wrote:
>> Since CRTC has already been disabled in crtc_funcs->prepare(), it doesn't hurt
>> to disable the primary plane in drm_helper_crtc_mode_set() before enabling it
>> in drm_helper_crtc_mode_set_base().  This makes those who reject active plane
>> update in plane_funcs->atomic_check() happy.
>
> How/where exactly do you blow up?
>>
>> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
>> ---
>>  drivers/gpu/drm/drm_crtc_helper.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
>> index 79555d2..7fabcd7 100644
>> --- a/drivers/gpu/drm/drm_crtc_helper.c
>> +++ b/drivers/gpu/drm/drm_crtc_helper.c
>> @@ -1013,6 +1013,15 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
>>                       goto out;
>>       }
>>
>> +     /*
>> +      * It doesn't hurt to disable primary plane here since crtc is off
>> +      * and we'll enable it again in drm_helper_crtc_mode_set_base()
>> +      * below soon.
>> +      */
>> +     ret = drm_plane_helper_disable(crtc->primary);
>
> You can't call this helper from crtc helpers. drm_plane_helper_disable
> assumes that a) you've already extracted universal plane support for the
> primary plane by registering a proper drm_plane for it b) that you're
> driver is at least partially using atomic hooks already.
>
> Both assumptions are wrong for all current users of this function.
> drm_helper_crtc_mode_set() is purely a legacy helper for legecy drivers.

Isn't the function a transitional helper, just as the kdoc claims?
It looks all current users are sort of atomic users already.

Regards,
Liu Ying

> -Daniel
>
>> +     if (ret)
>> +             goto out;
>> +
>>       swap(crtc->state, crtc_state);
>>
>>       crtc_funcs->mode_set_nofb(crtc);
>> --
>> 2.7.4
>>
>> _______________________________________________
>> 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

Re: [PATCH 04/14] gpu: ipu-v3: ipu-dmfc: Use static DMFC FIFO allocation mechanism

[Ying Liu]

On Tue, May 24, 2016 at 10:23 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Dienstag, den 24.05.2016, 18:10 +0800 schrieb Liu Ying:
>> For normal video modes such as 1280x720p@60Hz and 1920x1080p@60Hz,
>
> I'm very happy to drop all this code if possible, but the stated reason
> is not good enough. Rather we should look at the worst case and make
> sure that 2 FIFO slots are enough for 180 MPixel/s on i.MX5 and enough
> for 240 MPixel/s on i.MX6.

My stated reason is not accurate, perhaps.  Sorry.
IIRC, when I wrote this patch, I found we use 2 slots for all
video modes with the dynamic allocation mechanism.
That's why the static one comes up to mind.

> According to the current heuristic (total bandwidth = clkrate * 4 pixels
> with 8 slots) that should be true.

I'm not sure what's the best allocation algorithm is.
Hope that the static one can be accepted temporarily and some one
may offer a better solution in future.

Regards,
Liu Ying

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

Re: [PATCH 05/14] drm/crtc_helper: Disable and reenable primary plane in drm_helper_crtc_mode_set

[Daniel Vetter]

On Wed, May 25, 2016 at 05:37:41PM +0800, Ying Liu wrote:
> On Tue, May 24, 2016 at 6:57 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, May 24, 2016 at 06:10:44PM +0800, Liu Ying wrote:
> >> Since CRTC has already been disabled in crtc_funcs->prepare(), it doesn't hurt
> >> to disable the primary plane in drm_helper_crtc_mode_set() before enabling it
> >> in drm_helper_crtc_mode_set_base().  This makes those who reject active plane
> >> update in plane_funcs->atomic_check() happy.
> >
> > How/where exactly do you blow up?
> >>
> >> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> >> ---
> >>  drivers/gpu/drm/drm_crtc_helper.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> >> index 79555d2..7fabcd7 100644
> >> --- a/drivers/gpu/drm/drm_crtc_helper.c
> >> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> >> @@ -1013,6 +1013,15 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
> >>                       goto out;
> >>       }
> >>
> >> +     /*
> >> +      * It doesn't hurt to disable primary plane here since crtc is off
> >> +      * and we'll enable it again in drm_helper_crtc_mode_set_base()
> >> +      * below soon.
> >> +      */
> >> +     ret = drm_plane_helper_disable(crtc->primary);
> >
> > You can't call this helper from crtc helpers. drm_plane_helper_disable
> > assumes that a) you've already extracted universal plane support for the
> > primary plane by registering a proper drm_plane for it b) that you're
> > driver is at least partially using atomic hooks already.
> >
> > Both assumptions are wrong for all current users of this function.
> > drm_helper_crtc_mode_set() is purely a legacy helper for legecy drivers.
> 
> Isn't the function a transitional helper, just as the kdoc claims?
> It looks all current users are sort of atomic users already.

drm_helper_crtc_mode_set is exclusively used by legacy drivers.
drm_plane_helper_disable can be used for transitioning to atomic.

Calling the latter from the former means you break every non-transition
legacy driver.
-Daniel

> 
> Regards,
> Liu Ying
> 
> > -Daniel
> >
> >> +     if (ret)
> >> +             goto out;
> >> +
> >>       swap(crtc->state, crtc_state);
> >>
> >>       crtc_funcs->mode_set_nofb(crtc);
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> 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

-- 
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

Re: [PATCH 10/14] drm/atomic-helper: Disable planes when suspending

[Daniel Vetter]

On Wed, May 25, 2016 at 05:30:05PM +0800, Ying Liu wrote:
> On Tue, May 24, 2016 at 7:00 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, May 24, 2016 at 06:10:49PM +0800, Liu Ying wrote:
> >> We should disable planes explicitly when suspending.
> >> Especially, this is meaningful for those display controllers which
> >> don't support active planes without relevant CRTCs being enabled.
> >>
> >> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> >
> > Recommended way is to call drm_atomic_helper_disable_planes_on_crtc in
> > your crtc's ->disable() callback if your hw needs this. This is a general
> > problem (test e.g. dpms), not just an issue in suspend code.
> 
> It looks legacy fbdev unblank operation fails after call that function
> in ->disable(). I made the change on top of this patch set.
> If you have any idea, please help out here.

Please explain in detail what fails and how, but your patch here is
definitely not the solution. This is something your driver must be able to
handle, and which cannot be handled in all callers of ->atomic_commit.
-Daniel


> 
> Regards,
> Liu Ying
> 
> >
> > Also unsetting the planes from state has a semantic meaning: It unpins the
> > backing storage, which is definitely not what we want for suspend/resume.
> > -Daniel
> >
> >> ---
> >>  drivers/gpu/drm/drm_atomic_helper.c | 18 +++++++++++++++++-
> >>  1 file changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index 4befe25..5331d95 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -1967,7 +1967,7 @@ commit:
> >>   *
> >>   * Loops through all connectors, finding those that aren't turned off and then
> >>   * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
> >> - * that they are connected to.
> >> + * that they are connected to.  The relevant planes are deactivated as well.
> >>   *
> >>   * This is used for example in suspend/resume to disable all currently active
> >>   * functions when suspending.
> >> @@ -1997,6 +1997,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
> >>       drm_for_each_connector(conn, dev) {
> >>               struct drm_crtc *crtc = conn->state->crtc;
> >>               struct drm_crtc_state *crtc_state;
> >> +             struct drm_plane *plane;
> >>
> >>               if (!crtc || conn->dpms != DRM_MODE_DPMS_ON)
> >>                       continue;
> >> @@ -2008,6 +2009,21 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
> >>               }
> >>
> >>               crtc_state->active = false;
> >> +
> >> +             drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
> >> +                     struct drm_plane_state *plane_state;
> >> +
> >> +                     plane_state = drm_atomic_get_plane_state(state, plane);
> >> +                     if (IS_ERR(plane_state)) {
> >> +                             err = PTR_ERR(plane_state);
> >> +                             goto free;
> >> +                     }
> >> +
> >> +                     err = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> >> +                     if (err != 0)
> >> +                             goto free;
> >> +                     drm_atomic_set_fb_for_plane(plane_state, NULL);
> >> +             }
> >>       }
> >>
> >>       err = drm_atomic_commit(state);
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> 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

-- 
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

Re: [PATCH 05/14] drm/crtc_helper: Disable and reenable primary plane in drm_helper_crtc_mode_set

[Ying Liu]

On Wed, May 25, 2016 at 6:30 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, May 25, 2016 at 05:37:41PM +0800, Ying Liu wrote:
>> On Tue, May 24, 2016 at 6:57 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Tue, May 24, 2016 at 06:10:44PM +0800, Liu Ying wrote:
>> >> Since CRTC has already been disabled in crtc_funcs->prepare(), it doesn't hurt
>> >> to disable the primary plane in drm_helper_crtc_mode_set() before enabling it
>> >> in drm_helper_crtc_mode_set_base().  This makes those who reject active plane
>> >> update in plane_funcs->atomic_check() happy.
>> >
>> > How/where exactly do you blow up?
>> >>
>> >> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
>> >> ---
>> >>  drivers/gpu/drm/drm_crtc_helper.c | 9 +++++++++
>> >>  1 file changed, 9 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
>> >> index 79555d2..7fabcd7 100644
>> >> --- a/drivers/gpu/drm/drm_crtc_helper.c
>> >> +++ b/drivers/gpu/drm/drm_crtc_helper.c
>> >> @@ -1013,6 +1013,15 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
>> >>                       goto out;
>> >>       }
>> >>
>> >> +     /*
>> >> +      * It doesn't hurt to disable primary plane here since crtc is off
>> >> +      * and we'll enable it again in drm_helper_crtc_mode_set_base()
>> >> +      * below soon.
>> >> +      */
>> >> +     ret = drm_plane_helper_disable(crtc->primary);
>> >
>> > You can't call this helper from crtc helpers. drm_plane_helper_disable
>> > assumes that a) you've already extracted universal plane support for the
>> > primary plane by registering a proper drm_plane for it b) that you're
>> > driver is at least partially using atomic hooks already.
>> >
>> > Both assumptions are wrong for all current users of this function.
>> > drm_helper_crtc_mode_set() is purely a legacy helper for legecy drivers.
>>
>> Isn't the function a transitional helper, just as the kdoc claims?
>> It looks all current users are sort of atomic users already.
>
> drm_helper_crtc_mode_set is exclusively used by legacy drivers.
> drm_plane_helper_disable can be used for transitioning to atomic.

Again, please take a look at the kdoc of drm_helper_crtc_mode_set.
It reads 'Besides the atomic plane helper functions for the primary
plane the driver must also provide the ->mode_set_nofb callback
to set up the CRTC.  This is a transitional helper useful for converting
drivers to the atomic interfaces.'

>
> Calling the latter from the former means you break every non-transition
> legacy driver.

drm_helper_crtc_mode_set calls drm_helper_crtc_mode_set_base.
Both drm_helper_crtc_mode_set_base and drm_plane_helper_disable
call drm_plane_helper_commit, so they are somewhat counterparts,
that is to say,  drm_helper_crtc_mode_set may call
drm_plane_helper_disable technically.

Regards,
Liu Ying

> -Daniel
>
>>
>> Regards,
>> Liu Ying
>>
>> > -Daniel
>> >
>> >> +     if (ret)
>> >> +             goto out;
>> >> +
>> >>       swap(crtc->state, crtc_state);
>> >>
>> >>       crtc_funcs->mode_set_nofb(crtc);
>> >> --
>> >> 2.7.4
>> >>
>> >> _______________________________________________
>> >> 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
>
> --
> 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

Re: [PATCH 03/14] drm: imx: ipuv3 plane: Check different types of plane separately

[Ying Liu]

On Tue, May 24, 2016 at 10:20 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Dienstag, den 24.05.2016, 18:10 +0800 schrieb Liu Ying:
>> The IPUv3 primary plane doesn't support partial off screen.
>> So, this patch separates plane check logics for primary plane and overlay
>> plane and adds more limitations on the primary plane.
>>
>> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
>> ---
>>  drivers/gpu/drm/imx/ipuv3-plane.c | 72 ++++++++++++++++++++++-----------------
>>  1 file changed, 41 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
>> index e6ec8eb..8f91b2e 100644
>> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
>> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
>> @@ -190,44 +190,54 @@ int ipu_plane_mode_set(struct ipu_plane *ipu_plane, struct drm_crtc *crtc,
>>                      uint32_t src_x, uint32_t src_y,
>>                      uint32_t src_w, uint32_t src_h, bool interlaced)
>>  {
>> -     struct device *dev = ipu_plane->base.dev->dev;
>> +     struct drm_plane plane = ipu_plane->base;
>
> Why make a copy of the drm_plane here?

Will remove that copy.

Regards,
Liu Ying

>
>> +     struct device *dev = plane.dev->dev;
>>       int ret;
>>
>>       /* no scaling */
>>       if (src_w != crtc_w || src_h != crtc_h)
>>               return -EINVAL;
>>
>> -     /* clip to crtc bounds */
>> -     if (crtc_x < 0) {
>> -             if (-crtc_x > crtc_w)
>> +     if (plane.type == DRM_PLANE_TYPE_PRIMARY) {
>> +             /* full plane doesn't support partial off screen */
>> +             if (crtc_x || crtc_y || crtc_w != mode->hdisplay ||
>> +                     crtc_h != mode->vdisplay)
>
> As long as the requested plane is large enough to cover the whole base
> plane, we can fix the crtc_x/y/w/h up by clipping to the base plane
> boundaries. There is no need to return -EINVAL here as long as the IDMAC
> is capable to start reading at src_x/y = -crtc_x/y.
>
> regards
> Philipp
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/14] drm/crtc_helper: Disable and reenable primary plane in drm_helper_crtc_mode_set

[Daniel Vetter]

On Thu, May 26, 2016 at 11:02:55AM +0800, Ying Liu wrote:
> On Wed, May 25, 2016 at 6:30 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, May 25, 2016 at 05:37:41PM +0800, Ying Liu wrote:
> >> On Tue, May 24, 2016 at 6:57 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> > On Tue, May 24, 2016 at 06:10:44PM +0800, Liu Ying wrote:
> >> >> Since CRTC has already been disabled in crtc_funcs->prepare(), it doesn't hurt
> >> >> to disable the primary plane in drm_helper_crtc_mode_set() before enabling it
> >> >> in drm_helper_crtc_mode_set_base().  This makes those who reject active plane
> >> >> update in plane_funcs->atomic_check() happy.
> >> >
> >> > How/where exactly do you blow up?
> >> >>
> >> >> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> >> >> ---
> >> >>  drivers/gpu/drm/drm_crtc_helper.c | 9 +++++++++
> >> >>  1 file changed, 9 insertions(+)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> >> >> index 79555d2..7fabcd7 100644
> >> >> --- a/drivers/gpu/drm/drm_crtc_helper.c
> >> >> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> >> >> @@ -1013,6 +1013,15 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
> >> >>                       goto out;
> >> >>       }
> >> >>
> >> >> +     /*
> >> >> +      * It doesn't hurt to disable primary plane here since crtc is off
> >> >> +      * and we'll enable it again in drm_helper_crtc_mode_set_base()
> >> >> +      * below soon.
> >> >> +      */
> >> >> +     ret = drm_plane_helper_disable(crtc->primary);
> >> >
> >> > You can't call this helper from crtc helpers. drm_plane_helper_disable
> >> > assumes that a) you've already extracted universal plane support for the
> >> > primary plane by registering a proper drm_plane for it b) that you're
> >> > driver is at least partially using atomic hooks already.
> >> >
> >> > Both assumptions are wrong for all current users of this function.
> >> > drm_helper_crtc_mode_set() is purely a legacy helper for legecy drivers.
> >>
> >> Isn't the function a transitional helper, just as the kdoc claims?
> >> It looks all current users are sort of atomic users already.
> >
> > drm_helper_crtc_mode_set is exclusively used by legacy drivers.
> > drm_plane_helper_disable can be used for transitioning to atomic.
> 
> Again, please take a look at the kdoc of drm_helper_crtc_mode_set.
> It reads 'Besides the atomic plane helper functions for the primary
> plane the driver must also provide the ->mode_set_nofb callback
> to set up the CRTC.  This is a transitional helper useful for converting
> drivers to the atomic interfaces.'

Oops, my apologies, I mixed it all up in my head. Sorry for all the mess,
I'll start a new thread since this discussion here is a bit confusion now.
-Daniel


> > Calling the latter from the former means you break every non-transition
> > legacy driver.
> 
> drm_helper_crtc_mode_set calls drm_helper_crtc_mode_set_base.
> Both drm_helper_crtc_mode_set_base and drm_plane_helper_disable
> call drm_plane_helper_commit, so they are somewhat counterparts,
> that is to say,  drm_helper_crtc_mode_set may call
> drm_plane_helper_disable technically.
> 
> Regards,
> Liu Ying
> 
> > -Daniel
> >
> >>
> >> Regards,
> >> Liu Ying
> >>
> >> > -Daniel
> >> >
> >> >> +     if (ret)
> >> >> +             goto out;
> >> >> +
> >> >>       swap(crtc->state, crtc_state);
> >> >>
> >> >>       crtc_funcs->mode_set_nofb(crtc);
> >> >> --
> >> >> 2.7.4
> >> >>
> >> >> _______________________________________________
> >> >> 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
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
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

Re: [PATCH 05/14] drm/crtc_helper: Disable and reenable primary plane in drm_helper_crtc_mode_set

[Daniel Vetter]

On Tue, May 24, 2016 at 06:10:44PM +0800, Liu Ying wrote:
> Since CRTC has already been disabled in crtc_funcs->prepare(), it doesn't hurt
> to disable the primary plane in drm_helper_crtc_mode_set() before enabling it
> in drm_helper_crtc_mode_set_base().  This makes those who reject active plane
> update in plane_funcs->atomic_check() happy.
> 
> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> ---
>  drivers/gpu/drm/drm_crtc_helper.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 79555d2..7fabcd7 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -1013,6 +1013,15 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
>  			goto out;
>  	}
>  
> +	/*
> +	 * It doesn't hurt to disable primary plane here since crtc is off
> +	 * and we'll enable it again in drm_helper_crtc_mode_set_base()
> +	 * below soon.
> +	 */
> +	ret = drm_plane_helper_disable(crtc->primary);
> +	if (ret)
> +		goto out;
> +

Ok, so there's no problem with mixing up legacy and transitional helpers
here. It's still not how it's supposed to work though. All the helpers
(legacy, transitional & atomic) assume that when the helpers call into
crtc->disable hooks, the driver will also disable all the scanout engines.
The helpers will _not_ do that for you, it's the driver duties.

For atomic we have a helper function which you can call at the right
place, drm_atomic_helper_disable_planes_on_crtc(). Unfortunately it looks
like no one ever used it, so probably a bunch of bugs in drivers because
of this. Explicitly disabling the plane when the crtc is shut down might
confused/break drivers which get this right, hence why we can't have this
in the helper libraries.

In short: You need to fix this in your driver, by manually disabling the
plane at a suitable place first. Most likely that's in the crtc->prepare
hook (since you're still transitioning).
-Daniel

>  	swap(crtc->state, crtc_state);
>  
>  	crtc_funcs->mode_set_nofb(crtc);
> -- 
> 2.7.4
> 
> _______________________________________________
> 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