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]

 



Hi Daniel

Am 04.11.19 um 10:55 schrieb Daniel Vetter:
> On Mon, Oct 28, 2019 at 09:13:47AM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 25.10.19 um 20:54 schrieb Daniel Vetter:
>>> 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.
>>
>> I remember getting an eamil about this from some automated test system.
>> I haven't had time to change it and it's apparently not urgent.
>>
>> If we really want to fix the problem, we'd have to propagage is_iomem
>> through the DRM core; probably touching every vmap callback. OTOH this
>> might not be a bad thing. With is_iomem available in the generic fbdev
>> emulation, it could select between sys and cfb functions and support
>> drivers with cfb-based fbdev as well.
> 
> is_iomem doesn't work, it's hack, since the change is in the function
> prototypes/signatures. So we'd need to make sure we have 2 sets of kernel
> mapping functions for everything, plus 2 sets of anything that accesses
> through the kernel.
> 
> It's a huge task, and I'm really not sure we have any architecture we care
> about this ...

But the caller of vmap() doesn't know if it is I/O memory. It has to get
this information via vmap() from the memory manager. Having two
individual vmap() functions (and picking the correct one) would be quite
a burden to the caller.

But what's wrong with casting the returned address to void __iomem* if
is_iomem is true?

To get an idea of how well this works, I made a patchset to propagate
is_iomem through all the vmap() interfaces in DRM. I found that most
drivers' memory managers are SHMEM- or CMA-based and don't have to
bother. The rest is based on TTM, which already returns the correct
value for is_iomem. I didn't modify dma-buf interfaces and simply
assumed 'false' here.

The final patches modify the fbdev emulation to use either sys or cfb
functions, depending on the value of is_iomem. Admittedly, I don't have
the hardware where it makes a differences, but the change wasn't that
hard either.

I can probably post the patchset later this week for RFC.

Best regards
Thomas

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>> -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
>>>>
>>>
>>>
>>
>> -- 
>> 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
>>
> 
> 
> 
> 

-- 
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

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
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