Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-22 Thread Noralf Trønnes 


Den 22.04.2016 19:05, skrev Daniel Vetter:

On Fri, Apr 22, 2016 at 04:17:14PM +0200, Noralf Trønnes wrote:

Den 22.04.2016 10:27, skrev Daniel Vetter:

On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Trønnes wrote:

Den 20.04.2016 17:25, skrev Noralf Trønnes:

This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
Accumulated fbdev framebuffer changes are signaled using the callback
(struct drm_framebuffer_funcs *)->dirty()

The drm_fb_helper_sys_*() functions will accumulate changes and
schedule fb_info.deferred_work _if_ fb_info.fbdefio is set.
This worker is used by the deferred io mmap code to signal that it
has been collecting page faults. The page faults and/or other changes
are then merged into a drm_clip_rect and passed to the framebuffer
dirty() function.

The driver is responsible for setting up the fb_info.fbdefio structure
and calling fb_deferred_io_init() using the provided callback:
(struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;

Signed-off-by: Noralf Trønnes 
---
  drivers/gpu/drm/drm_fb_helper.c | 119 +++-
  include/drm/drm_fb_helper.h |  15 +
  2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c

[...]


+#ifdef CONFIG_FB_DEFERRED_IO
+/**
+ * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io 
callback
+ *   function
+ *
+ * This function always runs in process context (struct delayed_work) and calls
+ * the (struct drm_framebuffer_funcs)->dirty function with the collected
+ * damage. There's no need to worry about the possibility that the fb_sys_*()
+ * functions could be running in atomic context. They only schedule the
+ * delayed worker which then calls this deferred_io callback.
+ */
+void drm_fb_helper_deferred_io(struct fb_info *info,
+  struct list_head *pagelist)
+{
+   struct drm_fb_helper *helper = info->par;
+   unsigned long start, end, min, max;
+   struct drm_clip_rect clip;
+   unsigned long flags;
+   struct page *page;
+
+   if (!helper->fb->funcs->dirty)
+   return;
+
+   spin_lock_irqsave(>dirty_lock, flags);
+   clip = helper->dirty_clip;
+   drm_clip_rect_reset(>dirty_clip);
+   spin_unlock_irqrestore(>dirty_lock, flags);
+
+   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) {
+   struct drm_clip_rect mmap_clip;
+
+   mmap_clip.x1 = 0;
+   mmap_clip.x2 = info->var.xres;
+   mmap_clip.y1 = min / info->fix.line_length;
+   mmap_clip.y2 = min_t(u32, max / info->fix.line_length,
+info->var.yres);
+   drm_clip_rect_merge(, _clip, 1, 0, 0, 0);
+   }
+
+   if (!drm_clip_rect_is_empty())
+   helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, , 1);
+}
+EXPORT_SYMBOL(drm_fb_helper_deferred_io);

There is one thing I have wondered about when it comes to deferred io and
long run times for the defio worker with my displays:

Userspace writes to some pages then the deferred io worker kicks off and
runs for 100ms holding the page list mutex. While this is happening,
userspace writes to a new page triggering a page_mkwrite. Now this
function has to wait for the mutex to be released.

Who is actually waiting here and is there a problem that this can last
for 100ms?

No idea at all - I haven't looked that closely at  fbdev defio. But one
reason we have an explicit ioctl in drm to flush out frontbuffer rendering
is exactly that flushing could take some time, and should only be done
once userspace has completed some rendering. Not right in the middle of an
op.

I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl?
Otherwise you'll get to improve fbdev defio, and fbdev is deprecated
subsystem and a real horror show. I highly recommend against touching it
;-)

I have tried to track the call chain and it seems to be part of the
page fault handler. Which means it's userspace wanting to write to the
page that has to wait. And it has to wait at some random point in
whatever rendering it's doing.

Unless someone has any objections, I will make a change and add a worker
like qxl does. This decouples the deferred_io worker holding the mutex
from the framebuffer flushing job. However I intend to differ from qxl in
that I will use a delayed worker (run immediately from the mmap side which
has already been deferred). Since I don't see any point in flushing the
framebuffer immediately when fbcon has put out only one glyph, might as
well defer it a couple of jiffies to be able to capture some more glyphs.

Adding a worker

Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-22 Thread Noralf Trønnes 


Den 22.04.2016 19:05, skrev Daniel Vetter:

On Fri, Apr 22, 2016 at 04:17:14PM +0200, Noralf Trønnes wrote:

Den 22.04.2016 10:27, skrev Daniel Vetter:

On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Trønnes wrote:

Den 20.04.2016 17:25, skrev Noralf Trønnes:

This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
Accumulated fbdev framebuffer changes are signaled using the callback
(struct drm_framebuffer_funcs *)->dirty()

The drm_fb_helper_sys_*() functions will accumulate changes and
schedule fb_info.deferred_work _if_ fb_info.fbdefio is set.
This worker is used by the deferred io mmap code to signal that it
has been collecting page faults. The page faults and/or other changes
are then merged into a drm_clip_rect and passed to the framebuffer
dirty() function.

The driver is responsible for setting up the fb_info.fbdefio structure
and calling fb_deferred_io_init() using the provided callback:
(struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;

Signed-off-by: Noralf Trønnes 
---
  drivers/gpu/drm/drm_fb_helper.c | 119 +++-
  include/drm/drm_fb_helper.h |  15 +
  2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c

[...]


+#ifdef CONFIG_FB_DEFERRED_IO
+/**
+ * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io 
callback
+ *   function
+ *
+ * This function always runs in process context (struct delayed_work) and calls
+ * the (struct drm_framebuffer_funcs)->dirty function with the collected
+ * damage. There's no need to worry about the possibility that the fb_sys_*()
+ * functions could be running in atomic context. They only schedule the
+ * delayed worker which then calls this deferred_io callback.
+ */
+void drm_fb_helper_deferred_io(struct fb_info *info,
+  struct list_head *pagelist)
+{
+   struct drm_fb_helper *helper = info->par;
+   unsigned long start, end, min, max;
+   struct drm_clip_rect clip;
+   unsigned long flags;
+   struct page *page;
+
+   if (!helper->fb->funcs->dirty)
+   return;
+
+   spin_lock_irqsave(>dirty_lock, flags);
+   clip = helper->dirty_clip;
+   drm_clip_rect_reset(>dirty_clip);
+   spin_unlock_irqrestore(>dirty_lock, flags);
+
+   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) {
+   struct drm_clip_rect mmap_clip;
+
+   mmap_clip.x1 = 0;
+   mmap_clip.x2 = info->var.xres;
+   mmap_clip.y1 = min / info->fix.line_length;
+   mmap_clip.y2 = min_t(u32, max / info->fix.line_length,
+info->var.yres);
+   drm_clip_rect_merge(, _clip, 1, 0, 0, 0);
+   }
+
+   if (!drm_clip_rect_is_empty())
+   helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, , 1);
+}
+EXPORT_SYMBOL(drm_fb_helper_deferred_io);

There is one thing I have wondered about when it comes to deferred io and
long run times for the defio worker with my displays:

Userspace writes to some pages then the deferred io worker kicks off and
runs for 100ms holding the page list mutex. While this is happening,
userspace writes to a new page triggering a page_mkwrite. Now this
function has to wait for the mutex to be released.

Who is actually waiting here and is there a problem that this can last
for 100ms?

No idea at all - I haven't looked that closely at  fbdev defio. But one
reason we have an explicit ioctl in drm to flush out frontbuffer rendering
is exactly that flushing could take some time, and should only be done
once userspace has completed some rendering. Not right in the middle of an
op.

I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl?
Otherwise you'll get to improve fbdev defio, and fbdev is deprecated
subsystem and a real horror show. I highly recommend against touching it
;-)

I have tried to track the call chain and it seems to be part of the
page fault handler. Which means it's userspace wanting to write to the
page that has to wait. And it has to wait at some random point in
whatever rendering it's doing.

Unless someone has any objections, I will make a change and add a worker
like qxl does. This decouples the deferred_io worker holding the mutex
from the framebuffer flushing job. However I intend to differ from qxl in
that I will use a delayed worker (run immediately from the mmap side which
has already been deferred). Since I don't see any point in flushing the
framebuffer immediately when fbcon has put out only one glyph, might as
well defer it a couple of jiffies to be able to capture some more glyphs.

Adding a worker also means that udl

Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-22 Thread Daniel Vetter 
On Fri, Apr 22, 2016 at 7:28 PM, Noralf Trønnes  wrote:
> This patch extend the use of the fbdev deferred_io worker to also handle
> the fbdev drawing operations, which can happen in atomic context.
> The qxl driver adds an extra worker (struct qxl_device).fb_work which is
> used to flush the framebuffer. Both the mmap deferred_io (qxl_deferred_io())
> code which is run by the deferred io worker and the fbdev drawing operations
> (qxl_fb_fillrect() etc.) schedule this fb_work worker.
>
> So this patch uses 1 worker, qxl uses 2 workers.

Oh, I didn't realize that you're reusing the same worker for both
things. 2 workers indeed make sense, since the mmap one must have a
built-in delay (to coalesce a bunch of writes), whereas the other one
probably should be run right away, after each op.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-22 Thread Daniel Vetter 
On Fri, Apr 22, 2016 at 7:28 PM, Noralf Trønnes  wrote:
> This patch extend the use of the fbdev deferred_io worker to also handle
> the fbdev drawing operations, which can happen in atomic context.
> The qxl driver adds an extra worker (struct qxl_device).fb_work which is
> used to flush the framebuffer. Both the mmap deferred_io (qxl_deferred_io())
> code which is run by the deferred io worker and the fbdev drawing operations
> (qxl_fb_fillrect() etc.) schedule this fb_work worker.
>
> So this patch uses 1 worker, qxl uses 2 workers.

Oh, I didn't realize that you're reusing the same worker for both
things. 2 workers indeed make sense, since the mmap one must have a
built-in delay (to coalesce a bunch of writes), whereas the other one
probably should be run right away, after each op.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-22 Thread Daniel Vetter 
On Fri, Apr 22, 2016 at 04:17:14PM +0200, Noralf Trønnes wrote:
> 
> Den 22.04.2016 10:27, skrev Daniel Vetter:
> >On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Trønnes wrote:
> >>Den 20.04.2016 17:25, skrev Noralf Trønnes:
> >>>This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
> >>>Accumulated fbdev framebuffer changes are signaled using the callback
> >>>(struct drm_framebuffer_funcs *)->dirty()
> >>>
> >>>The drm_fb_helper_sys_*() functions will accumulate changes and
> >>>schedule fb_info.deferred_work _if_ fb_info.fbdefio is set.
> >>>This worker is used by the deferred io mmap code to signal that it
> >>>has been collecting page faults. The page faults and/or other changes
> >>>are then merged into a drm_clip_rect and passed to the framebuffer
> >>>dirty() function.
> >>>
> >>>The driver is responsible for setting up the fb_info.fbdefio structure
> >>>and calling fb_deferred_io_init() using the provided callback:
> >>>(struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;
> >>>
> >>>Signed-off-by: Noralf Trønnes 
> >>>---
> >>>  drivers/gpu/drm/drm_fb_helper.c | 119 
> >>> +++-
> >>>  include/drm/drm_fb_helper.h |  15 +
> >>>  2 files changed, 133 insertions(+), 1 deletion(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/drm_fb_helper.c 
> >>>b/drivers/gpu/drm/drm_fb_helper.c
> >>[...]
> >>
> >>>+#ifdef CONFIG_FB_DEFERRED_IO
> >>>+/**
> >>>+ * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io 
> >>>callback
> >>>+ *   function
> >>>+ *
> >>>+ * This function always runs in process context (struct delayed_work) and 
> >>>calls
> >>>+ * the (struct drm_framebuffer_funcs)->dirty function with the collected
> >>>+ * damage. There's no need to worry about the possibility that the 
> >>>fb_sys_*()
> >>>+ * functions could be running in atomic context. They only schedule the
> >>>+ * delayed worker which then calls this deferred_io callback.
> >>>+ */
> >>>+void drm_fb_helper_deferred_io(struct fb_info *info,
> >>>+ struct list_head *pagelist)
> >>>+{
> >>>+  struct drm_fb_helper *helper = info->par;
> >>>+  unsigned long start, end, min, max;
> >>>+  struct drm_clip_rect clip;
> >>>+  unsigned long flags;
> >>>+  struct page *page;
> >>>+
> >>>+  if (!helper->fb->funcs->dirty)
> >>>+  return;
> >>>+
> >>>+  spin_lock_irqsave(>dirty_lock, flags);
> >>>+  clip = helper->dirty_clip;
> >>>+  drm_clip_rect_reset(>dirty_clip);
> >>>+  spin_unlock_irqrestore(>dirty_lock, flags);
> >>>+
> >>>+  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) {
> >>>+  struct drm_clip_rect mmap_clip;
> >>>+
> >>>+  mmap_clip.x1 = 0;
> >>>+  mmap_clip.x2 = info->var.xres;
> >>>+  mmap_clip.y1 = min / info->fix.line_length;
> >>>+  mmap_clip.y2 = min_t(u32, max / info->fix.line_length,
> >>>+   info->var.yres);
> >>>+  drm_clip_rect_merge(, _clip, 1, 0, 0, 0);
> >>>+  }
> >>>+
> >>>+  if (!drm_clip_rect_is_empty())
> >>>+  helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, , 1);
> >>>+}
> >>>+EXPORT_SYMBOL(drm_fb_helper_deferred_io);
> >>There is one thing I have wondered about when it comes to deferred io and
> >>long run times for the defio worker with my displays:
> >>
> >>Userspace writes to some pages then the deferred io worker kicks off and
> >>runs for 100ms holding the page list mutex. While this is happening,
> >>userspace writes to a new page triggering a page_mkwrite. Now this
> >>function has to wait for the mutex to be released.
> >>
> >>Who is actually waiting here and is there a problem that this can last
> >>for 100ms?
> >No idea at all - I haven't looked that closely at  fbdev defio. But one
> >reason we have an explicit ioctl in drm to flush out frontbuffer rendering
> >is exactly that flushing could take some time, and should only be done
> >once userspace has completed some rendering. Not right in the middle of an
> >op.
> >
> >I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl?
> >Otherwise you'll get to improve fbdev defio, and fbdev is deprecated
> >subsystem and a real horror show. I highly recommend against touching it
> >;-)
> 
> I have tried to track the call chain and it seems to be part of the
> page fault handler. Which means it's userspace wanting to write to the
> page that has to wait. And it has to wait at some random point in
> whatever rendering it's doing.
> 
> Unless someone has any objections, I will make a change and add a worker
> like qxl does. This decouples the deferred_io worker holding the mutex
> from the framebuffer flushing job. However I intend to differ from qxl in
> that I

Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-22 Thread Daniel Vetter 
On Fri, Apr 22, 2016 at 04:17:14PM +0200, Noralf Trønnes wrote:
> 
> Den 22.04.2016 10:27, skrev Daniel Vetter:
> >On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Trønnes wrote:
> >>Den 20.04.2016 17:25, skrev Noralf Trønnes:
> >>>This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
> >>>Accumulated fbdev framebuffer changes are signaled using the callback
> >>>(struct drm_framebuffer_funcs *)->dirty()
> >>>
> >>>The drm_fb_helper_sys_*() functions will accumulate changes and
> >>>schedule fb_info.deferred_work _if_ fb_info.fbdefio is set.
> >>>This worker is used by the deferred io mmap code to signal that it
> >>>has been collecting page faults. The page faults and/or other changes
> >>>are then merged into a drm_clip_rect and passed to the framebuffer
> >>>dirty() function.
> >>>
> >>>The driver is responsible for setting up the fb_info.fbdefio structure
> >>>and calling fb_deferred_io_init() using the provided callback:
> >>>(struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;
> >>>
> >>>Signed-off-by: Noralf Trønnes 
> >>>---
> >>>  drivers/gpu/drm/drm_fb_helper.c | 119 
> >>> +++-
> >>>  include/drm/drm_fb_helper.h |  15 +
> >>>  2 files changed, 133 insertions(+), 1 deletion(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/drm_fb_helper.c 
> >>>b/drivers/gpu/drm/drm_fb_helper.c
> >>[...]
> >>
> >>>+#ifdef CONFIG_FB_DEFERRED_IO
> >>>+/**
> >>>+ * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io 
> >>>callback
> >>>+ *   function
> >>>+ *
> >>>+ * This function always runs in process context (struct delayed_work) and 
> >>>calls
> >>>+ * the (struct drm_framebuffer_funcs)->dirty function with the collected
> >>>+ * damage. There's no need to worry about the possibility that the 
> >>>fb_sys_*()
> >>>+ * functions could be running in atomic context. They only schedule the
> >>>+ * delayed worker which then calls this deferred_io callback.
> >>>+ */
> >>>+void drm_fb_helper_deferred_io(struct fb_info *info,
> >>>+ struct list_head *pagelist)
> >>>+{
> >>>+  struct drm_fb_helper *helper = info->par;
> >>>+  unsigned long start, end, min, max;
> >>>+  struct drm_clip_rect clip;
> >>>+  unsigned long flags;
> >>>+  struct page *page;
> >>>+
> >>>+  if (!helper->fb->funcs->dirty)
> >>>+  return;
> >>>+
> >>>+  spin_lock_irqsave(>dirty_lock, flags);
> >>>+  clip = helper->dirty_clip;
> >>>+  drm_clip_rect_reset(>dirty_clip);
> >>>+  spin_unlock_irqrestore(>dirty_lock, flags);
> >>>+
> >>>+  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) {
> >>>+  struct drm_clip_rect mmap_clip;
> >>>+
> >>>+  mmap_clip.x1 = 0;
> >>>+  mmap_clip.x2 = info->var.xres;
> >>>+  mmap_clip.y1 = min / info->fix.line_length;
> >>>+  mmap_clip.y2 = min_t(u32, max / info->fix.line_length,
> >>>+   info->var.yres);
> >>>+  drm_clip_rect_merge(, _clip, 1, 0, 0, 0);
> >>>+  }
> >>>+
> >>>+  if (!drm_clip_rect_is_empty())
> >>>+  helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, , 1);
> >>>+}
> >>>+EXPORT_SYMBOL(drm_fb_helper_deferred_io);
> >>There is one thing I have wondered about when it comes to deferred io and
> >>long run times for the defio worker with my displays:
> >>
> >>Userspace writes to some pages then the deferred io worker kicks off and
> >>runs for 100ms holding the page list mutex. While this is happening,
> >>userspace writes to a new page triggering a page_mkwrite. Now this
> >>function has to wait for the mutex to be released.
> >>
> >>Who is actually waiting here and is there a problem that this can last
> >>for 100ms?
> >No idea at all - I haven't looked that closely at  fbdev defio. But one
> >reason we have an explicit ioctl in drm to flush out frontbuffer rendering
> >is exactly that flushing could take some time, and should only be done
> >once userspace has completed some rendering. Not right in the middle of an
> >op.
> >
> >I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl?
> >Otherwise you'll get to improve fbdev defio, and fbdev is deprecated
> >subsystem and a real horror show. I highly recommend against touching it
> >;-)
> 
> I have tried to track the call chain and it seems to be part of the
> page fault handler. Which means it's userspace wanting to write to the
> page that has to wait. And it has to wait at some random point in
> whatever rendering it's doing.
> 
> Unless someone has any objections, I will make a change and add a worker
> like qxl does. This decouples the deferred_io worker holding the mutex
> from the framebuffer flushing job. However I intend to differ from qxl in
> that I will use a delayed

Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-22 Thread Noralf Trønnes 


Den 22.04.2016 10:27, skrev Daniel Vetter:

On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Trønnes wrote:

Den 20.04.2016 17:25, skrev Noralf Trønnes:

This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
Accumulated fbdev framebuffer changes are signaled using the callback
(struct drm_framebuffer_funcs *)->dirty()

The drm_fb_helper_sys_*() functions will accumulate changes and
schedule fb_info.deferred_work _if_ fb_info.fbdefio is set.
This worker is used by the deferred io mmap code to signal that it
has been collecting page faults. The page faults and/or other changes
are then merged into a drm_clip_rect and passed to the framebuffer
dirty() function.

The driver is responsible for setting up the fb_info.fbdefio structure
and calling fb_deferred_io_init() using the provided callback:
(struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;

Signed-off-by: Noralf Trønnes 
---
  drivers/gpu/drm/drm_fb_helper.c | 119 +++-
  include/drm/drm_fb_helper.h |  15 +
  2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c

[...]


+#ifdef CONFIG_FB_DEFERRED_IO
+/**
+ * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io 
callback
+ *   function
+ *
+ * This function always runs in process context (struct delayed_work) and calls
+ * the (struct drm_framebuffer_funcs)->dirty function with the collected
+ * damage. There's no need to worry about the possibility that the fb_sys_*()
+ * functions could be running in atomic context. They only schedule the
+ * delayed worker which then calls this deferred_io callback.
+ */
+void drm_fb_helper_deferred_io(struct fb_info *info,
+  struct list_head *pagelist)
+{
+   struct drm_fb_helper *helper = info->par;
+   unsigned long start, end, min, max;
+   struct drm_clip_rect clip;
+   unsigned long flags;
+   struct page *page;
+
+   if (!helper->fb->funcs->dirty)
+   return;
+
+   spin_lock_irqsave(>dirty_lock, flags);
+   clip = helper->dirty_clip;
+   drm_clip_rect_reset(>dirty_clip);
+   spin_unlock_irqrestore(>dirty_lock, flags);
+
+   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) {
+   struct drm_clip_rect mmap_clip;
+
+   mmap_clip.x1 = 0;
+   mmap_clip.x2 = info->var.xres;
+   mmap_clip.y1 = min / info->fix.line_length;
+   mmap_clip.y2 = min_t(u32, max / info->fix.line_length,
+info->var.yres);
+   drm_clip_rect_merge(, _clip, 1, 0, 0, 0);
+   }
+
+   if (!drm_clip_rect_is_empty())
+   helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, , 1);
+}
+EXPORT_SYMBOL(drm_fb_helper_deferred_io);

There is one thing I have wondered about when it comes to deferred io and
long run times for the defio worker with my displays:

Userspace writes to some pages then the deferred io worker kicks off and
runs for 100ms holding the page list mutex. While this is happening,
userspace writes to a new page triggering a page_mkwrite. Now this
function has to wait for the mutex to be released.

Who is actually waiting here and is there a problem that this can last
for 100ms?

No idea at all - I haven't looked that closely at  fbdev defio. But one
reason we have an explicit ioctl in drm to flush out frontbuffer rendering
is exactly that flushing could take some time, and should only be done
once userspace has completed some rendering. Not right in the middle of an
op.

I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl?
Otherwise you'll get to improve fbdev defio, and fbdev is deprecated
subsystem and a real horror show. I highly recommend against touching it
;-)


I have tried to track the call chain and it seems to be part of the
page fault handler. Which means it's userspace wanting to write to the
page that has to wait. And it has to wait at some random point in
whatever rendering it's doing.

Unless someone has any objections, I will make a change and add a worker
like qxl does. This decouples the deferred_io worker holding the mutex
from the framebuffer flushing job. However I intend to differ from qxl in
that I will use a delayed worker (run immediately from the mmap side which
has already been deferred). Since I don't see any point in flushing the
framebuffer immediately when fbcon has put out only one glyph, might as
well defer it a couple of jiffies to be able to capture some more glyphs.

Adding a worker also means that udl doesn't have to initialize deferred_io
because we won't be using the deferred_work

Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-22 Thread Noralf Trønnes 


Den 22.04.2016 10:27, skrev Daniel Vetter:

On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Trønnes wrote:

Den 20.04.2016 17:25, skrev Noralf Trønnes:

This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
Accumulated fbdev framebuffer changes are signaled using the callback
(struct drm_framebuffer_funcs *)->dirty()

The drm_fb_helper_sys_*() functions will accumulate changes and
schedule fb_info.deferred_work _if_ fb_info.fbdefio is set.
This worker is used by the deferred io mmap code to signal that it
has been collecting page faults. The page faults and/or other changes
are then merged into a drm_clip_rect and passed to the framebuffer
dirty() function.

The driver is responsible for setting up the fb_info.fbdefio structure
and calling fb_deferred_io_init() using the provided callback:
(struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;

Signed-off-by: Noralf Trønnes 
---
  drivers/gpu/drm/drm_fb_helper.c | 119 +++-
  include/drm/drm_fb_helper.h |  15 +
  2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c

[...]


+#ifdef CONFIG_FB_DEFERRED_IO
+/**
+ * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io 
callback
+ *   function
+ *
+ * This function always runs in process context (struct delayed_work) and calls
+ * the (struct drm_framebuffer_funcs)->dirty function with the collected
+ * damage. There's no need to worry about the possibility that the fb_sys_*()
+ * functions could be running in atomic context. They only schedule the
+ * delayed worker which then calls this deferred_io callback.
+ */
+void drm_fb_helper_deferred_io(struct fb_info *info,
+  struct list_head *pagelist)
+{
+   struct drm_fb_helper *helper = info->par;
+   unsigned long start, end, min, max;
+   struct drm_clip_rect clip;
+   unsigned long flags;
+   struct page *page;
+
+   if (!helper->fb->funcs->dirty)
+   return;
+
+   spin_lock_irqsave(>dirty_lock, flags);
+   clip = helper->dirty_clip;
+   drm_clip_rect_reset(>dirty_clip);
+   spin_unlock_irqrestore(>dirty_lock, flags);
+
+   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) {
+   struct drm_clip_rect mmap_clip;
+
+   mmap_clip.x1 = 0;
+   mmap_clip.x2 = info->var.xres;
+   mmap_clip.y1 = min / info->fix.line_length;
+   mmap_clip.y2 = min_t(u32, max / info->fix.line_length,
+info->var.yres);
+   drm_clip_rect_merge(, _clip, 1, 0, 0, 0);
+   }
+
+   if (!drm_clip_rect_is_empty())
+   helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, , 1);
+}
+EXPORT_SYMBOL(drm_fb_helper_deferred_io);

There is one thing I have wondered about when it comes to deferred io and
long run times for the defio worker with my displays:

Userspace writes to some pages then the deferred io worker kicks off and
runs for 100ms holding the page list mutex. While this is happening,
userspace writes to a new page triggering a page_mkwrite. Now this
function has to wait for the mutex to be released.

Who is actually waiting here and is there a problem that this can last
for 100ms?

No idea at all - I haven't looked that closely at  fbdev defio. But one
reason we have an explicit ioctl in drm to flush out frontbuffer rendering
is exactly that flushing could take some time, and should only be done
once userspace has completed some rendering. Not right in the middle of an
op.

I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl?
Otherwise you'll get to improve fbdev defio, and fbdev is deprecated
subsystem and a real horror show. I highly recommend against touching it
;-)


I have tried to track the call chain and it seems to be part of the
page fault handler. Which means it's userspace wanting to write to the
page that has to wait. And it has to wait at some random point in
whatever rendering it's doing.

Unless someone has any objections, I will make a change and add a worker
like qxl does. This decouples the deferred_io worker holding the mutex
from the framebuffer flushing job. However I intend to differ from qxl in
that I will use a delayed worker (run immediately from the mmap side which
has already been deferred). Since I don't see any point in flushing the
framebuffer immediately when fbcon has put out only one glyph, might as
well defer it a couple of jiffies to be able to capture some more glyphs.

Adding a worker also means that udl doesn't have to initialize deferred_io
because we won't be using the deferred_work worker for flushing

Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-22 Thread Daniel Vetter 
On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Trønnes wrote:
> 
> Den 20.04.2016 17:25, skrev Noralf Trønnes:
> >This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
> >Accumulated fbdev framebuffer changes are signaled using the callback
> >(struct drm_framebuffer_funcs *)->dirty()
> >
> >The drm_fb_helper_sys_*() functions will accumulate changes and
> >schedule fb_info.deferred_work _if_ fb_info.fbdefio is set.
> >This worker is used by the deferred io mmap code to signal that it
> >has been collecting page faults. The page faults and/or other changes
> >are then merged into a drm_clip_rect and passed to the framebuffer
> >dirty() function.
> >
> >The driver is responsible for setting up the fb_info.fbdefio structure
> >and calling fb_deferred_io_init() using the provided callback:
> >(struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;
> >
> >Signed-off-by: Noralf Trønnes 
> >---
> >  drivers/gpu/drm/drm_fb_helper.c | 119 
> > +++-
> >  include/drm/drm_fb_helper.h |  15 +
> >  2 files changed, 133 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/drm_fb_helper.c 
> >b/drivers/gpu/drm/drm_fb_helper.c
> 
> [...]
> 
> >+#ifdef CONFIG_FB_DEFERRED_IO
> >+/**
> >+ * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io 
> >callback
> >+ *   function
> >+ *
> >+ * This function always runs in process context (struct delayed_work) and 
> >calls
> >+ * the (struct drm_framebuffer_funcs)->dirty function with the collected
> >+ * damage. There's no need to worry about the possibility that the 
> >fb_sys_*()
> >+ * functions could be running in atomic context. They only schedule the
> >+ * delayed worker which then calls this deferred_io callback.
> >+ */
> >+void drm_fb_helper_deferred_io(struct fb_info *info,
> >+   struct list_head *pagelist)
> >+{
> >+struct drm_fb_helper *helper = info->par;
> >+unsigned long start, end, min, max;
> >+struct drm_clip_rect clip;
> >+unsigned long flags;
> >+struct page *page;
> >+
> >+if (!helper->fb->funcs->dirty)
> >+return;
> >+
> >+spin_lock_irqsave(>dirty_lock, flags);
> >+clip = helper->dirty_clip;
> >+drm_clip_rect_reset(>dirty_clip);
> >+spin_unlock_irqrestore(>dirty_lock, flags);
> >+
> >+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) {
> >+struct drm_clip_rect mmap_clip;
> >+
> >+mmap_clip.x1 = 0;
> >+mmap_clip.x2 = info->var.xres;
> >+mmap_clip.y1 = min / info->fix.line_length;
> >+mmap_clip.y2 = min_t(u32, max / info->fix.line_length,
> >+ info->var.yres);
> >+drm_clip_rect_merge(, _clip, 1, 0, 0, 0);
> >+}
> >+
> >+if (!drm_clip_rect_is_empty())
> >+helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, , 1);
> >+}
> >+EXPORT_SYMBOL(drm_fb_helper_deferred_io);
> 
> There is one thing I have wondered about when it comes to deferred io and
> long run times for the defio worker with my displays:
> 
> Userspace writes to some pages then the deferred io worker kicks off and
> runs for 100ms holding the page list mutex. While this is happening,
> userspace writes to a new page triggering a page_mkwrite. Now this
> function has to wait for the mutex to be released.
> 
> Who is actually waiting here and is there a problem that this can last
> for 100ms?

No idea at all - I haven't looked that closely at  fbdev defio. But one
reason we have an explicit ioctl in drm to flush out frontbuffer rendering
is exactly that flushing could take some time, and should only be done
once userspace has completed some rendering. Not right in the middle of an
op.

I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl?
Otherwise you'll get to improve fbdev defio, and fbdev is deprecated
subsystem and a real horror show. I highly recommend against touching it
;-)

Cheers, Daniel

> 
> Excerpt from drivers/video/fbdev/core/fb_defio.c:
> 
> /* vm_ops->page_mkwrite handler */
> static int fb_deferred_io_mkwrite(struct vm_area_struct *vma,
>   struct vm_fault *vmf)
> {
> ...
> /* this is a callback we get when userspace first tries to
> write to the page. we schedule a workqueue. that workqueue
> will eventually mkclean the touched pages and execute the
> deferred framebuffer IO. then if userspace touches a page
> again, we repeat the same scheme */
> ...
> /* protect against the workqueue changing the page list */
> mutex_lock(>lock);
> ...
> /*
>  * We want the page to remain locked from ->page_mkwrite until
>  * the PTE is marked dirty

Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-22 Thread Daniel Vetter 
On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Trønnes wrote:
> 
> Den 20.04.2016 17:25, skrev Noralf Trønnes:
> >This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
> >Accumulated fbdev framebuffer changes are signaled using the callback
> >(struct drm_framebuffer_funcs *)->dirty()
> >
> >The drm_fb_helper_sys_*() functions will accumulate changes and
> >schedule fb_info.deferred_work _if_ fb_info.fbdefio is set.
> >This worker is used by the deferred io mmap code to signal that it
> >has been collecting page faults. The page faults and/or other changes
> >are then merged into a drm_clip_rect and passed to the framebuffer
> >dirty() function.
> >
> >The driver is responsible for setting up the fb_info.fbdefio structure
> >and calling fb_deferred_io_init() using the provided callback:
> >(struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;
> >
> >Signed-off-by: Noralf Trønnes 
> >---
> >  drivers/gpu/drm/drm_fb_helper.c | 119 
> > +++-
> >  include/drm/drm_fb_helper.h |  15 +
> >  2 files changed, 133 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/drm_fb_helper.c 
> >b/drivers/gpu/drm/drm_fb_helper.c
> 
> [...]
> 
> >+#ifdef CONFIG_FB_DEFERRED_IO
> >+/**
> >+ * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io 
> >callback
> >+ *   function
> >+ *
> >+ * This function always runs in process context (struct delayed_work) and 
> >calls
> >+ * the (struct drm_framebuffer_funcs)->dirty function with the collected
> >+ * damage. There's no need to worry about the possibility that the 
> >fb_sys_*()
> >+ * functions could be running in atomic context. They only schedule the
> >+ * delayed worker which then calls this deferred_io callback.
> >+ */
> >+void drm_fb_helper_deferred_io(struct fb_info *info,
> >+   struct list_head *pagelist)
> >+{
> >+struct drm_fb_helper *helper = info->par;
> >+unsigned long start, end, min, max;
> >+struct drm_clip_rect clip;
> >+unsigned long flags;
> >+struct page *page;
> >+
> >+if (!helper->fb->funcs->dirty)
> >+return;
> >+
> >+spin_lock_irqsave(>dirty_lock, flags);
> >+clip = helper->dirty_clip;
> >+drm_clip_rect_reset(>dirty_clip);
> >+spin_unlock_irqrestore(>dirty_lock, flags);
> >+
> >+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) {
> >+struct drm_clip_rect mmap_clip;
> >+
> >+mmap_clip.x1 = 0;
> >+mmap_clip.x2 = info->var.xres;
> >+mmap_clip.y1 = min / info->fix.line_length;
> >+mmap_clip.y2 = min_t(u32, max / info->fix.line_length,
> >+ info->var.yres);
> >+drm_clip_rect_merge(, _clip, 1, 0, 0, 0);
> >+}
> >+
> >+if (!drm_clip_rect_is_empty())
> >+helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, , 1);
> >+}
> >+EXPORT_SYMBOL(drm_fb_helper_deferred_io);
> 
> There is one thing I have wondered about when it comes to deferred io and
> long run times for the defio worker with my displays:
> 
> Userspace writes to some pages then the deferred io worker kicks off and
> runs for 100ms holding the page list mutex. While this is happening,
> userspace writes to a new page triggering a page_mkwrite. Now this
> function has to wait for the mutex to be released.
> 
> Who is actually waiting here and is there a problem that this can last
> for 100ms?

No idea at all - I haven't looked that closely at  fbdev defio. But one
reason we have an explicit ioctl in drm to flush out frontbuffer rendering
is exactly that flushing could take some time, and should only be done
once userspace has completed some rendering. Not right in the middle of an
op.

I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl?
Otherwise you'll get to improve fbdev defio, and fbdev is deprecated
subsystem and a real horror show. I highly recommend against touching it
;-)

Cheers, Daniel

> 
> Excerpt from drivers/video/fbdev/core/fb_defio.c:
> 
> /* vm_ops->page_mkwrite handler */
> static int fb_deferred_io_mkwrite(struct vm_area_struct *vma,
>   struct vm_fault *vmf)
> {
> ...
> /* this is a callback we get when userspace first tries to
> write to the page. we schedule a workqueue. that workqueue
> will eventually mkclean the touched pages and execute the
> deferred framebuffer IO. then if userspace touches a page
> again, we repeat the same scheme */
> ...
> /* protect against the workqueue changing the page list */
> mutex_lock(>lock);
> ...
> /*
>  * We want the page to remain locked from ->page_mkwrite until
>  * the PTE is marked dirty to avoid

Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-21 Thread Noralf Trønnes 


Den 20.04.2016 17:25, skrev Noralf Trønnes:

This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
Accumulated fbdev framebuffer changes are signaled using the callback
(struct drm_framebuffer_funcs *)->dirty()

The drm_fb_helper_sys_*() functions will accumulate changes and
schedule fb_info.deferred_work _if_ fb_info.fbdefio is set.
This worker is used by the deferred io mmap code to signal that it
has been collecting page faults. The page faults and/or other changes
are then merged into a drm_clip_rect and passed to the framebuffer
dirty() function.

The driver is responsible for setting up the fb_info.fbdefio structure
and calling fb_deferred_io_init() using the provided callback:
(struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;

Signed-off-by: Noralf Trønnes 
---
  drivers/gpu/drm/drm_fb_helper.c | 119 +++-
  include/drm/drm_fb_helper.h |  15 +
  2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c


[...]


+#ifdef CONFIG_FB_DEFERRED_IO
+/**
+ * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io 
callback
+ *   function
+ *
+ * This function always runs in process context (struct delayed_work) and calls
+ * the (struct drm_framebuffer_funcs)->dirty function with the collected
+ * damage. There's no need to worry about the possibility that the fb_sys_*()
+ * functions could be running in atomic context. They only schedule the
+ * delayed worker which then calls this deferred_io callback.
+ */
+void drm_fb_helper_deferred_io(struct fb_info *info,
+  struct list_head *pagelist)
+{
+   struct drm_fb_helper *helper = info->par;
+   unsigned long start, end, min, max;
+   struct drm_clip_rect clip;
+   unsigned long flags;
+   struct page *page;
+
+   if (!helper->fb->funcs->dirty)
+   return;
+
+   spin_lock_irqsave(>dirty_lock, flags);
+   clip = helper->dirty_clip;
+   drm_clip_rect_reset(>dirty_clip);
+   spin_unlock_irqrestore(>dirty_lock, flags);
+
+   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) {
+   struct drm_clip_rect mmap_clip;
+
+   mmap_clip.x1 = 0;
+   mmap_clip.x2 = info->var.xres;
+   mmap_clip.y1 = min / info->fix.line_length;
+   mmap_clip.y2 = min_t(u32, max / info->fix.line_length,
+info->var.yres);
+   drm_clip_rect_merge(, _clip, 1, 0, 0, 0);
+   }
+
+   if (!drm_clip_rect_is_empty())
+   helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, , 1);
+}
+EXPORT_SYMBOL(drm_fb_helper_deferred_io);


There is one thing I have wondered about when it comes to deferred io and
long run times for the defio worker with my displays:

Userspace writes to some pages then the deferred io worker kicks off and
runs for 100ms holding the page list mutex. While this is happening,
userspace writes to a new page triggering a page_mkwrite. Now this
function has to wait for the mutex to be released.

Who is actually waiting here and is there a problem that this can last
for 100ms?

Excerpt from drivers/video/fbdev/core/fb_defio.c:

/* vm_ops->page_mkwrite handler */
static int fb_deferred_io_mkwrite(struct vm_area_struct *vma,
  struct vm_fault *vmf)
{
...
/* this is a callback we get when userspace first tries to
write to the page. we schedule a workqueue. that workqueue
will eventually mkclean the touched pages and execute the
deferred framebuffer IO. then if userspace touches a page
again, we repeat the same scheme */
...
/* protect against the workqueue changing the page list */
mutex_lock(>lock);
...
/*
 * We want the page to remain locked from ->page_mkwrite until
 * the PTE is marked dirty to avoid page_mkclean() being called
 * before the PTE is updated, which would leave the page ignored
 * by defio.
 * Do this by locking the page here and informing the caller
 * about it with VM_FAULT_LOCKED.
 */
lock_page(page);

/* we loop through the pagelist before adding in order
to keep the pagelist sorted */
list_for_each_entry(cur, >pagelist, lru) {
/* this check is to catch the case where a new
process could start writing to the same page
through a new pte. this new access can cause the
mkwrite even when the original ps's pte is marked
writable */
if (unlikely(cur == page))
goto page_already_added;
else if (cur->index > page->index)
break;
}

list_add_tail(>lru, >lru);

Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-21 Thread Noralf Trønnes 


Den 20.04.2016 17:25, skrev Noralf Trønnes:

This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled.
Accumulated fbdev framebuffer changes are signaled using the callback
(struct drm_framebuffer_funcs *)->dirty()

The drm_fb_helper_sys_*() functions will accumulate changes and
schedule fb_info.deferred_work _if_ fb_info.fbdefio is set.
This worker is used by the deferred io mmap code to signal that it
has been collecting page faults. The page faults and/or other changes
are then merged into a drm_clip_rect and passed to the framebuffer
dirty() function.

The driver is responsible for setting up the fb_info.fbdefio structure
and calling fb_deferred_io_init() using the provided callback:
(struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io;

Signed-off-by: Noralf Trønnes 
---
  drivers/gpu/drm/drm_fb_helper.c | 119 +++-
  include/drm/drm_fb_helper.h |  15 +
  2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c


[...]


+#ifdef CONFIG_FB_DEFERRED_IO
+/**
+ * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io 
callback
+ *   function
+ *
+ * This function always runs in process context (struct delayed_work) and calls
+ * the (struct drm_framebuffer_funcs)->dirty function with the collected
+ * damage. There's no need to worry about the possibility that the fb_sys_*()
+ * functions could be running in atomic context. They only schedule the
+ * delayed worker which then calls this deferred_io callback.
+ */
+void drm_fb_helper_deferred_io(struct fb_info *info,
+  struct list_head *pagelist)
+{
+   struct drm_fb_helper *helper = info->par;
+   unsigned long start, end, min, max;
+   struct drm_clip_rect clip;
+   unsigned long flags;
+   struct page *page;
+
+   if (!helper->fb->funcs->dirty)
+   return;
+
+   spin_lock_irqsave(>dirty_lock, flags);
+   clip = helper->dirty_clip;
+   drm_clip_rect_reset(>dirty_clip);
+   spin_unlock_irqrestore(>dirty_lock, flags);
+
+   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) {
+   struct drm_clip_rect mmap_clip;
+
+   mmap_clip.x1 = 0;
+   mmap_clip.x2 = info->var.xres;
+   mmap_clip.y1 = min / info->fix.line_length;
+   mmap_clip.y2 = min_t(u32, max / info->fix.line_length,
+info->var.yres);
+   drm_clip_rect_merge(, _clip, 1, 0, 0, 0);
+   }
+
+   if (!drm_clip_rect_is_empty())
+   helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, , 1);
+}
+EXPORT_SYMBOL(drm_fb_helper_deferred_io);


There is one thing I have wondered about when it comes to deferred io and
long run times for the defio worker with my displays:

Userspace writes to some pages then the deferred io worker kicks off and
runs for 100ms holding the page list mutex. While this is happening,
userspace writes to a new page triggering a page_mkwrite. Now this
function has to wait for the mutex to be released.

Who is actually waiting here and is there a problem that this can last
for 100ms?

Excerpt from drivers/video/fbdev/core/fb_defio.c:

/* vm_ops->page_mkwrite handler */
static int fb_deferred_io_mkwrite(struct vm_area_struct *vma,
  struct vm_fault *vmf)
{
...
/* this is a callback we get when userspace first tries to
write to the page. we schedule a workqueue. that workqueue
will eventually mkclean the touched pages and execute the
deferred framebuffer IO. then if userspace touches a page
again, we repeat the same scheme */
...
/* protect against the workqueue changing the page list */
mutex_lock(>lock);
...
/*
 * We want the page to remain locked from ->page_mkwrite until
 * the PTE is marked dirty to avoid page_mkclean() being called
 * before the PTE is updated, which would leave the page ignored
 * by defio.
 * Do this by locking the page here and informing the caller
 * about it with VM_FAULT_LOCKED.
 */
lock_page(page);

/* we loop through the pagelist before adding in order
to keep the pagelist sorted */
list_for_each_entry(cur, >pagelist, lru) {
/* this check is to catch the case where a new
process could start writing to the same page
through a new pte. this new access can cause the
mkwrite even when the original ps's pte is marked
writable */
if (unlikely(cur == page))
goto page_already_added;
else if (cur->index > page->index)
break;
}

list_add_tail(>lru, >lru);

page_already_added:

Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-20 Thread kbuild test robot 
Hi,

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v4.6-rc4 next-20160420]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Noralf-Tr-nnes/drm-Add-fbdev-deferred-io-support-to-helpers/20160420-234359
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/i915/i915_irq.c:2663: warning: No description found for 
parameter 'fmt'
   include/drm/drm_crtc.h:364: warning: No description found for parameter 
'mode_blob'
   include/drm/drm_crtc.h:779: warning: No description found for parameter 
'name'
   include/drm/drm_crtc.h:1238: warning: No description found for parameter 
'connector_id'
   include/drm/drm_crtc.h:1238: warning: No description found for parameter 
'tile_blob_ptr'
   include/drm/drm_crtc.h:1277: warning: No description found for parameter 
'rotation'
   include/drm/drm_crtc.h:1539: warning: No description found for parameter 
'name'
   include/drm/drm_crtc.h:1539: warning: No description found for parameter 
'mutex'
   include/drm/drm_crtc.h:1539: warning: No description found for parameter 
'helper_private'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tile_idr'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'connector_ida'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'delayed_event'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'edid_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'dpms_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'path_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tile_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'plane_type_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'rotation_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_src_x'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_src_y'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_src_w'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_src_h'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_crtc_x'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_crtc_y'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_crtc_w'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_crtc_h'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_fb_id'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_crtc_id'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_active'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_mode_id'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'dvi_i_subconnector_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'dvi_i_select_subconnector_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_subconnector_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_select_subconnector_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_mode_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_left_margin_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_right_margin_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_top_margin_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_bottom_margin_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_brightness_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_contrast_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_flicker_reduction_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_overscan_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_saturation_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_hue_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'scaling_mode_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'aspect_ratio_property'
   include/drm/drm_crtc.h:2175: warning: No

Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support

2016-04-20 Thread kbuild test robot 
Hi,

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v4.6-rc4 next-20160420]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Noralf-Tr-nnes/drm-Add-fbdev-deferred-io-support-to-helpers/20160420-234359
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/i915/i915_irq.c:2663: warning: No description found for 
parameter 'fmt'
   include/drm/drm_crtc.h:364: warning: No description found for parameter 
'mode_blob'
   include/drm/drm_crtc.h:779: warning: No description found for parameter 
'name'
   include/drm/drm_crtc.h:1238: warning: No description found for parameter 
'connector_id'
   include/drm/drm_crtc.h:1238: warning: No description found for parameter 
'tile_blob_ptr'
   include/drm/drm_crtc.h:1277: warning: No description found for parameter 
'rotation'
   include/drm/drm_crtc.h:1539: warning: No description found for parameter 
'name'
   include/drm/drm_crtc.h:1539: warning: No description found for parameter 
'mutex'
   include/drm/drm_crtc.h:1539: warning: No description found for parameter 
'helper_private'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tile_idr'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'connector_ida'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'delayed_event'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'edid_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'dpms_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'path_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tile_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'plane_type_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'rotation_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_src_x'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_src_y'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_src_w'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_src_h'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_crtc_x'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_crtc_y'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_crtc_w'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_crtc_h'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_fb_id'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_crtc_id'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_active'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'prop_mode_id'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'dvi_i_subconnector_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'dvi_i_select_subconnector_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_subconnector_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_select_subconnector_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_mode_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_left_margin_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_right_margin_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_top_margin_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_bottom_margin_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_brightness_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_contrast_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_flicker_reduction_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_overscan_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_saturation_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'tv_hue_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'scaling_mode_property'
   include/drm/drm_crtc.h:2175: warning: No description found for parameter 
'aspect_ratio_property'
   include/drm/drm_crtc.h:2175: warning: No