[PATCH 0/5] Unmappable DRM client buffers for fbdev emulation
Thomas Zimmermann
tzimmermann at suse.de
Thu Jul 4 11:10:41 UTC 2019
Hi
Am 04.07.19 um 12:18 schrieb Noralf Trønnes:
>
>
> Den 04.07.2019 09.43, skrev Thomas Zimmermann:
>> Hi
>>
>> Am 03.07.19 um 21:27 schrieb Noralf Trønnes:
>>>
>>>
>>> Den 03.07.2019 10.32, skrev Thomas Zimmermann:
>>>> DRM client buffers are permanently mapped throughout their lifetime. This
>>>> prevents us from using generic framebuffer emulation for devices with
>>>> small dedicated video memory, such as ast or mgag200. With fb buffers
>>>> permanently mapped, such devices often won't have enougth space left to
>>>> display other content (e.g., X11).
>>>>
>>>> This patch set introduces unmappable DRM client buffers for framebuffer
>>>> emulation with shadow buffers. While the shadow buffer remains in system
>>>> memory permanently, the respective buffer object will only be mapped briefly
>>>> during updates from the shadow buffer. Hence, the driver can relocate he
>>>> buffer object among memory regions as needed.
>>>>
>>>> The default behoviour for DRM client buffers is still to be permanently
>>>> mapped.
>>>>
>>>> The patch set converts ast and mgag200 to generic framebuffer emulation
>>>> and removes a large amount of framebuffer code from these drivers. For
>>>> bochs, a problem was reported where the driver could not display the console
>>>> because it was pinned in system memory. [1] The patch set fixes this bug
>>>> by converting bochs to use the shadow fb.
>>>>
>>>> The patch set has been tested on ast and mga200 HW.
>>>>
>>>
>>> I've been thinking, this enables dirty tracking in DRM userspace for
>>> these drivers since the dirty callback is now set on all framebuffers.
>>> Is this OK? Should we add a drm_fbdev_generic_shadow_setup() that sets a
>>> flag enabling the shadow buffer instead?
>>
>> Fbdev emulation is special wrt framebuffer setup and there's no way to
>> distinguish a regular FB from the fbdev's FB. I've been trying
>> drm_fbdev_generic_shadow_setup(), but ended up duplicating
>> functionality. The problem was that we cannot get state-flag arguments
>> into drm_fb_helper_generic_probe().
>>
>> There already is struct mode_config.prefer_shadow. It signals shadow FB
>> rendering to userspace. The easiest solution is to add
>> prefer_shadow_fbdev as well. If either flag is true, fbdev emulation
>> would enable shadow buffering.
>
> How about something like this:
I had something like that in mind, but maybe without a separate
function. I'll post my variant as part of the patch set's next iteration.
> diff --git a/drivers/gpu/drm/drm_fb_helper.c
> b/drivers/gpu/drm/drm_fb_helper.c
> index 1984e5c54d58..723fe56aa5f5 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -415,7 +415,8 @@ static void drm_fb_helper_dirty_work(struct
> work_struct *work)
> /* Generic fbdev uses a shadow buffer */
> if (helper->buffer)
> drm_fb_helper_dirty_blit_real(helper, &clip_copy);
> - helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
> + if (helper->fb->funcs->dirty)
> + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
> }
> }
>
> @@ -2209,7 +2210,7 @@ int drm_fb_helper_generic_probe(struct
> drm_fb_helper *fb_helper,
> #endif
> drm_fb_helper_fill_info(fbi, fb_helper, sizes);
>
> - if (fb->funcs->dirty) {
> + if (fb->funcs->dirty || fb_helper->use_shadow) {
> struct fb_ops *fbops;
> void *shadow;
>
> @@ -2310,6 +2311,44 @@ static const struct drm_client_funcs
> drm_fbdev_client_funcs = {
> .hotplug = drm_fbdev_client_hotplug,
> };
>
> +static int _drm_fbdev_generic_setup(struct drm_device *dev, unsigned
> int preferred_bpp, bool use_shadow)
> +{
> + struct drm_fb_helper *fb_helper;
> + int ret;
> +
> + WARN(dev->fb_helper, "fb_helper is already set!\n");
> +
> + if (!drm_fbdev_emulation)
> + return 0;
> +
> + fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
> + if (!fb_helper)
> + return -ENOMEM;
> +
> + ret = drm_client_init(dev, &fb_helper->client, "fbdev",
> &drm_fbdev_client_funcs);
> + if (ret) {
> + kfree(fb_helper);
> + DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", ret);
> + return ret;
> + }
> +
> + if (!preferred_bpp)
> + preferred_bpp = dev->mode_config.preferred_depth;
> + if (!preferred_bpp)
> + preferred_bpp = 32;
> + fb_helper->preferred_bpp = preferred_bpp;
> +
> + fb_helper->use_shadow = use_shadow;
> +
> + ret = drm_fbdev_client_hotplug(&fb_helper->client);
> + if (ret)
> + DRM_DEV_DEBUG(dev->dev, "client hotplug ret=%d\n", ret);
> +
> + drm_client_register(&fb_helper->client);
> +
> + return 0;
> +}
> +
> /**
> * drm_fbdev_generic_setup() - Setup generic fbdev emulation
> * @dev: DRM device
> @@ -2338,38 +2377,13 @@ static const struct drm_client_funcs
> drm_fbdev_client_funcs = {
> */
> int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int
> preferred_bpp)
> {
> - struct drm_fb_helper *fb_helper;
> - int ret;
> + return _drm_fbdev_generic_setup(dev, preferred_bpp, false);
> +}
> +EXPORT_SYMBOL(drm_fbdev_generic_setup);
>
> - WARN(dev->fb_helper, "fb_helper is already set!\n");
> -
> - if (!drm_fbdev_emulation)
> - return 0;
> -
> - fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
> - if (!fb_helper)
> - return -ENOMEM;
> -
> - ret = drm_client_init(dev, &fb_helper->client, "fbdev",
> &drm_fbdev_client_funcs);
> - if (ret) {
> - kfree(fb_helper);
> - DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", ret);
> - return ret;
> - }
> -
> - if (!preferred_bpp)
> - preferred_bpp = dev->mode_config.preferred_depth;
> - if (!preferred_bpp)
> - preferred_bpp = 32;
> - fb_helper->preferred_bpp = preferred_bpp;
> -
> - ret = drm_fbdev_client_hotplug(&fb_helper->client);
> - if (ret)
> - DRM_DEV_DEBUG(dev->dev, "client hotplug ret=%d\n", ret);
> -
> - drm_client_register(&fb_helper->client);
> -
> - return 0;
> +int drm_fbdev_generic_shadow_setup(struct drm_device *dev, unsigned int
> preferred_bpp)
> +{
> + return _drm_fbdev_generic_setup(dev, preferred_bpp, true);
> }
> EXPORT_SYMBOL(drm_fbdev_generic_setup);
>
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index c8a8ae2a678a..39f063de8cbc 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -186,6 +186,8 @@ struct drm_fb_helper {
> * See also: @deferred_setup
> */
> int preferred_bpp;
> +
> + bool use_shadow;
> };
>
> static inline struct drm_fb_helper *
>
>
>>
>> I'm not sure if we should check for the dirty() callback at all.
>>
>
> Hm, why do you think that?
Drivers may already come with their own shadow buffer. Cirrus is an
example of that. It uses shmem buffer objects as shadow fbs and
internally updates the device frame buffer in its dirty callback. Using
dirty() to select the shadow fbdev adds another buffer (and another
memcpy) for no reason.
Best regards
Thomas
> The thing with fbdev defio is that it only supports kmalloc and vmalloc
> allocated memory (page->lru is avail.). This means that only the CMA
> drivers can use defio without shadow memory. To keep things simple
> everyone with a dirty() callback gets a shadow buffer.
>
> Noralf.
>
>> Best regards
>> Thomas
>>
>>> Really nice diffstat by the way :-)
>>>
>>> Noralf.
>>>
>>>> [1] https://lists.freedesktop.org/archives/dri-devel/2019-June/224423.html
>>>>
>>>> Thomas Zimmermann (5):
>>>> drm/client: Support unmapping of DRM client buffers
>>>> drm/fb-helper: Unmap BO for shadow-buffered framebuffer console
>>>> drm/ast: Replace struct ast_fbdev with generic framebuffer emulation
>>>> drm/bochs: Use shadow buffer for bochs framebuffer console
>>>> drm/mgag200: Replace struct mga_fbdev with generic framebuffer
>>>> emulation
>>>>
>>>> drivers/gpu/drm/ast/Makefile | 2 +-
>>>> drivers/gpu/drm/ast/ast_drv.c | 22 +-
>>>> drivers/gpu/drm/ast/ast_drv.h | 17 --
>>>> drivers/gpu/drm/ast/ast_fb.c | 341 -------------------------
>>>> drivers/gpu/drm/ast/ast_main.c | 30 ++-
>>>> drivers/gpu/drm/ast/ast_mode.c | 21 --
>>>> drivers/gpu/drm/bochs/bochs_kms.c | 2 +-
>>>> drivers/gpu/drm/drm_client.c | 71 ++++-
>>>> drivers/gpu/drm/drm_fb_helper.c | 14 +-
>>>> drivers/gpu/drm/mgag200/Makefile | 2 +-
>>>> drivers/gpu/drm/mgag200/mgag200_drv.h | 19 --
>>>> drivers/gpu/drm/mgag200/mgag200_fb.c | 309 ----------------------
>>>> drivers/gpu/drm/mgag200/mgag200_main.c | 61 +++--
>>>> drivers/gpu/drm/mgag200/mgag200_mode.c | 27 --
>>>> include/drm/drm_client.h | 3 +
>>>> 15 files changed, 154 insertions(+), 787 deletions(-)
>>>> delete mode 100644 drivers/gpu/drm/ast/ast_fb.c
>>>> delete mode 100644 drivers/gpu/drm/mgag200/mgag200_fb.c
>>>>
>>>> --
>>>> 2.21.0
>>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
-------------- 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/20190704/2651ae6a/attachment-0001.sig>
More information about the dri-devel
mailing list