[PATCH 0/7] drm: Random pile of core stuff

[PATCH 0/7] drm: Random pile of core stuff

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I found this random pile of stuff lying around. Dusted it off
and tossed in the new selftests.

Ville Syrjälä (7):
  drm: Move page_flip fb lookup earlier
  drm: Allocate the page flip event earlier
  drm: Extract page_flip_{internal,atomic}()
  drm: Simplify the setplane old_fb handling further
  drm/selftests: Add some selftests for drm_atomic_set_mode_for_crtc()
  drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()
  drm/atomic: Reduce setplane locking

 drivers/gpu/drm/drm_atomic_uapi.c             |   9 +-
 drivers/gpu/drm/drm_plane.c                   | 285 +++++++++++-------
 drivers/gpu/drm/selftests/Makefile            |   3 +-
 .../gpu/drm/selftests/drm_modeset_selftests.h |   2 +
 .../gpu/drm/selftests/test-drm_atomic_uapi.c  | 110 +++++++
 .../drm/selftests/test-drm_modeset_common.h   |   2 +
 6 files changed, 291 insertions(+), 120 deletions(-)
 create mode 100644 drivers/gpu/drm/selftests/test-drm_atomic_uapi.c

-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 0/7] drm: Random pile of core stuff

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I found this random pile of stuff lying around. Dusted it off
and tossed in the new selftests.

Ville Syrjälä (7):
  drm: Move page_flip fb lookup earlier
  drm: Allocate the page flip event earlier
  drm: Extract page_flip_{internal,atomic}()
  drm: Simplify the setplane old_fb handling further
  drm/selftests: Add some selftests for drm_atomic_set_mode_for_crtc()
  drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()
  drm/atomic: Reduce setplane locking

 drivers/gpu/drm/drm_atomic_uapi.c             |   9 +-
 drivers/gpu/drm/drm_plane.c                   | 285 +++++++++++-------
 drivers/gpu/drm/selftests/Makefile            |   3 +-
 .../gpu/drm/selftests/drm_modeset_selftests.h |   2 +
 .../gpu/drm/selftests/test-drm_atomic_uapi.c  | 110 +++++++
 .../drm/selftests/test-drm_modeset_common.h   |   2 +
 6 files changed, 291 insertions(+), 120 deletions(-)
 create mode 100644 drivers/gpu/drm/selftests/test-drm_atomic_uapi.c

-- 
2.23.0

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

[PATCH 1/7] drm: Move page_flip fb lookup earlier

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

No reason that I can see to delay the fb lookup this late. Moving it
earlier allows us to keep it outside of the lock retry loop. This
makes error handling and whatnot simpler.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index d6ad60ab0d38..14a292d7817d 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1037,7 +1037,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	struct drm_mode_crtc_page_flip_target *page_flip = data;
 	struct drm_crtc *crtc;
 	struct drm_plane *plane;
-	struct drm_framebuffer *fb = NULL, *old_fb;
+	struct drm_framebuffer *fb, *old_fb;
 	struct drm_pending_vblank_event *e = NULL;
 	u32 target_vblank = page_flip->sequence;
 	struct drm_modeset_acquire_ctx ctx;
@@ -1086,16 +1086,16 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 				DRM_DEBUG("Invalid absolute flip target %u, "
 					  "must be <= %u\n", target_vblank,
 					  current_vblank + 1);
-				drm_crtc_vblank_put(crtc);
-				return -EINVAL;
+				ret = -EINVAL;
+				goto put_vblank;
 			}
 			break;
 		case DRM_MODE_PAGE_FLIP_TARGET_RELATIVE:
 			if (target_vblank != 0 && target_vblank != 1) {
 				DRM_DEBUG("Invalid relative flip target %u, "
 					  "must be 0 or 1\n", target_vblank);
-				drm_crtc_vblank_put(crtc);
-				return -EINVAL;
+				ret = -EINVAL;
+				goto put_vblank;
 			}
 			target_vblank += current_vblank;
 			break;
@@ -1106,7 +1106,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		}
 	} else if (crtc->funcs->page_flip == NULL ||
 		   (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET)) {
-		return -EINVAL;
+		ret = -EINVAL;
+		goto put_vblank;
+	}
+
+	fb = drm_framebuffer_lookup(dev, file_priv, page_flip->fb_id);
+	if (!fb) {
+		ret = -ENOENT;
+		goto put_vblank;
 	}
 
 	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
@@ -1132,12 +1139,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
-	fb = drm_framebuffer_lookup(dev, file_priv, page_flip->fb_id);
-	if (!fb) {
-		ret = -ENOENT;
-		goto out;
-	}
-
 	if (plane->state) {
 		const struct drm_plane_state *state = plane->state;
 
@@ -1201,8 +1202,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	}
 
 out:
-	if (fb)
-		drm_framebuffer_put(fb);
 	if (plane->old_fb)
 		drm_framebuffer_put(plane->old_fb);
 	plane->old_fb = NULL;
@@ -1216,6 +1215,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 
+	drm_framebuffer_put(fb);
+put_vblank:
 	if (ret && crtc->funcs->page_flip_target)
 		drm_crtc_vblank_put(crtc);
 
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/7] drm: Move page_flip fb lookup earlier

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

No reason that I can see to delay the fb lookup this late. Moving it
earlier allows us to keep it outside of the lock retry loop. This
makes error handling and whatnot simpler.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index d6ad60ab0d38..14a292d7817d 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1037,7 +1037,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	struct drm_mode_crtc_page_flip_target *page_flip = data;
 	struct drm_crtc *crtc;
 	struct drm_plane *plane;
-	struct drm_framebuffer *fb = NULL, *old_fb;
+	struct drm_framebuffer *fb, *old_fb;
 	struct drm_pending_vblank_event *e = NULL;
 	u32 target_vblank = page_flip->sequence;
 	struct drm_modeset_acquire_ctx ctx;
@@ -1086,16 +1086,16 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 				DRM_DEBUG("Invalid absolute flip target %u, "
 					  "must be <= %u\n", target_vblank,
 					  current_vblank + 1);
-				drm_crtc_vblank_put(crtc);
-				return -EINVAL;
+				ret = -EINVAL;
+				goto put_vblank;
 			}
 			break;
 		case DRM_MODE_PAGE_FLIP_TARGET_RELATIVE:
 			if (target_vblank != 0 && target_vblank != 1) {
 				DRM_DEBUG("Invalid relative flip target %u, "
 					  "must be 0 or 1\n", target_vblank);
-				drm_crtc_vblank_put(crtc);
-				return -EINVAL;
+				ret = -EINVAL;
+				goto put_vblank;
 			}
 			target_vblank += current_vblank;
 			break;
@@ -1106,7 +1106,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		}
 	} else if (crtc->funcs->page_flip == NULL ||
 		   (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET)) {
-		return -EINVAL;
+		ret = -EINVAL;
+		goto put_vblank;
+	}
+
+	fb = drm_framebuffer_lookup(dev, file_priv, page_flip->fb_id);
+	if (!fb) {
+		ret = -ENOENT;
+		goto put_vblank;
 	}
 
 	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
@@ -1132,12 +1139,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
-	fb = drm_framebuffer_lookup(dev, file_priv, page_flip->fb_id);
-	if (!fb) {
-		ret = -ENOENT;
-		goto out;
-	}
-
 	if (plane->state) {
 		const struct drm_plane_state *state = plane->state;
 
@@ -1201,8 +1202,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	}
 
 out:
-	if (fb)
-		drm_framebuffer_put(fb);
 	if (plane->old_fb)
 		drm_framebuffer_put(plane->old_fb);
 	plane->old_fb = NULL;
@@ -1216,6 +1215,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 
+	drm_framebuffer_put(fb);
+put_vblank:
 	if (ret && crtc->funcs->page_flip_target)
 		drm_crtc_vblank_put(crtc);
 
-- 
2.23.0

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

[PATCH 2/7] drm: Allocate the page flip event earlier

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Can't see why we need to delay the page flip event allocation until the
last moment. Move it earlier to simplify error handling.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 45 +++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 14a292d7817d..38878da5b704 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1116,6 +1116,26 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto put_vblank;
 	}
 
+	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
+		e = kzalloc(sizeof(*e), GFP_KERNEL);
+		if (!e) {
+			ret = -ENOMEM;
+			goto put_fb;
+		}
+
+		e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
+		e->event.base.length = sizeof(e->event);
+		e->event.vbl.user_data = page_flip->user_data;
+		e->event.vbl.crtc_id = crtc->base.id;
+
+		ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
+		if (ret) {
+			kfree(e);
+			e = NULL;
+			goto put_fb;
+		}
+	}
+
 	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
 retry:
 	ret = drm_modeset_lock(&crtc->mutex, &ctx);
@@ -1160,26 +1180,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
-	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
-		e = kzalloc(sizeof *e, GFP_KERNEL);
-		if (!e) {
-			ret = -ENOMEM;
-			goto out;
-		}
-
-		e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
-		e->event.base.length = sizeof(e->event);
-		e->event.vbl.user_data = page_flip->user_data;
-		e->event.vbl.crtc_id = crtc->base.id;
-
-		ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
-		if (ret) {
-			kfree(e);
-			e = NULL;
-			goto out;
-		}
-	}
-
 	plane->old_fb = plane->fb;
 	if (crtc->funcs->page_flip_target)
 		ret = crtc->funcs->page_flip_target(crtc, fb, e,
@@ -1190,8 +1190,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags,
 					     &ctx);
 	if (ret) {
-		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT)
-			drm_event_cancel_free(dev, &e->base);
 		/* Keep the old fb, don't unref it. */
 		plane->old_fb = NULL;
 	} else {
@@ -1215,6 +1213,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 
+	if (ret && e)
+		drm_event_cancel_free(dev, &e->base);
+put_fb:
 	drm_framebuffer_put(fb);
 put_vblank:
 	if (ret && crtc->funcs->page_flip_target)
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/7] drm: Allocate the page flip event earlier

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Can't see why we need to delay the page flip event allocation until the
last moment. Move it earlier to simplify error handling.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 45 +++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 14a292d7817d..38878da5b704 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1116,6 +1116,26 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto put_vblank;
 	}
 
+	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
+		e = kzalloc(sizeof(*e), GFP_KERNEL);
+		if (!e) {
+			ret = -ENOMEM;
+			goto put_fb;
+		}
+
+		e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
+		e->event.base.length = sizeof(e->event);
+		e->event.vbl.user_data = page_flip->user_data;
+		e->event.vbl.crtc_id = crtc->base.id;
+
+		ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
+		if (ret) {
+			kfree(e);
+			e = NULL;
+			goto put_fb;
+		}
+	}
+
 	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
 retry:
 	ret = drm_modeset_lock(&crtc->mutex, &ctx);
@@ -1160,26 +1180,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
-	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
-		e = kzalloc(sizeof *e, GFP_KERNEL);
-		if (!e) {
-			ret = -ENOMEM;
-			goto out;
-		}
-
-		e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
-		e->event.base.length = sizeof(e->event);
-		e->event.vbl.user_data = page_flip->user_data;
-		e->event.vbl.crtc_id = crtc->base.id;
-
-		ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
-		if (ret) {
-			kfree(e);
-			e = NULL;
-			goto out;
-		}
-	}
-
 	plane->old_fb = plane->fb;
 	if (crtc->funcs->page_flip_target)
 		ret = crtc->funcs->page_flip_target(crtc, fb, e,
@@ -1190,8 +1190,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags,
 					     &ctx);
 	if (ret) {
-		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT)
-			drm_event_cancel_free(dev, &e->base);
 		/* Keep the old fb, don't unref it. */
 		plane->old_fb = NULL;
 	} else {
@@ -1215,6 +1213,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 
+	if (ret && e)
+		drm_event_cancel_free(dev, &e->base);
+put_fb:
 	drm_framebuffer_put(fb);
 put_vblank:
 	if (ret && crtc->funcs->page_flip_target)
-- 
2.23.0

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

[PATCH 3/7] drm: Extract page_flip_{internal, atomic}()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Yank out the code for the plane->fb/old_fb/crtc handling from
the page flip path into page_flip_internal(), and provide a
simpler variant for atomic drivers.

We'll also move the fb vs. src viewport checks into the new
functions as they are slightly different between the two paths.
If the atomic .page_flip() implementations are guaranteed
to call drm_atomic_plane_check() we could even drop the check
entirely from page_flip_atomic(). For now toss in a FIXME.

v2: Bit more polish in page_flip_internal()

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
 1 file changed, 102 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 38878da5b704..6052475a20a5 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
 	return drm_mode_cursor_common(dev, req, file_priv);
 }
 
+static int page_flip_check_fbs(const struct drm_framebuffer *fb,
+			       const struct drm_framebuffer *old_fb)
+{
+	/* The framebuffer is currently unbound, presumably
+	 * due to a hotplug event, that userspace has not
+	 * yet discovered.
+	 */
+	if (!old_fb)
+		return -EBUSY;
+
+	if (old_fb->format != fb->format) {
+		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int page_flip_internal(struct drm_crtc *crtc,
+			      struct drm_framebuffer *fb,
+			      struct drm_pending_vblank_event *e,
+			      u32 flags,
+			      u32 target_vblank,
+			      struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_plane *plane = crtc->primary;
+	int ret;
+
+	WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
+
+	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
+				      &crtc->mode, fb);
+	if (ret)
+		return ret;
+
+	ret = page_flip_check_fbs(fb, plane->fb);
+	if (ret)
+		return ret;
+
+	plane->old_fb = plane->fb;
+	if (crtc->funcs->page_flip_target)
+		ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
+						    target_vblank, ctx);
+	else
+		ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
+	if (ret) {
+		plane->old_fb = NULL;
+		return ret;
+	}
+
+	plane->fb = fb;
+	drm_framebuffer_get(plane->fb);
+
+	drm_framebuffer_put(plane->old_fb);
+	plane->old_fb = NULL;
+
+	return 0;
+}
+
+static int page_flip_atomic(struct drm_crtc *crtc,
+			    struct drm_framebuffer *fb,
+			    struct drm_pending_vblank_event *e,
+			    u32 flags,
+			    u32 target_vblank,
+			    struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_plane *plane = crtc->primary;
+	struct drm_plane_state *plane_state = plane->state;
+	int ret;
+
+	WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
+
+	/*
+	 * FIXME: Can we assume all drivers end up calling
+	 * drm_atomic_plane_check() in their page flip paths?
+	 * If so we could remove this.
+	 */
+	ret = drm_framebuffer_check_src_coords(plane_state->src_x,
+					       plane_state->src_y,
+					       plane_state->src_w,
+					       plane_state->src_h,
+					       fb);
+	if (ret)
+		return ret;
+
+	ret = page_flip_check_fbs(fb, plane_state->fb);
+	if (ret)
+		return ret;
+
+	if (crtc->funcs->page_flip_target)
+		return crtc->funcs->page_flip_target(crtc, fb, e, flags,
+						     target_vblank, ctx);
+	else
+		return crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
+}
+
 int drm_mode_page_flip_ioctl(struct drm_device *dev,
 			     void *data, struct drm_file *file_priv)
 {
 	struct drm_mode_crtc_page_flip_target *page_flip = data;
 	struct drm_crtc *crtc;
 	struct drm_plane *plane;
-	struct drm_framebuffer *fb, *old_fb;
+	struct drm_framebuffer *fb;
 	struct drm_pending_vblank_event *e = NULL;
 	u32 target_vblank = page_flip->sequence;
 	struct drm_modeset_acquire_ctx ctx;
@@ -1145,65 +1241,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (ret)
 		goto out;
 
-	if (plane->state)
-		old_fb = plane->state->fb;
+	if (drm_drv_uses_atomic_modeset(dev))
+		ret = page_flip_atomic(crtc, fb, e, page_flip->flags,
+				       target_vblank, &ctx);
 	else
-		old_fb = plane->fb;
-
-	if (old_fb == NULL) {
-		/* The framebuffer is currently unbound, presumably
-		 * due to a hotplug event, that userspace has not
-		 * yet discovered.
-		 */
-		ret = -EBUSY;
-		goto out;
-	}
-
-	if (plane->state) {
-		const struct drm_plane_state *state = plane->state;
-
-		ret = drm_framebuffer_check_src_coords(state->src_x,
-						       state->src_y,
-						       state->src_w,
-						       state->src_h,
-						       fb);
-	} else {
-		ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
-					      &crtc->mode, fb);
-	}
-	if (ret)
-		goto out;
-
-	if (old_fb->format != fb->format) {
-		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
-	plane->old_fb = plane->fb;
-	if (crtc->funcs->page_flip_target)
-		ret = crtc->funcs->page_flip_target(crtc, fb, e,
-						    page_flip->flags,
-						    target_vblank,
-						    &ctx);
-	else
-		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags,
-					     &ctx);
-	if (ret) {
-		/* Keep the old fb, don't unref it. */
-		plane->old_fb = NULL;
-	} else {
-		if (!plane->state) {
-			plane->fb = fb;
-			drm_framebuffer_get(fb);
-		}
-	}
+		ret = page_flip_internal(crtc, fb, e, page_flip->flags,
+					 target_vblank, &ctx);
 
 out:
-	if (plane->old_fb)
-		drm_framebuffer_put(plane->old_fb);
-	plane->old_fb = NULL;
-
 	if (ret == -EDEADLK) {
 		ret = drm_modeset_backoff(&ctx);
 		if (!ret)
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/7] drm: Extract page_flip_{internal,atomic}()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Yank out the code for the plane->fb/old_fb/crtc handling from
the page flip path into page_flip_internal(), and provide a
simpler variant for atomic drivers.

We'll also move the fb vs. src viewport checks into the new
functions as they are slightly different between the two paths.
If the atomic .page_flip() implementations are guaranteed
to call drm_atomic_plane_check() we could even drop the check
entirely from page_flip_atomic(). For now toss in a FIXME.

v2: Bit more polish in page_flip_internal()

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
 1 file changed, 102 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 38878da5b704..6052475a20a5 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
 	return drm_mode_cursor_common(dev, req, file_priv);
 }
 
+static int page_flip_check_fbs(const struct drm_framebuffer *fb,
+			       const struct drm_framebuffer *old_fb)
+{
+	/* The framebuffer is currently unbound, presumably
+	 * due to a hotplug event, that userspace has not
+	 * yet discovered.
+	 */
+	if (!old_fb)
+		return -EBUSY;
+
+	if (old_fb->format != fb->format) {
+		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int page_flip_internal(struct drm_crtc *crtc,
+			      struct drm_framebuffer *fb,
+			      struct drm_pending_vblank_event *e,
+			      u32 flags,
+			      u32 target_vblank,
+			      struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_plane *plane = crtc->primary;
+	int ret;
+
+	WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
+
+	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
+				      &crtc->mode, fb);
+	if (ret)
+		return ret;
+
+	ret = page_flip_check_fbs(fb, plane->fb);
+	if (ret)
+		return ret;
+
+	plane->old_fb = plane->fb;
+	if (crtc->funcs->page_flip_target)
+		ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
+						    target_vblank, ctx);
+	else
+		ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
+	if (ret) {
+		plane->old_fb = NULL;
+		return ret;
+	}
+
+	plane->fb = fb;
+	drm_framebuffer_get(plane->fb);
+
+	drm_framebuffer_put(plane->old_fb);
+	plane->old_fb = NULL;
+
+	return 0;
+}
+
+static int page_flip_atomic(struct drm_crtc *crtc,
+			    struct drm_framebuffer *fb,
+			    struct drm_pending_vblank_event *e,
+			    u32 flags,
+			    u32 target_vblank,
+			    struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_plane *plane = crtc->primary;
+	struct drm_plane_state *plane_state = plane->state;
+	int ret;
+
+	WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
+
+	/*
+	 * FIXME: Can we assume all drivers end up calling
+	 * drm_atomic_plane_check() in their page flip paths?
+	 * If so we could remove this.
+	 */
+	ret = drm_framebuffer_check_src_coords(plane_state->src_x,
+					       plane_state->src_y,
+					       plane_state->src_w,
+					       plane_state->src_h,
+					       fb);
+	if (ret)
+		return ret;
+
+	ret = page_flip_check_fbs(fb, plane_state->fb);
+	if (ret)
+		return ret;
+
+	if (crtc->funcs->page_flip_target)
+		return crtc->funcs->page_flip_target(crtc, fb, e, flags,
+						     target_vblank, ctx);
+	else
+		return crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
+}
+
 int drm_mode_page_flip_ioctl(struct drm_device *dev,
 			     void *data, struct drm_file *file_priv)
 {
 	struct drm_mode_crtc_page_flip_target *page_flip = data;
 	struct drm_crtc *crtc;
 	struct drm_plane *plane;
-	struct drm_framebuffer *fb, *old_fb;
+	struct drm_framebuffer *fb;
 	struct drm_pending_vblank_event *e = NULL;
 	u32 target_vblank = page_flip->sequence;
 	struct drm_modeset_acquire_ctx ctx;
@@ -1145,65 +1241,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (ret)
 		goto out;
 
-	if (plane->state)
-		old_fb = plane->state->fb;
+	if (drm_drv_uses_atomic_modeset(dev))
+		ret = page_flip_atomic(crtc, fb, e, page_flip->flags,
+				       target_vblank, &ctx);
 	else
-		old_fb = plane->fb;
-
-	if (old_fb == NULL) {
-		/* The framebuffer is currently unbound, presumably
-		 * due to a hotplug event, that userspace has not
-		 * yet discovered.
-		 */
-		ret = -EBUSY;
-		goto out;
-	}
-
-	if (plane->state) {
-		const struct drm_plane_state *state = plane->state;
-
-		ret = drm_framebuffer_check_src_coords(state->src_x,
-						       state->src_y,
-						       state->src_w,
-						       state->src_h,
-						       fb);
-	} else {
-		ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
-					      &crtc->mode, fb);
-	}
-	if (ret)
-		goto out;
-
-	if (old_fb->format != fb->format) {
-		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
-	plane->old_fb = plane->fb;
-	if (crtc->funcs->page_flip_target)
-		ret = crtc->funcs->page_flip_target(crtc, fb, e,
-						    page_flip->flags,
-						    target_vblank,
-						    &ctx);
-	else
-		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags,
-					     &ctx);
-	if (ret) {
-		/* Keep the old fb, don't unref it. */
-		plane->old_fb = NULL;
-	} else {
-		if (!plane->state) {
-			plane->fb = fb;
-			drm_framebuffer_get(fb);
-		}
-	}
+		ret = page_flip_internal(crtc, fb, e, page_flip->flags,
+					 target_vblank, &ctx);
 
 out:
-	if (plane->old_fb)
-		drm_framebuffer_put(plane->old_fb);
-	plane->old_fb = NULL;
-
 	if (ret == -EDEADLK) {
 		ret = drm_modeset_backoff(&ctx);
 		if (!ret)
-- 
2.23.0

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

Re: [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()

[Daniel Vetter]

On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Yank out the code for the plane->fb/old_fb/crtc handling from
> the page flip path into page_flip_internal(), and provide a
> simpler variant for atomic drivers.
> 
> We'll also move the fb vs. src viewport checks into the new
> functions as they are slightly different between the two paths.
> If the atomic .page_flip() implementations are guaranteed
> to call drm_atomic_plane_check() we could even drop the check
> entirely from page_flip_atomic(). For now toss in a FIXME.
> 
> v2: Bit more polish in page_flip_internal()
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
>  1 file changed, 102 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 38878da5b704..6052475a20a5 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
>  	return drm_mode_cursor_common(dev, req, file_priv);
>  }
>  
> +static int page_flip_check_fbs(const struct drm_framebuffer *fb,
> +			       const struct drm_framebuffer *old_fb)
> +{
> +	/* The framebuffer is currently unbound, presumably
> +	 * due to a hotplug event, that userspace has not
> +	 * yet discovered.
> +	 */
> +	if (!old_fb)
> +		return -EBUSY;
> +
> +	if (old_fb->format != fb->format) {
> +		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int page_flip_internal(struct drm_crtc *crtc,
> +			      struct drm_framebuffer *fb,
> +			      struct drm_pending_vblank_event *e,
> +			      u32 flags,
> +			      u32 target_vblank,
> +			      struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	int ret;
> +
> +	WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
> +
> +	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> +				      &crtc->mode, fb);
> +	if (ret)
> +		return ret;
> +
> +	ret = page_flip_check_fbs(fb, plane->fb);
> +	if (ret)
> +		return ret;
> +
> +	plane->old_fb = plane->fb;
> +	if (crtc->funcs->page_flip_target)
> +		ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
> +						    target_vblank, ctx);
> +	else
> +		ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> +	if (ret) {
> +		plane->old_fb = NULL;
> +		return ret;
> +	}
> +
> +	plane->fb = fb;
> +	drm_framebuffer_get(plane->fb);
> +
> +	drm_framebuffer_put(plane->old_fb);
> +	plane->old_fb = NULL;
> +
> +	return 0;
> +}
> +
> +static int page_flip_atomic(struct drm_crtc *crtc,
> +			    struct drm_framebuffer *fb,
> +			    struct drm_pending_vblank_event *e,
> +			    u32 flags,
> +			    u32 target_vblank,
> +			    struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_plane_state *plane_state = plane->state;
> +	int ret;
> +
> +	WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
> +
> +	/*
> +	 * FIXME: Can we assume all drivers end up calling
> +	 * drm_atomic_plane_check() in their page flip paths?
> +	 * If so we could remove this.
> +	 */

This is definitely wrong, core has to check these. Currently no driver
checks this at all. I think you're thinking of
drm_atomic_helper_check_plane_state().

That aside I'm kinda not getting the point of the shuffling/cleanup in the
first 4 patches here, so will skip them.
-Daniel

> +	ret = drm_framebuffer_check_src_coords(plane_state->src_x,
> +					       plane_state->src_y,
> +					       plane_state->src_w,
> +					       plane_state->src_h,
> +					       fb);
> +	if (ret)
> +		return ret;
> +
> +	ret = page_flip_check_fbs(fb, plane_state->fb);
> +	if (ret)
> +		return ret;
> +
> +	if (crtc->funcs->page_flip_target)
> +		return crtc->funcs->page_flip_target(crtc, fb, e, flags,
> +						     target_vblank, ctx);
> +	else
> +		return crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> +}
> +
>  int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  			     void *data, struct drm_file *file_priv)
>  {
>  	struct drm_mode_crtc_page_flip_target *page_flip = data;
>  	struct drm_crtc *crtc;
>  	struct drm_plane *plane;
> -	struct drm_framebuffer *fb, *old_fb;
> +	struct drm_framebuffer *fb;
>  	struct drm_pending_vblank_event *e = NULL;
>  	u32 target_vblank = page_flip->sequence;
>  	struct drm_modeset_acquire_ctx ctx;
> @@ -1145,65 +1241,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (ret)
>  		goto out;
>  
> -	if (plane->state)
> -		old_fb = plane->state->fb;
> +	if (drm_drv_uses_atomic_modeset(dev))
> +		ret = page_flip_atomic(crtc, fb, e, page_flip->flags,
> +				       target_vblank, &ctx);
>  	else
> -		old_fb = plane->fb;
> -
> -	if (old_fb == NULL) {
> -		/* The framebuffer is currently unbound, presumably
> -		 * due to a hotplug event, that userspace has not
> -		 * yet discovered.
> -		 */
> -		ret = -EBUSY;
> -		goto out;
> -	}
> -
> -	if (plane->state) {
> -		const struct drm_plane_state *state = plane->state;
> -
> -		ret = drm_framebuffer_check_src_coords(state->src_x,
> -						       state->src_y,
> -						       state->src_w,
> -						       state->src_h,
> -						       fb);
> -	} else {
> -		ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> -					      &crtc->mode, fb);
> -	}
> -	if (ret)
> -		goto out;
> -
> -	if (old_fb->format != fb->format) {
> -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
> -	plane->old_fb = plane->fb;
> -	if (crtc->funcs->page_flip_target)
> -		ret = crtc->funcs->page_flip_target(crtc, fb, e,
> -						    page_flip->flags,
> -						    target_vblank,
> -						    &ctx);
> -	else
> -		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags,
> -					     &ctx);
> -	if (ret) {
> -		/* Keep the old fb, don't unref it. */
> -		plane->old_fb = NULL;
> -	} else {
> -		if (!plane->state) {
> -			plane->fb = fb;
> -			drm_framebuffer_get(fb);
> -		}
> -	}
> +		ret = page_flip_internal(crtc, fb, e, page_flip->flags,
> +					 target_vblank, &ctx);
>  
>  out:
> -	if (plane->old_fb)
> -		drm_framebuffer_put(plane->old_fb);
> -	plane->old_fb = NULL;
> -
>  	if (ret == -EDEADLK) {
>  		ret = drm_modeset_backoff(&ctx);
>  		if (!ret)
> -- 
> 2.23.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()

[Daniel Vetter]

On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Yank out the code for the plane->fb/old_fb/crtc handling from
> the page flip path into page_flip_internal(), and provide a
> simpler variant for atomic drivers.
> 
> We'll also move the fb vs. src viewport checks into the new
> functions as they are slightly different between the two paths.
> If the atomic .page_flip() implementations are guaranteed
> to call drm_atomic_plane_check() we could even drop the check
> entirely from page_flip_atomic(). For now toss in a FIXME.
> 
> v2: Bit more polish in page_flip_internal()
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
>  1 file changed, 102 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 38878da5b704..6052475a20a5 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
>  	return drm_mode_cursor_common(dev, req, file_priv);
>  }
>  
> +static int page_flip_check_fbs(const struct drm_framebuffer *fb,
> +			       const struct drm_framebuffer *old_fb)
> +{
> +	/* The framebuffer is currently unbound, presumably
> +	 * due to a hotplug event, that userspace has not
> +	 * yet discovered.
> +	 */
> +	if (!old_fb)
> +		return -EBUSY;
> +
> +	if (old_fb->format != fb->format) {
> +		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int page_flip_internal(struct drm_crtc *crtc,
> +			      struct drm_framebuffer *fb,
> +			      struct drm_pending_vblank_event *e,
> +			      u32 flags,
> +			      u32 target_vblank,
> +			      struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	int ret;
> +
> +	WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
> +
> +	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> +				      &crtc->mode, fb);
> +	if (ret)
> +		return ret;
> +
> +	ret = page_flip_check_fbs(fb, plane->fb);
> +	if (ret)
> +		return ret;
> +
> +	plane->old_fb = plane->fb;
> +	if (crtc->funcs->page_flip_target)
> +		ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
> +						    target_vblank, ctx);
> +	else
> +		ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> +	if (ret) {
> +		plane->old_fb = NULL;
> +		return ret;
> +	}
> +
> +	plane->fb = fb;
> +	drm_framebuffer_get(plane->fb);
> +
> +	drm_framebuffer_put(plane->old_fb);
> +	plane->old_fb = NULL;
> +
> +	return 0;
> +}
> +
> +static int page_flip_atomic(struct drm_crtc *crtc,
> +			    struct drm_framebuffer *fb,
> +			    struct drm_pending_vblank_event *e,
> +			    u32 flags,
> +			    u32 target_vblank,
> +			    struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_plane_state *plane_state = plane->state;
> +	int ret;
> +
> +	WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
> +
> +	/*
> +	 * FIXME: Can we assume all drivers end up calling
> +	 * drm_atomic_plane_check() in their page flip paths?
> +	 * If so we could remove this.
> +	 */

This is definitely wrong, core has to check these. Currently no driver
checks this at all. I think you're thinking of
drm_atomic_helper_check_plane_state().

That aside I'm kinda not getting the point of the shuffling/cleanup in the
first 4 patches here, so will skip them.
-Daniel

> +	ret = drm_framebuffer_check_src_coords(plane_state->src_x,
> +					       plane_state->src_y,
> +					       plane_state->src_w,
> +					       plane_state->src_h,
> +					       fb);
> +	if (ret)
> +		return ret;
> +
> +	ret = page_flip_check_fbs(fb, plane_state->fb);
> +	if (ret)
> +		return ret;
> +
> +	if (crtc->funcs->page_flip_target)
> +		return crtc->funcs->page_flip_target(crtc, fb, e, flags,
> +						     target_vblank, ctx);
> +	else
> +		return crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> +}
> +
>  int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  			     void *data, struct drm_file *file_priv)
>  {
>  	struct drm_mode_crtc_page_flip_target *page_flip = data;
>  	struct drm_crtc *crtc;
>  	struct drm_plane *plane;
> -	struct drm_framebuffer *fb, *old_fb;
> +	struct drm_framebuffer *fb;
>  	struct drm_pending_vblank_event *e = NULL;
>  	u32 target_vblank = page_flip->sequence;
>  	struct drm_modeset_acquire_ctx ctx;
> @@ -1145,65 +1241,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (ret)
>  		goto out;
>  
> -	if (plane->state)
> -		old_fb = plane->state->fb;
> +	if (drm_drv_uses_atomic_modeset(dev))
> +		ret = page_flip_atomic(crtc, fb, e, page_flip->flags,
> +				       target_vblank, &ctx);
>  	else
> -		old_fb = plane->fb;
> -
> -	if (old_fb == NULL) {
> -		/* The framebuffer is currently unbound, presumably
> -		 * due to a hotplug event, that userspace has not
> -		 * yet discovered.
> -		 */
> -		ret = -EBUSY;
> -		goto out;
> -	}
> -
> -	if (plane->state) {
> -		const struct drm_plane_state *state = plane->state;
> -
> -		ret = drm_framebuffer_check_src_coords(state->src_x,
> -						       state->src_y,
> -						       state->src_w,
> -						       state->src_h,
> -						       fb);
> -	} else {
> -		ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> -					      &crtc->mode, fb);
> -	}
> -	if (ret)
> -		goto out;
> -
> -	if (old_fb->format != fb->format) {
> -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
> -	plane->old_fb = plane->fb;
> -	if (crtc->funcs->page_flip_target)
> -		ret = crtc->funcs->page_flip_target(crtc, fb, e,
> -						    page_flip->flags,
> -						    target_vblank,
> -						    &ctx);
> -	else
> -		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags,
> -					     &ctx);
> -	if (ret) {
> -		/* Keep the old fb, don't unref it. */
> -		plane->old_fb = NULL;
> -	} else {
> -		if (!plane->state) {
> -			plane->fb = fb;
> -			drm_framebuffer_get(fb);
> -		}
> -	}
> +		ret = page_flip_internal(crtc, fb, e, page_flip->flags,
> +					 target_vblank, &ctx);
>  
>  out:
> -	if (plane->old_fb)
> -		drm_framebuffer_put(plane->old_fb);
> -	plane->old_fb = NULL;
> -
>  	if (ret == -EDEADLK) {
>  		ret = drm_modeset_backoff(&ctx);
>  		if (!ret)
> -- 
> 2.23.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()

[Ville Syrjälä]

On Tue, Nov 19, 2019 at 11:14:43AM +0100, Daniel Vetter wrote:
> On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Yank out the code for the plane->fb/old_fb/crtc handling from
> > the page flip path into page_flip_internal(), and provide a
> > simpler variant for atomic drivers.
> > 
> > We'll also move the fb vs. src viewport checks into the new
> > functions as they are slightly different between the two paths.
> > If the atomic .page_flip() implementations are guaranteed
> > to call drm_atomic_plane_check() we could even drop the check
> > entirely from page_flip_atomic(). For now toss in a FIXME.
> > 
> > v2: Bit more polish in page_flip_internal()
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
> >  1 file changed, 102 insertions(+), 57 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index 38878da5b704..6052475a20a5 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
> >  	return drm_mode_cursor_common(dev, req, file_priv);
> >  }
> >  
> > +static int page_flip_check_fbs(const struct drm_framebuffer *fb,
> > +			       const struct drm_framebuffer *old_fb)
> > +{
> > +	/* The framebuffer is currently unbound, presumably
> > +	 * due to a hotplug event, that userspace has not
> > +	 * yet discovered.
> > +	 */
> > +	if (!old_fb)
> > +		return -EBUSY;
> > +
> > +	if (old_fb->format != fb->format) {
> > +		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int page_flip_internal(struct drm_crtc *crtc,
> > +			      struct drm_framebuffer *fb,
> > +			      struct drm_pending_vblank_event *e,
> > +			      u32 flags,
> > +			      u32 target_vblank,
> > +			      struct drm_modeset_acquire_ctx *ctx)
> > +{
> > +	struct drm_plane *plane = crtc->primary;
> > +	int ret;
> > +
> > +	WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
> > +
> > +	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > +				      &crtc->mode, fb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = page_flip_check_fbs(fb, plane->fb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	plane->old_fb = plane->fb;
> > +	if (crtc->funcs->page_flip_target)
> > +		ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > +						    target_vblank, ctx);
> > +	else
> > +		ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > +	if (ret) {
> > +		plane->old_fb = NULL;
> > +		return ret;
> > +	}
> > +
> > +	plane->fb = fb;
> > +	drm_framebuffer_get(plane->fb);
> > +
> > +	drm_framebuffer_put(plane->old_fb);
> > +	plane->old_fb = NULL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int page_flip_atomic(struct drm_crtc *crtc,
> > +			    struct drm_framebuffer *fb,
> > +			    struct drm_pending_vblank_event *e,
> > +			    u32 flags,
> > +			    u32 target_vblank,
> > +			    struct drm_modeset_acquire_ctx *ctx)
> > +{
> > +	struct drm_plane *plane = crtc->primary;
> > +	struct drm_plane_state *plane_state = plane->state;
> > +	int ret;
> > +
> > +	WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
> > +
> > +	/*
> > +	 * FIXME: Can we assume all drivers end up calling
> > +	 * drm_atomic_plane_check() in their page flip paths?
> > +	 * If so we could remove this.
> > +	 */
> 
> This is definitely wrong, core has to check these. Currently no driver
> checks this at all. I think you're thinking of
> drm_atomic_helper_check_plane_state().

No, I'm thinking of drm_atomic_plane_check(). That one already does the
"does the src rect fit into the fb" check. It gets called from
drm_atomic_commit()->drm_atomic_check_only() but I wasn't sure if some
drivers might take some short circuit path past that for page_flip().

> 
> That aside I'm kinda not getting the point of the shuffling/cleanup in the
> first 4 patches here, so will skip them.

The point was to get that legacy plane->fb/old_fb stuff totally
out from the atomic codepath. Right now it's polluting the
supposedly higher level code shared by both atomic and legacy
drivers.

> -Daniel
> 
> > +	ret = drm_framebuffer_check_src_coords(plane_state->src_x,
> > +					       plane_state->src_y,
> > +					       plane_state->src_w,
> > +					       plane_state->src_h,
> > +					       fb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = page_flip_check_fbs(fb, plane_state->fb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (crtc->funcs->page_flip_target)
> > +		return crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > +						     target_vblank, ctx);
> > +	else
> > +		return crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > +}
> > +
> >  int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  			     void *data, struct drm_file *file_priv)
> >  {
> >  	struct drm_mode_crtc_page_flip_target *page_flip = data;
> >  	struct drm_crtc *crtc;
> >  	struct drm_plane *plane;
> > -	struct drm_framebuffer *fb, *old_fb;
> > +	struct drm_framebuffer *fb;
> >  	struct drm_pending_vblank_event *e = NULL;
> >  	u32 target_vblank = page_flip->sequence;
> >  	struct drm_modeset_acquire_ctx ctx;
> > @@ -1145,65 +1241,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  	if (ret)
> >  		goto out;
> >  
> > -	if (plane->state)
> > -		old_fb = plane->state->fb;
> > +	if (drm_drv_uses_atomic_modeset(dev))
> > +		ret = page_flip_atomic(crtc, fb, e, page_flip->flags,
> > +				       target_vblank, &ctx);
> >  	else
> > -		old_fb = plane->fb;
> > -
> > -	if (old_fb == NULL) {
> > -		/* The framebuffer is currently unbound, presumably
> > -		 * due to a hotplug event, that userspace has not
> > -		 * yet discovered.
> > -		 */
> > -		ret = -EBUSY;
> > -		goto out;
> > -	}
> > -
> > -	if (plane->state) {
> > -		const struct drm_plane_state *state = plane->state;
> > -
> > -		ret = drm_framebuffer_check_src_coords(state->src_x,
> > -						       state->src_y,
> > -						       state->src_w,
> > -						       state->src_h,
> > -						       fb);
> > -	} else {
> > -		ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > -					      &crtc->mode, fb);
> > -	}
> > -	if (ret)
> > -		goto out;
> > -
> > -	if (old_fb->format != fb->format) {
> > -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > -		ret = -EINVAL;
> > -		goto out;
> > -	}
> > -
> > -	plane->old_fb = plane->fb;
> > -	if (crtc->funcs->page_flip_target)
> > -		ret = crtc->funcs->page_flip_target(crtc, fb, e,
> > -						    page_flip->flags,
> > -						    target_vblank,
> > -						    &ctx);
> > -	else
> > -		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags,
> > -					     &ctx);
> > -	if (ret) {
> > -		/* Keep the old fb, don't unref it. */
> > -		plane->old_fb = NULL;
> > -	} else {
> > -		if (!plane->state) {
> > -			plane->fb = fb;
> > -			drm_framebuffer_get(fb);
> > -		}
> > -	}
> > +		ret = page_flip_internal(crtc, fb, e, page_flip->flags,
> > +					 target_vblank, &ctx);
> >  
> >  out:
> > -	if (plane->old_fb)
> > -		drm_framebuffer_put(plane->old_fb);
> > -	plane->old_fb = NULL;
> > -
> >  	if (ret == -EDEADLK) {
> >  		ret = drm_modeset_backoff(&ctx);
> >  		if (!ret)
> > -- 
> > 2.23.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()

[Ville Syrjälä]

On Tue, Nov 19, 2019 at 11:14:43AM +0100, Daniel Vetter wrote:
> On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Yank out the code for the plane->fb/old_fb/crtc handling from
> > the page flip path into page_flip_internal(), and provide a
> > simpler variant for atomic drivers.
> > 
> > We'll also move the fb vs. src viewport checks into the new
> > functions as they are slightly different between the two paths.
> > If the atomic .page_flip() implementations are guaranteed
> > to call drm_atomic_plane_check() we could even drop the check
> > entirely from page_flip_atomic(). For now toss in a FIXME.
> > 
> > v2: Bit more polish in page_flip_internal()
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
> >  1 file changed, 102 insertions(+), 57 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index 38878da5b704..6052475a20a5 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
> >  	return drm_mode_cursor_common(dev, req, file_priv);
> >  }
> >  
> > +static int page_flip_check_fbs(const struct drm_framebuffer *fb,
> > +			       const struct drm_framebuffer *old_fb)
> > +{
> > +	/* The framebuffer is currently unbound, presumably
> > +	 * due to a hotplug event, that userspace has not
> > +	 * yet discovered.
> > +	 */
> > +	if (!old_fb)
> > +		return -EBUSY;
> > +
> > +	if (old_fb->format != fb->format) {
> > +		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int page_flip_internal(struct drm_crtc *crtc,
> > +			      struct drm_framebuffer *fb,
> > +			      struct drm_pending_vblank_event *e,
> > +			      u32 flags,
> > +			      u32 target_vblank,
> > +			      struct drm_modeset_acquire_ctx *ctx)
> > +{
> > +	struct drm_plane *plane = crtc->primary;
> > +	int ret;
> > +
> > +	WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
> > +
> > +	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > +				      &crtc->mode, fb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = page_flip_check_fbs(fb, plane->fb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	plane->old_fb = plane->fb;
> > +	if (crtc->funcs->page_flip_target)
> > +		ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > +						    target_vblank, ctx);
> > +	else
> > +		ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > +	if (ret) {
> > +		plane->old_fb = NULL;
> > +		return ret;
> > +	}
> > +
> > +	plane->fb = fb;
> > +	drm_framebuffer_get(plane->fb);
> > +
> > +	drm_framebuffer_put(plane->old_fb);
> > +	plane->old_fb = NULL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int page_flip_atomic(struct drm_crtc *crtc,
> > +			    struct drm_framebuffer *fb,
> > +			    struct drm_pending_vblank_event *e,
> > +			    u32 flags,
> > +			    u32 target_vblank,
> > +			    struct drm_modeset_acquire_ctx *ctx)
> > +{
> > +	struct drm_plane *plane = crtc->primary;
> > +	struct drm_plane_state *plane_state = plane->state;
> > +	int ret;
> > +
> > +	WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
> > +
> > +	/*
> > +	 * FIXME: Can we assume all drivers end up calling
> > +	 * drm_atomic_plane_check() in their page flip paths?
> > +	 * If so we could remove this.
> > +	 */
> 
> This is definitely wrong, core has to check these. Currently no driver
> checks this at all. I think you're thinking of
> drm_atomic_helper_check_plane_state().

No, I'm thinking of drm_atomic_plane_check(). That one already does the
"does the src rect fit into the fb" check. It gets called from
drm_atomic_commit()->drm_atomic_check_only() but I wasn't sure if some
drivers might take some short circuit path past that for page_flip().

> 
> That aside I'm kinda not getting the point of the shuffling/cleanup in the
> first 4 patches here, so will skip them.

The point was to get that legacy plane->fb/old_fb stuff totally
out from the atomic codepath. Right now it's polluting the
supposedly higher level code shared by both atomic and legacy
drivers.

> -Daniel
> 
> > +	ret = drm_framebuffer_check_src_coords(plane_state->src_x,
> > +					       plane_state->src_y,
> > +					       plane_state->src_w,
> > +					       plane_state->src_h,
> > +					       fb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = page_flip_check_fbs(fb, plane_state->fb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (crtc->funcs->page_flip_target)
> > +		return crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > +						     target_vblank, ctx);
> > +	else
> > +		return crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > +}
> > +
> >  int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  			     void *data, struct drm_file *file_priv)
> >  {
> >  	struct drm_mode_crtc_page_flip_target *page_flip = data;
> >  	struct drm_crtc *crtc;
> >  	struct drm_plane *plane;
> > -	struct drm_framebuffer *fb, *old_fb;
> > +	struct drm_framebuffer *fb;
> >  	struct drm_pending_vblank_event *e = NULL;
> >  	u32 target_vblank = page_flip->sequence;
> >  	struct drm_modeset_acquire_ctx ctx;
> > @@ -1145,65 +1241,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  	if (ret)
> >  		goto out;
> >  
> > -	if (plane->state)
> > -		old_fb = plane->state->fb;
> > +	if (drm_drv_uses_atomic_modeset(dev))
> > +		ret = page_flip_atomic(crtc, fb, e, page_flip->flags,
> > +				       target_vblank, &ctx);
> >  	else
> > -		old_fb = plane->fb;
> > -
> > -	if (old_fb == NULL) {
> > -		/* The framebuffer is currently unbound, presumably
> > -		 * due to a hotplug event, that userspace has not
> > -		 * yet discovered.
> > -		 */
> > -		ret = -EBUSY;
> > -		goto out;
> > -	}
> > -
> > -	if (plane->state) {
> > -		const struct drm_plane_state *state = plane->state;
> > -
> > -		ret = drm_framebuffer_check_src_coords(state->src_x,
> > -						       state->src_y,
> > -						       state->src_w,
> > -						       state->src_h,
> > -						       fb);
> > -	} else {
> > -		ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > -					      &crtc->mode, fb);
> > -	}
> > -	if (ret)
> > -		goto out;
> > -
> > -	if (old_fb->format != fb->format) {
> > -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > -		ret = -EINVAL;
> > -		goto out;
> > -	}
> > -
> > -	plane->old_fb = plane->fb;
> > -	if (crtc->funcs->page_flip_target)
> > -		ret = crtc->funcs->page_flip_target(crtc, fb, e,
> > -						    page_flip->flags,
> > -						    target_vblank,
> > -						    &ctx);
> > -	else
> > -		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags,
> > -					     &ctx);
> > -	if (ret) {
> > -		/* Keep the old fb, don't unref it. */
> > -		plane->old_fb = NULL;
> > -	} else {
> > -		if (!plane->state) {
> > -			plane->fb = fb;
> > -			drm_framebuffer_get(fb);
> > -		}
> > -	}
> > +		ret = page_flip_internal(crtc, fb, e, page_flip->flags,
> > +					 target_vblank, &ctx);
> >  
> >  out:
> > -	if (plane->old_fb)
> > -		drm_framebuffer_put(plane->old_fb);
> > -	plane->old_fb = NULL;
> > -
> >  	if (ret == -EDEADLK) {
> >  		ret = drm_modeset_backoff(&ctx);
> >  		if (!ret)
> > -- 
> > 2.23.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()

[Daniel Vetter]

On Fri, Nov 22, 2019 at 08:35:13PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 19, 2019 at 11:14:43AM +0100, Daniel Vetter wrote:
> > On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Yank out the code for the plane->fb/old_fb/crtc handling from
> > > the page flip path into page_flip_internal(), and provide a
> > > simpler variant for atomic drivers.
> > > 
> > > We'll also move the fb vs. src viewport checks into the new
> > > functions as they are slightly different between the two paths.
> > > If the atomic .page_flip() implementations are guaranteed
> > > to call drm_atomic_plane_check() we could even drop the check
> > > entirely from page_flip_atomic(). For now toss in a FIXME.
> > > 
> > > v2: Bit more polish in page_flip_internal()
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
> > >  1 file changed, 102 insertions(+), 57 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index 38878da5b704..6052475a20a5 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
> > >  	return drm_mode_cursor_common(dev, req, file_priv);
> > >  }
> > >  
> > > +static int page_flip_check_fbs(const struct drm_framebuffer *fb,
> > > +			       const struct drm_framebuffer *old_fb)
> > > +{
> > > +	/* The framebuffer is currently unbound, presumably
> > > +	 * due to a hotplug event, that userspace has not
> > > +	 * yet discovered.
> > > +	 */
> > > +	if (!old_fb)
> > > +		return -EBUSY;
> > > +
> > > +	if (old_fb->format != fb->format) {
> > > +		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int page_flip_internal(struct drm_crtc *crtc,
> > > +			      struct drm_framebuffer *fb,
> > > +			      struct drm_pending_vblank_event *e,
> > > +			      u32 flags,
> > > +			      u32 target_vblank,
> > > +			      struct drm_modeset_acquire_ctx *ctx)
> > > +{
> > > +	struct drm_plane *plane = crtc->primary;
> > > +	int ret;
> > > +
> > > +	WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
> > > +
> > > +	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > > +				      &crtc->mode, fb);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = page_flip_check_fbs(fb, plane->fb);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	plane->old_fb = plane->fb;
> > > +	if (crtc->funcs->page_flip_target)
> > > +		ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > > +						    target_vblank, ctx);
> > > +	else
> > > +		ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > > +	if (ret) {
> > > +		plane->old_fb = NULL;
> > > +		return ret;
> > > +	}
> > > +
> > > +	plane->fb = fb;
> > > +	drm_framebuffer_get(plane->fb);
> > > +
> > > +	drm_framebuffer_put(plane->old_fb);
> > > +	plane->old_fb = NULL;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int page_flip_atomic(struct drm_crtc *crtc,
> > > +			    struct drm_framebuffer *fb,
> > > +			    struct drm_pending_vblank_event *e,
> > > +			    u32 flags,
> > > +			    u32 target_vblank,
> > > +			    struct drm_modeset_acquire_ctx *ctx)
> > > +{
> > > +	struct drm_plane *plane = crtc->primary;
> > > +	struct drm_plane_state *plane_state = plane->state;
> > > +	int ret;
> > > +
> > > +	WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
> > > +
> > > +	/*
> > > +	 * FIXME: Can we assume all drivers end up calling
> > > +	 * drm_atomic_plane_check() in their page flip paths?
> > > +	 * If so we could remove this.
> > > +	 */
> > 
> > This is definitely wrong, core has to check these. Currently no driver
> > checks this at all. I think you're thinking of
> > drm_atomic_helper_check_plane_state().
> 
> No, I'm thinking of drm_atomic_plane_check(). That one already does the
> "does the src rect fit into the fb" check. It gets called from
> drm_atomic_commit()->drm_atomic_check_only() but I wasn't sure if some
> drivers might take some short circuit path past that for page_flip().

tbh if you're worried about this, I'm tempted to rip out the option for
atomic drivers to overwrite their page_flip implementation, and force the
default function onto everyone (drm_atomic_helper_page_flip_target and
friends would need to be moved to drm_plane.c for that ofc).

We seem to have 0 need for hacks for old userspace in this area, which is
great. So the option to overwrite could only lead to trouble.

> > That aside I'm kinda not getting the point of the shuffling/cleanup in the
> > first 4 patches here, so will skip them.
> 
> The point was to get that legacy plane->fb/old_fb stuff totally
> out from the atomic codepath. Right now it's polluting the
> supposedly higher level code shared by both atomic and legacy
> drivers.

Hm ... I think if you sell it like that, and maybe even go one further and
inline page_flip for all atomic drivers, I can be sold on this. As-is I
just didn't really get why you're reworking all this.
-Daniel

> 
> > -Daniel
> > 
> > > +	ret = drm_framebuffer_check_src_coords(plane_state->src_x,
> > > +					       plane_state->src_y,
> > > +					       plane_state->src_w,
> > > +					       plane_state->src_h,
> > > +					       fb);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = page_flip_check_fbs(fb, plane_state->fb);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (crtc->funcs->page_flip_target)
> > > +		return crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > > +						     target_vblank, ctx);
> > > +	else
> > > +		return crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > > +}
> > > +
> > >  int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > >  			     void *data, struct drm_file *file_priv)
> > >  {
> > >  	struct drm_mode_crtc_page_flip_target *page_flip = data;
> > >  	struct drm_crtc *crtc;
> > >  	struct drm_plane *plane;
> > > -	struct drm_framebuffer *fb, *old_fb;
> > > +	struct drm_framebuffer *fb;
> > >  	struct drm_pending_vblank_event *e = NULL;
> > >  	u32 target_vblank = page_flip->sequence;
> > >  	struct drm_modeset_acquire_ctx ctx;
> > > @@ -1145,65 +1241,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > >  	if (ret)
> > >  		goto out;
> > >  
> > > -	if (plane->state)
> > > -		old_fb = plane->state->fb;
> > > +	if (drm_drv_uses_atomic_modeset(dev))
> > > +		ret = page_flip_atomic(crtc, fb, e, page_flip->flags,
> > > +				       target_vblank, &ctx);
> > >  	else
> > > -		old_fb = plane->fb;
> > > -
> > > -	if (old_fb == NULL) {
> > > -		/* The framebuffer is currently unbound, presumably
> > > -		 * due to a hotplug event, that userspace has not
> > > -		 * yet discovered.
> > > -		 */
> > > -		ret = -EBUSY;
> > > -		goto out;
> > > -	}
> > > -
> > > -	if (plane->state) {
> > > -		const struct drm_plane_state *state = plane->state;
> > > -
> > > -		ret = drm_framebuffer_check_src_coords(state->src_x,
> > > -						       state->src_y,
> > > -						       state->src_w,
> > > -						       state->src_h,
> > > -						       fb);
> > > -	} else {
> > > -		ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > > -					      &crtc->mode, fb);
> > > -	}
> > > -	if (ret)
> > > -		goto out;
> > > -
> > > -	if (old_fb->format != fb->format) {
> > > -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > -		ret = -EINVAL;
> > > -		goto out;
> > > -	}
> > > -
> > > -	plane->old_fb = plane->fb;
> > > -	if (crtc->funcs->page_flip_target)
> > > -		ret = crtc->funcs->page_flip_target(crtc, fb, e,
> > > -						    page_flip->flags,
> > > -						    target_vblank,
> > > -						    &ctx);
> > > -	else
> > > -		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags,
> > > -					     &ctx);
> > > -	if (ret) {
> > > -		/* Keep the old fb, don't unref it. */
> > > -		plane->old_fb = NULL;
> > > -	} else {
> > > -		if (!plane->state) {
> > > -			plane->fb = fb;
> > > -			drm_framebuffer_get(fb);
> > > -		}
> > > -	}
> > > +		ret = page_flip_internal(crtc, fb, e, page_flip->flags,
> > > +					 target_vblank, &ctx);
> > >  
> > >  out:
> > > -	if (plane->old_fb)
> > > -		drm_framebuffer_put(plane->old_fb);
> > > -	plane->old_fb = NULL;
> > > -
> > >  	if (ret == -EDEADLK) {
> > >  		ret = drm_modeset_backoff(&ctx);
> > >  		if (!ret)
> > > -- 
> > > 2.23.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel

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

Re: [Intel-gfx] [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()

[Daniel Vetter]

On Fri, Nov 22, 2019 at 08:35:13PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 19, 2019 at 11:14:43AM +0100, Daniel Vetter wrote:
> > On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Yank out the code for the plane->fb/old_fb/crtc handling from
> > > the page flip path into page_flip_internal(), and provide a
> > > simpler variant for atomic drivers.
> > > 
> > > We'll also move the fb vs. src viewport checks into the new
> > > functions as they are slightly different between the two paths.
> > > If the atomic .page_flip() implementations are guaranteed
> > > to call drm_atomic_plane_check() we could even drop the check
> > > entirely from page_flip_atomic(). For now toss in a FIXME.
> > > 
> > > v2: Bit more polish in page_flip_internal()
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
> > >  1 file changed, 102 insertions(+), 57 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index 38878da5b704..6052475a20a5 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
> > >  	return drm_mode_cursor_common(dev, req, file_priv);
> > >  }
> > >  
> > > +static int page_flip_check_fbs(const struct drm_framebuffer *fb,
> > > +			       const struct drm_framebuffer *old_fb)
> > > +{
> > > +	/* The framebuffer is currently unbound, presumably
> > > +	 * due to a hotplug event, that userspace has not
> > > +	 * yet discovered.
> > > +	 */
> > > +	if (!old_fb)
> > > +		return -EBUSY;
> > > +
> > > +	if (old_fb->format != fb->format) {
> > > +		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int page_flip_internal(struct drm_crtc *crtc,
> > > +			      struct drm_framebuffer *fb,
> > > +			      struct drm_pending_vblank_event *e,
> > > +			      u32 flags,
> > > +			      u32 target_vblank,
> > > +			      struct drm_modeset_acquire_ctx *ctx)
> > > +{
> > > +	struct drm_plane *plane = crtc->primary;
> > > +	int ret;
> > > +
> > > +	WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
> > > +
> > > +	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > > +				      &crtc->mode, fb);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = page_flip_check_fbs(fb, plane->fb);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	plane->old_fb = plane->fb;
> > > +	if (crtc->funcs->page_flip_target)
> > > +		ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > > +						    target_vblank, ctx);
> > > +	else
> > > +		ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > > +	if (ret) {
> > > +		plane->old_fb = NULL;
> > > +		return ret;
> > > +	}
> > > +
> > > +	plane->fb = fb;
> > > +	drm_framebuffer_get(plane->fb);
> > > +
> > > +	drm_framebuffer_put(plane->old_fb);
> > > +	plane->old_fb = NULL;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int page_flip_atomic(struct drm_crtc *crtc,
> > > +			    struct drm_framebuffer *fb,
> > > +			    struct drm_pending_vblank_event *e,
> > > +			    u32 flags,
> > > +			    u32 target_vblank,
> > > +			    struct drm_modeset_acquire_ctx *ctx)
> > > +{
> > > +	struct drm_plane *plane = crtc->primary;
> > > +	struct drm_plane_state *plane_state = plane->state;
> > > +	int ret;
> > > +
> > > +	WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
> > > +
> > > +	/*
> > > +	 * FIXME: Can we assume all drivers end up calling
> > > +	 * drm_atomic_plane_check() in their page flip paths?
> > > +	 * If so we could remove this.
> > > +	 */
> > 
> > This is definitely wrong, core has to check these. Currently no driver
> > checks this at all. I think you're thinking of
> > drm_atomic_helper_check_plane_state().
> 
> No, I'm thinking of drm_atomic_plane_check(). That one already does the
> "does the src rect fit into the fb" check. It gets called from
> drm_atomic_commit()->drm_atomic_check_only() but I wasn't sure if some
> drivers might take some short circuit path past that for page_flip().

tbh if you're worried about this, I'm tempted to rip out the option for
atomic drivers to overwrite their page_flip implementation, and force the
default function onto everyone (drm_atomic_helper_page_flip_target and
friends would need to be moved to drm_plane.c for that ofc).

We seem to have 0 need for hacks for old userspace in this area, which is
great. So the option to overwrite could only lead to trouble.

> > That aside I'm kinda not getting the point of the shuffling/cleanup in the
> > first 4 patches here, so will skip them.
> 
> The point was to get that legacy plane->fb/old_fb stuff totally
> out from the atomic codepath. Right now it's polluting the
> supposedly higher level code shared by both atomic and legacy
> drivers.

Hm ... I think if you sell it like that, and maybe even go one further and
inline page_flip for all atomic drivers, I can be sold on this. As-is I
just didn't really get why you're reworking all this.
-Daniel

> 
> > -Daniel
> > 
> > > +	ret = drm_framebuffer_check_src_coords(plane_state->src_x,
> > > +					       plane_state->src_y,
> > > +					       plane_state->src_w,
> > > +					       plane_state->src_h,
> > > +					       fb);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = page_flip_check_fbs(fb, plane_state->fb);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (crtc->funcs->page_flip_target)
> > > +		return crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > > +						     target_vblank, ctx);
> > > +	else
> > > +		return crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > > +}
> > > +
> > >  int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > >  			     void *data, struct drm_file *file_priv)
> > >  {
> > >  	struct drm_mode_crtc_page_flip_target *page_flip = data;
> > >  	struct drm_crtc *crtc;
> > >  	struct drm_plane *plane;
> > > -	struct drm_framebuffer *fb, *old_fb;
> > > +	struct drm_framebuffer *fb;
> > >  	struct drm_pending_vblank_event *e = NULL;
> > >  	u32 target_vblank = page_flip->sequence;
> > >  	struct drm_modeset_acquire_ctx ctx;
> > > @@ -1145,65 +1241,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > >  	if (ret)
> > >  		goto out;
> > >  
> > > -	if (plane->state)
> > > -		old_fb = plane->state->fb;
> > > +	if (drm_drv_uses_atomic_modeset(dev))
> > > +		ret = page_flip_atomic(crtc, fb, e, page_flip->flags,
> > > +				       target_vblank, &ctx);
> > >  	else
> > > -		old_fb = plane->fb;
> > > -
> > > -	if (old_fb == NULL) {
> > > -		/* The framebuffer is currently unbound, presumably
> > > -		 * due to a hotplug event, that userspace has not
> > > -		 * yet discovered.
> > > -		 */
> > > -		ret = -EBUSY;
> > > -		goto out;
> > > -	}
> > > -
> > > -	if (plane->state) {
> > > -		const struct drm_plane_state *state = plane->state;
> > > -
> > > -		ret = drm_framebuffer_check_src_coords(state->src_x,
> > > -						       state->src_y,
> > > -						       state->src_w,
> > > -						       state->src_h,
> > > -						       fb);
> > > -	} else {
> > > -		ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > > -					      &crtc->mode, fb);
> > > -	}
> > > -	if (ret)
> > > -		goto out;
> > > -
> > > -	if (old_fb->format != fb->format) {
> > > -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > -		ret = -EINVAL;
> > > -		goto out;
> > > -	}
> > > -
> > > -	plane->old_fb = plane->fb;
> > > -	if (crtc->funcs->page_flip_target)
> > > -		ret = crtc->funcs->page_flip_target(crtc, fb, e,
> > > -						    page_flip->flags,
> > > -						    target_vblank,
> > > -						    &ctx);
> > > -	else
> > > -		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags,
> > > -					     &ctx);
> > > -	if (ret) {
> > > -		/* Keep the old fb, don't unref it. */
> > > -		plane->old_fb = NULL;
> > > -	} else {
> > > -		if (!plane->state) {
> > > -			plane->fb = fb;
> > > -			drm_framebuffer_get(fb);
> > > -		}
> > > -	}
> > > +		ret = page_flip_internal(crtc, fb, e, page_flip->flags,
> > > +					 target_vblank, &ctx);
> > >  
> > >  out:
> > > -	if (plane->old_fb)
> > > -		drm_framebuffer_put(plane->old_fb);
> > > -	plane->old_fb = NULL;
> > > -
> > >  	if (ret == -EDEADLK) {
> > >  		ret = drm_modeset_backoff(&ctx);
> > >  		if (!ret)
> > > -- 
> > > 2.23.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel

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

Re: [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()

[Ville Syrjälä]

On Mon, Nov 25, 2019 at 10:02:38AM +0100, Daniel Vetter wrote:
> On Fri, Nov 22, 2019 at 08:35:13PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 19, 2019 at 11:14:43AM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Yank out the code for the plane->fb/old_fb/crtc handling from
> > > > the page flip path into page_flip_internal(), and provide a
> > > > simpler variant for atomic drivers.
> > > > 
> > > > We'll also move the fb vs. src viewport checks into the new
> > > > functions as they are slightly different between the two paths.
> > > > If the atomic .page_flip() implementations are guaranteed
> > > > to call drm_atomic_plane_check() we could even drop the check
> > > > entirely from page_flip_atomic(). For now toss in a FIXME.
> > > > 
> > > > v2: Bit more polish in page_flip_internal()
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
> > > >  1 file changed, 102 insertions(+), 57 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > index 38878da5b704..6052475a20a5 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
> > > >  	return drm_mode_cursor_common(dev, req, file_priv);
> > > >  }
> > > >  
> > > > +static int page_flip_check_fbs(const struct drm_framebuffer *fb,
> > > > +			       const struct drm_framebuffer *old_fb)
> > > > +{
> > > > +	/* The framebuffer is currently unbound, presumably
> > > > +	 * due to a hotplug event, that userspace has not
> > > > +	 * yet discovered.
> > > > +	 */
> > > > +	if (!old_fb)
> > > > +		return -EBUSY;
> > > > +
> > > > +	if (old_fb->format != fb->format) {
> > > > +		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int page_flip_internal(struct drm_crtc *crtc,
> > > > +			      struct drm_framebuffer *fb,
> > > > +			      struct drm_pending_vblank_event *e,
> > > > +			      u32 flags,
> > > > +			      u32 target_vblank,
> > > > +			      struct drm_modeset_acquire_ctx *ctx)
> > > > +{
> > > > +	struct drm_plane *plane = crtc->primary;
> > > > +	int ret;
> > > > +
> > > > +	WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
> > > > +
> > > > +	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > > > +				      &crtc->mode, fb);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = page_flip_check_fbs(fb, plane->fb);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	plane->old_fb = plane->fb;
> > > > +	if (crtc->funcs->page_flip_target)
> > > > +		ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > > > +						    target_vblank, ctx);
> > > > +	else
> > > > +		ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > > > +	if (ret) {
> > > > +		plane->old_fb = NULL;
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	plane->fb = fb;
> > > > +	drm_framebuffer_get(plane->fb);
> > > > +
> > > > +	drm_framebuffer_put(plane->old_fb);
> > > > +	plane->old_fb = NULL;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int page_flip_atomic(struct drm_crtc *crtc,
> > > > +			    struct drm_framebuffer *fb,
> > > > +			    struct drm_pending_vblank_event *e,
> > > > +			    u32 flags,
> > > > +			    u32 target_vblank,
> > > > +			    struct drm_modeset_acquire_ctx *ctx)
> > > > +{
> > > > +	struct drm_plane *plane = crtc->primary;
> > > > +	struct drm_plane_state *plane_state = plane->state;
> > > > +	int ret;
> > > > +
> > > > +	WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
> > > > +
> > > > +	/*
> > > > +	 * FIXME: Can we assume all drivers end up calling
> > > > +	 * drm_atomic_plane_check() in their page flip paths?
> > > > +	 * If so we could remove this.
> > > > +	 */
> > > 
> > > This is definitely wrong, core has to check these. Currently no driver
> > > checks this at all. I think you're thinking of
> > > drm_atomic_helper_check_plane_state().
> > 
> > No, I'm thinking of drm_atomic_plane_check(). That one already does the
> > "does the src rect fit into the fb" check. It gets called from
> > drm_atomic_commit()->drm_atomic_check_only() but I wasn't sure if some
> > drivers might take some short circuit path past that for page_flip().
> 
> tbh if you're worried about this, I'm tempted to rip out the option for
> atomic drivers to overwrite their page_flip implementation, and force the
> default function onto everyone (drm_atomic_helper_page_flip_target and
> friends would need to be moved to drm_plane.c for that ofc).
> 
> We seem to have 0 need for hacks for old userspace in this area, which is
> great. So the option to overwrite could only lead to trouble.
> 
> > > That aside I'm kinda not getting the point of the shuffling/cleanup in the
> > > first 4 patches here, so will skip them.
> > 
> > The point was to get that legacy plane->fb/old_fb stuff totally
> > out from the atomic codepath. Right now it's polluting the
> > supposedly higher level code shared by both atomic and legacy
> > drivers.
> 
> Hm ... I think if you sell it like that, and maybe even go one further and
> inline page_flip for all atomic drivers, I can be sold on this. As-is I
> just didn't really get why you're reworking all this.

Hmm. Inlining doesn't look like 100% trivial code shufling due to
.page_flip_target. We'd need some other mechanism for the driver to
indicate support for that.

OTOH I'm not sure we even support that stuff on any atomic driver.
We have the helper but no one actually uses it. And as ususal I'm
totally unusre what amdgpu is doing since I keep forgetting which
parts of it are atomic and which are not.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()

[Ville Syrjälä]

On Mon, Nov 25, 2019 at 10:02:38AM +0100, Daniel Vetter wrote:
> On Fri, Nov 22, 2019 at 08:35:13PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 19, 2019 at 11:14:43AM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Yank out the code for the plane->fb/old_fb/crtc handling from
> > > > the page flip path into page_flip_internal(), and provide a
> > > > simpler variant for atomic drivers.
> > > > 
> > > > We'll also move the fb vs. src viewport checks into the new
> > > > functions as they are slightly different between the two paths.
> > > > If the atomic .page_flip() implementations are guaranteed
> > > > to call drm_atomic_plane_check() we could even drop the check
> > > > entirely from page_flip_atomic(). For now toss in a FIXME.
> > > > 
> > > > v2: Bit more polish in page_flip_internal()
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
> > > >  1 file changed, 102 insertions(+), 57 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > index 38878da5b704..6052475a20a5 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
> > > >  	return drm_mode_cursor_common(dev, req, file_priv);
> > > >  }
> > > >  
> > > > +static int page_flip_check_fbs(const struct drm_framebuffer *fb,
> > > > +			       const struct drm_framebuffer *old_fb)
> > > > +{
> > > > +	/* The framebuffer is currently unbound, presumably
> > > > +	 * due to a hotplug event, that userspace has not
> > > > +	 * yet discovered.
> > > > +	 */
> > > > +	if (!old_fb)
> > > > +		return -EBUSY;
> > > > +
> > > > +	if (old_fb->format != fb->format) {
> > > > +		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int page_flip_internal(struct drm_crtc *crtc,
> > > > +			      struct drm_framebuffer *fb,
> > > > +			      struct drm_pending_vblank_event *e,
> > > > +			      u32 flags,
> > > > +			      u32 target_vblank,
> > > > +			      struct drm_modeset_acquire_ctx *ctx)
> > > > +{
> > > > +	struct drm_plane *plane = crtc->primary;
> > > > +	int ret;
> > > > +
> > > > +	WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
> > > > +
> > > > +	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > > > +				      &crtc->mode, fb);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = page_flip_check_fbs(fb, plane->fb);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	plane->old_fb = plane->fb;
> > > > +	if (crtc->funcs->page_flip_target)
> > > > +		ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > > > +						    target_vblank, ctx);
> > > > +	else
> > > > +		ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > > > +	if (ret) {
> > > > +		plane->old_fb = NULL;
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	plane->fb = fb;
> > > > +	drm_framebuffer_get(plane->fb);
> > > > +
> > > > +	drm_framebuffer_put(plane->old_fb);
> > > > +	plane->old_fb = NULL;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int page_flip_atomic(struct drm_crtc *crtc,
> > > > +			    struct drm_framebuffer *fb,
> > > > +			    struct drm_pending_vblank_event *e,
> > > > +			    u32 flags,
> > > > +			    u32 target_vblank,
> > > > +			    struct drm_modeset_acquire_ctx *ctx)
> > > > +{
> > > > +	struct drm_plane *plane = crtc->primary;
> > > > +	struct drm_plane_state *plane_state = plane->state;
> > > > +	int ret;
> > > > +
> > > > +	WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
> > > > +
> > > > +	/*
> > > > +	 * FIXME: Can we assume all drivers end up calling
> > > > +	 * drm_atomic_plane_check() in their page flip paths?
> > > > +	 * If so we could remove this.
> > > > +	 */
> > > 
> > > This is definitely wrong, core has to check these. Currently no driver
> > > checks this at all. I think you're thinking of
> > > drm_atomic_helper_check_plane_state().
> > 
> > No, I'm thinking of drm_atomic_plane_check(). That one already does the
> > "does the src rect fit into the fb" check. It gets called from
> > drm_atomic_commit()->drm_atomic_check_only() but I wasn't sure if some
> > drivers might take some short circuit path past that for page_flip().
> 
> tbh if you're worried about this, I'm tempted to rip out the option for
> atomic drivers to overwrite their page_flip implementation, and force the
> default function onto everyone (drm_atomic_helper_page_flip_target and
> friends would need to be moved to drm_plane.c for that ofc).
> 
> We seem to have 0 need for hacks for old userspace in this area, which is
> great. So the option to overwrite could only lead to trouble.
> 
> > > That aside I'm kinda not getting the point of the shuffling/cleanup in the
> > > first 4 patches here, so will skip them.
> > 
> > The point was to get that legacy plane->fb/old_fb stuff totally
> > out from the atomic codepath. Right now it's polluting the
> > supposedly higher level code shared by both atomic and legacy
> > drivers.
> 
> Hm ... I think if you sell it like that, and maybe even go one further and
> inline page_flip for all atomic drivers, I can be sold on this. As-is I
> just didn't really get why you're reworking all this.

Hmm. Inlining doesn't look like 100% trivial code shufling due to
.page_flip_target. We'd need some other mechanism for the driver to
indicate support for that.

OTOH I'm not sure we even support that stuff on any atomic driver.
We have the helper but no one actually uses it. And as ususal I'm
totally unusre what amdgpu is doing since I keep forgetting which
parts of it are atomic and which are not.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()

[Daniel Vetter]

On Mon, Nov 25, 2019 at 4:05 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Mon, Nov 25, 2019 at 10:02:38AM +0100, Daniel Vetter wrote:
> > On Fri, Nov 22, 2019 at 08:35:13PM +0200, Ville Syrjälä wrote:
> > > On Tue, Nov 19, 2019 at 11:14:43AM +0100, Daniel Vetter wrote:
> > > > On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > Yank out the code for the plane->fb/old_fb/crtc handling from
> > > > > the page flip path into page_flip_internal(), and provide a
> > > > > simpler variant for atomic drivers.
> > > > >
> > > > > We'll also move the fb vs. src viewport checks into the new
> > > > > functions as they are slightly different between the two paths.
> > > > > If the atomic .page_flip() implementations are guaranteed
> > > > > to call drm_atomic_plane_check() we could even drop the check
> > > > > entirely from page_flip_atomic(). For now toss in a FIXME.
> > > > >
> > > > > v2: Bit more polish in page_flip_internal()
> > > > >
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
> > > > >  1 file changed, 102 insertions(+), 57 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > > index 38878da5b704..6052475a20a5 100644
> > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
> > > > >         return drm_mode_cursor_common(dev, req, file_priv);
> > > > >  }
> > > > >
> > > > > +static int page_flip_check_fbs(const struct drm_framebuffer *fb,
> > > > > +                              const struct drm_framebuffer *old_fb)
> > > > > +{
> > > > > +       /* The framebuffer is currently unbound, presumably
> > > > > +        * due to a hotplug event, that userspace has not
> > > > > +        * yet discovered.
> > > > > +        */
> > > > > +       if (!old_fb)
> > > > > +               return -EBUSY;
> > > > > +
> > > > > +       if (old_fb->format != fb->format) {
> > > > > +               DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > > > +               return -EINVAL;
> > > > > +       }
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static int page_flip_internal(struct drm_crtc *crtc,
> > > > > +                             struct drm_framebuffer *fb,
> > > > > +                             struct drm_pending_vblank_event *e,
> > > > > +                             u32 flags,
> > > > > +                             u32 target_vblank,
> > > > > +                             struct drm_modeset_acquire_ctx *ctx)
> > > > > +{
> > > > > +       struct drm_plane *plane = crtc->primary;
> > > > > +       int ret;
> > > > > +
> > > > > +       WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
> > > > > +
> > > > > +       ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > > > > +                                     &crtc->mode, fb);
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +
> > > > > +       ret = page_flip_check_fbs(fb, plane->fb);
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +
> > > > > +       plane->old_fb = plane->fb;
> > > > > +       if (crtc->funcs->page_flip_target)
> > > > > +               ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > > > > +                                                   target_vblank, ctx);
> > > > > +       else
> > > > > +               ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > > > > +       if (ret) {
> > > > > +               plane->old_fb = NULL;
> > > > > +               return ret;
> > > > > +       }
> > > > > +
> > > > > +       plane->fb = fb;
> > > > > +       drm_framebuffer_get(plane->fb);
> > > > > +
> > > > > +       drm_framebuffer_put(plane->old_fb);
> > > > > +       plane->old_fb = NULL;
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static int page_flip_atomic(struct drm_crtc *crtc,
> > > > > +                           struct drm_framebuffer *fb,
> > > > > +                           struct drm_pending_vblank_event *e,
> > > > > +                           u32 flags,
> > > > > +                           u32 target_vblank,
> > > > > +                           struct drm_modeset_acquire_ctx *ctx)
> > > > > +{
> > > > > +       struct drm_plane *plane = crtc->primary;
> > > > > +       struct drm_plane_state *plane_state = plane->state;
> > > > > +       int ret;
> > > > > +
> > > > > +       WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
> > > > > +
> > > > > +       /*
> > > > > +        * FIXME: Can we assume all drivers end up calling
> > > > > +        * drm_atomic_plane_check() in their page flip paths?
> > > > > +        * If so we could remove this.
> > > > > +        */
> > > >
> > > > This is definitely wrong, core has to check these. Currently no driver
> > > > checks this at all. I think you're thinking of
> > > > drm_atomic_helper_check_plane_state().
> > >
> > > No, I'm thinking of drm_atomic_plane_check(). That one already does the
> > > "does the src rect fit into the fb" check. It gets called from
> > > drm_atomic_commit()->drm_atomic_check_only() but I wasn't sure if some
> > > drivers might take some short circuit path past that for page_flip().
> >
> > tbh if you're worried about this, I'm tempted to rip out the option for
> > atomic drivers to overwrite their page_flip implementation, and force the
> > default function onto everyone (drm_atomic_helper_page_flip_target and
> > friends would need to be moved to drm_plane.c for that ofc).
> >
> > We seem to have 0 need for hacks for old userspace in this area, which is
> > great. So the option to overwrite could only lead to trouble.
> >
> > > > That aside I'm kinda not getting the point of the shuffling/cleanup in the
> > > > first 4 patches here, so will skip them.
> > >
> > > The point was to get that legacy plane->fb/old_fb stuff totally
> > > out from the atomic codepath. Right now it's polluting the
> > > supposedly higher level code shared by both atomic and legacy
> > > drivers.
> >
> > Hm ... I think if you sell it like that, and maybe even go one further and
> > inline page_flip for all atomic drivers, I can be sold on this. As-is I
> > just didn't really get why you're reworking all this.
>
> Hmm. Inlining doesn't look like 100% trivial code shufling due to
> .page_flip_target. We'd need some other mechanism for the driver to
> indicate support for that.

Ah right, I forgot about that one.

> OTOH I'm not sure we even support that stuff on any atomic driver.
> We have the helper but no one actually uses it. And as ususal I'm
> totally unusre what amdgpu is doing since I keep forgetting which
> parts of it are atomic and which are not.

amdgpu/DC does. I guess the simplest soulution would be to add
mode_config->has_flip_target. It's all wired through atomic states,
just not yet exposed as a property. Until that's done we need a
separate flag.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()

[Daniel Vetter]

On Mon, Nov 25, 2019 at 4:05 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Mon, Nov 25, 2019 at 10:02:38AM +0100, Daniel Vetter wrote:
> > On Fri, Nov 22, 2019 at 08:35:13PM +0200, Ville Syrjälä wrote:
> > > On Tue, Nov 19, 2019 at 11:14:43AM +0100, Daniel Vetter wrote:
> > > > On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > Yank out the code for the plane->fb/old_fb/crtc handling from
> > > > > the page flip path into page_flip_internal(), and provide a
> > > > > simpler variant for atomic drivers.
> > > > >
> > > > > We'll also move the fb vs. src viewport checks into the new
> > > > > functions as they are slightly different between the two paths.
> > > > > If the atomic .page_flip() implementations are guaranteed
> > > > > to call drm_atomic_plane_check() we could even drop the check
> > > > > entirely from page_flip_atomic(). For now toss in a FIXME.
> > > > >
> > > > > v2: Bit more polish in page_flip_internal()
> > > > >
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
> > > > >  1 file changed, 102 insertions(+), 57 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > > index 38878da5b704..6052475a20a5 100644
> > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
> > > > >         return drm_mode_cursor_common(dev, req, file_priv);
> > > > >  }
> > > > >
> > > > > +static int page_flip_check_fbs(const struct drm_framebuffer *fb,
> > > > > +                              const struct drm_framebuffer *old_fb)
> > > > > +{
> > > > > +       /* The framebuffer is currently unbound, presumably
> > > > > +        * due to a hotplug event, that userspace has not
> > > > > +        * yet discovered.
> > > > > +        */
> > > > > +       if (!old_fb)
> > > > > +               return -EBUSY;
> > > > > +
> > > > > +       if (old_fb->format != fb->format) {
> > > > > +               DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > > > +               return -EINVAL;
> > > > > +       }
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static int page_flip_internal(struct drm_crtc *crtc,
> > > > > +                             struct drm_framebuffer *fb,
> > > > > +                             struct drm_pending_vblank_event *e,
> > > > > +                             u32 flags,
> > > > > +                             u32 target_vblank,
> > > > > +                             struct drm_modeset_acquire_ctx *ctx)
> > > > > +{
> > > > > +       struct drm_plane *plane = crtc->primary;
> > > > > +       int ret;
> > > > > +
> > > > > +       WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
> > > > > +
> > > > > +       ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > > > > +                                     &crtc->mode, fb);
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +
> > > > > +       ret = page_flip_check_fbs(fb, plane->fb);
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +
> > > > > +       plane->old_fb = plane->fb;
> > > > > +       if (crtc->funcs->page_flip_target)
> > > > > +               ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > > > > +                                                   target_vblank, ctx);
> > > > > +       else
> > > > > +               ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > > > > +       if (ret) {
> > > > > +               plane->old_fb = NULL;
> > > > > +               return ret;
> > > > > +       }
> > > > > +
> > > > > +       plane->fb = fb;
> > > > > +       drm_framebuffer_get(plane->fb);
> > > > > +
> > > > > +       drm_framebuffer_put(plane->old_fb);
> > > > > +       plane->old_fb = NULL;
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static int page_flip_atomic(struct drm_crtc *crtc,
> > > > > +                           struct drm_framebuffer *fb,
> > > > > +                           struct drm_pending_vblank_event *e,
> > > > > +                           u32 flags,
> > > > > +                           u32 target_vblank,
> > > > > +                           struct drm_modeset_acquire_ctx *ctx)
> > > > > +{
> > > > > +       struct drm_plane *plane = crtc->primary;
> > > > > +       struct drm_plane_state *plane_state = plane->state;
> > > > > +       int ret;
> > > > > +
> > > > > +       WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
> > > > > +
> > > > > +       /*
> > > > > +        * FIXME: Can we assume all drivers end up calling
> > > > > +        * drm_atomic_plane_check() in their page flip paths?
> > > > > +        * If so we could remove this.
> > > > > +        */
> > > >
> > > > This is definitely wrong, core has to check these. Currently no driver
> > > > checks this at all. I think you're thinking of
> > > > drm_atomic_helper_check_plane_state().
> > >
> > > No, I'm thinking of drm_atomic_plane_check(). That one already does the
> > > "does the src rect fit into the fb" check. It gets called from
> > > drm_atomic_commit()->drm_atomic_check_only() but I wasn't sure if some
> > > drivers might take some short circuit path past that for page_flip().
> >
> > tbh if you're worried about this, I'm tempted to rip out the option for
> > atomic drivers to overwrite their page_flip implementation, and force the
> > default function onto everyone (drm_atomic_helper_page_flip_target and
> > friends would need to be moved to drm_plane.c for that ofc).
> >
> > We seem to have 0 need for hacks for old userspace in this area, which is
> > great. So the option to overwrite could only lead to trouble.
> >
> > > > That aside I'm kinda not getting the point of the shuffling/cleanup in the
> > > > first 4 patches here, so will skip them.
> > >
> > > The point was to get that legacy plane->fb/old_fb stuff totally
> > > out from the atomic codepath. Right now it's polluting the
> > > supposedly higher level code shared by both atomic and legacy
> > > drivers.
> >
> > Hm ... I think if you sell it like that, and maybe even go one further and
> > inline page_flip for all atomic drivers, I can be sold on this. As-is I
> > just didn't really get why you're reworking all this.
>
> Hmm. Inlining doesn't look like 100% trivial code shufling due to
> .page_flip_target. We'd need some other mechanism for the driver to
> indicate support for that.

Ah right, I forgot about that one.

> OTOH I'm not sure we even support that stuff on any atomic driver.
> We have the helper but no one actually uses it. And as ususal I'm
> totally unusre what amdgpu is doing since I keep forgetting which
> parts of it are atomic and which are not.

amdgpu/DC does. I guess the simplest soulution would be to add
mode_config->has_flip_target. It's all wired through atomic states,
just not yet exposed as a property. Until that's done we need a
separate flag.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/7] drm: Simplify the setplane old_fb handling further

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Instead of doing the things in a convoluted way with the failure and
success paths mixed up let's just clear old_fb when we encounter an
error and bail out immediately. We already did this for the pageflip
path.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 6052475a20a5..ef0cc33b43ce 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -688,12 +688,13 @@ static int __setplane_internal(struct drm_plane *plane,
 	if (!fb) {
 		plane->old_fb = plane->fb;
 		ret = plane->funcs->disable_plane(plane, ctx);
-		if (!ret) {
-			plane->crtc = NULL;
-			plane->fb = NULL;
-		} else {
+		if (ret) {
 			plane->old_fb = NULL;
+			return ret;
 		}
+
+		plane->crtc = NULL;
+		plane->fb = NULL;
 		goto out;
 	}
 
@@ -701,26 +702,27 @@ static int __setplane_internal(struct drm_plane *plane,
 			       crtc_x, crtc_y, crtc_w, crtc_h,
 			       src_x, src_y, src_w, src_h);
 	if (ret)
-		goto out;
+		return ret;
 
 	plane->old_fb = plane->fb;
 	ret = plane->funcs->update_plane(plane, crtc, fb,
 					 crtc_x, crtc_y, crtc_w, crtc_h,
 					 src_x, src_y, src_w, src_h, ctx);
-	if (!ret) {
-		plane->crtc = crtc;
-		plane->fb = fb;
-		drm_framebuffer_get(plane->fb);
-	} else {
+	if (ret) {
 		plane->old_fb = NULL;
+		return ret;
 	}
 
+	plane->crtc = crtc;
+	plane->fb = fb;
+	drm_framebuffer_get(plane->fb);
+
 out:
 	if (plane->old_fb)
 		drm_framebuffer_put(plane->old_fb);
 	plane->old_fb = NULL;
 
-	return ret;
+	return 0;
 }
 
 static int __setplane_atomic(struct drm_plane *plane,
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/7] drm: Simplify the setplane old_fb handling further

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Instead of doing the things in a convoluted way with the failure and
success paths mixed up let's just clear old_fb when we encounter an
error and bail out immediately. We already did this for the pageflip
path.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 6052475a20a5..ef0cc33b43ce 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -688,12 +688,13 @@ static int __setplane_internal(struct drm_plane *plane,
 	if (!fb) {
 		plane->old_fb = plane->fb;
 		ret = plane->funcs->disable_plane(plane, ctx);
-		if (!ret) {
-			plane->crtc = NULL;
-			plane->fb = NULL;
-		} else {
+		if (ret) {
 			plane->old_fb = NULL;
+			return ret;
 		}
+
+		plane->crtc = NULL;
+		plane->fb = NULL;
 		goto out;
 	}
 
@@ -701,26 +702,27 @@ static int __setplane_internal(struct drm_plane *plane,
 			       crtc_x, crtc_y, crtc_w, crtc_h,
 			       src_x, src_y, src_w, src_h);
 	if (ret)
-		goto out;
+		return ret;
 
 	plane->old_fb = plane->fb;
 	ret = plane->funcs->update_plane(plane, crtc, fb,
 					 crtc_x, crtc_y, crtc_w, crtc_h,
 					 src_x, src_y, src_w, src_h, ctx);
-	if (!ret) {
-		plane->crtc = crtc;
-		plane->fb = fb;
-		drm_framebuffer_get(plane->fb);
-	} else {
+	if (ret) {
 		plane->old_fb = NULL;
+		return ret;
 	}
 
+	plane->crtc = crtc;
+	plane->fb = fb;
+	drm_framebuffer_get(plane->fb);
+
 out:
 	if (plane->old_fb)
 		drm_framebuffer_put(plane->old_fb);
 	plane->old_fb = NULL;
 
-	return ret;
+	return 0;
 }
 
 static int __setplane_atomic(struct drm_plane *plane,
-- 
2.23.0

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

[PATCH 5/7] drm/selftests: Add some selftests for drm_atomic_set_mode_for_crtc()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Test the basics of drm_atomic_set_mode_for_crtc(), and in particular
verify that the function doesn't take the shortcut incorrectly.

Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/selftests/Makefile            |   3 +-
 .../gpu/drm/selftests/drm_modeset_selftests.h |   2 +
 .../gpu/drm/selftests/test-drm_atomic_uapi.c  | 110 ++++++++++++++++++
 .../drm/selftests/test-drm_modeset_common.h   |   2 +
 4 files changed, 116 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/selftests/test-drm_atomic_uapi.c

diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
index d2137342b371..a5adc25bd345 100644
--- a/drivers/gpu/drm/selftests/Makefile
+++ b/drivers/gpu/drm/selftests/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 test-drm_modeset-y := test-drm_modeset_common.o test-drm_plane_helper.o \
                       test-drm_format.o test-drm_framebuffer.o \
-		      test-drm_damage_helper.o test-drm_dp_mst_helper.o
+		      test-drm_damage_helper.o test-drm_dp_mst_helper.o \
+		      test-drm_atomic_uapi.o
 
 obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o test-drm_cmdline_parser.o
diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
index 1898de0b4a4d..2cc4e2f43286 100644
--- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h
+++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
@@ -6,6 +6,8 @@
  *
  * Tests are executed in order by igt/drm_selftests_helper
  */
+selftest(atomic_set_mode_for_crtc, igt_atomic_set_mode_for_crtc)
+selftest(atomic_set_zeroed_mode_for_crtc, igt_atomic_set_zeroed_mode_for_crtc)
 selftest(check_plane_state, igt_check_plane_state)
 selftest(check_drm_format_block_width, igt_check_drm_format_block_width)
 selftest(check_drm_format_block_height, igt_check_drm_format_block_height)
diff --git a/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c b/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c
new file mode 100644
index 000000000000..0f9a4d0d2541
--- /dev/null
+++ b/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test cases for the drm_atomic_uapi functions
+ */
+
+#define pr_fmt(fmt) "drm_atomic_uapi: " fmt
+
+#include <drm/drm_atomic_uapi.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_modes.h>
+
+#include "test-drm_modeset_common.h"
+
+static const struct drm_display_mode zeroed_mode;
+
+static struct drm_driver mock_driver;
+static struct drm_device mock_device;
+static struct drm_crtc mock_crtc;
+
+static void init(void)
+{
+	memset(&mock_driver, 0, sizeof(mock_driver));
+	memset(&mock_device, 0, sizeof(mock_device));
+	memset(&mock_crtc, 0, sizeof(mock_crtc));
+
+	mock_device.driver = &mock_driver;
+	mock_crtc.dev = &mock_device;
+
+	drm_mode_config_init(&mock_device);
+}
+
+static void cleanup(void)
+{
+	drm_mode_config_cleanup(&mock_device);
+}
+
+int igt_atomic_set_mode_for_crtc(void *ignored)
+{
+	static const struct drm_display_mode mode1 = {
+		DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 25175,
+			 640, 656, 752, 800, 0, 480, 490, 492, 525, 0,
+			 DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
+	};
+	static const struct drm_display_mode mode2 = {
+		DRM_MODE("1024x768", DRM_MODE_TYPE_DRIVER, 65000,
+			 1024, 1048, 1184, 1344, 0, 768, 771, 777, 806, 0,
+			 DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
+	};
+	struct drm_crtc_state crtc_state = {
+		.crtc = &mock_crtc,
+	};
+	int ret;
+
+	init();
+
+	/*
+	 * Make	sure drm_atomic_set_mode_for_crtc()
+	 * updates the crtc state correctly.
+	 */
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state, &mode1);
+	FAIL(ret < 0, "Unable to set mode on crtc\n");
+	FAIL_ON(!crtc_state.enable);
+	FAIL_ON(memcmp(&crtc_state.mode, &mode1, sizeof(mode1)));
+
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state, &mode2);
+	FAIL(ret < 0, "Unable to set mode on crtc\n");
+	FAIL_ON(!crtc_state.enable);
+	FAIL_ON(memcmp(&crtc_state.mode, &mode2, sizeof(mode2)));
+
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state, NULL);
+	FAIL(ret < 0, "Unable to unset mode on crtc\n");
+	FAIL_ON(crtc_state.enable);
+	FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode)));
+
+	cleanup();
+
+	return 0;
+}
+
+int igt_atomic_set_zeroed_mode_for_crtc(void *ignored)
+{
+	struct drm_crtc_state crtc_state = {
+		.crtc = &mock_crtc,
+	};
+	int ret;
+
+	init();
+
+	/*
+	 * Make sure drm_atomic_set_mode_for_crtc() doesn't
+	 * take the shortcut and fail to update crtc_state.enable
+	 * when passed a zeroed mode (which is technically not
+	 * valid, but as drm_atomic_set_mode_for_crtc() does no
+	 * mode validation it should still work).
+	 */
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state, &zeroed_mode);
+	FAIL(ret < 0, "Unable to set mode on crtc\n");
+	FAIL_ON(!crtc_state.enable);
+	FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode)));
+
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state, NULL);
+	FAIL(ret < 0, "Unable to unset mode on crtc\n");
+	FAIL_ON(crtc_state.enable);
+	FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode)));
+
+	cleanup();
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/selftests/test-drm_modeset_common.h b/drivers/gpu/drm/selftests/test-drm_modeset_common.h
index 0fcb8bbc6a1b..a1248abf86f9 100644
--- a/drivers/gpu/drm/selftests/test-drm_modeset_common.h
+++ b/drivers/gpu/drm/selftests/test-drm_modeset_common.h
@@ -13,6 +13,8 @@
 
 #define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(" __stringify(x) ")\n")
 
+int igt_atomic_set_mode_for_crtc(void *ignored);
+int igt_atomic_set_zeroed_mode_for_crtc(void *ignored);
 int igt_check_plane_state(void *ignored);
 int igt_check_drm_format_block_width(void *ignored);
 int igt_check_drm_format_block_height(void *ignored);
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/7] drm/selftests: Add some selftests for drm_atomic_set_mode_for_crtc()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Test the basics of drm_atomic_set_mode_for_crtc(), and in particular
verify that the function doesn't take the shortcut incorrectly.

Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/selftests/Makefile            |   3 +-
 .../gpu/drm/selftests/drm_modeset_selftests.h |   2 +
 .../gpu/drm/selftests/test-drm_atomic_uapi.c  | 110 ++++++++++++++++++
 .../drm/selftests/test-drm_modeset_common.h   |   2 +
 4 files changed, 116 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/selftests/test-drm_atomic_uapi.c

diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
index d2137342b371..a5adc25bd345 100644
--- a/drivers/gpu/drm/selftests/Makefile
+++ b/drivers/gpu/drm/selftests/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 test-drm_modeset-y := test-drm_modeset_common.o test-drm_plane_helper.o \
                       test-drm_format.o test-drm_framebuffer.o \
-		      test-drm_damage_helper.o test-drm_dp_mst_helper.o
+		      test-drm_damage_helper.o test-drm_dp_mst_helper.o \
+		      test-drm_atomic_uapi.o
 
 obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o test-drm_cmdline_parser.o
diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
index 1898de0b4a4d..2cc4e2f43286 100644
--- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h
+++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
@@ -6,6 +6,8 @@
  *
  * Tests are executed in order by igt/drm_selftests_helper
  */
+selftest(atomic_set_mode_for_crtc, igt_atomic_set_mode_for_crtc)
+selftest(atomic_set_zeroed_mode_for_crtc, igt_atomic_set_zeroed_mode_for_crtc)
 selftest(check_plane_state, igt_check_plane_state)
 selftest(check_drm_format_block_width, igt_check_drm_format_block_width)
 selftest(check_drm_format_block_height, igt_check_drm_format_block_height)
diff --git a/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c b/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c
new file mode 100644
index 000000000000..0f9a4d0d2541
--- /dev/null
+++ b/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test cases for the drm_atomic_uapi functions
+ */
+
+#define pr_fmt(fmt) "drm_atomic_uapi: " fmt
+
+#include <drm/drm_atomic_uapi.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_modes.h>
+
+#include "test-drm_modeset_common.h"
+
+static const struct drm_display_mode zeroed_mode;
+
+static struct drm_driver mock_driver;
+static struct drm_device mock_device;
+static struct drm_crtc mock_crtc;
+
+static void init(void)
+{
+	memset(&mock_driver, 0, sizeof(mock_driver));
+	memset(&mock_device, 0, sizeof(mock_device));
+	memset(&mock_crtc, 0, sizeof(mock_crtc));
+
+	mock_device.driver = &mock_driver;
+	mock_crtc.dev = &mock_device;
+
+	drm_mode_config_init(&mock_device);
+}
+
+static void cleanup(void)
+{
+	drm_mode_config_cleanup(&mock_device);
+}
+
+int igt_atomic_set_mode_for_crtc(void *ignored)
+{
+	static const struct drm_display_mode mode1 = {
+		DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 25175,
+			 640, 656, 752, 800, 0, 480, 490, 492, 525, 0,
+			 DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
+	};
+	static const struct drm_display_mode mode2 = {
+		DRM_MODE("1024x768", DRM_MODE_TYPE_DRIVER, 65000,
+			 1024, 1048, 1184, 1344, 0, 768, 771, 777, 806, 0,
+			 DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
+	};
+	struct drm_crtc_state crtc_state = {
+		.crtc = &mock_crtc,
+	};
+	int ret;
+
+	init();
+
+	/*
+	 * Make	sure drm_atomic_set_mode_for_crtc()
+	 * updates the crtc state correctly.
+	 */
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state, &mode1);
+	FAIL(ret < 0, "Unable to set mode on crtc\n");
+	FAIL_ON(!crtc_state.enable);
+	FAIL_ON(memcmp(&crtc_state.mode, &mode1, sizeof(mode1)));
+
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state, &mode2);
+	FAIL(ret < 0, "Unable to set mode on crtc\n");
+	FAIL_ON(!crtc_state.enable);
+	FAIL_ON(memcmp(&crtc_state.mode, &mode2, sizeof(mode2)));
+
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state, NULL);
+	FAIL(ret < 0, "Unable to unset mode on crtc\n");
+	FAIL_ON(crtc_state.enable);
+	FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode)));
+
+	cleanup();
+
+	return 0;
+}
+
+int igt_atomic_set_zeroed_mode_for_crtc(void *ignored)
+{
+	struct drm_crtc_state crtc_state = {
+		.crtc = &mock_crtc,
+	};
+	int ret;
+
+	init();
+
+	/*
+	 * Make sure drm_atomic_set_mode_for_crtc() doesn't
+	 * take the shortcut and fail to update crtc_state.enable
+	 * when passed a zeroed mode (which is technically not
+	 * valid, but as drm_atomic_set_mode_for_crtc() does no
+	 * mode validation it should still work).
+	 */
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state, &zeroed_mode);
+	FAIL(ret < 0, "Unable to set mode on crtc\n");
+	FAIL_ON(!crtc_state.enable);
+	FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode)));
+
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state, NULL);
+	FAIL(ret < 0, "Unable to unset mode on crtc\n");
+	FAIL_ON(crtc_state.enable);
+	FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode)));
+
+	cleanup();
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/selftests/test-drm_modeset_common.h b/drivers/gpu/drm/selftests/test-drm_modeset_common.h
index 0fcb8bbc6a1b..a1248abf86f9 100644
--- a/drivers/gpu/drm/selftests/test-drm_modeset_common.h
+++ b/drivers/gpu/drm/selftests/test-drm_modeset_common.h
@@ -13,6 +13,8 @@
 
 #define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(" __stringify(x) ")\n")
 
+int igt_atomic_set_mode_for_crtc(void *ignored);
+int igt_atomic_set_zeroed_mode_for_crtc(void *ignored);
 int igt_check_plane_state(void *ignored);
 int igt_check_drm_format_block_width(void *ignored);
 int igt_check_drm_format_block_height(void *ignored);
-- 
2.23.0

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

Re: [PATCH 5/7] drm/selftests: Add some selftests for drm_atomic_set_mode_for_crtc()

[Daniel Vetter]

On Fri, Nov 15, 2019 at 09:42:02PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Test the basics of drm_atomic_set_mode_for_crtc(), and in particular
> verify that the function doesn't take the shortcut incorrectly.
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/selftests/Makefile            |   3 +-
>  .../gpu/drm/selftests/drm_modeset_selftests.h |   2 +
>  .../gpu/drm/selftests/test-drm_atomic_uapi.c  | 110 ++++++++++++++++++
>  .../drm/selftests/test-drm_modeset_common.h   |   2 +
>  4 files changed, 116 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/selftests/test-drm_atomic_uapi.c
> 
> diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
> index d2137342b371..a5adc25bd345 100644
> --- a/drivers/gpu/drm/selftests/Makefile
> +++ b/drivers/gpu/drm/selftests/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  test-drm_modeset-y := test-drm_modeset_common.o test-drm_plane_helper.o \
>                        test-drm_format.o test-drm_framebuffer.o \
> -		      test-drm_damage_helper.o test-drm_dp_mst_helper.o
> +		      test-drm_damage_helper.o test-drm_dp_mst_helper.o \
> +		      test-drm_atomic_uapi.o
>  
>  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o test-drm_cmdline_parser.o
> diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> index 1898de0b4a4d..2cc4e2f43286 100644
> --- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> +++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> @@ -6,6 +6,8 @@
>   *
>   * Tests are executed in order by igt/drm_selftests_helper
>   */
> +selftest(atomic_set_mode_for_crtc, igt_atomic_set_mode_for_crtc)
> +selftest(atomic_set_zeroed_mode_for_crtc, igt_atomic_set_zeroed_mode_for_crtc)
>  selftest(check_plane_state, igt_check_plane_state)
>  selftest(check_drm_format_block_width, igt_check_drm_format_block_width)
>  selftest(check_drm_format_block_height, igt_check_drm_format_block_height)
> diff --git a/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c b/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c
> new file mode 100644
> index 000000000000..0f9a4d0d2541
> --- /dev/null
> +++ b/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Test cases for the drm_atomic_uapi functions
> + */
> +
> +#define pr_fmt(fmt) "drm_atomic_uapi: " fmt
> +
> +#include <drm/drm_atomic_uapi.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_modes.h>
> +
> +#include "test-drm_modeset_common.h"
> +
> +static const struct drm_display_mode zeroed_mode;
> +
> +static struct drm_driver mock_driver;
> +static struct drm_device mock_device;
> +static struct drm_crtc mock_crtc;
> +
> +static void init(void)
> +{
> +	memset(&mock_driver, 0, sizeof(mock_driver));
> +	memset(&mock_device, 0, sizeof(mock_device));
> +	memset(&mock_crtc, 0, sizeof(mock_crtc));
> +
> +	mock_device.driver = &mock_driver;
> +	mock_crtc.dev = &mock_device;
> +
> +	drm_mode_config_init(&mock_device);
> +}

Hm some todof for a real mock_driver would be nice. I think the static
stuff is a bit too icky, I think we should go a proper mock_drm_device a
la mock_gem_device in i915 selftests. and mock_drm_crtc. But meh ...

> +
> +static void cleanup(void)
> +{
> +	drm_mode_config_cleanup(&mock_device);
> +}
> +
> +int igt_atomic_set_mode_for_crtc(void *ignored)
> +{
> +	static const struct drm_display_mode mode1 = {
> +		DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 25175,
> +			 640, 656, 752, 800, 0, 480, 490, 492, 525, 0,
> +			 DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
> +	};
> +	static const struct drm_display_mode mode2 = {
> +		DRM_MODE("1024x768", DRM_MODE_TYPE_DRIVER, 65000,
> +			 1024, 1048, 1184, 1344, 0, 768, 771, 777, 806, 0,
> +			 DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
> +	};
> +	struct drm_crtc_state crtc_state = {
> +		.crtc = &mock_crtc,
> +	};
> +	int ret;
> +
> +	init();
> +
> +	/*
> +	 * Make	sure drm_atomic_set_mode_for_crtc()
> +	 * updates the crtc state correctly.
> +	 */
> +	ret = drm_atomic_set_mode_for_crtc(&crtc_state, &mode1);
> +	FAIL(ret < 0, "Unable to set mode on crtc\n");
> +	FAIL_ON(!crtc_state.enable);
> +	FAIL_ON(memcmp(&crtc_state.mode, &mode1, sizeof(mode1)));
> +
> +	ret = drm_atomic_set_mode_for_crtc(&crtc_state, &mode2);
> +	FAIL(ret < 0, "Unable to set mode on crtc\n");
> +	FAIL_ON(!crtc_state.enable);
> +	FAIL_ON(memcmp(&crtc_state.mode, &mode2, sizeof(mode2)));
> +
> +	ret = drm_atomic_set_mode_for_crtc(&crtc_state, NULL);
> +	FAIL(ret < 0, "Unable to unset mode on crtc\n");
> +	FAIL_ON(crtc_state.enable);
> +	FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode)));
> +
> +	cleanup();
> +
> +	return 0;
> +}
> +
> +int igt_atomic_set_zeroed_mode_for_crtc(void *ignored)
> +{
> +	struct drm_crtc_state crtc_state = {
> +		.crtc = &mock_crtc,
> +	};
> +	int ret;
> +
> +	init();
> +
> +	/*
> +	 * Make sure drm_atomic_set_mode_for_crtc() doesn't
> +	 * take the shortcut and fail to update crtc_state.enable
> +	 * when passed a zeroed mode (which is technically not
> +	 * valid, but as drm_atomic_set_mode_for_crtc() does no
> +	 * mode validation it should still work).
> +	 */
> +	ret = drm_atomic_set_mode_for_crtc(&crtc_state, &zeroed_mode);
> +	FAIL(ret < 0, "Unable to set mode on crtc\n");
> +	FAIL_ON(!crtc_state.enable);
> +	FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode)));
> +
> +	ret = drm_atomic_set_mode_for_crtc(&crtc_state, NULL);
> +	FAIL(ret < 0, "Unable to unset mode on crtc\n");
> +	FAIL_ON(crtc_state.enable);
> +	FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode)));

Not really seeing the point of why we have to be careful here, but I guess
silly stuff can happen, and better to not be accidentally inconsistent.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> +
> +	cleanup();
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/selftests/test-drm_modeset_common.h b/drivers/gpu/drm/selftests/test-drm_modeset_common.h
> index 0fcb8bbc6a1b..a1248abf86f9 100644
> --- a/drivers/gpu/drm/selftests/test-drm_modeset_common.h
> +++ b/drivers/gpu/drm/selftests/test-drm_modeset_common.h
> @@ -13,6 +13,8 @@
>  
>  #define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(" __stringify(x) ")\n")
>  
> +int igt_atomic_set_mode_for_crtc(void *ignored);
> +int igt_atomic_set_zeroed_mode_for_crtc(void *ignored);
>  int igt_check_plane_state(void *ignored);
>  int igt_check_drm_format_block_width(void *ignored);
>  int igt_check_drm_format_block_height(void *ignored);
> -- 
> 2.23.0
> 

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

[PATCH 6/7] drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The early return in drm_atomic_set_mode_for_crtc() isn't quite
right. It would mistakenly return and fail to update
crtc_state->enable if someone actually tried to set a zeroed
mode on a currently disabled crtc. That should never actually
happen in response to any userspace request as the zeroed mode
would get rejected earlier. However there is some chance of this
happening internally (eg. during hw state readout) so it seems
best to not let the state become totally inconsistent.

Additionally the early return will not be taken if we're trying to
disable an already disabled crtc. While that is not actually
harmful it is inconsistent, so let's handle that case as well.

Testcase: igt/kms_selftest/check_atomic_set_mode_for_crtc
Testcase: igt/kms_selftest/check_atomic_set_zeroed_mode_fort_crtc
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 0d466d3b0809..a3a6a8137af4 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -68,8 +68,13 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
 	struct drm_mode_modeinfo umode;
 
 	/* Early return for no change. */
-	if (mode && memcmp(&state->mode, mode, sizeof(*mode)) == 0)
-		return 0;
+	if (state->enable) {
+		if (mode && memcmp(&state->mode, mode, sizeof(*mode)) == 0)
+			return 0;
+	} else {
+		if (!mode)
+			return 0;
+	}
 
 	drm_property_blob_put(state->mode_blob);
 	state->mode_blob = NULL;
-- 
2.23.0

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

Re: [PATCH 6/7] drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()

[Daniel Vetter]

On Fri, Nov 15, 2019 at 09:42:03PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The early return in drm_atomic_set_mode_for_crtc() isn't quite
> right. It would mistakenly return and fail to update
> crtc_state->enable if someone actually tried to set a zeroed
> mode on a currently disabled crtc. That should never actually
> happen in response to any userspace request as the zeroed mode
> would get rejected earlier. However there is some chance of this
> happening internally (eg. during hw state readout) so it seems
> best to not let the state become totally inconsistent.
> 
> Additionally the early return will not be taken if we're trying to
> disable an already disabled crtc. While that is not actually
> harmful it is inconsistent, so let's handle that case as well.
> 
> Testcase: igt/kms_selftest/check_atomic_set_mode_for_crtc
> Testcase: igt/kms_selftest/check_atomic_set_zeroed_mode_fort_crtc
> Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 0d466d3b0809..a3a6a8137af4 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -68,8 +68,13 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>  	struct drm_mode_modeinfo umode;
>  
>  	/* Early return for no change. */
> -	if (mode && memcmp(&state->mode, mode, sizeof(*mode)) == 0)
> -		return 0;
> +	if (state->enable) {

Hm I think this would be clearer if you go with

	if (state->enable == !!mode &&
	    (!mode || memcmp(&state->mode, mode, sizeof(*mode)) == 0))
	    	return 0;

But also somewhat a bikeshed. I'm also wondering whether we shouldn't just
declare this a driver bug (it means enable and mode are already out of
sync), but I guess hw state readout is special sometimes.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +		if (mode && memcmp(&state->mode, mode, sizeof(*mode)) == 0)
> +			return 0;
> +	} else {
> +		if (!mode)
> +			return 0;
> +	}
>  
>  	drm_property_blob_put(state->mode_blob);
>  	state->mode_blob = NULL;
> -- 
> 2.23.0
> 

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

Re: [PATCH 6/7] drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()

[Daniel Vetter]

On Fri, Nov 15, 2019 at 09:42:03PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The early return in drm_atomic_set_mode_for_crtc() isn't quite
> right. It would mistakenly return and fail to update
> crtc_state->enable if someone actually tried to set a zeroed
> mode on a currently disabled crtc. That should never actually
> happen in response to any userspace request as the zeroed mode
> would get rejected earlier. However there is some chance of this
> happening internally (eg. during hw state readout) so it seems
> best to not let the state become totally inconsistent.
> 
> Additionally the early return will not be taken if we're trying to
> disable an already disabled crtc. While that is not actually
> harmful it is inconsistent, so let's handle that case as well.
> 
> Testcase: igt/kms_selftest/check_atomic_set_mode_for_crtc
> Testcase: igt/kms_selftest/check_atomic_set_zeroed_mode_fort_crtc
> Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 0d466d3b0809..a3a6a8137af4 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -68,8 +68,13 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>  	struct drm_mode_modeinfo umode;
>  
>  	/* Early return for no change. */
> -	if (mode && memcmp(&state->mode, mode, sizeof(*mode)) == 0)
> -		return 0;
> +	if (state->enable) {

Hm I think this would be clearer if you go with

	if (state->enable == !!mode &&
	    (!mode || memcmp(&state->mode, mode, sizeof(*mode)) == 0))
	    	return 0;

But also somewhat a bikeshed. I'm also wondering whether we shouldn't just
declare this a driver bug (it means enable and mode are already out of
sync), but I guess hw state readout is special sometimes.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +		if (mode && memcmp(&state->mode, mode, sizeof(*mode)) == 0)
> +			return 0;
> +	} else {
> +		if (!mode)
> +			return 0;
> +	}
>  
>  	drm_property_blob_put(state->mode_blob);
>  	state->mode_blob = NULL;
> -- 
> 2.23.0
> 

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

[PATCH 7/7] drm/atomic: Reduce setplane locking

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently setplane grabs all modeset locks, which seems a bit
excessive. Let's reduce that to just the locks we really need
on atomic drivers. For non-atomic drivers let's stick to the
current scheme for now.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 56 +++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index ef0cc33b43ce..e40d8a93294b 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -680,10 +680,14 @@ static int __setplane_internal(struct drm_plane *plane,
 			       uint32_t src_w, uint32_t src_h,
 			       struct drm_modeset_acquire_ctx *ctx)
 {
-	int ret = 0;
+	int ret;
 
 	WARN_ON(drm_drv_uses_atomic_modeset(plane->dev));
 
+	ret = drm_modeset_lock_all_ctx(plane->dev, ctx);
+	if (ret)
+		return ret;
+
 	/* No fb means shut it down */
 	if (!fb) {
 		plane->old_fb = plane->fb;
@@ -767,32 +771,24 @@ static int setplane_internal(struct drm_plane *plane,
 			     uint32_t crtc_w, uint32_t crtc_h,
 			     /* src_{x,y,w,h} values are 16.16 fixed point */
 			     uint32_t src_x, uint32_t src_y,
-			     uint32_t src_w, uint32_t src_h)
+			     uint32_t src_w, uint32_t src_h,
+			     struct drm_modeset_acquire_ctx *ctx)
 {
-	struct drm_modeset_acquire_ctx ctx;
-	int ret;
-
-	DRM_MODESET_LOCK_ALL_BEGIN(plane->dev, ctx,
-				   DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret);
-
 	if (drm_drv_uses_atomic_modeset(plane->dev))
-		ret = __setplane_atomic(plane, crtc, fb,
-					crtc_x, crtc_y, crtc_w, crtc_h,
-					src_x, src_y, src_w, src_h, &ctx);
+		return __setplane_atomic(plane, crtc, fb,
+					 crtc_x, crtc_y, crtc_w, crtc_h,
+					 src_x, src_y, src_w, src_h, ctx);
 	else
-		ret = __setplane_internal(plane, crtc, fb,
-					  crtc_x, crtc_y, crtc_w, crtc_h,
-					  src_x, src_y, src_w, src_h, &ctx);
-
-	DRM_MODESET_LOCK_ALL_END(ctx, ret);
-
-	return ret;
+		return __setplane_internal(plane, crtc, fb,
+					   crtc_x, crtc_y, crtc_w, crtc_h,
+					   src_x, src_y, src_w, src_h, ctx);
 }
 
 int drm_mode_setplane(struct drm_device *dev, void *data,
 		      struct drm_file *file_priv)
 {
 	struct drm_mode_set_plane *plane_req = data;
+	struct drm_modeset_acquire_ctx ctx;
 	struct drm_plane *plane;
 	struct drm_crtc *crtc = NULL;
 	struct drm_framebuffer *fb = NULL;
@@ -829,11 +825,22 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 		}
 	}
 
+	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
+
+retry:
 	ret = setplane_internal(plane, crtc, fb,
 				plane_req->crtc_x, plane_req->crtc_y,
 				plane_req->crtc_w, plane_req->crtc_h,
 				plane_req->src_x, plane_req->src_y,
-				plane_req->src_w, plane_req->src_h);
+				plane_req->src_w, plane_req->src_h, &ctx);
+	if (ret == -EDEADLK) {
+		ret = drm_modeset_backoff(&ctx);
+		if (!ret)
+			goto retry;
+	}
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
 
 	if (fb)
 		drm_framebuffer_put(fb);
@@ -907,14 +914,9 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
 		src_h = fb->height << 16;
 	}
 
-	if (drm_drv_uses_atomic_modeset(dev))
-		ret = __setplane_atomic(plane, crtc, fb,
-					crtc_x, crtc_y, crtc_w, crtc_h,
-					0, 0, src_w, src_h, ctx);
-	else
-		ret = __setplane_internal(plane, crtc, fb,
-					  crtc_x, crtc_y, crtc_w, crtc_h,
-					  0, 0, src_w, src_h, ctx);
+	ret = setplane_internal(plane, crtc, fb,
+				crtc_x, crtc_y, crtc_w, crtc_h,
+				0, 0, src_w, src_h, ctx);
 
 	if (fb)
 		drm_framebuffer_put(fb);
-- 
2.23.0

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

Re: [PATCH 7/7] drm/atomic: Reduce setplane locking

[Daniel Vetter]

On Fri, Nov 15, 2019 at 09:42:04PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently setplane grabs all modeset locks, which seems a bit
> excessive. Let's reduce that to just the locks we really need
> on atomic drivers. For non-atomic drivers let's stick to the
> current scheme for now.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 56 +++++++++++++++++++------------------
>  1 file changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index ef0cc33b43ce..e40d8a93294b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -680,10 +680,14 @@ static int __setplane_internal(struct drm_plane *plane,
>  			       uint32_t src_w, uint32_t src_h,
>  			       struct drm_modeset_acquire_ctx *ctx)
>  {
> -	int ret = 0;
> +	int ret;
>  
>  	WARN_ON(drm_drv_uses_atomic_modeset(plane->dev));
>  
> +	ret = drm_modeset_lock_all_ctx(plane->dev, ctx);
> +	if (ret)
> +		return ret;

There's pre-nv50 nouveau and shmob which have planes and aren't atomic (if
I checked correctly), so we could probably forgo this, but also feels
like totally not worth the audit trouble.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
>  	/* No fb means shut it down */
>  	if (!fb) {
>  		plane->old_fb = plane->fb;
> @@ -767,32 +771,24 @@ static int setplane_internal(struct drm_plane *plane,
>  			     uint32_t crtc_w, uint32_t crtc_h,
>  			     /* src_{x,y,w,h} values are 16.16 fixed point */
>  			     uint32_t src_x, uint32_t src_y,
> -			     uint32_t src_w, uint32_t src_h)
> +			     uint32_t src_w, uint32_t src_h,
> +			     struct drm_modeset_acquire_ctx *ctx)
>  {
> -	struct drm_modeset_acquire_ctx ctx;
> -	int ret;
> -
> -	DRM_MODESET_LOCK_ALL_BEGIN(plane->dev, ctx,
> -				   DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret);
> -
>  	if (drm_drv_uses_atomic_modeset(plane->dev))
> -		ret = __setplane_atomic(plane, crtc, fb,
> -					crtc_x, crtc_y, crtc_w, crtc_h,
> -					src_x, src_y, src_w, src_h, &ctx);
> +		return __setplane_atomic(plane, crtc, fb,
> +					 crtc_x, crtc_y, crtc_w, crtc_h,
> +					 src_x, src_y, src_w, src_h, ctx);
>  	else
> -		ret = __setplane_internal(plane, crtc, fb,
> -					  crtc_x, crtc_y, crtc_w, crtc_h,
> -					  src_x, src_y, src_w, src_h, &ctx);
> -
> -	DRM_MODESET_LOCK_ALL_END(ctx, ret);
> -
> -	return ret;
> +		return __setplane_internal(plane, crtc, fb,
> +					   crtc_x, crtc_y, crtc_w, crtc_h,
> +					   src_x, src_y, src_w, src_h, ctx);
>  }
>  
>  int drm_mode_setplane(struct drm_device *dev, void *data,
>  		      struct drm_file *file_priv)
>  {
>  	struct drm_mode_set_plane *plane_req = data;
> +	struct drm_modeset_acquire_ctx ctx;
>  	struct drm_plane *plane;
>  	struct drm_crtc *crtc = NULL;
>  	struct drm_framebuffer *fb = NULL;
> @@ -829,11 +825,22 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  		}
>  	}
>  
> +	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> +
> +retry:
>  	ret = setplane_internal(plane, crtc, fb,
>  				plane_req->crtc_x, plane_req->crtc_y,
>  				plane_req->crtc_w, plane_req->crtc_h,
>  				plane_req->src_x, plane_req->src_y,
> -				plane_req->src_w, plane_req->src_h);
> +				plane_req->src_w, plane_req->src_h, &ctx);
> +	if (ret == -EDEADLK) {
> +		ret = drm_modeset_backoff(&ctx);
> +		if (!ret)
> +			goto retry;
> +	}
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
>  
>  	if (fb)
>  		drm_framebuffer_put(fb);
> @@ -907,14 +914,9 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
>  		src_h = fb->height << 16;
>  	}
>  
> -	if (drm_drv_uses_atomic_modeset(dev))
> -		ret = __setplane_atomic(plane, crtc, fb,
> -					crtc_x, crtc_y, crtc_w, crtc_h,
> -					0, 0, src_w, src_h, ctx);
> -	else
> -		ret = __setplane_internal(plane, crtc, fb,
> -					  crtc_x, crtc_y, crtc_w, crtc_h,
> -					  0, 0, src_w, src_h, ctx);
> +	ret = setplane_internal(plane, crtc, fb,
> +				crtc_x, crtc_y, crtc_w, crtc_h,
> +				0, 0, src_w, src_h, ctx);
>  
>  	if (fb)
>  		drm_framebuffer_put(fb);
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 7/7] drm/atomic: Reduce setplane locking

[Daniel Vetter]

On Fri, Nov 15, 2019 at 09:42:04PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently setplane grabs all modeset locks, which seems a bit
> excessive. Let's reduce that to just the locks we really need
> on atomic drivers. For non-atomic drivers let's stick to the
> current scheme for now.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 56 +++++++++++++++++++------------------
>  1 file changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index ef0cc33b43ce..e40d8a93294b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -680,10 +680,14 @@ static int __setplane_internal(struct drm_plane *plane,
>  			       uint32_t src_w, uint32_t src_h,
>  			       struct drm_modeset_acquire_ctx *ctx)
>  {
> -	int ret = 0;
> +	int ret;
>  
>  	WARN_ON(drm_drv_uses_atomic_modeset(plane->dev));
>  
> +	ret = drm_modeset_lock_all_ctx(plane->dev, ctx);
> +	if (ret)
> +		return ret;

There's pre-nv50 nouveau and shmob which have planes and aren't atomic (if
I checked correctly), so we could probably forgo this, but also feels
like totally not worth the audit trouble.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
>  	/* No fb means shut it down */
>  	if (!fb) {
>  		plane->old_fb = plane->fb;
> @@ -767,32 +771,24 @@ static int setplane_internal(struct drm_plane *plane,
>  			     uint32_t crtc_w, uint32_t crtc_h,
>  			     /* src_{x,y,w,h} values are 16.16 fixed point */
>  			     uint32_t src_x, uint32_t src_y,
> -			     uint32_t src_w, uint32_t src_h)
> +			     uint32_t src_w, uint32_t src_h,
> +			     struct drm_modeset_acquire_ctx *ctx)
>  {
> -	struct drm_modeset_acquire_ctx ctx;
> -	int ret;
> -
> -	DRM_MODESET_LOCK_ALL_BEGIN(plane->dev, ctx,
> -				   DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret);
> -
>  	if (drm_drv_uses_atomic_modeset(plane->dev))
> -		ret = __setplane_atomic(plane, crtc, fb,
> -					crtc_x, crtc_y, crtc_w, crtc_h,
> -					src_x, src_y, src_w, src_h, &ctx);
> +		return __setplane_atomic(plane, crtc, fb,
> +					 crtc_x, crtc_y, crtc_w, crtc_h,
> +					 src_x, src_y, src_w, src_h, ctx);
>  	else
> -		ret = __setplane_internal(plane, crtc, fb,
> -					  crtc_x, crtc_y, crtc_w, crtc_h,
> -					  src_x, src_y, src_w, src_h, &ctx);
> -
> -	DRM_MODESET_LOCK_ALL_END(ctx, ret);
> -
> -	return ret;
> +		return __setplane_internal(plane, crtc, fb,
> +					   crtc_x, crtc_y, crtc_w, crtc_h,
> +					   src_x, src_y, src_w, src_h, ctx);
>  }
>  
>  int drm_mode_setplane(struct drm_device *dev, void *data,
>  		      struct drm_file *file_priv)
>  {
>  	struct drm_mode_set_plane *plane_req = data;
> +	struct drm_modeset_acquire_ctx ctx;
>  	struct drm_plane *plane;
>  	struct drm_crtc *crtc = NULL;
>  	struct drm_framebuffer *fb = NULL;
> @@ -829,11 +825,22 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  		}
>  	}
>  
> +	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> +
> +retry:
>  	ret = setplane_internal(plane, crtc, fb,
>  				plane_req->crtc_x, plane_req->crtc_y,
>  				plane_req->crtc_w, plane_req->crtc_h,
>  				plane_req->src_x, plane_req->src_y,
> -				plane_req->src_w, plane_req->src_h);
> +				plane_req->src_w, plane_req->src_h, &ctx);
> +	if (ret == -EDEADLK) {
> +		ret = drm_modeset_backoff(&ctx);
> +		if (!ret)
> +			goto retry;
> +	}
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
>  
>  	if (fb)
>  		drm_framebuffer_put(fb);
> @@ -907,14 +914,9 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
>  		src_h = fb->height << 16;
>  	}
>  
> -	if (drm_drv_uses_atomic_modeset(dev))
> -		ret = __setplane_atomic(plane, crtc, fb,
> -					crtc_x, crtc_y, crtc_w, crtc_h,
> -					0, 0, src_w, src_h, ctx);
> -	else
> -		ret = __setplane_internal(plane, crtc, fb,
> -					  crtc_x, crtc_y, crtc_w, crtc_h,
> -					  0, 0, src_w, src_h, ctx);
> +	ret = setplane_internal(plane, crtc, fb,
> +				crtc_x, crtc_y, crtc_w, crtc_h,
> +				0, 0, src_w, src_h, ctx);
>  
>  	if (fb)
>  		drm_framebuffer_put(fb);
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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