thr3ads.net - Linux Virtualization - [PATCH 0/5] Unmappable DRM client buffers for fbdev emulation [Jul 2019]

If this information is useful, please help other people find it:
Share via:

Thomas Zimmermann

2019-Jul-04 11:10 UTC

[PATCH 0/5] Unmappable DRM client buffers for fbdev emulation

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:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20190704/2651ae6a/attachment.sig>

Noralf Trønnes

2019-Jul-04 14:15 UTC

head link

[PATCH 0/5] Unmappable DRM client buffers for fbdev emulation

Den 04.07.2019 13.10, skrev Thomas Zimmermann:> 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.
> 
<snip>
>>>
>>> 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.
Cirruc uses shmem buffers and they won't work with fbdev defio (both use
page->lru). shmem is the reason I added shadow buffering to generic
fbdev in the first place. It will now work with whatever backing buffer
the driver uses, as long as it can provide a virtual address on the dumb
buffer (not the case with rockchip for instance, due to limited virtual
address space).

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

2019-Jul-05 07:09 UTC

head link

[PATCH 0/5] Unmappable DRM client buffers for fbdev emulation

Hi

Am 04.07.19 um 16:15 schrieb Noralf Tr?nnes:>>> 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.
> 
> Cirruc uses shmem buffers and they won't work with fbdev defio (both
use
> page->lru). shmem is the reason I added shadow buffering to generic
> fbdev in the first place. It will now work with whatever backing buffer
> the driver uses, as long as it can provide a virtual address on the dumb
> buffer (not the case with rockchip for instance, due to limited virtual
> address space).
OK, I see. Thanks or clarifying.

Best regards
Thomas
> Noralf.
> 
>>
>> 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
>>>>>
>>>>
>>
> _______________________________________________
> 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:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20190705/fabc84b4/attachment.sig>

Possibly Parallel Threads

Search for more reasonably related threads

Linux Virtualization - Jul 2019 - [PATCH 0/5] Unmappable DRM client buffers for fbdev emulation

[PATCH 0/5] Unmappable DRM client buffers for fbdev emulation

[PATCH 0/5] Unmappable DRM client buffers for fbdev emulation

[PATCH 0/5] Unmappable DRM client buffers for fbdev emulation

Possibly Parallel Threads