[PATCH 1/2] drm/fb-helper: Remove drm_fb_helper_defio_init() and update docs
Thomas Zimmermann
tzimmermann at suse.de
Wed Nov 6 08:21:05 UTC 2019
Hi
Am 05.11.19 um 10:30 schrieb Daniel Vetter:
> On Mon, Nov 04, 2019 at 07:48:35PM +0100, Thomas Zimmermann wrote:
>> 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 at suse.de> 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 at suse.de>
>>>>>>>> ---
>>>>>>>> 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.
>
> The big reason behind __iomem is that you can use sparse to make sure you
> got it right. With is_iomem and the explicit cast, you'll lose that
> information. Which means no one will get it right (viz entire current drm
> except nouveau).
>
> That's why I think the 2 paths would be a lot nicer, and callers would
> need to first call the system memory mmap, then the iomem mmap, until they
> have a non-NULL pointer. Since they need the duplicated code anyway.
>
> Other option would be we do an entire new pointer like the below:
>
> struct opaque_buffer_ptr {
> union { void * smem; void * __iomem iomem ; };
> bool is_iomem;
> };
>
> And then an entire new set of functions that deals in this new primitive.
>
> But unloading the is_iomem explicitly on drivers, expecting them to get it
> right without the help or sparse, seems futile.
>
> Also, all of this is _huuuuuuuuge_ amounts of work, and I'm still not sure
> where we need it.
I'm telling you that I have the patches written, and you say it's too
much work? That makes no sense. :)
Let my post the patches that I have and continue discussing from there.
Best regards
Thomas
> -Daniel
>
--
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20191106/1ab6de05/attachment-0001.sig>
More information about the dri-devel
mailing list