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

[Noralf Trønnes <noralf@xxxxxxxxxxx>]

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.

If we do need a hook, I prefer that we add it to
drm_simple_display_pipe_funcs.

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