Re: [PATCH 1/2] drm/fb-helper: Remove drm_fb_helper_defio_init() and update docs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Hi
>
> Am 25.10.19 um 17:46 schrieb Noralf Trønnes:
> >
> >
> > Den 25.10.2019 11.27, skrev Thomas Zimmermann:
> >> There are no users of drm_fb_helper_defio_init(), so we can remove
> >> it. The documenation around defio support is a bit misleading and
> >> should mention compatibility issues with SHMEM helpers. Clarify this.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >> ---
> >>  drivers/gpu/drm/drm_fb_helper.c | 61 +++++++--------------------------
> >>  include/drm/drm_fb_helper.h     |  1 -
> >>  2 files changed, 13 insertions(+), 49 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >> index b75ae8555baf..8ebeccdeed23 100644
> >> --- a/drivers/gpu/drm/drm_fb_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
> >>   *
> >>   * Drivers that support a dumb buffer with a virtual address and mmap support,
> >>   * should try out the generic fbdev emulation using drm_fbdev_generic_setup().
> >> + * It will automatically set up deferred I/O if the driver requires a shadow
> >> + * buffer.
> >>   *
> >> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
> >> - * down by calling drm_fb_helper_fbdev_teardown().
> >> + * For other drivers, setup fbdev emulation by calling
> >> + * drm_fb_helper_fbdev_setup() and tear it down by calling
> >> + * drm_fb_helper_fbdev_teardown().
> >>   *
> >>   * At runtime drivers should restore the fbdev console by using
> >>   * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
> >> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
> >>   * always run in process context since the fb_*() function could be running in
> >>   * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io
> >>   * callback it will also schedule dirty_work with the damage collected from the
> >> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup
> >> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()).
> >> + * mmap page writes.
> >> + *
> >> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an
> >> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead.
> >>   */
> >>
> >>  static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
> >> @@ -679,49 +684,6 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
> >>  }
> >>  EXPORT_SYMBOL(drm_fb_helper_deferred_io);
> >>
> >> -/**
> >> - * drm_fb_helper_defio_init - fbdev deferred I/O initialization
> >> - * @fb_helper: driver-allocated fbdev helper
> >> - *
> >> - * This function allocates &fb_deferred_io, sets callback to
> >> - * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
> >> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
> >> - * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
> >> - *
> >> - * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
> >> - * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
> >> - * affect other instances of that &fb_ops.
> >> - *
> >> - * Returns:
> >> - * 0 on success or a negative error code on failure.
> >> - */
> >> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
> >> -{
> >> -    struct fb_info *info = fb_helper->fbdev;
> >> -    struct fb_deferred_io *fbdefio;
> >> -    struct fb_ops *fbops;
> >> -
> >> -    fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
> >> -    fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> >> -    if (!fbdefio || !fbops) {
> >> -            kfree(fbdefio);
> >> -            kfree(fbops);
> >> -            return -ENOMEM;
> >> -    }
> >> -
> >> -    info->fbdefio = fbdefio;
> >> -    fbdefio->delay = msecs_to_jiffies(50);
> >> -    fbdefio->deferred_io = drm_fb_helper_deferred_io;
> >> -
> >> -    *fbops = *info->fbops;
> >> -    info->fbops = fbops;
> >> -
> >> -    fb_deferred_io_init(info);
> >> -
> >> -    return 0;
> >> -}
> >> -EXPORT_SYMBOL(drm_fb_helper_defio_init);
> >> -
> >
> > With this gone you can remove the defio cleanup part from
> > drm_fb_helper_fbdev_teardown() as well.
> >
> > And I see that there's no users left of that function (the same with
> > *_seup()). Would be nice if you just removed them. I made them before I
> > embarked on the generic fbdev solution. I think it's better to try and
> > make the generic emulation usable for "everyone" and avoid the need for
> > drivers to have to do their own special stuff.
>
> The last user was vboxvideo, which I converted ~2 weeks ago. I haven't
> removed them yet, as there's a TODO item to convert drivers over to them.
>
> From a quick 'git grep':
>
> Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could
> probably be converted over to generic fbdev. That's hibmc (I have some
> untested patches for it), msm, omapdrm, tegra, and udl (currently being
> converted).
>
> Only nouveau and gma500 have some form of HW acceleration.
>
> The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are
> these strictly required, or can they be made available within generic fbdev?

Take a pile of digging through a few fb horrors, but this boils down to:

*sys* functions operate on an fb that works like normal system memory.

*cfb* functions operate on void __iomem * instead. Which makes a huge
difference on some architectures, but afaiui neither x86 nor arm care.
On ppc it seems to make a difference sometimes.

So for shmem we should use *sys* functions, for vram *cfb*.

And that leads me to realizing that drm_gem_vram_vmap has the totally
wrong type, it should be void __iomem *. There's this fancy is_iomem
parameter for ttm_kmap_obj_virtual that should help you figure out the
right type, but only nouveau bothers to implement this correctly.

I'm honestly not sure whether we should care.
-Daniel

>
> Best regards
> Thomas
>
> > code to avoid the shadow buffering would aid in that as Daniel have
> > talked about.
> >
> > Either way you choose, this patch is:
> >
> > Reviewed-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> >
> >>  /**
> >>   * drm_fb_helper_sys_read - wrapper around fb_sys_read
> >>   * @info: fb_info struct pointer
> >> @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
> >>   *
> >>   * Drivers that set the dirty callback on their framebuffer will get a shadow
> >>   * fbdev buffer that is blitted onto the real buffer. This is done in order to
> >> - * make deferred I/O work with all kinds of buffers.
> >> + * make deferred I/O work with all kinds of buffers. A shadow buffer can be
> >> + * requested explicitly by setting struct drm_mode_config.prefer_shadow or
> >> + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is
> >> + * required to use generic fbdev emulation with SHMEM helpers.
> >>   *
> >>   * This function is safe to call even when there are no connectors present.
> >>   * Setup will be retried on the next hotplug event.
> >> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> >> index 8dcc012ccbc8..2338e9f94a03 100644
> >> --- a/include/drm/drm_fb_helper.h
> >> +++ b/include/drm/drm_fb_helper.h
> >> @@ -235,7 +235,6 @@ 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);
> >> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
> >>
> >>  ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
> >>                             size_t count, loff_t *ppos);
> >>
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux