| Message ID | 1461165929-11344-5-git-send-email-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 description found for parameter 'dirty_info_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'suggested_x_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'suggested_y_property' include/drm/drm_crtc.h:2175: warning: No description found for parameter 'allow_fb_modifiers' drivers/gpu/drm/drm_atomic_helper.c:2924: warning: No description found for parameter 'start' drivers/gpu/drm/drm_atomic_helper.c:2924: warning: No description found for parameter 'start' drivers/gpu/drm/drm_atomic_helper.c:2924: warning: No description found for parameter 'start' drivers/gpu/drm/drm_atomic_helper.c:2924: warning: No description found for parameter 'start' >> drivers/gpu/drm/drm_fb_helper.c:867: warning: No description found for parameter 'info' >> drivers/gpu/drm/drm_fb_helper.c:867: warning: No description found for parameter 'pagelist' >> drivers/gpu/drm/drm_fb_helper.c:867: warning: No description found for parameter 'info' >> drivers/gpu/drm/drm_fb_helper.c:867: warning: No description found for parameter 'pagelist' include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_nack_count' include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_defer_count' drivers/gpu/drm/drm_dp_mst_topology.c:2356: warning: No description found for parameter 'connector' include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'cached_edid' include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'has_audio' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'max_dpcd_transaction_bytes' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'sink_count' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_slots' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'avail_slots' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_pbn' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'qlock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_msg_downq' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_down_in_progress' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_lock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'proposed_vcpis' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payloads' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_mask' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'vcpi_mask' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_waitq' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'work' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_work' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_list' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_lock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_work' drivers/gpu/drm/drm_dp_mst_topology.c:2356: warning: No description found for parameter 'connector' drivers/gpu/drm/drm_irq.c:176: warning: No description found for parameter 'flags' include/drm/drmP.h:168: warning: No description found for parameter 'fmt' include/drm/drmP.h:184: warning: No description found for parameter 'fmt' include/drm/drmP.h:202: warning: No description found for parameter 'fmt' include/drm/drmP.h:247: warning: No description found for parameter 'dev' include/drm/drmP.h:247: warning: No description found for parameter 'data' include/drm/drmP.h:247: warning: No description found for parameter 'file_priv' include/drm/drmP.h:280: warning: No description found for parameter 'ioctl' include/drm/drmP.h:280: warning: No description found for parameter '_func' include/drm/drmP.h:280: warning: No description found for parameter '_flags' include/drm/drmP.h:362: warning: cannot understand function prototype: 'struct drm_lock_data ' include/drm/drmP.h:415: warning: cannot understand function prototype: 'struct drm_driver ' include/drm/drmP.h:672: warning: cannot understand function prototype: 'struct drm_info_list ' include/drm/drmP.h:682: warning: cannot understand function prototype: 'struct drm_info_node ' include/drm/drmP.h:692: warning: cannot understand function prototype: 'struct drm_minor ' include/drm/drmP.h:740: warning: cannot understand function prototype: 'struct drm_device ' drivers/gpu/drm/i915/intel_runtime_pm.c:2275: warning: No description found for parameter 'resume' drivers/gpu/drm/i915/intel_runtime_pm.c:2275: warning: No description found for parameter 'resume' drivers/gpu/drm/i915/i915_irq.c:2663: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2663: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2663: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2663: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'args' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1245: warning: No description found for parameter 'rps' drivers/gpu/drm/i915/i915_gem.c:1459: warning: No description found for parameter 'req' drivers/gpu/drm/i915/i915_gem.c:1494: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:1494: warning: No description found for parameter 'readonly' drivers/gpu/drm/i915/i915_gem.c:1617: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1617: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1617: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1680: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1680: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1680: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1725: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1725: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1725: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:2013: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:2013: warning: No description found for parameter 'size' drivers/gpu/drm/i915/i915_gem.c:2013: warning: No description found for parameter 'tiling_mode' drivers/gpu/drm/i915/i915_gem.c:2013: warning: No description found for parameter 'fenced' drivers/gpu/drm/i915/i915_gem.c:2013: warning: Excess function parameter 'obj' description in 'i915_gem_get_gtt_alignment' drivers/gpu/drm/i915/i915_gem.c:2911: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_gem.c:3037: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3087: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:3087: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:3087: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:3087: warning: Excess function parameter 'DRM_IOCTL_ARGS' description in 'i915_gem_wait_ioctl' drivers/gpu/drm/i915/i915_gem.c:3459: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3459: warning: No description found for parameter 'vm' drivers/gpu/drm/i915/i915_gem.c:3459: warning: No description found for parameter 'ggtt_view' drivers/gpu/drm/i915/i915_gem.c:3459: warning: No description found for parameter 'alignment' drivers/gpu/drm/i915/i915_gem.c:3459: warning: No description found for parameter 'flags' drivers/gpu/drm/i915/i915_gem.c:3714: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3714: warning: No description found for parameter 'write' drivers/gpu/drm/i915/i915_gem.c:3789: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3789: warning: No description found for parameter 'cache_level' drivers/gpu/drm/i915/i915_gem.c:4063: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:4063: warning: No description found for parameter 'write' drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_init_ring' drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: No description found for parameter 'engine' drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_fini_ring' drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: No description found for parameter 'engine' vim +/info +867 drivers/gpu/drm/drm_fb_helper.c 851 } 852 EXPORT_SYMBOL(drm_fb_helper_unlink_fbi); 853 854 #ifdef CONFIG_FB_DEFERRED_IO 855 /** 856 * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io callback 857 * function 858 * 859 * This function always runs in process context (struct delayed_work) and calls 860 * the (struct drm_framebuffer_funcs)->dirty function with the collected 861 * damage. There's no need to worry about the possibility that the fb_sys_*() 862 * functions could be running in atomic context. They only schedule the 863 * delayed worker which then calls this deferred_io callback. 864 */ 865 void drm_fb_helper_deferred_io(struct fb_info *info, 866 struct list_head *pagelist) > 867 { 868 struct drm_fb_helper *helper = info->par; 869 unsigned long start, end, min, max; 870 struct drm_clip_rect clip; 871 unsigned long flags; 872 struct page *page; 873 874 if (!helper->fb->funcs->dirty) 875 return; --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
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 <noralf@tronnes.org> > --- > 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(&helper->dirty_lock, flags); > + clip = helper->dirty_clip; > + drm_clip_rect_reset(&helper->dirty_clip); > + spin_unlock_irqrestore(&helper->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, &mmap_clip, 1, 0, 0, 0); > + } > + > + if (!drm_clip_rect_is_empty(&clip)) > + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 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(&fbdefio->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, &fbdefio->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(&page->lru, &cur->lru); page_already_added: mutex_unlock(&fbdefio->lock); /* come back after delay to process the deferred IO */ schedule_delayed_work(&info->deferred_work, fbdefio->delay); return VM_FAULT_LOCKED; } static int fb_deferred_io_set_page_dirty(struct page *page) { if (!PageDirty(page)) SetPageDirty(page); return 0; } /* workqueue callback */ static void fb_deferred_io_work(struct work_struct *work) { ... /* here we mkclean the pages, then do all deferred IO */ mutex_lock(&fbdefio->lock); list_for_each_entry(cur, &fbdefio->pagelist, lru) { lock_page(cur); page_mkclean(cur); unlock_page(cur); } /* driver's callback with pagelist */ fbdefio->deferred_io(info, &fbdefio->pagelist); ... mutex_unlock(&fbdefio->lock); } Noralf. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <noralf@tronnes.org> > >--- > > 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(&helper->dirty_lock, flags); > >+ clip = helper->dirty_clip; > >+ drm_clip_rect_reset(&helper->dirty_clip); > >+ spin_unlock_irqrestore(&helper->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, &mmap_clip, 1, 0, 0, 0); > >+ } > >+ > >+ if (!drm_clip_rect_is_empty(&clip)) > >+ helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 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(&fbdefio->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, &fbdefio->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(&page->lru, &cur->lru); > > page_already_added: > mutex_unlock(&fbdefio->lock); > > /* come back after delay to process the deferred IO */ > schedule_delayed_work(&info->deferred_work, fbdefio->delay); > return VM_FAULT_LOCKED; > } > > static int fb_deferred_io_set_page_dirty(struct page *page) > { > if (!PageDirty(page)) > SetPageDirty(page); > return 0; > } > > /* workqueue callback */ > static void fb_deferred_io_work(struct work_struct *work) > { > ... > /* here we mkclean the pages, then do all deferred IO */ > mutex_lock(&fbdefio->lock); > list_for_each_entry(cur, &fbdefio->pagelist, lru) { > lock_page(cur); > page_mkclean(cur); > unlock_page(cur); > } > > /* driver's callback with pagelist */ > fbdefio->deferred_io(info, &fbdefio->pagelist); > ... > mutex_unlock(&fbdefio->lock); > } > > > Noralf. >
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 <noralf@tronnes.org> >>> --- >>> 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(&helper->dirty_lock, flags); >>> + clip = helper->dirty_clip; >>> + drm_clip_rect_reset(&helper->dirty_clip); >>> + spin_unlock_irqrestore(&helper->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, &mmap_clip, 1, 0, 0, 0); >>> + } >>> + >>> + if (!drm_clip_rect_is_empty(&clip)) >>> + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 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 fb_*(). And yes, using drm from userspace is "The solution" here :-), however I want to make the best out of fbdev since some of the tinydrm users coming from drivers/staging/fbtft will probably continue with fbdev. Noralf. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <noralf@tronnes.org> > >>>--- > >>> 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(&helper->dirty_lock, flags); > >>>+ clip = helper->dirty_clip; > >>>+ drm_clip_rect_reset(&helper->dirty_clip); > >>>+ spin_unlock_irqrestore(&helper->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, &mmap_clip, 1, 0, 0, 0); > >>>+ } > >>>+ > >>>+ if (!drm_clip_rect_is_empty(&clip)) > >>>+ helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 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 fb_*(). I'm confused ... I thought we already have enough workers? One in the fbdev deferred_io implementation used by default. The other in case we get a draw call from an atomic context. Why do we need even more workers? Have you measured that you actually hit this delay, or just conjecture from reading the code? Because my reading says that the defio mmap support in fbdev already does what you want, and should sufficiently coalesce mmap access. There's a delayed work/timer in there to make sure it doesn't flush on the very first faulted page. -Daniel > And yes, using drm from userspace is "The solution" here :-), however > I want to make the best out of fbdev since some of the tinydrm users > coming from drivers/staging/fbtft will probably continue with fbdev. > > > Noralf. > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 <noralf@tronnes.org> >>>>> --- >>>>> 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(&helper->dirty_lock, flags); >>>>> + clip = helper->dirty_clip; >>>>> + drm_clip_rect_reset(&helper->dirty_clip); >>>>> + spin_unlock_irqrestore(&helper->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, &mmap_clip, 1, 0, 0, 0); >>>>> + } >>>>> + >>>>> + if (!drm_clip_rect_is_empty(&clip)) >>>>> + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 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 fb_*(). > I'm confused ... I thought we already have enough workers? One in the > fbdev deferred_io implementation used by default. The other in case we get > a draw call from an atomic context. 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. It's no big deal for me, fbtft has used 1 worker since 2013 without anyone pointing out that this has been a problem. And and extra worker can be added later without changing the drivers. But since qxl used an extra worker I thought maybe there's a reason for that and it would remove my worry about those page faults being held up. Noralf. > > Why do we need even more workers? Have you measured that you actually hit > this delay, or just conjecture from reading the code? Because my reading > says that the defio mmap support in fbdev already does what you want, and > should sufficiently coalesce mmap access. There's a delayed work/timer in > there to make sure it doesn't flush on the very first faulted page. > -Daniel > >> And yes, using drm from userspace is "The solution" here :-), however >> I want to make the best out of fbdev since some of the tinydrm users >> coming from drivers/staging/fbtft will probably continue with fbdev. >> >> >> Noralf. >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 22, 2016 at 7:28 PM, Noralf Trønnes <noralf@tronnes.org> 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
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 855108e..79c974a 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -40,6 +40,7 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_rect.h> static bool drm_fbdev_emulation = true; module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600); @@ -48,6 +49,9 @@ MODULE_PARM_DESC(fbdev_emulation, static LIST_HEAD(kernel_fb_helper_list); +static void drm_fb_helper_defer_dirty(struct fb_info *info, u32 x, u32 y, + u32 width, u32 height); + /** * DOC: fbdev helpers * @@ -84,6 +88,13 @@ 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 (struct fb_info).fbdefio is set, + * the drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit} functions + * will accumulate changes and schedule (struct fb_info).deferred_work to run. + * This is the same worker used by the mmap deferred io code path. + * drm_fb_helper_deferred_io() will call + * (struct drm_framebuffer_funcs)->dirty(). */ /** @@ -401,11 +412,14 @@ backoff: static int restore_fbdev_mode(struct drm_fb_helper *fb_helper) { struct drm_device *dev = fb_helper->dev; + struct fb_info *info = fb_helper->fbdev; struct drm_plane *plane; int i; drm_warn_on_modeset_not_all_locked(dev); + drm_fb_helper_defer_dirty(info, 0, 0, info->var.xres, info->var.yres); + if (fb_helper->atomic) return restore_fbdev_mode_atomic(fb_helper); @@ -650,6 +664,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); +#ifdef CONFIG_FB_DEFERRED_IO + spin_lock_init(&helper->dirty_lock); +#endif helper->funcs = funcs; helper->dev = dev; } @@ -834,6 +851,87 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper) } EXPORT_SYMBOL(drm_fb_helper_unlink_fbi); +#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(&helper->dirty_lock, flags); + clip = helper->dirty_clip; + drm_clip_rect_reset(&helper->dirty_clip); + spin_unlock_irqrestore(&helper->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, &mmap_clip, 1, 0, 0, 0); + } + + if (!drm_clip_rect_is_empty(&clip)) + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 1); +} +EXPORT_SYMBOL(drm_fb_helper_deferred_io); + +static void drm_fb_helper_defer_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; + unsigned long flags; + + if (!info->fbdefio) + return; + + clip.x1 = x; + clip.x2 = x + width; + clip.y1 = y; + clip.y2 = y + height; + + spin_lock_irqsave(&helper->dirty_lock, flags); + drm_clip_rect_merge(&helper->dirty_clip, &clip, 1, 0, 0, 0); + spin_unlock_irqrestore(&helper->dirty_lock, flags); + + schedule_delayed_work(&info->deferred_work, info->fbdefio->delay); +} +#else +static void drm_fb_helper_defer_dirty(struct fb_info *info, u32 x, u32 y, + u32 width, u32 height) +{ +} +#endif + /** * drm_fb_helper_sys_read - wrapper around fb_sys_read * @info: fb_info struct pointer @@ -862,7 +960,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_defer_dirty(info, 0, 0, + info->var.xres, info->var.yres); + + return ret; } EXPORT_SYMBOL(drm_fb_helper_sys_write); @@ -877,6 +982,8 @@ void drm_fb_helper_sys_fillrect(struct fb_info *info, const struct fb_fillrect *rect) { sys_fillrect(info, rect); + drm_fb_helper_defer_dirty(info, rect->dx, rect->dy, + rect->width, rect->height); } EXPORT_SYMBOL(drm_fb_helper_sys_fillrect); @@ -891,6 +998,8 @@ void drm_fb_helper_sys_copyarea(struct fb_info *info, const struct fb_copyarea *area) { sys_copyarea(info, area); + drm_fb_helper_defer_dirty(info, area->dx, area->dy, + area->width, area->height); } EXPORT_SYMBOL(drm_fb_helper_sys_copyarea); @@ -905,6 +1014,8 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info, const struct fb_image *image) { sys_imageblit(info, image); + drm_fb_helper_defer_dirty(info, image->dx, image->dy, + image->width, image->height); } EXPORT_SYMBOL(drm_fb_helper_sys_imageblit); @@ -919,6 +1030,8 @@ void drm_fb_helper_cfb_fillrect(struct fb_info *info, const struct fb_fillrect *rect) { cfb_fillrect(info, rect); + drm_fb_helper_defer_dirty(info, rect->dx, rect->dy, + rect->width, rect->height); } EXPORT_SYMBOL(drm_fb_helper_cfb_fillrect); @@ -933,6 +1046,8 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info, const struct fb_copyarea *area) { cfb_copyarea(info, area); + drm_fb_helper_defer_dirty(info, area->dx, area->dy, + area->width, area->height); } EXPORT_SYMBOL(drm_fb_helper_cfb_copyarea); @@ -947,6 +1062,8 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info, const struct fb_image *image) { cfb_imageblit(info, image); + drm_fb_helper_defer_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..c9e9271 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -172,6 +172,9 @@ 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 * * This is the main structure used by the fbdev helpers. Drivers supporting * fbdev emulation should embedded this into their overall driver structure. @@ -189,6 +192,10 @@ struct drm_fb_helper { const struct drm_fb_helper_funcs *funcs; struct fb_info *fbdev; u32 pseudo_palette[17]; +#ifdef CONFIG_FB_DEFERRED_IO + struct drm_clip_rect dirty_clip; + spinlock_t dirty_lock; +#endif /** * @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)
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 <noralf@tronnes.org> --- drivers/gpu/drm/drm_fb_helper.c | 119 +++++++++++++++++++++++++++++++++++++++- include/drm/drm_fb_helper.h | 15 +++++ 2 files changed, 133 insertions(+), 1 deletion(-)