drm/rockchip: don't wait for vblank if fb hasn't changed

As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies on cursor ioctls being unsynced. Converting the rockchip driver to atomic has significantly impacted cursor performance by making every cursor update wait for vblank.

By skipping the vblank sync when the framebuffer has not changed (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the common case of moving the cursor and only need to delay the cursor ioctl when the cursor icon changes.

I originally inserted a check on legacy_cursor_update as well, but that caused a storm of iommu page faults. I didn't investigate the cause of those since this change gives enough of a performance improvement for my use case.

This is RFC because of that and because the framebuffer_changed() function is copied from drm_atomic_helper.c as a quick way to test the result.

Signed-off-by John Keeping <[email protected]>

---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +9 -0

index f784488..8fd9821 100644
@@ -177,8 +177,28 @@ static void rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 		crtc_funcs->wait_for_update(crtc);
 }
 
+static bool framebuffer_changed(struct drm_device *dev,
+				struct drm_atomic_state *old_state,
+				struct drm_crtc *crtc)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state;
+	int i;
+
+	for_each_plane_in_state(old_state, plane, old_plane_state, i) {

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +6 -2

+
 static void
-rockchip_atomic_wait_for_complete(struct drm_atomic_state *old_state)
+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;

@@ -194,6 +214,9 @@ rockchip_atomic_wait_for_complete(struct drm_atomic_state *old_state)
 		if (!crtc->state->active)
 			continue;
 
+		if (!framebuffer_changed(dev, old_state, crtc))
+			continue;
+
 		ret = drm_crtc_vblank_get(crtc);
 		if (ret != 0)
 			continue;

@@ -241,7 +264,7 @@ rockchip_atomic_commit_complete(struct rockchip_atomic_commit *commit)
 
 	drm_atomic_helper_commit_planes(dev, state, true);
 
-	rockchip_atomic_wait_for_complete(state);
+	rockchip_atomic_wait_for_complete(dev, state);
 
 	drm_atomic_helper_cleanup_planes(dev, state);
 

Signature · 6 lines

2.7.0.rc3.140.g520a093

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 43 lines · John Keeping

As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies on cursor ioctls being unsynced. Converting the rockchip driver to atomic has significantly impacted cursor performance by making every cursor update wait for vblank.

By skipping the vblank sync when the framebuffer has not changed (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the common case of moving the cursor and only need to delay the cursor ioctl when the cursor icon changes.

I originally inserted a check on legacy_cursor_update as well, but that caused a storm of iommu page faults. I didn't investigate the cause of those since this change gives enough of a performance improvement for my use case.

This is RFC because of that and because the framebuffer_changed() function is copied from drm_atomic_helper.c as a quick way to test the result.

Signed-off-by John Keeping <[email protected]>

---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27

+++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +9 -0

@@ -177,8 +177,28 @@ static void
 rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 crtc_funcs->wait_for_update(crtc); }
 
+static bool framebuffer_changed(struct drm_device *dev,
+				struct drm_atomic_state *old_state,
+				struct drm_crtc *crtc)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state;
+	int i;
+
+	for_each_plane_in_state(old_state, plane, old_plane_state,
 i) {

drivers/gpu/drm/rockchip/rockchip_drm_fb.c static void · line 199

Quoted 10 lines

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +10 -0

+		if (plane->state->crtc != crtc &&
+		    old_plane_state->crtc != crtc)
+			continue;
+
+		if (plane->state->fb != old_plane_state->fb)
+			return true;
+	}
+
+	return false;
+}  

Please don't hand-roll logic that affects semantics like this. Instead please use drm_atomic_helper_wait_for_vblanks(), which should do this correctly for you.

If that's not the case then we need to improve the generic helper, or figure out what's different with rockhip.

Quoted 33 lines

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +5 -2

+
 static void
-rockchip_atomic_wait_for_complete(struct drm_atomic_state
 *old_state) +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;

@@ -194,6 +214,9 @@ rockchip_atomic_wait_for_complete(struct
 drm_atomic_state *old_state) if (!crtc->state->active)
 			continue;
 
+		if (!framebuffer_changed(dev, old_state, crtc))
+			continue;
+
 		ret = drm_crtc_vblank_get(crtc);
 		if (ret != 0)
 			continue;

@@ -241,7 +264,7 @@ rockchip_atomic_commit_complete(struct
 rockchip_atomic_commit *commit) 
 	drm_atomic_helper_commit_planes(dev, state, true);
 
-	rockchip_atomic_wait_for_complete(state);
+	rockchip_atomic_wait_for_complete(dev, state);
 
 	drm_atomic_helper_cleanup_planes(dev, state);
 

Signature · 6 lines

2.7.0.rc3.140.g520a093

_______________________________________________
dri-devel mailing list
dri-devel at lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel  

Quoted 59 lines · Daniel Vetter

Quoted 43 lines · John Keeping

As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies on cursor ioctls being unsynced. Converting the rockchip driver to atomic has significantly impacted cursor performance by making every cursor update wait for vblank.

By skipping the vblank sync when the framebuffer has not changed (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the common case of moving the cursor and only need to delay the cursor ioctl when the cursor icon changes.

I originally inserted a check on legacy_cursor_update as well, but that caused a storm of iommu page faults. I didn't investigate the cause of those since this change gives enough of a performance improvement for my use case.

This is RFC because of that and because the framebuffer_changed() function is copied from drm_atomic_helper.c as a quick way to test the result.

Signed-off-by John Keeping <[email protected]>

---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27

+++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +9 -0

@@ -177,8 +177,28 @@ static void
 rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 crtc_funcs->wait_for_update(crtc); }
 
+static bool framebuffer_changed(struct drm_device *dev,
+				struct drm_atomic_state *old_state,
+				struct drm_crtc *crtc)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state;
+	int i;
+
+	for_each_plane_in_state(old_state, plane, old_plane_state,
 i) {

drivers/gpu/drm/rockchip/rockchip_drm_fb.c static void · line 199

Quoted 10 lines

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +10 -0

+		if (plane->state->crtc != crtc &&
+		    old_plane_state->crtc != crtc)
+			continue;
+
+		if (plane->state->fb != old_plane_state->fb)
+			return true;
+	}
+
+	return false;
+}  

Please don't hand-roll logic that affects semantics like this. Instead please use drm_atomic_helper_wait_for_vblanks(), which should do this correctly for you.

Quoted 2 lines

If that's not the case then we need to improve the generic helper, or figure out what's different with rockhip.

According to commit 63ebb9f (drm/rockchip: Convert to support atomic API) it's because rockchip doesn't have a hardware vblank counter.

I'm not entirely clear on why this prevents the use of drm_atomic_helper_wait_for_vblanks().

Quoted 67 lines · John Keeping

Quoted 59 lines · Daniel Vetter

Quoted 43 lines · John Keeping

As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies on cursor ioctls being unsynced. Converting the rockchip driver to atomic has significantly impacted cursor performance by making every cursor update wait for vblank.

By skipping the vblank sync when the framebuffer has not changed (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the common case of moving the cursor and only need to delay the cursor ioctl when the cursor icon changes.

I originally inserted a check on legacy_cursor_update as well, but that caused a storm of iommu page faults. I didn't investigate the cause of those since this change gives enough of a performance improvement for my use case.

This is RFC because of that and because the framebuffer_changed() function is copied from drm_atomic_helper.c as a quick way to test the result.

Signed-off-by John Keeping <[email protected]>

---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27

+++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +9 -0

@@ -177,8 +177,28 @@ static void
 rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 crtc_funcs->wait_for_update(crtc); }
 
+static bool framebuffer_changed(struct drm_device *dev,
+				struct drm_atomic_state *old_state,
+				struct drm_crtc *crtc)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state;
+	int i;
+
+	for_each_plane_in_state(old_state, plane, old_plane_state,
 i) {

drivers/gpu/drm/rockchip/rockchip_drm_fb.c static void · line 199

Quoted 10 lines

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +10 -0

+		if (plane->state->crtc != crtc &&
+		    old_plane_state->crtc != crtc)
+			continue;
+
+		if (plane->state->fb != old_plane_state->fb)
+			return true;
+	}
+
+	return false;
+}    

Please don't hand-roll logic that affects semantics like this. Instead please use drm_atomic_helper_wait_for_vblanks(), which should do this correctly for you.

Quoted 2 lines

If that's not the case then we need to improve the generic helper, or figure out what's different with rockhip.

According to commit 63ebb9f (drm/rockchip: Convert to support atomic API) it's because rockchip doesn't have a hardware vblank counter.

Quoted 2 lines

I'm not entirely clear on why this prevents the use of drm_atomic_helper_wait_for_vblanks().

Hm, that commit isn't terribly helpful. If that's really needed then imo I think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()" helper that's used by both. But since rockchip does vblank_get/put calls I'd hope vblanks actually work correctly. And then the helper should work too.

Quoted 68 lines · Daniel Vetter

Quoted 67 lines · John Keeping

Quoted 59 lines · Daniel Vetter

Quoted 43 lines · John Keeping

As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies on cursor ioctls being unsynced. Converting the rockchip driver to atomic has significantly impacted cursor performance by making every cursor update wait for vblank.

By skipping the vblank sync when the framebuffer has not changed (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the common case of moving the cursor and only need to delay the cursor ioctl when the cursor icon changes.

I originally inserted a check on legacy_cursor_update as well, but that caused a storm of iommu page faults. I didn't investigate the cause of those since this change gives enough of a performance improvement for my use case.

This is RFC because of that and because the framebuffer_changed() function is copied from drm_atomic_helper.c as a quick way to test the result.

Signed-off-by John Keeping <[email protected]>

---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27

+++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +9 -0

@@ -177,8 +177,28 @@ static void
 rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 crtc_funcs->wait_for_update(crtc); }
 
+static bool framebuffer_changed(struct drm_device *dev,
+				struct drm_atomic_state *old_state,
+				struct drm_crtc *crtc)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state;
+	int i;
+
+	for_each_plane_in_state(old_state, plane, old_plane_state,
 i) {

drivers/gpu/drm/rockchip/rockchip_drm_fb.c static void · line 199

Quoted 10 lines

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +10 -0

+		if (plane->state->crtc != crtc &&
+		    old_plane_state->crtc != crtc)
+			continue;
+
+		if (plane->state->fb != old_plane_state->fb)
+			return true;
+	}
+
+	return false;
+}    

Please don't hand-roll logic that affects semantics like this. Instead please use drm_atomic_helper_wait_for_vblanks(), which should do this correctly for you.

Quoted 2 lines

If that's not the case then we need to improve the generic helper, or figure out what's different with rockhip.

According to commit 63ebb9f (drm/rockchip: Convert to support atomic API) it's because rockchip doesn't have a hardware vblank counter.

Quoted 8 lines

Quoted 2 lines

I'm not entirely clear on why this prevents the use of drm_atomic_helper_wait_for_vblanks().

Hm, that commit isn't terribly helpful. If that's really needed then imo I think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()" helper that's used by both. But since rockchip does vblank_get/put calls I'd hope vblanks actually work correctly. And then the helper should work too.

I tried switching the call to rockchip_crtc_wait_for_update() to drm_atomic_helper_wait_for_vblanks() and it works fine until I switch the buffer associated with a cursor, at which point I get iommu page faults, presumably because the GEM buffer is unreferenced too early.

AFAICT the buffer will be released via drm_atomic_state_free() unconditionally, but I suspect I'm missing something since that would mean every driver would hit a similar problem.

Quoted 84 lines · John Keeping

Quoted 68 lines · Daniel Vetter

Quoted 67 lines · John Keeping

Quoted 59 lines · Daniel Vetter

Quoted 43 lines · John Keeping

As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies on cursor ioctls being unsynced. Converting the rockchip driver to atomic has significantly impacted cursor performance by making every cursor update wait for vblank.

By skipping the vblank sync when the framebuffer has not changed (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the common case of moving the cursor and only need to delay the cursor ioctl when the cursor icon changes.

I originally inserted a check on legacy_cursor_update as well, but that caused a storm of iommu page faults. I didn't investigate the cause of those since this change gives enough of a performance improvement for my use case.

This is RFC because of that and because the framebuffer_changed() function is copied from drm_atomic_helper.c as a quick way to test the result.

Signed-off-by John Keeping <[email protected]>

---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27

+++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +9 -0

@@ -177,8 +177,28 @@ static void
 rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 crtc_funcs->wait_for_update(crtc); }
 
+static bool framebuffer_changed(struct drm_device *dev,
+				struct drm_atomic_state *old_state,
+				struct drm_crtc *crtc)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state;
+	int i;
+
+	for_each_plane_in_state(old_state, plane, old_plane_state,
 i) {

drivers/gpu/drm/rockchip/rockchip_drm_fb.c static void · line 199

Quoted 10 lines

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +10 -0

+		if (plane->state->crtc != crtc &&
+		    old_plane_state->crtc != crtc)
+			continue;
+
+		if (plane->state->fb != old_plane_state->fb)
+			return true;
+	}
+
+	return false;
+}      

Please don't hand-roll logic that affects semantics like this. Instead please use drm_atomic_helper_wait_for_vblanks(), which should do this correctly for you.

Quoted 2 lines

If that's not the case then we need to improve the generic helper, or figure out what's different with rockhip.

According to commit 63ebb9f (drm/rockchip: Convert to support atomic API) it's because rockchip doesn't have a hardware vblank counter.

Quoted 8 lines

Quoted 2 lines

I'm not entirely clear on why this prevents the use of drm_atomic_helper_wait_for_vblanks().

Hm, that commit isn't terribly helpful. If that's really needed then imo I think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()" helper that's used by both. But since rockchip does vblank_get/put calls I'd hope vblanks actually work correctly. And then the helper should work too.

I tried switching the call to rockchip_crtc_wait_for_update() to drm_atomic_helper_wait_for_vblanks() and it works fine until I switch the buffer associated with a cursor, at which point I get iommu page faults, presumably because the GEM buffer is unreferenced too early.

Quoted 3 lines

AFAICT the buffer will be released via drm_atomic_state_free() unconditionally, but I suspect I'm missing something since that would mean every driver would hit a similar problem.

Yeah, with the helper we always skip, which means when the cursor bo changes you indeed unmap too early. So can't even share the overall condition, but we could definitely share the little framebuffer_changed helper.

Quoted 85 lines · Daniel Vetter

Quoted 84 lines · John Keeping

Quoted 68 lines · Daniel Vetter

Quoted 67 lines · John Keeping

Quoted 59 lines · Daniel Vetter

Quoted 43 lines · John Keeping

As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies on cursor ioctls being unsynced. Converting the rockchip driver to atomic has significantly impacted cursor performance by making every cursor update wait for vblank.

By skipping the vblank sync when the framebuffer has not changed (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the common case of moving the cursor and only need to delay the cursor ioctl when the cursor icon changes.

I originally inserted a check on legacy_cursor_update as well, but that caused a storm of iommu page faults. I didn't investigate the cause of those since this change gives enough of a performance improvement for my use case.

This is RFC because of that and because the framebuffer_changed() function is copied from drm_atomic_helper.c as a quick way to test the result.

Signed-off-by John Keeping <[email protected]>

---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27

+++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c

(quoted diff) +9 -0

@@ -177,8 +177,28 @@ static void
 rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 crtc_funcs->wait_for_update(crtc); }
 
+static bool framebuffer_changed(struct drm_device *dev,
+				struct drm_atomic_state *old_state,
+				struct drm_crtc *crtc)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state;
+	int i;
+
+	for_each_plane_in_state(old_state, plane, old_plane_state,
 i) {

Review static void · line 199

Quoted 10 lines

(quoted diff) +10 -0

+		if (plane->state->crtc != crtc &&
+		    old_plane_state->crtc != crtc)
+			continue;
+
+		if (plane->state->fb != old_plane_state->fb)
+			return true;
+	}
+
+	return false;
+}      

Please don't hand-roll logic that affects semantics like this. Instead please use drm_atomic_helper_wait_for_vblanks(), which should do this correctly for you.

Quoted 2 lines

If that's not the case then we need to improve the generic helper, or figure out what's different with rockhip.

According to commit 63ebb9f (drm/rockchip: Convert to support atomic API) it's because rockchip doesn't have a hardware vblank counter.

Quoted 8 lines

Quoted 2 lines

I'm not entirely clear on why this prevents the use of drm_atomic_helper_wait_for_vblanks().

Hm, that commit isn't terribly helpful. If that's really needed then imo I think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()" helper that's used by both. But since rockchip does vblank_get/put calls I'd hope vblanks actually work correctly. And then the helper should work too.

I tried switching the call to rockchip_crtc_wait_for_update() to drm_atomic_helper_wait_for_vblanks() and it works fine until I switch the buffer associated with a cursor, at which point I get iommu page faults, presumably because the GEM buffer is unreferenced too early.

Quoted 8 lines

Quoted 3 lines

AFAICT the buffer will be released via drm_atomic_state_free() unconditionally, but I suspect I'm missing something since that would mean every driver would hit a similar problem.

Yeah, with the helper we always skip, which means when the cursor bo changes you indeed unmap too early. So can't even share the overall condition, but we could definitely share the little framebuffer_changed helper.

That leaves me with the question: why do other atomic drivers work?

If drm_atomic_helper_wait_for_vblanks() skipping vblanks results in the cursor bo being unmapped too early for rockchip, why is it not unmapped too early for all of the other drivers using that helper?

Quoted 96 lines · John Keeping

Quoted 85 lines · Daniel Vetter

Quoted 84 lines · John Keeping

Quoted 68 lines · Daniel Vetter

Quoted 67 lines · John Keeping

Quoted 59 lines · Daniel Vetter

Quoted 43 lines · John Keeping

As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies on cursor ioctls being unsynced. Converting the rockchip driver to atomic has significantly impacted cursor performance by making every cursor update wait for vblank.

By skipping the vblank sync when the framebuffer has not changed (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the common case of moving the cursor and only need to delay the cursor ioctl when the cursor icon changes.

I originally inserted a check on legacy_cursor_update as well, but that caused a storm of iommu page faults. I didn't investigate the cause of those since this change gives enough of a performance improvement for my use case.

This is RFC because of that and because the framebuffer_changed() function is copied from drm_atomic_helper.c as a quick way to test the result.

Signed-off-by John Keeping <[email protected]>

---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27

+++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c

(quoted diff) +9 -0

@@ -177,8 +177,28 @@ static void
 rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 crtc_funcs->wait_for_update(crtc); }
 
+static bool framebuffer_changed(struct drm_device *dev,
+				struct drm_atomic_state *old_state,
+				struct drm_crtc *crtc)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state;
+	int i;
+
+	for_each_plane_in_state(old_state, plane, old_plane_state,
 i) {

Review static void · line 199

Quoted 10 lines

(quoted diff) +10 -0

+		if (plane->state->crtc != crtc &&
+		    old_plane_state->crtc != crtc)
+			continue;
+
+		if (plane->state->fb != old_plane_state->fb)
+			return true;
+	}
+
+	return false;
+}        

Please don't hand-roll logic that affects semantics like this. Instead please use drm_atomic_helper_wait_for_vblanks(), which should do this correctly for you.

Quoted 2 lines

If that's not the case then we need to improve the generic helper, or figure out what's different with rockhip.

According to commit 63ebb9f (drm/rockchip: Convert to support atomic API) it's because rockchip doesn't have a hardware vblank counter.

Quoted 8 lines

Quoted 2 lines

I'm not entirely clear on why this prevents the use of drm_atomic_helper_wait_for_vblanks().

Hm, that commit isn't terribly helpful. If that's really needed then imo I think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()" helper that's used by both. But since rockchip does vblank_get/put calls I'd hope vblanks actually work correctly. And then the helper should work too.

I tried switching the call to rockchip_crtc_wait_for_update() to drm_atomic_helper_wait_for_vblanks() and it works fine until I switch the buffer associated with a cursor, at which point I get iommu page faults, presumably because the GEM buffer is unreferenced too early.

Quoted 8 lines

Quoted 3 lines

AFAICT the buffer will be released via drm_atomic_state_free() unconditionally, but I suspect I'm missing something since that would mean every driver would hit a similar problem.

Yeah, with the helper we always skip, which means when the cursor bo changes you indeed unmap too early. So can't even share the overall condition, but we could definitely share the little framebuffer_changed helper.

Quoted 5 lines

That leaves me with the question: why do other atomic drivers work?

If drm_atomic_helper_wait_for_vblanks() skipping vblanks results in the cursor bo being unmapped too early for rockchip, why is it not unmapped too early for all of the other drivers using that helper?

It's unmapped too early for everyone, it's just that normally that doesn't result in a fireworks show. What we maybe could/should do is do the unmapping asynchronously, but that runs into the overall "current atomic helpers don't do async yet" problem. Might be a good point to start fixing this up though.

Quoted 93 lines · Daniel Vetter

Quoted 92 lines · John Keeping

Quoted 81 lines · Daniel Vetter

Quoted 80 lines · John Keeping

Quoted 66 lines · Daniel Vetter

Quoted 65 lines · John Keeping

Quoted 58 lines · Daniel Vetter

Quoted 43 lines · John Keeping

As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies on cursor ioctls being unsynced. Converting the rockchip driver to atomic has significantly impacted cursor performance by making every cursor update wait for vblank.

By skipping the vblank sync when the framebuffer has not changed (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the common case of moving the cursor and only need to delay the cursor ioctl when the cursor icon changes.

I originally inserted a check on legacy_cursor_update as well, but that caused a storm of iommu page faults. I didn't investigate the cause of those since this change gives enough of a performance improvement for my use case.

This is RFC because of that and because the framebuffer_changed() function is copied from drm_atomic_helper.c as a quick way to test the result.

Signed-off-by John Keeping <[email protected]>

---
  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27

+++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c

(quoted diff) +9 -0

@@ -177,8 +177,28 @@ static void
 rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 crtc_funcs->wait_for_update(crtc); }
 
+static bool framebuffer_changed(struct drm_device *dev,
+				struct drm_atomic_state *old_state,
+				struct drm_crtc *crtc)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state;
+	int i;
+
+	for_each_plane_in_state(old_state, plane, old_plane_state,
 i) {

Review static void · line 199

Quoted 10 lines

(quoted diff) +10 -0

+		if (plane->state->crtc != crtc &&
+		    old_plane_state->crtc != crtc)
+			continue;
+
+		if (plane->state->fb != old_plane_state->fb)
+			return true;
+	}
+
+	return false;
+}

Please don't hand-roll logic that affects semantics like this. Instead please use drm_atomic_helper_wait_for_vblanks(), which should do this correctly for you.

Quoted 2 lines

If that's not the case then we need to improve the generic helper, or figure out what's different with rockhip.

According to commit 63ebb9f (drm/rockchip: Convert to support atomic API) it's because rockchip doesn't have a hardware vblank counter.

Quoted 7 lines

Quoted 2 lines

I'm not entirely clear on why this prevents the use of drm_atomic_helper_wait_for_vblanks().

Hm, that commit isn't terribly helpful. If that's really needed then imo I think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()" helper that's used by both. But since rockchip does vblank_get/put calls I'd hope vblanks actually work correctly. And then the helper should work too.

I tried switching the call to rockchip_crtc_wait_for_update() to drm_atomic_helper_wait_for_vblanks() and it works fine until I switch the buffer associated with a cursor, at which point I get iommu page faults, presumably because the GEM buffer is unreferenced too early.

Quoted 7 lines

Quoted 3 lines

AFAICT the buffer will be released via drm_atomic_state_free() unconditionally, but I suspect I'm missing something since that would mean every driver would hit a similar problem.

Yeah, with the helper we always skip, which means when the cursor bo changes you indeed unmap too early. So can't even share the overall condition, but we could definitely share the little framebuffer_changed helper.

That leaves me with the question: why do other atomic drivers work?

Quoted 8 lines

Quoted 3 lines

If drm_atomic_helper_wait_for_vblanks() skipping vblanks results in the cursor bo being unmapped too early for rockchip, why is it not unmapped too early for all of the other drivers using that helper?

It's unmapped too early for everyone, it's just that normally that doesn't result in a fireworks show. What we maybe could/should do is do the unmapping asynchronously, but that runs into the overall "current atomic helpers don't do async yet" problem. Might be a good point to start fixing this up though.

OK, thanks, I think I'm beginning to understand how this all fits together.

It looks like there are two options for me to get reasonable cursor performance on rockchip in the short term:

1) Export the current framebuffer_changed() function as drm_atomic_helper_framebuffer_changed() and use it in rockchip_crtc_wait_for_update().

2) Add a mechanism to suppress the legacy_cursor_update check in drm_atomic_helper_wait_for_vblanks() and switch the rockchip driver over to it.

In both of these cases we're only restoring the unsynced cursor ioctls behaviour when the cursor is moved but it will still be expensive when the cursor bo changes. That gives sufficient performance in my testing.

Quoted 140 lines · John Keeping

Quoted 119 lines · Daniel Vetter

Quoted 112 lines · John Keeping

Quoted 100 lines · Daniel Vetter

Quoted 98 lines · John Keeping

Quoted 80 lines · Daniel Vetter

Quoted 72 lines · John Keeping

Quoted 65 lines · Daniel Vetter

Quoted 46 lines · John Keeping

As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies on cursor ioctls being unsynced. Converting the rockchip driver to atomic has significantly impacted cursor performance by making every cursor update wait for vblank.

By skipping the vblank sync when the framebuffer has not changed (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the common case of moving the cursor and only need to delay the cursor ioctl when the cursor icon changes.

I originally inserted a check on legacy_cursor_update as well, but that caused a storm of iommu page faults. I didn't investigate the
cause of those since this change gives enough of a performance improvement for my use case.

This is RFC because of that and because the framebuffer_changed() function is copied from drm_atomic_helper.c as a quick way to test the result.

Signed-off-by John Keeping <[email protected]>

---
  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27

+++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821
100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c

(quoted diff) +8 -0

@@ -177,8 +177,28 @@ static void
 rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 crtc_funcs->wait_for_update(crtc); }
  +static bool framebuffer_changed(struct drm_device *dev,
+                               struct drm_atomic_state
 *old_state,
+                               struct drm_crtc *crtc)
+{
+       struct drm_plane *plane;
+       struct drm_plane_state *old_plane_state;
+       int i;
+
+       for_each_plane_in_state(old_state, plane, old_plane_state,
 i) {

Review static void · line 199

Quoted 10 lines

(quoted diff) +10 -0

+               if (plane->state->crtc != crtc &&
+                   old_plane_state->crtc != crtc)
+                       continue;
+
+               if (plane->state->fb != old_plane_state->fb)
+                       return true;
+       }
+
+       return false;
+}

Please don't hand-roll logic that affects semantics like this. Instead
please use drm_atomic_helper_wait_for_vblanks(), which should do this
correctly for you.

Quoted 3 lines

If that's not the case then we need to improve the generic helper, or
figure out what's different with rockhip.

Quoted 5 lines

According to commit 63ebb9f (drm/rockchip: Convert to support atomic API) it's because rockchip doesn't have a hardware vblank counter.

I'm not entirely clear on why this prevents the use of drm_atomic_helper_wait_for_vblanks().

Quoted 9 lines

Hm, that commit isn't terribly helpful. If that's really needed then imo I
think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()"
helper that's used by both. But since rockchip does vblank_get/put calls
I'd hope vblanks actually work correctly. And then the helper should work
too.

I tried switching the call to rockchip_crtc_wait_for_update() to drm_atomic_helper_wait_for_vblanks() and it works fine until I switch the buffer associated with a cursor, at which point I get iommu page faults, presumably because the GEM buffer is unreferenced too early.

Quoted 8 lines

Quoted 3 lines

AFAICT the buffer will be released via drm_atomic_state_free() unconditionally, but I suspect I'm missing something since that would mean every driver would hit a similar problem.

Yeah, with the helper we always skip, which means when the cursor bo changes you indeed unmap too early. So can't even share the overall condition, but we could definitely share the little framebuffer_changed helper.

Quoted 5 lines

That leaves me with the question: why do other atomic drivers work?

If drm_atomic_helper_wait_for_vblanks() skipping vblanks results in the cursor bo being unmapped too early for rockchip, why is it not unmapped too early for all of the other drivers using that helper?

Quoted 7 lines

It's unmapped too early for everyone, it's just that normally that doesn't
result in a fireworks show. What we maybe could/should do is do the unmapping asynchronously, but that runs into the overall "current atomic helpers don't do async yet" problem. Might be a good point to start fixing
this up though.

OK, thanks, I think I'm beginning to understand how this all fits together.

It looks like there are two options for me to get reasonable cursor performance on rockchip in the short term:

1) Export the current framebuffer_changed() function as drm_atomic_helper_framebuffer_changed() and use it in rockchip_crtc_wait_for_update().

Quoted 10 lines

2) Add a mechanism to suppress the legacy_cursor_update check in drm_atomic_helper_wait_for_vblanks() and switch the rockchip driver over to it.

In both of these cases we're only restoring the unsynced cursor ioctls behaviour when the cursor is moved but it will still be expensive when the cursor bo changes. That gives sufficient performance in my testing.

Thanks for point that.

because rockchip not support hardware vblank counter, use drm_atomic_helper_wait_for_vblanks have under issues:

| <-- 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
there is no hardware vblank counter on rockchip vop, we can't ensure the consistency of reg take effect and vblank->count,
if plane commit hit into the period of reg take effect and vblank->count, cleanup_fb happen before old_fb swap out from vop,
then iommu crash.

That is why I special the wait_for_vblanks, we need check the reg really take effect before clean up old fb.
at vop_win_pending_is_complete function, check win enable and win address, to ensure that.

Not only rockchip drm do that thing:

exynos also check address before cleanup fb

        if (start == start_s)
            exynos_drm_crtc_finish_update(ctx->crtc, plane);

Thanks.

Quoted 161 lines · Mark yao

Quoted 124 lines · John Keeping

Quoted 105 lines · Daniel Vetter

Quoted 99 lines · John Keeping

Quoted 90 lines · Daniel Vetter

Quoted 89 lines · John Keeping

Quoted 73 lines · Daniel Vetter

Quoted 64 lines · John Keeping

Quoted 62 lines · Daniel Vetter

Quoted 45 lines · John Keeping

As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies on cursor ioctls being unsynced. Converting the rockchip driver to atomic has significantly impacted cursor performance by making every cursor update wait for vblank.

By skipping the vblank sync when the framebuffer has not changed (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the common case of moving the cursor and only need to delay the cursor ioctl when the cursor icon changes.

I originally inserted a check on legacy_cursor_update as well, but that caused a storm of iommu page faults. I didn't investigate the
cause of those since this change gives enough of a performance improvement for my use case.

This is RFC because of that and because the framebuffer_changed() function is copied from drm_atomic_helper.c as a quick way to test the result.

Signed-off-by John Keeping <[email protected]>

---
   drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27

+++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821
100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c

(quoted diff) +8 -0

@@ -177,8 +177,28 @@ static void
 rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 crtc_funcs->wait_for_update(crtc); }
   +static bool framebuffer_changed(struct drm_device *dev,
+                               struct drm_atomic_state
 *old_state,
+                               struct drm_crtc *crtc)
+{
+       struct drm_plane *plane;
+       struct drm_plane_state *old_plane_state;
+       int i;
+
+       for_each_plane_in_state(old_state, plane, old_plane_state,
 i) {

Review static void · line 199

Quoted 10 lines

(quoted diff) +10 -0

+               if (plane->state->crtc != crtc &&
+                   old_plane_state->crtc != crtc)
+                       continue;
+
+               if (plane->state->fb != old_plane_state->fb)
+                       return true;
+       }
+
+       return false;
+}

Please don't hand-roll logic that affects semantics like this. Instead
please use drm_atomic_helper_wait_for_vblanks(), which should do this
correctly for you.

Quoted 8 lines

Quoted 3 lines

If that's not the case then we need to improve the generic helper, or
figure out what's different with rockhip.

According to commit 63ebb9f (drm/rockchip: Convert to support atomic API) it's because rockchip doesn't have a hardware vblank counter.

I'm not entirely clear on why this prevents the use of drm_atomic_helper_wait_for_vblanks().

Quoted 9 lines

Hm, that commit isn't terribly helpful. If that's really needed then imo I
think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()"
helper that's used by both. But since rockchip does vblank_get/put calls
I'd hope vblanks actually work correctly. And then the helper should work
too.

I tried switching the call to rockchip_crtc_wait_for_update() to drm_atomic_helper_wait_for_vblanks() and it works fine until I switch the buffer associated with a cursor, at which point I get iommu page faults, presumably because the GEM buffer is unreferenced too early.

Quoted 7 lines

Quoted 3 lines

AFAICT the buffer will be released via drm_atomic_state_free() unconditionally, but I suspect I'm missing something since that would mean every driver would hit a similar problem.

Yeah, with the helper we always skip, which means when the cursor bo changes you indeed unmap too early. So can't even share the overall condition, but we could definitely share the little framebuffer_changed helper.

Quoted 5 lines

That leaves me with the question: why do other atomic drivers work?

If drm_atomic_helper_wait_for_vblanks() skipping vblanks results in the cursor bo being unmapped too early for rockchip, why is it not unmapped too early for all of the other drivers using that helper?

Quoted 7 lines

It's unmapped too early for everyone, it's just that normally that doesn't
result in a fireworks show. What we maybe could/should do is do the unmapping asynchronously, but that runs into the overall "current atomic helpers don't do async yet" problem. Might be a good point to start fixing
this up though.

OK, thanks, I think I'm beginning to understand how this all fits together.

It looks like there are two options for me to get reasonable cursor performance on rockchip in the short term:

1) Export the current framebuffer_changed() function as drm_atomic_helper_framebuffer_changed() and use it in rockchip_crtc_wait_for_update().

Quoted 10 lines

2) Add a mechanism to suppress the legacy_cursor_update check in drm_atomic_helper_wait_for_vblanks() and switch the rockchip driver over to it.

In both of these cases we're only restoring the unsynced cursor ioctls behaviour when the cursor is moved but it will still be expensive when the cursor bo changes. That gives sufficient performance in my testing.

Thanks for point that.

because rockchip not support hardware vblank counter, use drm_atomic_helper_wait_for_vblanks have under issues:

| <-- 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
there is no hardware vblank counter on rockchip vop, we can't ensure the consistency of reg take effect and vblank->count,
if plane commit hit into the period of reg take effect and vblank->count, cleanup_fb happen before old_fb swap out from vop,
then iommu crash.

That is why I special the wait_for_vblanks, we need check the reg really take effect before clean up old fb.
at vop_win_pending_is_complete function, check win enable and win address, to ensure that.

Quoted 7 lines

Not only rockchip drm do that thing:

exynos also check address before cleanup fb

         if (start == start_s)
             exynos_drm_crtc_finish_update(ctx->crtc, plane);

Thanks.

Do you have a scanline counter or something similar at least? Any other indication about how far along the chip is with scanning out? We use that in i915 to avoid races with the interrupt handler and detect this w/a scenario.

I think if you have a scanline counter then it should magically work, since the vblank code will realize that you're already past the last vblank interrupt and /should/ have incremented already. Or something like that.

Otherwise if this is common we might want to figure out how to solve this in a generic way. It's one of these problems that will make generic async support almost impossible.
-Daniel

Quoted 179 lines · Daniel Vetter

Quoted 161 lines · Mark yao

Quoted 124 lines · John Keeping

Quoted 105 lines · Daniel Vetter

Quoted 99 lines · John Keeping

Quoted 90 lines · Daniel Vetter

Quoted 89 lines · John Keeping

Quoted 73 lines · Daniel Vetter

Quoted 64 lines · John Keeping

Quoted 62 lines · Daniel Vetter

Quoted 45 lines · John Keeping

As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies on cursor ioctls being unsynced. Converting the rockchip driver to atomic has significantly impacted cursor performance by making every cursor update wait for vblank.

By skipping the vblank sync when the framebuffer has not changed (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the common case of moving the cursor and only need to delay the cursor ioctl when the cursor icon changes.

I originally inserted a check on legacy_cursor_update as well, but that caused a storm of iommu page faults. I didn't investigate the
cause of those since this change gives enough of a performance improvement for my use case.

This is RFC because of that and because the framebuffer_changed() function is copied from drm_atomic_helper.c as a quick way to test the result.

Signed-off-by John Keeping <[email protected]>

---
  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27

+++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821
100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c

(quoted diff) +8 -0

@@ -177,8 +177,28 @@ static void
 rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 crtc_funcs->wait_for_update(crtc); }
  +static bool framebuffer_changed(struct drm_device *dev,
+                               struct drm_atomic_state
 *old_state,
+                               struct drm_crtc *crtc)
+{
+       struct drm_plane *plane;
+       struct drm_plane_state *old_plane_state;
+       int i;
+
+       for_each_plane_in_state(old_state, plane, old_plane_state,
 i) {

Review static void · line 199

Quoted 10 lines

(quoted diff) +10 -0

+               if (plane->state->crtc != crtc &&
+                   old_plane_state->crtc != crtc)
+                       continue;
+
+               if (plane->state->fb != old_plane_state->fb)
+                       return true;
+       }
+
+       return false;
+}

Please don't hand-roll logic that affects semantics like this. Instead
please use drm_atomic_helper_wait_for_vblanks(), which should do this
correctly for you.

Quoted 8 lines

Quoted 3 lines

If that's not the case then we need to improve the generic helper, or
figure out what's different with rockhip.

According to commit 63ebb9f (drm/rockchip: Convert to support atomic API) it's because rockchip doesn't have a hardware vblank counter.

I'm not entirely clear on why this prevents the use of drm_atomic_helper_wait_for_vblanks().

Quoted 9 lines

Hm, that commit isn't terribly helpful. If that's really needed then imo I
think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()"
helper that's used by both. But since rockchip does vblank_get/put calls
I'd hope vblanks actually work correctly. And then the helper should work
too.

I tried switching the call to rockchip_crtc_wait_for_update() to drm_atomic_helper_wait_for_vblanks() and it works fine until I switch the buffer associated with a cursor, at which point I get iommu page faults, presumably because the GEM buffer is unreferenced too early.

Quoted 7 lines

Quoted 3 lines

AFAICT the buffer will be released via drm_atomic_state_free() unconditionally, but I suspect I'm missing something since that would mean every driver would hit a similar problem.

Yeah, with the helper we always skip, which means when the cursor bo changes you indeed unmap too early. So can't even share the overall condition, but we could definitely share the little framebuffer_changed helper.

Quoted 5 lines

That leaves me with the question: why do other atomic drivers work?

If drm_atomic_helper_wait_for_vblanks() skipping vblanks results in the cursor bo being unmapped too early for rockchip, why is it not unmapped too early for all of the other drivers using that helper?

Quoted 7 lines

It's unmapped too early for everyone, it's just that normally that doesn't
result in a fireworks show. What we maybe could/should do is do the unmapping asynchronously, but that runs into the overall "current atomic helpers don't do async yet" problem. Might be a good point to start fixing
this up though.

OK, thanks, I think I'm beginning to understand how this all fits together.

It looks like there are two options for me to get reasonable cursor performance on rockchip in the short term:

1) Export the current framebuffer_changed() function as drm_atomic_helper_framebuffer_changed() and use it in rockchip_crtc_wait_for_update().

Quoted 10 lines

2) Add a mechanism to suppress the legacy_cursor_update check in drm_atomic_helper_wait_for_vblanks() and switch the rockchip driver over to it.

In both of these cases we're only restoring the unsynced cursor ioctls behaviour when the cursor is moved but it will still be expensive when the cursor bo changes. That gives sufficient performance in my testing.

Thanks for point that.

because rockchip not support hardware vblank counter, use drm_atomic_helper_wait_for_vblanks have under issues:

| <-- 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
there is no hardware vblank counter on rockchip vop, we can't ensure the consistency of reg take effect and vblank->count,
if plane commit hit into the period of reg take effect and vblank->count, cleanup_fb happen before old_fb swap out from vop,
then iommu crash.

That is why I special the wait_for_vblanks, we need check the reg really take effect before clean up old fb.
at vop_win_pending_is_complete function, check win enable and win address, to ensure that.

Quoted 7 lines

Not only rockchip drm do that thing:

exynos also check address before cleanup fb

        if (start == start_s)
            exynos_drm_crtc_finish_update(ctx->crtc, plane);

Thanks.

Do you have a scanline counter or something similar at least? Any other indication about how far along the chip is with scanning out? We use that in i915 to avoid races with the interrupt handler and detect this w/a scenario.

I think if you have a scanline counter then it should magically work, since the vblank code will realize that you're already past the last vblank interrupt and /should/ have incremented already. Or something like that.

Quoted 4 lines

Otherwise if this is common we might want to figure out how to solve this in a generic way. It's one of these problems that will make generic async support almost impossible.
-Daniel

No, both rk3288 or rk3036 not support hardware vblank counter and scanline counter.

At android side, we use same way, check address and enable bit to ensure register take effect.

On future chips, scanline counter and hardware counter would be support, but not now.

Ugh. Oh well, there's not really anything we can do in core nore helpers to make this easier for drivers. This really only can be fixed sensibly at the hardware level.

So yeah I think exposing framebuffer_changed as a helper is the way to go here.

The Rockchip driver cannot use drm_atomic_helper_wait_for_vblanks() because it has hardware counters for neither vblanks nor scanlines.

In order to simplify re-implementing the functionality for this driver, export the framebuffer_changed() helper so it can be reused.

Signed-off-by John Keeping <[email protected]>

---
 drivers/gpu/drm/drm_atomic_helper.c | 24 ++++++++++++++++++++----
 include/drm/drm_atomic_helper.h     |  4 ++++
 2 files changed, 24 insertions(+), 4 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +20 -4

index 268d37f..7449293 100644
@@ -948,9 +948,23 @@ static void wait_for_fences(struct drm_device *dev,
 	}
 }
 
-static bool framebuffer_changed(struct drm_device *dev,
-				struct drm_atomic_state *old_state,
-				struct drm_crtc *crtc)
+/**
+ * drm_atomic_helper_framebuffer_changed - check if framebuffer has changed
+ * @dev: DRM device
+ * @old_state: atomic state object with old state structures
+ * @crtc: DRM crtc
+ *
+ * Checks whether the framebuffer used for this CRTC changes as a result of
+ * the atomic update.  This is useful for drivers which cannot use
+ * drm_atomic_helper_wait_for_vblanks() and need to reimplement its
+ * functionality.
+ *
+ * Returns:
+ * true if the framebuffer changed.
+ */
+bool drm_atomic_helper_framebuffer_changed(struct drm_device *dev,
+					   struct drm_atomic_state *old_state,
+					   struct drm_crtc *crtc)
 {
 	struct drm_plane *plane;
 	struct drm_plane_state *old_plane_state;

@@ -967,6 +981,7 @@ static bool framebuffer_changed(struct drm_device *dev,
 
 	return false;
 }
+EXPORT_SYMBOL(drm_atomic_helper_framebuffer_changed);
 
 /**
  * drm_atomic_helper_wait_for_vblanks - wait for vblank on crtcs

@@ -1001,7 +1016,8 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 		if (old_state->legacy_cursor_update)
 			continue;
 
-		if (!framebuffer_changed(dev, old_state, crtc))
+		if (!drm_atomic_helper_framebuffer_changed(dev,
+				old_state, crtc))
 			continue;
 
 		ret = drm_crtc_vblank_get(crtc);

include/drm/drm_atomic_helper.h +4 -0

index a286cce..74fce78 100644
@@ -42,6 +42,10 @@ int drm_atomic_helper_commit(struct drm_device *dev,
 			     struct drm_atomic_state *state,
 			     bool async);
 
+bool drm_atomic_helper_framebuffer_changed(struct drm_device *dev,
+					   struct drm_atomic_state *old_state,
+					   struct drm_crtc *crtc);
+
 void drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 					struct drm_atomic_state *old_state);
 

Signature · 1 line

2.7.0.226.gfe986fe

Signed-off-by John Keeping <[email protected]>

---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +0 -0

index 679d23a..b267ce4 100644

Quoted 13 lines · John Keeping

Signed-off-by John Keeping <[email protected]>

---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +0 -0

index 679d23a..b267ce4 100644
@@ -177,6 +177,12 @@ static void rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
 		crtc_funcs->wait_for_update(crtc);
 }
 

drivers/gpu/drm/rockchip/rockchip_drm_fb.c static void rockchip_crtc_wait_for_update(struct drm_crtc *crtc) · line 185

Quoted 6 lines

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +6 -0

+/*
+ * We can't use drm_atomic_helper_wait_for_vblanks() because rk3288 and rk3066
+ * have hardware counters for neither vblanks nor scanlines.  This function is
+ * equivalent but uses rockchip_crtc_wait_for_update() instead of waiting for
+ * vblank_count to change.
+ */  

This is kind of misleading. From reading earlier parts of the thread the reason why drm_atomic_helper_wait_for_vblanks() won't work is because it has a potential race condition that can't be detected unless you also have a vblank counter. However, the above comment makes it work like drm_atomic_helper_wait_for_vblanks() doesn't work in the absence of a vblank counter, which isn't quite true.

Quoted 14 lines · Thierry Reding

Quoted 13 lines · John Keeping

Signed-off-by John Keeping <[email protected]>

---
  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 6 ++++++
  1 file changed, 6 insertions(+)

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +0 -0

index 679d23a..b267ce4 100644
@@ -177,6 +177,12 @@ static void rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
  		crtc_funcs->wait_for_update(crtc);
  }
 

Quoted 12 lines

drivers/gpu/drm/rockchip/rockchip_drm_fb.c static void rockchip_crtc_wait_for_update(struct drm_crtc *crtc) · line 185

Quoted 6 lines

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +6 -0

+/*
+ * We can't use drm_atomic_helper_wait_for_vblanks() because rk3288 and rk3066
+ * have hardware counters for neither vblanks nor scanlines.  This function is
+ * equivalent but uses rockchip_crtc_wait_for_update() instead of waiting for
+ * vblank_count to change.
+ */

This is kind of misleading. From reading earlier parts of the thread the reason why drm_atomic_helper_wait_for_vblanks() won't work is because it has a potential race condition that can't be detected unless you also have a vblank counter. However, the above comment makes it work like drm_atomic_helper_wait_for_vblanks() doesn't work in the absence of a vblank counter, which isn't quite true.

How about something like this (using the sequence from Mark's message):

/*
* 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.
*/

The Rockchip driver cannot use drm_atomic_helper_wait_for_vblanks() because it has hardware counters for neither vblanks nor scanlines.

In order to simplify re-implementing the functionality for this driver, export the framebuffer_changed() helper so it can be reused.

Signed-off-by John Keeping <[email protected]>

Reviewed-by Daniel Vetter <[email protected]>

---
Unchanged since v1.

 drivers/gpu/drm/drm_atomic_helper.c | 24 ++++++++++++++++++++----
 include/drm/drm_atomic_helper.h     |  4 ++++
 2 files changed, 24 insertions(+), 4 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +20 -4

index 268d37f..7449293 100644
@@ -948,9 +948,23 @@ static void wait_for_fences(struct drm_device *dev,
 	}
 }
 
-static bool framebuffer_changed(struct drm_device *dev,
-				struct drm_atomic_state *old_state,
-				struct drm_crtc *crtc)
+/**
+ * drm_atomic_helper_framebuffer_changed - check if framebuffer has changed
+ * @dev: DRM device
+ * @old_state: atomic state object with old state structures
+ * @crtc: DRM crtc
+ *
+ * Checks whether the framebuffer used for this CRTC changes as a result of
+ * the atomic update.  This is useful for drivers which cannot use
+ * drm_atomic_helper_wait_for_vblanks() and need to reimplement its
+ * functionality.
+ *
+ * Returns:
+ * true if the framebuffer changed.
+ */
+bool drm_atomic_helper_framebuffer_changed(struct drm_device *dev,
+					   struct drm_atomic_state *old_state,
+					   struct drm_crtc *crtc)
 {
 	struct drm_plane *plane;
 	struct drm_plane_state *old_plane_state;

@@ -967,6 +981,7 @@ static bool framebuffer_changed(struct drm_device *dev,
 
 	return false;
 }
+EXPORT_SYMBOL(drm_atomic_helper_framebuffer_changed);
 
 /**
  * drm_atomic_helper_wait_for_vblanks - wait for vblank on crtcs

@@ -1001,7 +1016,8 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 		if (old_state->legacy_cursor_update)
 			continue;
 
-		if (!framebuffer_changed(dev, old_state, crtc))
+		if (!drm_atomic_helper_framebuffer_changed(dev,
+				old_state, crtc))
 			continue;
 
 		ret = drm_crtc_vblank_get(crtc);

include/drm/drm_atomic_helper.h +4 -0

index a286cce..74fce78 100644
@@ -42,6 +42,10 @@ int drm_atomic_helper_commit(struct drm_device *dev,
 			     struct drm_atomic_state *state,
 			     bool async);
 
+bool drm_atomic_helper_framebuffer_changed(struct drm_device *dev,
+					   struct drm_atomic_state *old_state,
+					   struct drm_crtc *crtc);
+
 void drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 					struct drm_atomic_state *old_state);
 

Signature · 1 line

2.7.0.226.gfe986fe

The first two patches are unchanged since v1 but the comment in the third has been expanded following Thierry's comments.

John Keeping (3):
drm/atomic-helper: Export framebuffer_changed() drm/rockchip: don't wait for vblank if fb hasn't changed drm/rockchip: explain why we can't wait_for_vblanks

  drivers/gpu/drm/drm_atomic_helper.c        | 24 ++++++++++++++++++++----
  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 23 +++++++++++++++++++++--
  include/drm/drm_atomic_helper.h            |  4 ++++
  3 files changed, 45 insertions(+), 6 deletions(-)

As commented in drm_atomic_helper_wait_for_vblanks(), userspace relies on cursor ioctls being unsynced. Converting the rockchip driver to atomic has significantly impacted cursor performance by making every cursor update wait for vblank.

By skipping the vblank sync when the framebuffer has not changed (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid this for the common case of moving the cursor and only need to delay the cursor ioctl when the cursor icon changes.

I originally inserted a check on legacy_cursor_update as well, but that caused a storm of iommu page faults. I didn't investigate the cause of those since this change gives enough of a performance improvement for my use case.

This is RFC because of that and because the framebuffer_changed() function is copied from drm_atomic_helper.c as a quick way to test the result.

Signed-off-by John Keeping <[email protected]>