Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1390316 > unrolled thread

[PATCH v4 0/7] drm: Add fbdev deferred io support to helpers

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/7] drm: Add fbdev deferred io support to helpers Noralf Trønnes <noralf@tronnes.org> - 2016-04-28 17:30 +0200
    [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support Noralf Trønnes <noralf@tronnes.org> - 2016-04-28 17:30 +0200
      Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support Tomi Valkeinen <tomi.valkeinen@ti.com> - 2016-04-29 15:00 +0200
        Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support Noralf Trønnes <noralf@tronnes.org> - 2016-04-29 16:50 +0200
          Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support Daniel Vetter <daniel@ffwll.ch> - 2016-05-02 16:30 +0200
        Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support Daniel Vetter <daniel@ffwll.ch> - 2016-04-29 17:00 +0200
          Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support Tomi Valkeinen <tomi.valkeinen@ti.com> - 2016-04-29 17:10 +0200
            Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support Daniel Vetter <daniel@ffwll.ch> - 2016-04-29 17:40 +0200
            Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support Daniel Vetter <daniel@ffwll.ch> - 2016-04-29 17:40 +0200
    [PATCH v4 4/7] fbdev: fb_defio: Export fb_deferred_io_mmap Noralf Trønnes <noralf@tronnes.org> - 2016-04-28 17:30 +0200
      Re: [PATCH v4 4/7] fbdev: fb_defio: Export fb_deferred_io_mmap Tomi Valkeinen <tomi.valkeinen@ti.com> - 2016-04-29 16:10 +0200
    Re: [PATCH v4 0/7] drm: Add fbdev deferred io support to helpers Daniel Vetter <daniel@ffwll.ch> - 2016-04-29 17:00 +0200
      Re: [PATCH v4 0/7] drm: Add fbdev deferred io support to helpers Daniel Vetter <daniel@ffwll.ch> - 2016-05-02 16:30 +0200

#1390316 — [PATCH v4 0/7] drm: Add fbdev deferred io support to helpers

This patchset adds fbdev deferred io support to drm_fb_helper and
drm_fb_cma_helper.

It channels fbdev mmap and fb_{write,fillrect,copyarea,imageblit} damage
through the (struct drm_framebuffer_funcs)->dirty callback on the
fb_helper framebuffer which will always run in process context.

I have also added patches that converts qxl and udl to use this
deferred io support. I have only compile tested it, no functional testing.
I know that qxl is purely a software thing so I could actually test it, but
I have never used qemu so I'm not keen on spending a lot of time on that.

This was originally part of the tinydrm patchset.

Changes since v3:
- drm/fb-helper: Add fb_deferred_io support
  - Don't use forward decl, move drm_fb_helper_dirty_work()
  - Use DIV_ROUND_UP in drm_fb_helper_deferred_io()

Changes since v2:
- drm/rect: Add some drm_clip_rect utility functions
  - This patch is dropped
- drm/fb-helper: Add fb_deferred_io support
  - FB_DEFERRED_IO is now always selected by DRM_KMS_FB_HELPER, ifdef removed
  - The drm_clip_rect utility functions are dropped, so open code it
  - docs: use & to denote structs
- drm/qxl: Use drm_fb_helper deferred_io support
  - The drm_clip_rect_{width/height} functions are dropped, so open code it

Changes since v1:
- drm/fb-helper: Add fb_deferred_io support
  - Use a dedicated worker to run the framebuffer flushing like qxl does
  - Add parameter descriptions to drm_fb_helper_deferred_io
- fbdev: fb_defio: Export fb_deferred_io_mmap
  - Expand commit message
- drm/qxl: Use drm_fb_helper deferred_io support
  - Add FIXME about special dirty() callback for fbdev
  - Remove note in commit message about deferred worker, drm_fb_helper
    is similar to qxl now.
- drm/udl: Use drm_fb_helper deferred_io support
  - No need to enable deferred_io by default since drm_fb_helper uses
    a dedicated worker for flushing

Changes since RFC:
- Fix drm_clip_rect use to be exclusive on x2/y2
- Put drm_clip_rect functions in drm_rect.{h,c}
- Take into account that (struct fb_ops *)->fb_{write,...}() can be called
  from atomic context (spin_lock_irqsave)
- Export fb_deferred_io_mmap()
- Add some more documentation
- Add qxl and udl patches

Noralf Trønnes (7):
  drm/udl: Change drm_fb_helper_sys_*() calls to sys_*()
  drm/qxl: Change drm_fb_helper_sys_*() calls to sys_*()
  drm/fb-helper: Add fb_deferred_io support
  fbdev: fb_defio: Export fb_deferred_io_mmap
  drm/fb-cma-helper: Add fb_deferred_io support
  drm/qxl: Use drm_fb_helper deferred_io support
  drm/udl: Use drm_fb_helper deferred_io support

 drivers/gpu/drm/Kconfig             |   1 +
 drivers/gpu/drm/drm_fb_cma_helper.c | 178 ++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_fb_helper.c     | 103 ++++++++++++++++-
 drivers/gpu/drm/qxl/qxl_display.c   |   9 +-
 drivers/gpu/drm/qxl/qxl_drv.h       |   7 +-
 drivers/gpu/drm/qxl/qxl_fb.c        | 223 +++++++++---------------------------
 drivers/gpu/drm/qxl/qxl_kms.c       |   4 -
 drivers/gpu/drm/udl/udl_drv.h       |   2 -
 drivers/gpu/drm/udl/udl_fb.c        | 140 +---------------------
 drivers/video/fbdev/core/fb_defio.c |   3 +-
 include/drm/drm_fb_cma_helper.h     |  14 +++
 include/drm/drm_fb_helper.h         |  15 +++
 include/linux/fb.h                  |   1 +
 13 files changed, 372 insertions(+), 328 deletions(-)

--
2.2.2

[toc] | [next] | [standalone]

#1390317 — [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support

This adds deferred io support to drm_fb_helper.
The fbdev framebuffer changes are flushed using the callback
(struct drm_framebuffer *)->funcs->dirty() by a dedicated worker
ensuring that it always runs in process context.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---

Changes since v3:
- Don't use forward decl, move drm_fb_helper_dirty_work()
- Use DIV_ROUND_UP in drm_fb_helper_deferred_io()

Changes since v2:
- FB_DEFERRED_IO is now always selected by DRM_KMS_FB_HELPER, ifdef removed
- The drm_clip_rect utility functions are dropped, so open code it
- docs: use & to denote structs

Changes since v1:
- Use a dedicated worker to run the framebuffer flushing like qxl does
- Add parameter descriptions to drm_fb_helper_deferred_io

 drivers/gpu/drm/Kconfig         |   1 +
 drivers/gpu/drm/drm_fb_helper.c | 103 +++++++++++++++++++++++++++++++++++++++-
 include/drm/drm_fb_helper.h     |  15 ++++++
 3 files changed, 118 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 9e4f2f1..8e6f34b 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -52,6 +52,7 @@ config DRM_KMS_FB_HELPER
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
+	select FB_DEFERRED_IO
 	help
 	  FBDEV helpers for KMS drivers.

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 855108e..62f849f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -84,6 +84,15 @@ static LIST_HEAD(kernel_fb_helper_list);
  * and set up an initial configuration using the detected hardware, drivers
  * should call drm_fb_helper_single_add_all_connectors() followed by
  * drm_fb_helper_initial_config().
+ *
+ * If CONFIG_FB_DEFERRED_IO is enabled and &drm_framebuffer_funcs ->dirty is
+ * set, the drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit}
+ * functions will accumulate changes and schedule &fb_helper .dirty_work to run
+ * right away. This worker then calls the dirty() function ensuring that it
+ * will always run in process context since the fb_*() function could be
+ * running in atomic context. If drm_fb_helper_deferred_io() is used as the
+ * deferred_io callback it will also schedule dirty_work with the damage
+ * collected from the mmap page writes.
  */

 /**
@@ -637,6 +646,23 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 	kfree(helper->crtc_info);
 }

+static void drm_fb_helper_dirty_work(struct work_struct *work)
+{
+	struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper,
+						    dirty_work);
+	struct drm_clip_rect *clip = &helper->dirty_clip;
+	struct drm_clip_rect clip_copy;
+	unsigned long flags;
+
+	spin_lock_irqsave(&helper->dirty_lock, flags);
+	clip_copy = *clip;
+	clip->x1 = clip->y1 = ~0;
+	clip->x2 = clip->y2 = 0;
+	spin_unlock_irqrestore(&helper->dirty_lock, flags);
+
+	helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
+}
+
 /**
  * drm_fb_helper_prepare - setup a drm_fb_helper structure
  * @dev: DRM device
@@ -650,6 +676,9 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
 			   const struct drm_fb_helper_funcs *funcs)
 {
 	INIT_LIST_HEAD(&helper->kernel_fb_list);
+	spin_lock_init(&helper->dirty_lock);
+	INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work);
+	helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0;
 	helper->funcs = funcs;
 	helper->dev = dev;
 }
@@ -834,6 +863,59 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
 }
 EXPORT_SYMBOL(drm_fb_helper_unlink_fbi);

+static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
+				u32 width, u32 height)
+{
+	struct drm_fb_helper *helper = info->par;
+	struct drm_clip_rect *clip = &helper->dirty_clip;
+	unsigned long flags;
+
+	if (!helper->fb->funcs->dirty)
+		return;
+
+	spin_lock_irqsave(&helper->dirty_lock, flags);
+	clip->x1 = min_t(u32, clip->x1, x);
+	clip->y1 = min_t(u32, clip->y1, y);
+	clip->x2 = max_t(u32, clip->x2, x + width);
+	clip->y2 = max_t(u32, clip->y2, y + height);
+	spin_unlock_irqrestore(&helper->dirty_lock, flags);
+
+	schedule_work(&helper->dirty_work);
+}
+
+/**
+ * drm_fb_helper_deferred_io() - fbdev deferred_io callback function
+ * @info: fb_info struct pointer
+ * @pagelist: list of dirty mmap framebuffer pages
+ *
+ * This function is used as the &fb_deferred_io ->deferred_io
+ * callback function for flushing the fbdev mmap writes.
+ */
+void drm_fb_helper_deferred_io(struct fb_info *info,
+			       struct list_head *pagelist)
+{
+	unsigned long start, end, min, max;
+	struct page *page;
+	u32 y1, y2;
+
+	min = ULONG_MAX;
+	max = 0;
+	list_for_each_entry(page, pagelist, lru) {
+		start = page->index << PAGE_SHIFT;
+		end = start + PAGE_SIZE - 1;
+		min = min(min, start);
+		max = max(max, end);
+	}
+
+	if (min < max) {
+		y1 = min / info->fix.line_length;
+		y2 = min_t(u32, DIV_ROUND_UP(max, info->fix.line_length),
+			   info->var.yres);
+		drm_fb_helper_dirty(info, 0, y1, info->var.xres, y2 - y1);
+	}
+}
+EXPORT_SYMBOL(drm_fb_helper_deferred_io);
+
 /**
  * drm_fb_helper_sys_read - wrapper around fb_sys_read
  * @info: fb_info struct pointer
@@ -862,7 +944,14 @@ EXPORT_SYMBOL(drm_fb_helper_sys_read);
 ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
 				size_t count, loff_t *ppos)
 {
-	return fb_sys_write(info, buf, count, ppos);
+	ssize_t ret;
+
+	ret = fb_sys_write(info, buf, count, ppos);
+	if (ret > 0)
+		drm_fb_helper_dirty(info, 0, 0, info->var.xres,
+				    info->var.yres);
+
+	return ret;
 }
 EXPORT_SYMBOL(drm_fb_helper_sys_write);

@@ -877,6 +966,8 @@ void drm_fb_helper_sys_fillrect(struct fb_info *info,
 				const struct fb_fillrect *rect)
 {
 	sys_fillrect(info, rect);
+	drm_fb_helper_dirty(info, rect->dx, rect->dy,
+			    rect->width, rect->height);
 }
 EXPORT_SYMBOL(drm_fb_helper_sys_fillrect);

@@ -891,6 +982,8 @@ void drm_fb_helper_sys_copyarea(struct fb_info *info,
 				const struct fb_copyarea *area)
 {
 	sys_copyarea(info, area);
+	drm_fb_helper_dirty(info, area->dx, area->dy,
+			    area->width, area->height);
 }
 EXPORT_SYMBOL(drm_fb_helper_sys_copyarea);

@@ -905,6 +998,8 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
 				 const struct fb_image *image)
 {
 	sys_imageblit(info, image);
+	drm_fb_helper_dirty(info, image->dx, image->dy,
+			    image->width, image->height);
 }
 EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);

@@ -919,6 +1014,8 @@ void drm_fb_helper_cfb_fillrect(struct fb_info *info,
 				const struct fb_fillrect *rect)
 {
 	cfb_fillrect(info, rect);
+	drm_fb_helper_dirty(info, rect->dx, rect->dy,
+			    rect->width, rect->height);
 }
 EXPORT_SYMBOL(drm_fb_helper_cfb_fillrect);

@@ -933,6 +1030,8 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info,
 				const struct fb_copyarea *area)
 {
 	cfb_copyarea(info, area);
+	drm_fb_helper_dirty(info, area->dx, area->dy,
+			    area->width, area->height);
 }
 EXPORT_SYMBOL(drm_fb_helper_cfb_copyarea);

@@ -947,6 +1046,8 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
 				 const struct fb_image *image)
 {
 	cfb_imageblit(info, image);
+	drm_fb_helper_dirty(info, image->dx, image->dy,
+			    image->width, image->height);
 }
 EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 062723b..5b4aa35 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -172,6 +172,10 @@ struct drm_fb_helper_connector {
  * @funcs: driver callbacks for fb helper
  * @fbdev: emulated fbdev device info struct
  * @pseudo_palette: fake palette of 16 colors
+ * @dirty_clip: clip rectangle used with deferred_io to accumulate damage to
+ *              the screen buffer
+ * @dirty_lock: spinlock protecting @dirty_clip
+ * @dirty_work: worker used to flush the framebuffer
  *
  * This is the main structure used by the fbdev helpers. Drivers supporting
  * fbdev emulation should embedded this into their overall driver structure.
@@ -189,6 +193,9 @@ struct drm_fb_helper {
 	const struct drm_fb_helper_funcs *funcs;
 	struct fb_info *fbdev;
 	u32 pseudo_palette[17];
+	struct drm_clip_rect dirty_clip;
+	spinlock_t dirty_lock;
+	struct work_struct dirty_work;

 	/**
 	 * @kernel_fb_list:
@@ -245,6 +252,9 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,

 void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);

+void drm_fb_helper_deferred_io(struct fb_info *info,
+			       struct list_head *pagelist);
+
 ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
 			       size_t count, loff_t *ppos);
 ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
@@ -368,6 +378,11 @@ static inline void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
 {
 }

+static inline void drm_fb_helper_deferred_io(struct fb_info *info,
+					     struct list_head *pagelist)
+{
+}
+
 static inline ssize_t drm_fb_helper_sys_read(struct fb_info *info,
 					     char __user *buf, size_t count,
 					     loff_t *ppos)
--
2.2.2

[toc] | [prev] | [next] | [standalone]

#1391097 — Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support

[Multipart message — attachments visible in raw view] — view raw

Hi,

On 28/04/16 18:18, Noralf Trønnes wrote:
> This adds deferred io support to drm_fb_helper.
> The fbdev framebuffer changes are flushed using the callback
> (struct drm_framebuffer *)->funcs->dirty() by a dedicated worker
> ensuring that it always runs in process context.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---

Thanks for the series! Unfortunately I haven't been able to follow the
discussions properly, so I hope my questions haven't been covered earlier.

> Changes since v3:
> - Don't use forward decl, move drm_fb_helper_dirty_work()
> - Use DIV_ROUND_UP in drm_fb_helper_deferred_io()
> 
> Changes since v2:
> - FB_DEFERRED_IO is now always selected by DRM_KMS_FB_HELPER, ifdef removed
> - The drm_clip_rect utility functions are dropped, so open code it
> - docs: use & to denote structs
> 
> Changes since v1:
> - Use a dedicated worker to run the framebuffer flushing like qxl does
> - Add parameter descriptions to drm_fb_helper_deferred_io
> 
>  drivers/gpu/drm/Kconfig         |   1 +
>  drivers/gpu/drm/drm_fb_helper.c | 103 +++++++++++++++++++++++++++++++++++++++-
>  include/drm/drm_fb_helper.h     |  15 ++++++
>  3 files changed, 118 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 9e4f2f1..8e6f34b 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -52,6 +52,7 @@ config DRM_KMS_FB_HELPER
>  	select FB_CFB_FILLRECT
>  	select FB_CFB_COPYAREA
>  	select FB_CFB_IMAGEBLIT
> +	select FB_DEFERRED_IO
>  	help
>  	  FBDEV helpers for KMS drivers.
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 855108e..62f849f 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -84,6 +84,15 @@ static LIST_HEAD(kernel_fb_helper_list);
>   * and set up an initial configuration using the detected hardware, drivers
>   * should call drm_fb_helper_single_add_all_connectors() followed by
>   * drm_fb_helper_initial_config().
> + *
> + * If CONFIG_FB_DEFERRED_IO is enabled and &drm_framebuffer_funcs ->dirty is
> + * set, the drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit}
> + * functions will accumulate changes and schedule &fb_helper .dirty_work to run
> + * right away. This worker then calls the dirty() function ensuring that it
> + * will always run in process context since the fb_*() function could be
> + * running in atomic context. If drm_fb_helper_deferred_io() is used as the

Who's calling {write,fillrect,copyarea,imageblit} in an atomic context?
That sounds like a very bad idea to me...

If this is only for accumulating changes, I think it may be better to
leave that to the driver as it may have better idea of how to accumulate.

But, of course, this is a helper, so if all the drivers use this kind of
accumulation, it makes sense =).

 Tomi

[toc] | [prev] | [next] | [standalone]

#1391199 — Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support

Den 29.04.2016 14:50, skrev Tomi Valkeinen:
> Hi,
>
> On 28/04/16 18:18, Noralf Trønnes wrote:
>> This adds deferred io support to drm_fb_helper.
>> The fbdev framebuffer changes are flushed using the callback
>> (struct drm_framebuffer *)->funcs->dirty() by a dedicated worker
>> ensuring that it always runs in process context.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
> Thanks for the series! Unfortunately I haven't been able to follow the
> discussions properly, so I hope my questions haven't been covered earlier.
>
>> Changes since v3:
>> - Don't use forward decl, move drm_fb_helper_dirty_work()
>> - Use DIV_ROUND_UP in drm_fb_helper_deferred_io()
>>
>> Changes since v2:
>> - FB_DEFERRED_IO is now always selected by DRM_KMS_FB_HELPER, ifdef removed
>> - The drm_clip_rect utility functions are dropped, so open code it
>> - docs: use & to denote structs
>>
>> Changes since v1:
>> - Use a dedicated worker to run the framebuffer flushing like qxl does
>> - Add parameter descriptions to drm_fb_helper_deferred_io
>>
>>   drivers/gpu/drm/Kconfig         |   1 +
>>   drivers/gpu/drm/drm_fb_helper.c | 103 +++++++++++++++++++++++++++++++++++++++-
>>   include/drm/drm_fb_helper.h     |  15 ++++++
>>   3 files changed, 118 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 9e4f2f1..8e6f34b 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -52,6 +52,7 @@ config DRM_KMS_FB_HELPER
>>   	select FB_CFB_FILLRECT
>>   	select FB_CFB_COPYAREA
>>   	select FB_CFB_IMAGEBLIT
>> +	select FB_DEFERRED_IO
>>   	help
>>   	  FBDEV helpers for KMS drivers.
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 855108e..62f849f 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -84,6 +84,15 @@ static LIST_HEAD(kernel_fb_helper_list);
>>    * and set up an initial configuration using the detected hardware, drivers
>>    * should call drm_fb_helper_single_add_all_connectors() followed by
>>    * drm_fb_helper_initial_config().
>> + *
>> + * If CONFIG_FB_DEFERRED_IO is enabled and &drm_framebuffer_funcs ->dirty is
>> + * set, the drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit}
>> + * functions will accumulate changes and schedule &fb_helper .dirty_work to run
>> + * right away. This worker then calls the dirty() function ensuring that it
>> + * will always run in process context since the fb_*() function could be
>> + * running in atomic context. If drm_fb_helper_deferred_io() is used as the
> Who's calling {write,fillrect,copyarea,imageblit} in an atomic context?
> That sounds like a very bad idea to me...

I haven't verified it myself, but took it as fact based on
commit: bcb39af4486be07e896fc374a2336bad3104ae0a

drm/udl: make usage as a console safer
Okay you don't really want to use udl devices as your console, but if
you are unlucky enough to do so, you run into a lot of schedule while atomic
due to printk being called from all sorts of funky places. So check if we
are in an atomic context, and queue the damage for later, the next printk
should cause it to appear. This isn't ideal, but it is simple, and seems to
work okay in my testing here.

(dirty area idea came from xenfb)

fixes a bunch of sleeping while atomic issues running fbcon on udl devices.

Cc: stable@vger.kernel.org
Signed-off-by: Dave Airlie <airlied@redhat.com>

> If this is only for accumulating changes, I think it may be better to
> leave that to the driver as it may have better idea of how to accumulate.
>
> But, of course, this is a helper, so if all the drivers use this kind of
> accumulation, it makes sense =).

qxl already accumulates damage this way and upcoming tinydrm also.
udl has handled this damage inline except when in_atomic() which results
in damage being deferred to the next fb_*() call.

It is possible for drivers to have their own fb_*() handling and still
use drm_fb_helper_deferred_io().


For those who likes details, this is fbcon unblanking which results in a
full display update on a 320x240 display, shell with blinking cursor on
the last line of the console:

[ 5505.164150] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
[ 5505.164186] [drm:drm_atomic_state_init] Allocated atomic state b859e400
[...more drm atomic msgs...]
[ 5505.164713] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
[ 5505.164746] [drm:drm_atomic_state_init] Allocated atomic state b859e440
[...more drm atomic msgs...]
[ 5505.165086] drm_fb_helper_dirty: x=0,width=320,y=0,height=16
[ 5505.165153] drm_fb_helper_dirty: x=0,width=320,y=16,height=16
[ 5505.165220] drm_fb_helper_dirty: x=0,width=320,y=32,height=16
[ 5505.165287] drm_fb_helper_dirty: x=0,width=320,y=48,height=16
[ 5505.165354] drm_fb_helper_dirty: x=0,width=320,y=64,height=16
[ 5505.165420] drm_fb_helper_dirty: x=0,width=320,y=80,height=16
[ 5505.165487] drm_fb_helper_dirty: x=0,width=320,y=96,height=16
[ 5505.165553] drm_fb_helper_dirty: x=0,width=320,y=112,height=16
[ 5505.165619] drm_fb_helper_dirty: x=0,width=320,y=128,height=16
[ 5505.165686] drm_fb_helper_dirty: x=0,width=320,y=144,height=16
[ 5505.165752] drm_fb_helper_dirty: x=0,width=320,y=160,height=16
[ 5505.165818] drm_fb_helper_dirty: x=0,width=320,y=176,height=16
[ 5505.165884] drm_fb_helper_dirty: x=0,width=320,y=192,height=16
[ 5505.165949] drm_fb_helper_dirty: x=0,width=320,y=208,height=16
[ 5505.165978] drm_fb_helper_dirty: x=0,width=112,y=224,height=16
[ 5505.165988] drm_fb_helper_dirty: x=112,width=8,y=224,height=16
[ 5505.166002] drm_fb_helper_dirty: x=120,width=24,y=224,height=16
[ 5505.166041] drm_fb_helper_dirty: x=144,width=176,y=224,height=16
[ 5505.166058] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
[ 5505.166079] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
[ 5505.166424] drm_fb_helper_dirty_work: x1=0,x2=320,y1=0,y2=240
[ 5505.166452] adafruit-tft spi0.0: mipi_dbi_dirtyfb: vmem=bac40000, 
x1=0, x2=320, y1=0, y2=240

Cursor blinking:

[ 5505.363478] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
[ 5505.363509] drm_fb_helper_dirty_work: x1=152,x2=160,y1=224,y2=240
[ 5505.363539] adafruit-tft spi0.0: mipi_dbi_dirtyfb: vmem=bac40000, 
x1=152, x2=160, y1=224, y2=240

[ 5505.563488] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
[ 5505.563514] drm_fb_helper_dirty_work: x1=152,x2=160,y1=224,y2=240
[ 5505.563542] adafruit-tft spi0.0: mipi_dbi_dirtyfb: vmem=bac40000, 
x1=152, x2=160, y1=224, y2=240


Noralf.

[toc] | [prev] | [next] | [standalone]

#1392237 — Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support

On Fri, Apr 29, 2016 at 04:47:08PM +0200, Noralf Trønnes wrote:
> 
> Den 29.04.2016 14:50, skrev Tomi Valkeinen:
> >Hi,
> >
> >On 28/04/16 18:18, Noralf Trønnes wrote:
> >>This adds deferred io support to drm_fb_helper.
> >>The fbdev framebuffer changes are flushed using the callback
> >>(struct drm_framebuffer *)->funcs->dirty() by a dedicated worker
> >>ensuring that it always runs in process context.
> >>
> >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>---
> >Thanks for the series! Unfortunately I haven't been able to follow the
> >discussions properly, so I hope my questions haven't been covered earlier.
> >
> >>Changes since v3:
> >>- Don't use forward decl, move drm_fb_helper_dirty_work()
> >>- Use DIV_ROUND_UP in drm_fb_helper_deferred_io()
> >>
> >>Changes since v2:
> >>- FB_DEFERRED_IO is now always selected by DRM_KMS_FB_HELPER, ifdef removed
> >>- The drm_clip_rect utility functions are dropped, so open code it
> >>- docs: use & to denote structs
> >>
> >>Changes since v1:
> >>- Use a dedicated worker to run the framebuffer flushing like qxl does
> >>- Add parameter descriptions to drm_fb_helper_deferred_io
> >>
> >>  drivers/gpu/drm/Kconfig         |   1 +
> >>  drivers/gpu/drm/drm_fb_helper.c | 103 +++++++++++++++++++++++++++++++++++++++-
> >>  include/drm/drm_fb_helper.h     |  15 ++++++
> >>  3 files changed, 118 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> >>index 9e4f2f1..8e6f34b 100644
> >>--- a/drivers/gpu/drm/Kconfig
> >>+++ b/drivers/gpu/drm/Kconfig
> >>@@ -52,6 +52,7 @@ config DRM_KMS_FB_HELPER
> >>  	select FB_CFB_FILLRECT
> >>  	select FB_CFB_COPYAREA
> >>  	select FB_CFB_IMAGEBLIT
> >>+	select FB_DEFERRED_IO
> >>  	help
> >>  	  FBDEV helpers for KMS drivers.
> >>
> >>diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >>index 855108e..62f849f 100644
> >>--- a/drivers/gpu/drm/drm_fb_helper.c
> >>+++ b/drivers/gpu/drm/drm_fb_helper.c
> >>@@ -84,6 +84,15 @@ static LIST_HEAD(kernel_fb_helper_list);
> >>   * and set up an initial configuration using the detected hardware, drivers
> >>   * should call drm_fb_helper_single_add_all_connectors() followed by
> >>   * drm_fb_helper_initial_config().
> >>+ *
> >>+ * If CONFIG_FB_DEFERRED_IO is enabled and &drm_framebuffer_funcs ->dirty is
> >>+ * set, the drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit}
> >>+ * functions will accumulate changes and schedule &fb_helper .dirty_work to run
> >>+ * right away. This worker then calls the dirty() function ensuring that it
> >>+ * will always run in process context since the fb_*() function could be
> >>+ * running in atomic context. If drm_fb_helper_deferred_io() is used as the
> >Who's calling {write,fillrect,copyarea,imageblit} in an atomic context?
> >That sounds like a very bad idea to me...
> 
> I haven't verified it myself, but took it as fact based on
> commit: bcb39af4486be07e896fc374a2336bad3104ae0a

I've added a citation to this commit as the reason why we need to handle
atomic, to make sure Tomi's question is answered for posterity.
-Daniel

> 
> drm/udl: make usage as a console safer
> Okay you don't really want to use udl devices as your console, but if
> you are unlucky enough to do so, you run into a lot of schedule while atomic
> due to printk being called from all sorts of funky places. So check if we
> are in an atomic context, and queue the damage for later, the next printk
> should cause it to appear. This isn't ideal, but it is simple, and seems to
> work okay in my testing here.
> 
> (dirty area idea came from xenfb)
> 
> fixes a bunch of sleeping while atomic issues running fbcon on udl devices.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> 
> >If this is only for accumulating changes, I think it may be better to
> >leave that to the driver as it may have better idea of how to accumulate.
> >
> >But, of course, this is a helper, so if all the drivers use this kind of
> >accumulation, it makes sense =).
> 
> qxl already accumulates damage this way and upcoming tinydrm also.
> udl has handled this damage inline except when in_atomic() which results
> in damage being deferred to the next fb_*() call.
> 
> It is possible for drivers to have their own fb_*() handling and still
> use drm_fb_helper_deferred_io().
> 
> 
> For those who likes details, this is fbcon unblanking which results in a
> full display update on a 320x240 display, shell with blinking cursor on
> the last line of the console:
> 
> [ 5505.164150] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
> [ 5505.164186] [drm:drm_atomic_state_init] Allocated atomic state b859e400
> [...more drm atomic msgs...]
> [ 5505.164713] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
> [ 5505.164746] [drm:drm_atomic_state_init] Allocated atomic state b859e440
> [...more drm atomic msgs...]
> [ 5505.165086] drm_fb_helper_dirty: x=0,width=320,y=0,height=16
> [ 5505.165153] drm_fb_helper_dirty: x=0,width=320,y=16,height=16
> [ 5505.165220] drm_fb_helper_dirty: x=0,width=320,y=32,height=16
> [ 5505.165287] drm_fb_helper_dirty: x=0,width=320,y=48,height=16
> [ 5505.165354] drm_fb_helper_dirty: x=0,width=320,y=64,height=16
> [ 5505.165420] drm_fb_helper_dirty: x=0,width=320,y=80,height=16
> [ 5505.165487] drm_fb_helper_dirty: x=0,width=320,y=96,height=16
> [ 5505.165553] drm_fb_helper_dirty: x=0,width=320,y=112,height=16
> [ 5505.165619] drm_fb_helper_dirty: x=0,width=320,y=128,height=16
> [ 5505.165686] drm_fb_helper_dirty: x=0,width=320,y=144,height=16
> [ 5505.165752] drm_fb_helper_dirty: x=0,width=320,y=160,height=16
> [ 5505.165818] drm_fb_helper_dirty: x=0,width=320,y=176,height=16
> [ 5505.165884] drm_fb_helper_dirty: x=0,width=320,y=192,height=16
> [ 5505.165949] drm_fb_helper_dirty: x=0,width=320,y=208,height=16
> [ 5505.165978] drm_fb_helper_dirty: x=0,width=112,y=224,height=16
> [ 5505.165988] drm_fb_helper_dirty: x=112,width=8,y=224,height=16
> [ 5505.166002] drm_fb_helper_dirty: x=120,width=24,y=224,height=16
> [ 5505.166041] drm_fb_helper_dirty: x=144,width=176,y=224,height=16
> [ 5505.166058] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
> [ 5505.166079] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
> [ 5505.166424] drm_fb_helper_dirty_work: x1=0,x2=320,y1=0,y2=240
> [ 5505.166452] adafruit-tft spi0.0: mipi_dbi_dirtyfb: vmem=bac40000, x1=0,
> x2=320, y1=0, y2=240
> 
> Cursor blinking:
> 
> [ 5505.363478] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
> [ 5505.363509] drm_fb_helper_dirty_work: x1=152,x2=160,y1=224,y2=240
> [ 5505.363539] adafruit-tft spi0.0: mipi_dbi_dirtyfb: vmem=bac40000, x1=152,
> x2=160, y1=224, y2=240
> 
> [ 5505.563488] drm_fb_helper_dirty: x=152,width=8,y=224,height=16
> [ 5505.563514] drm_fb_helper_dirty_work: x1=152,x2=160,y1=224,y2=240
> [ 5505.563542] adafruit-tft spi0.0: mipi_dbi_dirtyfb: vmem=bac40000, x1=152,
> x2=160, y1=224, y2=240
> 
> 
> Noralf.
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1391206 — Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support

On Fri, Apr 29, 2016 at 03:50:40PM +0300, Tomi Valkeinen wrote:
> Hi,
> 
> On 28/04/16 18:18, Noralf Trønnes wrote:
> > This adds deferred io support to drm_fb_helper.
> > The fbdev framebuffer changes are flushed using the callback
> > (struct drm_framebuffer *)->funcs->dirty() by a dedicated worker
> > ensuring that it always runs in process context.
> > 
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> 
> Thanks for the series! Unfortunately I haven't been able to follow the
> discussions properly, so I hope my questions haven't been covered earlier.
> 
> > Changes since v3:
> > - Don't use forward decl, move drm_fb_helper_dirty_work()
> > - Use DIV_ROUND_UP in drm_fb_helper_deferred_io()
> > 
> > Changes since v2:
> > - FB_DEFERRED_IO is now always selected by DRM_KMS_FB_HELPER, ifdef removed
> > - The drm_clip_rect utility functions are dropped, so open code it
> > - docs: use & to denote structs
> > 
> > Changes since v1:
> > - Use a dedicated worker to run the framebuffer flushing like qxl does
> > - Add parameter descriptions to drm_fb_helper_deferred_io
> > 
> >  drivers/gpu/drm/Kconfig         |   1 +
> >  drivers/gpu/drm/drm_fb_helper.c | 103 +++++++++++++++++++++++++++++++++++++++-
> >  include/drm/drm_fb_helper.h     |  15 ++++++
> >  3 files changed, 118 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 9e4f2f1..8e6f34b 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -52,6 +52,7 @@ config DRM_KMS_FB_HELPER
> >  	select FB_CFB_FILLRECT
> >  	select FB_CFB_COPYAREA
> >  	select FB_CFB_IMAGEBLIT
> > +	select FB_DEFERRED_IO
> >  	help
> >  	  FBDEV helpers for KMS drivers.
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 855108e..62f849f 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -84,6 +84,15 @@ static LIST_HEAD(kernel_fb_helper_list);
> >   * and set up an initial configuration using the detected hardware, drivers
> >   * should call drm_fb_helper_single_add_all_connectors() followed by
> >   * drm_fb_helper_initial_config().
> > + *
> > + * If CONFIG_FB_DEFERRED_IO is enabled and &drm_framebuffer_funcs ->dirty is
> > + * set, the drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit}
> > + * functions will accumulate changes and schedule &fb_helper .dirty_work to run
> > + * right away. This worker then calls the dirty() function ensuring that it
> > + * will always run in process context since the fb_*() function could be
> > + * running in atomic context. If drm_fb_helper_deferred_io() is used as the
> 
> Who's calling {write,fillrect,copyarea,imageblit} in an atomic context?
> That sounds like a very bad idea to me...
> 
> If this is only for accumulating changes, I think it may be better to
> leave that to the driver as it may have better idea of how to accumulate.
> 
> But, of course, this is a helper, so if all the drivers use this kind of
> accumulation, it makes sense =).

Apparently panic context and cursor timer and stuff like that. I think
this started with udl, and just to make sure (it's fbdev after all, no one
wants to read the code itself) we've put those checks onto all entry
points that draw stuff. It could be overkill, but this is what udl/qxl
already do, so better to keep imo for now.

I guess if it's really not needed we could later on change that, but not
sure that's worth the effort. And we can't get rid of it entirely.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1391232 — Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support

[Multipart message — attachments visible in raw view] — view raw

On 29/04/16 17:55, Daniel Vetter wrote:

>> Who's calling {write,fillrect,copyarea,imageblit} in an atomic context?
>> That sounds like a very bad idea to me...
>>
>> If this is only for accumulating changes, I think it may be better to
>> leave that to the driver as it may have better idea of how to accumulate.
>>
>> But, of course, this is a helper, so if all the drivers use this kind of
>> accumulation, it makes sense =).
> 
> Apparently panic context and cursor timer and stuff like that. I think
> this started with udl, and just to make sure (it's fbdev after all, no one
> wants to read the code itself) we've put those checks onto all entry
> points that draw stuff. It could be overkill, but this is what udl/qxl
> already do, so better to keep imo for now.
> 
> I guess if it's really not needed we could later on change that, but not
> sure that's worth the effort. And we can't get rid of it entirely.

Yep... Sounds fine to me.

Someone should implement (minimal) fbdev userspace API support to DRM
without using the current fbdev, and implement fbcon on top of that. I
don't like how DRM and fbdev gets mixed...

I offer a beer to anyone who does that =).

 Tomi

[toc] | [prev] | [next] | [standalone]

#1391257 — Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support

Adding David, forgot that before hitting send.
-Daniel

On Fri, Apr 29, 2016 at 5:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Apr 29, 2016 at 06:00:18PM +0300, Tomi Valkeinen wrote:
>>
>> On 29/04/16 17:55, Daniel Vetter wrote:
>>
>> >> Who's calling {write,fillrect,copyarea,imageblit} in an atomic context?
>> >> That sounds like a very bad idea to me...
>> >>
>> >> If this is only for accumulating changes, I think it may be better to
>> >> leave that to the driver as it may have better idea of how to accumulate.
>> >>
>> >> But, of course, this is a helper, so if all the drivers use this kind of
>> >> accumulation, it makes sense =).
>> >
>> > Apparently panic context and cursor timer and stuff like that. I think
>> > this started with udl, and just to make sure (it's fbdev after all, no one
>> > wants to read the code itself) we've put those checks onto all entry
>> > points that draw stuff. It could be overkill, but this is what udl/qxl
>> > already do, so better to keep imo for now.
>> >
>> > I guess if it's really not needed we could later on change that, but not
>> > sure that's worth the effort. And we can't get rid of it entirely.
>>
>> Yep... Sounds fine to me.
>>
>> Someone should implement (minimal) fbdev userspace API support to DRM
>> without using the current fbdev, and implement fbcon on top of that. I
>> don't like how DRM and fbdev gets mixed...
>>
>> I offer a beer to anyone who does that =).
>
> The big plan that has been floated years back already (before David
> Herrmann disappeared into the rh systemd gang) was to have a pure
> userspace kmscon for console (including vt-switching in userspace like X
> does), plus a super-minimal panic console on top of kms in the kernel.
> That looked like a really solid design, unfornutately it didn't go
> anywhere. But basic patches are still around, and it really just boils
> down to that some distro would need to integrate kmscon properly, and
> polish of the kernel side too for integration.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1391259 — Re: [PATCH v4 3/7] drm/fb-helper: Add fb_deferred_io support

On Fri, Apr 29, 2016 at 06:00:18PM +0300, Tomi Valkeinen wrote:
> 
> On 29/04/16 17:55, Daniel Vetter wrote:
> 
> >> Who's calling {write,fillrect,copyarea,imageblit} in an atomic context?
> >> That sounds like a very bad idea to me...
> >>
> >> If this is only for accumulating changes, I think it may be better to
> >> leave that to the driver as it may have better idea of how to accumulate.
> >>
> >> But, of course, this is a helper, so if all the drivers use this kind of
> >> accumulation, it makes sense =).
> > 
> > Apparently panic context and cursor timer and stuff like that. I think
> > this started with udl, and just to make sure (it's fbdev after all, no one
> > wants to read the code itself) we've put those checks onto all entry
> > points that draw stuff. It could be overkill, but this is what udl/qxl
> > already do, so better to keep imo for now.
> > 
> > I guess if it's really not needed we could later on change that, but not
> > sure that's worth the effort. And we can't get rid of it entirely.
> 
> Yep... Sounds fine to me.
> 
> Someone should implement (minimal) fbdev userspace API support to DRM
> without using the current fbdev, and implement fbcon on top of that. I
> don't like how DRM and fbdev gets mixed...
> 
> I offer a beer to anyone who does that =).

The big plan that has been floated years back already (before David
Herrmann disappeared into the rh systemd gang) was to have a pure
userspace kmscon for console (including vt-switching in userspace like X
does), plus a super-minimal panic console on top of kms in the kernel.
That looked like a really solid design, unfornutately it didn't go
anywhere. But basic patches are still around, and it really just boils
down to that some distro would need to integrate kmscon properly, and
polish of the kernel side too for integration.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1390319 — [PATCH v4 4/7] fbdev: fb_defio: Export fb_deferred_io_mmap

Export fb_deferred_io_mmap so drivers can change vma->vm_page_prot.
When the framebuffer memory is allocated using dma_alloc_writecombine()
instead of vmalloc(), I get cache syncing problems on ARM.
This solves it:

static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
					  struct vm_area_struct *vma)
{
	fb_deferred_io_mmap(info, vma);
	vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);

	return 0;
}

Could this have been done in the core?
Drivers that don't set (struct fb_ops *)->fb_mmap, gets a call to
fb_pgprotect() at the end of the default fb_mmap implementation
(drivers/video/fbdev/core/fbmem.c). This is an architecture specific
function that on many platforms uses pgprot_writecombine(), but not on
all. And looking at some of the fb_mmap implementations, some of them
sets vm_page_prot to nocache for instance, so I think the safest bet is
to do this in the driver and not in the fbdev core. And we can't call
fb_pgprotect() from fb_deferred_io_mmap() either because we don't have
access to the file pointer that powerpc needs.

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

Changes since v1:
- Expand commit message

 drivers/video/fbdev/core/fb_defio.c | 3 ++-
 include/linux/fb.h                  | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 57721c7..74b5bca 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -164,7 +164,7 @@ static const struct address_space_operations fb_deferred_io_aops = {
 	.set_page_dirty = fb_deferred_io_set_page_dirty,
 };

-static int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
+int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	vma->vm_ops = &fb_deferred_io_vm_ops;
 	vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
@@ -173,6 +173,7 @@ static int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
 	vma->vm_private_data = info;
 	return 0;
 }
+EXPORT_SYMBOL(fb_deferred_io_mmap);

 /* workqueue callback */
 static void fb_deferred_io_work(struct work_struct *work)
diff --git a/include/linux/fb.h b/include/linux/fb.h
index dfe8835..a964d07 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -673,6 +673,7 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
 }

 /* drivers/video/fb_defio.c */
+int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
 extern void fb_deferred_io_init(struct fb_info *info);
 extern void fb_deferred_io_open(struct fb_info *info,
 				struct inode *inode,
--
2.2.2

[toc] | [prev] | [next] | [standalone]

#1391181 — Re: [PATCH v4 4/7] fbdev: fb_defio: Export fb_deferred_io_mmap

[Multipart message — attachments visible in raw view] — view raw

On 28/04/16 18:18, Noralf Trønnes wrote:
> Export fb_deferred_io_mmap so drivers can change vma->vm_page_prot.
> When the framebuffer memory is allocated using dma_alloc_writecombine()
> instead of vmalloc(), I get cache syncing problems on ARM.
> This solves it:
> 
> static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
> 					  struct vm_area_struct *vma)
> {
> 	fb_deferred_io_mmap(info, vma);
> 	vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> 
> 	return 0;
> }
> 
> Could this have been done in the core?
> Drivers that don't set (struct fb_ops *)->fb_mmap, gets a call to
> fb_pgprotect() at the end of the default fb_mmap implementation
> (drivers/video/fbdev/core/fbmem.c). This is an architecture specific
> function that on many platforms uses pgprot_writecombine(), but not on
> all. And looking at some of the fb_mmap implementations, some of them
> sets vm_page_prot to nocache for instance, so I think the safest bet is
> to do this in the driver and not in the fbdev core. And we can't call
> fb_pgprotect() from fb_deferred_io_mmap() either because we don't have
> access to the file pointer that powerpc needs.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi

[toc] | [prev] | [next] | [standalone]

#1391211

On Thu, Apr 28, 2016 at 05:18:30PM +0200, Noralf Trønnes wrote:
> This patchset adds fbdev deferred io support to drm_fb_helper and
> drm_fb_cma_helper.
> 
> It channels fbdev mmap and fb_{write,fillrect,copyarea,imageblit} damage
> through the (struct drm_framebuffer_funcs)->dirty callback on the
> fb_helper framebuffer which will always run in process context.
> 
> I have also added patches that converts qxl and udl to use this
> deferred io support. I have only compile tested it, no functional testing.
> I know that qxl is purely a software thing so I could actually test it, but
> I have never used qemu so I'm not keen on spending a lot of time on that.
> 
> This was originally part of the tinydrm patchset.
> 
> Changes since v3:
> - drm/fb-helper: Add fb_deferred_io support
>   - Don't use forward decl, move drm_fb_helper_dirty_work()
>   - Use DIV_ROUND_UP in drm_fb_helper_deferred_io()
> 
> Changes since v2:
> - drm/rect: Add some drm_clip_rect utility functions
>   - This patch is dropped
> - drm/fb-helper: Add fb_deferred_io support
>   - FB_DEFERRED_IO is now always selected by DRM_KMS_FB_HELPER, ifdef removed
>   - The drm_clip_rect utility functions are dropped, so open code it
>   - docs: use & to denote structs
> - drm/qxl: Use drm_fb_helper deferred_io support
>   - The drm_clip_rect_{width/height} functions are dropped, so open code it
> 
> Changes since v1:
> - drm/fb-helper: Add fb_deferred_io support
>   - Use a dedicated worker to run the framebuffer flushing like qxl does
>   - Add parameter descriptions to drm_fb_helper_deferred_io
> - fbdev: fb_defio: Export fb_deferred_io_mmap
>   - Expand commit message
> - drm/qxl: Use drm_fb_helper deferred_io support
>   - Add FIXME about special dirty() callback for fbdev
>   - Remove note in commit message about deferred worker, drm_fb_helper
>     is similar to qxl now.
> - drm/udl: Use drm_fb_helper deferred_io support
>   - No need to enable deferred_io by default since drm_fb_helper uses
>     a dedicated worker for flushing
> 
> Changes since RFC:
> - Fix drm_clip_rect use to be exclusive on x2/y2
> - Put drm_clip_rect functions in drm_rect.{h,c}
> - Take into account that (struct fb_ops *)->fb_{write,...}() can be called
>   from atomic context (spin_lock_irqsave)
> - Export fb_deferred_io_mmap()
> - Add some more documentation
> - Add qxl and udl patches
> 
> Noralf Trønnes (7):
>   drm/udl: Change drm_fb_helper_sys_*() calls to sys_*()
>   drm/qxl: Change drm_fb_helper_sys_*() calls to sys_*()
>   drm/fb-helper: Add fb_deferred_io support
>   fbdev: fb_defio: Export fb_deferred_io_mmap
>   drm/fb-cma-helper: Add fb_deferred_io support
>   drm/qxl: Use drm_fb_helper deferred_io support
>   drm/udl: Use drm_fb_helper deferred_io support

Scrolled through them all once more, and didn't spot anything else. Great
work! I plan to pull it all into drm-misc next week for 4.7 still. Please
ping me in case I forget.
-Daniel

> 
>  drivers/gpu/drm/Kconfig             |   1 +
>  drivers/gpu/drm/drm_fb_cma_helper.c | 178 ++++++++++++++++++++++++++--
>  drivers/gpu/drm/drm_fb_helper.c     | 103 ++++++++++++++++-
>  drivers/gpu/drm/qxl/qxl_display.c   |   9 +-
>  drivers/gpu/drm/qxl/qxl_drv.h       |   7 +-
>  drivers/gpu/drm/qxl/qxl_fb.c        | 223 +++++++++---------------------------
>  drivers/gpu/drm/qxl/qxl_kms.c       |   4 -
>  drivers/gpu/drm/udl/udl_drv.h       |   2 -
>  drivers/gpu/drm/udl/udl_fb.c        | 140 +---------------------
>  drivers/video/fbdev/core/fb_defio.c |   3 +-
>  include/drm/drm_fb_cma_helper.h     |  14 +++
>  include/drm/drm_fb_helper.h         |  15 +++
>  include/linux/fb.h                  |   1 +
>  13 files changed, 372 insertions(+), 328 deletions(-)
> 
> --
> 2.2.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1392236

On Fri, Apr 29, 2016 at 04:57:05PM +0200, Daniel Vetter wrote:
> On Thu, Apr 28, 2016 at 05:18:30PM +0200, Noralf Trønnes wrote:
> > This patchset adds fbdev deferred io support to drm_fb_helper and
> > drm_fb_cma_helper.
> > 
> > It channels fbdev mmap and fb_{write,fillrect,copyarea,imageblit} damage
> > through the (struct drm_framebuffer_funcs)->dirty callback on the
> > fb_helper framebuffer which will always run in process context.
> > 
> > I have also added patches that converts qxl and udl to use this
> > deferred io support. I have only compile tested it, no functional testing.
> > I know that qxl is purely a software thing so I could actually test it, but
> > I have never used qemu so I'm not keen on spending a lot of time on that.
> > 
> > This was originally part of the tinydrm patchset.
> > 
> > Changes since v3:
> > - drm/fb-helper: Add fb_deferred_io support
> >   - Don't use forward decl, move drm_fb_helper_dirty_work()
> >   - Use DIV_ROUND_UP in drm_fb_helper_deferred_io()
> > 
> > Changes since v2:
> > - drm/rect: Add some drm_clip_rect utility functions
> >   - This patch is dropped
> > - drm/fb-helper: Add fb_deferred_io support
> >   - FB_DEFERRED_IO is now always selected by DRM_KMS_FB_HELPER, ifdef removed
> >   - The drm_clip_rect utility functions are dropped, so open code it
> >   - docs: use & to denote structs
> > - drm/qxl: Use drm_fb_helper deferred_io support
> >   - The drm_clip_rect_{width/height} functions are dropped, so open code it
> > 
> > Changes since v1:
> > - drm/fb-helper: Add fb_deferred_io support
> >   - Use a dedicated worker to run the framebuffer flushing like qxl does
> >   - Add parameter descriptions to drm_fb_helper_deferred_io
> > - fbdev: fb_defio: Export fb_deferred_io_mmap
> >   - Expand commit message
> > - drm/qxl: Use drm_fb_helper deferred_io support
> >   - Add FIXME about special dirty() callback for fbdev
> >   - Remove note in commit message about deferred worker, drm_fb_helper
> >     is similar to qxl now.
> > - drm/udl: Use drm_fb_helper deferred_io support
> >   - No need to enable deferred_io by default since drm_fb_helper uses
> >     a dedicated worker for flushing
> > 
> > Changes since RFC:
> > - Fix drm_clip_rect use to be exclusive on x2/y2
> > - Put drm_clip_rect functions in drm_rect.{h,c}
> > - Take into account that (struct fb_ops *)->fb_{write,...}() can be called
> >   from atomic context (spin_lock_irqsave)
> > - Export fb_deferred_io_mmap()
> > - Add some more documentation
> > - Add qxl and udl patches
> > 
> > Noralf Trønnes (7):
> >   drm/udl: Change drm_fb_helper_sys_*() calls to sys_*()
> >   drm/qxl: Change drm_fb_helper_sys_*() calls to sys_*()
> >   drm/fb-helper: Add fb_deferred_io support
> >   fbdev: fb_defio: Export fb_deferred_io_mmap
> >   drm/fb-cma-helper: Add fb_deferred_io support
> >   drm/qxl: Use drm_fb_helper deferred_io support
> >   drm/udl: Use drm_fb_helper deferred_io support
> 
> Scrolled through them all once more, and didn't spot anything else. Great
> work! I plan to pull it all into drm-misc next week for 4.7 still. Please
> ping me in case I forget.

And done, thanks a lot for your work!

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web