[RFC PATCH 0/9] drm/rockchip: covert to support atomic API

[RFC PATCH 0/9] drm/rockchip: covert to support atomic API

[Mark Yao]

The series of patches coverting drm rockchip to atomic API, do some
cleanup and some fixes on atomic side.

TODO: fence is not support on current version.

Tested on rk3288 popmetal board.

All guys can test it with following url:
 test case: https://github.com/markyzq/libdrm.git atomictest
 kernel: https://github.com/markyzq/kernel-drm-rockchip.git drm-rockchip-2015-11-31

Mark Yao (9):
  drm/rockchip: vop: replace dpms with enable/disable
  drm/rockchip: Use new vblank api drm_crtc_vblank_*
  drm/rockchip: Convert to support atomic API
  drm/rockchip: support atomic asynchronous commit
  drm/rockchip: Optimization vop mode set
  drm/rockchip: direct config connecter gate and out_mode
  drm/rockchip: force enable vop when do mode setting
  drm: bridge/dw_hdmi: Covert to support atomic API
  drm/rockchip: dw_hdmi: use encoder enable function

 drivers/gpu/drm/bridge/dw_hdmi.c            |    6 +-
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |   14 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c |   18 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |    9 +-
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |   99 ++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  735 +++++++++++----------------
 6 files changed, 413 insertions(+), 468 deletions(-)

-- 
1.7.9.5

[RFC PATCH 1/9] drm/rockchip: vop: replace dpms with enable/disable

[Mark Yao]

For vop, power by enable/disable is more suitable then legacy dpms
function, and enable/disable more closely to the new atomic API.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |   37 +++------------------------
 1 file changed, 4 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 5f79d06..41905e2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -633,7 +633,7 @@ static void vop_dsp_hold_valid_irq_disable(struct vop *vop)
 	spin_unlock_irqrestore(&vop->irq_lock, flags);
 }
 
-static void vop_enable(struct drm_crtc *crtc)
+static void vop_crtc_enable(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
 	int ret;
@@ -703,7 +703,7 @@ err_disable_hclk:
 	clk_disable(vop->hclk);
 }
 
-static void vop_disable(struct drm_crtc *crtc)
+static void vop_crtc_disable(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
 
@@ -1108,30 +1108,6 @@ static const struct rockchip_crtc_funcs private_crtc_funcs = {
 	.disable_vblank = vop_crtc_disable_vblank,
 };
 
-static void vop_crtc_dpms(struct drm_crtc *crtc, int mode)
-{
-	DRM_DEBUG_KMS("crtc[%d] mode[%d]\n", crtc->base.id, mode);
-
-	switch (mode) {
-	case DRM_MODE_DPMS_ON:
-		vop_enable(crtc);
-		break;
-	case DRM_MODE_DPMS_STANDBY:
-	case DRM_MODE_DPMS_SUSPEND:
-	case DRM_MODE_DPMS_OFF:
-		vop_disable(crtc);
-		break;
-	default:
-		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
-		break;
-	}
-}
-
-static void vop_crtc_prepare(struct drm_crtc *crtc)
-{
-	vop_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
-}
-
 static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
 				const struct drm_display_mode *mode,
 				struct drm_display_mode *adjusted_mode)
@@ -1242,17 +1218,12 @@ out:
 	return ret;
 }
 
-static void vop_crtc_commit(struct drm_crtc *crtc)
-{
-}
-
 static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
-	.dpms = vop_crtc_dpms,
-	.prepare = vop_crtc_prepare,
+	.enable = vop_crtc_enable,
+	.disable = vop_crtc_disable,
 	.mode_fixup = vop_crtc_mode_fixup,
 	.mode_set = vop_crtc_mode_set,
 	.mode_set_base = vop_crtc_mode_set_base,
-	.commit = vop_crtc_commit,
 };
 
 static int vop_crtc_page_flip(struct drm_crtc *crtc,
-- 
1.7.9.5

[RFC PATCH 2/9] drm/rockchip: Use new vblank api drm_crtc_vblank_*

[Mark Yao]

No functional update, drm_vblank_* is the legacy version of
drm_crtc_vblank_*. and use new api make driver more clean.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c |   13 +++++++------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |    7 +++----
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |   24 +++++++++++-------------
 3 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 574324e..ccd46f2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -65,11 +65,11 @@ void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
 }
 EXPORT_SYMBOL_GPL(rockchip_drm_dma_detach_device);
 
-int rockchip_register_crtc_funcs(struct drm_device *dev,
-				 const struct rockchip_crtc_funcs *crtc_funcs,
-				 int pipe)
+int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
+				 const struct rockchip_crtc_funcs *crtc_funcs)
 {
-	struct rockchip_drm_private *priv = dev->dev_private;
+	int pipe = drm_crtc_index(crtc);
+	struct rockchip_drm_private *priv = crtc->dev->dev_private;
 
 	if (pipe > ROCKCHIP_MAX_CRTC)
 		return -EINVAL;
@@ -80,9 +80,10 @@ int rockchip_register_crtc_funcs(struct drm_device *dev,
 }
 EXPORT_SYMBOL_GPL(rockchip_register_crtc_funcs);
 
-void rockchip_unregister_crtc_funcs(struct drm_device *dev, int pipe)
+void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc)
 {
-	struct rockchip_drm_private *priv = dev->dev_private;
+	int pipe = drm_crtc_index(crtc);
+	struct rockchip_drm_private *priv = crtc->dev->dev_private;
 
 	if (pipe > ROCKCHIP_MAX_CRTC)
 		return;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index dc4e5f0..069d6d4 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -52,10 +52,9 @@ struct rockchip_drm_private {
 	const struct rockchip_crtc_funcs *crtc_funcs[ROCKCHIP_MAX_CRTC];
 };
 
-int rockchip_register_crtc_funcs(struct drm_device *dev,
-				 const struct rockchip_crtc_funcs *crtc_funcs,
-				 int pipe);
-void rockchip_unregister_crtc_funcs(struct drm_device *dev, int pipe);
+int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
+				 const struct rockchip_crtc_funcs *crtc_funcs);
+void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc);
 int rockchip_drm_encoder_get_mux_id(struct device_node *node,
 				    struct drm_encoder *encoder);
 int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, int connector_type,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 41905e2..3d16e70 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -119,8 +119,6 @@ struct vop {
 	/* vop dclk reset */
 	struct reset_control *dclk_rst;
 
-	int pipe;
-
 	struct vop_win win[];
 };
 
@@ -691,7 +689,7 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
 
 	enable_irq(vop->irq);
 
-	drm_vblank_on(vop->drm_dev, vop->pipe);
+	drm_crtc_vblank_on(crtc);
 
 	return;
 
@@ -710,7 +708,7 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
 	if (!vop->is_enabled)
 		return;
 
-	drm_vblank_off(crtc->dev, vop->pipe);
+	drm_crtc_vblank_off(crtc);
 
 	/*
 	 * Vop standby will take effect at end of current frame,
@@ -917,7 +915,7 @@ static int vop_update_plane_event(struct drm_plane *plane,
 	 */
 	mutex_lock(&vop->vsync_mutex);
 	if (fb != vop_win_last_pending_fb(vop_win)) {
-		ret = drm_vblank_get(plane->dev, vop->pipe);
+		ret = drm_crtc_vblank_get(crtc);
 		if (ret) {
 			DRM_ERROR("failed to get vblank, %d\n", ret);
 			mutex_unlock(&vop->vsync_mutex);
@@ -928,7 +926,7 @@ static int vop_update_plane_event(struct drm_plane *plane,
 
 		ret = vop_win_queue_fb(vop_win, fb, yrgb_mst, event);
 		if (ret) {
-			drm_vblank_put(plane->dev, vop->pipe);
+			drm_crtc_vblank_put(crtc);
 			mutex_unlock(&vop->vsync_mutex);
 			return ret;
 		}
@@ -1022,7 +1020,7 @@ static int vop_disable_plane(struct drm_plane *plane)
 
 	vop = to_vop(plane->crtc);
 
-	ret = drm_vblank_get(plane->dev, vop->pipe);
+	ret = drm_crtc_vblank_get(plane->crtc);
 	if (ret) {
 		DRM_ERROR("failed to get vblank, %d\n", ret);
 		return ret;
@@ -1032,7 +1030,7 @@ static int vop_disable_plane(struct drm_plane *plane)
 
 	ret = vop_win_queue_fb(vop_win, NULL, 0, NULL);
 	if (ret) {
-		drm_vblank_put(plane->dev, vop->pipe);
+		drm_crtc_vblank_put(plane->crtc);
 		mutex_unlock(&vop->vsync_mutex);
 		return ret;
 	}
@@ -1265,7 +1263,7 @@ static void vop_win_state_complete(struct vop_win *vop_win,
 	}
 
 	list_del(&state->head);
-	drm_vblank_put(crtc->dev, vop->pipe);
+	drm_crtc_vblank_put(crtc);
 }
 
 static void vop_crtc_destroy(struct drm_crtc *crtc)
@@ -1380,6 +1378,7 @@ done:
 static irqreturn_t vop_isr(int irq, void *data)
 {
 	struct vop *vop = data;
+	struct drm_crtc *crtc = &vop->crtc;
 	uint32_t intr0_reg, active_irqs;
 	unsigned long flags;
 	int ret = IRQ_NONE;
@@ -1408,7 +1407,7 @@ static irqreturn_t vop_isr(int irq, void *data)
 	}
 
 	if (active_irqs & FS_INTR) {
-		drm_handle_vblank(vop->drm_dev, vop->pipe);
+		drm_crtc_handle_vblank(crtc);
 		active_irqs &= ~FS_INTR;
 		ret = (vop->vsync_work_pending) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
 	}
@@ -1501,8 +1500,7 @@ static int vop_create_crtc(struct vop *vop)
 
 	init_completion(&vop->dsp_hold_completion);
 	crtc->port = port;
-	vop->pipe = drm_crtc_index(crtc);
-	rockchip_register_crtc_funcs(drm_dev, &private_crtc_funcs, vop->pipe);
+	rockchip_register_crtc_funcs(crtc, &private_crtc_funcs);
 
 	return 0;
 
@@ -1518,7 +1516,7 @@ static void vop_destroy_crtc(struct vop *vop)
 {
 	struct drm_crtc *crtc = &vop->crtc;
 
-	rockchip_unregister_crtc_funcs(vop->drm_dev, vop->pipe);
+	rockchip_unregister_crtc_funcs(crtc);
 	of_node_put(crtc->port);
 	drm_crtc_cleanup(crtc);
 }
-- 
1.7.9.5

[RFC PATCH 2/9] drm/rockchip: Use new vblank api drm_crtc_vblank_*

[Daniel Stone]

Hi,

On 1 December 2015 at 03:26, Mark Yao <mark.yao@rock-chips.com> wrote:
> No functional update, drm_vblank_* is the legacy version of
> drm_crtc_vblank_*. and use new api make driver more clean.
>
> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>

Heh, I had the same patch in my series to fix pageflip events.

Reviewed-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel

[RFC PATCH 2/9] drm/rockchip: Use new vblank api drm_crtc_vblank_*

[Mark yao]

On 2015?12?01? 15:56, Daniel Stone wrote:
> Hi,
>
> On 1 December 2015 at 03:26, Mark Yao <mark.yao@rock-chips.com> wrote:
>> No functional update, drm_vblank_* is the legacy version of
>> drm_crtc_vblank_*. and use new api make driver more clean.
>>
>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> Heh, I had the same patch in my series to fix pageflip events.
>
> Reviewed-by: Daniel Stone <daniels@collabora.com>
>
> Cheers,
> Daniel
>
>
>

Hi Daniel
    I had picked your patch "[PATCH 1/2] drm/rockchip: Use CRTC vblank 
event interface" into my drm-next, this patch is base on it.

Thanks for your review.

-- 
?ark Yao

[RFC PATCH 2/9] drm/rockchip: Use new vblank api drm_crtc_vblank_*

[Daniel Vetter]

On Tue, Dec 01, 2015 at 04:33:27PM +0800, Mark yao wrote:
> On 2015?12?01? 15:56, Daniel Stone wrote:
> >Hi,
> >
> >On 1 December 2015 at 03:26, Mark Yao <mark.yao@rock-chips.com> wrote:
> >>No functional update, drm_vblank_* is the legacy version of
> >>drm_crtc_vblank_*. and use new api make driver more clean.
> >>
> >>Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> >Heh, I had the same patch in my series to fix pageflip events.
> >
> >Reviewed-by: Daniel Stone <daniels@collabora.com>
> >
> >Cheers,
> >Daniel
> >
> >
> >
> 
> Hi Daniel
>    I had picked your patch "[PATCH 1/2] drm/rockchip: Use CRTC vblank event
> interface" into my drm-next, this patch is base on it.

That really should be mentioned in the commit message, and you must keep
the signed-off-by chain intact when adapting or reusing other peoples
work.
-Daniel

> 
> Thanks for your review.
> 
> -- 
> ?ark Yao
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[RFC PATCH 2/9] drm/rockchip: Use new vblank api drm_crtc_vblank_*

[Mark yao]

On 2015?12?01? 17:01, Daniel Vetter wrote:
> On Tue, Dec 01, 2015 at 04:33:27PM +0800, Mark yao wrote:
>> On 2015?12?01? 15:56, Daniel Stone wrote:
>>> Hi,
>>>
>>> On 1 December 2015 at 03:26, Mark Yao <mark.yao@rock-chips.com> wrote:
>>>> No functional update, drm_vblank_* is the legacy version of
>>>> drm_crtc_vblank_*. and use new api make driver more clean.
>>>>
>>>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>>> Heh, I had the same patch in my series to fix pageflip events.
>>>
>>> Reviewed-by: Daniel Stone <daniels@collabora.com>
>>>
>>> Cheers,
>>> Daniel
>>>
>>>
>>>
>> Hi Daniel
>>     I had picked your patch "[PATCH 1/2] drm/rockchip: Use CRTC vblank event
>> interface" into my drm-next, this patch is base on it.
> That really should be mentioned in the commit message, and you must keep
> the signed-off-by chain intact when adapting or reusing other peoples
> work.
> -Daniel

Oh, Sorry for that, this patch is another patch rebase on Daniel Stone's 
one, I use "base on it" may be ambiguity.
like that:
0e6919f drm/rockchip: Use new vblank api drm_crtc_vblank_*
4f0cb7c drm/rockchip: Use CRTC vblank event interface

Thanks for pointing that, I will be carefully next time.

>> Thanks for your review.
>>
>> -- 
>> ?ark Yao
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 3/9] drm/rockchip: Convert to support atomic API

[Mark Yao]

Rockchip vop not support hw vblank counter, needed check the committed
register if it's really take effect.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c |    5 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |    2 +
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |   65 +++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  603 ++++++++++-----------------
 4 files changed, 301 insertions(+), 374 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index ccd46f2..5de51fd 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -214,6 +214,8 @@ static int rockchip_drm_load(struct drm_device *drm_dev, unsigned long flags)
 	 */
 	drm_dev->vblank_disable_allowed = true;
 
+	drm_mode_config_reset(drm_dev);
+
 	ret = rockchip_drm_fbdev_init(drm_dev);
 	if (ret)
 		goto err_vblank_cleanup;
@@ -277,7 +279,8 @@ const struct vm_operations_struct rockchip_drm_vm_ops = {
 };
 
 static struct drm_driver rockchip_drm_driver = {
-	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME,
+	.driver_features	= DRIVER_MODESET | DRIVER_GEM |
+				  DRIVER_PRIME | DRIVER_ATOMIC,
 	.load			= rockchip_drm_load,
 	.unload			= rockchip_drm_unload,
 	.lastclose		= rockchip_drm_lastclose,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 069d6d4..513ff98 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -18,6 +18,7 @@
 #define _ROCKCHIP_DRM_DRV_H
 
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem.h>
 
 #include <linux/module.h>
@@ -63,5 +64,6 @@ int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
 				   struct device *dev);
 void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
 				    struct device *dev);
+void rockchip_crtc_wait_for_update(struct drm_crtc *crtc);
 
 #endif /* _ROCKCHIP_DRM_DRV_H_ */
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 002645b..c86d93a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #include <drm/drm.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_crtc_helper.h>
 
@@ -166,9 +167,73 @@ static void rockchip_drm_output_poll_changed(struct drm_device *dev)
 		drm_fb_helper_hotplug_event(fb_helper);
 }
 
+static void rockchip_atomic_wait_for_complete(struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int i;
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (!crtc->state->active)
+			continue;
+
+		WARN_ON(drm_crtc_vblank_get(crtc));
+	}
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (!crtc->state->active)
+			continue;
+
+		rockchip_crtc_wait_for_update(crtc);
+	}
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (!crtc->state->active)
+			continue;
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+int rockchip_drm_atomic_commit(struct drm_device *dev,
+			       struct drm_atomic_state *state,
+			       bool async)
+{
+	int ret;
+
+	if (async)
+		return -EBUSY;
+
+	ret = drm_atomic_helper_prepare_planes(dev, state);
+	if (ret)
+		return ret;
+
+	drm_atomic_helper_swap_state(dev, state);
+
+	/*
+	 * TODO: do fence wait here.
+	 */
+
+	drm_atomic_helper_commit_modeset_disables(dev, state);
+
+	drm_atomic_helper_commit_planes(dev, state, false);
+
+	drm_atomic_helper_commit_modeset_enables(dev, state);
+
+	rockchip_atomic_wait_for_complete(state);
+
+	drm_atomic_helper_cleanup_planes(dev, state);
+
+	drm_atomic_state_free(state);
+
+	return 0;
+}
+
 static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
 	.fb_create = rockchip_user_fb_create,
 	.output_poll_changed = rockchip_drm_output_poll_changed,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = rockchip_drm_atomic_commit,
 };
 
 struct drm_framebuffer *
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 3d16e70..a28e255 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -14,6 +14,7 @@
 
 #include <drm/drm.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_plane_helper.h>
@@ -63,12 +64,16 @@
 
 #define to_vop(x) container_of(x, struct vop, crtc)
 #define to_vop_win(x) container_of(x, struct vop_win, base)
-
-struct vop_win_state {
-	struct list_head head;
-	struct drm_framebuffer *fb;
-	dma_addr_t yrgb_mst;
-	struct drm_pending_vblank_event *event;
+#define to_vop_plane_state(x) container_of(x, struct vop_plane_state, base)
+
+struct vop_plane_state {
+	struct drm_plane_state base;
+	dma_addr_t dma_addr[ROCKCHIP_MAX_FB_BUFFER];
+	uint32_t vir_stride[ROCKCHIP_MAX_FB_BUFFER];
+	int format;
+	struct drm_rect src;
+	struct drm_rect dest;
+	bool enable;
 };
 
 struct vop_win {
@@ -76,8 +81,7 @@ struct vop_win {
 	const struct vop_win_data *data;
 	struct vop *vop;
 
-	struct list_head pending;
-	struct vop_win_state *active;
+	struct vop_plane_state state;
 };
 
 struct vop {
@@ -93,6 +97,7 @@ struct vop {
 	struct mutex vsync_mutex;
 	bool vsync_work_pending;
 	struct completion dsp_hold_completion;
+	struct completion wait_update_complete;
 
 	const struct vop_data *data;
 
@@ -745,148 +750,99 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
 	pm_runtime_put(vop->dev);
 }
 
-/*
- * Caller must hold vsync_mutex.
- */
-static struct drm_framebuffer *vop_win_last_pending_fb(struct vop_win *vop_win)
-{
-	struct vop_win_state *last;
-	struct vop_win_state *active = vop_win->active;
-
-	if (list_empty(&vop_win->pending))
-		return active ? active->fb : NULL;
-
-	last = list_last_entry(&vop_win->pending, struct vop_win_state, head);
-	return last ? last->fb : NULL;
-}
-
-/*
- * Caller must hold vsync_mutex.
- */
-static int vop_win_queue_fb(struct vop_win *vop_win,
-			    struct drm_framebuffer *fb, dma_addr_t yrgb_mst,
-			    struct drm_pending_vblank_event *event)
+static void vop_plane_destroy(struct drm_plane *plane)
 {
-	struct vop_win_state *state;
-
-	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (!state)
-		return -ENOMEM;
-
-	state->fb = fb;
-	state->yrgb_mst = yrgb_mst;
-	state->event = event;
-
-	list_add_tail(&state->head, &vop_win->pending);
-
-	return 0;
+	drm_plane_cleanup(plane);
 }
 
-static int vop_update_plane_event(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 drm_pending_vblank_event *event)
+static int vop_plane_atomic_check(struct drm_plane *plane,
+			   struct drm_plane_state *state)
 {
+	struct drm_crtc *crtc = state->crtc;
+	struct drm_framebuffer *fb = state->fb;
 	struct vop_win *vop_win = to_vop_win(plane);
+	struct vop_plane_state *vop_plane_state = to_vop_plane_state(state);
 	const struct vop_win_data *win = vop_win->data;
-	struct vop *vop = to_vop(crtc);
 	struct drm_gem_object *obj;
 	struct rockchip_gem_object *rk_obj;
-	struct drm_gem_object *uv_obj;
-	struct rockchip_gem_object *rk_uv_obj;
 	unsigned long offset;
-	unsigned int actual_w;
-	unsigned int actual_h;
-	unsigned int dsp_stx;
-	unsigned int dsp_sty;
-	unsigned int y_vir_stride;
-	unsigned int uv_vir_stride = 0;
-	dma_addr_t yrgb_mst;
-	dma_addr_t uv_mst = 0;
-	enum vop_data_format format;
-	uint32_t val;
-	bool is_alpha;
-	bool rb_swap;
 	bool is_yuv;
 	bool visible;
 	int ret;
-	struct drm_rect dest = {
-		.x1 = crtc_x,
-		.y1 = crtc_y,
-		.x2 = crtc_x + crtc_w,
-		.y2 = crtc_y + crtc_h,
-	};
-	struct drm_rect src = {
-		/* 16.16 fixed point */
-		.x1 = src_x,
-		.y1 = src_y,
-		.x2 = src_x + src_w,
-		.y2 = src_y + src_h,
-	};
-	const struct drm_rect clip = {
-		.x2 = crtc->mode.hdisplay,
-		.y2 = crtc->mode.vdisplay,
-	};
-	bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY;
+	struct drm_rect *dest = &vop_plane_state->dest;
+	struct drm_rect *src = &vop_plane_state->src;
+	struct drm_rect clip;
 	int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
 					DRM_PLANE_HELPER_NO_SCALING;
 	int max_scale = win->phy->scl ? FRAC_16_16(8, 1) :
 					DRM_PLANE_HELPER_NO_SCALING;
 
-	ret = drm_plane_helper_check_update(plane, crtc, fb,
-					    &src, &dest, &clip,
+	crtc = crtc ? crtc : plane->state->crtc;
+	/*
+	 * Both crtc or plane->state->crtc can be null.
+	 */
+	if (!crtc || !fb)
+		goto out_disable;
+	src->x1 = state->src_x;
+	src->y1 = state->src_y;
+	src->x2 = state->src_x + state->src_w;
+	src->y2 = state->src_y + state->src_h;
+	dest->x1 = state->crtc_x;
+	dest->y1 = state->crtc_y;
+	dest->x2 = state->crtc_x + state->crtc_w;
+	dest->y2 = state->crtc_y + state->crtc_h;
+
+	clip.x1 = 0;
+	clip.y1 = 0;
+	clip.x2 = crtc->mode.hdisplay;
+	clip.y2 = crtc->mode.vdisplay;
+
+	ret = drm_plane_helper_check_update(plane, crtc, state->fb,
+					    src, dest, &clip,
 					    min_scale,
 					    max_scale,
-					    can_position, false, &visible);
+					    true, true, &visible);
 	if (ret)
 		return ret;
 
 	if (!visible)
-		return 0;
-
-	is_alpha = is_alpha_support(fb->pixel_format);
-	rb_swap = has_rb_swapped(fb->pixel_format);
-	is_yuv = is_yuv_support(fb->pixel_format);
+		goto out_disable;
 
-	format = vop_convert_format(fb->pixel_format);
-	if (format < 0)
-		return format;
+	vop_plane_state->format = vop_convert_format(fb->pixel_format);
+	if (vop_plane_state->format < 0)
+		return vop_plane_state->format;
 
-	obj = rockchip_fb_get_gem_obj(fb, 0);
-	if (!obj) {
-		DRM_ERROR("fail to get rockchip gem object from framebuffer\n");
-		return -EINVAL;
-	}
-
-	rk_obj = to_rockchip_obj(obj);
+	is_yuv = is_yuv_support(fb->pixel_format);
 
 	if (is_yuv) {
 		/*
 		 * Src.x1 can be odd when do clip, but yuv plane start point
 		 * need align with 2 pixel.
 		 */
-		val = (src.x1 >> 16) % 2;
-		src.x1 += val << 16;
-		src.x2 += val << 16;
+		uint32_t temp = (src->x1 >> 16) % 2;
+
+		src->x1 += temp << 16;
+		src->x2 += temp << 16;
 	}
 
-	actual_w = (src.x2 - src.x1) >> 16;
-	actual_h = (src.y2 - src.y1) >> 16;
+	offset = (src->x1 >> 16) * drm_format_plane_cpp(fb->pixel_format, 0);
+	offset += (src->y1 >> 16) * fb->pitches[0];
 
-	dsp_stx = dest.x1 + crtc->mode.htotal - crtc->mode.hsync_start;
-	dsp_sty = dest.y1 + crtc->mode.vtotal - crtc->mode.vsync_start;
+	obj = rockchip_fb_get_gem_obj(fb, 0);
+	if (!obj) {
+		DRM_ERROR("fail to get rockchip gem object from framebuffer\n");
+		return -EINVAL;
+	}
 
-	offset = (src.x1 >> 16) * drm_format_plane_cpp(fb->pixel_format, 0);
-	offset += (src.y1 >> 16) * fb->pitches[0];
+	rk_obj = to_rockchip_obj(obj);
 
-	yrgb_mst = rk_obj->dma_addr + offset + fb->offsets[0];
-	y_vir_stride = fb->pitches[0] >> 2;
+	vop_plane_state->dma_addr[0] = rk_obj->dma_addr + offset +
+					fb->offsets[0];
+	vop_plane_state->vir_stride[0] = fb->pitches[0] >> 2;
 
 	if (is_yuv) {
+		struct drm_gem_object *uv_obj;
+		struct rockchip_gem_object *rk_uv_obj;
 		int hsub = drm_format_horz_chroma_subsampling(fb->pixel_format);
 		int vsub = drm_format_vert_chroma_subsampling(fb->pixel_format);
 		int bpp = drm_format_plane_cpp(fb->pixel_format, 1);
@@ -897,72 +853,89 @@ static int vop_update_plane_event(struct drm_plane *plane,
 			return -EINVAL;
 		}
 		rk_uv_obj = to_rockchip_obj(uv_obj);
-		uv_vir_stride = fb->pitches[1] >> 2;
+		vop_plane_state->vir_stride[1] = fb->pitches[1] >> 2;
 
-		offset = (src.x1 >> 16) * bpp / hsub;
-		offset += (src.y1 >> 16) * fb->pitches[1] / vsub;
+		offset = (src->x1 >> 16) * bpp / hsub;
+		offset += (src->y1 >> 16) * fb->pitches[1] / vsub;
 
-		uv_mst = rk_uv_obj->dma_addr + offset + fb->offsets[1];
+		vop_plane_state->dma_addr[1] = rk_uv_obj->dma_addr + offset +
+						fb->offsets[1];
 	}
+	vop_plane_state->enable = true;
+
+	return 0;
+
+out_disable:
+	vop_plane_state->enable = false;
+	return 0;
+}
+
+static void vop_plane_atomic_update(struct drm_plane *plane,
+		struct drm_plane_state *old_state)
+{
+	struct drm_plane_state *state = plane->state;
+	struct drm_crtc *crtc = state->crtc;
+	struct vop_win *vop_win = to_vop_win(plane);
+	struct vop_plane_state *vop_plane_state = to_vop_plane_state(state);
+	const struct vop_win_data *win = vop_win->data;
+	struct vop *vop = to_vop(state->crtc);
+	struct drm_framebuffer *fb = state->fb;
+	unsigned int actual_w, actual_h;
+	unsigned int dsp_stx, dsp_sty;
+	uint32_t act_info, dsp_info, dsp_st;
+	struct drm_rect *src = &vop_plane_state->src;
+	struct drm_rect *dest = &vop_plane_state->dest;
+	uint32_t val;
+	bool rb_swap;
 
 	/*
-	 * If this plane update changes the plane's framebuffer, (or more
-	 * precisely, if this update has a different framebuffer than the last
-	 * update), enqueue it so we can track when it completes.
-	 *
-	 * Only when we discover that this update has completed, can we
-	 * unreference any previous framebuffers.
+	 * can't update plane when vop is disabled.
 	 */
-	mutex_lock(&vop->vsync_mutex);
-	if (fb != vop_win_last_pending_fb(vop_win)) {
-		ret = drm_crtc_vblank_get(crtc);
-		if (ret) {
-			DRM_ERROR("failed to get vblank, %d\n", ret);
-			mutex_unlock(&vop->vsync_mutex);
-			return ret;
-		}
+	if (!crtc || !vop->is_enabled)
+		return;
 
-		drm_framebuffer_reference(fb);
+	if (!vop_plane_state->enable) {
+		spin_lock(&vop->reg_lock);
 
-		ret = vop_win_queue_fb(vop_win, fb, yrgb_mst, event);
-		if (ret) {
-			drm_crtc_vblank_put(crtc);
-			mutex_unlock(&vop->vsync_mutex);
-			return ret;
-		}
+		VOP_WIN_SET(vop, win, enable, 0);
 
-		vop->vsync_work_pending = true;
+		spin_unlock(&vop->reg_lock);
+		return;
 	}
-	mutex_unlock(&vop->vsync_mutex);
+	actual_w = drm_rect_width(src) >> 16;
+	actual_h = drm_rect_height(src) >> 16;
+	act_info = (actual_h - 1) << 16 | ((actual_w - 1) & 0xffff);
+
+	dsp_info = (drm_rect_height(dest) - 1) << 16;
+	dsp_info |= (drm_rect_width(dest) - 1) & 0xffff;
+
+	dsp_stx = dest->x1 + crtc->mode.htotal - crtc->mode.hsync_start;
+	dsp_sty = dest->y1 + crtc->mode.vtotal - crtc->mode.vsync_start;
+	dsp_st = dsp_sty << 16 | (dsp_stx & 0xffff);
 
 	spin_lock(&vop->reg_lock);
 
-	VOP_WIN_SET(vop, win, format, format);
-	VOP_WIN_SET(vop, win, yrgb_vir, y_vir_stride);
-	VOP_WIN_SET(vop, win, yrgb_mst, yrgb_mst);
-	if (is_yuv) {
-		VOP_WIN_SET(vop, win, uv_vir, uv_vir_stride);
-		VOP_WIN_SET(vop, win, uv_mst, uv_mst);
+	VOP_WIN_SET(vop, win, format, vop_plane_state->format);
+	VOP_WIN_SET(vop, win, yrgb_vir, vop_plane_state->vir_stride[0]);
+	VOP_WIN_SET(vop, win, yrgb_mst, vop_plane_state->dma_addr[0]);
+	if (is_yuv_support(fb->pixel_format)) {
+		VOP_WIN_SET(vop, win, uv_vir, vop_plane_state->vir_stride[1]);
+		VOP_WIN_SET(vop, win, uv_mst, vop_plane_state->dma_addr[1]);
 	}
 
 	if (win->phy->scl)
 		scl_vop_cal_scl_fac(vop, win, actual_w, actual_h,
-				    dest.x2 - dest.x1, dest.y2 - dest.y1,
+				    drm_rect_width(dest), drm_rect_height(dest),
 				    fb->pixel_format);
 
-	val = (actual_h - 1) << 16;
-	val |= (actual_w - 1) & 0xffff;
-	VOP_WIN_SET(vop, win, act_info, val);
+	VOP_WIN_SET(vop, win, act_info, act_info);
+	VOP_WIN_SET(vop, win, dsp_info, dsp_info);
+	VOP_WIN_SET(vop, win, dsp_st, dsp_st);
 
-	val = (dest.y2 - dest.y1 - 1) << 16;
-	val |= (dest.x2 - dest.x1 - 1) & 0xffff;
-	VOP_WIN_SET(vop, win, dsp_info, val);
-	val = dsp_sty << 16;
-	val |= dsp_stx & 0xffff;
-	VOP_WIN_SET(vop, win, dsp_st, val);
+	rb_swap = has_rb_swapped(fb->pixel_format);
 	VOP_WIN_SET(vop, win, rb_swap, rb_swap);
 
-	if (is_alpha) {
+	if (is_alpha_support(fb->pixel_format)) {
 		VOP_WIN_SET(vop, win, dst_alpha_ctl,
 			    DST_FACTOR_M0(ALPHA_SRC_INVERSE));
 		val = SRC_ALPHA_EN(1) | SRC_COLOR_M0(ALPHA_SRC_PRE_MUL) |
@@ -976,86 +949,78 @@ static int vop_update_plane_event(struct drm_plane *plane,
 	}
 
 	VOP_WIN_SET(vop, win, enable, 1);
-
-	vop_cfg_done(vop);
 	spin_unlock(&vop->reg_lock);
-
-	return 0;
 }
 
-static int vop_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)
-{
-	return vop_update_plane_event(plane, crtc, fb, crtc_x, crtc_y, crtc_w,
-				      crtc_h, src_x, src_y, src_w, src_h,
-				      NULL);
-}
+static const struct drm_plane_helper_funcs plane_helper_funcs = {
+	.atomic_check = vop_plane_atomic_check,
+	.atomic_update = vop_plane_atomic_update,
+};
 
-static int vop_update_primary_plane(struct drm_crtc *crtc,
-				    struct drm_pending_vblank_event *event)
+void rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 {
-	unsigned int crtc_w, crtc_h;
-
-	crtc_w = crtc->primary->fb->width - crtc->x;
-	crtc_h = crtc->primary->fb->height - crtc->y;
+	struct vop *vop = to_vop(crtc);
 
-	return vop_update_plane_event(crtc->primary, crtc, crtc->primary->fb,
-				      0, 0, crtc_w, crtc_h, crtc->x << 16,
-				      crtc->y << 16, crtc_w << 16,
-				      crtc_h << 16, event);
+	reinit_completion(&vop->wait_update_complete);
+	WARN_ON(!wait_for_completion_timeout(&vop->wait_update_complete, 100));
 }
 
-static int vop_disable_plane(struct drm_plane *plane)
+void vop_atomic_plane_reset(struct drm_plane *plane)
 {
-	struct vop_win *vop_win = to_vop_win(plane);
-	const struct vop_win_data *win = vop_win->data;
-	struct vop *vop;
-	int ret;
+	struct vop_plane_state *vop_plane_state =
+					to_vop_plane_state(plane->state);
 
-	if (!plane->crtc)
-		return 0;
+	if (plane->state && plane->state->fb)
+		drm_framebuffer_unreference(plane->state->fb);
 
-	vop = to_vop(plane->crtc);
+	kfree(vop_plane_state);
+	vop_plane_state = kzalloc(sizeof(*vop_plane_state), GFP_KERNEL);
+	if (!vop_plane_state)
+		return;
 
-	ret = drm_crtc_vblank_get(plane->crtc);
-	if (ret) {
-		DRM_ERROR("failed to get vblank, %d\n", ret);
-		return ret;
-	}
+	plane->state = &vop_plane_state->base;
+	plane->state->plane = plane;
+}
 
-	mutex_lock(&vop->vsync_mutex);
+struct drm_plane_state *
+vop_atomic_plane_duplicate_state(struct drm_plane *plane)
+{
+	struct vop_plane_state *old_vop_plane_state;
+	struct vop_plane_state *vop_plane_state;
 
-	ret = vop_win_queue_fb(vop_win, NULL, 0, NULL);
-	if (ret) {
-		drm_crtc_vblank_put(plane->crtc);
-		mutex_unlock(&vop->vsync_mutex);
-		return ret;
-	}
+	if (WARN_ON(!plane->state))
+		return NULL;
 
-	vop->vsync_work_pending = true;
-	mutex_unlock(&vop->vsync_mutex);
+	old_vop_plane_state = to_vop_plane_state(plane->state);
+	vop_plane_state = kmemdup(old_vop_plane_state,
+				  sizeof(*vop_plane_state), GFP_KERNEL);
+	if (!vop_plane_state)
+		return NULL;
 
-	spin_lock(&vop->reg_lock);
-	VOP_WIN_SET(vop, win, enable, 0);
-	vop_cfg_done(vop);
-	spin_unlock(&vop->reg_lock);
+	if (vop_plane_state->base.fb)
+		drm_framebuffer_reference(vop_plane_state->base.fb);
 
-	return 0;
+	return &vop_plane_state->base;
 }
 
-static void vop_plane_destroy(struct drm_plane *plane)
+static void vop_atomic_plane_destroy_state(struct drm_plane *plane,
+					   struct drm_plane_state *state)
 {
-	vop_disable_plane(plane);
-	drm_plane_cleanup(plane);
+	struct vop_plane_state *vop_state = to_vop_plane_state(state);
+
+	if (state->fb)
+		drm_framebuffer_unreference(state->fb);
+
+	kfree(vop_state);
 }
 
 static const struct drm_plane_funcs vop_plane_funcs = {
-	.update_plane = vop_update_plane,
-	.disable_plane = vop_disable_plane,
+	.update_plane	= drm_atomic_helper_update_plane,
+	.disable_plane	= drm_atomic_helper_disable_plane,
 	.destroy = vop_plane_destroy,
+	.reset = vop_atomic_plane_reset,
+	.atomic_duplicate_state = vop_atomic_plane_duplicate_state,
+	.atomic_destroy_state = vop_atomic_plane_destroy_state,
 };
 
 int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
@@ -1116,29 +1081,10 @@ static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
 	return true;
 }
 
-static int vop_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
-				  struct drm_framebuffer *old_fb)
-{
-	int ret;
-
-	crtc->x = x;
-	crtc->y = y;
-
-	ret = vop_update_primary_plane(crtc, NULL);
-	if (ret < 0) {
-		DRM_ERROR("fail to update plane\n");
-		return ret;
-	}
-
-	return 0;
-}
-
-static int vop_crtc_mode_set(struct drm_crtc *crtc,
-			     struct drm_display_mode *mode,
-			     struct drm_display_mode *adjusted_mode,
-			     int x, int y, struct drm_framebuffer *fb)
+static void vop_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
+	struct drm_display_mode *adjusted_mode = &crtc->state->adjusted_mode;
 	u16 hsync_len = adjusted_mode->hsync_end - adjusted_mode->hsync_start;
 	u16 hdisplay = adjusted_mode->hdisplay;
 	u16 htotal = adjusted_mode->htotal;
@@ -1149,7 +1095,6 @@ static int vop_crtc_mode_set(struct drm_crtc *crtc,
 	u16 vsync_len = adjusted_mode->vsync_end - adjusted_mode->vsync_start;
 	u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start;
 	u16 vact_end = vact_st + vdisplay;
-	int ret, ret_clk;
 	uint32_t val;
 
 	/*
@@ -1171,7 +1116,6 @@ static int vop_crtc_mode_set(struct drm_crtc *crtc,
 	default:
 		DRM_ERROR("unsupport connector_type[%d]\n",
 			  vop->connector_type);
-		ret = -EINVAL;
 		goto out;
 	};
 	VOP_CTRL_SET(vop, out_mode, vop->connector_out_mode);
@@ -1193,9 +1137,6 @@ static int vop_crtc_mode_set(struct drm_crtc *crtc,
 	VOP_CTRL_SET(vop, vact_st_end, val);
 	VOP_CTRL_SET(vop, vpost_st_end, val);
 
-	ret = vop_crtc_mode_set_base(crtc, x, y, fb);
-	if (ret)
-		goto out;
 
 	/*
 	 * reset dclk, take all mode config affect, so the clk would run in
@@ -1207,64 +1148,33 @@ static int vop_crtc_mode_set(struct drm_crtc *crtc,
 
 	clk_set_rate(vop->dclk, adjusted_mode->clock * 1000);
 out:
-	ret_clk = clk_enable(vop->dclk);
-	if (ret_clk < 0) {
+	if (clk_enable(vop->dclk) < 0)
 		dev_err(vop->dev, "failed to enable dclk - %d\n", ret_clk);
-		return ret_clk;
-	}
 
-	return ret;
 }
 
-static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
-	.enable = vop_crtc_enable,
-	.disable = vop_crtc_disable,
-	.mode_fixup = vop_crtc_mode_fixup,
-	.mode_set = vop_crtc_mode_set,
-	.mode_set_base = vop_crtc_mode_set_base,
-};
-
-static int vop_crtc_page_flip(struct drm_crtc *crtc,
-			      struct drm_framebuffer *fb,
-			      struct drm_pending_vblank_event *event,
-			      uint32_t page_flip_flags)
+static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
+				  struct drm_crtc_state *old_crtc_state)
 {
 	struct vop *vop = to_vop(crtc);
-	struct drm_framebuffer *old_fb = crtc->primary->fb;
-	int ret;
 
-	/* when the page flip is requested, crtc should be on */
-	if (!vop->is_enabled) {
-		DRM_DEBUG("page flip request rejected because crtc is off.\n");
-		return 0;
-	}
+	if (!vop->is_enabled)
+		return;
 
-	crtc->primary->fb = fb;
+	spin_lock(&vop->reg_lock);
 
-	ret = vop_update_primary_plane(crtc, event);
-	if (ret)
-		crtc->primary->fb = old_fb;
+	vop_cfg_done(vop);
 
-	return ret;
+	spin_unlock(&vop->reg_lock);
 }
 
-static void vop_win_state_complete(struct vop_win *vop_win,
-				   struct vop_win_state *state)
-{
-	struct vop *vop = vop_win->vop;
-	struct drm_crtc *crtc = &vop->crtc;
-	struct drm_device *drm = crtc->dev;
-	unsigned long flags;
-
-	if (state->event) {
-		spin_lock_irqsave(&drm->event_lock, flags);
-		drm_crtc_send_vblank_event(crtc, state->event);
-		spin_unlock_irqrestore(&drm->event_lock, flags);
-	}
-
-	list_del(&state->head);
-	drm_crtc_vblank_put(crtc);
-}
+static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
+	.enable = vop_crtc_enable,
+	.disable = vop_crtc_disable,
+	.mode_fixup = vop_crtc_mode_fixup,
+	.mode_set_nofb = vop_crtc_mode_set_nofb,
+	.atomic_flush = vop_crtc_atomic_flush,
+};
 
 static void vop_crtc_destroy(struct drm_crtc *crtc)
 {
@@ -1272,107 +1182,51 @@ static void vop_crtc_destroy(struct drm_crtc *crtc)
 }
 
 static const struct drm_crtc_funcs vop_crtc_funcs = {
-	.set_config = drm_crtc_helper_set_config,
-	.page_flip = vop_crtc_page_flip,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
 	.destroy = vop_crtc_destroy,
+	.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 bool vop_win_state_is_active(struct vop_win *vop_win,
-				    struct vop_win_state *state)
+static bool vop_win_pending_is_complete(struct vop_win *vop_win)
 {
-	bool active = false;
-
-	if (state->fb) {
-		dma_addr_t yrgb_mst;
-
-		/* check yrgb_mst to tell if pending_fb is now front */
-		yrgb_mst = VOP_WIN_GET_YRGBADDR(vop_win->vop, vop_win->data);
-
-		active = (yrgb_mst == state->yrgb_mst);
-	} else {
-		bool enabled;
-
-		/* if enable bit is clear, plane is now disabled */
-		enabled = VOP_WIN_GET(vop_win->vop, vop_win->data, enable);
-
-		active = (enabled == 0);
-	}
-
-	return active;
-}
+	struct drm_plane *plane = &vop_win->base;
+	struct vop_plane_state *state = to_vop_plane_state(plane->state);
+	dma_addr_t yrgb_mst;
 
-static void vop_win_state_destroy(struct vop_win_state *state)
-{
-	struct drm_framebuffer *fb = state->fb;
+	if (!state->enable)
+		return VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0;
 
-	if (fb)
-		drm_framebuffer_unreference(fb);
+	yrgb_mst = VOP_WIN_GET_YRGBADDR(vop_win->vop, vop_win->data);
 
-	kfree(state);
+	return yrgb_mst == state->dma_addr[0];
 }
 
-static void vop_win_update_state(struct vop_win *vop_win)
+static void vop_handle_vblank(struct vop *vop)
 {
-	struct vop_win_state *state, *n, *new_active = NULL;
-
-	/* Check if any pending states are now active */
-	list_for_each_entry(state, &vop_win->pending, head)
-		if (vop_win_state_is_active(vop_win, state)) {
-			new_active = state;
-			break;
-		}
-
-	if (!new_active)
-		return;
+	struct drm_device *drm = vop->drm_dev;
+	struct drm_crtc *crtc = &vop->crtc;
+	struct drm_pending_vblank_event *event = crtc->state->event;
+	unsigned long flags;
+	int i;
 
-	/*
-	 * Destroy any 'skipped' pending states - states that were queued
-	 * before the newly active state.
-	 */
-	list_for_each_entry_safe(state, n, &vop_win->pending, head) {
-		if (state == new_active)
-			break;
-		vop_win_state_complete(vop_win, state);
-		vop_win_state_destroy(state);
+	for (i = 0; i < vop->data->win_size; i++) {
+		if (!vop_win_pending_is_complete(&vop->win[i]))
+			return;
 	}
 
-	vop_win_state_complete(vop_win, new_active);
-
-	if (vop_win->active)
-		vop_win_state_destroy(vop_win->active);
-	vop_win->active = new_active;
-}
-
-static bool vop_win_has_pending_state(struct vop_win *vop_win)
-{
-	return !list_empty(&vop_win->pending);
-}
-
-static irqreturn_t vop_isr_thread(int irq, void *data)
-{
-	struct vop *vop = data;
-	const struct vop_data *vop_data = vop->data;
-	unsigned int i;
-
-	mutex_lock(&vop->vsync_mutex);
-
-	if (!vop->vsync_work_pending)
-		goto done;
+	if (event) {
+		spin_lock_irqsave(&drm->event_lock, flags);
 
-	vop->vsync_work_pending = false;
+		drm_crtc_send_vblank_event(crtc, event);
+		crtc->state->event = NULL;
 
-	for (i = 0; i < vop_data->win_size; i++) {
-		struct vop_win *vop_win = &vop->win[i];
-
-		vop_win_update_state(vop_win);
-		if (vop_win_has_pending_state(vop_win))
-			vop->vsync_work_pending = true;
+		spin_unlock_irqrestore(&drm->event_lock, flags);
 	}
-
-done:
-	mutex_unlock(&vop->vsync_mutex);
-
-	return IRQ_HANDLED;
+	if (!completion_done(&vop->wait_update_complete))
+		complete(&vop->wait_update_complete);
 }
 
 static irqreturn_t vop_isr(int irq, void *data)
@@ -1408,8 +1262,9 @@ static irqreturn_t vop_isr(int irq, void *data)
 
 	if (active_irqs & FS_INTR) {
 		drm_crtc_handle_vblank(crtc);
+		vop_handle_vblank(vop);
 		active_irqs &= ~FS_INTR;
-		ret = (vop->vsync_work_pending) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
+		ret = IRQ_HANDLED;
 	}
 
 	/* Unhandled irqs are spurious. */
@@ -1454,6 +1309,7 @@ static int vop_create_crtc(struct vop *vop)
 		}
 
 		plane = &vop_win->base;
+		drm_plane_helper_add(plane, &plane_helper_funcs);
 		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
 			primary = plane;
 		else if (plane->type == DRM_PLANE_TYPE_CURSOR)
@@ -1489,6 +1345,7 @@ static int vop_create_crtc(struct vop *vop)
 			DRM_ERROR("failed to initialize overlay plane\n");
 			goto err_cleanup_crtc;
 		}
+		drm_plane_helper_add(&vop_win->base, &plane_helper_funcs);
 	}
 
 	port = of_get_child_by_name(dev->of_node, "port");
@@ -1499,6 +1356,7 @@ static int vop_create_crtc(struct vop *vop)
 	}
 
 	init_completion(&vop->dsp_hold_completion);
+	init_completion(&vop->wait_update_complete);
 	crtc->port = port;
 	rockchip_register_crtc_funcs(crtc, &private_crtc_funcs);
 
@@ -1632,7 +1490,6 @@ static void vop_win_init(struct vop *vop)
 
 		vop_win->data = win_data;
 		vop_win->vop = vop;
-		INIT_LIST_HEAD(&vop_win->pending);
 	}
 }
 
@@ -1693,8 +1550,8 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 
 	mutex_init(&vop->vsync_mutex);
 
-	ret = devm_request_threaded_irq(dev, vop->irq, vop_isr, vop_isr_thread,
-					IRQF_SHARED, dev_name(dev), vop);
+	ret = devm_request_irq(dev, vop->irq, vop_isr,
+			       IRQF_SHARED, dev_name(dev), vop);
 	if (ret)
 		return ret;
 
-- 
1.7.9.5

[RFC PATCH 3/9] drm/rockchip: Convert to support atomic API

[Daniel Stone]

Hi Mark,

On 1 December 2015 at 03:26, Mark Yao <mark.yao@rock-chips.com> wrote:
> +static void rockchip_atomic_wait_for_complete(struct drm_atomic_state *state)
> +{
> +       struct drm_crtc_state *crtc_state;
> +       struct drm_crtc *crtc;
> +       int i;
> +
> +       for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +               if (!crtc->state->active)
> +                       continue;
> +
> +               WARN_ON(drm_crtc_vblank_get(crtc));
> +       }
> +
> +       for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +               if (!crtc->state->active)
> +                       continue;
> +
> +               rockchip_crtc_wait_for_update(crtc);
> +       }

I'd be much more comfortable if this passed in an explicit pointer to
state, or an address to wait for, rather than have wait_for_complete
dig out state with no locking. The latter is potentially racy for
async operations.

> +struct vop_plane_state {
> +       struct drm_plane_state base;
> +       dma_addr_t dma_addr[ROCKCHIP_MAX_FB_BUFFER];

Can you get rid of this by just using the pointer to a
rockchip_gem_object already provided?

> -static int vop_update_plane_event(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 drm_pending_vblank_event *event)
> +static int vop_plane_atomic_check(struct drm_plane *plane,
> +                          struct drm_plane_state *state)
>  {
> +       struct drm_crtc *crtc = state->crtc;
> +       struct drm_framebuffer *fb = state->fb;
>         struct vop_win *vop_win = to_vop_win(plane);
> +       struct vop_plane_state *vop_plane_state = to_vop_plane_state(state);
>         const struct vop_win_data *win = vop_win->data;
> -       struct vop *vop = to_vop(crtc);
>         struct drm_gem_object *obj;
>         struct rockchip_gem_object *rk_obj;
> -       struct drm_gem_object *uv_obj;
> -       struct rockchip_gem_object *rk_uv_obj;
>         unsigned long offset;
> -       unsigned int actual_w;
> -       unsigned int actual_h;
> -       unsigned int dsp_stx;
> -       unsigned int dsp_sty;
> -       unsigned int y_vir_stride;
> -       unsigned int uv_vir_stride = 0;
> -       dma_addr_t yrgb_mst;
> -       dma_addr_t uv_mst = 0;
> -       enum vop_data_format format;
> -       uint32_t val;
> -       bool is_alpha;
> -       bool rb_swap;
>         bool is_yuv;
>         bool visible;
>         int ret;
> -       struct drm_rect dest = {
> -               .x1 = crtc_x,
> -               .y1 = crtc_y,
> -               .x2 = crtc_x + crtc_w,
> -               .y2 = crtc_y + crtc_h,
> -       };
> -       struct drm_rect src = {
> -               /* 16.16 fixed point */
> -               .x1 = src_x,
> -               .y1 = src_y,
> -               .x2 = src_x + src_w,
> -               .y2 = src_y + src_h,
> -       };
> -       const struct drm_rect clip = {
> -               .x2 = crtc->mode.hdisplay,
> -               .y2 = crtc->mode.vdisplay,
> -       };
> -       bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY;
> +       struct drm_rect *dest = &vop_plane_state->dest;
> +       struct drm_rect *src = &vop_plane_state->src;
> +       struct drm_rect clip;
>         int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
>                                         DRM_PLANE_HELPER_NO_SCALING;
>         int max_scale = win->phy->scl ? FRAC_16_16(8, 1) :
>                                         DRM_PLANE_HELPER_NO_SCALING;
>
> -       ret = drm_plane_helper_check_update(plane, crtc, fb,
> -                                           &src, &dest, &clip,
> +       crtc = crtc ? crtc : plane->state->crtc;
> +       /*
> +        * Both crtc or plane->state->crtc can be null.
> +        */
> +       if (!crtc || !fb)
> +               goto out_disable;
> +       src->x1 = state->src_x;
> +       src->y1 = state->src_y;
> +       src->x2 = state->src_x + state->src_w;
> +       src->y2 = state->src_y + state->src_h;
> +       dest->x1 = state->crtc_x;
> +       dest->y1 = state->crtc_y;
> +       dest->x2 = state->crtc_x + state->crtc_w;
> +       dest->y2 = state->crtc_y + state->crtc_h;
> +
> +       clip.x1 = 0;
> +       clip.y1 = 0;
> +       clip.x2 = crtc->mode.hdisplay;
> +       clip.y2 = crtc->mode.vdisplay;
> +
> +       ret = drm_plane_helper_check_update(plane, crtc, state->fb,
> +                                           src, dest, &clip,
>                                             min_scale,
>                                             max_scale,
> -                                           can_position, false, &visible);
> +                                           true, true, &visible);
>         if (ret)
>                 return ret;
>
>         if (!visible)
> -               return 0;
> -
> -       is_alpha = is_alpha_support(fb->pixel_format);
> -       rb_swap = has_rb_swapped(fb->pixel_format);
> -       is_yuv = is_yuv_support(fb->pixel_format);
> +               goto out_disable;
>
> -       format = vop_convert_format(fb->pixel_format);
> -       if (format < 0)
> -               return format;
> +       vop_plane_state->format = vop_convert_format(fb->pixel_format);
> +       if (vop_plane_state->format < 0)
> +               return vop_plane_state->format;
>
> -       obj = rockchip_fb_get_gem_obj(fb, 0);
> -       if (!obj) {
> -               DRM_ERROR("fail to get rockchip gem object from framebuffer\n");
> -               return -EINVAL;
> -       }
> -
> -       rk_obj = to_rockchip_obj(obj);
> +       is_yuv = is_yuv_support(fb->pixel_format);
>
>         if (is_yuv) {
>                 /*
>                  * Src.x1 can be odd when do clip, but yuv plane start point
>                  * need align with 2 pixel.
>                  */
> -               val = (src.x1 >> 16) % 2;
> -               src.x1 += val << 16;
> -               src.x2 += val << 16;
> +               uint32_t temp = (src->x1 >> 16) % 2;
> +
> +               src->x1 += temp << 16;
> +               src->x2 += temp << 16;
>         }

I know this isn't new, but moving the plane around is bad. If the user
gives you a pixel boundary that you can't actually use, please reject
the configuration rather than silently trying to fix it up.

> -static void vop_plane_destroy(struct drm_plane *plane)
> +static void vop_atomic_plane_destroy_state(struct drm_plane *plane,
> +                                          struct drm_plane_state *state)
>  {
> -       vop_disable_plane(plane);
> -       drm_plane_cleanup(plane);
> +       struct vop_plane_state *vop_state = to_vop_plane_state(state);
> +
> +       if (state->fb)
> +               drm_framebuffer_unreference(state->fb);
> +
> +       kfree(vop_state);
>  }

You can replace this hook with drm_atomic_helper_plane_destroy_state.

> -static void vop_win_state_complete(struct vop_win *vop_win,
> -                                  struct vop_win_state *state)
> -{
> -       struct vop *vop = vop_win->vop;
> -       struct drm_crtc *crtc = &vop->crtc;
> -       struct drm_device *drm = crtc->dev;
> -       unsigned long flags;
> -
> -       if (state->event) {
> -               spin_lock_irqsave(&drm->event_lock, flags);
> -               drm_crtc_send_vblank_event(crtc, state->event);
> -               spin_unlock_irqrestore(&drm->event_lock, flags);
> -       }

Ah, I see this is based on top of the patches I sent to fix pageflips?
If they have been merged, I would like to send you some others to
merge as well, which fix an OOPS when you close Weston. I didn't send
them yet as I didn't get a response to the previous patchset, so did
not know you had merged it. There are a lot of other correctness fixes
I had in there (e.g. using the CRTC vblank functions, some locking
fixes), but if this code is going to be thrown away then I will just
discard most of them as well.

Still, I would like to prepare another series for you to merge through
drm-fixes, to be able to run Weston (the same-fb-flip patch I sent
along with the drm_crtc_send_vblank_event conversion, also adding a
preclose hook to discard events when clients exit).

Cheers,
Daniel

[RFC PATCH 3/9] drm/rockchip: Convert to support atomic API

[Mark yao]

On 2015?12?01? 16:18, Daniel Stone wrote:
> Hi Mark,
>
> On 1 December 2015 at 03:26, Mark Yao <mark.yao@rock-chips.com> wrote:
>> +static void rockchip_atomic_wait_for_complete(struct drm_atomic_state *state)
>> +{
>> +       struct drm_crtc_state *crtc_state;
>> +       struct drm_crtc *crtc;
>> +       int i;
>> +
>> +       for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +               if (!crtc->state->active)
>> +                       continue;
>> +
>> +               WARN_ON(drm_crtc_vblank_get(crtc));
>> +       }
>> +
>> +       for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +               if (!crtc->state->active)
>> +                       continue;
>> +
>> +               rockchip_crtc_wait_for_update(crtc);
>> +       }
> I'd be much more comfortable if this passed in an explicit pointer to
> state, or an address to wait for, rather than have wait_for_complete
> dig out state with no locking. The latter is potentially racy for
> async operations.
>
>> +struct vop_plane_state {
>> +       struct drm_plane_state base;
>> +       dma_addr_t dma_addr[ROCKCHIP_MAX_FB_BUFFER];
> Can you get rid of this by just using the pointer to a
> rockchip_gem_object already provided?

Good idea, I will do that.

>> -static int vop_update_plane_event(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 drm_pending_vblank_event *event)
>> +static int vop_plane_atomic_check(struct drm_plane *plane,
>> +                          struct drm_plane_state *state)
>>   {
>> +       struct drm_crtc *crtc = state->crtc;
>> +       struct drm_framebuffer *fb = state->fb;
>>          struct vop_win *vop_win = to_vop_win(plane);
>> +       struct vop_plane_state *vop_plane_state = to_vop_plane_state(state);
>>          const struct vop_win_data *win = vop_win->data;
>> -       struct vop *vop = to_vop(crtc);
>>          struct drm_gem_object *obj;
>>          struct rockchip_gem_object *rk_obj;
>> -       struct drm_gem_object *uv_obj;
>> -       struct rockchip_gem_object *rk_uv_obj;
>>          unsigned long offset;
>> -       unsigned int actual_w;
>> -       unsigned int actual_h;
>> -       unsigned int dsp_stx;
>> -       unsigned int dsp_sty;
>> -       unsigned int y_vir_stride;
>> -       unsigned int uv_vir_stride = 0;
>> -       dma_addr_t yrgb_mst;
>> -       dma_addr_t uv_mst = 0;
>> -       enum vop_data_format format;
>> -       uint32_t val;
>> -       bool is_alpha;
>> -       bool rb_swap;
>>          bool is_yuv;
>>          bool visible;
>>          int ret;
>> -       struct drm_rect dest = {
>> -               .x1 = crtc_x,
>> -               .y1 = crtc_y,
>> -               .x2 = crtc_x + crtc_w,
>> -               .y2 = crtc_y + crtc_h,
>> -       };
>> -       struct drm_rect src = {
>> -               /* 16.16 fixed point */
>> -               .x1 = src_x,
>> -               .y1 = src_y,
>> -               .x2 = src_x + src_w,
>> -               .y2 = src_y + src_h,
>> -       };
>> -       const struct drm_rect clip = {
>> -               .x2 = crtc->mode.hdisplay,
>> -               .y2 = crtc->mode.vdisplay,
>> -       };
>> -       bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY;
>> +       struct drm_rect *dest = &vop_plane_state->dest;
>> +       struct drm_rect *src = &vop_plane_state->src;
>> +       struct drm_rect clip;
>>          int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
>>                                          DRM_PLANE_HELPER_NO_SCALING;
>>          int max_scale = win->phy->scl ? FRAC_16_16(8, 1) :
>>                                          DRM_PLANE_HELPER_NO_SCALING;
>>
>> -       ret = drm_plane_helper_check_update(plane, crtc, fb,
>> -                                           &src, &dest, &clip,
>> +       crtc = crtc ? crtc : plane->state->crtc;
>> +       /*
>> +        * Both crtc or plane->state->crtc can be null.
>> +        */
>> +       if (!crtc || !fb)
>> +               goto out_disable;
>> +       src->x1 = state->src_x;
>> +       src->y1 = state->src_y;
>> +       src->x2 = state->src_x + state->src_w;
>> +       src->y2 = state->src_y + state->src_h;
>> +       dest->x1 = state->crtc_x;
>> +       dest->y1 = state->crtc_y;
>> +       dest->x2 = state->crtc_x + state->crtc_w;
>> +       dest->y2 = state->crtc_y + state->crtc_h;
>> +
>> +       clip.x1 = 0;
>> +       clip.y1 = 0;
>> +       clip.x2 = crtc->mode.hdisplay;
>> +       clip.y2 = crtc->mode.vdisplay;
>> +
>> +       ret = drm_plane_helper_check_update(plane, crtc, state->fb,
>> +                                           src, dest, &clip,
>>                                              min_scale,
>>                                              max_scale,
>> -                                           can_position, false, &visible);
>> +                                           true, true, &visible);
>>          if (ret)
>>                  return ret;
>>
>>          if (!visible)
>> -               return 0;
>> -
>> -       is_alpha = is_alpha_support(fb->pixel_format);
>> -       rb_swap = has_rb_swapped(fb->pixel_format);
>> -       is_yuv = is_yuv_support(fb->pixel_format);
>> +               goto out_disable;
>>
>> -       format = vop_convert_format(fb->pixel_format);
>> -       if (format < 0)
>> -               return format;
>> +       vop_plane_state->format = vop_convert_format(fb->pixel_format);
>> +       if (vop_plane_state->format < 0)
>> +               return vop_plane_state->format;
>>
>> -       obj = rockchip_fb_get_gem_obj(fb, 0);
>> -       if (!obj) {
>> -               DRM_ERROR("fail to get rockchip gem object from framebuffer\n");
>> -               return -EINVAL;
>> -       }
>> -
>> -       rk_obj = to_rockchip_obj(obj);
>> +       is_yuv = is_yuv_support(fb->pixel_format);
>>
>>          if (is_yuv) {
>>                  /*
>>                   * Src.x1 can be odd when do clip, but yuv plane start point
>>                   * need align with 2 pixel.
>>                   */
>> -               val = (src.x1 >> 16) % 2;
>> -               src.x1 += val << 16;
>> -               src.x2 += val << 16;
>> +               uint32_t temp = (src->x1 >> 16) % 2;
>> +
>> +               src->x1 += temp << 16;
>> +               src->x2 += temp << 16;
>>          }
> I know this isn't new, but moving the plane around is bad. If the user
> gives you a pixel boundary that you can't actually use, please reject
> the configuration rather than silently trying to fix it up.

the origin src.x1 would align with 2 pixel, but when we move the dest 
window, and do clip by output, the src.x1 may be clipped to odd.
regect this configuration may let user confuse, sometimes good, 
sometimes bad.

>> -static void vop_plane_destroy(struct drm_plane *plane)
>> +static void vop_atomic_plane_destroy_state(struct drm_plane *plane,
>> +                                          struct drm_plane_state *state)
>>   {
>> -       vop_disable_plane(plane);
>> -       drm_plane_cleanup(plane);
>> +       struct vop_plane_state *vop_state = to_vop_plane_state(state);
>> +
>> +       if (state->fb)
>> +               drm_framebuffer_unreference(state->fb);
>> +
>> +       kfree(vop_state);
>>   }
> You can replace this hook with drm_atomic_helper_plane_destroy_state.

Hmm, only can hook with __drm_atomic_helper_plane_destroy_state.

>> -static void vop_win_state_complete(struct vop_win *vop_win,
>> -                                  struct vop_win_state *state)
>> -{
>> -       struct vop *vop = vop_win->vop;
>> -       struct drm_crtc *crtc = &vop->crtc;
>> -       struct drm_device *drm = crtc->dev;
>> -       unsigned long flags;
>> -
>> -       if (state->event) {
>> -               spin_lock_irqsave(&drm->event_lock, flags);
>> -               drm_crtc_send_vblank_event(crtc, state->event);
>> -               spin_unlock_irqrestore(&drm->event_lock, flags);
>> -       }
> Ah, I see this is based on top of the patches I sent to fix pageflips?
> If they have been merged, I would like to send you some others to
> merge as well, which fix an OOPS when you close Weston. I didn't send
> them yet as I didn't get a response to the previous patchset, so did
> not know you had merged it. There are a lot of other correctness fixes
> I had in there (e.g. using the CRTC vblank functions, some locking
> fixes), but if this code is going to be thrown away then I will just
> discard most of them as well.
>
> Still, I would like to prepare another series for you to merge through
> drm-fixes, to be able to run Weston (the same-fb-flip patch I sent
> along with the drm_crtc_send_vblank_event conversion, also adding a
> preclose hook to discard events when clients exit).
>
> Cheers,
> Daniel
>
>
>
Hi Daniel
     Can you share your Weston environment to me, I'm interesting to 
test drm rockchip on weston.

-- 
?ark Yao

[RFC PATCH 3/9] drm/rockchip: Convert to support atomic API

[Mark yao]

On 2015?12?01? 16:18, Daniel Stone wrote:
> Hi Mark,
>
> On 1 December 2015 at 03:26, Mark Yao<mark.yao@rock-chips.com>  wrote:
>> >+static void rockchip_atomic_wait_for_complete(struct drm_atomic_state *state)
>> >+{
>> >+       struct drm_crtc_state *crtc_state;
>> >+       struct drm_crtc *crtc;
>> >+       int i;
>> >+
>> >+       for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> >+               if (!crtc->state->active)
>> >+                       continue;
>> >+
>> >+               WARN_ON(drm_crtc_vblank_get(crtc));
>> >+       }
>> >+
>> >+       for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> >+               if (!crtc->state->active)
>> >+                       continue;
>> >+
>> >+               rockchip_crtc_wait_for_update(crtc);
>> >+       }
> I'd be much more comfortable if this passed in an explicit pointer to
> state, or an address to wait for, rather than have wait_for_complete
> dig out state with no locking. The latter is potentially racy for
> async operations.
>
Hi Daniel
    "if this passed in an explicit pointer to state, or an address to 
wait for", I don't understand, can you point how it work?

-- 
?ark Yao

[RFC PATCH 3/9] drm/rockchip: Convert to support atomic API

[Daniel Stone]

Hi Mark,
Thanks for getting back to this.

On 1 December 2015 at 09:31, Mark yao <mark.yao@rock-chips.com> wrote:
> On 2015?12?01? 16:18, Daniel Stone wrote:
>> On 1 December 2015 at 03:26, Mark Yao<mark.yao@rock-chips.com>  wrote:
>>> >+       for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>> >+               if (!crtc->state->active)
>>> >+                       continue;
>>> >+
>>> >+               rockchip_crtc_wait_for_update(crtc);
>>> >+       }
>>
>> I'd be much more comfortable if this passed in an explicit pointer to
>> state, or an address to wait for, rather than have wait_for_complete
>> dig out state with no locking. The latter is potentially racy for
>> async operations.
>>
> Hi Daniel
>    "if this passed in an explicit pointer to state, or an address to wait
> for", I don't understand, can you point how it work?

Ah, OK. I mean that rockchip_crtc_wait_for_update takes a drm_crtc
pointer, and establishes the state from that (e.g.
crtc->primary->state). This can easily lead to confusion in async
contexts, as the states attached to a drm_crtc and a drm_plane can
change here while you wait for it.

It would be better if the call was:

for_each_plane_in_state(state, plane, plane_state, i) {
    if (plane->type == DRM_PLANE_TYPE_PRIMARY)
        rockchip_crtc_wait_for_update(plane_state->crtc, plane_state);
}

This way it is very clear, and there is no confusion as to which
request we are waiting to complete.

In general, using crtc->state or plane->state is a very bad idea,
_except_ in the atomic_check function where you are calculating
changes (e.g. if (plane_state->fb->pitches[0] !=
plane->state->fb->pitches[0]) rockchip_plane_state->pitch_changed =
true). After atomic_check, you should always pass around pointers to
the plane state explicitly, and avoid using the pointers from drm_crtc
and drm_plane.

Does that help?

>>>          if (is_yuv) {
>>>                  /*
>>>                   * Src.x1 can be odd when do clip, but yuv plane start
>>> point
>>>                   * need align with 2 pixel.
>>>                   */
>>> -               val = (src.x1 >> 16) % 2;
>>> -               src.x1 += val << 16;
>>> -               src.x2 += val << 16;
>>> +               uint32_t temp = (src->x1 >> 16) % 2;
>>> +
>>> +               src->x1 += temp << 16;
>>> +               src->x2 += temp << 16;
>>>          }
>>
>> I know this isn't new, but moving the plane around is bad. If the user
>> gives you a pixel boundary that you can't actually use, please reject
>> the configuration rather than silently trying to fix it up.
>
> the origin src.x1 would align with 2 pixel, but when we move the dest
> window, and do clip by output, the src.x1 may be clipped to odd.
> regect this configuration may let user confuse, sometimes good, sometimes
> bad.

For me, it is more confusing when the display shows something
different to what I have requested. In some media usecases, doing this
is a showstopper and will result in products failing acceptance
testing. Userspace can make a policy decision to try different
alignments to get _something_ to show (even if it's not what was
explicitly requested), but doing this in the kernel is inappropriate:
please just reject it, and have userspace fall back to another
composition method (e.g. GL) in these cases.

>>> -static void vop_plane_destroy(struct drm_plane *plane)
>>> +static void vop_atomic_plane_destroy_state(struct drm_plane *plane,
>>> +                                          struct drm_plane_state *state)
>>>   {
>>> -       vop_disable_plane(plane);
>>> -       drm_plane_cleanup(plane);
>>> +       struct vop_plane_state *vop_state = to_vop_plane_state(state);
>>> +
>>> +       if (state->fb)
>>> +               drm_framebuffer_unreference(state->fb);
>>> +
>>> +       kfree(vop_state);
>>>   }
>>
>> You can replace this hook with drm_atomic_helper_plane_destroy_state.
>
>
> Hmm, only can hook with __drm_atomic_helper_plane_destroy_state.

Ah yes, you're right. But still, using that would be better than duplicating it.

>     Can you share your Weston environment to me, I'm interesting to test drm
> rockchip on weston.

Of course. You can download Weston from http://wayland.freedesktop.org
- the most interesting dependencies are libevdev, libinput, and
wayland itself. If you are building newer Weston from git, you'll need
the wayland-protocols repository as well, from
anongit.freedesktop.org/git/wayland/wayland-protocols/. Please let me
know privately if you need some more help with building these, but
they should be quite straightforward.

Cheers,
Daniel

[RFC PATCH 3/9] drm/rockchip: Convert to support atomic API

[Daniel Stone]

Hi Mark,

On 2 December 2015 at 14:18, Daniel Stone <daniel@fooishbar.org> wrote:
> On 1 December 2015 at 09:31, Mark yao <mark.yao@rock-chips.com> wrote:
>>     Can you share your Weston environment to me, I'm interesting to test drm
>> rockchip on weston.
>
> Of course. You can download Weston from http://wayland.freedesktop.org
> - the most interesting dependencies are libevdev, libinput, and
> wayland itself. If you are building newer Weston from git, you'll need
> the wayland-protocols repository as well, from
> anongit.freedesktop.org/git/wayland/wayland-protocols/. Please let me
> know privately if you need some more help with building these, but
> they should be quite straightforward.

Sorry, left one thing out. When running, if you do not have
GBM/Wayland enabled for Mali (or no Mali support), you should run
with:
weston --use-pixman
or:
weston-launch -- --use-pixman

Launching from SSH is a bit more complicated:
sudo weston-launch --user=$username --tty=/dev/tty3 -- --use-pixman

(Replace tty3 with the TTY of your choice: it must be in text mode.)

Once you have done this, you will find three issues:
  - passing -1 to drm_send_vblank event causes OOPS - now fixed in your tree
  - not sending pageflip events for same-fb flips causes Weston hang -
fixed with my patch, and I believe now fixed in atomic (though it may
still have some timing issues; I hope to get to review this early next
week)
  - not having a preclose hook causes OOPS when Weston exits in the
middle of rendering - fixed in
https://git.collabora.com/cgit/user/daniels/linux.git/commit/?h=wip/4.4.x/rockchip-drm-fixes&id=d14f21bcd7e7a1b9ca129c411a9da9c911037965
and the preceding commit, which I hope to re-send for 4.4 -fixes in
the next couple of days

Hope this helps.

Cheers,
Daniel

[RFC PATCH 3/9] drm/rockchip: Convert to support atomic API

[Mark yao]

On 2015?12?02? 22:18, Daniel Stone wrote:
> Hi Mark,
> Thanks for getting back to this.
>
> On 1 December 2015 at 09:31, Mark yao <mark.yao@rock-chips.com> wrote:
>> On 2015?12?01? 16:18, Daniel Stone wrote:
>>> On 1 December 2015 at 03:26, Mark Yao<mark.yao@rock-chips.com>  wrote:
>>>>> +       for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>>>> +               if (!crtc->state->active)
>>>>> +                       continue;
>>>>> +
>>>>> +               rockchip_crtc_wait_for_update(crtc);
>>>>> +       }
>>> I'd be much more comfortable if this passed in an explicit pointer to
>>> state, or an address to wait for, rather than have wait_for_complete
>>> dig out state with no locking. The latter is potentially racy for
>>> async operations.
>>>
>> Hi Daniel
>>     "if this passed in an explicit pointer to state, or an address to wait
>> for", I don't understand, can you point how it work?
> Ah, OK. I mean that rockchip_crtc_wait_for_update takes a drm_crtc
> pointer, and establishes the state from that (e.g.
> crtc->primary->state). This can easily lead to confusion in async
> contexts, as the states attached to a drm_crtc and a drm_plane can
> change here while you wait for it.
>
> It would be better if the call was:
>
> for_each_plane_in_state(state, plane, plane_state, i) {
>      if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>          rockchip_crtc_wait_for_update(plane_state->crtc, plane_state);
> }
>
> This way it is very clear, and there is no confusion as to which
> request we are waiting to complete.
>
> In general, using crtc->state or plane->state is a very bad idea,
> _except_ in the atomic_check function where you are calculating
> changes (e.g. if (plane_state->fb->pitches[0] !=
> plane->state->fb->pitches[0]) rockchip_plane_state->pitch_changed =
> true). After atomic_check, you should always pass around pointers to
> the plane state explicitly, and avoid using the pointers from drm_crtc
> and drm_plane.
>
> Does that help?

Hi Daniel

Sorry, I don't actually understand why crtc->state or plane->state is no 
recommended
except atomic_check, on atomic_update, we need use plane->state, is that 
a problem?

I guess that, drm_atomic_helper_swap_state would race with async operations,
so use crtc->state on async stack is not safe. is it correct?

I think we can make asynchronous commit serialize as tegra drm done to 
avoid this problem:


   86         /* serialize outstanding asynchronous commits */
   87 mutex_lock(&tegra->commit.lock);
   88 flush_work(&tegra->commit.work);
89
   90 /*
   91          * This is the point of no return - everything below never 
fails except
   92          * when the hw goes bonghits. Which means we can commit 
the new state on
   93          * the software side now.
   94 */
95
   96         drm_atomic_helper_swap_state(drm, state);
97
   98         if (async)
   99                 tegra_atomic_schedule(tegra, state);
  100 else
  101                 tegra_atomic_complete(tegra, state);
  102
  103         mutex_unlock(&tegra->commit.lock);


>
>>>>           if (is_yuv) {
>>>>                   /*
>>>>                    * Src.x1 can be odd when do clip, but yuv plane start
>>>> point
>>>>                    * need align with 2 pixel.
>>>>                    */
>>>> -               val = (src.x1 >> 16) % 2;
>>>> -               src.x1 += val << 16;
>>>> -               src.x2 += val << 16;
>>>> +               uint32_t temp = (src->x1 >> 16) % 2;
>>>> +
>>>> +               src->x1 += temp << 16;
>>>> +               src->x2 += temp << 16;
>>>>           }
>>> I know this isn't new, but moving the plane around is bad. If the user
>>> gives you a pixel boundary that you can't actually use, please reject
>>> the configuration rather than silently trying to fix it up.
>> the origin src.x1 would align with 2 pixel, but when we move the dest
>> window, and do clip by output, the src.x1 may be clipped to odd.
>> regect this configuration may let user confuse, sometimes good, sometimes
>> bad.
> For me, it is more confusing when the display shows something
> different to what I have requested. In some media usecases, doing this
> is a showstopper and will result in products failing acceptance
> testing. Userspace can make a policy decision to try different
> alignments to get _something_ to show (even if it's not what was
> explicitly requested), but doing this in the kernel is inappropriate:
> please just reject it, and have userspace fall back to another
> composition method (e.g. GL) in these cases.
>
>>>> -static void vop_plane_destroy(struct drm_plane *plane)
>>>> +static void vop_atomic_plane_destroy_state(struct drm_plane *plane,
>>>> +                                          struct drm_plane_state *state)
>>>>    {
>>>> -       vop_disable_plane(plane);
>>>> -       drm_plane_cleanup(plane);
>>>> +       struct vop_plane_state *vop_state = to_vop_plane_state(state);
>>>> +
>>>> +       if (state->fb)
>>>> +               drm_framebuffer_unreference(state->fb);
>>>> +
>>>> +       kfree(vop_state);
>>>>    }
>>> You can replace this hook with drm_atomic_helper_plane_destroy_state.
>>
>> Hmm, only can hook with __drm_atomic_helper_plane_destroy_state.
> Ah yes, you're right. But still, using that would be better than duplicating it.
>
>>      Can you share your Weston environment to me, I'm interesting to test drm
>> rockchip on weston.
> Of course. You can download Weston from http://wayland.freedesktop.org
> - the most interesting dependencies are libevdev, libinput, and
> wayland itself. If you are building newer Weston from git, you'll need
> the wayland-protocols repository as well, from
> anongit.freedesktop.org/git/wayland/wayland-protocols/. Please let me
> know privately if you need some more help with building these, but
> they should be quite straightforward.
>
> Cheers,
> Daniel
>
>
>


-- 
?ark Yao

[RFC PATCH 4/9] drm/rockchip: support atomic asynchronous commit

[Mark Yao]

If drm core requests a async commit, rockchip_drm_atomic_commit
will schedule a work task to update later.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c |   60 ++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index c86d93a..30ddf4a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -29,6 +29,12 @@ struct rockchip_drm_fb {
 	struct drm_gem_object *obj[ROCKCHIP_MAX_FB_BUFFER];
 };
 
+struct rockchip_atomic_commit {
+	struct work_struct	work;
+	struct drm_device	*dev;
+	struct drm_atomic_state *state;
+};
+
 struct drm_gem_object *rockchip_fb_get_gem_obj(struct drm_framebuffer *fb,
 					       unsigned int plane)
 {
@@ -195,20 +201,11 @@ static void rockchip_atomic_wait_for_complete(struct drm_atomic_state *state)
 	}
 }
 
-int rockchip_drm_atomic_commit(struct drm_device *dev,
-			       struct drm_atomic_state *state,
-			       bool async)
+static void
+rockchip_atomic_commit_complete(struct rockchip_atomic_commit *commit)
 {
-	int ret;
-
-	if (async)
-		return -EBUSY;
-
-	ret = drm_atomic_helper_prepare_planes(dev, state);
-	if (ret)
-		return ret;
-
-	drm_atomic_helper_swap_state(dev, state);
+	struct drm_device *dev = commit->dev;
+	struct drm_atomic_state *state = commit->state;
 
 	/*
 	 * TODO: do fence wait here.
@@ -226,6 +223,43 @@ int rockchip_drm_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_state_free(state);
 
+	kfree(commit);
+}
+
+static void rockchip_drm_atomic_work(struct work_struct *work)
+{
+	struct rockchip_atomic_commit *commit = container_of(work,
+					struct rockchip_atomic_commit, work);
+
+	rockchip_atomic_commit_complete(commit);
+}
+
+int rockchip_drm_atomic_commit(struct drm_device *dev,
+			       struct drm_atomic_state *state,
+			       bool async)
+{
+	int ret;
+	struct rockchip_atomic_commit *commit;
+
+	ret = drm_atomic_helper_prepare_planes(dev, state);
+	if (ret)
+		return ret;
+
+	drm_atomic_helper_swap_state(dev, state);
+
+	commit = kzalloc(sizeof(*commit), GFP_KERNEL);
+	if (!commit)
+		return -ENOMEM;
+
+	INIT_WORK(&commit->work, rockchip_drm_atomic_work);
+	commit->dev = dev;
+	commit->state = state;
+
+	if (async)
+		schedule_work(&commit->work);
+	else
+		rockchip_atomic_commit_complete(commit);
+
 	return 0;
 }
 
-- 
1.7.9.5

[RFC PATCH 5/9] drm/rockchip: Optimization vop mode set

[Mark Yao]

Rk3288 vop timing registers is immediately register, when configure
timing on display active time, will cause tearing. use dclk reset is
not a good idea to avoid this tearing. we can avoid tearing by using
standby register.

Vop standby register will take effect at end of current frame, and
go back to work immediately when exit standby.

So we can use standby register to protect this context.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |   49 +++++++++++++++++++--------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index a28e255..6317dea 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1098,10 +1098,40 @@ static void vop_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	uint32_t val;
 
 	/*
-	 * disable dclk to stop frame scan, so that we can safe config mode and
-	 * enable iommu.
+	 * If dclk rate is zero, mean that scanout is stop,
+	 * we don't need wait any more.
 	 */
-	clk_disable(vop->dclk);
+	if (clk_get_rate(vop->dclk)) {
+		/*
+		 * Rk3288 vop timing register is immediately, when configure
+		 * display timing on display time, may cause tearing.
+		 *
+		 * Vop standby will take effect at end of current frame,
+		 * if dsp hold valid irq happen, it means standby complete.
+		 *
+		 * mode set:
+		 *    standby and wait complete --> |----
+		 *                                  | display time
+		 *                                  |----
+		 *                                  |---> dsp hold irq
+		 *     configure display timing --> |
+		 *         standby exit             |
+		 *                                  | new frame start.
+		 */
+
+		reinit_completion(&vop->dsp_hold_completion);
+		vop_dsp_hold_valid_irq_enable(vop);
+
+		spin_lock(&vop->reg_lock);
+
+		VOP_CTRL_SET(vop, standby, 1);
+
+		spin_unlock(&vop->reg_lock);
+
+		wait_for_completion(&vop->dsp_hold_completion);
+
+		vop_dsp_hold_valid_irq_disable(vop);
+	}
 
 	switch (vop->connector_type) {
 	case DRM_MODE_CONNECTOR_LVDS:
@@ -1137,20 +1167,9 @@ static void vop_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	VOP_CTRL_SET(vop, vact_st_end, val);
 	VOP_CTRL_SET(vop, vpost_st_end, val);
 
-
-	/*
-	 * reset dclk, take all mode config affect, so the clk would run in
-	 * correct frame.
-	 */
-	reset_control_assert(vop->dclk_rst);
-	usleep_range(10, 20);
-	reset_control_deassert(vop->dclk_rst);
-
 	clk_set_rate(vop->dclk, adjusted_mode->clock * 1000);
-out:
-	if (clk_enable(vop->dclk) < 0)
-		dev_err(vop->dev, "failed to enable dclk - %d\n", ret_clk);
 
+	VOP_CTRL_SET(vop, standby, 0);
 }
 
 static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
-- 
1.7.9.5

[RFC PATCH 6/9] drm/rockchip: direct config connecter gate and out_mode

[Mark Yao]

Both connecter gate and out_mode are not conflict with mode set
configure. Direct setting connecter gate and out_mode, that allow
connector do rockchip_drm_crtc_mode_config after mode set.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |   41 ++++++++++++---------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 6317dea..c65b454 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -90,9 +90,6 @@ struct vop {
 	struct drm_device *drm_dev;
 	bool is_enabled;
 
-	int connector_type;
-	int connector_out_mode;
-
 	/* mutex vsync_ work */
 	struct mutex vsync_mutex;
 	bool vsync_work_pending;
@@ -1029,8 +1026,24 @@ int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
 {
 	struct vop *vop = to_vop(crtc);
 
-	vop->connector_type = connector_type;
-	vop->connector_out_mode = out_mode;
+	if (WARN_ON(!vop->is_enabled))
+		return -EINVAL;
+
+	switch (connector_type) {
+	case DRM_MODE_CONNECTOR_LVDS:
+		VOP_CTRL_SET(vop, rgb_en, 1);
+		break;
+	case DRM_MODE_CONNECTOR_eDP:
+		VOP_CTRL_SET(vop, edp_en, 1);
+		break;
+	case DRM_MODE_CONNECTOR_HDMIA:
+		VOP_CTRL_SET(vop, hdmi_en, 1);
+		break;
+	default:
+		DRM_ERROR("unsupport connector_type[%d]\n", connector_type);
+		return -EINVAL;
+	};
+	VOP_CTRL_SET(vop, out_mode, out_mode);
 
 	return 0;
 }
@@ -1132,24 +1145,6 @@ static void vop_crtc_mode_set_nofb(struct drm_crtc *crtc)
 
 		vop_dsp_hold_valid_irq_disable(vop);
 	}
-
-	switch (vop->connector_type) {
-	case DRM_MODE_CONNECTOR_LVDS:
-		VOP_CTRL_SET(vop, rgb_en, 1);
-		break;
-	case DRM_MODE_CONNECTOR_eDP:
-		VOP_CTRL_SET(vop, edp_en, 1);
-		break;
-	case DRM_MODE_CONNECTOR_HDMIA:
-		VOP_CTRL_SET(vop, hdmi_en, 1);
-		break;
-	default:
-		DRM_ERROR("unsupport connector_type[%d]\n",
-			  vop->connector_type);
-		goto out;
-	};
-	VOP_CTRL_SET(vop, out_mode, vop->connector_out_mode);
-
 	val = 0x8;
 	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC) ? 0 : 1;
 	val |= (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC) ? 0 : (1 << 1);
-- 
1.7.9.5

[RFC PATCH 7/9] drm/rockchip: force enable vop when do mode setting

[Mark Yao]

When do mode setting, mean that we want to enable display output,
but sometimes, vop_crtc_enable is after mode_set, we can't allow
that, so force enable vop in mode setting.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index c65b454..7c07537 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1110,6 +1110,7 @@ static void vop_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	u16 vact_end = vact_st + vdisplay;
 	uint32_t val;
 
+	vop_crtc_enable(crtc);
 	/*
 	 * If dclk rate is zero, mean that scanout is stop,
 	 * we don't need wait any more.
-- 
1.7.9.5

[RFC PATCH 7/9] drm/rockchip: force enable vop when do mode setting

[Thierry Reding]

On Tue, Dec 01, 2015 at 11:32:01AM +0800, Mark Yao wrote:
> When do mode setting, mean that we want to enable display output,
> but sometimes, vop_crtc_enable is after mode_set, we can't allow
> that, so force enable vop in mode setting.
> 
> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index c65b454..7c07537 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1110,6 +1110,7 @@ static void vop_crtc_mode_set_nofb(struct drm_crtc *crtc)
>  	u16 vact_end = vact_st + vdisplay;
>  	uint32_t val;
>  
> +	vop_crtc_enable(crtc);
>  	/*
>  	 * If dclk rate is zero, mean that scanout is stop,
>  	 * we don't need wait any more.

Have you considered simply moving everything into ->enable()? That's
what I did for Tegra, for much the same reasons that you gave in the
commit message. Doing so gives you a much simpler call graph. Really
the only thing you need to do is move around the code, and perhaps a
different way to get ahold of the display mode, but you can use the
Tegra driver as a reference for how to do that.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151202/8fd3a088/attachment.sig>

[RFC PATCH 7/9] drm/rockchip: force enable vop when do mode setting

[Daniel Vetter]

On Wed, Dec 02, 2015 at 05:55:36PM +0100, Thierry Reding wrote:
> On Tue, Dec 01, 2015 at 11:32:01AM +0800, Mark Yao wrote:
> > When do mode setting, mean that we want to enable display output,
> > but sometimes, vop_crtc_enable is after mode_set, we can't allow
> > that, so force enable vop in mode setting.
> > 
> > Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> > ---
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index c65b454..7c07537 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -1110,6 +1110,7 @@ static void vop_crtc_mode_set_nofb(struct drm_crtc *crtc)
> >  	u16 vact_end = vact_st + vdisplay;
> >  	uint32_t val;
> >  
> > +	vop_crtc_enable(crtc);
> >  	/*
> >  	 * If dclk rate is zero, mean that scanout is stop,
> >  	 * we don't need wait any more.
> 
> Have you considered simply moving everything into ->enable()? That's
> what I did for Tegra, for much the same reasons that you gave in the
> commit message. Doing so gives you a much simpler call graph. Really
> the only thing you need to do is move around the code, and perhaps a
> different way to get ahold of the display mode, but you can use the
> Tegra driver as a reference for how to do that.

Yeah if writing mode related registers requires the thing to be on on your
hw then you can't use the ->mode_set hooks. Those are explicitly called
when everything is off (not just sometimes, at least with atomic helpers).

Like Thierry said the recommendation is to just shovel that code into
->enable hooks. ->mode_set_nofb is mostly there to support easier
transition for drivers which started with the legacy crtc helpers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[RFC PATCH 7/9] drm/rockchip: force enable vop when do mode setting

[Mark yao]

On 2015?12?03? 06:17, Daniel Vetter wrote:
> On Wed, Dec 02, 2015 at 05:55:36PM +0100, Thierry Reding wrote:
>> On Tue, Dec 01, 2015 at 11:32:01AM +0800, Mark Yao wrote:
>>> When do mode setting, mean that we want to enable display output,
>>> but sometimes, vop_crtc_enable is after mode_set, we can't allow
>>> that, so force enable vop in mode setting.
>>>
>>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>>> ---
>>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> index c65b454..7c07537 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> @@ -1110,6 +1110,7 @@ static void vop_crtc_mode_set_nofb(struct drm_crtc *crtc)
>>>   	u16 vact_end = vact_st + vdisplay;
>>>   	uint32_t val;
>>>   
>>> +	vop_crtc_enable(crtc);
>>>   	/*
>>>   	 * If dclk rate is zero, mean that scanout is stop,
>>>   	 * we don't need wait any more.
>> Have you considered simply moving everything into ->enable()? That's
>> what I did for Tegra, for much the same reasons that you gave in the
>> commit message. Doing so gives you a much simpler call graph. Really
>> the only thing you need to do is move around the code, and perhaps a
>> different way to get ahold of the display mode, but you can use the
>> Tegra driver as a reference for how to do that.
> Yeah if writing mode related registers requires the thing to be on on your
> hw then you can't use the ->mode_set hooks. Those are explicitly called
> when everything is off (not just sometimes, at least with atomic helpers).
>
> Like Thierry said the recommendation is to just shovel that code into
> ->enable hooks. ->mode_set_nofb is mostly there to support easier
> transition for drivers which started with the legacy crtc helpers.
> -Daniel

Good, thanks, actually it solve my confusion.

-- 
?ark Yao

[RFC PATCH 8/9] drm: bridge/dw_hdmi: Covert to support atomic API

[Mark Yao]

Fill atomic needed funcs with default atomic helper library.

Rockchip use dw_hdmi, and drm/rockchip will covert to atomic api,
we need dw_hdmi support atomic funcs.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/bridge/dw_hdmi.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 56de9f1..587065a 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -22,6 +22,7 @@
 
 #include <drm/drm_of.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_encoder_slave.h>
@@ -1515,11 +1516,14 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
 }
 
 static struct drm_connector_funcs dw_hdmi_connector_funcs = {
-	.dpms = drm_helper_connector_dpms,
+	.dpms = drm_atomic_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,
+	.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 struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
-- 
1.7.9.5

[RFC PATCH 8/9] drm: bridge/dw_hdmi: Covert to support atomic API

[Daniel Vetter]

On Tue, Dec 01, 2015 at 11:35:53AM +0800, Mark Yao wrote:
> Fill atomic needed funcs with default atomic helper library.
> 
> Rockchip use dw_hdmi, and drm/rockchip will covert to atomic api,
> we need dw_hdmi support atomic funcs.
> 
> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>

Aren't there drivers around which use dw_hdmi and which are still not yet
atomic? This would break them. I think we neeed two connector_func tables
and dw_hdmi needs to check for DRIVER_ATOMIC at runtime and pick the right
version.

The larger problem here is that "who should register the drm_connector" is
a bit an unsolved problem, since both the bridge and the driver should be
able to customize/adjust the drm_connector at the end of a bridge chain.
This here is just another example of this problem.
-Daniel

> ---
>  drivers/gpu/drm/bridge/dw_hdmi.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
> index 56de9f1..587065a 100644
> --- a/drivers/gpu/drm/bridge/dw_hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c
> @@ -22,6 +22,7 @@
>  
>  #include <drm/drm_of.h>
>  #include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_encoder_slave.h>
> @@ -1515,11 +1516,14 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
>  }
>  
>  static struct drm_connector_funcs dw_hdmi_connector_funcs = {
> -	.dpms = drm_helper_connector_dpms,
> +	.dpms = drm_atomic_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,
> +	.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 struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[RFC PATCH 8/9] drm: bridge/dw_hdmi: Covert to support atomic API

[Mark yao]

On 2015?12?01? 15:21, Daniel Vetter wrote:
> On Tue, Dec 01, 2015 at 11:35:53AM +0800, Mark Yao wrote:
>> Fill atomic needed funcs with default atomic helper library.
>>
>> Rockchip use dw_hdmi, and drm/rockchip will covert to atomic api,
>> we need dw_hdmi support atomic funcs.
>>
>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> Aren't there drivers around which use dw_hdmi and which are still not yet
> atomic? This would break them. I think we neeed two connector_func tables
> and dw_hdmi needs to check for DRIVER_ATOMIC at runtime and pick the right
> version.

Right, another drm driver use dw_hdmi is imx, not yet atomic, this would 
break it.
as you said, I would resend a patch to check DRIVER_ATOMIC at runtime.

Thanks
-Mark

>
> The larger problem here is that "who should register the drm_connector" is
> a bit an unsolved problem, since both the bridge and the driver should be
> able to customize/adjust the drm_connector at the end of a bridge chain.
> This here is just another example of this problem.
> -Daniel
>
>> ---
>>   drivers/gpu/drm/bridge/dw_hdmi.c |    6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
>> index 56de9f1..587065a 100644
>> --- a/drivers/gpu/drm/bridge/dw_hdmi.c
>> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c
>> @@ -22,6 +22,7 @@
>>   
>>   #include <drm/drm_of.h>
>>   #include <drm/drmP.h>
>> +#include <drm/drm_atomic_helper.h>
>>   #include <drm/drm_crtc_helper.h>
>>   #include <drm/drm_edid.h>
>>   #include <drm/drm_encoder_slave.h>
>> @@ -1515,11 +1516,14 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
>>   }
>>   
>>   static struct drm_connector_funcs dw_hdmi_connector_funcs = {
>> -	.dpms = drm_helper_connector_dpms,
>> +	.dpms = drm_atomic_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,
>> +	.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 struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
>> -- 
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: bridge/dw_hdmi: add atomic API support

[Mark Yao]

Fill atomic needed funcs with default atomic helper library.

Rockchip use dw_hdmi, and drm/rockchip will covert to atomic api,
we need dw_hdmi support atomic funcs.

Now another drm driver use dw_hdmi is imx, not yet atomic, so
check DRIVER_ATOMIC at runtime to spilt atomic and not atomic.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/bridge/dw_hdmi.c |   23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 56de9f1..dc0bdd4 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -22,6 +22,7 @@
 
 #include <drm/drm_of.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_encoder_slave.h>
@@ -1522,6 +1523,17 @@ static struct drm_connector_funcs dw_hdmi_connector_funcs = {
 	.force = dw_hdmi_connector_force,
 };
 
+static struct drm_connector_funcs dw_hdmi_atomic_connector_funcs = {
+	.dpms = drm_atomic_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,
+	.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 struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
 	.get_modes = dw_hdmi_connector_get_modes,
 	.mode_valid = dw_hdmi_connector_mode_valid,
@@ -1645,8 +1657,15 @@ static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi)
 
 	drm_connector_helper_add(&hdmi->connector,
 				 &dw_hdmi_connector_helper_funcs);
-	drm_connector_init(drm, &hdmi->connector, &dw_hdmi_connector_funcs,
-			   DRM_MODE_CONNECTOR_HDMIA);
+
+	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);
 
 	hdmi->connector.encoder = encoder;
 
-- 
1.7.9.5

[RFC PATCH 9/9] drm/rockchip: dw_hdmi: use encoder enable function

[Mark Yao]

encoder.enable is more compatible to atomic api than encoder.prepare/commit

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 80d6fc8..cfe052c 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -195,12 +195,15 @@ static void dw_hdmi_rockchip_encoder_mode_set(struct drm_encoder *encoder,
 {
 }
 
-static void dw_hdmi_rockchip_encoder_commit(struct drm_encoder *encoder)
+static void dw_hdmi_rockchip_encoder_enable(struct drm_encoder *encoder)
 {
 	struct rockchip_hdmi *hdmi = to_rockchip_hdmi(encoder);
 	u32 val;
 	int mux;
 
+	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
+				      ROCKCHIP_OUT_MODE_AAAA);
+
 	mux = rockchip_drm_encoder_get_mux_id(hdmi->dev->of_node, encoder);
 	if (mux)
 		val = HDMI_SEL_VOP_LIT | (HDMI_SEL_VOP_LIT << 16);
@@ -212,17 +215,10 @@ static void dw_hdmi_rockchip_encoder_commit(struct drm_encoder *encoder)
 		(mux) ? "LIT" : "BIG");
 }
 
-static void dw_hdmi_rockchip_encoder_prepare(struct drm_encoder *encoder)
-{
-	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
-				      ROCKCHIP_OUT_MODE_AAAA);
-}
-
 static struct drm_encoder_helper_funcs dw_hdmi_rockchip_encoder_helper_funcs = {
 	.mode_fixup = dw_hdmi_rockchip_encoder_mode_fixup,
 	.mode_set   = dw_hdmi_rockchip_encoder_mode_set,
-	.prepare    = dw_hdmi_rockchip_encoder_prepare,
-	.commit     = dw_hdmi_rockchip_encoder_commit,
+	.enable     = dw_hdmi_rockchip_encoder_enable,
 	.disable    = dw_hdmi_rockchip_encoder_disable,
 };
 
-- 
1.7.9.5