[PATCH 0/9] implicit fencing clarification

[PATCH 0/9] implicit fencing clarification

[Daniel Vetter]

Hi all,

Somewhat motivated (but only really tangentially) by the dirtyfb
discussion with Rob and Thomas I started digging around in the various
driver implementations for implicit vs. explicit fencing.

There's definitely a huge pile of drivers which don't do any implicit
fencing at all - not sure that's good or not. And for some of the drivers
with more history I think they don't correctly overwrite implicit fencing
when explicit fencing is present. At least I've gotten lost in the mazes
before I found positive proof.

So this is just the lower hanging stuff, plus a doc patch to hopefully
clarify this all better.

Comments and review and especially in the case of the msm/vc4 patches,
also testing, very much welcome.

Thanks, Daniel

Daniel Vetter (9):
  drm/vmwgfx: Remove no-op prepare/cleanup_fb callbacks
  drm: Move simple_display_pipe prepare_fb helper into gem fb helpers
  drm/tve200: Use simple_display_pipe prepare_fb helper
  drm/pl111: Use simple_display_pipe prepare_fb helper
  drm/mxsfb: Use simple_display_pipe prepare_fb helper
  drm/atomic: better doc for implicit vs explicit fencing
  drm/gem-fb-helper: Always do implicit sync
  drm/vc4: Always obey implicit sync
  drm/msm: Always obey implicit fencing

 drivers/gpu/drm/drm_atomic.c                 |  8 +++++++
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 21 ++++++++++++++++-
 drivers/gpu/drm/msm/msm_atomic.c             |  2 +-
 drivers/gpu/drm/mxsfb/mxsfb_drv.c            |  8 +------
 drivers/gpu/drm/pl111/pl111_display.c        |  8 +------
 drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c  | 17 --------------
 drivers/gpu/drm/tinydrm/ili9225.c            |  2 +-
 drivers/gpu/drm/tinydrm/mi0283qt.c           |  3 ++-
 drivers/gpu/drm/tinydrm/repaper.c            |  2 +-
 drivers/gpu/drm/tinydrm/st7586.c             |  2 +-
 drivers/gpu/drm/tinydrm/st7735r.c            |  2 +-
 drivers/gpu/drm/tve200/tve200_display.c      |  8 +------
 drivers/gpu/drm/vc4/vc4_plane.c              | 11 +++++----
 drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c          | 35 ----------------------------
 include/drm/drm_gem_framebuffer_helper.h     |  3 +++
 include/drm/drm_modeset_helper_vtables.h     |  5 +++-
 include/drm/drm_plane.h                      |  7 +++++-
 include/drm/drm_simple_kms_helper.h          |  3 +++
 include/drm/tinydrm/tinydrm.h                |  2 --
 19 files changed, 61 insertions(+), 88 deletions(-)

-- 
2.16.2

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

[PATCH 1/9] drm/vmwgfx: Remove no-op prepare/cleanup_fb callbacks

[Daniel Vetter]

Less hits to go through when I git grep over all drivers. These
callbacks are optional.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
Cc: Sinclair Yeh <syeh@vmware.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 35 -----------------------------------
 1 file changed, 35 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
index 3824595fece1..4a5907e3f560 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
@@ -281,39 +281,6 @@ drm_connector_helper_funcs vmw_ldu_connector_helper_funcs = {
  * Legacy Display Plane Functions
  */
 
-/**
- * vmw_ldu_primary_plane_cleanup_fb - Noop
- *
- * @plane:  display plane
- * @old_state: Contains the FB to clean up
- *
- * Unpins the display surface
- *
- * Returns 0 on success
- */
-static void
-vmw_ldu_primary_plane_cleanup_fb(struct drm_plane *plane,
-				 struct drm_plane_state *old_state)
-{
-}
-
-
-/**
- * vmw_ldu_primary_plane_prepare_fb - Noop
- *
- * @plane:  display plane
- * @new_state: info on the new plane state, including the FB
- *
- * Returns 0 on success
- */
-static int
-vmw_ldu_primary_plane_prepare_fb(struct drm_plane *plane,
-				 struct drm_plane_state *new_state)
-{
-	return 0;
-}
-
-
 static void
 vmw_ldu_primary_plane_atomic_update(struct drm_plane *plane,
 				    struct drm_plane_state *old_state)
@@ -373,8 +340,6 @@ static const struct
 drm_plane_helper_funcs vmw_ldu_primary_plane_helper_funcs = {
 	.atomic_check = vmw_du_primary_plane_atomic_check,
 	.atomic_update = vmw_ldu_primary_plane_atomic_update,
-	.prepare_fb = vmw_ldu_primary_plane_prepare_fb,
-	.cleanup_fb = vmw_ldu_primary_plane_cleanup_fb,
 };
 
 static const struct drm_crtc_helper_funcs vmw_ldu_crtc_helper_funcs = {
-- 
2.16.2

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

[PATCH 2/9] drm: Move simple_display_pipe prepare_fb helper into gem fb helpers

[Daniel Vetter]

There's nothing tinydrm specific to this, and there's a few more
copies of the same in various other drivers.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: David Lechner <david@lechnology.com>
Cc: "Noralf Trønnes" <noralf@tronnes.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Daniel Stone <daniels@collabora.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 19 +++++++++++++++++++
 drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c  | 17 -----------------
 drivers/gpu/drm/tinydrm/ili9225.c            |  2 +-
 drivers/gpu/drm/tinydrm/mi0283qt.c           |  3 ++-
 drivers/gpu/drm/tinydrm/repaper.c            |  2 +-
 drivers/gpu/drm/tinydrm/st7586.c             |  2 +-
 drivers/gpu/drm/tinydrm/st7735r.c            |  2 +-
 include/drm/drm_gem_framebuffer_helper.h     |  3 +++
 include/drm/drm_simple_kms_helper.h          |  3 +++
 include/drm/tinydrm/tinydrm.h                |  2 --
 10 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index 4d682a6e8bcb..acfbc0641a06 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -22,6 +22,7 @@
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_modeset_helper.h>
+#include <drm/drm_simple_kms_helper.h>
 
 /**
  * DOC: overview
@@ -265,6 +266,24 @@ int drm_gem_fb_prepare_fb(struct drm_plane *plane,
 }
 EXPORT_SYMBOL_GPL(drm_gem_fb_prepare_fb);
 
+/**
+ * drm_gem_fb_simple_display_pipe_prepare_fb - prepare_fb helper for
+ *     &drm_simple_display_pipe
+ * @pipe: Simple display pipe
+ * @plane_state: Plane state
+ *
+ * This function uses drm_gem_fb_prepare_fb() to check if the plane FB has a
+ * &dma_buf attached, extracts the exclusive fence and attaches it to plane
+ * state for the atomic helper to wait on. Drivers can use this as their
+ * &drm_simple_display_pipe_funcs.prepare_fb callback.
+ */
+int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
+					      struct drm_plane_state *plane_state)
+{
+	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
+}
+EXPORT_SYMBOL(drm_gem_fb_simple_display_pipe_prepare_fb);
+
 /**
  * drm_gem_fbdev_fb_create - Create a GEM backed &drm_framebuffer for fbdev
  *                           emulation
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
index e68b528ae64d..7e8e24d0b7a7 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
@@ -138,23 +138,6 @@ void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
 }
 EXPORT_SYMBOL(tinydrm_display_pipe_update);
 
-/**
- * tinydrm_display_pipe_prepare_fb - Display pipe prepare_fb helper
- * @pipe: Simple display pipe
- * @plane_state: Plane state
- *
- * This function uses drm_gem_fb_prepare_fb() to check if the plane FB has an
- * dma-buf attached, extracts the exclusive fence and attaches it to plane
- * state for the atomic helper to wait on. Drivers can use this as their
- * &drm_simple_display_pipe_funcs->prepare_fb callback.
- */
-int tinydrm_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
-				    struct drm_plane_state *plane_state)
-{
-	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
-}
-EXPORT_SYMBOL(tinydrm_display_pipe_prepare_fb);
-
 static int tinydrm_rotate_mode(struct drm_display_mode *mode,
 			       unsigned int rotation)
 {
diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
index 0874e877b111..841c69aba059 100644
--- a/drivers/gpu/drm/tinydrm/ili9225.c
+++ b/drivers/gpu/drm/tinydrm/ili9225.c
@@ -354,7 +354,7 @@ static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
 	.enable		= ili9225_pipe_enable,
 	.disable	= ili9225_pipe_disable,
 	.update		= tinydrm_display_pipe_update,
-	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
+	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
 static const struct drm_display_mode ili9225_mode = {
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 4e6d2ee94e55..d5ef65179c16 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -19,6 +19,7 @@
 
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_modeset_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/tinydrm/mipi-dbi.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
 #include <video/mipi_display.h>
@@ -134,7 +135,7 @@ static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
 	.enable = mi0283qt_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = tinydrm_display_pipe_update,
-	.prepare_fb = tinydrm_display_pipe_prepare_fb,
+	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
 static const struct drm_display_mode mi0283qt_mode = {
diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c
index bb6f80a81899..1ee6855212a0 100644
--- a/drivers/gpu/drm/tinydrm/repaper.c
+++ b/drivers/gpu/drm/tinydrm/repaper.c
@@ -841,7 +841,7 @@ static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
 	.enable = repaper_pipe_enable,
 	.disable = repaper_pipe_disable,
 	.update = tinydrm_display_pipe_update,
-	.prepare_fb = tinydrm_display_pipe_prepare_fb,
+	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
 static const uint32_t repaper_formats[] = {
diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
index 22644b88199a..5c29e3803ecb 100644
--- a/drivers/gpu/drm/tinydrm/st7586.c
+++ b/drivers/gpu/drm/tinydrm/st7586.c
@@ -290,7 +290,7 @@ static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
 	.enable		= st7586_pipe_enable,
 	.disable	= st7586_pipe_disable,
 	.update		= tinydrm_display_pipe_update,
-	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
+	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
 static const struct drm_display_mode st7586_mode = {
diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
index 189a07894d36..6c7b15c9da4f 100644
--- a/drivers/gpu/drm/tinydrm/st7735r.c
+++ b/drivers/gpu/drm/tinydrm/st7735r.c
@@ -106,7 +106,7 @@ static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = {
 	.enable		= jd_t18003_t01_pipe_enable,
 	.disable	= mipi_dbi_pipe_disable,
 	.update		= tinydrm_display_pipe_update,
-	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
+	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
 static const struct drm_display_mode jd_t18003_t01_mode = {
diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
index 5ca7cdc3f527..a38de7eb55b4 100644
--- a/include/drm/drm_gem_framebuffer_helper.h
+++ b/include/drm/drm_gem_framebuffer_helper.h
@@ -10,6 +10,7 @@ struct drm_gem_object;
 struct drm_mode_fb_cmd2;
 struct drm_plane;
 struct drm_plane_state;
+struct drm_simple_display_pipe;
 
 struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
 					  unsigned int plane);
@@ -27,6 +28,8 @@ drm_gem_fb_create(struct drm_device *dev, struct drm_file *file,
 
 int drm_gem_fb_prepare_fb(struct drm_plane *plane,
 			  struct drm_plane_state *state);
+int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
+					      struct drm_plane_state *plane_state);
 
 struct drm_framebuffer *
 drm_gem_fbdev_fb_create(struct drm_device *dev,
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
index b02793742317..451960438a29 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -116,6 +116,9 @@ struct drm_simple_display_pipe_funcs {
 	 * Optional, called by &drm_plane_helper_funcs.prepare_fb.  Please read
 	 * the documentation for the &drm_plane_helper_funcs.prepare_fb hook for
 	 * more details.
+	 *
+	 * Drivers which always have their buffers pinned should use
+	 * drm_gem_fb_simple_display_pipe_prepare_fb() for this hook.
 	 */
 	int (*prepare_fb)(struct drm_simple_display_pipe *pipe,
 			  struct drm_plane_state *plane_state);
diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h
index 6e2b960e25eb..56e4a916b5e8 100644
--- a/include/drm/tinydrm/tinydrm.h
+++ b/include/drm/tinydrm/tinydrm.h
@@ -95,8 +95,6 @@ void tinydrm_shutdown(struct tinydrm_device *tdev);
 
 void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
 				 struct drm_plane_state *old_state);
-int tinydrm_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
-				    struct drm_plane_state *plane_state);
 int
 tinydrm_display_pipe_init(struct tinydrm_device *tdev,
 			  const struct drm_simple_display_pipe_funcs *funcs,
-- 
2.16.2

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

[PATCH 3/9] drm/tve200: Use simple_display_pipe prepare_fb helper

[Daniel Vetter]

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/tve200/tve200_display.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/tve200/tve200_display.c b/drivers/gpu/drm/tve200/tve200_display.c
index 108f3b2b5d25..e8723a2412a6 100644
--- a/drivers/gpu/drm/tve200/tve200_display.c
+++ b/drivers/gpu/drm/tve200/tve200_display.c
@@ -293,18 +293,12 @@ static void tve200_display_disable_vblank(struct drm_simple_display_pipe *pipe)
 	writel(0, priv->regs + TVE200_INT_EN);
 }
 
-static int tve200_display_prepare_fb(struct drm_simple_display_pipe *pipe,
-				    struct drm_plane_state *plane_state)
-{
-	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
-}
-
 static const struct drm_simple_display_pipe_funcs tve200_display_funcs = {
 	.check = tve200_display_check,
 	.enable = tve200_display_enable,
 	.disable = tve200_display_disable,
 	.update = tve200_display_update,
-	.prepare_fb = tve200_display_prepare_fb,
+	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
 	.enable_vblank = tve200_display_enable_vblank,
 	.disable_vblank = tve200_display_disable_vblank,
 };
-- 
2.16.2

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

[PATCH 4/9] drm/pl111: Use simple_display_pipe prepare_fb helper

[Daniel Vetter]

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/pl111/pl111_display.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
index 1fee578e05b0..19b0d006a54a 100644
--- a/drivers/gpu/drm/pl111/pl111_display.c
+++ b/drivers/gpu/drm/pl111/pl111_display.c
@@ -377,19 +377,13 @@ static void pl111_display_disable_vblank(struct drm_simple_display_pipe *pipe)
 	writel(0, priv->regs + priv->ienb);
 }
 
-static int pl111_display_prepare_fb(struct drm_simple_display_pipe *pipe,
-				    struct drm_plane_state *plane_state)
-{
-	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
-}
-
 static struct drm_simple_display_pipe_funcs pl111_display_funcs = {
 	.mode_valid = pl111_mode_valid,
 	.check = pl111_display_check,
 	.enable = pl111_display_enable,
 	.disable = pl111_display_disable,
 	.update = pl111_display_update,
-	.prepare_fb = pl111_display_prepare_fb,
+	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
 static int pl111_clk_div_choose_div(struct clk_hw *hw, unsigned long rate,
-- 
2.16.2

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

[PATCH 5/9] drm/mxsfb: Use simple_display_pipe prepare_fb helper

[Daniel Vetter]

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Marek Vasut <marex@denx.de>
---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index b9c7507813db..ffe5137ccaf8 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -126,12 +126,6 @@ static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe,
 	mxsfb_plane_atomic_update(mxsfb, plane_state);
 }
 
-static int mxsfb_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
-				 struct drm_plane_state *plane_state)
-{
-	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
-}
-
 static int mxsfb_pipe_enable_vblank(struct drm_simple_display_pipe *pipe)
 {
 	struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe);
@@ -160,7 +154,7 @@ static struct drm_simple_display_pipe_funcs mxsfb_funcs = {
 	.enable		= mxsfb_pipe_enable,
 	.disable	= mxsfb_pipe_disable,
 	.update		= mxsfb_pipe_update,
-	.prepare_fb	= mxsfb_pipe_prepare_fb,
+	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
 	.enable_vblank	= mxsfb_pipe_enable_vblank,
 	.disable_vblank	= mxsfb_pipe_disable_vblank,
 };
-- 
2.16.2

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

[PATCH 6/9] drm/atomic: better doc for implicit vs explicit fencing

[Daniel Vetter]

Note that a pile of drivers don't seem to take implicit fencing into
account, or at least don't call drm_atoimc_set_fence_for_plane().
Cc'ing relevant people, or at least some. Some drivers also look like
they don't disable implicit fencing (e.g. amdgpu) because the explicit
fences and implicit fences are handled by entirely independent code
paths.

I also wonder whether we shouldn't just make the recommended helpers
the default ones, since a lot of drivers don't bother to handle the
implicit fences at all it seems. The helpers won't blow up even for
non-GEM drivers or GEM drivers which don't fill out the gem bo
pointers in struct drm_framebuffer.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Sinclair Yeh <syeh@vmware.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic.c             | 8 ++++++++
 include/drm/drm_modeset_helper_vtables.h | 5 ++++-
 include/drm/drm_plane.h                  | 7 ++++++-
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42f22db..ec77afbda0c3 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1492,6 +1492,14 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
  * Otherwise, if &drm_plane_state.fence is not set this function we just set it
  * with the received implicit fence. In both cases this function consumes a
  * reference for @fence.
+ *
+ * This way explicit fencing can be used to overrule implicit fencing, which is
+ * important to make explicit fencing use-cases work: One example is using one
+ * buffer for 2 screens with different refresh rates. Implicit fencing will
+ * clamp rendering to the refresh rate of the slower screen, whereas explicit
+ * fence allows 2 independent render and display loops on a single buffer. If a
+ * driver allows obeys both implicit and explicit fences for plane updates, then
+ * it will break all the benefits of explicit fencing.
  */
 void
 drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 3e76ca805b0f..35e2a3a79fc5 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1004,11 +1004,14 @@ struct drm_plane_helper_funcs {
 	 * This function must not block for outstanding rendering, since it is
 	 * called in the context of the atomic IOCTL even for async commits to
 	 * be able to return any errors to userspace. Instead the recommended
-	 * way is to fill out the fence member of the passed-in
+	 * way is to fill out the &drm_plane_state.fence of the passed-in
 	 * &drm_plane_state. If the driver doesn't support native fences then
 	 * equivalent functionality should be implemented through private
 	 * members in the plane structure.
 	 *
+	 * Drivers which always have their buffers pinned should use
+	 * drm_gem_fb_prepare_fb() for this hook.
+	 *
 	 * The helpers will call @cleanup_fb with matching arguments for every
 	 * successful call to this hook.
 	 *
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index d6da26d66a4b..1e2622e33208 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -80,7 +80,12 @@ struct drm_plane_state {
 	 * @fence:
 	 *
 	 * Optional fence to wait for before scanning out @fb. Do not write this
-	 * directly, use drm_atomic_set_fence_for_plane()
+	 * directly, use drm_atomic_set_fence_for_plane(). The core atomic code
+	 * will set this when userspace is using explicit fencing.
+	 *
+	 * Drivers should store any implicit fences in this from their
+	 * &drm_plane_helper.prepare_fb callback. See drm_gem_fb_prepare_fb()
+	 * and drm_gem_fb_simple_display_pipe_prepare_fb() for suitable helpers.
 	 */
 	struct dma_fence *fence;
 
-- 
2.16.2

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

[PATCH 7/9] drm/gem-fb-helper: Always do implicit sync

[Daniel Vetter]

I've done a lot of history digging. The first signs of this
optimization was introduced in i915:

commit 25067bfc060d1a481584dcb51ef4b5680176ecb6
Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Date:   Wed Sep 10 12:03:17 2014 -0300

    drm/i915: pin sprite fb only if it changed

without much justification. Pinning already pinned stuff is real cheap
(it's just obj->pin_count++ really), and the missing implicit sync was
entirely forgotten about it seems. It's at least not mentioned
anywhere it the commit message.

It was also promptly removed shortly afterwards in

commit ea2c67bb4affa84080c616920f3899f123786e56
Author: Matt Roper <matthew.d.roper@intel.com>
Date:   Tue Dec 23 10:41:52 2014 -0800

    drm/i915: Move to atomic plane helpers (v9)

again without really mentioning the side-effect that plane updates
with the same fb now again obey implicit syncing.

Note that this only ever applied to the plane_update hook, all other
legacy entry points (set_base, page_flip) always obeyed implicit sync
in the drm/i915 driver.

The real source of this code here seems to be msm, copied to vc4, then
copied to tinydrm. I've also tried to dig around in all available msm
sources, but the corresponding check for fb != old_fb is present ever
since the initial merge in

commit cf3a7e4ce08e6876cdcb80390876647f28a7cf8f
Author: Rob Clark <robdclark@gmail.com>
Date:   Sat Nov 8 13:21:06 2014 -0500

    drm/msm: atomic core bits

The only older version I've found of msm atomic code predates the
atomic helpers, and so didn't even use any of this. It also does not
have a corresponding check (because it simply did no implicit sync at
all).

I've chatted with Rob on irc, and he didn't remember the reason for
this either.

Note we had epic amounts of fun with too much syncing against
_vblank_, especially around cursor updates. But I don't ever
discussing a need for less syncing against implicit fences.

Also note that explicit fencing allows you to sidetrack all of this,
at least for all the drivers correctly implemented using
drm_atomic_set_fence_for_plane().

Given that it seems to be an accident of history, and that big drivers
like i915 (and also nouveau it seems, I didn't follow the
amdgpu/radeon sync code to figure this out properly there) never have
done it, let's remove this.

Cc: Rob Clark <robdclark@gmail.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: "Noralf Trønnes" <noralf@tronnes.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index acfbc0641a06..2810d4131411 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -253,7 +253,7 @@ int drm_gem_fb_prepare_fb(struct drm_plane *plane,
 	struct dma_buf *dma_buf;
 	struct dma_fence *fence;
 
-	if (plane->state->fb == state->fb || !state->fb)
+	if (!state->fb)
 		return 0;
 
 	dma_buf = drm_gem_fb_get_obj(state->fb, 0)->dma_buf;
-- 
2.16.2

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

[PATCH 8/9] drm/vc4: Always obey implicit sync

[Daniel Vetter]

Same justification as for drm_gem_fb_prepare_fb.

Cc: Eric Anholt <eric@anholt.net>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/vc4/vc4_plane.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index ce39390be389..498b41d8d4e5 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -748,18 +748,21 @@ static int vc4_prepare_fb(struct drm_plane *plane,
 	struct dma_fence *fence;
 	int ret;
 
-	if ((plane->state->fb == state->fb) || !state->fb)
+	if (!state->fb)
 		return 0;
 
 	bo = to_vc4_bo(&drm_fb_cma_get_gem_obj(state->fb, 0)->base);
 
+	fence = reservation_object_get_excl_rcu(bo->resv);
+	drm_atomic_set_fence_for_plane(state, fence);
+
+	if (plane->state->fb == state->fb)
+		return 0;
+
 	ret = vc4_bo_inc_usecnt(bo);
 	if (ret)
 		return ret;
 
-	fence = reservation_object_get_excl_rcu(bo->resv);
-	drm_atomic_set_fence_for_plane(state, fence);
-
 	return 0;
 }
 
-- 
2.16.2

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

[PATCH 9/9] drm/msm: Always obey implicit fencing

[Daniel Vetter]

Again same justification as for drm_gem_fb_prepare_fb().

Definitely needs some testing because Rob doesn't remember why he did
this, and Google/git.fd.o or anything also doesn't shed some light on
it.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
---
 drivers/gpu/drm/msm/msm_atomic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index bf5f8c39f34d..0dcdc922fc61 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -201,7 +201,7 @@ int msm_atomic_commit(struct drm_device *dev,
 	 * Figure out what fence to wait for:
 	 */
 	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
-		if ((new_plane_state->fb != old_plane_state->fb) && new_plane_state->fb) {
+		if (new_plane_state->fb) {
 			struct drm_gem_object *obj = msm_framebuffer_bo(new_plane_state->fb, 0);
 			struct msm_gem_object *msm_obj = to_msm_bo(obj);
 			struct dma_fence *fence = reservation_object_get_excl_rcu(msm_obj->resv);
-- 
2.16.2

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 1/9] drm/vmwgfx: Remove no-op prepare/cleanup_fb callbacks

[Thomas Hellstrom]

On 04/05/2018 05:44 PM, Daniel Vetter wrote:
> Less hits to go through when I git grep over all drivers. These
> callbacks are optional.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
> Cc: Sinclair Yeh <syeh@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> ---
>   drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 35 -----------------------------------
>   1 file changed, 35 deletions(-)
Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>

Will you take this upstream?

/Thomas

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

Re: [PATCH 1/9] drm/vmwgfx: Remove no-op prepare/cleanup_fb callbacks

[Daniel Vetter]

On Thu, Apr 5, 2018 at 7:06 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> On 04/05/2018 05:44 PM, Daniel Vetter wrote:
>>
>> Less hits to go through when I git grep over all drivers. These
>> callbacks are optional.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
>> Cc: Sinclair Yeh <syeh@vmware.com>
>> Cc: Thomas Hellstrom <thellstrom@vmware.com>
>> ---
>>   drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 35
>> -----------------------------------
>>   1 file changed, 35 deletions(-)
>
> Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>
>
> Will you take this upstream?

Thanks for the review. Assuming I get positive review/testing on the
other patches I can apply them all through drm-misc. Or you can pick
this one up, it's really just a quick aside while I tried to
understand what various drivers are doing in their prepare_fb hook and
around handling implicit/explicit sync. No dependencies with anything
else in this series.
-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

Re: [PATCH 0/9] implicit fencing clarification

[Oleksandr Andrushchenko]

Hi, Daniel!

It seems that this series misses xen-front's
"Use simple_display_pipe prepare_fb helper" change?

Thank you,
Oleksandr

On 04/05/2018 06:44 PM, Daniel Vetter wrote:
> Hi all,
>
> Somewhat motivated (but only really tangentially) by the dirtyfb
> discussion with Rob and Thomas I started digging around in the various
> driver implementations for implicit vs. explicit fencing.
>
> There's definitely a huge pile of drivers which don't do any implicit
> fencing at all - not sure that's good or not. And for some of the drivers
> with more history I think they don't correctly overwrite implicit fencing
> when explicit fencing is present. At least I've gotten lost in the mazes
> before I found positive proof.
>
> So this is just the lower hanging stuff, plus a doc patch to hopefully
> clarify this all better.
>
> Comments and review and especially in the case of the msm/vc4 patches,
> also testing, very much welcome.
>
> Thanks, Daniel
>
> Daniel Vetter (9):
>    drm/vmwgfx: Remove no-op prepare/cleanup_fb callbacks
>    drm: Move simple_display_pipe prepare_fb helper into gem fb helpers
>    drm/tve200: Use simple_display_pipe prepare_fb helper
>    drm/pl111: Use simple_display_pipe prepare_fb helper
>    drm/mxsfb: Use simple_display_pipe prepare_fb helper
>    drm/atomic: better doc for implicit vs explicit fencing
>    drm/gem-fb-helper: Always do implicit sync
>    drm/vc4: Always obey implicit sync
>    drm/msm: Always obey implicit fencing
>
>   drivers/gpu/drm/drm_atomic.c                 |  8 +++++++
>   drivers/gpu/drm/drm_gem_framebuffer_helper.c | 21 ++++++++++++++++-
>   drivers/gpu/drm/msm/msm_atomic.c             |  2 +-
>   drivers/gpu/drm/mxsfb/mxsfb_drv.c            |  8 +------
>   drivers/gpu/drm/pl111/pl111_display.c        |  8 +------
>   drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c  | 17 --------------
>   drivers/gpu/drm/tinydrm/ili9225.c            |  2 +-
>   drivers/gpu/drm/tinydrm/mi0283qt.c           |  3 ++-
>   drivers/gpu/drm/tinydrm/repaper.c            |  2 +-
>   drivers/gpu/drm/tinydrm/st7586.c             |  2 +-
>   drivers/gpu/drm/tinydrm/st7735r.c            |  2 +-
>   drivers/gpu/drm/tve200/tve200_display.c      |  8 +------
>   drivers/gpu/drm/vc4/vc4_plane.c              | 11 +++++----
>   drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c          | 35 ----------------------------
>   include/drm/drm_gem_framebuffer_helper.h     |  3 +++
>   include/drm/drm_modeset_helper_vtables.h     |  5 +++-
>   include/drm/drm_plane.h                      |  7 +++++-
>   include/drm/drm_simple_kms_helper.h          |  3 +++
>   include/drm/tinydrm/tinydrm.h                |  2 --
>   19 files changed, 61 insertions(+), 88 deletions(-)
>

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

Re: [PATCH 3/9] drm/tve200: Use simple_display_pipe prepare_fb helper

[Linus Walleij]

On Thu, Apr 5, 2018 at 5:44 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>

Elegant!

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

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

Re: [PATCH 2/9] drm: Move simple_display_pipe prepare_fb helper into gem fb helpers

[Noralf Trønnes]

Den 05.04.2018 17.44, skrev Daniel Vetter:
> There's nothing tinydrm specific to this, and there's a few more
> copies of the same in various other drivers.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: David Lechner <david@lechnology.com>
> Cc: "Noralf Trønnes" <noralf@tronnes.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/drm_gem_framebuffer_helper.c | 19 +++++++++++++++++++
>   drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c  | 17 -----------------
>   drivers/gpu/drm/tinydrm/ili9225.c            |  2 +-
>   drivers/gpu/drm/tinydrm/mi0283qt.c           |  3 ++-
>   drivers/gpu/drm/tinydrm/repaper.c            |  2 +-
>   drivers/gpu/drm/tinydrm/st7586.c             |  2 +-
>   drivers/gpu/drm/tinydrm/st7735r.c            |  2 +-
>   include/drm/drm_gem_framebuffer_helper.h     |  3 +++
>   include/drm/drm_simple_kms_helper.h          |  3 +++
>   include/drm/tinydrm/tinydrm.h                |  2 --
>   10 files changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index 4d682a6e8bcb..acfbc0641a06 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -22,6 +22,7 @@
>   #include <drm/drm_gem.h>
>   #include <drm/drm_gem_framebuffer_helper.h>
>   #include <drm/drm_modeset_helper.h>
> +#include <drm/drm_simple_kms_helper.h>
>   
>   /**
>    * DOC: overview
> @@ -265,6 +266,24 @@ int drm_gem_fb_prepare_fb(struct drm_plane *plane,
>   }
>   EXPORT_SYMBOL_GPL(drm_gem_fb_prepare_fb);
>   
> +/**
> + * drm_gem_fb_simple_display_pipe_prepare_fb - prepare_fb helper for
> + *     &drm_simple_display_pipe
> + * @pipe: Simple display pipe
> + * @plane_state: Plane state
> + *
> + * This function uses drm_gem_fb_prepare_fb() to check if the plane FB has a
> + * &dma_buf attached, extracts the exclusive fence and attaches it to plane
> + * state for the atomic helper to wait on. Drivers can use this as their
> + * &drm_simple_display_pipe_funcs.prepare_fb callback.
> + */
> +int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> +					      struct drm_plane_state *plane_state)
> +{
> +	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
> +}
> +EXPORT_SYMBOL(drm_gem_fb_simple_display_pipe_prepare_fb);
> +

I was suprised that you didn't put this function in drm_simple_kms_helper.c.
Anyways, thanks for slimming down tinydrm.

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

>   /**
>    * drm_gem_fbdev_fb_create - Create a GEM backed &drm_framebuffer for fbdev
>    *                           emulation
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> index e68b528ae64d..7e8e24d0b7a7 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> @@ -138,23 +138,6 @@ void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
>   }
>   EXPORT_SYMBOL(tinydrm_display_pipe_update);
>   
> -/**
> - * tinydrm_display_pipe_prepare_fb - Display pipe prepare_fb helper
> - * @pipe: Simple display pipe
> - * @plane_state: Plane state
> - *
> - * This function uses drm_gem_fb_prepare_fb() to check if the plane FB has an
> - * dma-buf attached, extracts the exclusive fence and attaches it to plane
> - * state for the atomic helper to wait on. Drivers can use this as their
> - * &drm_simple_display_pipe_funcs->prepare_fb callback.
> - */
> -int tinydrm_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> -				    struct drm_plane_state *plane_state)
> -{
> -	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
> -}
> -EXPORT_SYMBOL(tinydrm_display_pipe_prepare_fb);
> -
>   static int tinydrm_rotate_mode(struct drm_display_mode *mode,
>   			       unsigned int rotation)
>   {
> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
> index 0874e877b111..841c69aba059 100644
> --- a/drivers/gpu/drm/tinydrm/ili9225.c
> +++ b/drivers/gpu/drm/tinydrm/ili9225.c
> @@ -354,7 +354,7 @@ static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
>   	.enable		= ili9225_pipe_enable,
>   	.disable	= ili9225_pipe_disable,
>   	.update		= tinydrm_display_pipe_update,
> -	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
> +	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
>   };
>   
>   static const struct drm_display_mode ili9225_mode = {
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 4e6d2ee94e55..d5ef65179c16 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -19,6 +19,7 @@
>   
>   #include <drm/drm_fb_helper.h>
>   #include <drm/drm_modeset_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
>   #include <drm/tinydrm/mipi-dbi.h>
>   #include <drm/tinydrm/tinydrm-helpers.h>
>   #include <video/mipi_display.h>
> @@ -134,7 +135,7 @@ static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
>   	.enable = mi0283qt_enable,
>   	.disable = mipi_dbi_pipe_disable,
>   	.update = tinydrm_display_pipe_update,
> -	.prepare_fb = tinydrm_display_pipe_prepare_fb,
> +	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
>   };
>   
>   static const struct drm_display_mode mi0283qt_mode = {
> diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c
> index bb6f80a81899..1ee6855212a0 100644
> --- a/drivers/gpu/drm/tinydrm/repaper.c
> +++ b/drivers/gpu/drm/tinydrm/repaper.c
> @@ -841,7 +841,7 @@ static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
>   	.enable = repaper_pipe_enable,
>   	.disable = repaper_pipe_disable,
>   	.update = tinydrm_display_pipe_update,
> -	.prepare_fb = tinydrm_display_pipe_prepare_fb,
> +	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
>   };
>   
>   static const uint32_t repaper_formats[] = {
> diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
> index 22644b88199a..5c29e3803ecb 100644
> --- a/drivers/gpu/drm/tinydrm/st7586.c
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -290,7 +290,7 @@ static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
>   	.enable		= st7586_pipe_enable,
>   	.disable	= st7586_pipe_disable,
>   	.update		= tinydrm_display_pipe_update,
> -	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
> +	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
>   };
>   
>   static const struct drm_display_mode st7586_mode = {
> diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
> index 189a07894d36..6c7b15c9da4f 100644
> --- a/drivers/gpu/drm/tinydrm/st7735r.c
> +++ b/drivers/gpu/drm/tinydrm/st7735r.c
> @@ -106,7 +106,7 @@ static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = {
>   	.enable		= jd_t18003_t01_pipe_enable,
>   	.disable	= mipi_dbi_pipe_disable,
>   	.update		= tinydrm_display_pipe_update,
> -	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
> +	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
>   };
>   
>   static const struct drm_display_mode jd_t18003_t01_mode = {
> diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> index 5ca7cdc3f527..a38de7eb55b4 100644
> --- a/include/drm/drm_gem_framebuffer_helper.h
> +++ b/include/drm/drm_gem_framebuffer_helper.h
> @@ -10,6 +10,7 @@ struct drm_gem_object;
>   struct drm_mode_fb_cmd2;
>   struct drm_plane;
>   struct drm_plane_state;
> +struct drm_simple_display_pipe;
>   
>   struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
>   					  unsigned int plane);
> @@ -27,6 +28,8 @@ drm_gem_fb_create(struct drm_device *dev, struct drm_file *file,
>   
>   int drm_gem_fb_prepare_fb(struct drm_plane *plane,
>   			  struct drm_plane_state *state);
> +int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> +					      struct drm_plane_state *plane_state);
>   
>   struct drm_framebuffer *
>   drm_gem_fbdev_fb_create(struct drm_device *dev,
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index b02793742317..451960438a29 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -116,6 +116,9 @@ struct drm_simple_display_pipe_funcs {
>   	 * Optional, called by &drm_plane_helper_funcs.prepare_fb.  Please read
>   	 * the documentation for the &drm_plane_helper_funcs.prepare_fb hook for
>   	 * more details.
> +	 *
> +	 * Drivers which always have their buffers pinned should use
> +	 * drm_gem_fb_simple_display_pipe_prepare_fb() for this hook.
>   	 */
>   	int (*prepare_fb)(struct drm_simple_display_pipe *pipe,
>   			  struct drm_plane_state *plane_state);
> diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h
> index 6e2b960e25eb..56e4a916b5e8 100644
> --- a/include/drm/tinydrm/tinydrm.h
> +++ b/include/drm/tinydrm/tinydrm.h
> @@ -95,8 +95,6 @@ void tinydrm_shutdown(struct tinydrm_device *tdev);
>   
>   void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
>   				 struct drm_plane_state *old_state);
> -int tinydrm_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> -				    struct drm_plane_state *plane_state);
>   int
>   tinydrm_display_pipe_init(struct tinydrm_device *tdev,
>   			  const struct drm_simple_display_pipe_funcs *funcs,

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

Re: [PATCH 2/9] drm: Move simple_display_pipe prepare_fb helper into gem fb helpers

[Daniel Vetter]

On Fri, Apr 06, 2018 at 11:42:42AM +0200, Noralf Trønnes wrote:
> 
> Den 05.04.2018 17.44, skrev Daniel Vetter:
> > There's nothing tinydrm specific to this, and there's a few more
> > copies of the same in various other drivers.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Gustavo Padovan <gustavo@padovan.org>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: David Lechner <david@lechnology.com>
> > Cc: "Noralf Trønnes" <noralf@tronnes.org>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Daniel Stone <daniels@collabora.com>
> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/drm_gem_framebuffer_helper.c | 19 +++++++++++++++++++
> >   drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c  | 17 -----------------
> >   drivers/gpu/drm/tinydrm/ili9225.c            |  2 +-
> >   drivers/gpu/drm/tinydrm/mi0283qt.c           |  3 ++-
> >   drivers/gpu/drm/tinydrm/repaper.c            |  2 +-
> >   drivers/gpu/drm/tinydrm/st7586.c             |  2 +-
> >   drivers/gpu/drm/tinydrm/st7735r.c            |  2 +-
> >   include/drm/drm_gem_framebuffer_helper.h     |  3 +++
> >   include/drm/drm_simple_kms_helper.h          |  3 +++
> >   include/drm/tinydrm/tinydrm.h                |  2 --
> >   10 files changed, 31 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > index 4d682a6e8bcb..acfbc0641a06 100644
> > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > @@ -22,6 +22,7 @@
> >   #include <drm/drm_gem.h>
> >   #include <drm/drm_gem_framebuffer_helper.h>
> >   #include <drm/drm_modeset_helper.h>
> > +#include <drm/drm_simple_kms_helper.h>
> >   /**
> >    * DOC: overview
> > @@ -265,6 +266,24 @@ int drm_gem_fb_prepare_fb(struct drm_plane *plane,
> >   }
> >   EXPORT_SYMBOL_GPL(drm_gem_fb_prepare_fb);
> > +/**
> > + * drm_gem_fb_simple_display_pipe_prepare_fb - prepare_fb helper for
> > + *     &drm_simple_display_pipe
> > + * @pipe: Simple display pipe
> > + * @plane_state: Plane state
> > + *
> > + * This function uses drm_gem_fb_prepare_fb() to check if the plane FB has a
> > + * &dma_buf attached, extracts the exclusive fence and attaches it to plane
> > + * state for the atomic helper to wait on. Drivers can use this as their
> > + * &drm_simple_display_pipe_funcs.prepare_fb callback.
> > + */
> > +int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> > +					      struct drm_plane_state *plane_state)
> > +{
> > +	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
> > +}
> > +EXPORT_SYMBOL(drm_gem_fb_simple_display_pipe_prepare_fb);
> > +
> 
> I was suprised that you didn't put this function in drm_simple_kms_helper.c.
> Anyways, thanks for slimming down tinydrm.

It was a coin toss between that and the gem helpers. I tossed for gem :-)

> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

Thanks for the review, I'll pull these all in once more acks/reviews have
shown up.
-Daniel

> 
> >   /**
> >    * drm_gem_fbdev_fb_create - Create a GEM backed &drm_framebuffer for fbdev
> >    *                           emulation
> > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> > index e68b528ae64d..7e8e24d0b7a7 100644
> > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> > @@ -138,23 +138,6 @@ void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
> >   }
> >   EXPORT_SYMBOL(tinydrm_display_pipe_update);
> > -/**
> > - * tinydrm_display_pipe_prepare_fb - Display pipe prepare_fb helper
> > - * @pipe: Simple display pipe
> > - * @plane_state: Plane state
> > - *
> > - * This function uses drm_gem_fb_prepare_fb() to check if the plane FB has an
> > - * dma-buf attached, extracts the exclusive fence and attaches it to plane
> > - * state for the atomic helper to wait on. Drivers can use this as their
> > - * &drm_simple_display_pipe_funcs->prepare_fb callback.
> > - */
> > -int tinydrm_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> > -				    struct drm_plane_state *plane_state)
> > -{
> > -	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
> > -}
> > -EXPORT_SYMBOL(tinydrm_display_pipe_prepare_fb);
> > -
> >   static int tinydrm_rotate_mode(struct drm_display_mode *mode,
> >   			       unsigned int rotation)
> >   {
> > diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
> > index 0874e877b111..841c69aba059 100644
> > --- a/drivers/gpu/drm/tinydrm/ili9225.c
> > +++ b/drivers/gpu/drm/tinydrm/ili9225.c
> > @@ -354,7 +354,7 @@ static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
> >   	.enable		= ili9225_pipe_enable,
> >   	.disable	= ili9225_pipe_disable,
> >   	.update		= tinydrm_display_pipe_update,
> > -	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
> > +	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
> >   };
> >   static const struct drm_display_mode ili9225_mode = {
> > diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > index 4e6d2ee94e55..d5ef65179c16 100644
> > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > @@ -19,6 +19,7 @@
> >   #include <drm/drm_fb_helper.h>
> >   #include <drm/drm_modeset_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> >   #include <drm/tinydrm/mipi-dbi.h>
> >   #include <drm/tinydrm/tinydrm-helpers.h>
> >   #include <video/mipi_display.h>
> > @@ -134,7 +135,7 @@ static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
> >   	.enable = mi0283qt_enable,
> >   	.disable = mipi_dbi_pipe_disable,
> >   	.update = tinydrm_display_pipe_update,
> > -	.prepare_fb = tinydrm_display_pipe_prepare_fb,
> > +	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >   };
> >   static const struct drm_display_mode mi0283qt_mode = {
> > diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c
> > index bb6f80a81899..1ee6855212a0 100644
> > --- a/drivers/gpu/drm/tinydrm/repaper.c
> > +++ b/drivers/gpu/drm/tinydrm/repaper.c
> > @@ -841,7 +841,7 @@ static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
> >   	.enable = repaper_pipe_enable,
> >   	.disable = repaper_pipe_disable,
> >   	.update = tinydrm_display_pipe_update,
> > -	.prepare_fb = tinydrm_display_pipe_prepare_fb,
> > +	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> >   };
> >   static const uint32_t repaper_formats[] = {
> > diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
> > index 22644b88199a..5c29e3803ecb 100644
> > --- a/drivers/gpu/drm/tinydrm/st7586.c
> > +++ b/drivers/gpu/drm/tinydrm/st7586.c
> > @@ -290,7 +290,7 @@ static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
> >   	.enable		= st7586_pipe_enable,
> >   	.disable	= st7586_pipe_disable,
> >   	.update		= tinydrm_display_pipe_update,
> > -	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
> > +	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
> >   };
> >   static const struct drm_display_mode st7586_mode = {
> > diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
> > index 189a07894d36..6c7b15c9da4f 100644
> > --- a/drivers/gpu/drm/tinydrm/st7735r.c
> > +++ b/drivers/gpu/drm/tinydrm/st7735r.c
> > @@ -106,7 +106,7 @@ static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = {
> >   	.enable		= jd_t18003_t01_pipe_enable,
> >   	.disable	= mipi_dbi_pipe_disable,
> >   	.update		= tinydrm_display_pipe_update,
> > -	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
> > +	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
> >   };
> >   static const struct drm_display_mode jd_t18003_t01_mode = {
> > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> > index 5ca7cdc3f527..a38de7eb55b4 100644
> > --- a/include/drm/drm_gem_framebuffer_helper.h
> > +++ b/include/drm/drm_gem_framebuffer_helper.h
> > @@ -10,6 +10,7 @@ struct drm_gem_object;
> >   struct drm_mode_fb_cmd2;
> >   struct drm_plane;
> >   struct drm_plane_state;
> > +struct drm_simple_display_pipe;
> >   struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> >   					  unsigned int plane);
> > @@ -27,6 +28,8 @@ drm_gem_fb_create(struct drm_device *dev, struct drm_file *file,
> >   int drm_gem_fb_prepare_fb(struct drm_plane *plane,
> >   			  struct drm_plane_state *state);
> > +int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> > +					      struct drm_plane_state *plane_state);
> >   struct drm_framebuffer *
> >   drm_gem_fbdev_fb_create(struct drm_device *dev,
> > diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> > index b02793742317..451960438a29 100644
> > --- a/include/drm/drm_simple_kms_helper.h
> > +++ b/include/drm/drm_simple_kms_helper.h
> > @@ -116,6 +116,9 @@ struct drm_simple_display_pipe_funcs {
> >   	 * Optional, called by &drm_plane_helper_funcs.prepare_fb.  Please read
> >   	 * the documentation for the &drm_plane_helper_funcs.prepare_fb hook for
> >   	 * more details.
> > +	 *
> > +	 * Drivers which always have their buffers pinned should use
> > +	 * drm_gem_fb_simple_display_pipe_prepare_fb() for this hook.
> >   	 */
> >   	int (*prepare_fb)(struct drm_simple_display_pipe *pipe,
> >   			  struct drm_plane_state *plane_state);
> > diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h
> > index 6e2b960e25eb..56e4a916b5e8 100644
> > --- a/include/drm/tinydrm/tinydrm.h
> > +++ b/include/drm/tinydrm/tinydrm.h
> > @@ -95,8 +95,6 @@ void tinydrm_shutdown(struct tinydrm_device *tdev);
> >   void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
> >   				 struct drm_plane_state *old_state);
> > -int tinydrm_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> > -				    struct drm_plane_state *plane_state);
> >   int
> >   tinydrm_display_pipe_init(struct tinydrm_device *tdev,
> >   			  const struct drm_simple_display_pipe_funcs *funcs,
> 

-- 
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 0/9] implicit fencing clarification

[Daniel Vetter]

On Fri, Apr 06, 2018 at 10:38:38AM +0300, Oleksandr Andrushchenko wrote:
> Hi, Daniel!
> 
> It seems that this series misses xen-front's
> "Use simple_display_pipe prepare_fb helper" change?

Hm indeed, I guess I was on an older tree. Will follow up. Care to review
the other patches meanwhile?
-Daniel

> 
> Thank you,
> Oleksandr
> 
> On 04/05/2018 06:44 PM, Daniel Vetter wrote:
> > Hi all,
> > 
> > Somewhat motivated (but only really tangentially) by the dirtyfb
> > discussion with Rob and Thomas I started digging around in the various
> > driver implementations for implicit vs. explicit fencing.
> > 
> > There's definitely a huge pile of drivers which don't do any implicit
> > fencing at all - not sure that's good or not. And for some of the drivers
> > with more history I think they don't correctly overwrite implicit fencing
> > when explicit fencing is present. At least I've gotten lost in the mazes
> > before I found positive proof.
> > 
> > So this is just the lower hanging stuff, plus a doc patch to hopefully
> > clarify this all better.
> > 
> > Comments and review and especially in the case of the msm/vc4 patches,
> > also testing, very much welcome.
> > 
> > Thanks, Daniel
> > 
> > Daniel Vetter (9):
> >    drm/vmwgfx: Remove no-op prepare/cleanup_fb callbacks
> >    drm: Move simple_display_pipe prepare_fb helper into gem fb helpers
> >    drm/tve200: Use simple_display_pipe prepare_fb helper
> >    drm/pl111: Use simple_display_pipe prepare_fb helper
> >    drm/mxsfb: Use simple_display_pipe prepare_fb helper
> >    drm/atomic: better doc for implicit vs explicit fencing
> >    drm/gem-fb-helper: Always do implicit sync
> >    drm/vc4: Always obey implicit sync
> >    drm/msm: Always obey implicit fencing
> > 
> >   drivers/gpu/drm/drm_atomic.c                 |  8 +++++++
> >   drivers/gpu/drm/drm_gem_framebuffer_helper.c | 21 ++++++++++++++++-
> >   drivers/gpu/drm/msm/msm_atomic.c             |  2 +-
> >   drivers/gpu/drm/mxsfb/mxsfb_drv.c            |  8 +------
> >   drivers/gpu/drm/pl111/pl111_display.c        |  8 +------
> >   drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c  | 17 --------------
> >   drivers/gpu/drm/tinydrm/ili9225.c            |  2 +-
> >   drivers/gpu/drm/tinydrm/mi0283qt.c           |  3 ++-
> >   drivers/gpu/drm/tinydrm/repaper.c            |  2 +-
> >   drivers/gpu/drm/tinydrm/st7586.c             |  2 +-
> >   drivers/gpu/drm/tinydrm/st7735r.c            |  2 +-
> >   drivers/gpu/drm/tve200/tve200_display.c      |  8 +------
> >   drivers/gpu/drm/vc4/vc4_plane.c              | 11 +++++----
> >   drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c          | 35 ----------------------------
> >   include/drm/drm_gem_framebuffer_helper.h     |  3 +++
> >   include/drm/drm_modeset_helper_vtables.h     |  5 +++-
> >   include/drm/drm_plane.h                      |  7 +++++-
> >   include/drm/drm_simple_kms_helper.h          |  3 +++
> >   include/drm/tinydrm/tinydrm.h                |  2 --
> >   19 files changed, 61 insertions(+), 88 deletions(-)
> > 
> 

-- 
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] drm/xen-front: use simple display pipe prepare_fb helper

[Daniel Vetter]

I missed this one because on an older tree.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Cc: xen-devel@lists.xen.org
---
 drivers/gpu/drm/xen/xen_drm_front_kms.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
index 0bd6681fa4f3..a3479eb72d79 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
@@ -226,12 +226,6 @@ static bool display_send_page_flip(struct drm_simple_display_pipe *pipe,
 	return false;
 }
 
-static int display_prepare_fb(struct drm_simple_display_pipe *pipe,
-			      struct drm_plane_state *plane_state)
-{
-	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
-}
-
 static void display_update(struct drm_simple_display_pipe *pipe,
 			   struct drm_plane_state *old_plane_state)
 {
@@ -294,7 +288,7 @@ static const struct drm_simple_display_pipe_funcs display_funcs = {
 	.mode_valid = display_mode_valid,
 	.enable = display_enable,
 	.disable = display_disable,
-	.prepare_fb = display_prepare_fb,
+	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
 	.update = display_update,
 };
 
-- 
2.16.2

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

[PATCH] drm/xen-front: use simple display pipe prepare_fb helper

[Daniel Vetter]

I missed this one because on an older tree.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Cc: xen-devel@lists.xen.org
---
 drivers/gpu/drm/xen/xen_drm_front_kms.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
index 0bd6681fa4f3..a3479eb72d79 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
@@ -226,12 +226,6 @@ static bool display_send_page_flip(struct drm_simple_display_pipe *pipe,
 	return false;
 }
 
-static int display_prepare_fb(struct drm_simple_display_pipe *pipe,
-			      struct drm_plane_state *plane_state)
-{
-	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
-}
-
 static void display_update(struct drm_simple_display_pipe *pipe,
 			   struct drm_plane_state *old_plane_state)
 {
@@ -294,7 +288,7 @@ static const struct drm_simple_display_pipe_funcs display_funcs = {
 	.mode_valid = display_mode_valid,
 	.enable = display_enable,
 	.disable = display_disable,
-	.prepare_fb = display_prepare_fb,
+	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
 	.update = display_update,
 };
 
-- 
2.16.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/9] drm: Move simple_display_pipe prepare_fb helper into gem fb helpers

[David Lechner]

On 04/05/2018 10:44 AM, Daniel Vetter wrote:
> There's nothing tinydrm specific to this, and there's a few more
> copies of the same in various other drivers.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: David Lechner <david@lechnology.com>
> Cc: "Noralf Trønnes" <noralf@tronnes.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> ---

Acked-by: David Lechner <david@lechnology.com>


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

Re: [PATCH 2/9] drm: Move simple_display_pipe prepare_fb helper into gem fb helpers

[Oleksandr Andrushchenko]

On 04/05/2018 06:44 PM, Daniel Vetter wrote:
> There's nothing tinydrm specific to this, and there's a few more
> copies of the same in various other drivers.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: David Lechner <david@lechnology.com>
> Cc: "Noralf Trønnes" <noralf@tronnes.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/drm_gem_framebuffer_helper.c | 19 +++++++++++++++++++
>   drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c  | 17 -----------------
>   drivers/gpu/drm/tinydrm/ili9225.c            |  2 +-
>   drivers/gpu/drm/tinydrm/mi0283qt.c           |  3 ++-
>   drivers/gpu/drm/tinydrm/repaper.c            |  2 +-
>   drivers/gpu/drm/tinydrm/st7586.c             |  2 +-
>   drivers/gpu/drm/tinydrm/st7735r.c            |  2 +-
>   include/drm/drm_gem_framebuffer_helper.h     |  3 +++
>   include/drm/drm_simple_kms_helper.h          |  3 +++
>   include/drm/tinydrm/tinydrm.h                |  2 --
>   10 files changed, 31 insertions(+), 24 deletions(-)
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index 4d682a6e8bcb..acfbc0641a06 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -22,6 +22,7 @@
>   #include <drm/drm_gem.h>
>   #include <drm/drm_gem_framebuffer_helper.h>
>   #include <drm/drm_modeset_helper.h>
> +#include <drm/drm_simple_kms_helper.h>
>   
>   /**
>    * DOC: overview
> @@ -265,6 +266,24 @@ int drm_gem_fb_prepare_fb(struct drm_plane *plane,
>   }
>   EXPORT_SYMBOL_GPL(drm_gem_fb_prepare_fb);
>   
> +/**
> + * drm_gem_fb_simple_display_pipe_prepare_fb - prepare_fb helper for
> + *     &drm_simple_display_pipe
> + * @pipe: Simple display pipe
> + * @plane_state: Plane state
> + *
> + * This function uses drm_gem_fb_prepare_fb() to check if the plane FB has a
> + * &dma_buf attached, extracts the exclusive fence and attaches it to plane
> + * state for the atomic helper to wait on. Drivers can use this as their
> + * &drm_simple_display_pipe_funcs.prepare_fb callback.
> + */
> +int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> +					      struct drm_plane_state *plane_state)
> +{
> +	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
> +}
> +EXPORT_SYMBOL(drm_gem_fb_simple_display_pipe_prepare_fb);
> +
>   /**
>    * drm_gem_fbdev_fb_create - Create a GEM backed &drm_framebuffer for fbdev
>    *                           emulation
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> index e68b528ae64d..7e8e24d0b7a7 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> @@ -138,23 +138,6 @@ void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
>   }
>   EXPORT_SYMBOL(tinydrm_display_pipe_update);
>   
> -/**
> - * tinydrm_display_pipe_prepare_fb - Display pipe prepare_fb helper
> - * @pipe: Simple display pipe
> - * @plane_state: Plane state
> - *
> - * This function uses drm_gem_fb_prepare_fb() to check if the plane FB has an
> - * dma-buf attached, extracts the exclusive fence and attaches it to plane
> - * state for the atomic helper to wait on. Drivers can use this as their
> - * &drm_simple_display_pipe_funcs->prepare_fb callback.
> - */
> -int tinydrm_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> -				    struct drm_plane_state *plane_state)
> -{
> -	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
> -}
> -EXPORT_SYMBOL(tinydrm_display_pipe_prepare_fb);
> -
>   static int tinydrm_rotate_mode(struct drm_display_mode *mode,
>   			       unsigned int rotation)
>   {
> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
> index 0874e877b111..841c69aba059 100644
> --- a/drivers/gpu/drm/tinydrm/ili9225.c
> +++ b/drivers/gpu/drm/tinydrm/ili9225.c
> @@ -354,7 +354,7 @@ static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
>   	.enable		= ili9225_pipe_enable,
>   	.disable	= ili9225_pipe_disable,
>   	.update		= tinydrm_display_pipe_update,
> -	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
> +	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
>   };
>   
>   static const struct drm_display_mode ili9225_mode = {
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 4e6d2ee94e55..d5ef65179c16 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -19,6 +19,7 @@
>   
>   #include <drm/drm_fb_helper.h>
>   #include <drm/drm_modeset_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
>   #include <drm/tinydrm/mipi-dbi.h>
>   #include <drm/tinydrm/tinydrm-helpers.h>
>   #include <video/mipi_display.h>
> @@ -134,7 +135,7 @@ static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
>   	.enable = mi0283qt_enable,
>   	.disable = mipi_dbi_pipe_disable,
>   	.update = tinydrm_display_pipe_update,
> -	.prepare_fb = tinydrm_display_pipe_prepare_fb,
> +	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
>   };
>   
>   static const struct drm_display_mode mi0283qt_mode = {
> diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c
> index bb6f80a81899..1ee6855212a0 100644
> --- a/drivers/gpu/drm/tinydrm/repaper.c
> +++ b/drivers/gpu/drm/tinydrm/repaper.c
> @@ -841,7 +841,7 @@ static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
>   	.enable = repaper_pipe_enable,
>   	.disable = repaper_pipe_disable,
>   	.update = tinydrm_display_pipe_update,
> -	.prepare_fb = tinydrm_display_pipe_prepare_fb,
> +	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
>   };
>   
>   static const uint32_t repaper_formats[] = {
> diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
> index 22644b88199a..5c29e3803ecb 100644
> --- a/drivers/gpu/drm/tinydrm/st7586.c
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -290,7 +290,7 @@ static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
>   	.enable		= st7586_pipe_enable,
>   	.disable	= st7586_pipe_disable,
>   	.update		= tinydrm_display_pipe_update,
> -	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
> +	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
>   };
>   
>   static const struct drm_display_mode st7586_mode = {
> diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
> index 189a07894d36..6c7b15c9da4f 100644
> --- a/drivers/gpu/drm/tinydrm/st7735r.c
> +++ b/drivers/gpu/drm/tinydrm/st7735r.c
> @@ -106,7 +106,7 @@ static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = {
>   	.enable		= jd_t18003_t01_pipe_enable,
>   	.disable	= mipi_dbi_pipe_disable,
>   	.update		= tinydrm_display_pipe_update,
> -	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
> +	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
>   };
>   
>   static const struct drm_display_mode jd_t18003_t01_mode = {
> diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> index 5ca7cdc3f527..a38de7eb55b4 100644
> --- a/include/drm/drm_gem_framebuffer_helper.h
> +++ b/include/drm/drm_gem_framebuffer_helper.h
> @@ -10,6 +10,7 @@ struct drm_gem_object;
>   struct drm_mode_fb_cmd2;
>   struct drm_plane;
>   struct drm_plane_state;
> +struct drm_simple_display_pipe;
>   
>   struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
>   					  unsigned int plane);
> @@ -27,6 +28,8 @@ drm_gem_fb_create(struct drm_device *dev, struct drm_file *file,
>   
>   int drm_gem_fb_prepare_fb(struct drm_plane *plane,
>   			  struct drm_plane_state *state);
> +int drm_gem_fb_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> +					      struct drm_plane_state *plane_state);
>   
>   struct drm_framebuffer *
>   drm_gem_fbdev_fb_create(struct drm_device *dev,
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index b02793742317..451960438a29 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -116,6 +116,9 @@ struct drm_simple_display_pipe_funcs {
>   	 * Optional, called by &drm_plane_helper_funcs.prepare_fb.  Please read
>   	 * the documentation for the &drm_plane_helper_funcs.prepare_fb hook for
>   	 * more details.
> +	 *
> +	 * Drivers which always have their buffers pinned should use
> +	 * drm_gem_fb_simple_display_pipe_prepare_fb() for this hook.
>   	 */
>   	int (*prepare_fb)(struct drm_simple_display_pipe *pipe,
>   			  struct drm_plane_state *plane_state);
> diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h
> index 6e2b960e25eb..56e4a916b5e8 100644
> --- a/include/drm/tinydrm/tinydrm.h
> +++ b/include/drm/tinydrm/tinydrm.h
> @@ -95,8 +95,6 @@ void tinydrm_shutdown(struct tinydrm_device *tdev);
>   
>   void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
>   				 struct drm_plane_state *old_state);
> -int tinydrm_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> -				    struct drm_plane_state *plane_state);
>   int
>   tinydrm_display_pipe_init(struct tinydrm_device *tdev,
>   			  const struct drm_simple_display_pipe_funcs *funcs,

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

Re: [PATCH] drm/xen-front: use simple display pipe prepare_fb helper

[Oleksandr Andrushchenko]

On 04/09/2018 11:51 AM, Daniel Vetter wrote:
> I missed this one because on an older tree.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Cc: xen-devel@lists.xen.org
> ---
>   drivers/gpu/drm/xen/xen_drm_front_kms.c | 8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> index 0bd6681fa4f3..a3479eb72d79 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> @@ -226,12 +226,6 @@ static bool display_send_page_flip(struct drm_simple_display_pipe *pipe,
>   	return false;
>   }
>   
> -static int display_prepare_fb(struct drm_simple_display_pipe *pipe,
> -			      struct drm_plane_state *plane_state)
> -{
> -	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
> -}
> -
>   static void display_update(struct drm_simple_display_pipe *pipe,
>   			   struct drm_plane_state *old_plane_state)
>   {
> @@ -294,7 +288,7 @@ static const struct drm_simple_display_pipe_funcs display_funcs = {
>   	.mode_valid = display_mode_valid,
>   	.enable = display_enable,
>   	.disable = display_disable,
> -	.prepare_fb = display_prepare_fb,
> +	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
>   	.update = display_update,
>   };
>   
Thank you,
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/xen-front: use simple display pipe prepare_fb helper

[Oleksandr Andrushchenko]

On 04/09/2018 11:51 AM, Daniel Vetter wrote:
> I missed this one because on an older tree.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Cc: xen-devel@lists.xen.org
> ---
>   drivers/gpu/drm/xen/xen_drm_front_kms.c | 8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> index 0bd6681fa4f3..a3479eb72d79 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> @@ -226,12 +226,6 @@ static bool display_send_page_flip(struct drm_simple_display_pipe *pipe,
>   	return false;
>   }
>   
> -static int display_prepare_fb(struct drm_simple_display_pipe *pipe,
> -			      struct drm_plane_state *plane_state)
> -{
> -	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
> -}
> -
>   static void display_update(struct drm_simple_display_pipe *pipe,
>   			   struct drm_plane_state *old_plane_state)
>   {
> @@ -294,7 +288,7 @@ static const struct drm_simple_display_pipe_funcs display_funcs = {
>   	.mode_valid = display_mode_valid,
>   	.enable = display_enable,
>   	.disable = display_disable,
> -	.prepare_fb = display_prepare_fb,
> +	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
>   	.update = display_update,
>   };
>   
Thank you,
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/9] drm/mxsfb: Use simple_display_pipe prepare_fb helper

[Eric Anholt]

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

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Marek Vasut <marex@denx.de>

4,5 are:

Reviewed-by: Eric Anholt <eric@anholt.net>

It would be great to land this series up to here soon, as I was
surprised to see another new simple_display_pipe prepare implementation
in a driver I reviewed this week.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

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

Re: [PATCH 6/9] drm/atomic: better doc for implicit vs explicit fencing

[Eric Anholt]

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

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> Note that a pile of drivers don't seem to take implicit fencing into
> account, or at least don't call drm_atoimc_set_fence_for_plane().

                                     ^atomic

> Cc'ing relevant people, or at least some. Some drivers also look like
> they don't disable implicit fencing (e.g. amdgpu) because the explicit
> fences and implicit fences are handled by entirely independent code
> paths.
>
> I also wonder whether we shouldn't just make the recommended helpers
> the default ones, since a lot of drivers don't bother to handle the
> implicit fences at all it seems. The helpers won't blow up even for
> non-GEM drivers or GEM drivers which don't fill out the gem bo
> pointers in struct drm_framebuffer.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Sinclair Yeh <syeh@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c             | 8 ++++++++
>  include/drm/drm_modeset_helper_vtables.h | 5 ++++-
>  include/drm/drm_plane.h                  | 7 ++++++-
>  3 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7d25c42f22db..ec77afbda0c3 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1492,6 +1492,14 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>   * Otherwise, if &drm_plane_state.fence is not set this function we just set it
>   * with the received implicit fence. In both cases this function consumes a
>   * reference for @fence.
> + *
> + * This way explicit fencing can be used to overrule implicit fencing, which is
> + * important to make explicit fencing use-cases work: One example is using one
> + * buffer for 2 screens with different refresh rates. Implicit fencing will
> + * clamp rendering to the refresh rate of the slower screen, whereas explicit
> + * fence allows 2 independent render and display loops on a single buffer. If a
> + * driver allows obeys both implicit and explicit fences for plane updates, then
> + * it will break all the benefits of explicit fencing.
>   */

Thanks for explaining why we should care about explicit fencing for
display!  I'd been trying and failing to generate a usecase.

> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index d6da26d66a4b..1e2622e33208 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -80,7 +80,12 @@ struct drm_plane_state {
>  	 * @fence:
>  	 *
>  	 * Optional fence to wait for before scanning out @fb. Do not write this
> -	 * directly, use drm_atomic_set_fence_for_plane()
> +	 * directly, use drm_atomic_set_fence_for_plane(). The core atomic code
> +	 * will set this when userspace is using explicit fencing.

Optional suggestion:

	 * Optional fence to wait for before scanning out @fb. The core
	 * atomic code will set this when userspace is using explicit
	 * fencing. Do not write this directly for a driver's implicit
	 * fence, use drm_atomic_set_fence_for_plane() to ensure that
	 * an explicit fence is preserved.

> +	 *
> +	 * Drivers should store any implicit fences in this from their

Maybe s/fences/fence/ to make it more obvious that you can only attach
one?

> +	 * &drm_plane_helper.prepare_fb callback. See drm_gem_fb_prepare_fb()
> +	 * and drm_gem_fb_simple_display_pipe_prepare_fb() for suitable helpers.
>  	 */
>  	struct dma_fence *fence;

Regardless,

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

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

Re: [PATCH 7/9] drm/gem-fb-helper: Always do implicit sync

[Eric Anholt]

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

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> I've done a lot of history digging. The first signs of this
> optimization was introduced in i915:

I can't come up with any reason this would matter.  I almost came up
with "You're doing tearing X11 front buffer rendering, and you do a
modeset using the same fb, and so you block that modeset behind your
rendering."  Except that:

1) who cares
2) this helper is only for dma-bufs, not normal X11 rendering
3) your X11 driver should be doing pageflipping to be tear-free anyway,
   let's just fix that[1].

Reviewed-by: Eric Anholt <eric@anholt.net>

[1] This is not actually me volunteering myself or anyone else to go fix
that.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

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

Re: [PATCH 8/9] drm/vc4: Always obey implicit sync

[Eric Anholt]

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

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> Same justification as for drm_gem_fb_prepare_fb.

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

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

Re: [PATCH 6/9] drm/atomic: better doc for implicit vs explicit fencing

[Daniel Vetter]

On Fri, Apr 20, 2018 at 03:04:58PM -0700, Eric Anholt wrote:
> Daniel Vetter <daniel.vetter@ffwll.ch> writes:
> 
> > Note that a pile of drivers don't seem to take implicit fencing into
> > account, or at least don't call drm_atoimc_set_fence_for_plane().
> 
>                                      ^atomic
> 
> > Cc'ing relevant people, or at least some. Some drivers also look like
> > they don't disable implicit fencing (e.g. amdgpu) because the explicit
> > fences and implicit fences are handled by entirely independent code
> > paths.
> >
> > I also wonder whether we shouldn't just make the recommended helpers
> > the default ones, since a lot of drivers don't bother to handle the
> > implicit fences at all it seems. The helpers won't blow up even for
> > non-GEM drivers or GEM drivers which don't fill out the gem bo
> > pointers in struct drm_framebuffer.
> >
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Sinclair Yeh <syeh@vmware.com>
> > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > Cc: Gustavo Padovan <gustavo@padovan.org>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic.c             | 8 ++++++++
> >  include/drm/drm_modeset_helper_vtables.h | 5 ++++-
> >  include/drm/drm_plane.h                  | 7 ++++++-
> >  3 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 7d25c42f22db..ec77afbda0c3 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1492,6 +1492,14 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
> >   * Otherwise, if &drm_plane_state.fence is not set this function we just set it
> >   * with the received implicit fence. In both cases this function consumes a
> >   * reference for @fence.
> > + *
> > + * This way explicit fencing can be used to overrule implicit fencing, which is
> > + * important to make explicit fencing use-cases work: One example is using one
> > + * buffer for 2 screens with different refresh rates. Implicit fencing will
> > + * clamp rendering to the refresh rate of the slower screen, whereas explicit
> > + * fence allows 2 independent render and display loops on a single buffer. If a
> > + * driver allows obeys both implicit and explicit fences for plane updates, then
> > + * it will break all the benefits of explicit fencing.
> >   */
> 
> Thanks for explaining why we should care about explicit fencing for
> display!  I'd been trying and failing to generate a usecase.
> 
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index d6da26d66a4b..1e2622e33208 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -80,7 +80,12 @@ struct drm_plane_state {
> >  	 * @fence:
> >  	 *
> >  	 * Optional fence to wait for before scanning out @fb. Do not write this
> > -	 * directly, use drm_atomic_set_fence_for_plane()
> > +	 * directly, use drm_atomic_set_fence_for_plane(). The core atomic code
> > +	 * will set this when userspace is using explicit fencing.
> 
> Optional suggestion:
> 
> 	 * Optional fence to wait for before scanning out @fb. The core
> 	 * atomic code will set this when userspace is using explicit
> 	 * fencing. Do not write this directly for a driver's implicit
> 	 * fence, use drm_atomic_set_fence_for_plane() to ensure that
> 	 * an explicit fence is preserved.

Yeah, that's better.
> 
> > +	 *
> > +	 * Drivers should store any implicit fences in this from their
> 
> Maybe s/fences/fence/ to make it more obvious that you can only attach
> one?

Both suggestions implemented while applying, thanks very much for your
review.
-Daniel

> 
> > +	 * &drm_plane_helper.prepare_fb callback. See drm_gem_fb_prepare_fb()
> > +	 * and drm_gem_fb_simple_display_pipe_prepare_fb() for suitable helpers.
> >  	 */
> >  	struct dma_fence *fence;
> 
> Regardless,
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>



-- 
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 7/9] drm/gem-fb-helper: Always do implicit sync

[Daniel Vetter]

On Fri, Apr 20, 2018 at 03:11:24PM -0700, Eric Anholt wrote:
> Daniel Vetter <daniel.vetter@ffwll.ch> writes:
> 
> > I've done a lot of history digging. The first signs of this
> > optimization was introduced in i915:
> 
> I can't come up with any reason this would matter.  I almost came up
> with "You're doing tearing X11 front buffer rendering, and you do a
> modeset using the same fb, and so you block that modeset behind your
> rendering."  Except that:
> 
> 1) who cares
> 2) this helper is only for dma-bufs, not normal X11 rendering
> 3) your X11 driver should be doing pageflipping to be tear-free anyway,
>    let's just fix that[1].
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>
> 
> [1] This is not actually me volunteering myself or anyone else to go fix
> that.

Ok, merged everything except the final three patches. I'll poke Rob for a
proper ack first before doing that. There's not much point in trying to
unify behaviour if there's still a driver doing things differently :-)

Thanks very much for your review.
-Daniel
-- 
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 9/9] drm/msm: Always obey implicit fencing

[Daniel Vetter]

On Thu, Apr 05, 2018 at 05:44:49PM +0200, Daniel Vetter wrote:
> Again same justification as for drm_gem_fb_prepare_fb().
> 
> Definitely needs some testing because Rob doesn't remember why he did
> this, and Google/git.fd.o or anything also doesn't shed some light on
> it.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org

Just rebased this and noticed it's supreseeded by Sean Paul's work. I'd
appreciate some more official msm group maintainership so I have more
people to nag, since this not going anywhere has held up the entire patch
series.
-Daniel

> ---
>  drivers/gpu/drm/msm/msm_atomic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index bf5f8c39f34d..0dcdc922fc61 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -201,7 +201,7 @@ int msm_atomic_commit(struct drm_device *dev,
>  	 * Figure out what fence to wait for:
>  	 */
>  	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> -		if ((new_plane_state->fb != old_plane_state->fb) && new_plane_state->fb) {
> +		if (new_plane_state->fb) {
>  			struct drm_gem_object *obj = msm_framebuffer_bo(new_plane_state->fb, 0);
>  			struct msm_gem_object *msm_obj = to_msm_bo(obj);
>  			struct dma_fence *fence = reservation_object_get_excl_rcu(msm_obj->resv);
> -- 
> 2.16.2
> 

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

Re: [PATCH 7/9] drm/gem-fb-helper: Always do implicit sync

[Daniel Vetter]

On Tue, Apr 24, 2018 at 02:04:10PM +0200, Daniel Vetter wrote:
> On Fri, Apr 20, 2018 at 03:11:24PM -0700, Eric Anholt wrote:
> > Daniel Vetter <daniel.vetter@ffwll.ch> writes:
> > 
> > > I've done a lot of history digging. The first signs of this
> > > optimization was introduced in i915:
> > 
> > I can't come up with any reason this would matter.  I almost came up
> > with "You're doing tearing X11 front buffer rendering, and you do a
> > modeset using the same fb, and so you block that modeset behind your
> > rendering."  Except that:
> > 
> > 1) who cares
> > 2) this helper is only for dma-bufs, not normal X11 rendering
> > 3) your X11 driver should be doing pageflipping to be tear-free anyway,
> >    let's just fix that[1].
> > 
> > Reviewed-by: Eric Anholt <eric@anholt.net>
> > 
> > [1] This is not actually me volunteering myself or anyone else to go fix
> > that.
> 
> Ok, merged everything except the final three patches. I'll poke Rob for a
> proper ack first before doing that. There's not much point in trying to
> unify behaviour if there's still a driver doing things differently :-)
> 
> Thanks very much for your review.

Ok with the msm conversion to atomic commit helpers msm aligned to what
we're doing here without my patch. I pulled in the remaining to for 4.19.
-Daniel
-- 
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