[PATCH 0/8] drm/bochs: convert bochs to atomic mode-setting

[PATCH 0/8] drm/bochs: convert bochs to atomic mode-setting

[John Hunter]

From: Zhao Junwang <zhjwpku@gmail.com>

This patch series aim to convert DRM_BOCHS to atomic mode-setting.
I did this mostly reference Gustavo Padovan's patch series on
drm/exynos conversion.

Zhao Junwang (8):
  drm/bochs: phase 1 - use the transitional helpers
  drm/bochs: phase 2: wire up state reset(), duplicate() and destroy
  drm/bochs: stop looking at legacy state
  drm/bochs: phase 3: for plane updates: switch to atomic helper
    internally
  drm/bochs: phase 3: use atomic .set_config helper
  drm/bochs: phase 3: provide a custom ->atomic_commit implementation
  drm/bochs: phase 3: switch to drm_atomic_helper_page_flip
  drm/bochs: atomic dpms support

 drivers/gpu/drm/bochs/bochs.h     |    2 +
 drivers/gpu/drm/bochs/bochs_drv.c |    2 +-
 drivers/gpu/drm/bochs/bochs_kms.c |  221 +++++++++++++++++++++++--------------
 drivers/gpu/drm/bochs/bochs_mm.c  |    9 ++
 4 files changed, 148 insertions(+), 86 deletions(-)

-- 
1.7.10.4


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

[PATCH 1/8] drm/bochs: phase 1 - use the transitional helpers

[John Hunter]

From: Zhao Junwang <zhjwpku@gmail.com>

-register driver's own primary plane
-use drm_crtc_init_with_planes instead of drm_crtc_init

-split ->mode_set into:
	1. set the new hw mode
	2. update the primary plane (This is done by ->set_base)
-move what ->set_base do into ->atomic_update

-the new atomic infrastructure needs the ->mode_set_nofb callback
 to update CRTC timings before setting any plane

-since the ->cleanup_fb can't fail, set the interruptible argument
 of the ttm_bo_reserve to false, this make sure the ttm_bo_reserve
 can't fail

v2: -add a few checks to plane's ->atomic_check, using
     drm_plane_helper_check_update

v3: -polish the atomic_check, it does too much in v2, remove the
     ->disable_plane and ->set_config

v4: -use plane->state instead of old_state in
     bochs_plane_atomic_update

v5: -just return 0 in plane ->atomic_check, i.e. remove what we
     do in v2-v4

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
 drivers/gpu/drm/bochs/bochs.h     |    2 +
 drivers/gpu/drm/bochs/bochs_kms.c |  159 ++++++++++++++++++++++++-------------
 2 files changed, 108 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 71f2687..2f10480 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -5,6 +5,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_fb_helper.h>
 
 #include <drm/drm_gem.h>
@@ -72,6 +73,7 @@ struct bochs_device {
 
 	/* drm */
 	struct drm_device  *dev;
+	struct drm_plane primary;
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;
 	struct drm_connector connector;
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
index 26bcd03..1b66dd3 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -10,6 +10,11 @@
 
 static int defx = 1024;
 static int defy = 768;
+static u64 gpu_addr = 0;
+
+static const uint32_t bochs_primary_formats[] = {
+	DRM_FORMAT_XRGB8888,
+};
 
 module_param(defx, int, 0444);
 module_param(defy, int, 0444);
@@ -37,59 +42,12 @@ static bool bochs_crtc_mode_fixup(struct drm_crtc *crtc,
 	return true;
 }
 
-static int bochs_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
-				    struct drm_framebuffer *old_fb)
+static void bochs_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct bochs_device *bochs =
 		container_of(crtc, struct bochs_device, crtc);
-	struct bochs_framebuffer *bochs_fb;
-	struct bochs_bo *bo;
-	u64 gpu_addr = 0;
-	int ret;
 
-	if (old_fb) {
-		bochs_fb = to_bochs_framebuffer(old_fb);
-		bo = gem_to_bochs_bo(bochs_fb->obj);
-		ret = ttm_bo_reserve(&bo->bo, true, false, false, NULL);
-		if (ret) {
-			DRM_ERROR("failed to reserve old_fb bo\n");
-		} else {
-			bochs_bo_unpin(bo);
-			ttm_bo_unreserve(&bo->bo);
-		}
-	}
-
-	if (WARN_ON(crtc->primary->fb == NULL))
-		return -EINVAL;
-
-	bochs_fb = to_bochs_framebuffer(crtc->primary->fb);
-	bo = gem_to_bochs_bo(bochs_fb->obj);
-	ret = ttm_bo_reserve(&bo->bo, true, false, false, NULL);
-	if (ret)
-		return ret;
-
-	ret = bochs_bo_pin(bo, TTM_PL_FLAG_VRAM, &gpu_addr);
-	if (ret) {
-		ttm_bo_unreserve(&bo->bo);
-		return ret;
-	}
-
-	ttm_bo_unreserve(&bo->bo);
-	bochs_hw_setbase(bochs, x, y, gpu_addr);
-	return 0;
-}
-
-static int bochs_crtc_mode_set(struct drm_crtc *crtc,
-			       struct drm_display_mode *mode,
-			       struct drm_display_mode *adjusted_mode,
-			       int x, int y, struct drm_framebuffer *old_fb)
-{
-	struct bochs_device *bochs =
-		container_of(crtc, struct bochs_device, crtc);
-
-	bochs_hw_setmode(bochs, mode);
-	bochs_crtc_mode_set_base(crtc, x, y, old_fb);
-	return 0;
+	bochs_hw_setmode(bochs, &crtc->mode);
 }
 
 static void bochs_crtc_prepare(struct drm_crtc *crtc)
@@ -116,7 +74,7 @@ static int bochs_crtc_page_flip(struct drm_crtc *crtc,
 	unsigned long irqflags;
 
 	crtc->primary->fb = fb;
-	bochs_crtc_mode_set_base(crtc, 0, 0, old_fb);
+	drm_helper_crtc_mode_set_base(crtc, 0, 0, old_fb);
 	if (event) {
 		spin_lock_irqsave(&bochs->dev->event_lock, irqflags);
 		drm_send_vblank_event(bochs->dev, -1, event);
@@ -136,8 +94,9 @@ static const struct drm_crtc_funcs bochs_crtc_funcs = {
 static const struct drm_crtc_helper_funcs bochs_helper_funcs = {
 	.dpms = bochs_crtc_dpms,
 	.mode_fixup = bochs_crtc_mode_fixup,
-	.mode_set = bochs_crtc_mode_set,
-	.mode_set_base = bochs_crtc_mode_set_base,
+	.mode_set = drm_helper_crtc_mode_set,
+	.mode_set_base = drm_helper_crtc_mode_set_base,
+	.mode_set_nofb = bochs_crtc_mode_set_nofb,
 	.prepare = bochs_crtc_prepare,
 	.commit = bochs_crtc_commit,
 };
@@ -146,12 +105,105 @@ static void bochs_crtc_init(struct drm_device *dev)
 {
 	struct bochs_device *bochs = dev->dev_private;
 	struct drm_crtc *crtc = &bochs->crtc;
+	struct drm_plane *primary = &bochs->primary;
 
-	drm_crtc_init(dev, crtc, &bochs_crtc_funcs);
+	drm_crtc_init_with_planes(dev, crtc, primary, NULL, &bochs_crtc_funcs);
 	drm_mode_crtc_set_gamma_size(crtc, 256);
 	drm_crtc_helper_add(crtc, &bochs_helper_funcs);
 }
 
+static int bochs_plane_prepare_fb(struct drm_plane *plane,
+			  struct drm_framebuffer *fb,
+			  const struct drm_plane_state *new_state)
+{
+	struct bochs_framebuffer *bochs_fb;
+	struct bochs_bo *bo;
+	int ret;
+
+	if (WARN_ON(plane->fb == NULL))
+		return -EINVAL;
+
+	bochs_fb = to_bochs_framebuffer(plane->fb);
+	bo = gem_to_bochs_bo(bochs_fb->obj);
+	ttm_bo_reserve(&bo->bo, true, false, false, NULL);
+
+	ret = bochs_bo_pin(bo, TTM_PL_FLAG_VRAM, &gpu_addr);
+	if (ret) {
+		ttm_bo_unreserve(&bo->bo);
+		return ret;
+	}
+
+	ttm_bo_unreserve(&bo->bo);
+	return 0;
+}
+
+static void bochs_plane_cleanup_fb(struct drm_plane *plane,
+			   struct drm_framebuffer *old_fb,
+			   const struct drm_plane_state *old_state)
+{
+	struct bochs_framebuffer *bochs_fb;
+	struct bochs_bo *bo;
+
+	bochs_fb = to_bochs_framebuffer(old_fb);
+	bo = gem_to_bochs_bo(bochs_fb->obj);
+
+	/*
+	 * Since cleanup can't fail, change the interruptible argument i.e. the
+	 * second argument to false, this make sure ttm_bo_unpin can't fail.
+	 */
+	ttm_bo_reserve(&bo->bo, false, false, false, NULL);
+	bochs_bo_unpin(bo);
+	ttm_bo_unreserve(&bo->bo);
+}
+
+static int bochs_plane_atomic_check(struct drm_plane *plane,
+			    struct drm_plane_state *plane_state)
+{
+	return 0;
+}
+
+static void bochs_plane_atomic_update(struct drm_plane *plane,
+			      struct drm_plane_state *old_state)
+{
+	struct bochs_device *bochs =
+		container_of(plane, struct bochs_device, primary);
+	int x = plane->state->src_x >> 16;
+	int y = plane->state->src_y >> 16;
+
+	bochs_hw_setbase(bochs, x, y, gpu_addr);
+}
+
+static const struct drm_plane_funcs bochs_plane_funcs = {
+	.update_plane = drm_plane_helper_update,
+	.disable_plane = drm_plane_helper_disable,
+};
+
+static const struct drm_plane_helper_funcs bochs_plane_helper_funcs = {
+	.prepare_fb = bochs_plane_prepare_fb,
+	.cleanup_fb = bochs_plane_cleanup_fb,
+	.atomic_check = bochs_plane_atomic_check,
+	.atomic_update = bochs_plane_atomic_update,
+};
+
+static void bochs_plane_init(struct drm_device *dev)
+{
+	struct bochs_device *bochs = dev->dev_private;
+	struct drm_plane *primary = &bochs->primary;
+	int ret;
+
+	ret = drm_universal_plane_init(dev, primary, 0,
+				       &bochs_plane_funcs,
+				       bochs_primary_formats,
+				       ARRAY_SIZE(bochs_primary_formats),
+				       DRM_PLANE_TYPE_PRIMARY);
+	if (ret) {
+		DRM_DEBUG_KMS("Failed to init primary plane init");
+		return;
+	}
+
+	drm_plane_helper_add(primary, &bochs_plane_helper_funcs);
+}
+
 static bool bochs_encoder_mode_fixup(struct drm_encoder *encoder,
 				     const struct drm_display_mode *mode,
 				     struct drm_display_mode *adjusted_mode)
@@ -285,6 +337,7 @@ int bochs_kms_init(struct bochs_device *bochs)
 
 	bochs->dev->mode_config.funcs = (void *)&bochs_mode_funcs;
 
+	bochs_plane_init(bochs->dev);
 	bochs_crtc_init(bochs->dev);
 	bochs_encoder_init(bochs->dev);
 	bochs_connector_init(bochs->dev);
-- 
1.7.10.4


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

Re: [PATCH 1/8] drm/bochs: phase 1 - use the transitional helpers

[Gerd Hoffmann]

On Do, 2015-07-16 at 20:20 +0800, John Hunter wrote:
> --- a/drivers/gpu/drm/bochs/bochs_kms.c
> +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> @@ -10,6 +10,11 @@
>  
>  static int defx = 1024;
>  static int defy = 768;
> +static u64 gpu_addr = 0;

That isn't going to fly.  Guess you could it that into struct
bochs_framebuffer.

cheers,
  Gerd


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

Re: [PATCH 1/8] drm/bochs: phase 1 - use the transitional helpers

[Daniel Vetter]

On Thu, Jul 16, 2015 at 05:49:43PM +0200, Gerd Hoffmann wrote:
> On Do, 2015-07-16 at 20:20 +0800, John Hunter wrote:
> > --- a/drivers/gpu/drm/bochs/bochs_kms.c
> > +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> > @@ -10,6 +10,11 @@
> >  
> >  static int defx = 1024;
> >  static int defy = 768;
> > +static u64 gpu_addr = 0;
> 
> That isn't going to fly.  Guess you could it that into struct
> bochs_framebuffer.

Right I completely missed this in the prelimary review I've done. Simplest
solution would be to remove gpu_addr and just lookup the bo offset again
using bochs_bo_gpu_offset.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/8] drm/bochs: phase 2: wire up state reset(), duplicate() and destroy

[John Hunter]

From: Zhao Junwang <zhjwpku@gmail.com>

Set CRTC, planes and connectors to use the default implementations
from the atomic helper library.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
 drivers/gpu/drm/bochs/bochs_kms.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
index 1b66dd3..a14bac4 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -89,6 +89,9 @@ static const struct drm_crtc_funcs bochs_crtc_funcs = {
 	.set_config = drm_crtc_helper_set_config,
 	.destroy = drm_crtc_cleanup,
 	.page_flip = bochs_crtc_page_flip,
+	.reset = drm_atomic_helper_crtc_reset,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
 static const struct drm_crtc_helper_funcs bochs_helper_funcs = {
@@ -176,6 +179,9 @@ static void bochs_plane_atomic_update(struct drm_plane *plane,
 static const struct drm_plane_funcs bochs_plane_funcs = {
 	.update_plane = drm_plane_helper_update,
 	.disable_plane = drm_plane_helper_disable,
+	.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 const struct drm_plane_helper_funcs bochs_plane_helper_funcs = {
@@ -308,6 +314,9 @@ struct drm_connector_funcs bochs_connector_connector_funcs = {
 	.detect = bochs_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static void bochs_connector_init(struct drm_device *dev)
@@ -341,6 +350,9 @@ int bochs_kms_init(struct bochs_device *bochs)
 	bochs_crtc_init(bochs->dev);
 	bochs_encoder_init(bochs->dev);
 	bochs_connector_init(bochs->dev);
+
+	drm_mode_config_reset(bochs->dev);
+
 	drm_mode_connector_attach_encoder(&bochs->connector,
 					  &bochs->encoder);
 
-- 
1.7.10.4


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

[PATCH 3/8] drm/bochs: stop looking at legacy state

[John Hunter]

From: Zhao Junwang <zhjwpku@gmail.com>

with transitional helpers we've used the new callbacks, but control
flow was still old, but with atomic first step is to call atomic_check,
then prepare_fb, and so on, by this time, we can not look at any legacy
state like plane->fb or crtc->enabled, because they are updated at the
very end.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
 drivers/gpu/drm/bochs/bochs_kms.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
index a14bac4..e5fa125 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -123,10 +123,10 @@ static int bochs_plane_prepare_fb(struct drm_plane *plane,
 	struct bochs_bo *bo;
 	int ret;
 
-	if (WARN_ON(plane->fb == NULL))
+	if (WARN_ON(fb == NULL))
 		return -EINVAL;
 
-	bochs_fb = to_bochs_framebuffer(plane->fb);
+	bochs_fb = to_bochs_framebuffer(fb);
 	bo = gem_to_bochs_bo(bochs_fb->obj);
 	ttm_bo_reserve(&bo->bo, true, false, false, NULL);
 
-- 
1.7.10.4


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

[PATCH 4/8] drm/bochs: phase 3: for plane updates: switch to atomic helper internally

[John Hunter]

From: Zhao Junwang <zhjwpku@gmail.com>

- planes:
	drm_atomic_helper_update_plane()
	drm_atomic_helper_diable_plane()
- Driver:
	drm_atomic_helper_check()
	drm_atomic_helper_commit()

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
 drivers/gpu/drm/bochs/bochs_kms.c |    4 ++--
 drivers/gpu/drm/bochs/bochs_mm.c  |    2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
index e5fa125..2d2de7c 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -177,8 +177,8 @@ static void bochs_plane_atomic_update(struct drm_plane *plane,
 }
 
 static const struct drm_plane_funcs bochs_plane_funcs = {
-	.update_plane = drm_plane_helper_update,
-	.disable_plane = drm_plane_helper_disable,
+	.update_plane = drm_atomic_helper_update_plane,
+	.disable_plane = drm_atomic_helper_disable_plane,
 	.reset = drm_atomic_helper_plane_reset,
 	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index 66286ff..c1d819c 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -547,4 +547,6 @@ bochs_user_framebuffer_create(struct drm_device *dev,
 
 const struct drm_mode_config_funcs bochs_mode_funcs = {
 	.fb_create = bochs_user_framebuffer_create,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
 };
-- 
1.7.10.4


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

[PATCH 5/8] drm/bochs: phase 3: use atomic .set_config helper

[John Hunter]

From: Zhao Junwang <zhjwpku@gmail.com>

Now that phase 1 and phase 2 are complete, switch .set_config helper
to use the atomic one.

-since .prepare() callbacks are no more needed, remove them
-.mode_set() and .mode_set_base() are no longer needed, remove

-as we are not using the transitional helper now, we can use the
 drm_atomic_helper_plane_check_update this time.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
 drivers/gpu/drm/bochs/bochs_kms.c |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
index 2d2de7c..f0e93e1 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -50,10 +50,6 @@ static void bochs_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	bochs_hw_setmode(bochs, &crtc->mode);
 }
 
-static void bochs_crtc_prepare(struct drm_crtc *crtc)
-{
-}
-
 static void bochs_crtc_commit(struct drm_crtc *crtc)
 {
 }
@@ -86,7 +82,7 @@ static int bochs_crtc_page_flip(struct drm_crtc *crtc,
 /* These provide the minimum set of functions required to handle a CRTC */
 static const struct drm_crtc_funcs bochs_crtc_funcs = {
 	.gamma_set = bochs_crtc_gamma_set,
-	.set_config = drm_crtc_helper_set_config,
+	.set_config = drm_atomic_helper_set_config,
 	.destroy = drm_crtc_cleanup,
 	.page_flip = bochs_crtc_page_flip,
 	.reset = drm_atomic_helper_crtc_reset,
@@ -97,10 +93,7 @@ static const struct drm_crtc_funcs bochs_crtc_funcs = {
 static const struct drm_crtc_helper_funcs bochs_helper_funcs = {
 	.dpms = bochs_crtc_dpms,
 	.mode_fixup = bochs_crtc_mode_fixup,
-	.mode_set = drm_helper_crtc_mode_set,
-	.mode_set_base = drm_helper_crtc_mode_set_base,
 	.mode_set_nofb = bochs_crtc_mode_set_nofb,
-	.prepare = bochs_crtc_prepare,
 	.commit = bochs_crtc_commit,
 };
 
@@ -162,7 +155,15 @@ static void bochs_plane_cleanup_fb(struct drm_plane *plane,
 static int bochs_plane_atomic_check(struct drm_plane *plane,
 			    struct drm_plane_state *plane_state)
 {
-	return 0;
+	bool visible;
+
+	if (!plane_state->fb)
+		return 0;
+
+	return drm_atomic_helper_plane_check_update(plane_state,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    false, &visible);
 }
 
 static void bochs_plane_atomic_update(struct drm_plane *plane,
@@ -227,10 +228,6 @@ static void bochs_encoder_dpms(struct drm_encoder *encoder, int state)
 {
 }
 
-static void bochs_encoder_prepare(struct drm_encoder *encoder)
-{
-}
-
 static void bochs_encoder_commit(struct drm_encoder *encoder)
 {
 }
@@ -239,7 +236,6 @@ static const struct drm_encoder_helper_funcs bochs_encoder_helper_funcs = {
 	.dpms = bochs_encoder_dpms,
 	.mode_fixup = bochs_encoder_mode_fixup,
 	.mode_set = bochs_encoder_mode_set,
-	.prepare = bochs_encoder_prepare,
 	.commit = bochs_encoder_commit,
 };
 
-- 
1.7.10.4


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

[PATCH 6/8] drm/bochs: phase 3: provide a custom ->atomic_commit implementation

[John Hunter]

From: Zhao Junwang <zhjwpku@gmail.com>

This supports the asynchronous commits, required for page-flipping
Since it's virtual hw it's ok to commit async stuff right away, we
never have to wait for vblank.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
 drivers/gpu/drm/bochs/bochs_mm.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index c1d819c..37ac2ca 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -545,8 +545,15 @@ bochs_user_framebuffer_create(struct drm_device *dev,
 	return &bochs_fb->base;
 }
 
+static int bochs_atomic_commit(struct drm_device *dev,
+			     struct drm_atomic_state *a,
+			     bool async)
+{
+	return drm_atomic_helper_commit(dev, a, false);
+}
+
 const struct drm_mode_config_funcs bochs_mode_funcs = {
 	.fb_create = bochs_user_framebuffer_create,
 	.atomic_check = drm_atomic_helper_check,
-	.atomic_commit = drm_atomic_helper_commit,
+	.atomic_commit = bochs_atomic_commit,
 };
-- 
1.7.10.4


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

Re: [PATCH 6/8] drm/bochs: phase 3: provide a custom ->atomic_commit implementation

[Pekka Paalanen]

On Thu, 16 Jul 2015 20:20:39 +0800
John Hunter <zhjwpku@gmail.com> wrote:

> From: Zhao Junwang <zhjwpku@gmail.com>
> 
> This supports the asynchronous commits, required for page-flipping
> Since it's virtual hw it's ok to commit async stuff right away, we
> never have to wait for vblank.

Hi,

in theory, yes. This is what a patch to bochs implemented not too long
ago, so AFAIK you are only replicating the existing behaviour.

However, if userspace doing an async commit (or sync, I suppose) does
not incur any waits in the kernel in e.g. sending the page flip event,
then flip driven programs (e.g. a Wayland compositor, say, Weston)
will be running its rendering loop as a busy-loop, because the kernel
does not throttle it to the (virtual) display refresh rate.

This will cause maximal CPU usage and poor user experience as
everything else needs to fight for CPU time and event dispatch to get
through, like input.

I would hope someone could do a follow-up to implement a refresh cycle
emulation based on a clock. Userspace expects page flips to happen at
most at refresh rate when asking for vblank-synced flips. It's only
natural for userspace to drive its rendering loop based on the vblank
cycle.


Thanks,
pq

> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
> ---
>  drivers/gpu/drm/bochs/bochs_mm.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
> index c1d819c..37ac2ca 100644
> --- a/drivers/gpu/drm/bochs/bochs_mm.c
> +++ b/drivers/gpu/drm/bochs/bochs_mm.c
> @@ -545,8 +545,15 @@ bochs_user_framebuffer_create(struct drm_device *dev,
>  	return &bochs_fb->base;
>  }
>  
> +static int bochs_atomic_commit(struct drm_device *dev,
> +			     struct drm_atomic_state *a,
> +			     bool async)
> +{
> +	return drm_atomic_helper_commit(dev, a, false);
> +}
> +
>  const struct drm_mode_config_funcs bochs_mode_funcs = {
>  	.fb_create = bochs_user_framebuffer_create,
>  	.atomic_check = drm_atomic_helper_check,
> -	.atomic_commit = drm_atomic_helper_commit,
> +	.atomic_commit = bochs_atomic_commit,
>  };

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

Re: [PATCH 6/8] drm/bochs: phase 3: provide a custom ->atomic_commit implementation

[John Hunter]

[-- Attachment #1.1: Type: text/plain, Size: 2789 bytes --]

Hi Pekka,

Thanks for the information, I will talk to my mentor Daniel and try to find
out
what I can do about this.

Cheers,
Zhao Junwang

On Fri, Jul 17, 2015 at 2:08 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Thu, 16 Jul 2015 20:20:39 +0800
> John Hunter <zhjwpku@gmail.com> wrote:
>
> > From: Zhao Junwang <zhjwpku@gmail.com>
> >
> > This supports the asynchronous commits, required for page-flipping
> > Since it's virtual hw it's ok to commit async stuff right away, we
> > never have to wait for vblank.
>
> Hi,
>
> in theory, yes. This is what a patch to bochs implemented not too long
> ago, so AFAIK you are only replicating the existing behaviour.
>
> However, if userspace doing an async commit (or sync, I suppose) does
> not incur any waits in the kernel in e.g. sending the page flip event,
> then flip driven programs (e.g. a Wayland compositor, say, Weston)
> will be running its rendering loop as a busy-loop, because the kernel
> does not throttle it to the (virtual) display refresh rate.
>
> This will cause maximal CPU usage and poor user experience as
> everything else needs to fight for CPU time and event dispatch to get
> through, like input.
>
> I would hope someone could do a follow-up to implement a refresh cycle
> emulation based on a clock. Userspace expects page flips to happen at
> most at refresh rate when asking for vblank-synced flips. It's only
> natural for userspace to drive its rendering loop based on the vblank
> cycle.
>
>
> Thanks,
> pq
>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
> > ---
> >  drivers/gpu/drm/bochs/bochs_mm.c |    9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bochs/bochs_mm.c
> b/drivers/gpu/drm/bochs/bochs_mm.c
> > index c1d819c..37ac2ca 100644
> > --- a/drivers/gpu/drm/bochs/bochs_mm.c
> > +++ b/drivers/gpu/drm/bochs/bochs_mm.c
> > @@ -545,8 +545,15 @@ bochs_user_framebuffer_create(struct drm_device
> *dev,
> >       return &bochs_fb->base;
> >  }
> >
> > +static int bochs_atomic_commit(struct drm_device *dev,
> > +                          struct drm_atomic_state *a,
> > +                          bool async)
> > +{
> > +     return drm_atomic_helper_commit(dev, a, false);
> > +}
> > +
> >  const struct drm_mode_config_funcs bochs_mode_funcs = {
> >       .fb_create = bochs_user_framebuffer_create,
> >       .atomic_check = drm_atomic_helper_check,
> > -     .atomic_commit = drm_atomic_helper_commit,
> > +     .atomic_commit = bochs_atomic_commit,
> >  };
>
>


-- 
Best regards
Junwang Zhao
Microprocessor Research and Develop Center
Department of Computer Science &Technology
Peking University
Beijing, 100871, PRC

[-- Attachment #1.2: Type: text/html, Size: 3920 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 6/8] drm/bochs: phase 3: provide a custom ->atomic_commit implementation

[Stéphane Marchesin]

On Thu, Jul 16, 2015 at 11:08 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Thu, 16 Jul 2015 20:20:39 +0800
> John Hunter <zhjwpku@gmail.com> wrote:
>
> > From: Zhao Junwang <zhjwpku@gmail.com>
> >
> > This supports the asynchronous commits, required for page-flipping
> > Since it's virtual hw it's ok to commit async stuff right away, we
> > never have to wait for vblank.
>
> Hi,
>
> in theory, yes. This is what a patch to bochs implemented not too long
> ago, so AFAIK you are only replicating the existing behaviour.
>
> However, if userspace doing an async commit (or sync, I suppose) does
> not incur any waits in the kernel in e.g. sending the page flip event,
> then flip driven programs (e.g. a Wayland compositor, say, Weston)
> will be running its rendering loop as a busy-loop, because the kernel
> does not throttle it to the (virtual) display refresh rate.
>
> This will cause maximal CPU usage and poor user experience as
> everything else needs to fight for CPU time and event dispatch to get
> through, like input.
>
> I would hope someone could do a follow-up to implement a refresh cycle
> emulation based on a clock. Userspace expects page flips to happen at
> most at refresh rate when asking for vblank-synced flips. It's only
> natural for userspace to drive its rendering loop based on the vblank
> cycle.


I've been asking myself the same question (for the UDL driver) and I'm
not sure if this policy should go in the kernel. After all, there
could be legitimate reasons for user space to render lots of frames
per second. It seems to me that if user space doesn't want too many
fps, it should just throttle itself.

Stéphane


>
>
>
> Thanks,
> pq
>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
> > ---
> >  drivers/gpu/drm/bochs/bochs_mm.c |    9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
> > index c1d819c..37ac2ca 100644
> > --- a/drivers/gpu/drm/bochs/bochs_mm.c
> > +++ b/drivers/gpu/drm/bochs/bochs_mm.c
> > @@ -545,8 +545,15 @@ bochs_user_framebuffer_create(struct drm_device *dev,
> >       return &bochs_fb->base;
> >  }
> >
> > +static int bochs_atomic_commit(struct drm_device *dev,
> > +                          struct drm_atomic_state *a,
> > +                          bool async)
> > +{
> > +     return drm_atomic_helper_commit(dev, a, false);
> > +}
> > +
> >  const struct drm_mode_config_funcs bochs_mode_funcs = {
> >       .fb_create = bochs_user_framebuffer_create,
> >       .atomic_check = drm_atomic_helper_check,
> > -     .atomic_commit = drm_atomic_helper_commit,
> > +     .atomic_commit = bochs_atomic_commit,
> >  };
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/8] drm/bochs: phase 3: provide a custom ->atomic_commit implementation

[Pekka Paalanen]

On Sun, 19 Jul 2015 17:20:32 -0700
Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:

> On Thu, Jul 16, 2015 at 11:08 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Thu, 16 Jul 2015 20:20:39 +0800
> > John Hunter <zhjwpku@gmail.com> wrote:
> >
> > > From: Zhao Junwang <zhjwpku@gmail.com>
> > >
> > > This supports the asynchronous commits, required for page-flipping
> > > Since it's virtual hw it's ok to commit async stuff right away, we
> > > never have to wait for vblank.
> >
> > Hi,
> >
> > in theory, yes. This is what a patch to bochs implemented not too long
> > ago, so AFAIK you are only replicating the existing behaviour.
> >
> > However, if userspace doing an async commit (or sync, I suppose) does
> > not incur any waits in the kernel in e.g. sending the page flip event,
> > then flip driven programs (e.g. a Wayland compositor, say, Weston)
> > will be running its rendering loop as a busy-loop, because the kernel
> > does not throttle it to the (virtual) display refresh rate.
> >
> > This will cause maximal CPU usage and poor user experience as
> > everything else needs to fight for CPU time and event dispatch to get
> > through, like input.
> >
> > I would hope someone could do a follow-up to implement a refresh cycle
> > emulation based on a clock. Userspace expects page flips to happen at
> > most at refresh rate when asking for vblank-synced flips. It's only
> > natural for userspace to drive its rendering loop based on the vblank
> > cycle.
> 
> 
> I've been asking myself the same question (for the UDL driver) and I'm
> not sure if this policy should go in the kernel. After all, there
> could be legitimate reasons for user space to render lots of frames
> per second. It seems to me that if user space doesn't want too many
> fps, it should just throttle itself.

If userspace wants to render lots of frames per second, IMO it should
not be using vblank-synced operations in a way that may throttle it.
The lots of frames use case is already non-working for the majority of
the drivers without DRM_MODE_PAGE_FLIP_ASYNC, right?

The problem here I see is that one DRM driver decides to work different
to other DRM drivers. All real-hardware DRM drivers, when asked to do
vblank-synced update, actually do throttle to the vblank AFAIK. Is it
too much to assume, that the video mode set in a driver (refresh rate)
corresponds to the vblank rate which implicitly delays the completion
of vblank-sync'd operations to at least the next vblank boundary?

I think, if the driver cannot implement proper semantics (which IMO
includes the throttling) for vblank-sync'd operations and it does not
want to fake them with a clock, it should just refuse vblank-synced
operations. That would push the problem to userspace, and it would be
obvious what's going wrong. Naturally, it would break some userspace
programs that expect vblank-synced operations to work, but is that
so much different to the current unfixed situation?


Thanks,
pq
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/8] drm/bochs: phase 3: provide a custom ->atomic_commit implementation

[Stéphane Marchesin]

On Mon, Jul 20, 2015 at 12:46 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Sun, 19 Jul 2015 17:20:32 -0700
> Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
>
>> On Thu, Jul 16, 2015 at 11:08 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>> >
>> > On Thu, 16 Jul 2015 20:20:39 +0800
>> > John Hunter <zhjwpku@gmail.com> wrote:
>> >
>> > > From: Zhao Junwang <zhjwpku@gmail.com>
>> > >
>> > > This supports the asynchronous commits, required for page-flipping
>> > > Since it's virtual hw it's ok to commit async stuff right away, we
>> > > never have to wait for vblank.
>> >
>> > Hi,
>> >
>> > in theory, yes. This is what a patch to bochs implemented not too long
>> > ago, so AFAIK you are only replicating the existing behaviour.
>> >
>> > However, if userspace doing an async commit (or sync, I suppose) does
>> > not incur any waits in the kernel in e.g. sending the page flip event,
>> > then flip driven programs (e.g. a Wayland compositor, say, Weston)
>> > will be running its rendering loop as a busy-loop, because the kernel
>> > does not throttle it to the (virtual) display refresh rate.
>> >
>> > This will cause maximal CPU usage and poor user experience as
>> > everything else needs to fight for CPU time and event dispatch to get
>> > through, like input.
>> >
>> > I would hope someone could do a follow-up to implement a refresh cycle
>> > emulation based on a clock. Userspace expects page flips to happen at
>> > most at refresh rate when asking for vblank-synced flips. It's only
>> > natural for userspace to drive its rendering loop based on the vblank
>> > cycle.
>>
>>
>> I've been asking myself the same question (for the UDL driver) and I'm
>> not sure if this policy should go in the kernel. After all, there
>> could be legitimate reasons for user space to render lots of frames
>> per second. It seems to me that if user space doesn't want too many
>> fps, it should just throttle itself.
>
> If userspace wants to render lots of frames per second, IMO it should
> not be using vblank-synced operations in a way that may throttle it.
> The lots of frames use case is already non-working for the majority of
> the drivers without DRM_MODE_PAGE_FLIP_ASYNC, right?
>
> The problem here I see is that one DRM driver decides to work different
> to other DRM drivers. All real-hardware DRM drivers, when asked to do
> vblank-synced update, actually do throttle to the vblank AFAIK.

udl is an exception here. It is (arguably) real hardware but doesn't throttle.

> Is it
> too much to assume, that the video mode set in a driver (refresh rate)
> corresponds to the vblank rate which implicitly delays the completion
> of vblank-sync'd operations to at least the next vblank boundary?

I think it's wrong to make user space think that a vsynced display
always matches the refresh rate in a world where:

- some displays have variable refresh rates (not just the fancy new
stuff like g-sync, look for lvds_downclock in the intel driver for
example, also consider DSI displays)

- some displays have no refresh rate (the ones we are talking about
here: udl, bochs...)

- you can do partial vsynced updates by just waiting for a specific
scanline range which again breaks the assumption that "vsynced" ==
"refreshes at the monitor rate". In this case there is no visible
tearing (in that sense it is vsynced) but the flip time is not
predictable using the refresh rate.

So I don't think we should perpetuate that problem. And I would like
user space to "see" the actual flip times to enable some kind of
scheduling where possible.

>
> I think, if the driver cannot implement proper semantics (which IMO
> includes the throttling) for vblank-sync'd operations and it does not
> want to fake them with a clock, it should just refuse vblank-synced
> operations.

Yes refusing vsynced flips for these drivers sounds reasonable. But
please let's not bake in another assumption in the API (or rather,
let's try to un-bake it).

Stéphane


> That would push the problem to userspace, and it would be
> obvious what's going wrong. Naturally, it would break some userspace
> programs that expect vblank-synced operations to work, but is that
> so much different to the current unfixed situation?
>
>
> Thanks,
> pq
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

KMS timings (Re: [PATCH 6/8] drm/bochs: phase 3: provide a custom ->atomic_commit implementation)

[Pekka Paalanen]

On Mon, 20 Jul 2015 01:58:33 -0700
Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:

> On Mon, Jul 20, 2015 at 12:46 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Sun, 19 Jul 2015 17:20:32 -0700
> > Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> >
> >> On Thu, Jul 16, 2015 at 11:08 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >> >
> >> > On Thu, 16 Jul 2015 20:20:39 +0800
> >> > John Hunter <zhjwpku@gmail.com> wrote:
> >> >
> >> > > From: Zhao Junwang <zhjwpku@gmail.com>
> >> > >
> >> > > This supports the asynchronous commits, required for page-flipping
> >> > > Since it's virtual hw it's ok to commit async stuff right away, we
> >> > > never have to wait for vblank.
> >> >
> >> > Hi,
> >> >
> >> > in theory, yes. This is what a patch to bochs implemented not too long
> >> > ago, so AFAIK you are only replicating the existing behaviour.
> >> >
> >> > However, if userspace doing an async commit (or sync, I suppose) does
> >> > not incur any waits in the kernel in e.g. sending the page flip event,
> >> > then flip driven programs (e.g. a Wayland compositor, say, Weston)
> >> > will be running its rendering loop as a busy-loop, because the kernel
> >> > does not throttle it to the (virtual) display refresh rate.
> >> >
> >> > This will cause maximal CPU usage and poor user experience as
> >> > everything else needs to fight for CPU time and event dispatch to get
> >> > through, like input.
> >> >
> >> > I would hope someone could do a follow-up to implement a refresh cycle
> >> > emulation based on a clock. Userspace expects page flips to happen at
> >> > most at refresh rate when asking for vblank-synced flips. It's only
> >> > natural for userspace to drive its rendering loop based on the vblank
> >> > cycle.
> >>
> >>
> >> I've been asking myself the same question (for the UDL driver) and I'm
> >> not sure if this policy should go in the kernel. After all, there
> >> could be legitimate reasons for user space to render lots of frames
> >> per second. It seems to me that if user space doesn't want too many
> >> fps, it should just throttle itself.
> >
> > If userspace wants to render lots of frames per second, IMO it should
> > not be using vblank-synced operations in a way that may throttle it.
> > The lots of frames use case is already non-working for the majority of
> > the drivers without DRM_MODE_PAGE_FLIP_ASYNC, right?
> >
> > The problem here I see is that one DRM driver decides to work different
> > to other DRM drivers. All real-hardware DRM drivers, when asked to do
> > vblank-synced update, actually do throttle to the vblank AFAIK.
> 
> udl is an exception here. It is (arguably) real hardware but doesn't throttle.
> 
> > Is it
> > too much to assume, that the video mode set in a driver (refresh rate)
> > corresponds to the vblank rate which implicitly delays the completion
> > of vblank-sync'd operations to at least the next vblank boundary?
> 
> I think it's wrong to make user space think that a vsynced display
> always matches the refresh rate in a world where:
> 
> - some displays have variable refresh rates (not just the fancy new
> stuff like g-sync, look for lvds_downclock in the intel driver for
> example, also consider DSI displays)
> 
> - some displays have no refresh rate (the ones we are talking about
> here: udl, bochs...)

That means that refresh rate in a video mode is bogus. Can userspace
know when the refresh rate is meaningless? I suppose there are two
different cases of meaningless, too: when the driver ignores it as
input argument, and when it is used but has no guarantees for timings.

Assuming it's always meaningless wrt. timings is pretty harsh. E.g. the
Wayland Presentation extension's implementation in Weston uses the
refresh rate to predict the next flip time and hands it out to clients
for scheduling/interpolation purposes.

> - you can do partial vsynced updates by just waiting for a specific
> scanline range which again breaks the assumption that "vsynced" ==
> "refreshes at the monitor rate". In this case there is no visible
> tearing (in that sense it is vsynced) but the flip time is not
> predictable using the refresh rate.

Okay. That also invalidates the design (well, at least the
implementation, and sounds like DRM does not give any tools to allow
implementing it) the Wayland Presentation extension even on "good"
hardware, so nice to realize. I was already suggesting we should
stabilize it since it looks good, but this puts it all back to the
drawing board.

I think it also mostly invalidates the whole scheduling implementation
in Weston.

> So I don't think we should perpetuate that problem. And I would like
> user space to "see" the actual flip times to enable some kind of
> scheduling where possible.
> 
> >
> > I think, if the driver cannot implement proper semantics (which IMO
> > includes the throttling) for vblank-sync'd operations and it does not
> > want to fake them with a clock, it should just refuse vblank-synced
> > operations.
> 
> Yes refusing vsynced flips for these drivers sounds reasonable. But
> please let's not bake in another assumption in the API (or rather,
> let's try to un-bake it).

Could you be more specific on everything, please?

What should drivers do in different situations, what guarantees we do
have, and how does userspace predict the earliest possible flip time?
How do you define flip time to begin with, if it's not tied to the
scanout cycle (vblank)?

How should a compositor schedule eveything, and what can it tell to the
clients about the timings in the immediate future?

You gave me the feeling that everything I thought I knew and relied on
is wrong.

> > That would push the problem to userspace, and it would be
> > obvious what's going wrong. Naturally, it would break some userspace
> > programs that expect vblank-synced operations to work, but is that
> > so much different to the current unfixed situation?

Thanks,
pq
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS timings (Re: [PATCH 6/8] drm/bochs: phase 3: provide a custom ->atomic_commit implementation)

[Daniel Vetter]

On Mon, Jul 20, 2015 at 12:35:48PM +0300, Pekka Paalanen wrote:
> On Mon, 20 Jul 2015 01:58:33 -0700
> Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> 
> > On Mon, Jul 20, 2015 at 12:46 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > On Sun, 19 Jul 2015 17:20:32 -0700
> > > Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> > >
> > >> On Thu, Jul 16, 2015 at 11:08 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > >> >
> > >> > On Thu, 16 Jul 2015 20:20:39 +0800
> > >> > John Hunter <zhjwpku@gmail.com> wrote:
> > >> >
> > >> > > From: Zhao Junwang <zhjwpku@gmail.com>
> > >> > >
> > >> > > This supports the asynchronous commits, required for page-flipping
> > >> > > Since it's virtual hw it's ok to commit async stuff right away, we
> > >> > > never have to wait for vblank.
> > >> >
> > >> > Hi,
> > >> >
> > >> > in theory, yes. This is what a patch to bochs implemented not too long
> > >> > ago, so AFAIK you are only replicating the existing behaviour.
> > >> >
> > >> > However, if userspace doing an async commit (or sync, I suppose) does
> > >> > not incur any waits in the kernel in e.g. sending the page flip event,
> > >> > then flip driven programs (e.g. a Wayland compositor, say, Weston)
> > >> > will be running its rendering loop as a busy-loop, because the kernel
> > >> > does not throttle it to the (virtual) display refresh rate.
> > >> >
> > >> > This will cause maximal CPU usage and poor user experience as
> > >> > everything else needs to fight for CPU time and event dispatch to get
> > >> > through, like input.
> > >> >
> > >> > I would hope someone could do a follow-up to implement a refresh cycle
> > >> > emulation based on a clock. Userspace expects page flips to happen at
> > >> > most at refresh rate when asking for vblank-synced flips. It's only
> > >> > natural for userspace to drive its rendering loop based on the vblank
> > >> > cycle.
> > >>
> > >>
> > >> I've been asking myself the same question (for the UDL driver) and I'm
> > >> not sure if this policy should go in the kernel. After all, there
> > >> could be legitimate reasons for user space to render lots of frames
> > >> per second. It seems to me that if user space doesn't want too many
> > >> fps, it should just throttle itself.
> > >
> > > If userspace wants to render lots of frames per second, IMO it should
> > > not be using vblank-synced operations in a way that may throttle it.
> > > The lots of frames use case is already non-working for the majority of
> > > the drivers without DRM_MODE_PAGE_FLIP_ASYNC, right?
> > >
> > > The problem here I see is that one DRM driver decides to work different
> > > to other DRM drivers. All real-hardware DRM drivers, when asked to do
> > > vblank-synced update, actually do throttle to the vblank AFAIK.
> > 
> > udl is an exception here. It is (arguably) real hardware but doesn't throttle.
> > 
> > > Is it
> > > too much to assume, that the video mode set in a driver (refresh rate)
> > > corresponds to the vblank rate which implicitly delays the completion
> > > of vblank-sync'd operations to at least the next vblank boundary?
> > 
> > I think it's wrong to make user space think that a vsynced display
> > always matches the refresh rate in a world where:
> > 
> > - some displays have variable refresh rates (not just the fancy new
> > stuff like g-sync, look for lvds_downclock in the intel driver for
> > example, also consider DSI displays)
> > 
> > - some displays have no refresh rate (the ones we are talking about
> > here: udl, bochs...)
> 
> That means that refresh rate in a video mode is bogus. Can userspace
> know when the refresh rate is meaningless? I suppose there are two
> different cases of meaningless, too: when the driver ignores it as
> input argument, and when it is used but has no guarantees for timings.
> 
> Assuming it's always meaningless wrt. timings is pretty harsh. E.g. the
> Wayland Presentation extension's implementation in Weston uses the
> refresh rate to predict the next flip time and hands it out to clients
> for scheduling/interpolation purposes.

We removed lvds_downclock in i915 (was never enabled by default). The new
code is currently edp only, but enabled by default. And it tries _really_
hard to keep up the illusion that the requested vrefresh is the one you
actually have - it upclocks every time userspace changes the screen.

The only exception is when you only ask for vblank events, and the delay
to the next pageflip is longer than the timeout. That can't be fixed right
now because drm_irq.c is the last midlayer needed by modern drivers, but
I'd like to fix it eventually.

There's future plans to allow userspace to explicitly ask for a reduced
vrefresh (e.g. video playback), but then obviously they'll match again
too.

We also have a big pile of testcases in kms_flip which check that the
timestamps are evenly spaced and agree with vrefresh. There's some oddball
bugs around tv-out that we never bothered to fix (since the requested mode
is a fake one and rescaled by the TV port, which also has it's own clock).

Imo aiming for vrefresh to be accurate is good. For gsync and friends I
think we should have an explicit range or flag to make userspace aware of
what's going on.

> > - you can do partial vsynced updates by just waiting for a specific
> > scanline range which again breaks the assumption that "vsynced" ==
> > "refreshes at the monitor rate". In this case there is no visible
> > tearing (in that sense it is vsynced) but the flip time is not
> > predictable using the refresh rate.
> 
> Okay. That also invalidates the design (well, at least the
> implementation, and sounds like DRM does not give any tools to allow
> implementing it) the Wayland Presentation extension even on "good"
> hardware, so nice to realize. I was already suggesting we should
> stabilize it since it looks good, but this puts it all back to the
> drawing board.
> 
> I think it also mostly invalidates the whole scheduling implementation
> in Weston.

Currently scanline waits is just something the intel DDX does to update
dri and video clients without tearing while still essentially doing
unsynced frontbuffer rendering. It sucks down massive amounts of gpu
bandwidth since it fully stalls the rendering (at least on i915 hw).

As long as you have buffer_age and friends you should be able to be as
efficient with pageflips (or better) as with scanline waits.

> > So I don't think we should perpetuate that problem. And I would like
> > user space to "see" the actual flip times to enable some kind of
> > scheduling where possible.
> > 
> > >
> > > I think, if the driver cannot implement proper semantics (which IMO
> > > includes the throttling) for vblank-sync'd operations and it does not
> > > want to fake them with a clock, it should just refuse vblank-synced
> > > operations.
> > 
> > Yes refusing vsynced flips for these drivers sounds reasonable. But
> > please let's not bake in another assumption in the API (or rather,
> > let's try to un-bake it).
> 
> Could you be more specific on everything, please?
> 
> What should drivers do in different situations, what guarantees we do
> have, and how does userspace predict the earliest possible flip time?
> How do you define flip time to begin with, if it's not tied to the
> scanout cycle (vblank)?
> 
> How should a compositor schedule eveything, and what can it tell to the
> clients about the timings in the immediate future?
> 
> You gave me the feeling that everything I thought I knew and relied on
> is wrong.

I guess we either kick out page_flip for all drivers who fake it. And if
that's causing regressions then we probably want to fake it with a timer.
Unpretty, but such is the game of backwards compat forever. But I'm not
sure whether we established that we have a problem already, at least I'm
missing users screaming about udl/bochs & friends.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS timings (Re: [PATCH 6/8] drm/bochs: phase 3: provide a custom ->atomic_commit implementation)

[Stéphane Marchesin]

On Mon, Jul 20, 2015 at 7:21 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Jul 20, 2015 at 12:35:48PM +0300, Pekka Paalanen wrote:
>> On Mon, 20 Jul 2015 01:58:33 -0700
>> Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
>>
>> > On Mon, Jul 20, 2015 at 12:46 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>> > > On Sun, 19 Jul 2015 17:20:32 -0700
>> > > Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
>> > >
>> > >> On Thu, Jul 16, 2015 at 11:08 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>> > >> >
>> > >> > On Thu, 16 Jul 2015 20:20:39 +0800
>> > >> > John Hunter <zhjwpku@gmail.com> wrote:
>> > >> >
>> > >> > > From: Zhao Junwang <zhjwpku@gmail.com>
>> > >> > >
>> > >> > > This supports the asynchronous commits, required for page-flipping
>> > >> > > Since it's virtual hw it's ok to commit async stuff right away, we
>> > >> > > never have to wait for vblank.
>> > >> >
>> > >> > Hi,
>> > >> >
>> > >> > in theory, yes. This is what a patch to bochs implemented not too long
>> > >> > ago, so AFAIK you are only replicating the existing behaviour.
>> > >> >
>> > >> > However, if userspace doing an async commit (or sync, I suppose) does
>> > >> > not incur any waits in the kernel in e.g. sending the page flip event,
>> > >> > then flip driven programs (e.g. a Wayland compositor, say, Weston)
>> > >> > will be running its rendering loop as a busy-loop, because the kernel
>> > >> > does not throttle it to the (virtual) display refresh rate.
>> > >> >
>> > >> > This will cause maximal CPU usage and poor user experience as
>> > >> > everything else needs to fight for CPU time and event dispatch to get
>> > >> > through, like input.
>> > >> >
>> > >> > I would hope someone could do a follow-up to implement a refresh cycle
>> > >> > emulation based on a clock. Userspace expects page flips to happen at
>> > >> > most at refresh rate when asking for vblank-synced flips. It's only
>> > >> > natural for userspace to drive its rendering loop based on the vblank
>> > >> > cycle.
>> > >>
>> > >>
>> > >> I've been asking myself the same question (for the UDL driver) and I'm
>> > >> not sure if this policy should go in the kernel. After all, there
>> > >> could be legitimate reasons for user space to render lots of frames
>> > >> per second. It seems to me that if user space doesn't want too many
>> > >> fps, it should just throttle itself.
>> > >
>> > > If userspace wants to render lots of frames per second, IMO it should
>> > > not be using vblank-synced operations in a way that may throttle it.
>> > > The lots of frames use case is already non-working for the majority of
>> > > the drivers without DRM_MODE_PAGE_FLIP_ASYNC, right?
>> > >
>> > > The problem here I see is that one DRM driver decides to work different
>> > > to other DRM drivers. All real-hardware DRM drivers, when asked to do
>> > > vblank-synced update, actually do throttle to the vblank AFAIK.
>> >
>> > udl is an exception here. It is (arguably) real hardware but doesn't throttle.
>> >
>> > > Is it
>> > > too much to assume, that the video mode set in a driver (refresh rate)
>> > > corresponds to the vblank rate which implicitly delays the completion
>> > > of vblank-sync'd operations to at least the next vblank boundary?
>> >
>> > I think it's wrong to make user space think that a vsynced display
>> > always matches the refresh rate in a world where:
>> >
>> > - some displays have variable refresh rates (not just the fancy new
>> > stuff like g-sync, look for lvds_downclock in the intel driver for
>> > example, also consider DSI displays)
>> >
>> > - some displays have no refresh rate (the ones we are talking about
>> > here: udl, bochs...)
>>
>> That means that refresh rate in a video mode is bogus. Can userspace
>> know when the refresh rate is meaningless? I suppose there are two
>> different cases of meaningless, too: when the driver ignores it as
>> input argument, and when it is used but has no guarantees for timings.
>>
>> Assuming it's always meaningless wrt. timings is pretty harsh. E.g. the
>> Wayland Presentation extension's implementation in Weston uses the
>> refresh rate to predict the next flip time and hands it out to clients
>> for scheduling/interpolation purposes.
>
> We removed lvds_downclock in i915 (was never enabled by default).

We use/ship it, so there is a value I would say.

> The new
> code is currently edp only, but enabled by default. And it tries _really_
> hard to keep up the illusion that the requested vrefresh is the one you
> actually have - it upclocks every time userspace changes the screen.

Except that there is latency in doing so (it can never be done
instantly), and user space definitely sees it, at least when it queues
the first flip. So the illusion doesn't exist.

>
> The only exception is when you only ask for vblank events, and the delay
> to the next pageflip is longer than the timeout. That can't be fixed right
> now because drm_irq.c is the last midlayer needed by modern drivers, but
> I'd like to fix it eventually.
>
> There's future plans to allow userspace to explicitly ask for a reduced
> vrefresh (e.g. video playback), but then obviously they'll match again
> too.
>
> We also have a big pile of testcases in kms_flip which check that the
> timestamps are evenly spaced and agree with vrefresh. There's some oddball
> bugs around tv-out that we never bothered to fix (since the requested mode
> is a fake one and rescaled by the TV port, which also has it's own clock).
>
> Imo aiming for vrefresh to be accurate is good. For gsync and friends I
> think we should have an explicit range or flag to make userspace aware of
> what's going on.

I think the concept of vrefresh is flawed and not really future-proof
(I gave a few examples in my previous email). I agree we should keep
it as legacy, but we should add something else for the more advanced
cases.

>
>> > - you can do partial vsynced updates by just waiting for a specific
>> > scanline range which again breaks the assumption that "vsynced" ==
>> > "refreshes at the monitor rate". In this case there is no visible
>> > tearing (in that sense it is vsynced) but the flip time is not
>> > predictable using the refresh rate.
>>
>> Okay. That also invalidates the design (well, at least the
>> implementation, and sounds like DRM does not give any tools to allow
>> implementing it) the Wayland Presentation extension even on "good"
>> hardware, so nice to realize. I was already suggesting we should
>> stabilize it since it looks good, but this puts it all back to the
>> drawing board.
>>
>> I think it also mostly invalidates the whole scheduling implementation
>> in Weston.
>
> Currently scanline waits is just something the intel DDX does to update
> dri and video clients without tearing while still essentially doing
> unsynced frontbuffer rendering. It sucks down massive amounts of gpu
> bandwidth since it fully stalls the rendering (at least on i915 hw).

That's an i915 specificity though, if it doesn't work on i915, just
don't expose it on i915...

>
> As long as you have buffer_age and friends you should be able to be as
> efficient with pageflips (or better) as with scanline waits.

Flips and scanline waits are completely orthogonal things though --
there are 4 valid combinations in { wait for a scanline, wait for a
vsync } x { do a flip, do a blit}. And you can be more efficient if
you wait for a scanline; for example if you are using a damage rect at
(x,y) and the "beam" is past scanline 0 but before y, you can still do
your scanline flip right away while you can't with vsynced flips.

>
>> > So I don't think we should perpetuate that problem. And I would like
>> > user space to "see" the actual flip times to enable some kind of
>> > scheduling where possible.
>> >
>> > >
>> > > I think, if the driver cannot implement proper semantics (which IMO
>> > > includes the throttling) for vblank-sync'd operations and it does not
>> > > want to fake them with a clock, it should just refuse vblank-synced
>> > > operations.
>> >
>> > Yes refusing vsynced flips for these drivers sounds reasonable. But
>> > please let's not bake in another assumption in the API (or rather,
>> > let's try to un-bake it).
>>
>> Could you be more specific on everything, please?
>>
>> What should drivers do in different situations, what guarantees we do
>> have, and how does userspace predict the earliest possible flip time?
>> How do you define flip time to begin with, if it's not tied to the
>> scanout cycle (vblank)?
>>
>> How should a compositor schedule eveything, and what can it tell to the
>> clients about the timings in the immediate future?
>>
>> You gave me the feeling that everything I thought I knew and relied on
>> is wrong.
>
> I guess we either kick out page_flip for all drivers who fake it. And if
> that's causing regressions then we probably want to fake it with a timer.
> Unpretty, but such is the game of backwards compat forever. But I'm not
> sure whether we established that we have a problem already, at least I'm
> missing users screaming about udl/bochs & friends.

Yeah I don't think I care about the old interface, it is what it is.
But we should design something which works well for the future use
cases.

Stéphane

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

Re: KMS timings (Re: [PATCH 6/8] drm/bochs: phase 3: provide a custom ->atomic_commit implementation)

[Pekka Paalanen]

On Mon, 20 Jul 2015 10:32:31 -0700
Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:

> On Mon, Jul 20, 2015 at 7:21 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Jul 20, 2015 at 12:35:48PM +0300, Pekka Paalanen wrote:
> >> On Mon, 20 Jul 2015 01:58:33 -0700
> >> Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> >>
> >> > On Mon, Jul 20, 2015 at 12:46 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >> > > On Sun, 19 Jul 2015 17:20:32 -0700
> >> > > Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> >> > >
> >> > >> On Thu, Jul 16, 2015 at 11:08 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >> > >> >
> >> > >> > On Thu, 16 Jul 2015 20:20:39 +0800
> >> > >> > John Hunter <zhjwpku@gmail.com> wrote:
> >> > >> >
> >> > >> > > From: Zhao Junwang <zhjwpku@gmail.com>
> >> > >> > >
> >> > >> > > This supports the asynchronous commits, required for page-flipping
> >> > >> > > Since it's virtual hw it's ok to commit async stuff right away, we
> >> > >> > > never have to wait for vblank.
> >> > >> >
> >> > >> > Hi,
> >> > >> >
> >> > >> > in theory, yes. This is what a patch to bochs implemented not too long
> >> > >> > ago, so AFAIK you are only replicating the existing behaviour.
> >> > >> >
> >> > >> > However, if userspace doing an async commit (or sync, I suppose) does
> >> > >> > not incur any waits in the kernel in e.g. sending the page flip event,
> >> > >> > then flip driven programs (e.g. a Wayland compositor, say, Weston)
> >> > >> > will be running its rendering loop as a busy-loop, because the kernel
> >> > >> > does not throttle it to the (virtual) display refresh rate.
> >> > >> >
> >> > >> > This will cause maximal CPU usage and poor user experience as
> >> > >> > everything else needs to fight for CPU time and event dispatch to get
> >> > >> > through, like input.
> >> > >> >
> >> > >> > I would hope someone could do a follow-up to implement a refresh cycle
> >> > >> > emulation based on a clock. Userspace expects page flips to happen at
> >> > >> > most at refresh rate when asking for vblank-synced flips. It's only
> >> > >> > natural for userspace to drive its rendering loop based on the vblank
> >> > >> > cycle.
> >> > >>
> >> > >>
> >> > >> I've been asking myself the same question (for the UDL driver) and I'm
> >> > >> not sure if this policy should go in the kernel. After all, there
> >> > >> could be legitimate reasons for user space to render lots of frames
> >> > >> per second. It seems to me that if user space doesn't want too many
> >> > >> fps, it should just throttle itself.
> >> > >
> >> > > If userspace wants to render lots of frames per second, IMO it should
> >> > > not be using vblank-synced operations in a way that may throttle it.
> >> > > The lots of frames use case is already non-working for the majority of
> >> > > the drivers without DRM_MODE_PAGE_FLIP_ASYNC, right?
> >> > >
> >> > > The problem here I see is that one DRM driver decides to work different
> >> > > to other DRM drivers. All real-hardware DRM drivers, when asked to do
> >> > > vblank-synced update, actually do throttle to the vblank AFAIK.
> >> >
> >> > udl is an exception here. It is (arguably) real hardware but doesn't throttle.
> >> >
> >> > > Is it
> >> > > too much to assume, that the video mode set in a driver (refresh rate)
> >> > > corresponds to the vblank rate which implicitly delays the completion
> >> > > of vblank-sync'd operations to at least the next vblank boundary?
> >> >
> >> > I think it's wrong to make user space think that a vsynced display
> >> > always matches the refresh rate in a world where:
> >> >
> >> > - some displays have variable refresh rates (not just the fancy new
> >> > stuff like g-sync, look for lvds_downclock in the intel driver for
> >> > example, also consider DSI displays)
> >> >
> >> > - some displays have no refresh rate (the ones we are talking about
> >> > here: udl, bochs...)

> > Imo aiming for vrefresh to be accurate is good. For gsync and friends I
> > think we should have an explicit range or flag to make userspace aware of
> > what's going on.
> 
> I think the concept of vrefresh is flawed and not really future-proof
> (I gave a few examples in my previous email). I agree we should keep
> it as legacy, but we should add something else for the more advanced
> cases.

Right, so let's add something new for new hardware features and keep
the existing behavior existing.

I suppose the problem is that the existing behavior is not really
documented so we have to resort to screaming users?

If one does not ask for ASYNC with a page flip, does it mean flipping
on the next vblank, or flipping such that it cannot tear but allowing
techiques like scanline waits?

It used to be reasonable to assume a constant refresh rate apart from
explicit mode changes. Should we keep this assumption the default and
add API to say different?

> >> > > I think, if the driver cannot implement proper semantics (which IMO
> >> > > includes the throttling) for vblank-sync'd operations and it does not
> >> > > want to fake them with a clock, it should just refuse vblank-synced
> >> > > operations.
> >> >
> >> > Yes refusing vsynced flips for these drivers sounds reasonable. But
> >> > please let's not bake in another assumption in the API (or rather,
> >> > let's try to un-bake it).
> >>
> >> Could you be more specific on everything, please?
> >>
> >> What should drivers do in different situations, what guarantees we do
> >> have, and how does userspace predict the earliest possible flip time?
> >> How do you define flip time to begin with, if it's not tied to the
> >> scanout cycle (vblank)?
> >>
> >> How should a compositor schedule eveything, and what can it tell to the
> >> clients about the timings in the immediate future?
> >>
> >> You gave me the feeling that everything I thought I knew and relied on
> >> is wrong.
> >
> > I guess we either kick out page_flip for all drivers who fake it. And if
> > that's causing regressions then we probably want to fake it with a timer.
> > Unpretty, but such is the game of backwards compat forever. But I'm not
> > sure whether we established that we have a problem already, at least I'm
> > missing users screaming about udl/bochs & friends.

I suppose not hearing users scream is that X.org is not affected,
because it simply doesn't work in a way it would be affected? Or the
various DDX'en. The combination of affected userspace with drivers like
bochs is even more rare.

I first heard about Weston having an abysmal user experience on
drm/bochs when a co-worker was looking at testing Weston in
QEMU/stdvga, which according to others was supposed to be the best
working QEMU output / DRM KMS driver combination. Might have something
to do with needing it on a virtual ARM cpu.

Weston in a VM has not been too attractive before, because of the
missing EGL platform support for swrast, but that has been fixed
recently. I've also heard users (RebeccaBlackOS) to just revert to
Weston's fbdev backend, when the DRM backend just doesn't work right in
a VM.

So I would say it is a known problem with Weston, but users tend to
just dismiss it rather than start pushing it forward. Whether you count
Weston as an existing user in the first place is up to you, I suppose.

I asked Jasper about Mutter:
< pq> Jasper, trying to find users of the KMS API who would work badly,
      if page flips were signalled always immediately
< Jasper> pq, oh, god, us.
< Jasper> pq, we'd spin ourselves in a paint loop to death

I think there might be two different problems here: a) signalling page
flips immediately, and b) "variable refresh rate" systems / on-demand
updates like g-sync, UDL, etc.

My immediate concern is to outlaw immediate signalling of operations
that are intended to be vblank-synced (those that have
traditionally taken time to complete on real hardware, if you don't
like the definition of vblank-synced).

The problem with variable refresh rate systems is unpredictability,
which is different. Submitting updates will take some time in any case,
so you cannot really loop to death (I hope). There I would be happy to
just know that predictions based on perpetual refresh rate are invalid.
That would be enough for Wayland at first.

Stéphane also raised the concern that scanout downclocking etc. may
cause flips to be quite late from predicted. This is less of a problem,
rendering can also take arbitrary times and software is usually written
to deal with missing deadlines. It's a problem that happens all the
time anyway. As it is a problem with prediction accuracy, we could just
wave it off by setting the imaginary "no constant refresh rate" flag.
IMHO this is a problem that should be solved at another occasion, if
necessary.

> Yeah I don't think I care about the old interface, it is what it is.
> But we should design something which works well for the future use
> cases.

Are you referring to the atomic API as the old thing?

In the end, the question is how paranoid should display servers be
about the timings? Do we need to fix Weston, Mutter and likely others
to deal with page flips being signalled immediately, or is it more
appropriate to fix the DRM drivers to keep up the illusion of providing
what the API appears to suggest?


Thanks,
pq
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS timings (Re: [PATCH 6/8] drm/bochs: phase 3: provide a custom ->atomic_commit implementation)

[Daniel Vetter]

On Tue, Jul 21, 2015 at 10:06:09AM +0300, Pekka Paalanen wrote:
> On Mon, 20 Jul 2015 10:32:31 -0700
> Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> 
> > On Mon, Jul 20, 2015 at 7:21 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Mon, Jul 20, 2015 at 12:35:48PM +0300, Pekka Paalanen wrote:
> > >> On Mon, 20 Jul 2015 01:58:33 -0700
> > >> Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> > >>
> > >> > On Mon, Jul 20, 2015 at 12:46 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > >> > > On Sun, 19 Jul 2015 17:20:32 -0700
> > >> > > Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> > >> > >
> > >> > >> On Thu, Jul 16, 2015 at 11:08 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > >> > >> >
> > >> > >> > On Thu, 16 Jul 2015 20:20:39 +0800
> > >> > >> > John Hunter <zhjwpku@gmail.com> wrote:
> > >> > >> >
> > >> > >> > > From: Zhao Junwang <zhjwpku@gmail.com>
> > >> > >> > >
> > >> > >> > > This supports the asynchronous commits, required for page-flipping
> > >> > >> > > Since it's virtual hw it's ok to commit async stuff right away, we
> > >> > >> > > never have to wait for vblank.
> > >> > >> >
> > >> > >> > Hi,
> > >> > >> >
> > >> > >> > in theory, yes. This is what a patch to bochs implemented not too long
> > >> > >> > ago, so AFAIK you are only replicating the existing behaviour.
> > >> > >> >
> > >> > >> > However, if userspace doing an async commit (or sync, I suppose) does
> > >> > >> > not incur any waits in the kernel in e.g. sending the page flip event,
> > >> > >> > then flip driven programs (e.g. a Wayland compositor, say, Weston)
> > >> > >> > will be running its rendering loop as a busy-loop, because the kernel
> > >> > >> > does not throttle it to the (virtual) display refresh rate.
> > >> > >> >
> > >> > >> > This will cause maximal CPU usage and poor user experience as
> > >> > >> > everything else needs to fight for CPU time and event dispatch to get
> > >> > >> > through, like input.
> > >> > >> >
> > >> > >> > I would hope someone could do a follow-up to implement a refresh cycle
> > >> > >> > emulation based on a clock. Userspace expects page flips to happen at
> > >> > >> > most at refresh rate when asking for vblank-synced flips. It's only
> > >> > >> > natural for userspace to drive its rendering loop based on the vblank
> > >> > >> > cycle.
> > >> > >>
> > >> > >>
> > >> > >> I've been asking myself the same question (for the UDL driver) and I'm
> > >> > >> not sure if this policy should go in the kernel. After all, there
> > >> > >> could be legitimate reasons for user space to render lots of frames
> > >> > >> per second. It seems to me that if user space doesn't want too many
> > >> > >> fps, it should just throttle itself.
> > >> > >
> > >> > > If userspace wants to render lots of frames per second, IMO it should
> > >> > > not be using vblank-synced operations in a way that may throttle it.
> > >> > > The lots of frames use case is already non-working for the majority of
> > >> > > the drivers without DRM_MODE_PAGE_FLIP_ASYNC, right?
> > >> > >
> > >> > > The problem here I see is that one DRM driver decides to work different
> > >> > > to other DRM drivers. All real-hardware DRM drivers, when asked to do
> > >> > > vblank-synced update, actually do throttle to the vblank AFAIK.
> > >> >
> > >> > udl is an exception here. It is (arguably) real hardware but doesn't throttle.
> > >> >
> > >> > > Is it
> > >> > > too much to assume, that the video mode set in a driver (refresh rate)
> > >> > > corresponds to the vblank rate which implicitly delays the completion
> > >> > > of vblank-sync'd operations to at least the next vblank boundary?
> > >> >
> > >> > I think it's wrong to make user space think that a vsynced display
> > >> > always matches the refresh rate in a world where:
> > >> >
> > >> > - some displays have variable refresh rates (not just the fancy new
> > >> > stuff like g-sync, look for lvds_downclock in the intel driver for
> > >> > example, also consider DSI displays)
> > >> >
> > >> > - some displays have no refresh rate (the ones we are talking about
> > >> > here: udl, bochs...)
> 
> > > Imo aiming for vrefresh to be accurate is good. For gsync and friends I
> > > think we should have an explicit range or flag to make userspace aware of
> > > what's going on.
> > 
> > I think the concept of vrefresh is flawed and not really future-proof
> > (I gave a few examples in my previous email). I agree we should keep
> > it as legacy, but we should add something else for the more advanced
> > cases.
> 
> Right, so let's add something new for new hardware features and keep
> the existing behavior existing.
> 
> I suppose the problem is that the existing behavior is not really
> documented so we have to resort to screaming users?
> 
> If one does not ask for ASYNC with a page flip, does it mean flipping
> on the next vblank, or flipping such that it cannot tear but allowing
> techiques like scanline waits?

Since legacy page_flip is always for the full primary plane you can't do
scanline waits - it covers everything anyway.

> It used to be reasonable to assume a constant refresh rate apart from
> explicit mode changes. Should we keep this assumption the default and
> add API to say different?
> 
> > >> > > I think, if the driver cannot implement proper semantics (which IMO
> > >> > > includes the throttling) for vblank-sync'd operations and it does not
> > >> > > want to fake them with a clock, it should just refuse vblank-synced
> > >> > > operations.
> > >> >
> > >> > Yes refusing vsynced flips for these drivers sounds reasonable. But
> > >> > please let's not bake in another assumption in the API (or rather,
> > >> > let's try to un-bake it).
> > >>
> > >> Could you be more specific on everything, please?
> > >>
> > >> What should drivers do in different situations, what guarantees we do
> > >> have, and how does userspace predict the earliest possible flip time?
> > >> How do you define flip time to begin with, if it's not tied to the
> > >> scanout cycle (vblank)?
> > >>
> > >> How should a compositor schedule eveything, and what can it tell to the
> > >> clients about the timings in the immediate future?
> > >>
> > >> You gave me the feeling that everything I thought I knew and relied on
> > >> is wrong.
> > >
> > > I guess we either kick out page_flip for all drivers who fake it. And if
> > > that's causing regressions then we probably want to fake it with a timer.
> > > Unpretty, but such is the game of backwards compat forever. But I'm not
> > > sure whether we established that we have a problem already, at least I'm
> > > missing users screaming about udl/bochs & friends.
> 
> I suppose not hearing users scream is that X.org is not affected,
> because it simply doesn't work in a way it would be affected? Or the
> various DDX'en. The combination of affected userspace with drivers like
> bochs is even more rare.
> 
> I first heard about Weston having an abysmal user experience on
> drm/bochs when a co-worker was looking at testing Weston in
> QEMU/stdvga, which according to others was supposed to be the best
> working QEMU output / DRM KMS driver combination. Might have something
> to do with needing it on a virtual ARM cpu.
> 
> Weston in a VM has not been too attractive before, because of the
> missing EGL platform support for swrast, but that has been fixed
> recently. I've also heard users (RebeccaBlackOS) to just revert to
> Weston's fbdev backend, when the DRM backend just doesn't work right in
> a VM.
> 
> So I would say it is a known problem with Weston, but users tend to
> just dismiss it rather than start pushing it forward. Whether you count
> Weston as an existing user in the first place is up to you, I suppose.
> 
> I asked Jasper about Mutter:
> < pq> Jasper, trying to find users of the KMS API who would work badly,
>       if page flips were signalled always immediately
> < Jasper> pq, oh, god, us.
> < Jasper> pq, we'd spin ourselves in a paint loop to death
> 
> I think there might be two different problems here: a) signalling page
> flips immediately, and b) "variable refresh rate" systems / on-demand
> updates like g-sync, UDL, etc.
> 
> My immediate concern is to outlaw immediate signalling of operations
> that are intended to be vblank-synced (those that have
> traditionally taken time to complete on real hardware, if you don't
> like the definition of vblank-synced).
> 
> The problem with variable refresh rate systems is unpredictability,
> which is different. Submitting updates will take some time in any case,
> so you cannot really loop to death (I hope). There I would be happy to
> just know that predictions based on perpetual refresh rate are invalid.
> That would be enough for Wayland at first.
> 
> Stéphane also raised the concern that scanout downclocking etc. may
> cause flips to be quite late from predicted. This is less of a problem,
> rendering can also take arbitrary times and software is usually written
> to deal with missing deadlines. It's a problem that happens all the
> time anyway. As it is a problem with prediction accuracy, we could just
> wave it off by setting the imaginary "no constant refresh rate" flag.
> IMHO this is a problem that should be solved at another occasion, if
> necessary.

Downclocking should only be able to delay the very first frame in a
sequence (video playback, animation). Imo we can shrug that off as "clock
skew" ;-)

Imo once someone updates frames regularly vblank timestamps should be
evenly spaced and pageflips not instant (at least by default).

> > Yeah I don't think I care about the old interface, it is what it is.
> > But we should design something which works well for the future use
> > cases.
> 
> Are you referring to the atomic API as the old thing?
> 
> In the end, the question is how paranoid should display servers be
> about the timings? Do we need to fix Weston, Mutter and likely others
> to deal with page flips being signalled immediately, or is it more
> appropriate to fix the DRM drivers to keep up the illusion of providing
> what the API appears to suggest?

I guess they can't assume too much about vblanks (too many drivers don't
even bother with precise/irq-delay-correct timestamps), but I think
assuming that doing a page_flip completion event based renderer shouldn't
result in spinning is reasonable.

I guess for bochs/udl and others we could create a small drm driver which
keeps track of the last vblank ts (we have those already) and suitable
delays the even/timestamp to keep up the illusion. Or we just rip out
pageflip support for those drivers. But if weston&co can't cope with that
that would be worse.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS timings (Re: [PATCH 6/8] drm/bochs: phase 3: provide a custom ->atomic_commit implementation)

[Pekka Paalanen]

On Tue, 21 Jul 2015 11:02:58 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Jul 21, 2015 at 10:06:09AM +0300, Pekka Paalanen wrote:
> > On Mon, 20 Jul 2015 10:32:31 -0700
> > Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> > 
> > > On Mon, Jul 20, 2015 at 7:21 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > On Mon, Jul 20, 2015 at 12:35:48PM +0300, Pekka Paalanen wrote:
> > > >> On Mon, 20 Jul 2015 01:58:33 -0700
> > > >> Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> > > >>
> > > >> > On Mon, Jul 20, 2015 at 12:46 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > >> > > On Sun, 19 Jul 2015 17:20:32 -0700
> > > >> > > Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> > > >> > >
> > > >> > >> On Thu, Jul 16, 2015 at 11:08 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > >> > >> >
> > > >> > >> > On Thu, 16 Jul 2015 20:20:39 +0800
> > > >> > >> > John Hunter <zhjwpku@gmail.com> wrote:
> > > >> > >> >
> > > >> > >> > > From: Zhao Junwang <zhjwpku@gmail.com>
> > > >> > >> > >
> > > >> > >> > > This supports the asynchronous commits, required for page-flipping
> > > >> > >> > > Since it's virtual hw it's ok to commit async stuff right away, we
> > > >> > >> > > never have to wait for vblank.
> > > >> > >> >
> > > >> > >> > Hi,
> > > >> > >> >
> > > >> > >> > in theory, yes. This is what a patch to bochs implemented not too long
> > > >> > >> > ago, so AFAIK you are only replicating the existing behaviour.
> > > >> > >> >
> > > >> > >> > However, if userspace doing an async commit (or sync, I suppose) does
> > > >> > >> > not incur any waits in the kernel in e.g. sending the page flip event,
> > > >> > >> > then flip driven programs (e.g. a Wayland compositor, say, Weston)
> > > >> > >> > will be running its rendering loop as a busy-loop, because the kernel
> > > >> > >> > does not throttle it to the (virtual) display refresh rate.
> > > >> > >> >
> > > >> > >> > This will cause maximal CPU usage and poor user experience as
> > > >> > >> > everything else needs to fight for CPU time and event dispatch to get
> > > >> > >> > through, like input.
> > > >> > >> >
> > > >> > >> > I would hope someone could do a follow-up to implement a refresh cycle
> > > >> > >> > emulation based on a clock. Userspace expects page flips to happen at
> > > >> > >> > most at refresh rate when asking for vblank-synced flips. It's only
> > > >> > >> > natural for userspace to drive its rendering loop based on the vblank
> > > >> > >> > cycle.
> > > >> > >>
> > > >> > >>
> > > >> > >> I've been asking myself the same question (for the UDL driver) and I'm
> > > >> > >> not sure if this policy should go in the kernel. After all, there
> > > >> > >> could be legitimate reasons for user space to render lots of frames
> > > >> > >> per second. It seems to me that if user space doesn't want too many
> > > >> > >> fps, it should just throttle itself.
> > > >> > >
> > > >> > > If userspace wants to render lots of frames per second, IMO it should
> > > >> > > not be using vblank-synced operations in a way that may throttle it.
> > > >> > > The lots of frames use case is already non-working for the majority of
> > > >> > > the drivers without DRM_MODE_PAGE_FLIP_ASYNC, right?
> > > >> > >
> > > >> > > The problem here I see is that one DRM driver decides to work different
> > > >> > > to other DRM drivers. All real-hardware DRM drivers, when asked to do
> > > >> > > vblank-synced update, actually do throttle to the vblank AFAIK.
> > > >> >
> > > >> > udl is an exception here. It is (arguably) real hardware but doesn't throttle.
> > > >> >
> > > >> > > Is it
> > > >> > > too much to assume, that the video mode set in a driver (refresh rate)
> > > >> > > corresponds to the vblank rate which implicitly delays the completion
> > > >> > > of vblank-sync'd operations to at least the next vblank boundary?
> > > >> >
> > > >> > I think it's wrong to make user space think that a vsynced display
> > > >> > always matches the refresh rate in a world where:
> > > >> >
> > > >> > - some displays have variable refresh rates (not just the fancy new
> > > >> > stuff like g-sync, look for lvds_downclock in the intel driver for
> > > >> > example, also consider DSI displays)
> > > >> >
> > > >> > - some displays have no refresh rate (the ones we are talking about
> > > >> > here: udl, bochs...)
> > 
> > > > Imo aiming for vrefresh to be accurate is good. For gsync and friends I
> > > > think we should have an explicit range or flag to make userspace aware of
> > > > what's going on.
> > > 
> > > I think the concept of vrefresh is flawed and not really future-proof
> > > (I gave a few examples in my previous email). I agree we should keep
> > > it as legacy, but we should add something else for the more advanced
> > > cases.
> > 
> > Right, so let's add something new for new hardware features and keep
> > the existing behavior existing.
> > 
> > I suppose the problem is that the existing behavior is not really
> > documented so we have to resort to screaming users?
> > 
> > If one does not ask for ASYNC with a page flip, does it mean flipping
> > on the next vblank, or flipping such that it cannot tear but allowing
> > techiques like scanline waits?
> 
> Since legacy page_flip is always for the full primary plane you can't do
> scanline waits - it covers everything anyway.

Ah, nice. Obvious in hindsight, at least if the display server does
not mark dirty regions.

But there seems to be drmModeDirtyFB(), can that not be used with page
flip? If userspace provided dirty regions, would scanline-wait be
possible in theory? If yes, would it be acceptable for drivers to do?

> > Stéphane also raised the concern that scanout downclocking etc. may
> > cause flips to be quite late from predicted. This is less of a problem,
> > rendering can also take arbitrary times and software is usually written
> > to deal with missing deadlines. It's a problem that happens all the
> > time anyway. As it is a problem with prediction accuracy, we could just
> > wave it off by setting the imaginary "no constant refresh rate" flag.
> > IMHO this is a problem that should be solved at another occasion, if
> > necessary.
> 
> Downclocking should only be able to delay the very first frame in a
> sequence (video playback, animation). Imo we can shrug that off as "clock
> skew" ;-)

Indeed, and at least Weston has special repaint loop start logic to
deal with that.

> Imo once someone updates frames regularly vblank timestamps should be
> evenly spaced and pageflips not instant (at least by default).

Glad to hear. :-)

> > > Yeah I don't think I care about the old interface, it is what it is.
> > > But we should design something which works well for the future use
> > > cases.
> > 
> > Are you referring to the atomic API as the old thing?
> > 
> > In the end, the question is how paranoid should display servers be
> > about the timings? Do we need to fix Weston, Mutter and likely others
> > to deal with page flips being signalled immediately, or is it more
> > appropriate to fix the DRM drivers to keep up the illusion of providing
> > what the API appears to suggest?
> 
> I guess they can't assume too much about vblanks (too many drivers don't
> even bother with precise/irq-delay-correct timestamps), but I think
> assuming that doing a page_flip completion event based renderer shouldn't
> result in spinning is reasonable.

Right. Reliability of timestamps is a whole another can.

> I guess for bochs/udl and others we could create a small drm driver which
> keeps track of the last vblank ts (we have those already) and suitable
> delays the even/timestamp to keep up the illusion. Or we just rip out
> pageflip support for those drivers. But if weston&co can't cope with that
> that would be worse.

Now that you ask, I'm not even really sure what the non-page-flip KMS
display update way is, so no, I would say Weston cannot deal with it as
is.

Who's up for the job? :-)


Thanks,
pq
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS timings (Re: [PATCH 6/8] drm/bochs: phase 3: provide a custom ->atomic_commit implementation)

[Daniel Vetter]

On Tue, Jul 21, 2015 at 02:22:03PM +0300, Pekka Paalanen wrote:
> On Tue, 21 Jul 2015 11:02:58 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Tue, Jul 21, 2015 at 10:06:09AM +0300, Pekka Paalanen wrote:
> > > On Mon, 20 Jul 2015 10:32:31 -0700
> > > Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:

> > > If one does not ask for ASYNC with a page flip, does it mean flipping
> > > on the next vblank, or flipping such that it cannot tear but allowing
> > > techiques like scanline waits?
> > 
> > Since legacy page_flip is always for the full primary plane you can't do
> > scanline waits - it covers everything anyway.
> 
> Ah, nice. Obvious in hindsight, at least if the display server does
> not mark dirty regions.
> 
> But there seems to be drmModeDirtyFB(), can that not be used with page
> flip? If userspace provided dirty regions, would scanline-wait be
> possible in theory? If yes, would it be acceptable for drivers to do?

DirtyFB is exclusively for frontbuffer rendering. We could easily add a
dirty-x/y/w/h rectangle to atomic (I don't think a list makes sense, most
hw can only do one), but right now there's no way to do FlipWithDamage.
Definitely something we want to look into (and we should be able to make
it work on skl on at least in some cases).

> > I guess for bochs/udl and others we could create a small drm driver which
> > keeps track of the last vblank ts (we have those already) and suitable
> > delays the even/timestamp to keep up the illusion. Or we just rip out
> > pageflip support for those drivers. But if weston&co can't cope with that
> > that would be worse.
> 
> Now that you ask, I'm not even really sure what the non-page-flip KMS
> display update way is, so no, I would say Weston cannot deal with it as
> is.
> 
> Who's up for the job? :-)

Implement a drm_send_fake_vblank_event in drm_irq.c and then roll it out
shouldnt be too much work really. We can store crtc-private date needed
for this in the drm_vblank structure (just need a timer plus maybe a
high-res timestamp to make the illusion better). Then we could replace the
send_vblank_event call with that helper in bochs/udl/...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/8] drm/bochs: phase 3: switch to drm_atomic_helper_page_flip

[John Hunter]

From: Zhao Junwang <zhjwpku@gmail.com>

PageFlips now use the atomic helper to work through the atomic
modesetting API.

v2: -Since driver is now fully atomic, we should add DRIVER_ATOMIC
     to .driver_features.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
 drivers/gpu/drm/bochs/bochs_drv.c |    2 +-
 drivers/gpu/drm/bochs/bochs_kms.c |   22 +---------------------
 2 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
index 98837bd..fc31643 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -79,7 +79,7 @@ static const struct file_operations bochs_fops = {
 };
 
 static struct drm_driver bochs_driver = {
-	.driver_features	= DRIVER_GEM | DRIVER_MODESET,
+	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
 	.load			= bochs_load,
 	.unload			= bochs_unload,
 	.set_busid		= drm_pci_set_busid,
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
index f0e93e1..599f367 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -59,32 +59,12 @@ static void bochs_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
 {
 }
 
-static int bochs_crtc_page_flip(struct drm_crtc *crtc,
-				struct drm_framebuffer *fb,
-				struct drm_pending_vblank_event *event,
-				uint32_t page_flip_flags)
-{
-	struct bochs_device *bochs =
-		container_of(crtc, struct bochs_device, crtc);
-	struct drm_framebuffer *old_fb = crtc->primary->fb;
-	unsigned long irqflags;
-
-	crtc->primary->fb = fb;
-	drm_helper_crtc_mode_set_base(crtc, 0, 0, old_fb);
-	if (event) {
-		spin_lock_irqsave(&bochs->dev->event_lock, irqflags);
-		drm_send_vblank_event(bochs->dev, -1, event);
-		spin_unlock_irqrestore(&bochs->dev->event_lock, irqflags);
-	}
-	return 0;
-}
-
 /* These provide the minimum set of functions required to handle a CRTC */
 static const struct drm_crtc_funcs bochs_crtc_funcs = {
 	.gamma_set = bochs_crtc_gamma_set,
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = drm_crtc_cleanup,
-	.page_flip = bochs_crtc_page_flip,
+	.page_flip = drm_atomic_helper_page_flip,
 	.reset = drm_atomic_helper_crtc_reset,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
-- 
1.7.10.4


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

[PATCH 8/8] drm/bochs: atomic dpms support

[John Hunter]

From: Zhao Junwang <zhjwpku@gmail.com>

- use ->disable() and ->enable() for crct and plane
- use drm_atomic_helper_connector_dpms for connector

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
 drivers/gpu/drm/bochs/bochs_kms.c |   34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
index 599f367..eccd0a7 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -23,16 +23,20 @@ MODULE_PARM_DESC(defy, "default y resolution");
 
 /* ---------------------------------------------------------------------- */
 
-static void bochs_crtc_dpms(struct drm_crtc *crtc, int mode)
+static void bochs_crtc_enable(struct drm_crtc *crtc)
 {
-	switch (mode) {
-	case DRM_MODE_DPMS_ON:
-	case DRM_MODE_DPMS_STANDBY:
-	case DRM_MODE_DPMS_SUSPEND:
-	case DRM_MODE_DPMS_OFF:
-	default:
+	if (crtc->enabled)
 		return;
-	}
+
+	crtc->enabled = true;
+}
+
+static void bochs_crtc_disable(struct drm_crtc *crtc)
+{
+	if (!crtc->enabled)
+		return;
+
+	crtc->enabled = false;
 }
 
 static bool bochs_crtc_mode_fixup(struct drm_crtc *crtc,
@@ -71,7 +75,8 @@ static const struct drm_crtc_funcs bochs_crtc_funcs = {
 };
 
 static const struct drm_crtc_helper_funcs bochs_helper_funcs = {
-	.dpms = bochs_crtc_dpms,
+	.enable = bochs_crtc_enable,
+	.disable = bochs_crtc_disable,
 	.mode_fixup = bochs_crtc_mode_fixup,
 	.mode_set_nofb = bochs_crtc_mode_set_nofb,
 	.commit = bochs_crtc_commit,
@@ -204,7 +209,11 @@ static void bochs_encoder_mode_set(struct drm_encoder *encoder,
 {
 }
 
-static void bochs_encoder_dpms(struct drm_encoder *encoder, int state)
+static void bochs_encoder_enable(struct drm_encoder *encoder)
+{
+}
+
+static void bochs_encoder_disable(struct drm_encoder *encoder)
 {
 }
 
@@ -213,7 +222,8 @@ static void bochs_encoder_commit(struct drm_encoder *encoder)
 }
 
 static const struct drm_encoder_helper_funcs bochs_encoder_helper_funcs = {
-	.dpms = bochs_encoder_dpms,
+	.enable = bochs_encoder_enable,
+	.disable = bochs_encoder_disable,
 	.mode_fixup = bochs_encoder_mode_fixup,
 	.mode_set = bochs_encoder_mode_set,
 	.commit = bochs_encoder_commit,
@@ -286,7 +296,7 @@ struct drm_connector_helper_funcs bochs_connector_connector_helper_funcs = {
 };
 
 struct drm_connector_funcs bochs_connector_connector_funcs = {
-	.dpms = drm_helper_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = bochs_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = drm_connector_cleanup,
-- 
1.7.10.4


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

Re: [PATCH 0/8] drm/bochs: convert bochs to atomic mode-setting

[Emil Velikov]

[+Dave, +Gerd]

Hi all,

Just noticed that this old(ish) series never hit master.

On 16 July 2015 at 13:20, John Hunter <zhjwpku@gmail.com> wrote:
> From: Zhao Junwang <zhjwpku@gmail.com>
>
> This patch series aim to convert DRM_BOCHS to atomic mode-setting.
> I did this mostly reference Gustavo Padovan's patch series on
> drm/exynos conversion.
>
Dave, Gerd, being the authors/maintainers of this driver can you guys
take a look/test.
Ackes, review and testing will be greatly appreciated.

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

Re: [PATCH 0/8] drm/bochs: convert bochs to atomic mode-setting

[Gerd Hoffmann]

On Mi, 2016-03-02 at 10:13 +0000, Emil Velikov wrote:
> [+Dave, +Gerd]
> 
> Hi all,
> 
> Just noticed that this old(ish) series never hit master.
> 
> On 16 July 2015 at 13:20, John Hunter <zhjwpku@gmail.com> wrote:
> > From: Zhao Junwang <zhjwpku@gmail.com>
> >
> > This patch series aim to convert DRM_BOCHS to atomic mode-setting.
> > I did this mostly reference Gustavo Padovan's patch series on
> > drm/exynos conversion.
> >
> Dave, Gerd, being the authors/maintainers of this driver can you guys
> take a look/test.
> Ackes, review and testing will be greatly appreciated.

Did testing a while back & reported back to John (not sure this was in
public on the list as we had some ping-ping emails beforehand due to
some problems of applying the patches).  No new version of the series
since.

cheers,
  Gerd

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

Re: [PATCH 0/8] drm/bochs: convert bochs to atomic mode-setting

[Emil Velikov]

On 2 March 2016 at 15:38, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Mi, 2016-03-02 at 10:13 +0000, Emil Velikov wrote:
>> [+Dave, +Gerd]
>>
>> Hi all,
>>
>> Just noticed that this old(ish) series never hit master.
>>
>> On 16 July 2015 at 13:20, John Hunter <zhjwpku@gmail.com> wrote:
>> > From: Zhao Junwang <zhjwpku@gmail.com>
>> >
>> > This patch series aim to convert DRM_BOCHS to atomic mode-setting.
>> > I did this mostly reference Gustavo Padovan's patch series on
>> > drm/exynos conversion.
>> >
>> Dave, Gerd, being the authors/maintainers of this driver can you guys
>> take a look/test.
>> Ackes, review and testing will be greatly appreciated.
>
> Did testing a while back & reported back to John (not sure this was in
> public on the list as we had some ping-ping emails beforehand due to
> some problems of applying the patches).  No new version of the series
> since.
>
The only comment that I can see is this one [1] which was addressed a
couple of days later [2].
If you have any other comments please bring them up publicly.

Thanks
Emil

[1] https://lists.freedesktop.org/archives/dri-devel/2015-July/086651.html
[2] https://lists.freedesktop.org/archives/dri-devel/2015-July/086776.html
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/8] drm/bochs: convert bochs to atomic mode-setting

[Gerd Hoffmann]

  Hi,

> > Did testing a while back & reported back to John (not sure this was in
> > public on the list as we had some ping-ping emails beforehand due to
> > some problems of applying the patches).  No new version of the series
> > since.
> >
> The only comment that I can see is this one [1] which was addressed a
> couple of days later [2].
> If you have any other comments please bring them up publicly.

Digged the mails up in the archive.

First, v2 posting was apparently incomplete, only one of the 8 patches
landed in my inbox.

John pointed me to
https://github.com/zhjwpku/gsoc/commits/20150720_no_gpu_addr which I've
used to test things.

Second, page flipping failed in testing.

cheers,
  Gerd

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

Re: [PATCH 0/8] drm/bochs: convert bochs to atomic mode-setting

[John Hunter]

[-- Attachment #1.1: Type: text/plain, Size: 962 bytes --]

Hi Gerd,

Sorry about that, but I am now busy with my master's degree stuffs.

Really can't spare any time on this until maybe July.

On Thu, Mar 3, 2016 at 5:28 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
>
> > > Did testing a while back & reported back to John (not sure this was in
> > > public on the list as we had some ping-ping emails beforehand due to
> > > some problems of applying the patches).  No new version of the series
> > > since.
> > >
> > The only comment that I can see is this one [1] which was addressed a
> > couple of days later [2].
> > If you have any other comments please bring them up publicly.
>
> Digged the mails up in the archive.
>
> First, v2 posting was apparently incomplete, only one of the 8 patches
> landed in my inbox.
>
> John pointed me to
> https://github.com/zhjwpku/gsoc/commits/20150720_no_gpu_addr which I've
> used to test things.
>
> Second, page flipping failed in testing.
>
> cheers,
>   Gerd
>
>

[-- Attachment #1.2: Type: text/html, Size: 1548 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 0/8] drm/bochs: convert bochs to atomic mode-setting

[Emil Velikov]

On 3 March 2016 at 09:28, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>> > Did testing a while back & reported back to John (not sure this was in
>> > public on the list as we had some ping-ping emails beforehand due to
>> > some problems of applying the patches).  No new version of the series
>> > since.
>> >
>> The only comment that I can see is this one [1] which was addressed a
>> couple of days later [2].
>> If you have any other comments please bring them up publicly.
>
> Digged the mails up in the archive.
>
Silly me assumed that the MUA will bring up the the whole thread. Will
assume less and post links for the future.

> First, v2 posting was apparently incomplete, only one of the 8 patches
> landed in my inbox.
>
Pretty sure that's intentional since only the first patch has changed.
Up-to recently that was the more commonly used approach on dri-devel.

> John pointed me to
> https://github.com/zhjwpku/gsoc/commits/20150720_no_gpu_addr which I've
> used to test things.
>
> Second, page flipping failed in testing.
>
Thank you for this missing piece of the puzzle. You have to admit that
without it, things looked as if the patches fell through the cracks.

If you can share some tips about the testing procedure and/or actual
issue that would be beneficial. I'm not personally aiming to tackle
this, although the information will be appreciated.

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

Re: [PATCH 0/8] drm/bochs: convert bochs to atomic mode-setting

[Gerd Hoffmann]

  Hi,

> > Digged the mails up in the archive.
> >
> Silly me assumed that the MUA will bring up the the whole thread.

If your mbox goes back that far ...

I usually have a few months history in my mailbox folders which is
enough in the vast majority of the cases where I have to check some past
mail, but wasn't in this case.

> Pretty sure that's intentional since only the first patch has changed.
> Up-to recently that was the more commonly used approach on dri-devel.

Ah, ok.  That explains it.

> > John pointed me to
> > https://github.com/zhjwpku/gsoc/commits/20150720_no_gpu_addr which I've
> > used to test things.
> >
> > Second, page flipping failed in testing.

> If you can share some tips about the testing procedure and/or actual
> issue that would be beneficial.

Simple: wayland breaks.  kmscon uses pageflips too and can also be used
for testing (and this is alot easier in case your distro hasn't packaged
up wayland).

cheers,
  Gerd

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