Re: [PATCH v3 3/7] drm/fb-helper: Add fb_deferred_io support

Daniel Vetter
2016-04-28T07:17:53Z

On Wed, Apr 27, 2016 at 09:24:52PM +0200, Noralf Trønnes wrote:
>
> Den 27.04.2016 21:20, skrev Daniel Vetter:
> >On Wed, Apr 27, 2016 at 08:16:32PM +0200, Noralf Trønnes wrote:
> >>This adds deferred io support to drm_fb_helper.
> >>The fbdev framebuffer changes are flushed using the callback
> >>(struct drm_framebuffer *)->funcs->dirty() by a dedicated worker
> >>ensuring that it always runs in process context.
> >>
> >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>---
> >>
> >>Changes since v2:
> >>- FB_DEFERRED_IO is now always selected by DRM_KMS_FB_HELPER, ifdef removed
> >>- The drm_clip_rect utility functions are dropped, so open code it
> >>- docs: use & to denote structs
> >>
> >>Changes since v1:
> >>- Use a dedicated worker to run the framebuffer flushing like qxl does
> >>- Add parameter descriptions to drm_fb_helper_deferred_io
> >>
> >> drivers/gpu/drm/Kconfig | 1 +
> >> drivers/gpu/drm/drm_fb_helper.c | 109 +++++++++++++++++++++++++++++++++++++++-
> >> include/drm/drm_fb_helper.h | 15 ++++++
> >> 3 files changed, 124 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> >>index 9e4f2f1..8e6f34b 100644
> >>--- a/drivers/gpu/drm/Kconfig
> >>+++ b/drivers/gpu/drm/Kconfig
> >>@@ -52,6 +52,7 @@ config DRM_KMS_FB_HELPER
> >> select FB_CFB_FILLRECT
> >> select FB_CFB_COPYAREA
> >> select FB_CFB_IMAGEBLIT
> >>+ select FB_DEFERRED_IO
> >> help
> >> FBDEV helpers for KMS drivers.
> >>
> >>diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >>index 855108e..5112b5d 100644
> >>--- a/drivers/gpu/drm/drm_fb_helper.c
> >>+++ b/drivers/gpu/drm/drm_fb_helper.c
> >>@@ -48,6 +48,8 @@ MODULE_PARM_DESC(fbdev_emulation,
> >>
> >> static LIST_HEAD(kernel_fb_helper_list);
> >>
> >>+static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper);
> >Instead of this forward decl just inline the few setup commands into
> >drm_fb_helper_prepare.
>
> That would mean that I have to forward declare drm_fb_helper_dirty_work()
> instead. Is that better?
> Or should I relocate the functions?

Yeah, just move them all I think. This means that usually the setup
function for a component is at the very bottom, and also that you have the
inverted reading order with first reading leaf/helper functions, then the
bigger ones that assemble things together. Personally I find that
backwards, but otoh consistency is more important. And avoid forward decls
for static functions is standard style in the kernel.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation