All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/5] drm/tinydrm: Use damage helper for dirtyfb
@ 2019-01-11 20:12 Noralf Trønnes
  2019-01-11 20:12 ` [PATCH v2 1/5] drm/gem-fb-helper: Add drm_gem_fb_create_with_dirty() Noralf Trønnes
                   ` (5 more replies)
  0 siblings, 6 replies; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-11 20:12 UTC (permalink / raw)
  To: dri-devel; +Cc: drawat, sam, david

Changes:
- Improve drm_gem_fb_create_with_dirty() docs (Daniel)
- Remove unnecessary clearing of damage clips
- Rebase on Sam's drmP.h series
- Remove fb NULL check in mipi_dbi_enable_flush() (kbuild test robot)
- Update todo.rst (Sam)

Maxime has done a backmerge so the damage helper is now in
drm-misc-next.

Noralf.

Noralf Trønnes (5):
  drm/gem-fb-helper: Add drm_gem_fb_create_with_dirty()
  drm/damage-helper: Add drm_atomic_helper_damage_merged()
  drm/tinydrm: Use struct drm_rect
  drm/tinydrm: Use damage helper for dirtyfb
  drm/todo: Tick off some tinydrm entries

 Documentation/gpu/todo.rst                    |  35 -----
 drivers/gpu/drm/drm_damage_helper.c           |  41 ++++++
 drivers/gpu/drm/drm_gem_framebuffer_helper.c  |  50 ++++++-
 drivers/gpu/drm/tinydrm/core/tinydrm-core.c   |  21 +--
 .../gpu/drm/tinydrm/core/tinydrm-helpers.c    | 100 +------------
 drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c   |  30 ----
 drivers/gpu/drm/tinydrm/hx8357d.c             |   2 +-
 drivers/gpu/drm/tinydrm/ili9225.c             | 138 +++++++-----------
 drivers/gpu/drm/tinydrm/ili9341.c             |   2 +-
 drivers/gpu/drm/tinydrm/mi0283qt.c            |   2 +-
 drivers/gpu/drm/tinydrm/mipi-dbi.c            |  89 ++++++-----
 drivers/gpu/drm/tinydrm/repaper.c             |  42 +++---
 drivers/gpu/drm/tinydrm/st7586.c              |  73 ++++-----
 drivers/gpu/drm/tinydrm/st7735r.c             |   2 +-
 include/drm/drm_damage_helper.h               |   3 +
 include/drm/drm_gem_framebuffer_helper.h      |   3 +
 include/drm/tinydrm/mipi-dbi.h                |   5 +-
 include/drm/tinydrm/tinydrm-helpers.h         |  20 +--
 include/drm/tinydrm/tinydrm.h                 |  26 ----
 19 files changed, 282 insertions(+), 402 deletions(-)

-- 
2.20.1

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
* [PATCH v2 1/5] drm/gem-fb-helper: Add drm_gem_fb_create_with_dirty()
  2019-01-11 20:12 [PATCH v2 0/5] drm/tinydrm: Use damage helper for dirtyfb Noralf Trønnes
@ 2019-01-11 20:12 ` Noralf Trønnes
  2019-01-11 20:12 ` [PATCH v2 2/5] drm/damage-helper: Add drm_atomic_helper_damage_merged() Noralf Trønnes
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-11 20:12 UTC (permalink / raw)
  To: dri-devel; +Cc: Daniel Vetter, drawat, sam, david

This adds a .fb_create helper that sets the .dirty callback to
drm_atomic_helper_dirtyfb().

v2: Improve docs (Daniel)

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 50 +++++++++++++++++---
 include/drm/drm_gem_framebuffer_helper.h     |  3 ++
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index acb466d25afc..65edb1ccb185 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -17,6 +17,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_uapi.h>
+#include <drm/drm_damage_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_framebuffer.h>
@@ -136,10 +137,9 @@ EXPORT_SYMBOL(drm_gem_fb_create_handle);
  * @mode_cmd: Metadata from the userspace framebuffer creation request
  * @funcs: vtable to be used for the new framebuffer object
  *
- * This can be used to set &drm_framebuffer_funcs for drivers that need the
- * &drm_framebuffer_funcs.dirty callback. Use drm_gem_fb_create() if you don't
- * need to change &drm_framebuffer_funcs.
- * The function does buffer size validation.
+ * This function can be used to set &drm_framebuffer_funcs for drivers that need
+ * custom framebuffer callbacks. Use drm_gem_fb_create() if you don't need to
+ * change &drm_framebuffer_funcs. The function does buffer size validation.
  *
  * Returns:
  * Pointer to a &drm_framebuffer on success or an error pointer on failure.
@@ -215,8 +215,8 @@ static const struct drm_framebuffer_funcs drm_gem_fb_funcs = {
  *
  * If your hardware has special alignment or pitch requirements these should be
  * checked before calling this function. The function does buffer size
- * validation. Use drm_gem_fb_create_with_funcs() if you need to set
- * &drm_framebuffer_funcs.dirty.
+ * validation. Use drm_gem_fb_create_with_dirty() if you need framebuffer
+ * flushing.
  *
  * Drivers can use this as their &drm_mode_config_funcs.fb_create callback.
  * The ADDFB2 IOCTL calls into this callback.
@@ -233,6 +233,44 @@ drm_gem_fb_create(struct drm_device *dev, struct drm_file *file,
 }
 EXPORT_SYMBOL_GPL(drm_gem_fb_create);
 
+static const struct drm_framebuffer_funcs drm_gem_fb_funcs_dirtyfb = {
+	.destroy	= drm_gem_fb_destroy,
+	.create_handle	= drm_gem_fb_create_handle,
+	.dirty		= drm_atomic_helper_dirtyfb,
+};
+
+/**
+ * drm_gem_fb_create_with_dirty() - Helper function for the
+ *                       &drm_mode_config_funcs.fb_create callback
+ * @dev: DRM device
+ * @file: DRM file that holds the GEM handle(s) backing the framebuffer
+ * @mode_cmd: Metadata from the userspace framebuffer creation request
+ *
+ * This function creates a new framebuffer object described by
+ * &drm_mode_fb_cmd2. This description includes handles for the buffer(s)
+ * backing the framebuffer. drm_atomic_helper_dirtyfb() is used for the dirty
+ * callback giving framebuffer flushing through the atomic machinery. Use
+ * drm_gem_fb_create() if you don't need the dirty callback.
+ * The function does buffer size validation.
+ *
+ * Drivers should also call drm_plane_enable_fb_damage_clips() on all planes
+ * to enable userspace to use damage clips also with the ATOMIC IOCTL.
+ *
+ * Drivers can use this as their &drm_mode_config_funcs.fb_create callback.
+ * The ADDFB2 IOCTL calls into this callback.
+ *
+ * Returns:
+ * Pointer to a &drm_framebuffer on success or an error pointer on failure.
+ */
+struct drm_framebuffer *
+drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file,
+			     const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
+					    &drm_gem_fb_funcs_dirtyfb);
+}
+EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty);
+
 /**
  * drm_gem_fb_prepare_fb() - Prepare a GEM backed framebuffer
  * @plane: Plane
diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
index a38de7eb55b4..7f307e834eef 100644
--- a/include/drm/drm_gem_framebuffer_helper.h
+++ b/include/drm/drm_gem_framebuffer_helper.h
@@ -25,6 +25,9 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
 struct drm_framebuffer *
 drm_gem_fb_create(struct drm_device *dev, struct drm_file *file,
 		  const struct drm_mode_fb_cmd2 *mode_cmd);
+struct drm_framebuffer *
+drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file,
+			     const struct drm_mode_fb_cmd2 *mode_cmd);
 
 int drm_gem_fb_prepare_fb(struct drm_plane *plane,
 			  struct drm_plane_state *state);
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH v2 2/5] drm/damage-helper: Add drm_atomic_helper_damage_merged()
  2019-01-11 20:12 [PATCH v2 0/5] drm/tinydrm: Use damage helper for dirtyfb Noralf Trønnes
  2019-01-11 20:12 ` [PATCH v2 1/5] drm/gem-fb-helper: Add drm_gem_fb_create_with_dirty() Noralf Trønnes
@ 2019-01-11 20:12 ` Noralf Trønnes
  2019-01-11 20:12 ` [PATCH v2 3/5] drm/tinydrm: Use struct drm_rect Noralf Trønnes
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-11 20:12 UTC (permalink / raw)
  To: dri-devel; +Cc: Daniel Vetter, drawat, sam, david

Useful for drivers that only care about the combined damage.

v2: Remove unnecessary clearing of damage clips

Cc: Deepak Rawat <drawat@vmware.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_damage_helper.c | 41 +++++++++++++++++++++++++++++
 include/drm/drm_damage_helper.h     |  3 +++
 2 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index 31032407254d..e16aa5ae00b4 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -333,3 +333,44 @@ drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
 	return ret;
 }
 EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
+
+/**
+ * drm_atomic_helper_damage_merged - Merged plane damage
+ * @old_state: Old plane state for validation.
+ * @state: Plane state from which to iterate the damage clips.
+ * @rect: Returns the merged damage rectangle
+ *
+ * This function merges any valid plane damage clips into one rectangle and
+ * returns it in @rect.
+ *
+ * For details see: drm_atomic_helper_damage_iter_init() and
+ * drm_atomic_helper_damage_iter_next().
+ *
+ * Returns:
+ * True if there is valid plane damage otherwise false.
+ */
+bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
+				     struct drm_plane_state *state,
+				     struct drm_rect *rect)
+{
+	struct drm_atomic_helper_damage_iter iter;
+	struct drm_rect clip;
+	bool valid = false;
+
+	rect->x1 = INT_MAX;
+	rect->y1 = INT_MAX;
+	rect->x2 = 0;
+	rect->y2 = 0;
+
+	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
+	drm_atomic_for_each_plane_damage(&iter, &clip) {
+		rect->x1 = min(rect->x1, clip.x1);
+		rect->y1 = min(rect->y1, clip.y1);
+		rect->x2 = max(rect->x2, clip.x2);
+		rect->y2 = max(rect->y2, clip.y2);
+		valid = true;
+	}
+
+	return valid;
+}
+EXPORT_SYMBOL(drm_atomic_helper_damage_merged);
diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h
index 4487660b26b8..40c34a5bf149 100644
--- a/include/drm/drm_damage_helper.h
+++ b/include/drm/drm_damage_helper.h
@@ -78,6 +78,9 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
 bool
 drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
 				   struct drm_rect *rect);
+bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
+				     struct drm_plane_state *state,
+				     struct drm_rect *rect);
 
 /**
  * drm_helper_get_plane_damage_clips - Returns damage clips in &drm_rect.
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH v2 3/5] drm/tinydrm: Use struct drm_rect
  2019-01-11 20:12 [PATCH v2 0/5] drm/tinydrm: Use damage helper for dirtyfb Noralf Trønnes
  2019-01-11 20:12 ` [PATCH v2 1/5] drm/gem-fb-helper: Add drm_gem_fb_create_with_dirty() Noralf Trønnes
  2019-01-11 20:12 ` [PATCH v2 2/5] drm/damage-helper: Add drm_atomic_helper_damage_merged() Noralf Trønnes
@ 2019-01-11 20:12 ` Noralf Trønnes
  2019-01-11 20:12 ` [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb Noralf Trønnes
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-11 20:12 UTC (permalink / raw)
  To: dri-devel; +Cc: Daniel Vetter, drawat, sam, david

This prepares for the switch to drm_atomic_helper_dirtyfb() in the next
patch. The damage helper returns a drm_rect so switch to that everywhere
including using a pointer in the dirty functions.

This is a non-functional change except for the debug print which looks a
bit different.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 .../gpu/drm/tinydrm/core/tinydrm-helpers.c    | 19 ++++----
 drivers/gpu/drm/tinydrm/ili9225.c             | 43 ++++++++++---------
 drivers/gpu/drm/tinydrm/mipi-dbi.c            | 21 ++++-----
 drivers/gpu/drm/tinydrm/repaper.c             |  3 +-
 drivers/gpu/drm/tinydrm/st7586.c              | 27 ++++++------
 include/drm/tinydrm/mipi-dbi.h                |  3 +-
 include/drm/tinydrm/tinydrm-helpers.h         | 11 ++---
 7 files changed, 67 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index bf6bfbc5d412..d0ece6ad4a1c 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -18,6 +18,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_print.h>
+#include <drm/drm_rect.h>
 #include <drm/tinydrm/tinydrm.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
 #include <uapi/drm/drm.h>
@@ -41,7 +42,7 @@ MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size");
  * Returns:
  * true if it's a full clip, false otherwise
  */
-bool tinydrm_merge_clips(struct drm_clip_rect *dst,
+bool tinydrm_merge_clips(struct drm_rect *dst,
 			 struct drm_clip_rect *src, unsigned int num_clips,
 			 unsigned int flags, u32 max_width, u32 max_height)
 {
@@ -63,10 +64,10 @@ bool tinydrm_merge_clips(struct drm_clip_rect *dst,
 	for (i = 0; i < num_clips; i++) {
 		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
 			i++;
-		dst->x1 = min(dst->x1, src[i].x1);
-		dst->x2 = max(dst->x2, src[i].x2);
-		dst->y1 = min(dst->y1, src[i].y1);
-		dst->y2 = max(dst->y2, src[i].y2);
+		dst->x1 = min_t(int, dst->x1, src[i].x1);
+		dst->x2 = max_t(int, dst->x2, src[i].x2);
+		dst->y1 = min_t(int, dst->y1, src[i].y1);
+		dst->y2 = max_t(int, dst->y2, src[i].y2);
 	}
 
 	if (dst->x2 > max_width || dst->y2 > max_height ||
@@ -122,7 +123,7 @@ EXPORT_SYMBOL(tinydrm_fb_dirty);
  * @clip: Clip rectangle area to copy
  */
 void tinydrm_memcpy(void *dst, void *vaddr, struct drm_framebuffer *fb,
-		    struct drm_clip_rect *clip)
+		    struct drm_rect *clip)
 {
 	unsigned int cpp = drm_format_plane_cpp(fb->format->format, 0);
 	unsigned int pitch = fb->pitches[0];
@@ -146,7 +147,7 @@ EXPORT_SYMBOL(tinydrm_memcpy);
  * @clip: Clip rectangle area to copy
  */
 void tinydrm_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb,
-		    struct drm_clip_rect *clip)
+		    struct drm_rect *clip)
 {
 	size_t len = (clip->x2 - clip->x1) * sizeof(u16);
 	unsigned int x, y;
@@ -186,7 +187,7 @@ EXPORT_SYMBOL(tinydrm_swab16);
  */
 void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
 				struct drm_framebuffer *fb,
-				struct drm_clip_rect *clip, bool swap)
+				struct drm_rect *clip, bool swap)
 {
 	size_t len = (clip->x2 - clip->x1) * sizeof(u32);
 	unsigned int x, y;
@@ -235,7 +236,7 @@ EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
  * ITU BT.601 is used for the RGB -> luma (brightness) conversion.
  */
 void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
-			       struct drm_clip_rect *clip)
+			       struct drm_rect *clip)
 {
 	unsigned int len = (clip->x2 - clip->x1) * sizeof(u32);
 	unsigned int x, y;
diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
index b0ad58b97227..ad644069089f 100644
--- a/drivers/gpu/drm/tinydrm/ili9225.c
+++ b/drivers/gpu/drm/tinydrm/ili9225.c
@@ -25,6 +25,7 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_rect.h>
 #include <drm/tinydrm/mipi-dbi.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
 
@@ -84,7 +85,8 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb,
 	struct tinydrm_device *tdev = fb->dev->dev_private;
 	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
 	bool swap = mipi->swap_bytes;
-	struct drm_clip_rect clip;
+	struct drm_rect clip;
+	struct drm_rect *rect = &clip;
 	u16 x_start, y_start;
 	u16 x1, x2, y1, y2;
 	int ret = 0;
@@ -97,13 +99,12 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb,
 	full = tinydrm_merge_clips(&clip, clips, num_clips, flags,
 				   fb->width, fb->height);
 
-	DRM_DEBUG("Flushing [FB:%d] x1=%u, x2=%u, y1=%u, y2=%u\n", fb->base.id,
-		  clip.x1, clip.x2, clip.y1, clip.y2);
+	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
 
 	if (!mipi->dc || !full || swap ||
 	    fb->format->format == DRM_FORMAT_XRGB8888) {
 		tr = mipi->tx_buf;
-		ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip, swap);
+		ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, rect, swap);
 		if (ret)
 			return ret;
 	} else {
@@ -112,34 +113,34 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb,
 
 	switch (mipi->rotation) {
 	default:
-		x1 = clip.x1;
-		x2 = clip.x2 - 1;
-		y1 = clip.y1;
-		y2 = clip.y2 - 1;
+		x1 = rect->x1;
+		x2 = rect->x2 - 1;
+		y1 = rect->y1;
+		y2 = rect->y2 - 1;
 		x_start = x1;
 		y_start = y1;
 		break;
 	case 90:
-		x1 = clip.y1;
-		x2 = clip.y2 - 1;
-		y1 = fb->width - clip.x2;
-		y2 = fb->width - clip.x1 - 1;
+		x1 = rect->y1;
+		x2 = rect->y2 - 1;
+		y1 = fb->width - rect->x2;
+		y2 = fb->width - rect->x1 - 1;
 		x_start = x1;
 		y_start = y2;
 		break;
 	case 180:
-		x1 = fb->width - clip.x2;
-		x2 = fb->width - clip.x1 - 1;
-		y1 = fb->height - clip.y2;
-		y2 = fb->height - clip.y1 - 1;
+		x1 = fb->width - rect->x2;
+		x2 = fb->width - rect->x1 - 1;
+		y1 = fb->height - rect->y2;
+		y2 = fb->height - rect->y1 - 1;
 		x_start = x2;
 		y_start = y2;
 		break;
 	case 270:
-		x1 = fb->height - clip.y2;
-		x2 = fb->height - clip.y1 - 1;
-		y1 = clip.x1;
-		y2 = clip.x2 - 1;
+		x1 = fb->height - rect->y2;
+		x2 = fb->height - rect->y1 - 1;
+		y1 = rect->x1;
+		y2 = rect->x2 - 1;
 		x_start = x2;
 		y_start = y1;
 		break;
@@ -154,7 +155,7 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb,
 	ili9225_command(mipi, ILI9225_RAM_ADDRESS_SET_2, y_start);
 
 	ret = mipi_dbi_command_buf(mipi, ILI9225_WRITE_DATA_TO_GRAM, tr,
-				(clip.x2 - clip.x1) * (clip.y2 - clip.y1) * 2);
+				   (rect->x2 - rect->x1) * (rect->y2 - rect->y1) * 2);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index 10294e1283dd..1e7e8cfc99f0 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -22,6 +22,7 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_rect.h>
 #include <drm/tinydrm/mipi-dbi.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
 #include <uapi/drm/drm.h>
@@ -172,7 +173,7 @@ EXPORT_SYMBOL(mipi_dbi_command_buf);
  * Zero on success, negative error code on failure.
  */
 int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
-		      struct drm_clip_rect *clip, bool swap)
+		      struct drm_rect *clip, bool swap)
 {
 	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
 	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
@@ -221,7 +222,8 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
 	struct tinydrm_device *tdev = fb->dev->dev_private;
 	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
 	bool swap = mipi->swap_bytes;
-	struct drm_clip_rect clip;
+	struct drm_rect clip;
+	struct drm_rect *rect = &clip;
 	int ret = 0;
 	bool full;
 	void *tr;
@@ -232,13 +234,12 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
 	full = tinydrm_merge_clips(&clip, clips, num_clips, flags,
 				   fb->width, fb->height);
 
-	DRM_DEBUG("Flushing [FB:%d] x1=%u, x2=%u, y1=%u, y2=%u\n", fb->base.id,
-		  clip.x1, clip.x2, clip.y1, clip.y2);
+	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
 
 	if (!mipi->dc || !full || swap ||
 	    fb->format->format == DRM_FORMAT_XRGB8888) {
 		tr = mipi->tx_buf;
-		ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip, swap);
+		ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, rect, swap);
 		if (ret)
 			return ret;
 	} else {
@@ -246,14 +247,14 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
 	}
 
 	mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS,
-			 (clip.x1 >> 8) & 0xFF, clip.x1 & 0xFF,
-			 ((clip.x2 - 1) >> 8) & 0xFF, (clip.x2 - 1) & 0xFF);
+			 (rect->x1 >> 8) & 0xff, rect->x1 & 0xff,
+			 ((rect->x2 - 1) >> 8) & 0xff, (rect->x2 - 1) & 0xff);
 	mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS,
-			 (clip.y1 >> 8) & 0xFF, clip.y1 & 0xFF,
-			 ((clip.y2 - 1) >> 8) & 0xFF, (clip.y2 - 1) & 0xFF);
+			 (rect->y1 >> 8) & 0xff, rect->y1 & 0xff,
+			 ((rect->y2 - 1) >> 8) & 0xff, (rect->y2 - 1) & 0xff);
 
 	ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START, tr,
-				(clip.x2 - clip.x1) * (clip.y2 - clip.y1) * 2);
+				   (rect->x2 - rect->x1) * (rect->y2 - rect->y1) * 2);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c
index b2a8f894946a..238515de449e 100644
--- a/drivers/gpu/drm/tinydrm/repaper.c
+++ b/drivers/gpu/drm/tinydrm/repaper.c
@@ -30,6 +30,7 @@
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_rect.h>
 #include <drm/tinydrm/tinydrm.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
 
@@ -532,7 +533,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,
 	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
 	struct tinydrm_device *tdev = fb->dev->dev_private;
 	struct repaper_epd *epd = epd_from_tinydrm(tdev);
-	struct drm_clip_rect clip;
+	struct drm_rect clip;
 	u8 *buf = NULL;
 	int ret = 0;
 
diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
index bf518167760a..a52bc3b02606 100644
--- a/drivers/gpu/drm/tinydrm/st7586.c
+++ b/drivers/gpu/drm/tinydrm/st7586.c
@@ -21,6 +21,7 @@
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_rect.h>
 #include <drm/tinydrm/mipi-dbi.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
 
@@ -62,7 +63,7 @@ static const u8 st7586_lookup[] = { 0x7, 0x4, 0x2, 0x0 };
 
 static void st7586_xrgb8888_to_gray332(u8 *dst, void *vaddr,
 				       struct drm_framebuffer *fb,
-				       struct drm_clip_rect *clip)
+				       struct drm_rect *clip)
 {
 	size_t len = (clip->x2 - clip->x1) * (clip->y2 - clip->y1);
 	unsigned int x, y;
@@ -88,7 +89,7 @@ static void st7586_xrgb8888_to_gray332(u8 *dst, void *vaddr,
 }
 
 static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb,
-			   struct drm_clip_rect *clip)
+			   struct drm_rect *clip)
 {
 	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
 	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
@@ -118,7 +119,8 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb,
 {
 	struct tinydrm_device *tdev = fb->dev->dev_private;
 	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
-	struct drm_clip_rect clip;
+	struct drm_rect clip;
+	struct drm_rect *rect = &clip;
 	int start, end;
 	int ret = 0;
 
@@ -129,30 +131,29 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb,
 			    fb->height);
 
 	/* 3 pixels per byte, so grow clip to nearest multiple of 3 */
-	clip.x1 = rounddown(clip.x1, 3);
-	clip.x2 = roundup(clip.x2, 3);
+	rect->x1 = rounddown(rect->x1, 3);
+	rect->x2 = roundup(rect->x2, 3);
 
-	DRM_DEBUG("Flushing [FB:%d] x1=%u, x2=%u, y1=%u, y2=%u\n", fb->base.id,
-		  clip.x1, clip.x2, clip.y1, clip.y2);
+	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
 
-	ret = st7586_buf_copy(mipi->tx_buf, fb, &clip);
+	ret = st7586_buf_copy(mipi->tx_buf, fb, rect);
 	if (ret)
 		return ret;
 
 	/* Pixels are packed 3 per byte */
-	start = clip.x1 / 3;
-	end = clip.x2 / 3;
+	start = rect->x1 / 3;
+	end = rect->x2 / 3;
 
 	mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS,
 			 (start >> 8) & 0xFF, start & 0xFF,
 			 (end >> 8) & 0xFF, (end - 1) & 0xFF);
 	mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS,
-			 (clip.y1 >> 8) & 0xFF, clip.y1 & 0xFF,
-			 (clip.y2 >> 8) & 0xFF, (clip.y2 - 1) & 0xFF);
+			 (rect->y1 >> 8) & 0xFF, rect->y1 & 0xFF,
+			 (rect->y2 >> 8) & 0xFF, (rect->y2 - 1) & 0xFF);
 
 	ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START,
 				   (u8 *)mipi->tx_buf,
-				   (end - start) * (clip.y2 - clip.y1));
+				   (end - start) * (rect->y2 - rect->y1));
 
 	return ret;
 }
diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
index b8ba58861986..b52f32897f23 100644
--- a/include/drm/tinydrm/mipi-dbi.h
+++ b/include/drm/tinydrm/mipi-dbi.h
@@ -14,6 +14,7 @@
 
 #include <drm/tinydrm/tinydrm.h>
 
+struct drm_rect;
 struct spi_device;
 struct gpio_desc;
 struct regulator;
@@ -80,7 +81,7 @@ u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
 int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val);
 int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
 int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
-		      struct drm_clip_rect *clip, bool swap);
+		      struct drm_rect *clip, bool swap);
 /**
  * mipi_dbi_command - MIPI DCS command with optional parameter(s)
  * @mipi: MIPI structure
diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
index 5b96f0b12c8c..8edb75df4e31 100644
--- a/include/drm/tinydrm/tinydrm-helpers.h
+++ b/include/drm/tinydrm/tinydrm-helpers.h
@@ -13,6 +13,7 @@
 struct backlight_device;
 struct tinydrm_device;
 struct drm_clip_rect;
+struct drm_rect;
 struct spi_transfer;
 struct spi_message;
 struct spi_device;
@@ -33,7 +34,7 @@ static inline bool tinydrm_machine_little_endian(void)
 #endif
 }
 
-bool tinydrm_merge_clips(struct drm_clip_rect *dst,
+bool tinydrm_merge_clips(struct drm_rect *dst,
 			 struct drm_clip_rect *src, unsigned int num_clips,
 			 unsigned int flags, u32 max_width, u32 max_height);
 int tinydrm_fb_dirty(struct drm_framebuffer *fb,
@@ -42,14 +43,14 @@ int tinydrm_fb_dirty(struct drm_framebuffer *fb,
 		     struct drm_clip_rect *clips,
 		     unsigned int num_clips);
 void tinydrm_memcpy(void *dst, void *vaddr, struct drm_framebuffer *fb,
-		    struct drm_clip_rect *clip);
+		    struct drm_rect *clip);
 void tinydrm_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb,
-		    struct drm_clip_rect *clip);
+		    struct drm_rect *clip);
 void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
 				struct drm_framebuffer *fb,
-				struct drm_clip_rect *clip, bool swap);
+				struct drm_rect *clip, bool swap);
 void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
-			       struct drm_clip_rect *clip);
+			       struct drm_rect *clip);
 
 size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
 bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-11 20:12 [PATCH v2 0/5] drm/tinydrm: Use damage helper for dirtyfb Noralf Trønnes
                   ` (2 preceding siblings ...)
  2019-01-11 20:12 ` [PATCH v2 3/5] drm/tinydrm: Use struct drm_rect Noralf Trønnes
@ 2019-01-11 20:12 ` Noralf Trønnes
  2019-01-13 21:19   ` David Lechner
  2019-01-14 22:06   ` David Lechner
  2019-01-11 20:12 ` [PATCH v2 5/5] drm/todo: Tick off some tinydrm entries Noralf Trønnes
  2019-01-12 21:43 ` [PATCH v2 0/5] drm/tinydrm: Use damage helper for dirtyfb Sam Ravnborg
  5 siblings, 2 replies; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-11 20:12 UTC (permalink / raw)
  To: dri-devel; +Cc: david, Daniel Vetter, drawat, sam

This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty
handler. All flushing will now happen in the pipe functions.

Also enable the damage plane property for all except repaper which can
only do full updates.

ili9225:
This change made ili9225_init() equal to mipi_dbi_init() so use it.

v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL
    (kbuild test robot)

Cc: David Lechner <david@lechnology.com>
Cc: Eric Anholt <eric@anholt.net>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/tinydrm/core/tinydrm-core.c   |  21 +---
 .../gpu/drm/tinydrm/core/tinydrm-helpers.c    |  91 +---------------
 drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c   |  30 ------
 drivers/gpu/drm/tinydrm/hx8357d.c             |   2 +-
 drivers/gpu/drm/tinydrm/ili9225.c             | 101 ++++++------------
 drivers/gpu/drm/tinydrm/ili9341.c             |   2 +-
 drivers/gpu/drm/tinydrm/mi0283qt.c            |   2 +-
 drivers/gpu/drm/tinydrm/mipi-dbi.c            |  74 ++++++++-----
 drivers/gpu/drm/tinydrm/repaper.c             |  39 ++++---
 drivers/gpu/drm/tinydrm/st7586.c              |  50 +++++----
 drivers/gpu/drm/tinydrm/st7735r.c             |   2 +-
 include/drm/tinydrm/mipi-dbi.h                |   2 +
 include/drm/tinydrm/tinydrm-helpers.h         |  11 +-
 include/drm/tinydrm/tinydrm.h                 |  26 -----
 14 files changed, 138 insertions(+), 315 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
index aeb93eadb047..614f532ea89f 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
@@ -39,31 +39,17 @@
  * and registers the DRM device using devm_tinydrm_register().
  */
 
-static struct drm_framebuffer *
-tinydrm_fb_create(struct drm_device *drm, struct drm_file *file_priv,
-		  const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	struct tinydrm_device *tdev = drm->dev_private;
-
-	return drm_gem_fb_create_with_funcs(drm, file_priv, mode_cmd,
-					    tdev->fb_funcs);
-}
-
 static const struct drm_mode_config_funcs tinydrm_mode_config_funcs = {
-	.fb_create = tinydrm_fb_create,
+	.fb_create = drm_gem_fb_create_with_dirty,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
 
 static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
-			const struct drm_framebuffer_funcs *fb_funcs,
 			struct drm_driver *driver)
 {
 	struct drm_device *drm;
 
-	mutex_init(&tdev->dirty_lock);
-	tdev->fb_funcs = fb_funcs;
-
 	/*
 	 * We don't embed drm_device, because that prevent us from using
 	 * devm_kzalloc() to allocate tinydrm_device in the driver since
@@ -86,7 +72,6 @@ static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
 static void tinydrm_fini(struct tinydrm_device *tdev)
 {
 	drm_mode_config_cleanup(tdev->drm);
-	mutex_destroy(&tdev->dirty_lock);
 	tdev->drm->dev_private = NULL;
 	drm_dev_put(tdev->drm);
 }
@@ -100,7 +85,6 @@ static void devm_tinydrm_release(void *data)
  * devm_tinydrm_init - Initialize tinydrm device
  * @parent: Parent device object
  * @tdev: tinydrm device
- * @fb_funcs: Framebuffer functions
  * @driver: DRM driver
  *
  * This function initializes @tdev, the underlying DRM device and it's
@@ -111,12 +95,11 @@ static void devm_tinydrm_release(void *data)
  * Zero on success, negative error code on failure.
  */
 int devm_tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
-		      const struct drm_framebuffer_funcs *fb_funcs,
 		      struct drm_driver *driver)
 {
 	int ret;
 
-	ret = tinydrm_init(parent, tdev, fb_funcs, driver);
+	ret = tinydrm_init(parent, tdev, driver);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index d0ece6ad4a1c..2737b6fdadc8 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -17,104 +17,15 @@
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_framebuffer.h>
 #include <drm/drm_print.h>
 #include <drm/drm_rect.h>
-#include <drm/tinydrm/tinydrm.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
-#include <uapi/drm/drm.h>
 
 static unsigned int spi_max;
 module_param(spi_max, uint, 0400);
 MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size");
 
-/**
- * tinydrm_merge_clips - Merge clip rectangles
- * @dst: Destination clip rectangle
- * @src: Source clip rectangle(s)
- * @num_clips: Number of @src clip rectangles
- * @flags: Dirty fb ioctl flags
- * @max_width: Maximum width of @dst
- * @max_height: Maximum height of @dst
- *
- * This function merges @src clip rectangle(s) into @dst. If @src is NULL,
- * @max_width and @min_width is used to set a full @dst clip rectangle.
- *
- * Returns:
- * true if it's a full clip, false otherwise
- */
-bool tinydrm_merge_clips(struct drm_rect *dst,
-			 struct drm_clip_rect *src, unsigned int num_clips,
-			 unsigned int flags, u32 max_width, u32 max_height)
-{
-	unsigned int i;
-
-	if (!src || !num_clips) {
-		dst->x1 = 0;
-		dst->x2 = max_width;
-		dst->y1 = 0;
-		dst->y2 = max_height;
-		return true;
-	}
-
-	dst->x1 = ~0;
-	dst->y1 = ~0;
-	dst->x2 = 0;
-	dst->y2 = 0;
-
-	for (i = 0; i < num_clips; i++) {
-		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
-			i++;
-		dst->x1 = min_t(int, dst->x1, src[i].x1);
-		dst->x2 = max_t(int, dst->x2, src[i].x2);
-		dst->y1 = min_t(int, dst->y1, src[i].y1);
-		dst->y2 = max_t(int, dst->y2, src[i].y2);
-	}
-
-	if (dst->x2 > max_width || dst->y2 > max_height ||
-	    dst->x1 >= dst->x2 || dst->y1 >= dst->y2) {
-		DRM_DEBUG_KMS("Illegal clip: x1=%u, x2=%u, y1=%u, y2=%u\n",
-			      dst->x1, dst->x2, dst->y1, dst->y2);
-		dst->x1 = 0;
-		dst->y1 = 0;
-		dst->x2 = max_width;
-		dst->y2 = max_height;
-	}
-
-	return (dst->x2 - dst->x1) == max_width &&
-	       (dst->y2 - dst->y1) == max_height;
-}
-EXPORT_SYMBOL(tinydrm_merge_clips);
-
-int tinydrm_fb_dirty(struct drm_framebuffer *fb,
-		     struct drm_file *file_priv,
-		     unsigned int flags, unsigned int color,
-		     struct drm_clip_rect *clips,
-		     unsigned int num_clips)
-{
-	struct tinydrm_device *tdev = fb->dev->dev_private;
-	struct drm_plane *plane = &tdev->pipe.plane;
-	int ret = 0;
-
-	drm_modeset_lock(&plane->mutex, NULL);
-
-	/* fbdev can flush even when we're not interested */
-	if (plane->state->fb == fb) {
-		mutex_lock(&tdev->dirty_lock);
-		ret = tdev->fb_dirty(fb, file_priv, flags,
-				     color, clips, num_clips);
-		mutex_unlock(&tdev->dirty_lock);
-	}
-
-	drm_modeset_unlock(&plane->mutex);
-
-	if (ret)
-		dev_err_once(fb->dev->dev,
-			     "Failed to update display %d\n", ret);
-
-	return ret;
-}
-EXPORT_SYMBOL(tinydrm_fb_dirty);
-
 /**
  * tinydrm_memcpy - Copy clip buffer
  * @dst: Destination buffer
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
index d4576d6e8ce4..a4796ddb08f0 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
@@ -111,36 +111,6 @@ tinydrm_connector_create(struct drm_device *drm,
 	return connector;
 }
 
-/**
- * tinydrm_display_pipe_update - Display pipe update helper
- * @pipe: Simple display pipe
- * @old_state: Old plane state
- *
- * This function does a full framebuffer flush if the plane framebuffer
- * has changed. It also handles vblank events. Drivers can use this as their
- * &drm_simple_display_pipe_funcs->update callback.
- */
-void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
-				 struct drm_plane_state *old_state)
-{
-	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
-	struct drm_framebuffer *fb = pipe->plane.state->fb;
-	struct drm_crtc *crtc = &tdev->pipe.crtc;
-
-	if (fb && (fb != old_state->fb)) {
-		if (tdev->fb_dirty)
-			tdev->fb_dirty(fb, NULL, 0, 0, NULL, 0);
-	}
-
-	if (crtc->state->event) {
-		spin_lock_irq(&crtc->dev->event_lock);
-		drm_crtc_send_vblank_event(crtc, crtc->state->event);
-		spin_unlock_irq(&crtc->dev->event_lock);
-		crtc->state->event = NULL;
-	}
-}
-EXPORT_SYMBOL(tinydrm_display_pipe_update);
-
 static int tinydrm_rotate_mode(struct drm_display_mode *mode,
 			       unsigned int rotation)
 {
diff --git a/drivers/gpu/drm/tinydrm/hx8357d.c b/drivers/gpu/drm/tinydrm/hx8357d.c
index 3ae11aa4b73b..8bbd0beafc6a 100644
--- a/drivers/gpu/drm/tinydrm/hx8357d.c
+++ b/drivers/gpu/drm/tinydrm/hx8357d.c
@@ -176,7 +176,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
 static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
 	.enable = yx240qv29_enable,
 	.disable = mipi_dbi_pipe_disable,
-	.update = tinydrm_display_pipe_update,
+	.update = mipi_dbi_pipe_update,
 	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
index ad644069089f..aa1376a22bda 100644
--- a/drivers/gpu/drm/tinydrm/ili9225.c
+++ b/drivers/gpu/drm/tinydrm/ili9225.c
@@ -20,6 +20,7 @@
 #include <linux/spi/spi.h>
 #include <video/mipi_display.h>
 
+#include <drm/drm_damage_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_fourcc.h>
@@ -76,17 +77,14 @@ static inline int ili9225_command(struct mipi_dbi *mipi, u8 cmd, u16 data)
 	return mipi_dbi_command_buf(mipi, cmd, par, 2);
 }
 
-static int ili9225_fb_dirty(struct drm_framebuffer *fb,
-			    struct drm_file *file_priv, unsigned int flags,
-			    unsigned int color, struct drm_clip_rect *clips,
-			    unsigned int num_clips)
+static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
 {
 	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
 	struct tinydrm_device *tdev = fb->dev->dev_private;
 	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+	unsigned int height = rect->y2 - rect->y1;
+	unsigned int width = rect->x2 - rect->x1;
 	bool swap = mipi->swap_bytes;
-	struct drm_rect clip;
-	struct drm_rect *rect = &clip;
 	u16 x_start, y_start;
 	u16 x1, x2, y1, y2;
 	int ret = 0;
@@ -94,10 +92,9 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb,
 	void *tr;
 
 	if (!mipi->enabled)
-		return 0;
+		return;
 
-	full = tinydrm_merge_clips(&clip, clips, num_clips, flags,
-				   fb->width, fb->height);
+	full = width == fb->width && height == fb->height;
 
 	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
 
@@ -106,7 +103,7 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb,
 		tr = mipi->tx_buf;
 		ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, rect, swap);
 		if (ret)
-			return ret;
+			goto err_msg;
 	} else {
 		tr = cma_obj->vaddr;
 	}
@@ -155,16 +152,29 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb,
 	ili9225_command(mipi, ILI9225_RAM_ADDRESS_SET_2, y_start);
 
 	ret = mipi_dbi_command_buf(mipi, ILI9225_WRITE_DATA_TO_GRAM, tr,
-				   (rect->x2 - rect->x1) * (rect->y2 - rect->y1) * 2);
-
-	return ret;
+				   width * height * 2);
+err_msg:
+	if (ret)
+		dev_err_once(fb->dev->dev, "Failed to update display %d\n", ret);
 }
 
-static const struct drm_framebuffer_funcs ili9225_fb_funcs = {
-	.destroy	= drm_gem_fb_destroy,
-	.create_handle	= drm_gem_fb_create_handle,
-	.dirty		= tinydrm_fb_dirty,
-};
+static void ili9225_pipe_update(struct drm_simple_display_pipe *pipe,
+				struct drm_plane_state *old_state)
+{
+	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_rect rect;
+
+	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
+		ili9225_fb_dirty(state->fb, &rect);
+
+	if (crtc->state->event) {
+		spin_lock_irq(&crtc->dev->event_lock);
+		drm_crtc_send_vblank_event(crtc, crtc->state->event);
+		spin_unlock_irq(&crtc->dev->event_lock);
+		crtc->state->event = NULL;
+	}
+}
 
 static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
 				struct drm_crtc_state *crtc_state,
@@ -305,59 +315,10 @@ static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
 	return tinydrm_spi_transfer(spi, speed_hz, NULL, bpw, par, num);
 }
 
-static const u32 ili9225_formats[] = {
-	DRM_FORMAT_RGB565,
-	DRM_FORMAT_XRGB8888,
-};
-
-static int ili9225_init(struct device *dev, struct mipi_dbi *mipi,
-			const struct drm_simple_display_pipe_funcs *pipe_funcs,
-			struct drm_driver *driver,
-			const struct drm_display_mode *mode,
-			unsigned int rotation)
-{
-	size_t bufsize = mode->vdisplay * mode->hdisplay * sizeof(u16);
-	struct tinydrm_device *tdev = &mipi->tinydrm;
-	int ret;
-
-	if (!mipi->command)
-		return -EINVAL;
-
-	mutex_init(&mipi->cmdlock);
-
-	mipi->tx_buf = devm_kmalloc(dev, bufsize, GFP_KERNEL);
-	if (!mipi->tx_buf)
-		return -ENOMEM;
-
-	ret = devm_tinydrm_init(dev, tdev, &ili9225_fb_funcs, driver);
-	if (ret)
-		return ret;
-
-	tdev->fb_dirty = ili9225_fb_dirty;
-
-	ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
-					DRM_MODE_CONNECTOR_VIRTUAL,
-					ili9225_formats,
-					ARRAY_SIZE(ili9225_formats), mode,
-					rotation);
-	if (ret)
-		return ret;
-
-	tdev->drm->mode_config.preferred_depth = 16;
-	mipi->rotation = rotation;
-
-	drm_mode_config_reset(tdev->drm);
-
-	DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n",
-		      tdev->drm->mode_config.preferred_depth, rotation);
-
-	return 0;
-}
-
 static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
 	.enable		= ili9225_pipe_enable,
 	.disable	= ili9225_pipe_disable,
-	.update		= tinydrm_display_pipe_update,
+	.update		= ili9225_pipe_update,
 	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
@@ -424,8 +385,8 @@ static int ili9225_probe(struct spi_device *spi)
 	/* override the command function set in  mipi_dbi_spi_init() */
 	mipi->command = ili9225_dbi_command;
 
-	ret = ili9225_init(&spi->dev, mipi, &ili9225_pipe_funcs,
-			   &ili9225_driver, &ili9225_mode, rotation);
+	ret = mipi_dbi_init(&spi->dev, mipi, &ili9225_pipe_funcs,
+			    &ili9225_driver, &ili9225_mode, rotation);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/tinydrm/ili9341.c b/drivers/gpu/drm/tinydrm/ili9341.c
index bcdf10906ade..713bb2dd7e04 100644
--- a/drivers/gpu/drm/tinydrm/ili9341.c
+++ b/drivers/gpu/drm/tinydrm/ili9341.c
@@ -132,7 +132,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
 static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
 	.enable = yx240qv29_enable,
 	.disable = mipi_dbi_pipe_disable,
-	.update = tinydrm_display_pipe_update,
+	.update = mipi_dbi_pipe_update,
 	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 97805ca37a04..82a92ec9ae3c 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -140,7 +140,7 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,
 static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
 	.enable = mi0283qt_enable,
 	.disable = mipi_dbi_pipe_disable,
-	.update = tinydrm_display_pipe_update,
+	.update = mipi_dbi_pipe_update,
 	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index 1e7e8cfc99f0..680692a83f94 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -17,6 +17,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 
+#include <drm/drm_damage_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
@@ -25,7 +26,6 @@
 #include <drm/drm_rect.h>
 #include <drm/tinydrm/mipi-dbi.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
-#include <uapi/drm/drm.h>
 #include <video/mipi_display.h>
 
 #define MIPI_DBI_MAX_SPI_READ_SPEED 2000000 /* 2MHz */
@@ -212,27 +212,22 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL(mipi_dbi_buf_copy);
 
-static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
-			     struct drm_file *file_priv,
-			     unsigned int flags, unsigned int color,
-			     struct drm_clip_rect *clips,
-			     unsigned int num_clips)
+static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
 {
 	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
 	struct tinydrm_device *tdev = fb->dev->dev_private;
 	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+	unsigned int height = rect->y2 - rect->y1;
+	unsigned int width = rect->x2 - rect->x1;
 	bool swap = mipi->swap_bytes;
-	struct drm_rect clip;
-	struct drm_rect *rect = &clip;
 	int ret = 0;
 	bool full;
 	void *tr;
 
 	if (!mipi->enabled)
-		return 0;
+		return;
 
-	full = tinydrm_merge_clips(&clip, clips, num_clips, flags,
-				   fb->width, fb->height);
+	full = width == fb->width && height == fb->height;
 
 	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
 
@@ -241,7 +236,7 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
 		tr = mipi->tx_buf;
 		ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, rect, swap);
 		if (ret)
-			return ret;
+			goto err_msg;
 	} else {
 		tr = cma_obj->vaddr;
 	}
@@ -254,16 +249,38 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
 			 ((rect->y2 - 1) >> 8) & 0xff, (rect->y2 - 1) & 0xff);
 
 	ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START, tr,
-				   (rect->x2 - rect->x1) * (rect->y2 - rect->y1) * 2);
-
-	return ret;
+				   width * height * 2);
+err_msg:
+	if (ret)
+		dev_err_once(fb->dev->dev, "Failed to update display %d\n", ret);
 }
 
-static const struct drm_framebuffer_funcs mipi_dbi_fb_funcs = {
-	.destroy	= drm_gem_fb_destroy,
-	.create_handle	= drm_gem_fb_create_handle,
-	.dirty		= tinydrm_fb_dirty,
-};
+/**
+ * mipi_dbi_pipe_update - Display pipe update helper
+ * @pipe: Simple display pipe
+ * @old_state: Old plane state
+ *
+ * This function handles framebuffer flushing and vblank events. Drivers can use
+ * this as their &drm_simple_display_pipe_funcs->update callback.
+ */
+void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
+			  struct drm_plane_state *old_state)
+{
+	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_rect rect;
+
+	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
+		mipi_dbi_fb_dirty(state->fb, &rect);
+
+	if (crtc->state->event) {
+		spin_lock_irq(&crtc->dev->event_lock);
+		drm_crtc_send_vblank_event(crtc, crtc->state->event);
+		spin_unlock_irq(&crtc->dev->event_lock);
+		crtc->state->event = NULL;
+	}
+}
+EXPORT_SYMBOL(mipi_dbi_pipe_update);
 
 /**
  * mipi_dbi_enable_flush - MIPI DBI enable helper
@@ -279,13 +296,16 @@ void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
 			   struct drm_crtc_state *crtc_state,
 			   struct drm_plane_state *plane_state)
 {
-	struct tinydrm_device *tdev = &mipi->tinydrm;
 	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_rect rect = {
+		.x1 = 0,
+		.x2 = fb->width,
+		.y1 = 0,
+		.y2 = fb->height,
+	};
 
 	mipi->enabled = true;
-	if (fb)
-		tdev->fb_dirty(fb, NULL, 0, 0, NULL, 0);
-
+	mipi_dbi_fb_dirty(fb, &rect);
 	backlight_enable(mipi->backlight);
 }
 EXPORT_SYMBOL(mipi_dbi_enable_flush);
@@ -377,12 +397,10 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
 	if (!mipi->tx_buf)
 		return -ENOMEM;
 
-	ret = devm_tinydrm_init(dev, tdev, &mipi_dbi_fb_funcs, driver);
+	ret = devm_tinydrm_init(dev, tdev, driver);
 	if (ret)
 		return ret;
 
-	tdev->fb_dirty = mipi_dbi_fb_dirty;
-
 	/* TODO: Maybe add DRM_MODE_CONNECTOR_SPI */
 	ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
 					DRM_MODE_CONNECTOR_VIRTUAL,
@@ -392,6 +410,8 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
 	if (ret)
 		return ret;
 
+	drm_plane_enable_fb_damage_clips(&tdev->pipe.plane);
+
 	tdev->drm->mode_config.preferred_depth = 16;
 	mipi->rotation = rotation;
 
diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c
index 238515de449e..f9cacf49f16b 100644
--- a/drivers/gpu/drm/tinydrm/repaper.c
+++ b/drivers/gpu/drm/tinydrm/repaper.c
@@ -26,6 +26,7 @@
 #include <linux/spi/spi.h>
 #include <linux/thermal.h>
 
+#include <drm/drm_damage_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
@@ -523,11 +524,7 @@ static void repaper_gray8_to_mono_reversed(u8 *buf, u32 width, u32 height)
 		}
 }
 
-static int repaper_fb_dirty(struct drm_framebuffer *fb,
-			    struct drm_file *file_priv,
-			    unsigned int flags, unsigned int color,
-			    struct drm_clip_rect *clips,
-			    unsigned int num_clips)
+static int repaper_fb_dirty(struct drm_framebuffer *fb)
 {
 	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
 	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
@@ -626,12 +623,6 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,
 	return ret;
 }
 
-static const struct drm_framebuffer_funcs repaper_fb_funcs = {
-	.destroy	= drm_gem_fb_destroy,
-	.create_handle	= drm_gem_fb_create_handle,
-	.dirty		= tinydrm_fb_dirty,
-};
-
 static void power_off(struct repaper_epd *epd)
 {
 	/* Turn off power and all signals */
@@ -795,9 +786,7 @@ static void repaper_pipe_disable(struct drm_simple_display_pipe *pipe)
 
 	DRM_DEBUG_DRIVER("\n");
 
-	mutex_lock(&tdev->dirty_lock);
 	epd->enabled = false;
-	mutex_unlock(&tdev->dirty_lock);
 
 	/* Nothing frame */
 	for (line = 0; line < epd->height; line++)
@@ -840,10 +829,28 @@ static void repaper_pipe_disable(struct drm_simple_display_pipe *pipe)
 	power_off(epd);
 }
 
+static void repaper_pipe_update(struct drm_simple_display_pipe *pipe,
+				struct drm_plane_state *old_state)
+{
+	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_rect rect;
+
+	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
+		repaper_fb_dirty(state->fb);
+
+	if (crtc->state->event) {
+		spin_lock_irq(&crtc->dev->event_lock);
+		drm_crtc_send_vblank_event(crtc, crtc->state->event);
+		spin_unlock_irq(&crtc->dev->event_lock);
+		crtc->state->event = NULL;
+	}
+}
+
 static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
 	.enable = repaper_pipe_enable,
 	.disable = repaper_pipe_disable,
-	.update = tinydrm_display_pipe_update,
+	.update = repaper_pipe_update,
 	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
@@ -1057,12 +1064,10 @@ static int repaper_probe(struct spi_device *spi)
 
 	tdev = &epd->tinydrm;
 
-	ret = devm_tinydrm_init(dev, tdev, &repaper_fb_funcs, &repaper_driver);
+	ret = devm_tinydrm_init(dev, tdev, &repaper_driver);
 	if (ret)
 		return ret;
 
-	tdev->fb_dirty = repaper_fb_dirty;
-
 	ret = tinydrm_display_pipe_init(tdev, &repaper_pipe_funcs,
 					DRM_MODE_CONNECTOR_VIRTUAL,
 					repaper_formats,
diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
index a52bc3b02606..50e370ce5a59 100644
--- a/drivers/gpu/drm/tinydrm/st7586.c
+++ b/drivers/gpu/drm/tinydrm/st7586.c
@@ -17,6 +17,7 @@
 #include <linux/spi/spi.h>
 #include <video/mipi_display.h>
 
+#include <drm/drm_damage_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
@@ -112,23 +113,15 @@ static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb,
 	return ret;
 }
 
-static int st7586_fb_dirty(struct drm_framebuffer *fb,
-			   struct drm_file *file_priv, unsigned int flags,
-			   unsigned int color, struct drm_clip_rect *clips,
-			   unsigned int num_clips)
+static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
 {
 	struct tinydrm_device *tdev = fb->dev->dev_private;
 	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
-	struct drm_rect clip;
-	struct drm_rect *rect = &clip;
 	int start, end;
 	int ret = 0;
 
 	if (!mipi->enabled)
-		return 0;
-
-	tinydrm_merge_clips(&clip, clips, num_clips, flags, fb->width,
-			    fb->height);
+		return;
 
 	/* 3 pixels per byte, so grow clip to nearest multiple of 3 */
 	rect->x1 = rounddown(rect->x1, 3);
@@ -138,7 +131,7 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb,
 
 	ret = st7586_buf_copy(mipi->tx_buf, fb, rect);
 	if (ret)
-		return ret;
+		goto err_msg;
 
 	/* Pixels are packed 3 per byte */
 	start = rect->x1 / 3;
@@ -154,15 +147,28 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb,
 	ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START,
 				   (u8 *)mipi->tx_buf,
 				   (end - start) * (rect->y2 - rect->y1));
-
-	return ret;
+err_msg:
+	if (ret)
+		dev_err_once(fb->dev->dev, "Failed to update display %d\n", ret);
 }
 
-static const struct drm_framebuffer_funcs st7586_fb_funcs = {
-	.destroy	= drm_gem_fb_destroy,
-	.create_handle	= drm_gem_fb_create_handle,
-	.dirty		= tinydrm_fb_dirty,
-};
+static void st7586_pipe_update(struct drm_simple_display_pipe *pipe,
+			       struct drm_plane_state *old_state)
+{
+	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_rect rect;
+
+	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
+		st7586_fb_dirty(state->fb, &rect);
+
+	if (crtc->state->event) {
+		spin_lock_irq(&crtc->dev->event_lock);
+		drm_crtc_send_vblank_event(crtc, crtc->state->event);
+		spin_unlock_irq(&crtc->dev->event_lock);
+		crtc->state->event = NULL;
+	}
+}
 
 static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
 			       struct drm_crtc_state *crtc_state,
@@ -264,12 +270,10 @@ static int st7586_init(struct device *dev, struct mipi_dbi *mipi,
 	if (!mipi->tx_buf)
 		return -ENOMEM;
 
-	ret = devm_tinydrm_init(dev, tdev, &st7586_fb_funcs, driver);
+	ret = devm_tinydrm_init(dev, tdev, driver);
 	if (ret)
 		return ret;
 
-	tdev->fb_dirty = st7586_fb_dirty;
-
 	ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
 					DRM_MODE_CONNECTOR_VIRTUAL,
 					st7586_formats,
@@ -278,6 +282,8 @@ static int st7586_init(struct device *dev, struct mipi_dbi *mipi,
 	if (ret)
 		return ret;
 
+	drm_plane_enable_fb_damage_clips(&tdev->pipe.plane);
+
 	tdev->drm->mode_config.preferred_depth = 32;
 	mipi->rotation = rotation;
 
@@ -292,7 +298,7 @@ static int st7586_init(struct device *dev, struct mipi_dbi *mipi,
 static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
 	.enable		= st7586_pipe_enable,
 	.disable	= st7586_pipe_disable,
-	.update		= tinydrm_display_pipe_update,
+	.update		= st7586_pipe_update,
 	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
index 9bc93d5a0401..3bab9a9569a6 100644
--- a/drivers/gpu/drm/tinydrm/st7735r.c
+++ b/drivers/gpu/drm/tinydrm/st7735r.c
@@ -106,7 +106,7 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe,
 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,
+	.update		= mipi_dbi_pipe_update,
 	.prepare_fb	= drm_gem_fb_simple_display_pipe_prepare_fb,
 };
 
diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
index b52f32897f23..f4ec2834bc22 100644
--- a/include/drm/tinydrm/mipi-dbi.h
+++ b/include/drm/tinydrm/mipi-dbi.h
@@ -68,6 +68,8 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
 		  const struct drm_simple_display_pipe_funcs *pipe_funcs,
 		  struct drm_driver *driver,
 		  const struct drm_display_mode *mode, unsigned int rotation);
+void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
+			  struct drm_plane_state *old_state);
 void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
 			   struct drm_crtc_state *crtc_state,
 			   struct drm_plane_state *plan_state);
diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
index 8edb75df4e31..f0d598789e4d 100644
--- a/include/drm/tinydrm/tinydrm-helpers.h
+++ b/include/drm/tinydrm/tinydrm-helpers.h
@@ -11,8 +11,7 @@
 #define __LINUX_TINYDRM_HELPERS_H
 
 struct backlight_device;
-struct tinydrm_device;
-struct drm_clip_rect;
+struct drm_framebuffer;
 struct drm_rect;
 struct spi_transfer;
 struct spi_message;
@@ -34,14 +33,6 @@ static inline bool tinydrm_machine_little_endian(void)
 #endif
 }
 
-bool tinydrm_merge_clips(struct drm_rect *dst,
-			 struct drm_clip_rect *src, unsigned int num_clips,
-			 unsigned int flags, u32 max_width, u32 max_height);
-int tinydrm_fb_dirty(struct drm_framebuffer *fb,
-		     struct drm_file *file_priv,
-		     unsigned int flags, unsigned int color,
-		     struct drm_clip_rect *clips,
-		     unsigned int num_clips);
 void tinydrm_memcpy(void *dst, void *vaddr, struct drm_framebuffer *fb,
 		    struct drm_rect *clip);
 void tinydrm_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb,
diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h
index 448aa5ea4722..5621688edcc0 100644
--- a/include/drm/tinydrm/tinydrm.h
+++ b/include/drm/tinydrm/tinydrm.h
@@ -10,14 +10,9 @@
 #ifndef __LINUX_TINYDRM_H
 #define __LINUX_TINYDRM_H
 
-#include <linux/mutex.h>
 #include <drm/drm_simple_kms_helper.h>
 
-struct drm_clip_rect;
 struct drm_driver;
-struct drm_file;
-struct drm_framebuffer;
-struct drm_framebuffer_funcs;
 
 /**
  * struct tinydrm_device - tinydrm device
@@ -32,24 +27,6 @@ struct tinydrm_device {
 	 * @pipe: Display pipe structure
 	 */
 	struct drm_simple_display_pipe pipe;
-
-	/**
-	 * @dirty_lock: Serializes framebuffer flushing
-	 */
-	struct mutex dirty_lock;
-
-	/**
-	 * @fb_funcs: Framebuffer functions used when creating framebuffers
-	 */
-	const struct drm_framebuffer_funcs *fb_funcs;
-
-	/**
-	 * @fb_dirty: Framebuffer dirty callback
-	 */
-	int (*fb_dirty)(struct drm_framebuffer *framebuffer,
-			struct drm_file *file_priv, unsigned flags,
-			unsigned color, struct drm_clip_rect *clips,
-			unsigned num_clips);
 };
 
 static inline struct tinydrm_device *
@@ -82,13 +59,10 @@ pipe_to_tinydrm(struct drm_simple_display_pipe *pipe)
 	.clock = 1 /* pass validation */
 
 int devm_tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
-		      const struct drm_framebuffer_funcs *fb_funcs,
 		      struct drm_driver *driver);
 int devm_tinydrm_register(struct tinydrm_device *tdev);
 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_init(struct tinydrm_device *tdev,
 			  const struct drm_simple_display_pipe_funcs *funcs,
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH v2 5/5] drm/todo: Tick off some tinydrm entries
  2019-01-11 20:12 [PATCH v2 0/5] drm/tinydrm: Use damage helper for dirtyfb Noralf Trønnes
                   ` (3 preceding siblings ...)
  2019-01-11 20:12 ` [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb Noralf Trønnes
@ 2019-01-11 20:12 ` Noralf Trønnes
  2019-01-12 21:43 ` [PATCH v2 0/5] drm/tinydrm: Use damage helper for dirtyfb Sam Ravnborg
  5 siblings, 0 replies; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-11 20:12 UTC (permalink / raw)
  To: dri-devel; +Cc: drawat, sam, david

- Better manual-upload support for atomic
  The new damage helper has the necessary pieces to make this work.

- tinydrm_gem_cma_prime_import_sg_table
  This is now moved to the CMA helper and can be set using the
  DRM_GEM_CMA_VMAP_DRIVER_OPS macro.

- tinydrm_fb_create
  This is now covered by drm_gem_fb_create_with_dirty()

Cc: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 Documentation/gpu/todo.rst | 35 -----------------------------------
 1 file changed, 35 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 0a85dad876ae..38360ede1221 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -82,30 +82,6 @@ events for atomic commits correctly. But fixing these bugs is good anyway.
 
 Contact: Daniel Vetter, respective driver maintainers
 
-Better manual-upload support for atomic
----------------------------------------
-
-This would be especially useful for tinydrm:
-
-- Add a struct drm_rect dirty_clip to drm_crtc_state. When duplicating the
-  crtc state, clear that to the max values, x/y = 0 and w/h = MAX_INT, in
-  __drm_atomic_helper_crtc_duplicate_state().
-
-- Move tinydrm_merge_clips into drm_framebuffer.c, dropping the tinydrm\_
-  prefix ofc and using drm_fb\_. drm_framebuffer.c makes sense since this
-  is a function useful to implement the fb->dirty function.
-
-- Create a new drm_fb_dirty function which does essentially what e.g.
-  mipi_dbi_fb_dirty does. You can use e.g. drm_atomic_helper_update_plane as the
-  template. But instead of doing a simple full-screen plane update, this new
-  helper also sets crtc_state->dirty_clip to the right coordinates. And of
-  course it needs to check whether the fb is actually active (and maybe where),
-  so there's some book-keeping involved. There's also some good fun involved in
-  scaling things appropriately. For that case we might simply give up and
-  declare the entire area covered by the plane as dirty.
-
-Contact: Noralf Trønnes, Daniel Vetter
-
 Fallout from atomic KMS
 -----------------------
 
@@ -459,21 +435,10 @@ those drivers as simple as possible, so lots of room for refactoring:
   one of the ideas for having a shared dsi/dbi helper, abstracting away the
   transport details more.
 
-- tinydrm_gem_cma_prime_import_sg_table should probably go into the cma
-  helpers, as a _vmapped variant (since not every driver needs the vmap).
-  And tinydrm_gem_cma_free_object could the be merged into
-  drm_gem_cma_free_object().
-
-- tinydrm_fb_create we could move into drm_simple_pipe, only need to add
-  the fb_create hook to drm_simple_pipe_funcs, which would again simplify a
-  bunch of things (since it gives you a one-stop vfunc for simple drivers).
-
 - Quick aside: The unregister devm stuff is kinda getting the lifetimes of
   a drm_device wrong. Doesn't matter, since everyone else gets it wrong
   too :-)
 
-- also rework the drm_framebuffer_funcs->dirty hook wire-up, see above.
-
 Contact: Noralf Trønnes, Daniel Vetter
 
 AMD DC Display Driver
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-11 20:12 [PATCH v2 0/5] drm/tinydrm: Use damage helper for dirtyfb Noralf Trønnes
                   ` (4 preceding siblings ...)
  2019-01-11 20:12 ` [PATCH v2 5/5] drm/todo: Tick off some tinydrm entries Noralf Trønnes
@ 2019-01-12 21:43 ` Sam Ravnborg
  5 siblings, 0 replies; 22+ messages in thread
From: Sam Ravnborg @ 2019-01-12 21:43 UTC (permalink / raw)
  To: Noralf Trønnes; +Cc: drawat, david, dri-devel

Hi Noralf.

On Fri, Jan 11, 2019 at 09:12:01PM +0100, Noralf Trønnes wrote:
> Changes:
> - Improve drm_gem_fb_create_with_dirty() docs (Daniel)
> - Remove unnecessary clearing of damage clips
> - Rebase on Sam's drmP.h series
> - Remove fb NULL check in mipi_dbi_enable_flush() (kbuild test robot)
> - Update todo.rst (Sam)
> 
> Maxime has done a backmerge so the damage helper is now in
> drm-misc-next.

I applied this set on top of drm-misc-next + my drmP.h removal patches.
To fix the build I had to add a few includes see below.

Other than that it looked good.
I was about to complain about "Use struct drm_rect" patch,
but looking at patch "Use damage helper for dirtyfb" it all made
sense again.

The full series can get my:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

But please consider applying the fixes below to avoid breaking
the build due to the drmP.h crunch.

I also removed drm_vblank.h from tinydrm-pipe.c as it
is no longer used - also part of patch below.

	Sam

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
index a4796ddb08f0..323564329535 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
@@ -13,7 +13,6 @@
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_modes.h>
 #include <drm/drm_print.h>
-#include <drm/drm_vblank.h>
 #include <drm/tinydrm/tinydrm.h>
 
 struct tinydrm_connector {
diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
index aa1376a22bda..8b118b476301 100644
--- a/drivers/gpu/drm/tinydrm/ili9225.c
+++ b/drivers/gpu/drm/tinydrm/ili9225.c
@@ -27,6 +27,7 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_rect.h>
+#include <drm/drm_vblank.h>
 #include <drm/tinydrm/mipi-dbi.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
 
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index 680692a83f94..8604db734756 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -24,6 +24,7 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_rect.h>
+#include <drm/drm_vblank.h>
 #include <drm/tinydrm/mipi-dbi.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
 #include <video/mipi_display.h>
diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c
index f9cacf49f16b..b037c6540cf3 100644
--- a/drivers/gpu/drm/tinydrm/repaper.c
+++ b/drivers/gpu/drm/tinydrm/repaper.c
@@ -32,6 +32,7 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_rect.h>
+#include <drm/drm_vblank.h>
 #include <drm/tinydrm/tinydrm.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
 
diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
index 50e370ce5a59..d39417b74e8b 100644
--- a/drivers/gpu/drm/tinydrm/st7586.c
+++ b/drivers/gpu/drm/tinydrm/st7586.c
@@ -23,6 +23,7 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_rect.h>
+#include <drm/drm_vblank.h>
 #include <drm/tinydrm/mipi-dbi.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-11 20:12 ` [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb Noralf Trønnes
@ 2019-01-13 21:19   ` David Lechner
  2019-01-14  2:15     ` David Lechner
  2019-01-14 22:06   ` David Lechner
  1 sibling, 1 reply; 22+ messages in thread
From: David Lechner @ 2019-01-13 21:19 UTC (permalink / raw)
  To: Noralf Trønnes, dri-devel; +Cc: drawat, sam, Daniel Vetter

On 1/11/19 2:12 PM, Noralf Trønnes wrote:
> This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty
> handler. All flushing will now happen in the pipe functions.
> 
> Also enable the damage plane property for all except repaper which can
> only do full updates.
> 
> ili9225:
> This change made ili9225_init() equal to mipi_dbi_init() so use it.
> 
> v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL
>      (kbuild test robot)
> 
> Cc: David Lechner <david@lechnology.com>
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---

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

Tested with ST7586 on LEGO MINDSTORMS EV3 and ILI9225 on
BeagleBone Green. (FWIW, I encounter other issues that had to
be fixed to make things work on both of these, but none were
actually related to this series.)

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-13 21:19   ` David Lechner
@ 2019-01-14  2:15     ` David Lechner
  2019-01-14 15:00       ` Noralf Trønnes
  0 siblings, 1 reply; 22+ messages in thread
From: David Lechner @ 2019-01-14  2:15 UTC (permalink / raw)
  To: Noralf Trønnes, dri-devel; +Cc: drawat, sam, Daniel Vetter

On 1/13/19 3:19 PM, David Lechner wrote:
> On 1/11/19 2:12 PM, Noralf Trønnes wrote:
>> This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty
>> handler. All flushing will now happen in the pipe functions.
>>
>> Also enable the damage plane property for all except repaper which can
>> only do full updates.
>>
>> ili9225:
>> This change made ili9225_init() equal to mipi_dbi_init() so use it.
>>
>> v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL
>>      (kbuild test robot)
>>
>> Cc: David Lechner <david@lechnology.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> Acked-by: Sam Ravnborg <sam@ravnborg.org>
>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
> 
> Tested-by: David Lechner <david@lechnology.com>
> Reviewed-by: David Lechner <david@lechnology.com>
> 
> Tested with ST7586 on LEGO MINDSTORMS EV3 and ILI9225 on
> BeagleBone Green. (FWIW, I encounter other issues that had to
> be fixed to make things work on both of these, but none were
> actually related to this series.)
> 

Actually, I have to take this back. It appears that the problems
that I had on LEGO MINDSTORMS EV3 are actually caused by this
series. I did a git bisect to be sure and ended up with this
patch as the bad commit. I'm guessing that the damage helper
must be causing some kind of memory corruption. Here is the
crash I am getting:

Unable to handle kernel NULL pointer dereference at virtual address 00000004
pgd = (ptrval)
[00000004] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 5.0.0-rc1-00125-ge9acadba5409 #1389
Hardware name: Generic DA850/OMAP-L138/AM18x
PC is at rb_insert_color+0x1c/0x1a4
LR is at kernfs_link_sibling+0x94/0xcc
pc : [<c04b95ec>]    lr : [<c014bfdc>]    psr: 60000013
sp : c2831b38  ip : 00000000  fp : c06b762c
r10: 00000000  r9 : c06b835c  r8 : 00000000
r7 : c2963f00  r6 : c066b028  r5 : c2016cc0  r4 : 00000000
r3 : 00000000  r2 : c2983010  r1 : c2963f2c  r0 : c2016cd0
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 0005317f  Table: c0004000  DAC: 00000053
Process swapper (pid: 1, stack limit = 0x(ptrval))
Stack: (0xc2831b38 to 0xc2832000)
1b20:                                                       00000000 c2016cc0
1b40: c066b028 c014bfdc c2016cc0 c2963f00 c066b028 c014c768 c2963f00 c014c448
1b60: 00000000 c2016cc0 c2963f00 c066b028 c2963f00 c014c860 00000000 00000001
1b80: c0589938 c2b4b408 00000000 c014ec70 00000000 c2b4b408 00000000 c04c4cb0
1ba0: 0000070f 00000000 00000000 9fd04dd9 00000000 c2b4b408 c066b028 00000000
1bc0: c293ac98 c04b58f8 c059081c 00000000 c2b4b408 c066b028 00000000 00000000
1be0: 00000000 c04b5d64 c2831c08 9fd04dd9 c2b4b3c0 c293ac80 c2bd16c0 c2b4b408
1c00: c00d650c c059081c c2bd16c0 c28e3a80 c2b4b3c0 00000000 c2b4b3c0 00000000
1c20: c06b7634 00020000 c06b835c c00d72f8 00000000 c00b0c24 00000000 00000074
1c40: c0590728 00000000 c0590728 c2b4b3c0 00000074 c0590728 00020000 00000000
1c60: c06b762c c00b0958 00000000 00380bc6 00000000 c06bbf1c c2bd9c00 c2bfe000
1c80: c06bbf14 0000000c 00000000 00000000 c2831e0c c00b0a90 00000000 00000000
1ca0: 00000000 00000000 003b2580 c017d6e4 00000000 00000000 00000000 c2bfe000
1cc0: c2bd9c00 00000000 003b2580 00000000 00000000 c01971a0 00001103 00000000
1ce0: c2bd8400 00000400 00000400 9fd04dd9 00000000 0000000a 00000002 00000000
1d00: ffffffff 00000000 ffffffff 00000000 00000000 00000001 00000a04 00000032
1d20: 00000000 0000000c 00000004 c00af550 c2bd9ecc 9fd04dd9 c2404480 c2bd9eb4
1d40: c2bd8400 c2404480 00000002 00000001 00000000 00000000 00000001 00000000
1d60: 00000000 00000000 00000000 00000000 00000000 00000000 00000001 00000000
1d80: 00380bc6 00000000 003b257f 00000000 00000077 0000ffff 00000200 00000002
1da0: 00000001 0000ffff 00000000 00000401 c2bfe22c 00000000 00000000 00000000
1dc0: 00000000 00000000 c2012400 c24be100 00001000 00000000 c2404480 00000000
1de0: 00004003 9fd04dd9 00000000 c2bd9c00 c2404480 00000083 c24044fc 00008000
1e00: 00000000 00000020 00008000 c00e4d88 c2404480 00000000 00000000 c0190afc
1e20: c2bd0be0 c0692898 c0692898 00000000 00000020 c0190b10 c0194f48 c2bd0be0
1e40: c066b370 c00e568c c29f5380 c01002d4 c29f5380 c2bd0be0 00008000 c0100488
1e60: c0692898 00000000 c066b028 c2bd0be0 c2bd0bc0 c01033ec 00000000 ffffff00
1e80: ffff0a00 c0575ff4 c2bd0be0 c281a010 c2417098 c2bd0be0 0000000a 00000000
1ea0: 0000000a 9fd04dd9 0000000a c2bd0be0 c2bd0bc0 00000000 c0575ff8 00008000
1ec0: c066b028 00008000 c0575ff4 c0104178 00000000 00000000 c2014000 c2014005
1ee0: c0575ff8 c3fb1280 c0652868 c062b1ec 00000000 c01013d8 c24c3558 00000000
1f00: 00000000 00006180 c24c3558 c00f17c4 e10c76ac 0b300002 c2858015 c281a010
1f20: c2415da8 9fd04dd9 00000000 00000002 c06ad310 c066b028 c066da68 00000000
1f40: 00000000 00000000 00000000 c062b478 00000000 c0037200 00306b6c 9fd00000
1f60: c0576098 9fd04dd9 00000002 c06ad310 c0652878 00000000 00000000 00000000
1f80: 00000000 00000000 00000000 c062b5e4 00000000 00000000 00000000 00000000
1fa0: c04c7860 c04c7868 00000000 c00090e0 00000000 00000000 00000000 00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[<c04b95ec>] (rb_insert_color) from [<c014bfdc>] (kernfs_link_sibling+0x94/0xcc)
[<c014bfdc>] (kernfs_link_sibling) from [<c014c768>] (kernfs_add_one+0x90/0x140)
[<c014c768>] (kernfs_add_one) from [<c014c860>] (kernfs_create_dir_ns+0x48/0x74)
[<c014c860>] (kernfs_create_dir_ns) from [<c014ec70>] (sysfs_create_dir_ns+0x68/0xd0)
[<c014ec70>] (sysfs_create_dir_ns) from [<c04b58f8>] (kobject_add_internal+0x9c/0x2c4)
[<c04b58f8>] (kobject_add_internal) from [<c04b5d64>] (kobject_init_and_add+0x54/0x94)
[<c04b5d64>] (kobject_init_and_add) from [<c00d650c>] (sysfs_slab_add+0x10c/0x220)
[<c00d650c>] (sysfs_slab_add) from [<c00d72f8>] (__kmem_cache_create+0x1d8/0x338)
[<c00d72f8>] (__kmem_cache_create) from [<c00b0958>] (kmem_cache_create_usercopy+0x180/0x298)
[<c00b0958>] (kmem_cache_create_usercopy) from [<c00b0a90>] (kmem_cache_create+0x20/0x28)
[<c00b0a90>] (kmem_cache_create) from [<c017d6e4>] (ext4_mb_init+0x374/0x44c)
[<c017d6e4>] (ext4_mb_init) from [<c01971a0>] (ext4_fill_super+0x2258/0x2ef0)
[<c01971a0>] (ext4_fill_super) from [<c00e4d88>] (mount_bdev+0x154/0x18c)
[<c00e4d88>] (mount_bdev) from [<c0190b10>] (ext4_mount+0x14/0x20)
[<c0190b10>] (ext4_mount) from [<c00e568c>] (mount_fs+0x14/0xa8)
[<c00e568c>] (mount_fs) from [<c0100488>] (vfs_kern_mount+0x48/0xf0)
[<c0100488>] (vfs_kern_mount) from [<c01033ec>] (do_mount+0x180/0xb9c)
[<c01033ec>] (do_mount) from [<c0104178>] (ksys_mount+0x8c/0xb4)
[<c0104178>] (ksys_mount) from [<c062b1ec>] (mount_block_root+0x128/0x2a4)
[<c062b1ec>] (mount_block_root) from [<c062b478>] (mount_root+0x110/0x154)
[<c062b478>] (mount_root) from [<c062b5e4>] (prepare_namespace+0x128/0x188)
[<c062b5e4>] (prepare_namespace) from [<c04c7868>] (kernel_init+0x8/0xf4)
[<c04c7868>] (kernel_init) from [<c00090e0>] (ret_from_fork+0x14/0x34)
Exception stack(0xc2831fb0 to 0xc2831ff8)
1fa0:                                     00000000 00000000 00000000 00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
Code: e5923000 e3130001 1a000054 e92d4070 (e593c004)
---[ end trace 2c8fc104914a532b ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-14  2:15     ` David Lechner
@ 2019-01-14 15:00       ` Noralf Trønnes
  2019-01-14 16:13         ` Noralf Trønnes
  0 siblings, 1 reply; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-14 15:00 UTC (permalink / raw)
  To: David Lechner, dri-devel; +Cc: drawat, sam, Daniel Vetter



Den 14.01.2019 03.15, skrev David Lechner:
> On 1/13/19 3:19 PM, David Lechner wrote:
>> On 1/11/19 2:12 PM, Noralf Trønnes wrote:
>>> This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty
>>> handler. All flushing will now happen in the pipe functions.
>>>
>>> Also enable the damage plane property for all except repaper which can
>>> only do full updates.
>>>
>>> ili9225:
>>> This change made ili9225_init() equal to mipi_dbi_init() so use it.
>>>
>>> v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL
>>>      (kbuild test robot)
>>>
>>> Cc: David Lechner <david@lechnology.com>
>>> Cc: Eric Anholt <eric@anholt.net>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> Acked-by: Sam Ravnborg <sam@ravnborg.org>
>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>
>> Tested-by: David Lechner <david@lechnology.com>
>> Reviewed-by: David Lechner <david@lechnology.com>
>>
>> Tested with ST7586 on LEGO MINDSTORMS EV3 and ILI9225 on
>> BeagleBone Green. (FWIW, I encounter other issues that had to
>> be fixed to make things work on both of these, but none were
>> actually related to this series.)
>>
> 
> Actually, I have to take this back. It appears that the problems
> that I had on LEGO MINDSTORMS EV3 are actually caused by this
> series. I did a git bisect to be sure and ended up with this
> patch as the bad commit. I'm guessing that the damage helper
> must be causing some kind of memory corruption. Here is the
> crash I am getting:
> 
> Unable to handle kernel NULL pointer dereference at virtual address
> 00000004
> pgd = (ptrval)
> [00000004] *pgd=00000000
> Internal error: Oops: 5 [#1] PREEMPT ARM
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper Not tainted 5.0.0-rc1-00125-ge9acadba5409 #1389
> Hardware name: Generic DA850/OMAP-L138/AM18x
> PC is at rb_insert_color+0x1c/0x1a4
> LR is at kernfs_link_sibling+0x94/0xcc
> pc : [<c04b95ec>]    lr : [<c014bfdc>]    psr: 60000013
> sp : c2831b38  ip : 00000000  fp : c06b762c
> r10: 00000000  r9 : c06b835c  r8 : 00000000
> r7 : c2963f00  r6 : c066b028  r5 : c2016cc0  r4 : 00000000
> r3 : 00000000  r2 : c2983010  r1 : c2963f2c  r0 : c2016cd0
> Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 0005317f  Table: c0004000  DAC: 00000053
> Process swapper (pid: 1, stack limit = 0x(ptrval))
> Stack: (0xc2831b38 to 0xc2832000)
> 1b20:                                                       00000000
> c2016cc0
> 1b40: c066b028 c014bfdc c2016cc0 c2963f00 c066b028 c014c768 c2963f00
> c014c448
> 1b60: 00000000 c2016cc0 c2963f00 c066b028 c2963f00 c014c860 00000000
> 00000001
> 1b80: c0589938 c2b4b408 00000000 c014ec70 00000000 c2b4b408 00000000
> c04c4cb0
> 1ba0: 0000070f 00000000 00000000 9fd04dd9 00000000 c2b4b408 c066b028
> 00000000
> 1bc0: c293ac98 c04b58f8 c059081c 00000000 c2b4b408 c066b028 00000000
> 00000000
> 1be0: 00000000 c04b5d64 c2831c08 9fd04dd9 c2b4b3c0 c293ac80 c2bd16c0
> c2b4b408
> 1c00: c00d650c c059081c c2bd16c0 c28e3a80 c2b4b3c0 00000000 c2b4b3c0
> 00000000
> 1c20: c06b7634 00020000 c06b835c c00d72f8 00000000 c00b0c24 00000000
> 00000074
> 1c40: c0590728 00000000 c0590728 c2b4b3c0 00000074 c0590728 00020000
> 00000000
> 1c60: c06b762c c00b0958 00000000 00380bc6 00000000 c06bbf1c c2bd9c00
> c2bfe000
> 1c80: c06bbf14 0000000c 00000000 00000000 c2831e0c c00b0a90 00000000
> 00000000
> 1ca0: 00000000 00000000 003b2580 c017d6e4 00000000 00000000 00000000
> c2bfe000
> 1cc0: c2bd9c00 00000000 003b2580 00000000 00000000 c01971a0 00001103
> 00000000
> 1ce0: c2bd8400 00000400 00000400 9fd04dd9 00000000 0000000a 00000002
> 00000000
> 1d00: ffffffff 00000000 ffffffff 00000000 00000000 00000001 00000a04
> 00000032
> 1d20: 00000000 0000000c 00000004 c00af550 c2bd9ecc 9fd04dd9 c2404480
> c2bd9eb4
> 1d40: c2bd8400 c2404480 00000002 00000001 00000000 00000000 00000001
> 00000000
> 1d60: 00000000 00000000 00000000 00000000 00000000 00000000 00000001
> 00000000
> 1d80: 00380bc6 00000000 003b257f 00000000 00000077 0000ffff 00000200
> 00000002
> 1da0: 00000001 0000ffff 00000000 00000401 c2bfe22c 00000000 00000000
> 00000000
> 1dc0: 00000000 00000000 c2012400 c24be100 00001000 00000000 c2404480
> 00000000
> 1de0: 00004003 9fd04dd9 00000000 c2bd9c00 c2404480 00000083 c24044fc
> 00008000
> 1e00: 00000000 00000020 00008000 c00e4d88 c2404480 00000000 00000000
> c0190afc
> 1e20: c2bd0be0 c0692898 c0692898 00000000 00000020 c0190b10 c0194f48
> c2bd0be0
> 1e40: c066b370 c00e568c c29f5380 c01002d4 c29f5380 c2bd0be0 00008000
> c0100488
> 1e60: c0692898 00000000 c066b028 c2bd0be0 c2bd0bc0 c01033ec 00000000
> ffffff00
> 1e80: ffff0a00 c0575ff4 c2bd0be0 c281a010 c2417098 c2bd0be0 0000000a
> 00000000
> 1ea0: 0000000a 9fd04dd9 0000000a c2bd0be0 c2bd0bc0 00000000 c0575ff8
> 00008000
> 1ec0: c066b028 00008000 c0575ff4 c0104178 00000000 00000000 c2014000
> c2014005
> 1ee0: c0575ff8 c3fb1280 c0652868 c062b1ec 00000000 c01013d8 c24c3558
> 00000000
> 1f00: 00000000 00006180 c24c3558 c00f17c4 e10c76ac 0b300002 c2858015
> c281a010
> 1f20: c2415da8 9fd04dd9 00000000 00000002 c06ad310 c066b028 c066da68
> 00000000
> 1f40: 00000000 00000000 00000000 c062b478 00000000 c0037200 00306b6c
> 9fd00000
> 1f60: c0576098 9fd04dd9 00000002 c06ad310 c0652878 00000000 00000000
> 00000000
> 1f80: 00000000 00000000 00000000 c062b5e4 00000000 00000000 00000000
> 00000000
> 1fa0: c04c7860 c04c7868 00000000 c00090e0 00000000 00000000 00000000
> 00000000
> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 00000000
> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000
> 00000000
> [<c04b95ec>] (rb_insert_color) from [<c014bfdc>]
> (kernfs_link_sibling+0x94/0xcc)
> [<c014bfdc>] (kernfs_link_sibling) from [<c014c768>]
> (kernfs_add_one+0x90/0x140)
> [<c014c768>] (kernfs_add_one) from [<c014c860>]
> (kernfs_create_dir_ns+0x48/0x74)
> [<c014c860>] (kernfs_create_dir_ns) from [<c014ec70>]
> (sysfs_create_dir_ns+0x68/0xd0)
> [<c014ec70>] (sysfs_create_dir_ns) from [<c04b58f8>]
> (kobject_add_internal+0x9c/0x2c4)
> [<c04b58f8>] (kobject_add_internal) from [<c04b5d64>]
> (kobject_init_and_add+0x54/0x94)
> [<c04b5d64>] (kobject_init_and_add) from [<c00d650c>]
> (sysfs_slab_add+0x10c/0x220)
> [<c00d650c>] (sysfs_slab_add) from [<c00d72f8>]
> (__kmem_cache_create+0x1d8/0x338)
> [<c00d72f8>] (__kmem_cache_create) from [<c00b0958>]
> (kmem_cache_create_usercopy+0x180/0x298)
> [<c00b0958>] (kmem_cache_create_usercopy) from [<c00b0a90>]
> (kmem_cache_create+0x20/0x28)
> [<c00b0a90>] (kmem_cache_create) from [<c017d6e4>]
> (ext4_mb_init+0x374/0x44c)
> [<c017d6e4>] (ext4_mb_init) from [<c01971a0>]
> (ext4_fill_super+0x2258/0x2ef0)
> [<c01971a0>] (ext4_fill_super) from [<c00e4d88>] (mount_bdev+0x154/0x18c)
> [<c00e4d88>] (mount_bdev) from [<c0190b10>] (ext4_mount+0x14/0x20)
> [<c0190b10>] (ext4_mount) from [<c00e568c>] (mount_fs+0x14/0xa8)
> [<c00e568c>] (mount_fs) from [<c0100488>] (vfs_kern_mount+0x48/0xf0)
> [<c0100488>] (vfs_kern_mount) from [<c01033ec>] (do_mount+0x180/0xb9c)
> [<c01033ec>] (do_mount) from [<c0104178>] (ksys_mount+0x8c/0xb4)
> [<c0104178>] (ksys_mount) from [<c062b1ec>] (mount_block_root+0x128/0x2a4)
> [<c062b1ec>] (mount_block_root) from [<c062b478>] (mount_root+0x110/0x154)
> [<c062b478>] (mount_root) from [<c062b5e4>] (prepare_namespace+0x128/0x188)
> [<c062b5e4>] (prepare_namespace) from [<c04c7868>] (kernel_init+0x8/0xf4)
> [<c04c7868>] (kernel_init) from [<c00090e0>] (ret_from_fork+0x14/0x34)
> Exception stack(0xc2831fb0 to 0xc2831ff8)
> 1fa0:                                     00000000 00000000 00000000
> 00000000
> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 00000000
> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> Code: e5923000 e3130001 1a000054 e92d4070 (e593c004)
> ---[ end trace 2c8fc104914a532b ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> ---[ end Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x0000000b ]---
> 

First step is to test if it's the damage clip allocation/freeing that's
causing problems. Setting clips to NULL avoids allocation and results in
a full update each time:

diff --git a/drivers/gpu/drm/drm_damage_helper.c
b/drivers/gpu/drm/drm_damage_helper.c
index e16aa5ae00b4..943ae17b8b1c 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -182,6 +182,7 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer
*fb,
 	}
 	state->acquire_ctx = &ctx;

+	clips = NULL;
 	if (clips) {
 		uint32_t inc = 1;

If that helps, try clearing/freeing the damage explicitly:

diff --git a/drivers/gpu/drm/drm_damage_helper.c
b/drivers/gpu/drm/drm_damage_helper.c
index e16aa5ae00b4..0a59263e4733 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -371,6 +371,8 @@ bool drm_atomic_helper_damage_merged(const struct
drm_plane_state *old_state,
 		valid = true;
 	}

+	drm_property_replace_blob(&state->fb_damage_clips, NULL);
+
 	return valid;
 }
 EXPORT_SYMBOL(drm_atomic_helper_damage_merged);

If that doesn't help, try disabling the call to st7586_fb_dirty() so we
can rule out the driver.
If it helps, verify that the rectangle is within bounds.

Noralf.

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

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-14 15:00       ` Noralf Trønnes
@ 2019-01-14 16:13         ` Noralf Trønnes
  2019-01-14 21:50           ` David Lechner
  0 siblings, 1 reply; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-14 16:13 UTC (permalink / raw)
  To: David Lechner, dri-devel; +Cc: Daniel Vetter, drawat, sam



Den 14.01.2019 16.00, skrev Noralf Trønnes:
> 
> 
> Den 14.01.2019 03.15, skrev David Lechner:
>> On 1/13/19 3:19 PM, David Lechner wrote:
>>> On 1/11/19 2:12 PM, Noralf Trønnes wrote:
>>>> This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty
>>>> handler. All flushing will now happen in the pipe functions.
>>>>
>>>> Also enable the damage plane property for all except repaper which can
>>>> only do full updates.
>>>>
>>>> ili9225:
>>>> This change made ili9225_init() equal to mipi_dbi_init() so use it.
>>>>
>>>> v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL
>>>>      (kbuild test robot)
>>>>
>>>> Cc: David Lechner <david@lechnology.com>
>>>> Cc: Eric Anholt <eric@anholt.net>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>> Acked-by: Sam Ravnborg <sam@ravnborg.org>
>>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> ---
>>>
>>> Tested-by: David Lechner <david@lechnology.com>
>>> Reviewed-by: David Lechner <david@lechnology.com>
>>>
>>> Tested with ST7586 on LEGO MINDSTORMS EV3 and ILI9225 on
>>> BeagleBone Green. (FWIW, I encounter other issues that had to
>>> be fixed to make things work on both of these, but none were
>>> actually related to this series.)
>>>
>>
>> Actually, I have to take this back. It appears that the problems
>> that I had on LEGO MINDSTORMS EV3 are actually caused by this
>> series. I did a git bisect to be sure and ended up with this
>> patch as the bad commit. I'm guessing that the damage helper
>> must be causing some kind of memory corruption. Here is the
>> crash I am getting:
>>
>> Unable to handle kernel NULL pointer dereference at virtual address
>> 00000004
>> pgd = (ptrval)
>> [00000004] *pgd=00000000
>> Internal error: Oops: 5 [#1] PREEMPT ARM
>> Modules linked in:
>> CPU: 0 PID: 1 Comm: swapper Not tainted 5.0.0-rc1-00125-ge9acadba5409 #1389
>> Hardware name: Generic DA850/OMAP-L138/AM18x
>> PC is at rb_insert_color+0x1c/0x1a4
>> LR is at kernfs_link_sibling+0x94/0xcc
>> pc : [<c04b95ec>]    lr : [<c014bfdc>]    psr: 60000013
>> sp : c2831b38  ip : 00000000  fp : c06b762c
>> r10: 00000000  r9 : c06b835c  r8 : 00000000
>> r7 : c2963f00  r6 : c066b028  r5 : c2016cc0  r4 : 00000000
>> r3 : 00000000  r2 : c2983010  r1 : c2963f2c  r0 : c2016cd0
>> Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>> Control: 0005317f  Table: c0004000  DAC: 00000053
>> Process swapper (pid: 1, stack limit = 0x(ptrval))
>> Stack: (0xc2831b38 to 0xc2832000)
>> 1b20:                                                       00000000
>> c2016cc0
>> 1b40: c066b028 c014bfdc c2016cc0 c2963f00 c066b028 c014c768 c2963f00
>> c014c448
>> 1b60: 00000000 c2016cc0 c2963f00 c066b028 c2963f00 c014c860 00000000
>> 00000001
>> 1b80: c0589938 c2b4b408 00000000 c014ec70 00000000 c2b4b408 00000000
>> c04c4cb0
>> 1ba0: 0000070f 00000000 00000000 9fd04dd9 00000000 c2b4b408 c066b028
>> 00000000
>> 1bc0: c293ac98 c04b58f8 c059081c 00000000 c2b4b408 c066b028 00000000
>> 00000000
>> 1be0: 00000000 c04b5d64 c2831c08 9fd04dd9 c2b4b3c0 c293ac80 c2bd16c0
>> c2b4b408
>> 1c00: c00d650c c059081c c2bd16c0 c28e3a80 c2b4b3c0 00000000 c2b4b3c0
>> 00000000
>> 1c20: c06b7634 00020000 c06b835c c00d72f8 00000000 c00b0c24 00000000
>> 00000074
>> 1c40: c0590728 00000000 c0590728 c2b4b3c0 00000074 c0590728 00020000
>> 00000000
>> 1c60: c06b762c c00b0958 00000000 00380bc6 00000000 c06bbf1c c2bd9c00
>> c2bfe000
>> 1c80: c06bbf14 0000000c 00000000 00000000 c2831e0c c00b0a90 00000000
>> 00000000
>> 1ca0: 00000000 00000000 003b2580 c017d6e4 00000000 00000000 00000000
>> c2bfe000
>> 1cc0: c2bd9c00 00000000 003b2580 00000000 00000000 c01971a0 00001103
>> 00000000
>> 1ce0: c2bd8400 00000400 00000400 9fd04dd9 00000000 0000000a 00000002
>> 00000000
>> 1d00: ffffffff 00000000 ffffffff 00000000 00000000 00000001 00000a04
>> 00000032
>> 1d20: 00000000 0000000c 00000004 c00af550 c2bd9ecc 9fd04dd9 c2404480
>> c2bd9eb4
>> 1d40: c2bd8400 c2404480 00000002 00000001 00000000 00000000 00000001
>> 00000000
>> 1d60: 00000000 00000000 00000000 00000000 00000000 00000000 00000001
>> 00000000
>> 1d80: 00380bc6 00000000 003b257f 00000000 00000077 0000ffff 00000200
>> 00000002
>> 1da0: 00000001 0000ffff 00000000 00000401 c2bfe22c 00000000 00000000
>> 00000000
>> 1dc0: 00000000 00000000 c2012400 c24be100 00001000 00000000 c2404480
>> 00000000
>> 1de0: 00004003 9fd04dd9 00000000 c2bd9c00 c2404480 00000083 c24044fc
>> 00008000
>> 1e00: 00000000 00000020 00008000 c00e4d88 c2404480 00000000 00000000
>> c0190afc
>> 1e20: c2bd0be0 c0692898 c0692898 00000000 00000020 c0190b10 c0194f48
>> c2bd0be0
>> 1e40: c066b370 c00e568c c29f5380 c01002d4 c29f5380 c2bd0be0 00008000
>> c0100488
>> 1e60: c0692898 00000000 c066b028 c2bd0be0 c2bd0bc0 c01033ec 00000000
>> ffffff00
>> 1e80: ffff0a00 c0575ff4 c2bd0be0 c281a010 c2417098 c2bd0be0 0000000a
>> 00000000
>> 1ea0: 0000000a 9fd04dd9 0000000a c2bd0be0 c2bd0bc0 00000000 c0575ff8
>> 00008000
>> 1ec0: c066b028 00008000 c0575ff4 c0104178 00000000 00000000 c2014000
>> c2014005
>> 1ee0: c0575ff8 c3fb1280 c0652868 c062b1ec 00000000 c01013d8 c24c3558
>> 00000000
>> 1f00: 00000000 00006180 c24c3558 c00f17c4 e10c76ac 0b300002 c2858015
>> c281a010
>> 1f20: c2415da8 9fd04dd9 00000000 00000002 c06ad310 c066b028 c066da68
>> 00000000
>> 1f40: 00000000 00000000 00000000 c062b478 00000000 c0037200 00306b6c
>> 9fd00000
>> 1f60: c0576098 9fd04dd9 00000002 c06ad310 c0652878 00000000 00000000
>> 00000000
>> 1f80: 00000000 00000000 00000000 c062b5e4 00000000 00000000 00000000
>> 00000000
>> 1fa0: c04c7860 c04c7868 00000000 c00090e0 00000000 00000000 00000000
>> 00000000
>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> 00000000
>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000
>> 00000000
>> [<c04b95ec>] (rb_insert_color) from [<c014bfdc>]
>> (kernfs_link_sibling+0x94/0xcc)
>> [<c014bfdc>] (kernfs_link_sibling) from [<c014c768>]
>> (kernfs_add_one+0x90/0x140)
>> [<c014c768>] (kernfs_add_one) from [<c014c860>]
>> (kernfs_create_dir_ns+0x48/0x74)
>> [<c014c860>] (kernfs_create_dir_ns) from [<c014ec70>]
>> (sysfs_create_dir_ns+0x68/0xd0)
>> [<c014ec70>] (sysfs_create_dir_ns) from [<c04b58f8>]
>> (kobject_add_internal+0x9c/0x2c4)
>> [<c04b58f8>] (kobject_add_internal) from [<c04b5d64>]
>> (kobject_init_and_add+0x54/0x94)
>> [<c04b5d64>] (kobject_init_and_add) from [<c00d650c>]
>> (sysfs_slab_add+0x10c/0x220)
>> [<c00d650c>] (sysfs_slab_add) from [<c00d72f8>]
>> (__kmem_cache_create+0x1d8/0x338)
>> [<c00d72f8>] (__kmem_cache_create) from [<c00b0958>]
>> (kmem_cache_create_usercopy+0x180/0x298)
>> [<c00b0958>] (kmem_cache_create_usercopy) from [<c00b0a90>]
>> (kmem_cache_create+0x20/0x28)
>> [<c00b0a90>] (kmem_cache_create) from [<c017d6e4>]
>> (ext4_mb_init+0x374/0x44c)
>> [<c017d6e4>] (ext4_mb_init) from [<c01971a0>]
>> (ext4_fill_super+0x2258/0x2ef0)
>> [<c01971a0>] (ext4_fill_super) from [<c00e4d88>] (mount_bdev+0x154/0x18c)
>> [<c00e4d88>] (mount_bdev) from [<c0190b10>] (ext4_mount+0x14/0x20)
>> [<c0190b10>] (ext4_mount) from [<c00e568c>] (mount_fs+0x14/0xa8)
>> [<c00e568c>] (mount_fs) from [<c0100488>] (vfs_kern_mount+0x48/0xf0)
>> [<c0100488>] (vfs_kern_mount) from [<c01033ec>] (do_mount+0x180/0xb9c)
>> [<c01033ec>] (do_mount) from [<c0104178>] (ksys_mount+0x8c/0xb4)
>> [<c0104178>] (ksys_mount) from [<c062b1ec>] (mount_block_root+0x128/0x2a4)
>> [<c062b1ec>] (mount_block_root) from [<c062b478>] (mount_root+0x110/0x154)
>> [<c062b478>] (mount_root) from [<c062b5e4>] (prepare_namespace+0x128/0x188)
>> [<c062b5e4>] (prepare_namespace) from [<c04c7868>] (kernel_init+0x8/0xf4)
>> [<c04c7868>] (kernel_init) from [<c00090e0>] (ret_from_fork+0x14/0x34)
>> Exception stack(0xc2831fb0 to 0xc2831ff8)
>> 1fa0:                                     00000000 00000000 00000000
>> 00000000
>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> 00000000
>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> Code: e5923000 e3130001 1a000054 e92d4070 (e593c004)
>> ---[ end trace 2c8fc104914a532b ]---
>> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
>> ---[ end Kernel panic - not syncing: Attempted to kill init!
>> exitcode=0x0000000b ]---
>>
> 
> First step is to test if it's the damage clip allocation/freeing that's
> causing problems. Setting clips to NULL avoids allocation and results in
> a full update each time:
> 
> diff --git a/drivers/gpu/drm/drm_damage_helper.c
> b/drivers/gpu/drm/drm_damage_helper.c
> index e16aa5ae00b4..943ae17b8b1c 100644
> --- a/drivers/gpu/drm/drm_damage_helper.c
> +++ b/drivers/gpu/drm/drm_damage_helper.c
> @@ -182,6 +182,7 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer
> *fb,
>  	}
>  	state->acquire_ctx = &ctx;
> 
> +	clips = NULL;
>  	if (clips) {
>  		uint32_t inc = 1;
> 
> If that helps, try clearing/freeing the damage explicitly:
> 
> diff --git a/drivers/gpu/drm/drm_damage_helper.c
> b/drivers/gpu/drm/drm_damage_helper.c
> index e16aa5ae00b4..0a59263e4733 100644
> --- a/drivers/gpu/drm/drm_damage_helper.c
> +++ b/drivers/gpu/drm/drm_damage_helper.c
> @@ -371,6 +371,8 @@ bool drm_atomic_helper_damage_merged(const struct
> drm_plane_state *old_state,
>  		valid = true;
>  	}
> 
> +	drm_property_replace_blob(&state->fb_damage_clips, NULL);
> +
>  	return valid;
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_damage_merged);
> 
> If that doesn't help, try disabling the call to st7586_fb_dirty() so we
> can rule out the driver.
> If it helps, verify that the rectangle is within bounds.
> 

I see that you have this call chain:
st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty().

That doesn't look safe. The st7586 driver allocates a tx_buf with size:
	size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;

whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len:
fb->width * fb->height * 2

It looks like you're writing zeroes way past the end of the buffer.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-14 16:13         ` Noralf Trønnes
@ 2019-01-14 21:50           ` David Lechner
  2019-01-14 22:33             ` David Lechner
  0 siblings, 1 reply; 22+ messages in thread
From: David Lechner @ 2019-01-14 21:50 UTC (permalink / raw)
  To: Noralf Trønnes, dri-devel; +Cc: Daniel Vetter, drawat, sam

On 1/14/19 10:13 AM, Noralf Trønnes wrote:
> 
> I see that you have this call chain:
> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty().
> 
> That doesn't look safe. The st7586 driver allocates a tx_buf with size:
> 	size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
> 
> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len:
> fb->width * fb->height * 2
> 
> It looks like you're writing zeroes way past the end of the buffer.
> 
> Noralf.
> 

Thanks! That does indeed seem to be the problem. I'll put together
a patch to fix this. I'm thinking it will be easier to make the
fix before applying this series so that it will be easier to
backport.

FWIW, here is the change I made on top of this series.

diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
index 50e370ce5a59..a6cad8f1d2c9 100644
--- a/drivers/gpu/drm/tinydrm/st7586.c
+++ b/drivers/gpu/drm/tinydrm/st7586.c
@@ -176,6 +176,12 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
  {
         struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
         struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+       struct drm_rect rect = {
+               .x1 = 0,
+               .x2 = plane_state->fb->width,
+               .y1 = 0,
+               .y2 = plane_state->fb->height,
+       };
         int ret;
         u8 addr_mode;

@@ -234,7 +240,7 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,

         mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);

-       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
+       st7586_fb_dirty(plane_state->fb, &rect);
  }

  static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-11 20:12 ` [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb Noralf Trønnes
  2019-01-13 21:19   ` David Lechner
@ 2019-01-14 22:06   ` David Lechner
  2019-01-14 22:46     ` Noralf Trønnes
  1 sibling, 1 reply; 22+ messages in thread
From: David Lechner @ 2019-01-14 22:06 UTC (permalink / raw)
  To: Noralf Trønnes, dri-devel; +Cc: drawat, sam, Daniel Vetter

On 1/11/19 2:12 PM, Noralf Trønnes wrote:
> This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty
> handler. All flushing will now happen in the pipe functions.
> 
> Also enable the damage plane property for all except repaper which can
> only do full updates.
> 
> ili9225:
> This change made ili9225_init() equal to mipi_dbi_init() so use it.
> 
> v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL
>      (kbuild test robot)
> 
> Cc: David Lechner <david@lechnology.com>
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---


...

> diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
> index a52bc3b02606..50e370ce5a59 100644
> --- a/drivers/gpu/drm/tinydrm/st7586.c
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -17,6 +17,7 @@
>   #include <linux/spi/spi.h>
>   #include <video/mipi_display.h>
>   
> +#include <drm/drm_damage_helper.h>
>   #include <drm/drm_drv.h>
>   #include <drm/drm_fb_cma_helper.h>
>   #include <drm/drm_gem_cma_helper.h>
> @@ -112,23 +113,15 @@ static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb,
>   	return ret;
>   }
>   
> -static int st7586_fb_dirty(struct drm_framebuffer *fb,
> -			   struct drm_file *file_priv, unsigned int flags,
> -			   unsigned int color, struct drm_clip_rect *clips,
> -			   unsigned int num_clips)
> +static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>   {
>   	struct tinydrm_device *tdev = fb->dev->dev_private;
>   	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> -	struct drm_rect clip;
> -	struct drm_rect *rect = &clip;

This function later modifies the fields of the struct pointed to by rect.
Are all callers of this function OK with having it modified or should we
make a local copy of rect and modify that instead?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-14 21:50           ` David Lechner
@ 2019-01-14 22:33             ` David Lechner
  2019-01-14 22:43               ` Noralf Trønnes
  2019-01-14 22:53               ` Noralf Trønnes
  0 siblings, 2 replies; 22+ messages in thread
From: David Lechner @ 2019-01-14 22:33 UTC (permalink / raw)
  To: Noralf Trønnes, dri-devel; +Cc: Daniel Vetter, drawat, sam

On 1/14/19 3:50 PM, David Lechner wrote:
> On 1/14/19 10:13 AM, Noralf Trønnes wrote:
>>
>> I see that you have this call chain:
>> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty().
>>
>> That doesn't look safe. The st7586 driver allocates a tx_buf with size:
>>     size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
>>
>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len:
>> fb->width * fb->height * 2
>>
>> It looks like you're writing zeroes way past the end of the buffer.
>>
>> Noralf.
>>
> 
> Thanks! That does indeed seem to be the problem. I'll put together
> a patch to fix this. I'm thinking it will be easier to make the
> fix before applying this series so that it will be easier to
> backport.
> 

Well, now that I am looking into it more, I see that the problem
was not preexisting. This patch ("drm/tinydrm: Use damage helper
for dirtyfb") also changes mipi_dbi_enable_flush() from calling
tdev->fb_dirty() to mipi_dbi_fb_dirty().

Perhaps we should not be dropping tdev->fb_dirty()?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-14 22:33             ` David Lechner
@ 2019-01-14 22:43               ` Noralf Trønnes
  2019-01-14 23:03                 ` David Lechner
  2019-01-14 22:53               ` Noralf Trønnes
  1 sibling, 1 reply; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-14 22:43 UTC (permalink / raw)
  To: David Lechner, dri-devel; +Cc: Daniel Vetter, drawat, sam



Den 14.01.2019 23.33, skrev David Lechner:
> On 1/14/19 3:50 PM, David Lechner wrote:
>> On 1/14/19 10:13 AM, Noralf Trønnes wrote:
>>>
>>> I see that you have this call chain:
>>> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty().
>>>
>>> That doesn't look safe. The st7586 driver allocates a tx_buf with size:
>>>     size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
>>>
>>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len:
>>> fb->width * fb->height * 2
>>>
>>> It looks like you're writing zeroes way past the end of the buffer.
>>>
>>> Noralf.
>>>
>>
>> Thanks! That does indeed seem to be the problem. I'll put together
>> a patch to fix this. I'm thinking it will be easier to make the
>> fix before applying this series so that it will be easier to
>> backport.
>>
> 
> Well, now that I am looking into it more, I see that the problem
> was not preexisting. This patch ("drm/tinydrm: Use damage helper
> for dirtyfb") also changes mipi_dbi_enable_flush() from calling
> tdev->fb_dirty() to mipi_dbi_fb_dirty().
> 
> Perhaps we should not be dropping tdev->fb_dirty()?

Ah, now it makes sense. I thought it strange that you hadn't experienced
the corruption before.

How about this change?

diff --git a/drivers/gpu/drm/tinydrm/st7586.c
b/drivers/gpu/drm/tinydrm/st7586.c
index d39417b74e8b..01a8077954b3 100644
--- a/drivers/gpu/drm/tinydrm/st7586.c
+++ b/drivers/gpu/drm/tinydrm/st7586.c
@@ -177,6 +177,13 @@ static void st7586_pipe_enable(struct
drm_simple_display_pipe *pipe,
 {
        struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
        struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+       struct drm_framebuffer *fb = plane_state->fb;
+       struct drm_rect rect = {
+               .x1 = 0,
+               .x2 = fb->width,
+               .y1 = 0,
+               .y2 = fb->height,
+       };
        int ret;
        u8 addr_mode;

@@ -233,9 +240,10 @@ static void st7586_pipe_enable(struct
drm_simple_display_pipe *pipe,

        msleep(100);

-       mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
+       mipi->enabled = true;
+       st7586_fb_dirty(fb, &rect);

-       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
+       mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
 }

 static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)


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

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-14 22:06   ` David Lechner
@ 2019-01-14 22:46     ` Noralf Trønnes
  0 siblings, 0 replies; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-14 22:46 UTC (permalink / raw)
  To: David Lechner, dri-devel; +Cc: drawat, sam, Daniel Vetter



Den 14.01.2019 23.06, skrev David Lechner:
> On 1/11/19 2:12 PM, Noralf Trønnes wrote:
>> This switches to drm_atomic_helper_dirtyfb() as the framebuffer dirty
>> handler. All flushing will now happen in the pipe functions.
>>
>> Also enable the damage plane property for all except repaper which can
>> only do full updates.
>>
>> ili9225:
>> This change made ili9225_init() equal to mipi_dbi_init() so use it.
>>
>> v2: Remove fb check in mipi_dbi_enable_flush() it can't be NULL
>>      (kbuild test robot)
>>
>> Cc: David Lechner <david@lechnology.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> Acked-by: Sam Ravnborg <sam@ravnborg.org>
>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
> 
> 
> ...
> 
>> diff --git a/drivers/gpu/drm/tinydrm/st7586.c
>> b/drivers/gpu/drm/tinydrm/st7586.c
>> index a52bc3b02606..50e370ce5a59 100644
>> --- a/drivers/gpu/drm/tinydrm/st7586.c
>> +++ b/drivers/gpu/drm/tinydrm/st7586.c
>> @@ -17,6 +17,7 @@
>>   #include <linux/spi/spi.h>
>>   #include <video/mipi_display.h>
>>   +#include <drm/drm_damage_helper.h>
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_fb_cma_helper.h>
>>   #include <drm/drm_gem_cma_helper.h>
>> @@ -112,23 +113,15 @@ static int st7586_buf_copy(void *dst, struct
>> drm_framebuffer *fb,
>>       return ret;
>>   }
>>   -static int st7586_fb_dirty(struct drm_framebuffer *fb,
>> -               struct drm_file *file_priv, unsigned int flags,
>> -               unsigned int color, struct drm_clip_rect *clips,
>> -               unsigned int num_clips)
>> +static void st7586_fb_dirty(struct drm_framebuffer *fb, struct
>> drm_rect *rect)
>>   {
>>       struct tinydrm_device *tdev = fb->dev->dev_private;
>>       struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
>> -    struct drm_rect clip;
>> -    struct drm_rect *rect = &clip;
> 
> This function later modifies the fields of the struct pointed to by rect.
> Are all callers of this function OK with having it modified or should we
> make a local copy of rect and modify that instead?

It's safe to modify it. st7586_pipe_update() owns the structure and it
doesn't use it after calling st7586_fb_dirty().

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-14 22:33             ` David Lechner
  2019-01-14 22:43               ` Noralf Trønnes
@ 2019-01-14 22:53               ` Noralf Trønnes
  2019-01-14 23:26                 ` David Lechner
  1 sibling, 1 reply; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-14 22:53 UTC (permalink / raw)
  To: David Lechner, dri-devel; +Cc: Daniel Vetter, drawat, sam



Den 14.01.2019 23.33, skrev David Lechner:
> On 1/14/19 3:50 PM, David Lechner wrote:
>> On 1/14/19 10:13 AM, Noralf Trønnes wrote:
>>>
>>> I see that you have this call chain:
>>> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty().
>>>
>>> That doesn't look safe. The st7586 driver allocates a tx_buf with size:
>>>     size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
>>>
>>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len:
>>> fb->width * fb->height * 2
>>>
>>> It looks like you're writing zeroes way past the end of the buffer.
>>>
>>> Noralf.
>>>
>>
>> Thanks! That does indeed seem to be the problem. I'll put together
>> a patch to fix this. I'm thinking it will be easier to make the
>> fix before applying this series so that it will be easier to
>> backport.
>>
> 
> Well, now that I am looking into it more, I see that the problem
> was not preexisting. This patch ("drm/tinydrm: Use damage helper
> for dirtyfb") also changes mipi_dbi_enable_flush() from calling
> tdev->fb_dirty() to mipi_dbi_fb_dirty().
> 
> Perhaps we should not be dropping tdev->fb_dirty()?

I want to get rid of tinydrm_device, to avoid tinydrm being like a
mid-layer. My goal is to make tinydrm just a collection of tiny regular
DRM drivers.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-14 22:43               ` Noralf Trønnes
@ 2019-01-14 23:03                 ` David Lechner
  2019-01-14 23:25                   ` Noralf Trønnes
  0 siblings, 1 reply; 22+ messages in thread
From: David Lechner @ 2019-01-14 23:03 UTC (permalink / raw)
  To: Noralf Trønnes, dri-devel; +Cc: Daniel Vetter, drawat, sam

On 1/14/19 4:43 PM, Noralf Trønnes wrote:
> 
> 
> Den 14.01.2019 23.33, skrev David Lechner:
>> On 1/14/19 3:50 PM, David Lechner wrote:
>>> On 1/14/19 10:13 AM, Noralf Trønnes wrote:
>>>>
>>>> I see that you have this call chain:
>>>> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty().
>>>>
>>>> That doesn't look safe. The st7586 driver allocates a tx_buf with size:
>>>>      size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
>>>>
>>>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len:
>>>> fb->width * fb->height * 2
>>>>
>>>> It looks like you're writing zeroes way past the end of the buffer.
>>>>
>>>> Noralf.
>>>>
>>>
>>> Thanks! That does indeed seem to be the problem. I'll put together
>>> a patch to fix this. I'm thinking it will be easier to make the
>>> fix before applying this series so that it will be easier to
>>> backport.
>>>
>>
>> Well, now that I am looking into it more, I see that the problem
>> was not preexisting. This patch ("drm/tinydrm: Use damage helper
>> for dirtyfb") also changes mipi_dbi_enable_flush() from calling
>> tdev->fb_dirty() to mipi_dbi_fb_dirty().
>>
>> Perhaps we should not be dropping tdev->fb_dirty()?
> 
> Ah, now it makes sense. I thought it strange that you hadn't experienced
> the corruption before.
> 
> How about this change?
> 
> diff --git a/drivers/gpu/drm/tinydrm/st7586.c
> b/drivers/gpu/drm/tinydrm/st7586.c
> index d39417b74e8b..01a8077954b3 100644
> --- a/drivers/gpu/drm/tinydrm/st7586.c
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -177,6 +177,13 @@ static void st7586_pipe_enable(struct
> drm_simple_display_pipe *pipe,
>   {
>          struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
>          struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> +       struct drm_framebuffer *fb = plane_state->fb;
> +       struct drm_rect rect = {
> +               .x1 = 0,
> +               .x2 = fb->width,
> +               .y1 = 0,
> +               .y2 = fb->height,
> +       };
>          int ret;
>          u8 addr_mode;
> 
> @@ -233,9 +240,10 @@ static void st7586_pipe_enable(struct
> drm_simple_display_pipe *pipe,
> 
>          msleep(100);
> 
> -       mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
> +       mipi->enabled = true;
> +       st7586_fb_dirty(fb, &rect);
> 
> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
> +       mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
>   }
> 
>   static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
> 
> 


That looks good. I also noticed that ili9225 has a custom dirty
function, so it will need a similar change as well.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-14 23:03                 ` David Lechner
@ 2019-01-14 23:25                   ` Noralf Trønnes
  0 siblings, 0 replies; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-14 23:25 UTC (permalink / raw)
  To: David Lechner, dri-devel; +Cc: Daniel Vetter, drawat, sam



Den 15.01.2019 00.03, skrev David Lechner:
> On 1/14/19 4:43 PM, Noralf Trønnes wrote:
>>
>>
>> Den 14.01.2019 23.33, skrev David Lechner:
>>> On 1/14/19 3:50 PM, David Lechner wrote:
>>>> On 1/14/19 10:13 AM, Noralf Trønnes wrote:
>>>>>
>>>>> I see that you have this call chain:
>>>>> st7586_pipe_enable() -> mipi_dbi_enable_flush() ->
>>>>> mipi_dbi_fb_dirty().
>>>>>
>>>>> That doesn't look safe. The st7586 driver allocates a tx_buf with
>>>>> size:
>>>>>      size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
>>>>>
>>>>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with
>>>>> len:
>>>>> fb->width * fb->height * 2
>>>>>
>>>>> It looks like you're writing zeroes way past the end of the buffer.
>>>>>
>>>>> Noralf.
>>>>>
>>>>
>>>> Thanks! That does indeed seem to be the problem. I'll put together
>>>> a patch to fix this. I'm thinking it will be easier to make the
>>>> fix before applying this series so that it will be easier to
>>>> backport.
>>>>
>>>
>>> Well, now that I am looking into it more, I see that the problem
>>> was not preexisting. This patch ("drm/tinydrm: Use damage helper
>>> for dirtyfb") also changes mipi_dbi_enable_flush() from calling
>>> tdev->fb_dirty() to mipi_dbi_fb_dirty().
>>>
>>> Perhaps we should not be dropping tdev->fb_dirty()?
>>
>> Ah, now it makes sense. I thought it strange that you hadn't experienced
>> the corruption before.
>>
>> How about this change?
>>
>> diff --git a/drivers/gpu/drm/tinydrm/st7586.c
>> b/drivers/gpu/drm/tinydrm/st7586.c
>> index d39417b74e8b..01a8077954b3 100644
>> --- a/drivers/gpu/drm/tinydrm/st7586.c
>> +++ b/drivers/gpu/drm/tinydrm/st7586.c
>> @@ -177,6 +177,13 @@ static void st7586_pipe_enable(struct
>> drm_simple_display_pipe *pipe,
>>   {
>>          struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
>>          struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
>> +       struct drm_framebuffer *fb = plane_state->fb;
>> +       struct drm_rect rect = {
>> +               .x1 = 0,
>> +               .x2 = fb->width,
>> +               .y1 = 0,
>> +               .y2 = fb->height,
>> +       };
>>          int ret;
>>          u8 addr_mode;
>>
>> @@ -233,9 +240,10 @@ static void st7586_pipe_enable(struct
>> drm_simple_display_pipe *pipe,
>>
>>          msleep(100);
>>
>> -       mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
>> +       mipi->enabled = true;
>> +       st7586_fb_dirty(fb, &rect);
>>
>> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
>> +       mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
>>   }
>>
>>   static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
>>
>>
> 
> 
> That looks good. I also noticed that ili9225 has a custom dirty
> function, so it will need a similar change as well.
> 

Thanks for tracking this down, I'll spin a new version tomorrow.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-14 22:53               ` Noralf Trønnes
@ 2019-01-14 23:26                 ` David Lechner
  2019-01-15  4:38                   ` Noralf Trønnes
  0 siblings, 1 reply; 22+ messages in thread
From: David Lechner @ 2019-01-14 23:26 UTC (permalink / raw)
  To: Noralf Trønnes, dri-devel; +Cc: Daniel Vetter, drawat, sam

On 1/14/19 4:53 PM, Noralf Trønnes wrote:
> 
> 
> Den 14.01.2019 23.33, skrev David Lechner:
>> On 1/14/19 3:50 PM, David Lechner wrote:
>>> On 1/14/19 10:13 AM, Noralf Trønnes wrote:
>>>>
>>>> I see that you have this call chain:
>>>> st7586_pipe_enable() -> mipi_dbi_enable_flush() -> mipi_dbi_fb_dirty().
>>>>
>>>> That doesn't look safe. The st7586 driver allocates a tx_buf with size:
>>>>      size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
>>>>
>>>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with len:
>>>> fb->width * fb->height * 2
>>>>
>>>> It looks like you're writing zeroes way past the end of the buffer.
>>>>
>>>> Noralf.
>>>>
>>>
>>> Thanks! That does indeed seem to be the problem. I'll put together
>>> a patch to fix this. I'm thinking it will be easier to make the
>>> fix before applying this series so that it will be easier to
>>> backport.
>>>
>>
>> Well, now that I am looking into it more, I see that the problem
>> was not preexisting. This patch ("drm/tinydrm: Use damage helper
>> for dirtyfb") also changes mipi_dbi_enable_flush() from calling
>> tdev->fb_dirty() to mipi_dbi_fb_dirty().
>>
>> Perhaps we should not be dropping tdev->fb_dirty()?
> 
> I want to get rid of tinydrm_device, to avoid tinydrm being like a
> mid-layer. My goal is to make tinydrm just a collection of tiny regular
> DRM drivers.
> 

OK. Perhaps we should add a comment to mipi_dbi_enable_flush() to
say that it should not be used by drivers with a custom dirty function.

Or perhaps we could pass an optional parameter with the custom dirty
function to mipi_dbi_enable_flush() like this:

diff --git a/drivers/gpu/drm/tinydrm/hx8357d.c b/drivers/gpu/drm/tinydrm/hx8357d.c
index 8bbd0beafc6a..4b9981ffe5a4 100644
--- a/drivers/gpu/drm/tinydrm/hx8357d.c
+++ b/drivers/gpu/drm/tinydrm/hx8357d.c
@@ -170,7 +170,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
                 break;
         }
         mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
-       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
+       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
  }

  static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
index aa1376a22bda..bebfd0d01340 100644
--- a/drivers/gpu/drm/tinydrm/ili9225.c
+++ b/drivers/gpu/drm/tinydrm/ili9225.c
@@ -270,7 +270,7 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,

         ili9225_command(mipi, ILI9225_DISPLAY_CONTROL_1, 0x1017);

-       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
+       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, ili9225_fb_dirty);
  }

  static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe)
diff --git a/drivers/gpu/drm/tinydrm/ili9341.c b/drivers/gpu/drm/tinydrm/ili9341.c
index 713bb2dd7e04..ecfb8ff02a40 100644
--- a/drivers/gpu/drm/tinydrm/ili9341.c
+++ b/drivers/gpu/drm/tinydrm/ili9341.c
@@ -126,7 +126,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
         }
         addr_mode |= ILI9341_MADCTL_BGR;
         mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
-       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
+       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
  }

  static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 82a92ec9ae3c..c81aa1fbc2ad 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -134,7 +134,7 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,
         }
         addr_mode |= ILI9341_MADCTL_BGR;
         mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
-       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
+       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
  }

  static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index 680692a83f94..399c66b3f0d7 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -287,6 +287,7 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update);
   * @mipi: MIPI DBI structure
   * @crtc_state: CRTC state
   * @plane_state: Plane state
+ * @dirty: Optional custom dirty function
   *
   * This function sets &mipi_dbi->enabled, flushes the whole framebuffer and
   * enables the backlight. Drivers can use this in their
@@ -294,7 +295,9 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update);
   */
  void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
                            struct drm_crtc_state *crtc_state,
-                          struct drm_plane_state *plane_state)
+                          struct drm_plane_state *plane_state,
+                          void (*dirty)(struct drm_framebuffer *fb,
+                                        struct drm_rect *rect))
  {
         struct drm_framebuffer *fb = plane_state->fb;
         struct drm_rect rect = {
@@ -305,7 +308,10 @@ void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
         };

         mipi->enabled = true;
-       mipi_dbi_fb_dirty(fb, &rect);
+       if (!dirty) {
+               dirty = mipi_dbi_fb_dirty;
+       }
+       dirty(fb, &rect);
         backlight_enable(mipi->backlight);
  }
  EXPORT_SYMBOL(mipi_dbi_enable_flush);
diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
index 50e370ce5a59..d6ffac64822f 100644
--- a/drivers/gpu/drm/tinydrm/st7586.c
+++ b/drivers/gpu/drm/tinydrm/st7586.c
@@ -234,7 +234,7 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,

         mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);

-       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
+       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, st7586_fb_dirty);
  }

  static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
index 3bab9a9569a6..dee143007048 100644
--- a/drivers/gpu/drm/tinydrm/st7735r.c
+++ b/drivers/gpu/drm/tinydrm/st7735r.c
@@ -100,7 +100,7 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe,

         msleep(20);

-       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
+       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
  }

  static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = {
diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
index f4ec2834bc22..3d9f003fdcc6 100644
--- a/include/drm/tinydrm/mipi-dbi.h
+++ b/include/drm/tinydrm/mipi-dbi.h
@@ -72,7 +72,9 @@ void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
                           struct drm_plane_state *old_state);
  void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
                            struct drm_crtc_state *crtc_state,
-                          struct drm_plane_state *plan_state);
+                          struct drm_plane_state *plan_state,
+                          void (*dirty)(struct drm_framebuffer *fb,
+                                        struct drm_rect *rect));
  void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
  void mipi_dbi_hw_reset(struct mipi_dbi *mipi);
  bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-14 23:26                 ` David Lechner
@ 2019-01-15  4:38                   ` Noralf Trønnes
  2019-01-15  4:54                     ` Noralf Trønnes
  0 siblings, 1 reply; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-15  4:38 UTC (permalink / raw)
  To: David Lechner, dri-devel; +Cc: Daniel Vetter, drawat, sam



Den 15.01.2019 00.26, skrev David Lechner:
> On 1/14/19 4:53 PM, Noralf Trønnes wrote:
>>
>>
>> Den 14.01.2019 23.33, skrev David Lechner:
>>> On 1/14/19 3:50 PM, David Lechner wrote:
>>>> On 1/14/19 10:13 AM, Noralf Trønnes wrote:
>>>>>
>>>>> I see that you have this call chain:
>>>>> st7586_pipe_enable() -> mipi_dbi_enable_flush() ->
>>>>> mipi_dbi_fb_dirty().
>>>>>
>>>>> That doesn't look safe. The st7586 driver allocates a tx_buf with
>>>>> size:
>>>>>      size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
>>>>>
>>>>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with
>>>>> len:
>>>>> fb->width * fb->height * 2
>>>>>
>>>>> It looks like you're writing zeroes way past the end of the buffer.
>>>>>
>>>>> Noralf.
>>>>>
>>>>
>>>> Thanks! That does indeed seem to be the problem. I'll put together
>>>> a patch to fix this. I'm thinking it will be easier to make the
>>>> fix before applying this series so that it will be easier to
>>>> backport.
>>>>
>>>
>>> Well, now that I am looking into it more, I see that the problem
>>> was not preexisting. This patch ("drm/tinydrm: Use damage helper
>>> for dirtyfb") also changes mipi_dbi_enable_flush() from calling
>>> tdev->fb_dirty() to mipi_dbi_fb_dirty().
>>>
>>> Perhaps we should not be dropping tdev->fb_dirty()?
>>
>> I want to get rid of tinydrm_device, to avoid tinydrm being like a
>> mid-layer. My goal is to make tinydrm just a collection of tiny regular
>> DRM drivers.
>>
> 
> OK. Perhaps we should add a comment to mipi_dbi_enable_flush() to
> say that it should not be used by drivers with a custom dirty function.
> 

I've added a note.

> Or perhaps we could pass an optional parameter with the custom dirty
> function to mipi_dbi_enable_flush() like this:
> 

I prefer to open code the function instead since it's only 2 lines of code.

Noralf.

> diff --git a/drivers/gpu/drm/tinydrm/hx8357d.c
> b/drivers/gpu/drm/tinydrm/hx8357d.c
> index 8bbd0beafc6a..4b9981ffe5a4 100644
> --- a/drivers/gpu/drm/tinydrm/hx8357d.c
> +++ b/drivers/gpu/drm/tinydrm/hx8357d.c
> @@ -170,7 +170,7 @@ static void yx240qv29_enable(struct
> drm_simple_display_pipe *pipe,
>                 break;
>         }
>         mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
> +       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
>  }
> 
>  static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c
> b/drivers/gpu/drm/tinydrm/ili9225.c
> index aa1376a22bda..bebfd0d01340 100644
> --- a/drivers/gpu/drm/tinydrm/ili9225.c
> +++ b/drivers/gpu/drm/tinydrm/ili9225.c
> @@ -270,7 +270,7 @@ static void ili9225_pipe_enable(struct
> drm_simple_display_pipe *pipe,
> 
>         ili9225_command(mipi, ILI9225_DISPLAY_CONTROL_1, 0x1017);
> 
> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
> +       mipi_dbi_enable_flush(mipi, crtc_state, plane_state,
> ili9225_fb_dirty);
>  }
> 
>  static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe)
> diff --git a/drivers/gpu/drm/tinydrm/ili9341.c
> b/drivers/gpu/drm/tinydrm/ili9341.c
> index 713bb2dd7e04..ecfb8ff02a40 100644
> --- a/drivers/gpu/drm/tinydrm/ili9341.c
> +++ b/drivers/gpu/drm/tinydrm/ili9341.c
> @@ -126,7 +126,7 @@ static void yx240qv29_enable(struct
> drm_simple_display_pipe *pipe,
>         }
>         addr_mode |= ILI9341_MADCTL_BGR;
>         mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
> +       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
>  }
> 
>  static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c
> b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 82a92ec9ae3c..c81aa1fbc2ad 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -134,7 +134,7 @@ static void mi0283qt_enable(struct
> drm_simple_display_pipe *pipe,
>         }
>         addr_mode |= ILI9341_MADCTL_BGR;
>         mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
> +       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
>  }
> 
>  static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> index 680692a83f94..399c66b3f0d7 100644
> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -287,6 +287,7 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update);
>   * @mipi: MIPI DBI structure
>   * @crtc_state: CRTC state
>   * @plane_state: Plane state
> + * @dirty: Optional custom dirty function
>   *
>   * This function sets &mipi_dbi->enabled, flushes the whole framebuffer
> and
>   * enables the backlight. Drivers can use this in their
> @@ -294,7 +295,9 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update);
>   */
>  void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
>                            struct drm_crtc_state *crtc_state,
> -                          struct drm_plane_state *plane_state)
> +                          struct drm_plane_state *plane_state,
> +                          void (*dirty)(struct drm_framebuffer *fb,
> +                                        struct drm_rect *rect))
>  {
>         struct drm_framebuffer *fb = plane_state->fb;
>         struct drm_rect rect = {
> @@ -305,7 +308,10 @@ void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
>         };
> 
>         mipi->enabled = true;
> -       mipi_dbi_fb_dirty(fb, &rect);
> +       if (!dirty) {
> +               dirty = mipi_dbi_fb_dirty;
> +       }
> +       dirty(fb, &rect);
>         backlight_enable(mipi->backlight);
>  }
>  EXPORT_SYMBOL(mipi_dbi_enable_flush);
> diff --git a/drivers/gpu/drm/tinydrm/st7586.c
> b/drivers/gpu/drm/tinydrm/st7586.c
> index 50e370ce5a59..d6ffac64822f 100644
> --- a/drivers/gpu/drm/tinydrm/st7586.c
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -234,7 +234,7 @@ static void st7586_pipe_enable(struct
> drm_simple_display_pipe *pipe,
> 
>         mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
> 
> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
> +       mipi_dbi_enable_flush(mipi, crtc_state, plane_state,
> st7586_fb_dirty);
>  }
> 
>  static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
> diff --git a/drivers/gpu/drm/tinydrm/st7735r.c
> b/drivers/gpu/drm/tinydrm/st7735r.c
> index 3bab9a9569a6..dee143007048 100644
> --- a/drivers/gpu/drm/tinydrm/st7735r.c
> +++ b/drivers/gpu/drm/tinydrm/st7735r.c
> @@ -100,7 +100,7 @@ static void jd_t18003_t01_pipe_enable(struct
> drm_simple_display_pipe *pipe,
> 
>         msleep(20);
> 
> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
> +       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
>  }
> 
>  static const struct drm_simple_display_pipe_funcs
> jd_t18003_t01_pipe_funcs = {
> diff --git a/include/drm/tinydrm/mipi-dbi.h
> b/include/drm/tinydrm/mipi-dbi.h
> index f4ec2834bc22..3d9f003fdcc6 100644
> --- a/include/drm/tinydrm/mipi-dbi.h
> +++ b/include/drm/tinydrm/mipi-dbi.h
> @@ -72,7 +72,9 @@ void mipi_dbi_pipe_update(struct
> drm_simple_display_pipe *pipe,
>                           struct drm_plane_state *old_state);
>  void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
>                            struct drm_crtc_state *crtc_state,
> -                          struct drm_plane_state *plan_state);
> +                          struct drm_plane_state *plan_state,
> +                          void (*dirty)(struct drm_framebuffer *fb,
> +                                        struct drm_rect *rect));
>  void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
>  void mipi_dbi_hw_reset(struct mipi_dbi *mipi);
>  bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb
  2019-01-15  4:38                   ` Noralf Trønnes
@ 2019-01-15  4:54                     ` Noralf Trønnes
  0 siblings, 0 replies; 22+ messages in thread
From: Noralf Trønnes @ 2019-01-15  4:54 UTC (permalink / raw)
  To: David Lechner, dri-devel; +Cc: Daniel Vetter, drawat, sam



Den 15.01.2019 05.38, skrev Noralf Trønnes:
> 
> 
> Den 15.01.2019 00.26, skrev David Lechner:
>> On 1/14/19 4:53 PM, Noralf Trønnes wrote:
>>>
>>>
>>> Den 14.01.2019 23.33, skrev David Lechner:
>>>> On 1/14/19 3:50 PM, David Lechner wrote:
>>>>> On 1/14/19 10:13 AM, Noralf Trønnes wrote:
>>>>>>
>>>>>> I see that you have this call chain:
>>>>>> st7586_pipe_enable() -> mipi_dbi_enable_flush() ->
>>>>>> mipi_dbi_fb_dirty().
>>>>>>
>>>>>> That doesn't look safe. The st7586 driver allocates a tx_buf with
>>>>>> size:
>>>>>>      size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
>>>>>>
>>>>>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with
>>>>>> len:
>>>>>> fb->width * fb->height * 2
>>>>>>
>>>>>> It looks like you're writing zeroes way past the end of the buffer.
>>>>>>
>>>>>> Noralf.
>>>>>>
>>>>>
>>>>> Thanks! That does indeed seem to be the problem. I'll put together
>>>>> a patch to fix this. I'm thinking it will be easier to make the
>>>>> fix before applying this series so that it will be easier to
>>>>> backport.
>>>>>
>>>>
>>>> Well, now that I am looking into it more, I see that the problem
>>>> was not preexisting. This patch ("drm/tinydrm: Use damage helper
>>>> for dirtyfb") also changes mipi_dbi_enable_flush() from calling
>>>> tdev->fb_dirty() to mipi_dbi_fb_dirty().
>>>>
>>>> Perhaps we should not be dropping tdev->fb_dirty()?
>>>
>>> I want to get rid of tinydrm_device, to avoid tinydrm being like a
>>> mid-layer. My goal is to make tinydrm just a collection of tiny regular
>>> DRM drivers.
>>>
>>
>> OK. Perhaps we should add a comment to mipi_dbi_enable_flush() to
>> say that it should not be used by drivers with a custom dirty function.
>>
> 
> I've added a note.
> 
>> Or perhaps we could pass an optional parameter with the custom dirty
>> function to mipi_dbi_enable_flush() like this:
>>
> 
> I prefer to open code the function instead since it's only 2 lines of code.

'2 lines of code', that sounds like a misleading commercial :-)
It's more like 9 including the declarions. But anyways I prefer it.


> 
> Noralf.
> 
>> diff --git a/drivers/gpu/drm/tinydrm/hx8357d.c
>> b/drivers/gpu/drm/tinydrm/hx8357d.c
>> index 8bbd0beafc6a..4b9981ffe5a4 100644
>> --- a/drivers/gpu/drm/tinydrm/hx8357d.c
>> +++ b/drivers/gpu/drm/tinydrm/hx8357d.c
>> @@ -170,7 +170,7 @@ static void yx240qv29_enable(struct
>> drm_simple_display_pipe *pipe,
>>                 break;
>>         }
>>         mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
>> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
>> +       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
>>  }
>>
>>  static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
>> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c
>> b/drivers/gpu/drm/tinydrm/ili9225.c
>> index aa1376a22bda..bebfd0d01340 100644
>> --- a/drivers/gpu/drm/tinydrm/ili9225.c
>> +++ b/drivers/gpu/drm/tinydrm/ili9225.c
>> @@ -270,7 +270,7 @@ static void ili9225_pipe_enable(struct
>> drm_simple_display_pipe *pipe,
>>
>>         ili9225_command(mipi, ILI9225_DISPLAY_CONTROL_1, 0x1017);
>>
>> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
>> +       mipi_dbi_enable_flush(mipi, crtc_state, plane_state,
>> ili9225_fb_dirty);
>>  }
>>
>>  static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe)
>> diff --git a/drivers/gpu/drm/tinydrm/ili9341.c
>> b/drivers/gpu/drm/tinydrm/ili9341.c
>> index 713bb2dd7e04..ecfb8ff02a40 100644
>> --- a/drivers/gpu/drm/tinydrm/ili9341.c
>> +++ b/drivers/gpu/drm/tinydrm/ili9341.c
>> @@ -126,7 +126,7 @@ static void yx240qv29_enable(struct
>> drm_simple_display_pipe *pipe,
>>         }
>>         addr_mode |= ILI9341_MADCTL_BGR;
>>         mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
>> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
>> +       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
>>  }
>>
>>  static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c
>> b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> index 82a92ec9ae3c..c81aa1fbc2ad 100644
>> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
>> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> @@ -134,7 +134,7 @@ static void mi0283qt_enable(struct
>> drm_simple_display_pipe *pipe,
>>         }
>>         addr_mode |= ILI9341_MADCTL_BGR;
>>         mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
>> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
>> +       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
>>  }
>>
>>  static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
>> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> index 680692a83f94..399c66b3f0d7 100644
>> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>> @@ -287,6 +287,7 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update);
>>   * @mipi: MIPI DBI structure
>>   * @crtc_state: CRTC state
>>   * @plane_state: Plane state
>> + * @dirty: Optional custom dirty function
>>   *
>>   * This function sets &mipi_dbi->enabled, flushes the whole framebuffer
>> and
>>   * enables the backlight. Drivers can use this in their
>> @@ -294,7 +295,9 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update);
>>   */
>>  void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
>>                            struct drm_crtc_state *crtc_state,
>> -                          struct drm_plane_state *plane_state)
>> +                          struct drm_plane_state *plane_state,
>> +                          void (*dirty)(struct drm_framebuffer *fb,
>> +                                        struct drm_rect *rect))
>>  {
>>         struct drm_framebuffer *fb = plane_state->fb;
>>         struct drm_rect rect = {
>> @@ -305,7 +308,10 @@ void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
>>         };
>>
>>         mipi->enabled = true;
>> -       mipi_dbi_fb_dirty(fb, &rect);
>> +       if (!dirty) {
>> +               dirty = mipi_dbi_fb_dirty;
>> +       }
>> +       dirty(fb, &rect);
>>         backlight_enable(mipi->backlight);
>>  }
>>  EXPORT_SYMBOL(mipi_dbi_enable_flush);
>> diff --git a/drivers/gpu/drm/tinydrm/st7586.c
>> b/drivers/gpu/drm/tinydrm/st7586.c
>> index 50e370ce5a59..d6ffac64822f 100644
>> --- a/drivers/gpu/drm/tinydrm/st7586.c
>> +++ b/drivers/gpu/drm/tinydrm/st7586.c
>> @@ -234,7 +234,7 @@ static void st7586_pipe_enable(struct
>> drm_simple_display_pipe *pipe,
>>
>>         mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
>>
>> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
>> +       mipi_dbi_enable_flush(mipi, crtc_state, plane_state,
>> st7586_fb_dirty);
>>  }
>>
>>  static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
>> diff --git a/drivers/gpu/drm/tinydrm/st7735r.c
>> b/drivers/gpu/drm/tinydrm/st7735r.c
>> index 3bab9a9569a6..dee143007048 100644
>> --- a/drivers/gpu/drm/tinydrm/st7735r.c
>> +++ b/drivers/gpu/drm/tinydrm/st7735r.c
>> @@ -100,7 +100,7 @@ static void jd_t18003_t01_pipe_enable(struct
>> drm_simple_display_pipe *pipe,
>>
>>         msleep(20);
>>
>> -       mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
>> +       mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
>>  }
>>
>>  static const struct drm_simple_display_pipe_funcs
>> jd_t18003_t01_pipe_funcs = {
>> diff --git a/include/drm/tinydrm/mipi-dbi.h
>> b/include/drm/tinydrm/mipi-dbi.h
>> index f4ec2834bc22..3d9f003fdcc6 100644
>> --- a/include/drm/tinydrm/mipi-dbi.h
>> +++ b/include/drm/tinydrm/mipi-dbi.h
>> @@ -72,7 +72,9 @@ void mipi_dbi_pipe_update(struct
>> drm_simple_display_pipe *pipe,
>>                           struct drm_plane_state *old_state);
>>  void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
>>                            struct drm_crtc_state *crtc_state,
>> -                          struct drm_plane_state *plan_state);
>> +                          struct drm_plane_state *plan_state,
>> +                          void (*dirty)(struct drm_framebuffer *fb,
>> +                                        struct drm_rect *rect));
>>  void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
>>  void mipi_dbi_hw_reset(struct mipi_dbi *mipi);
>>  bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-01-15  4:54 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-11 20:12 [PATCH v2 0/5] drm/tinydrm: Use damage helper for dirtyfb Noralf Trønnes
2019-01-11 20:12 ` [PATCH v2 1/5] drm/gem-fb-helper: Add drm_gem_fb_create_with_dirty() Noralf Trønnes
2019-01-11 20:12 ` [PATCH v2 2/5] drm/damage-helper: Add drm_atomic_helper_damage_merged() Noralf Trønnes
2019-01-11 20:12 ` [PATCH v2 3/5] drm/tinydrm: Use struct drm_rect Noralf Trønnes
2019-01-11 20:12 ` [PATCH v2 4/5] drm/tinydrm: Use damage helper for dirtyfb Noralf Trønnes
2019-01-13 21:19   ` David Lechner
2019-01-14  2:15     ` David Lechner
2019-01-14 15:00       ` Noralf Trønnes
2019-01-14 16:13         ` Noralf Trønnes
2019-01-14 21:50           ` David Lechner
2019-01-14 22:33             ` David Lechner
2019-01-14 22:43               ` Noralf Trønnes
2019-01-14 23:03                 ` David Lechner
2019-01-14 23:25                   ` Noralf Trønnes
2019-01-14 22:53               ` Noralf Trønnes
2019-01-14 23:26                 ` David Lechner
2019-01-15  4:38                   ` Noralf Trønnes
2019-01-15  4:54                     ` Noralf Trønnes
2019-01-14 22:06   ` David Lechner
2019-01-14 22:46     ` Noralf Trønnes
2019-01-11 20:12 ` [PATCH v2 5/5] drm/todo: Tick off some tinydrm entries Noralf Trønnes
2019-01-12 21:43 ` [PATCH v2 0/5] drm/tinydrm: Use damage helper for dirtyfb Sam Ravnborg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.