[PATCH 0/8] drm/rockchip: Flip wait clean-up

[PATCH 0/8] drm/rockchip: Flip wait clean-up

[Tomasz Figa]

The display controller found on Rockchip SoCs supported by Rockchip DRM
driver (VOP) is a bit problematic, because it does not provide hardware
vblank counter. Because vblank interrupt is used to feed the software
counter, the driver had custom code to wait for flip completion to avoid
race between atomic flush and vblank interrupt handler.

This, however, brought a different set of issues. In fact, even with the
custom wait code, there is stil a race between the handler and driver
state update (of the values used to compare with registers to determine
if flip has completed). On top of that, legacy cursor updates are not
implemented properly and have to wait for vblank to complete, which
is against the API specification.

This series attempts to clean up the driver from this custom waiting code,
eliminating related races and bringing correct handling of legacy cursor
plane. It also gives a nice effect of more than 100 lines of code removed.

This is a forward port of patches from 4.4 kernel used by ChromiumOS and
tested there. Even though the code base has not changed significantly, it
would be nice if someone with proper testing environment could give them
a try.

Based on for-next branch of Sean Paul's dogwood tree:
https://cgit.freedesktop.org/~seanpaul/dogwood/log/?h=for-next
git://people.freedesktop.org/~seanpaul/dogwood

Tomasz Figa (8):
  drm/rockchip: Clear interrupt status bits before enabling
  drm/rockchip: Get rid of some unnecessary code
  drm/rockchip: Avoid race with vblank count increment
  drm/rockchip: Unreference framebuffers from flip work
  drm/rockchip: Replace custom wait_for_vblanks with helper
  drm/rockchip: Do not enable vblank without event
  drm/rockchip: Always signal event in next vblank after cfg_done
  drm/rockchip: Kill vop_plane_state

 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   1 -
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  64 +------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 247 +++++++++++-----------------
 3 files changed, 95 insertions(+), 217 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/8] drm/rockchip: Clear interrupt status bits before enabling

[Tomasz Figa]

The enable register only masks the raw status bits to signal CPU
interrupt only for enabled interrupts. The status bits are activated
regardless of the enable register. This means that we might have an old
interrupt event queued, which we are not interested in. To avoid getting
a spurious interrupt signalled, we have to clear the old bit before we
update the enable register.

Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 209167b..7e811cf 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -414,6 +414,7 @@ static void vop_dsp_hold_valid_irq_enable(struct vop *vop)
 
 	spin_lock_irqsave(&vop->irq_lock, flags);
 
+	VOP_INTR_SET_TYPE(vop, clear, DSP_HOLD_VALID_INTR, 1);
 	VOP_INTR_SET_TYPE(vop, enable, DSP_HOLD_VALID_INTR, 1);
 
 	spin_unlock_irqrestore(&vop->irq_lock, flags);
@@ -479,6 +480,7 @@ static void vop_line_flag_irq_enable(struct vop *vop, int line_num)
 	spin_lock_irqsave(&vop->irq_lock, flags);
 
 	VOP_CTRL_SET(vop, line_flag_num[0], line_num);
+	VOP_INTR_SET_TYPE(vop, clear, LINE_FLAG_INTR, 1);
 	VOP_INTR_SET_TYPE(vop, enable, LINE_FLAG_INTR, 1);
 
 	spin_unlock_irqrestore(&vop->irq_lock, flags);
@@ -921,6 +923,7 @@ static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
 
 	spin_lock_irqsave(&vop->irq_lock, flags);
 
+	VOP_INTR_SET_TYPE(vop, clear, FS_INTR, 1);
 	VOP_INTR_SET_TYPE(vop, enable, FS_INTR, 1);
 
 	spin_unlock_irqrestore(&vop->irq_lock, flags);
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/8] drm/rockchip: Get rid of some unnecessary code

[Tomasz Figa]

Current code implements prepare_fb and cleanup_fb callbacks only to
grab/release fb references, which is already done by atomic framework
when creating/destryoing plane state. Let's remove these
unused bits.

Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 7e811cf..68988c6 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -641,22 +641,6 @@ static void vop_plane_destroy(struct drm_plane *plane)
 	drm_plane_cleanup(plane);
 }
 
-static int vop_plane_prepare_fb(struct drm_plane *plane,
-				struct drm_plane_state *new_state)
-{
-	if (plane->state->fb)
-		drm_framebuffer_reference(plane->state->fb);
-
-	return 0;
-}
-
-static void vop_plane_cleanup_fb(struct drm_plane *plane,
-				 struct drm_plane_state *old_state)
-{
-	if (old_state->fb)
-		drm_framebuffer_unreference(old_state->fb);
-}
-
 static int vop_plane_atomic_check(struct drm_plane *plane,
 			   struct drm_plane_state *state)
 {
@@ -849,8 +833,6 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
 }
 
 static const struct drm_plane_helper_funcs plane_helper_funcs = {
-	.prepare_fb = vop_plane_prepare_fb,
-	.cleanup_fb = vop_plane_cleanup_fb,
 	.atomic_check = vop_plane_atomic_check,
 	.atomic_update = vop_plane_atomic_update,
 	.atomic_disable = vop_plane_atomic_disable,
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 2/8] drm/rockchip: Get rid of some unnecessary code

[Mark yao]

On 2016年09月14日 20:54, Tomasz Figa wrote:
> Current code implements prepare_fb and cleanup_fb callbacks only to
> grab/release fb references, which is already done by atomic framework
> when creating/destryoing plane state. Let's remove these
> unused bits.
>
> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ------------------
>   1 file changed, 18 deletions(-)

Hi Tomasz

I think we can't get rid of the prepare_fb and cleanup_fb

see the reason:
commit 44d0237a26395ac94160cf23f32769013b365590
Author: Mark Yao <mark.yao@rock-chips.com>
Date:   Fri Apr 29 11:37:20 2016 +0800

     drm/rockchip: vop: fix iommu crash with async atomic

     After async atomic_commit callback, drm_atomic_clean_old_fb will
     clean all old fb, but because async, the old fb may be also on
     the vop hardware, dma will access the old fb buffer, clean old
     fb will cause iommu page fault.

     Reference the fb and unreference it when the fb actuall swap out
     from vop hardware.


> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 7e811cf..68988c6 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -641,22 +641,6 @@ static void vop_plane_destroy(struct drm_plane *plane)
>   	drm_plane_cleanup(plane);
>   }
>   
> -static int vop_plane_prepare_fb(struct drm_plane *plane,
> -				struct drm_plane_state *new_state)
> -{
> -	if (plane->state->fb)
> -		drm_framebuffer_reference(plane->state->fb);
> -
> -	return 0;
> -}
> -
> -static void vop_plane_cleanup_fb(struct drm_plane *plane,
> -				 struct drm_plane_state *old_state)
> -{
> -	if (old_state->fb)
> -		drm_framebuffer_unreference(old_state->fb);
> -}
> -
>   static int vop_plane_atomic_check(struct drm_plane *plane,
>   			   struct drm_plane_state *state)
>   {
> @@ -849,8 +833,6 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>   }
>   
>   static const struct drm_plane_helper_funcs plane_helper_funcs = {
> -	.prepare_fb = vop_plane_prepare_fb,
> -	.cleanup_fb = vop_plane_cleanup_fb,
>   	.atomic_check = vop_plane_atomic_check,
>   	.atomic_update = vop_plane_atomic_update,
>   	.atomic_disable = vop_plane_atomic_disable,


-- 
Ｍark Yao

Re: [PATCH 2/8] drm/rockchip: Get rid of some unnecessary code

[Tomasz Figa]

Hi Mark,

On Sun, Sep 18, 2016 at 10:50 AM, Mark yao <mark.yao@rock-chips.com> wrote:
> On 2016年09月14日 20:54, Tomasz Figa wrote:
>>
>> Current code implements prepare_fb and cleanup_fb callbacks only to
>> grab/release fb references, which is already done by atomic framework
>> when creating/destryoing plane state. Let's remove these
>> unused bits.
>>
>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
>> ---
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ------------------
>>   1 file changed, 18 deletions(-)
>
>
> Hi Tomasz
>
> I think we can't get rid of the prepare_fb and cleanup_fb

I think I have to disagree. Please see below for detailed explanation.

>
> see the reason:
> commit 44d0237a26395ac94160cf23f32769013b365590
> Author: Mark Yao <mark.yao@rock-chips.com>
> Date:   Fri Apr 29 11:37:20 2016 +0800
>
>     drm/rockchip: vop: fix iommu crash with async atomic
>
>     After async atomic_commit callback, drm_atomic_clean_old_fb will
>     clean all old fb, but because async, the old fb may be also on
>     the vop hardware, dma will access the old fb buffer, clean old
>     fb will cause iommu page fault.

I think the above is not quite right. Atomic plane state holds a
reference to its fb and old state is not supposed to be destroyed
until the flip completes.

Indeed current rockchip_atomic_commit implementation has following
order of calls: rockchip_atomic_wait_for_complete(),
drm_atomic_helper_cleanup_planes(), drm_atomic_state_free(). This
means that .cleanup_fb() is called from
drm_atomic_helper_cleanup_planes() just before drm_atomic_state_free()
will release references by destroying old plane states. Note that both
are called already after rockchip_atomic_wait_for_complete(), so it
should be already safe to free the old fbs.

So the above fix doesn't really do anything, possibly just covers the
race condition of the original wait for vblank function by delaying
drm_atomic_state_free() a bit.

Moreover, the whole series has been thoroughly tested in Chrome OS 4.4
kernel, including async commits. (There is still a possibility some
newer upstream changes slightly modified the semantics, but I couldn't
find such difference. Actually one of the advantages of atomic helpers
was to avoid manually refcounting the fbs from the driver.)

Best regards,
Tomasz

Re: [PATCH 2/8] drm/rockchip: Get rid of some unnecessary code

[Mark yao]

On 2016年09月18日 12:01, Tomasz Figa wrote:
> Hi Mark,
>
> On Sun, Sep 18, 2016 at 10:50 AM, Mark yao <mark.yao@rock-chips.com> wrote:
>> On 2016年09月14日 20:54, Tomasz Figa wrote:
>>> Current code implements prepare_fb and cleanup_fb callbacks only to
>>> grab/release fb references, which is already done by atomic framework
>>> when creating/destryoing plane state. Let's remove these
>>> unused bits.
>>>
>>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
>>> ---
>>>    drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ------------------
>>>    1 file changed, 18 deletions(-)
>>
>> Hi Tomasz
>>
>> I think we can't get rid of the prepare_fb and cleanup_fb
> I think I have to disagree. Please see below for detailed explanation.
>
>> see the reason:
>> commit 44d0237a26395ac94160cf23f32769013b365590
>> Author: Mark Yao <mark.yao@rock-chips.com>
>> Date:   Fri Apr 29 11:37:20 2016 +0800
>>
>>      drm/rockchip: vop: fix iommu crash with async atomic
>>
>>      After async atomic_commit callback, drm_atomic_clean_old_fb will
>>      clean all old fb, but because async, the old fb may be also on
>>      the vop hardware, dma will access the old fb buffer, clean old
>>      fb will cause iommu page fault.
> I think the above is not quite right. Atomic plane state holds a
> reference to its fb and old state is not supposed to be destroyed
> until the flip completes.
>
> Indeed current rockchip_atomic_commit implementation has following
> order of calls: rockchip_atomic_wait_for_complete(),
> drm_atomic_helper_cleanup_planes(), drm_atomic_state_free(). This
> means that .cleanup_fb() is called from
> drm_atomic_helper_cleanup_planes() just before drm_atomic_state_free()
> will release references by destroying old plane states. Note that both
> are called already after rockchip_atomic_wait_for_complete(), so it
> should be already safe to free the old fbs.
>
> So the above fix doesn't really do anything, possibly just covers the
> race condition of the original wait for vblank function by delaying
> drm_atomic_state_free() a bit.
>
> Moreover, the whole series has been thoroughly tested in Chrome OS 4.4
> kernel, including async commits. (There is still a possibility some
> newer upstream changes slightly modified the semantics, but I couldn't
> find such difference. Actually one of the advantages of atomic helpers
> was to avoid manually refcounting the fbs from the driver.)
>
> Best regards,
> Tomasz
>
Hi Tomasz

You are right, plane_duplicate_state/plane_destroy_state already protect 
the old fbs.
we can get rid of prepare_fb and cleanup_fb.

-- 
Ｍark Yao

[PATCH 3/8] drm/rockchip: Avoid race with vblank count increment

[Tomasz Figa]

Since VOP does not have a hardware vblank count register, the ongoing
commit might be racing with a requested vblank interrupt, which would
increment the software vblank counter before the changes being committed
actually happen.

To avoid this, we can extend .atomic_flush(), so after it sets cfg_done
bit, it polls the vblank interrupt bit until it's inactive to make sure
that any old vblank interrupt gets to the handler and then uses
synchronize_irq(vop->irq) to make sure the handler finishes running.

The polling case should happen very rarely, but even if, the total wait
time should be relatively low and in practice almost equal to the vop
hardirq handler running time.

Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 34 +++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 68988c6..f989440 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -23,6 +23,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
+#include <linux/iopoll.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
@@ -1063,6 +1064,32 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
 	rockchip_drm_psr_activate(&vop->crtc);
 }
 
+static bool vop_fs_irq_is_pending(struct vop *vop)
+{
+	return VOP_INTR_GET_TYPE(vop, status, FS_INTR);
+}
+
+static void vop_wait_for_irq_handler(struct vop *vop)
+{
+	bool pending;
+	int ret;
+
+	/*
+	 * Spin until frame start interrupt status bit goes low, which means
+	 * that interrupt handler was invoked and cleared it. The timeout of
+	 * 10 msecs is really too long, but it is just a safety measure if
+	 * something goes really wrong. The wait will only happen in the very
+	 * unlikely case of a vblank happening exactly at the same time and
+	 * shouldn't exceed microseconds range.
+	 */
+	ret = readx_poll_timeout_atomic(vop_fs_irq_is_pending, vop, pending,
+					!pending, 0, 10 * 1000);
+	if (ret)
+		DRM_DEV_ERROR(vop->dev, "VOP vblank IRQ stuck for 10 ms\n");
+
+	synchronize_irq(vop->irq);
+}
+
 static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 				  struct drm_crtc_state *old_crtc_state)
 {
@@ -1076,6 +1103,13 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 	vop_cfg_done(vop);
 
 	spin_unlock(&vop->reg_lock);
+
+	/*
+	 * There is a (rather unlikely) possiblity that a vblank interrupt
+	 * fired before we set the cfg_done bit. To avoid spuriously
+	 * signalling flip completion we need to wait for it to finish.
+	 */
+	vop_wait_for_irq_handler(vop);
 }
 
 static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
-- 
2.8.0.rc3.226.g39d4020

[PATCH 4/8] drm/rockchip: Unreference framebuffers from flip work

[Tomasz Figa]

Currently the driver waits for vblank and then unreferences old
framebuffers from atomic commit code path. This is however breaking the
legacy cursor API, which requires the updates to be fully asynchronous.
Instead of just adding a special case for cursor, we can have actually
smaller amount of code to unreference any changed framebuffer from a
flip work.

Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 41 +++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index f989440..86829f5 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -17,6 +17,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_flip_work.h>
 #include <drm/drm_plane_helper.h>
 
 #include <linux/kernel.h>
@@ -89,6 +90,10 @@
 #define to_vop_win(x) container_of(x, struct vop_win, base)
 #define to_vop_plane_state(x) container_of(x, struct vop_plane_state, base)
 
+enum vop_pending {
+	VOP_PENDING_FB_UNREF,
+};
+
 struct vop_plane_state {
 	struct drm_plane_state base;
 	int format;
@@ -122,6 +127,9 @@ struct vop {
 	/* protected by dev->event_lock */
 	struct drm_pending_vblank_event *event;
 
+	struct drm_flip_work fb_unref_work;
+	unsigned long pending;
+
 	struct completion line_flag_completion;
 
 	const struct vop_data *data;
@@ -1093,7 +1101,11 @@ static void vop_wait_for_irq_handler(struct vop *vop)
 static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 				  struct drm_crtc_state *old_crtc_state)
 {
+	struct drm_atomic_state *old_state = old_crtc_state->state;
+	struct drm_plane_state *old_plane_state;
 	struct vop *vop = to_vop(crtc);
+	struct drm_plane *plane;
+	int i;
 
 	if (WARN_ON(!vop->is_enabled))
 		return;
@@ -1110,6 +1122,19 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 	 * signalling flip completion we need to wait for it to finish.
 	 */
 	vop_wait_for_irq_handler(vop);
+
+	for_each_plane_in_state(old_state, plane, old_plane_state, i) {
+		if (!old_plane_state->fb)
+			continue;
+
+		if (old_plane_state->fb == plane->state->fb)
+			continue;
+
+		drm_framebuffer_reference(old_plane_state->fb);
+		drm_flip_work_queue(&vop->fb_unref_work, old_plane_state->fb);
+		set_bit(VOP_PENDING_FB_UNREF, &vop->pending);
+		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+	}
 }
 
 static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
@@ -1185,6 +1210,15 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
 	.atomic_destroy_state = vop_crtc_destroy_state,
 };
 
+static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
+{
+	struct vop *vop = container_of(work, struct vop, fb_unref_work);
+	struct drm_framebuffer *fb = val;
+
+	drm_crtc_vblank_put(&vop->crtc);
+	drm_framebuffer_unreference(fb);
+}
+
 static bool vop_win_pending_is_complete(struct vop_win *vop_win)
 {
 	dma_addr_t yrgb_mst;
@@ -1223,6 +1257,9 @@ static void vop_handle_vblank(struct vop *vop)
 
 	if (!completion_done(&vop->wait_update_complete))
 		complete(&vop->wait_update_complete);
+
+	if (test_and_clear_bit(VOP_PENDING_FB_UNREF, &vop->pending))
+		drm_flip_work_commit(&vop->fb_unref_work, system_unbound_wq);
 }
 
 static irqreturn_t vop_isr(int irq, void *data)
@@ -1361,6 +1398,9 @@ static int vop_create_crtc(struct vop *vop)
 		goto err_cleanup_crtc;
 	}
 
+	drm_flip_work_init(&vop->fb_unref_work, "fb_unref",
+			   vop_fb_unref_worker);
+
 	init_completion(&vop->dsp_hold_completion);
 	init_completion(&vop->wait_update_complete);
 	init_completion(&vop->line_flag_completion);
@@ -1404,6 +1444,7 @@ static void vop_destroy_crtc(struct vop *vop)
 	 * references the CRTC.
 	 */
 	drm_crtc_cleanup(crtc);
+	drm_flip_work_cleanup(&vop->fb_unref_work);
 }
 
 static int vop_initial(struct vop *vop)
-- 
2.8.0.rc3.226.g39d4020

[PATCH 5/8] drm/rockchip: Replace custom wait_for_vblanks with helper

[Tomasz Figa]

Currently the driver uses a custom function to wait for flip to complete
after an atomic commit. It was needed before because of two problems:
 - there is no hardware vblank counter, so the original helper would
   have a race condition with the vblank interrupt,
 - the driver didn't support unreferencing cursor framebuffers
   asynchronously to the commit, which was what the helper expected.
Since both problems have been solved by previous patches, we can now
make the driver use the generic helper and remove custom waiting code.

Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |  1 -
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c  | 64 +----------------------------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 14 -------
 3 files changed, 1 insertion(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 5c69845..fb6226c 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -39,7 +39,6 @@ struct drm_connector;
 struct rockchip_crtc_funcs {
 	int (*enable_vblank)(struct drm_crtc *crtc);
 	void (*disable_vblank)(struct drm_crtc *crtc);
-	void (*wait_for_update)(struct drm_crtc *crtc);
 };
 
 struct rockchip_crtc_state {
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 9890ecc..0f6eda0 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -174,68 +174,6 @@ static void rockchip_drm_output_poll_changed(struct drm_device *dev)
 		drm_fb_helper_hotplug_event(fb_helper);
 }
 
-static void rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
-{
-	struct rockchip_drm_private *priv = crtc->dev->dev_private;
-	int pipe = drm_crtc_index(crtc);
-	const struct rockchip_crtc_funcs *crtc_funcs = priv->crtc_funcs[pipe];
-
-	if (crtc_funcs && crtc_funcs->wait_for_update)
-		crtc_funcs->wait_for_update(crtc);
-}
-
-/*
- * We can't use drm_atomic_helper_wait_for_vblanks() because rk3288 and rk3066
- * have hardware counters for neither vblanks nor scanlines, which results in
- * a race where:
- *				| <-- HW vsync irq and reg take effect
- *	       plane_commit --> |
- *	get_vblank and wait --> |
- *				| <-- handle_vblank, vblank->count + 1
- *		 cleanup_fb --> |
- *		iommu crash --> |
- *				| <-- HW vsync irq and reg take effect
- *
- * This function is equivalent but uses rockchip_crtc_wait_for_update() instead
- * of waiting for vblank_count to change.
- */
-static void
-rockchip_atomic_wait_for_complete(struct drm_device *dev, struct drm_atomic_state *old_state)
-{
-	struct drm_crtc_state *old_crtc_state;
-	struct drm_crtc *crtc;
-	int i, ret;
-
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
-		/* No one cares about the old state, so abuse it for tracking
-		 * and store whether we hold a vblank reference (and should do a
-		 * vblank wait) in the ->enable boolean.
-		 */
-		old_crtc_state->enable = false;
-
-		if (!crtc->state->active)
-			continue;
-
-		if (!drm_atomic_helper_framebuffer_changed(dev,
-				old_state, crtc))
-			continue;
-
-		ret = drm_crtc_vblank_get(crtc);
-		if (ret != 0)
-			continue;
-
-		old_crtc_state->enable = true;
-	}
-
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
-		if (!old_crtc_state->enable)
-			continue;
-
-		rockchip_crtc_wait_for_update(crtc);
-		drm_crtc_vblank_put(crtc);
-	}
-}
-
 static void
 rockchip_atomic_commit_tail(struct drm_atomic_state *state)
 {
@@ -250,7 +188,7 @@ rockchip_atomic_commit_tail(struct drm_atomic_state *state)
 
 	drm_atomic_helper_commit_hw_done(state);
 
-	rockchip_atomic_wait_for_complete(dev, state);
+	drm_atomic_helper_wait_for_vblanks(dev, state);
 
 	drm_atomic_helper_cleanup_planes(dev, state);
 }
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 86829f5..af9ddbe 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -122,7 +122,6 @@ struct vop {
 	struct mutex vsync_mutex;
 	bool vsync_work_pending;
 	struct completion dsp_hold_completion;
-	struct completion wait_update_complete;
 
 	/* protected by dev->event_lock */
 	struct drm_pending_vblank_event *event;
@@ -937,18 +936,9 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc)
 	spin_unlock_irqrestore(&vop->irq_lock, flags);
 }
 
-static void vop_crtc_wait_for_update(struct drm_crtc *crtc)
-{
-	struct vop *vop = to_vop(crtc);
-
-	reinit_completion(&vop->wait_update_complete);
-	WARN_ON(!wait_for_completion_timeout(&vop->wait_update_complete, 100));
-}
-
 static const struct rockchip_crtc_funcs private_crtc_funcs = {
 	.enable_vblank = vop_crtc_enable_vblank,
 	.disable_vblank = vop_crtc_disable_vblank,
-	.wait_for_update = vop_crtc_wait_for_update,
 };
 
 static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
@@ -1255,9 +1245,6 @@ static void vop_handle_vblank(struct vop *vop)
 	}
 	spin_unlock_irqrestore(&drm->event_lock, flags);
 
-	if (!completion_done(&vop->wait_update_complete))
-		complete(&vop->wait_update_complete);
-
 	if (test_and_clear_bit(VOP_PENDING_FB_UNREF, &vop->pending))
 		drm_flip_work_commit(&vop->fb_unref_work, system_unbound_wq);
 }
@@ -1402,7 +1389,6 @@ static int vop_create_crtc(struct vop *vop)
 			   vop_fb_unref_worker);
 
 	init_completion(&vop->dsp_hold_completion);
-	init_completion(&vop->wait_update_complete);
 	init_completion(&vop->line_flag_completion);
 	crtc->port = port;
 	rockchip_register_crtc_funcs(crtc, &private_crtc_funcs);
-- 
2.8.0.rc3.226.g39d4020

[PATCH 6/8] drm/rockchip: Do not enable vblank without event

[Tomasz Figa]

Originally we needed to enable vblank for any atomic commit to kick the
PSR machine, but that was changed and we no longer need to do so from
a vblank interrupt. Let's return to original behavior of enabling
vblank only if it is really necessary.

This essentially reverts commit 5b6804034ae9 ("drm/rockchip: Enable
vblank without event").

Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index af9ddbe..bb7a865 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -116,7 +116,6 @@ struct vop {
 	struct device *dev;
 	struct drm_device *drm_dev;
 	bool is_enabled;
-	bool vblank_active;
 
 	/* mutex vsync_ work */
 	struct mutex vsync_mutex;
@@ -1135,11 +1134,10 @@ static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
 	rockchip_drm_psr_flush(crtc);
 
 	spin_lock_irq(&crtc->dev->event_lock);
-	vop->vblank_active = true;
-	WARN_ON(drm_crtc_vblank_get(crtc) != 0);
-	WARN_ON(vop->event);
-
 	if (crtc->state->event) {
+		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+		WARN_ON(vop->event);
+
 		vop->event = crtc->state->event;
 		crtc->state->event = NULL;
 	}
@@ -1236,12 +1234,8 @@ static void vop_handle_vblank(struct vop *vop)
 	spin_lock_irqsave(&drm->event_lock, flags);
 	if (vop->event) {
 		drm_crtc_send_vblank_event(crtc, vop->event);
-		vop->event = NULL;
-
-	}
-	if (vop->vblank_active) {
-		vop->vblank_active = false;
 		drm_crtc_vblank_put(crtc);
+		vop->event = NULL;
 	}
 	spin_unlock_irqrestore(&drm->event_lock, flags);
 
@@ -1518,7 +1512,6 @@ static int vop_initial(struct vop *vop)
 	clk_disable(vop->aclk);
 
 	vop->is_enabled = false;
-	vop->vblank_active = false;
 
 	return 0;
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH 7/8] drm/rockchip: Always signal event in next vblank after cfg_done

[Tomasz Figa]

This patch makes the driver send the pending vblank event in next vblank
following the commit, relying on vblank signalling improvements done in
previous patches. This gives us vblank events that always represent the
real moment of changes hitting on the screen (which was the case only
for complete FB changes before) and lets us remove the manual window
update check.

Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 54 ++++++-----------------------
 1 file changed, 10 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index bb7a865..cacdffb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -105,10 +105,6 @@ struct vop_win {
 	struct drm_plane base;
 	const struct vop_win_data *data;
 	struct vop *vop;
-
-	/* protected by dev->event_lock */
-	bool enable;
-	dma_addr_t yrgb_mst;
 };
 
 struct vop {
@@ -716,11 +712,6 @@ static void vop_plane_atomic_disable(struct drm_plane *plane,
 	if (!old_state->crtc)
 		return;
 
-	spin_lock_irq(&plane->dev->event_lock);
-	vop_win->enable = false;
-	vop_win->yrgb_mst = 0;
-	spin_unlock_irq(&plane->dev->event_lock);
-
 	spin_lock(&vop->reg_lock);
 
 	VOP_WIN_SET(vop, win, enable, 0);
@@ -784,11 +775,6 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
 	offset += (src->y1 >> 16) * fb->pitches[0];
 	vop_plane_state->yrgb_mst = rk_obj->dma_addr + offset + fb->offsets[0];
 
-	spin_lock_irq(&plane->dev->event_lock);
-	vop_win->enable = true;
-	vop_win->yrgb_mst = vop_plane_state->yrgb_mst;
-	spin_unlock_irq(&plane->dev->event_lock);
-
 	spin_lock(&vop->reg_lock);
 
 	VOP_WIN_SET(vop, win, format, vop_plane_state->format);
@@ -1112,6 +1098,16 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 	 */
 	vop_wait_for_irq_handler(vop);
 
+	spin_lock_irq(&crtc->dev->event_lock);
+	if (crtc->state->event) {
+		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+		WARN_ON(vop->event);
+
+		vop->event = crtc->state->event;
+		crtc->state->event = NULL;
+	}
+	spin_unlock_irq(&crtc->dev->event_lock);
+
 	for_each_plane_in_state(old_state, plane, old_plane_state, i) {
 		if (!old_plane_state->fb)
 			continue;
@@ -1129,19 +1125,7 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
 				  struct drm_crtc_state *old_crtc_state)
 {
-	struct vop *vop = to_vop(crtc);
-
 	rockchip_drm_psr_flush(crtc);
-
-	spin_lock_irq(&crtc->dev->event_lock);
-	if (crtc->state->event) {
-		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
-		WARN_ON(vop->event);
-
-		vop->event = crtc->state->event;
-		crtc->state->event = NULL;
-	}
-	spin_unlock_irq(&crtc->dev->event_lock);
 }
 
 static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
@@ -1207,29 +1191,11 @@ static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
 	drm_framebuffer_unreference(fb);
 }
 
-static bool vop_win_pending_is_complete(struct vop_win *vop_win)
-{
-	dma_addr_t yrgb_mst;
-
-	if (!vop_win->enable)
-		return VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0;
-
-	yrgb_mst = VOP_WIN_GET_YRGBADDR(vop_win->vop, vop_win->data);
-
-	return yrgb_mst == vop_win->yrgb_mst;
-}
-
 static void vop_handle_vblank(struct vop *vop)
 {
 	struct drm_device *drm = vop->drm_dev;
 	struct drm_crtc *crtc = &vop->crtc;
 	unsigned long flags;
-	int i;
-
-	for (i = 0; i < vop->data->win_size; i++) {
-		if (!vop_win_pending_is_complete(&vop->win[i]))
-			return;
-	}
 
 	spin_lock_irqsave(&drm->event_lock, flags);
 	if (vop->event) {
-- 
2.8.0.rc3.226.g39d4020

[PATCH 8/8] drm/rockchip: Kill vop_plane_state

[Tomasz Figa]

After changes introduced by last patches, there is no useful data stored
in vop_plane_state struct.  Let's remove it and make the driver use
generic plane state alone.

Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 94 +++++------------------------
 1 file changed, 15 insertions(+), 79 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index cacdffb..57650c9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -88,19 +88,11 @@
 
 #define to_vop(x) container_of(x, struct vop, crtc)
 #define to_vop_win(x) container_of(x, struct vop_win, base)
-#define to_vop_plane_state(x) container_of(x, struct vop_plane_state, base)
 
 enum vop_pending {
 	VOP_PENDING_FB_UNREF,
 };
 
-struct vop_plane_state {
-	struct drm_plane_state base;
-	int format;
-	dma_addr_t yrgb_mst;
-	bool enable;
-};
-
 struct vop_win {
 	struct drm_plane base;
 	const struct vop_win_data *data;
@@ -651,7 +643,6 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
 	struct drm_crtc_state *crtc_state;
 	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;
 	int ret;
 	struct drm_rect clip;
@@ -661,7 +652,7 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
 					DRM_PLANE_HELPER_NO_SCALING;
 
 	if (!crtc || !fb)
-		goto out_disable;
+		return 0;
 
 	crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
 	if (WARN_ON(!crtc_state))
@@ -679,11 +670,11 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
 		return ret;
 
 	if (!state->visible)
-		goto out_disable;
+		return 0;
 
-	vop_plane_state->format = vop_convert_format(fb->pixel_format);
-	if (vop_plane_state->format < 0)
-		return vop_plane_state->format;
+	ret = vop_convert_format(fb->pixel_format);
+	if (ret < 0)
+		return ret;
 
 	/*
 	 * Src.x1 can be odd when do clip, but yuv plane start point
@@ -692,19 +683,12 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
 	if (is_yuv_support(fb->pixel_format) && ((state->src.x1 >> 16) % 2))
 		return -EINVAL;
 
-	vop_plane_state->enable = true;
-
-	return 0;
-
-out_disable:
-	vop_plane_state->enable = false;
 	return 0;
 }
 
 static void vop_plane_atomic_disable(struct drm_plane *plane,
 				     struct drm_plane_state *old_state)
 {
-	struct vop_plane_state *vop_plane_state = to_vop_plane_state(old_state);
 	struct vop_win *vop_win = to_vop_win(plane);
 	const struct vop_win_data *win = vop_win->data;
 	struct vop *vop = to_vop(old_state->crtc);
@@ -717,8 +701,6 @@ static void vop_plane_atomic_disable(struct drm_plane *plane,
 	VOP_WIN_SET(vop, win, enable, 0);
 
 	spin_unlock(&vop->reg_lock);
-
-	vop_plane_state->enable = false;
 }
 
 static void vop_plane_atomic_update(struct drm_plane *plane,
@@ -727,7 +709,6 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
 	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;
@@ -742,6 +723,7 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
 	dma_addr_t dma_addr;
 	uint32_t val;
 	bool rb_swap;
+	int format;
 
 	/*
 	 * can't update plane when vop is disabled.
@@ -752,7 +734,7 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
 	if (WARN_ON(!vop->is_enabled))
 		return;
 
-	if (!vop_plane_state->enable) {
+	if (!state->visible) {
 		vop_plane_atomic_disable(plane, old_state);
 		return;
 	}
@@ -773,13 +755,15 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
 
 	offset = (src->x1 >> 16) * drm_format_plane_cpp(fb->pixel_format, 0);
 	offset += (src->y1 >> 16) * fb->pitches[0];
-	vop_plane_state->yrgb_mst = rk_obj->dma_addr + offset + fb->offsets[0];
+	dma_addr = rk_obj->dma_addr + offset + fb->offsets[0];
+
+	format = vop_convert_format(fb->pixel_format);
 
 	spin_lock(&vop->reg_lock);
 
-	VOP_WIN_SET(vop, win, format, vop_plane_state->format);
+	VOP_WIN_SET(vop, win, format, format);
 	VOP_WIN_SET(vop, win, yrgb_vir, fb->pitches[0] >> 2);
-	VOP_WIN_SET(vop, win, yrgb_mst, vop_plane_state->yrgb_mst);
+	VOP_WIN_SET(vop, win, yrgb_mst, dma_addr);
 	if (is_yuv_support(fb->pixel_format)) {
 		int hsub = drm_format_horz_chroma_subsampling(fb->pixel_format);
 		int vsub = drm_format_vert_chroma_subsampling(fb->pixel_format);
@@ -831,61 +815,13 @@ static const struct drm_plane_helper_funcs plane_helper_funcs = {
 	.atomic_disable = vop_plane_atomic_disable,
 };
 
-static void vop_atomic_plane_reset(struct drm_plane *plane)
-{
-	struct vop_plane_state *vop_plane_state =
-					to_vop_plane_state(plane->state);
-
-	if (plane->state && plane->state->fb)
-		drm_framebuffer_unreference(plane->state->fb);
-
-	kfree(vop_plane_state);
-	vop_plane_state = kzalloc(sizeof(*vop_plane_state), GFP_KERNEL);
-	if (!vop_plane_state)
-		return;
-
-	plane->state = &vop_plane_state->base;
-	plane->state->plane = plane;
-}
-
-static 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;
-
-	if (WARN_ON(!plane->state))
-		return NULL;
-
-	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;
-
-	__drm_atomic_helper_plane_duplicate_state(plane,
-						  &vop_plane_state->base);
-
-	return &vop_plane_state->base;
-}
-
-static void vop_atomic_plane_destroy_state(struct drm_plane *plane,
-					   struct drm_plane_state *state)
-{
-	struct vop_plane_state *vop_state = to_vop_plane_state(state);
-
-	__drm_atomic_helper_plane_destroy_state(state);
-
-	kfree(vop_state);
-}
-
 static const struct drm_plane_funcs vop_plane_funcs = {
 	.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,
+	.reset = drm_atomic_helper_plane_reset,
+	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
 };
 
 static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 0/8] drm/rockchip: Flip wait clean-up

[Sean Paul]

On Wed, Sep 14, 2016 at 8:54 AM, Tomasz Figa <tfiga@chromium.org> wrote:
> The display controller found on Rockchip SoCs supported by Rockchip DRM
> driver (VOP) is a bit problematic, because it does not provide hardware
> vblank counter. Because vblank interrupt is used to feed the software
> counter, the driver had custom code to wait for flip completion to avoid
> race between atomic flush and vblank interrupt handler.
>
> This, however, brought a different set of issues. In fact, even with the
> custom wait code, there is stil a race between the handler and driver
> state update (of the values used to compare with registers to determine
> if flip has completed). On top of that, legacy cursor updates are not
> implemented properly and have to wait for vblank to complete, which
> is against the API specification.
>
> This series attempts to clean up the driver from this custom waiting code,
> eliminating related races and bringing correct handling of legacy cursor
> plane. It also gives a nice effect of more than 100 lines of code removed.
>
> This is a forward port of patches from 4.4 kernel used by ChromiumOS and
> tested there. Even though the code base has not changed significantly, it
> would be nice if someone with proper testing environment could give them
> a try.
>
> Based on for-next branch of Sean Paul's dogwood tree:
> https://cgit.freedesktop.org/~seanpaul/dogwood/log/?h=for-next
> git://people.freedesktop.org/~seanpaul/dogwood
>

Thanks for the patches, Tomasz.

I'll queue them up in my rockchip for-next branch.

Sean



> Tomasz Figa (8):
>   drm/rockchip: Clear interrupt status bits before enabling
>   drm/rockchip: Get rid of some unnecessary code
>   drm/rockchip: Avoid race with vblank count increment
>   drm/rockchip: Unreference framebuffers from flip work
>   drm/rockchip: Replace custom wait_for_vblanks with helper
>   drm/rockchip: Do not enable vblank without event
>   drm/rockchip: Always signal event in next vblank after cfg_done
>   drm/rockchip: Kill vop_plane_state
>
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   1 -
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  64 +------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 247 +++++++++++-----------------
>  3 files changed, 95 insertions(+), 217 deletions(-)
>
> --
> 2.8.0.rc3.226.g39d4020
>