Re: [PATCH 2/2] drm/tinydrm: Make fb_dirty into a lower level hook

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Den 23.03.2018 12.31, skrev Ville Syrjälä:
On Fri, Mar 23, 2018 at 12:43:58AM +0100, Noralf Trønnes wrote:

Den 22.03.2018 21.27, skrev Ville Syrjala:
From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>

mipi_dbi_enable_flush() wants to call the fb->dirty() hook from the
bowels of the .atomic_enable() hook. That prevents us from taking the
plane mutex in fb->dirty() unless we also plumb down the acquire
context.

Instead it seems simpler to split the fb->dirty() into a tinydrm
specific lower level hook that can be called from
mipi_dbi_enable_flush() and from a generic higher level
tinydrm_fb_dirty() helper. As we don't have a tinydrm specific
vfuncs table we'll just stick it into tinydrm_device directly
for now.
The long term goal is to try and get rid of tinydrm.ko by moving stuff
elsewhere as it's kind of a middle layer. So I'd really like to avoid
adding a callback like this.
Hopefully we can work out a solution based on my suggestion in the
'drm: Eliminate plane->fb/crtc usage for atomic drivers' thread.
I don't want to start redesigning the entire architecture at
this point. That would also cause a bigger risk of regression and
then we'd potentially have to try and revert the entire plane->fb
thing, which would not be fun if any significant changes have
occurred in the meantime.

If we do need a hook, I prefer that we add it to
drm_simple_display_pipe_funcs.
If you have plans to redesign tinydrm anyway it seems to me that
a temporary hook in tinydrm is may be a bit less intrusive than
inflicting it on the simple_kms_helper.

Yes you're right of course, what was I thinking.

You missed out on one call site:

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
index 11ae950b0fc9..7924eb59c2e1 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
@@ -124,11 +124,8 @@ void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
        struct drm_framebuffer *fb = pipe->plane.state->fb;
        struct drm_crtc *crtc = &tdev->pipe.crtc;

-       if (fb && (fb != old_state->fb)) {
-               pipe->plane.fb = fb;
-               if (fb->funcs->dirty)
-                       fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
-       }
+       if (fb && (fb != old_state->fb))
+               tdev->fb_dirty(fb, NULL, 0, 0, NULL, 0);

        if (crtc->state->event) {
                spin_lock_irq(&crtc->dev->event_lock);

With that fixed, series is:

Reviewed-by: Noralf Trønnes <noralf@xxxxxxxxxxx>

To be honest I don't understand why it's necessary to wire through the
plane state to the enable hook instead of just looking it up on the pipe
object. You look it up on the pipe in tinydrm_fb_dirty(), and I look up
the new plane state and crtc state on the pipe in the update hook.

Noralf.


But if you do prefer
drm_simple_display_pipe_funcs I can move it there of course.

Noralf.

Cc: "Noralf Trønnes" <noralf@xxxxxxxxxxx>
Cc: David Lechner <david@xxxxxxxxxxxxxx>
Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
---
   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 30 ++++++++++++++++++++++++++
   drivers/gpu/drm/tinydrm/ili9225.c              | 23 ++++++--------------
   drivers/gpu/drm/tinydrm/mi0283qt.c             |  2 +-
   drivers/gpu/drm/tinydrm/mipi-dbi.c             | 30 ++++++++++----------------
   drivers/gpu/drm/tinydrm/repaper.c              | 28 ++++++++----------------
   drivers/gpu/drm/tinydrm/st7586.c               | 23 ++++++--------------
   drivers/gpu/drm/tinydrm/st7735r.c              |  2 +-
   include/drm/tinydrm/mipi-dbi.h                 |  4 +++-
   include/drm/tinydrm/tinydrm-helpers.h          |  5 +++++
   include/drm/tinydrm/tinydrm.h                  |  4 ++++
   10 files changed, 76 insertions(+), 75 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index d1c3ce9ab294..dcd390163a4a 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -78,6 +78,36 @@ bool tinydrm_merge_clips(struct drm_clip_rect *dst,
   }
   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/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
index 089d22798c8b..0874e877b111 100644
--- a/drivers/gpu/drm/tinydrm/ili9225.c
+++ b/drivers/gpu/drm/tinydrm/ili9225.c
@@ -88,14 +88,8 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb,
   	bool full;
   	void *tr;
- mutex_lock(&tdev->dirty_lock);
-
   	if (!mipi->enabled)
-		goto out_unlock;
-
-	/* fbdev can flush even when we're not interested */
-	if (tdev->pipe.plane.fb != fb)
-		goto out_unlock;
+		return 0;
full = tinydrm_merge_clips(&clip, clips, num_clips, flags,
   				   fb->width, fb->height);
@@ -108,7 +102,7 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb,
   		tr = mipi->tx_buf;
   		ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip, swap);
   		if (ret)
-			goto out_unlock;
+			return ret;
   	} else {
   		tr = cma_obj->vaddr;
   	}
@@ -159,20 +153,13 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb,
   	ret = mipi_dbi_command_buf(mipi, ILI9225_WRITE_DATA_TO_GRAM, tr,
   				(clip.x2 - clip.x1) * (clip.y2 - clip.y1) * 2);
-out_unlock:
-	mutex_unlock(&tdev->dirty_lock);
-
-	if (ret)
-		dev_err_once(fb->dev->dev, "Failed to update display %d\n",
-			     ret);
-
   	return ret;
   }
static const struct drm_framebuffer_funcs ili9225_fb_funcs = {
   	.destroy	= drm_gem_fb_destroy,
   	.create_handle	= drm_gem_fb_create_handle,
-	.dirty		= ili9225_fb_dirty,
+	.dirty		= tinydrm_fb_dirty,
   };
static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
@@ -269,7 +256,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);
+	mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
   }
static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe)
@@ -342,6 +329,8 @@ static int ili9225_init(struct device *dev, struct mipi_dbi *mipi,
   	if (ret)
   		return ret;
+ tdev->fb_dirty = ili9225_fb_dirty;
+
   	ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
   					DRM_MODE_CONNECTOR_VIRTUAL,
   					ili9225_formats,
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 82ad9b61898e..4e6d2ee94e55 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -127,7 +127,7 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,
   	msleep(100);
out_enable:
-	mipi_dbi_enable_flush(mipi);
+	mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
   }
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 9e903812b573..4d1fb31a781f 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -219,14 +219,8 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
   	bool full;
   	void *tr;
- mutex_lock(&tdev->dirty_lock);
-
   	if (!mipi->enabled)
-		goto out_unlock;
-
-	/* fbdev can flush even when we're not interested */
-	if (tdev->pipe.plane.fb != fb)
-		goto out_unlock;
+		return 0;
full = tinydrm_merge_clips(&clip, clips, num_clips, flags,
   				   fb->width, fb->height);
@@ -239,7 +233,7 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
   		tr = mipi->tx_buf;
   		ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip, swap);
   		if (ret)
-			goto out_unlock;
+			return ret;
   	} else {
   		tr = cma_obj->vaddr;
   	}
@@ -254,20 +248,13 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
   	ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START, tr,
   				(clip.x2 - clip.x1) * (clip.y2 - clip.y1) * 2);
-out_unlock:
-	mutex_unlock(&tdev->dirty_lock);
-
-	if (ret)
-		dev_err_once(fb->dev->dev, "Failed to update display %d\n",
-			     ret);
-
   	return ret;
   }
static const struct drm_framebuffer_funcs mipi_dbi_fb_funcs = {
   	.destroy	= drm_gem_fb_destroy,
   	.create_handle	= drm_gem_fb_create_handle,
-	.dirty		= mipi_dbi_fb_dirty,
+	.dirty		= tinydrm_fb_dirty,
   };
/**
@@ -278,13 +265,16 @@ static const struct drm_framebuffer_funcs mipi_dbi_fb_funcs = {
    * enables the backlight. Drivers can use this in their
    * &drm_simple_display_pipe_funcs->enable callback.
    */
-void mipi_dbi_enable_flush(struct mipi_dbi *mipi)
+void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
+			   struct drm_crtc_state *crtc_state,
+			   struct drm_plane_state *plane_state)
   {
-	struct drm_framebuffer *fb = mipi->tinydrm.pipe.plane.fb;
+	struct tinydrm_device *tdev = &mipi->tinydrm;
+	struct drm_framebuffer *fb = plane_state->fb;
mipi->enabled = true;
   	if (fb)
-		fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
+		tdev->fb_dirty(fb, NULL, 0, 0, NULL, 0);
backlight_enable(mipi->backlight);
   }
@@ -381,6 +371,8 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
   	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,
diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c
index 33b4a71916e4..bb6f80a81899 100644
--- a/drivers/gpu/drm/tinydrm/repaper.c
+++ b/drivers/gpu/drm/tinydrm/repaper.c
@@ -540,14 +540,8 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,
   	clip.y1 = 0;
   	clip.y2 = fb->height;
- mutex_lock(&tdev->dirty_lock);
-
   	if (!epd->enabled)
-		goto out_unlock;
-
-	/* fbdev can flush even when we're not interested */
-	if (tdev->pipe.plane.fb != fb)
-		goto out_unlock;
+		return 0;
repaper_get_temperature(epd); @@ -555,16 +549,14 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,
   		  epd->factored_stage_time);
buf = kmalloc(fb->width * fb->height, GFP_KERNEL);
-	if (!buf) {
-		ret = -ENOMEM;
-		goto out_unlock;
-	}
+	if (!buf)
+		return -ENOMEM;
if (import_attach) {
   		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
   					       DMA_FROM_DEVICE);
   		if (ret)
-			goto out_unlock;
+			goto out_free;
   	}
tinydrm_xrgb8888_to_gray8(buf, cma_obj->vaddr, fb, &clip);
@@ -573,7 +565,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,
   		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
   					     DMA_FROM_DEVICE);
   		if (ret)
-			goto out_unlock;
+			goto out_free;
   	}
repaper_gray8_to_mono_reversed(buf, fb->width, fb->height);
@@ -625,11 +617,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,
   			}
   	}
-out_unlock:
-	mutex_unlock(&tdev->dirty_lock);
-
-	if (ret)
-		DRM_DEV_ERROR(fb->dev->dev, "Failed to update display (%d)\n", ret);
+out_free:
   	kfree(buf);
return ret;
@@ -638,7 +626,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,
   static const struct drm_framebuffer_funcs repaper_fb_funcs = {
   	.destroy	= drm_gem_fb_destroy,
   	.create_handle	= drm_gem_fb_create_handle,
-	.dirty		= repaper_fb_dirty,
+	.dirty		= tinydrm_fb_dirty,
   };
static void power_off(struct repaper_epd *epd)
@@ -1070,6 +1058,8 @@ static int repaper_probe(struct spi_device *spi)
   	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 bb08b293c8ce..22644b88199a 100644
--- a/drivers/gpu/drm/tinydrm/st7586.c
+++ b/drivers/gpu/drm/tinydrm/st7586.c
@@ -120,14 +120,8 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb,
   	int start, end;
   	int ret = 0;
- mutex_lock(&tdev->dirty_lock);
-
   	if (!mipi->enabled)
-		goto out_unlock;
-
-	/* fbdev can flush even when we're not interested */
-	if (tdev->pipe.plane.fb != fb)
-		goto out_unlock;
+		return 0;
tinydrm_merge_clips(&clip, clips, num_clips, flags, fb->width,
   			    fb->height);
@@ -141,7 +135,7 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb,
ret = st7586_buf_copy(mipi->tx_buf, fb, &clip);
   	if (ret)
-		goto out_unlock;
+		return ret;
/* Pixels are packed 3 per byte */
   	start = clip.x1 / 3;
@@ -158,20 +152,13 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb,
   				   (u8 *)mipi->tx_buf,
   				   (end - start) * (clip.y2 - clip.y1));
-out_unlock:
-	mutex_unlock(&tdev->dirty_lock);
-
-	if (ret)
-		dev_err_once(fb->dev->dev, "Failed to update display %d\n",
-			     ret);
-
   	return ret;
   }
static const struct drm_framebuffer_funcs st7586_fb_funcs = {
   	.destroy	= drm_gem_fb_destroy,
   	.create_handle	= drm_gem_fb_create_handle,
-	.dirty		= st7586_fb_dirty,
+	.dirty		= tinydrm_fb_dirty,
   };
static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
@@ -238,7 +225,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);
+	mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
   }
static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
@@ -278,6 +265,8 @@ static int st7586_init(struct device *dev, struct mipi_dbi *mipi,
   	if (ret)
   		return ret;
+ tdev->fb_dirty = st7586_fb_dirty;
+
   	ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
   					DRM_MODE_CONNECTOR_VIRTUAL,
   					st7586_formats,
diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c
index 19b28f8c78db..189a07894d36 100644
--- a/drivers/gpu/drm/tinydrm/st7735r.c
+++ b/drivers/gpu/drm/tinydrm/st7735r.c
@@ -99,7 +99,7 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe,
msleep(20); - mipi_dbi_enable_flush(mipi);
+	mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
   }
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 44e824af2ef6..b8ba58861986 100644
--- a/include/drm/tinydrm/mipi-dbi.h
+++ b/include/drm/tinydrm/mipi-dbi.h
@@ -67,7 +67,9 @@ 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_enable_flush(struct mipi_dbi *mipi);
+void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
+			   struct drm_crtc_state *crtc_state,
+			   struct drm_plane_state *plan_state);
   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);
diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
index 0a4ddbc04c60..5b96f0b12c8c 100644
--- a/include/drm/tinydrm/tinydrm-helpers.h
+++ b/include/drm/tinydrm/tinydrm-helpers.h
@@ -36,6 +36,11 @@ static inline bool tinydrm_machine_little_endian(void)
   bool tinydrm_merge_clips(struct drm_clip_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_clip_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 07a9a11fe19d..ea2c71081190 100644
--- a/include/drm/tinydrm/tinydrm.h
+++ b/include/drm/tinydrm/tinydrm.h
@@ -26,6 +26,10 @@ struct tinydrm_device {
   	struct drm_simple_display_pipe pipe;
   	struct mutex dirty_lock;
   	const struct drm_framebuffer_funcs *fb_funcs;
+	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 *

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux