On 2023/3/20 23:07, Thomas Zimmermann wrote:
Remove all codepaths that implement fbdev output directly on GEM
buffers. Always allocate a shadow buffer in system memory and set
up deferred I/O for mmap.
The fbdev code that operated directly on GEM buffers was used by
drivers based on GEM DMA helpers. Those drivers have been migrated
to use fbdev-dma, a dedicated fbdev emulation for DMA memory. All
remaining users of fbdev-generic require shadow buffering.
Memory management of the remaining callers uses TTM, GEM SHMEM
helpers or a variant of GEM DMA helpers that is incompatible with
fbdev-dma. Therefore remove the unused codepaths from fbdev-generic
and simplify the code.
Using a shadow buffer with deferred I/O is probably the best case
for most remaining callers. Some of the TTM-based drivers might
benefit from a dedicated fbdev emulation that operates directly on
the driver's video memory.
I don't understand here, the TTM-based drivers should have equivalent
performance
with you implement. Because device memory typically very slow for cpu
read, at least
this is true for Mips and loongarch architecture. TTM-based drivers for
those platform
is also prefer to render to system ram first(for fast reading and
compositing) and then
blit to the real framebuffer pinned to VRAM.
In turn, I think shmem helper based drivers might benefit from a
dedicated fbdev emulation.
Because you are blit to the shadow of the video memory for shmem helper
based driver. The
driver may need another blit to the ultimate framebuffer. Using a
shadow buffer is still acceptable
though, but why do you say "the TTM-based drivers might benefit from a
dedicated fbdev emulation" ?
Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
Acked-by: Zack Rusin <zackr@xxxxxxxxxx>
---
drivers/gpu/drm/drm_fbdev_generic.c | 184 +++++-----------------------
1 file changed, 30 insertions(+), 154 deletions(-)
diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
index 4d6325e91565..e48a8e82378d 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -11,16 +11,6 @@
#include <drm/drm_fbdev_generic.h>
-static bool drm_fbdev_use_shadow_fb(struct drm_fb_helper *fb_helper)
-{
- struct drm_device *dev = fb_helper->dev;
- struct drm_framebuffer *fb = fb_helper->fb;
-
- return dev->mode_config.prefer_shadow_fbdev ||
- dev->mode_config.prefer_shadow ||
- fb->funcs->dirty;
-}
-
/* @user: 1=userspace, 0=fbcon */
static int drm_fbdev_fb_open(struct fb_info *info, int user)
{
@@ -46,115 +36,33 @@ static int drm_fbdev_fb_release(struct fb_info *info, int user)
static void drm_fbdev_fb_destroy(struct fb_info *info)
{
struct drm_fb_helper *fb_helper = info->par;
- void *shadow = NULL;
+ void *shadow = info->screen_buffer;
if (!fb_helper->dev)
return;
- if (info->fbdefio)
- fb_deferred_io_cleanup(info);
- if (drm_fbdev_use_shadow_fb(fb_helper))
- shadow = info->screen_buffer;
-
+ fb_deferred_io_cleanup(info);
drm_fb_helper_fini(fb_helper);
-
- if (shadow)
- vfree(shadow);
- else if (fb_helper->buffer)
- drm_client_buffer_vunmap(fb_helper->buffer);
-
+ vfree(shadow);
drm_client_framebuffer_delete(fb_helper->buffer);
- drm_client_release(&fb_helper->client);
+ drm_client_release(&fb_helper->client);
drm_fb_helper_unprepare(fb_helper);
kfree(fb_helper);
}
-static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
-{
- struct drm_fb_helper *fb_helper = info->par;
-
- if (drm_fbdev_use_shadow_fb(fb_helper))
- return fb_deferred_io_mmap(info, vma);
- else if (fb_helper->dev->driver->gem_prime_mmap)
- return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
- else
- return -ENODEV;
-}
-
-static bool drm_fbdev_use_iomem(struct fb_info *info)
-{
- struct drm_fb_helper *fb_helper = info->par;
- struct drm_client_buffer *buffer = fb_helper->buffer;
-
- return !drm_fbdev_use_shadow_fb(fb_helper) && buffer->map.is_iomem;
-}
-
-static ssize_t drm_fbdev_fb_read(struct fb_info *info, char __user *buf,
- size_t count, loff_t *ppos)
-{
- ssize_t ret;
-
- if (drm_fbdev_use_iomem(info))
- ret = drm_fb_helper_cfb_read(info, buf, count, ppos);
- else
- ret = drm_fb_helper_sys_read(info, buf, count, ppos);
-
- return ret;
-}
-
-static ssize_t drm_fbdev_fb_write(struct fb_info *info, const char __user *buf,
- size_t count, loff_t *ppos)
-{
- ssize_t ret;
-
- if (drm_fbdev_use_iomem(info))
- ret = drm_fb_helper_cfb_write(info, buf, count, ppos);
- else
- ret = drm_fb_helper_sys_write(info, buf, count, ppos);
-
- return ret;
-}
-
-static void drm_fbdev_fb_fillrect(struct fb_info *info,
- const struct fb_fillrect *rect)
-{
- if (drm_fbdev_use_iomem(info))
- drm_fb_helper_cfb_fillrect(info, rect);
- else
- drm_fb_helper_sys_fillrect(info, rect);
-}
-
-static void drm_fbdev_fb_copyarea(struct fb_info *info,
- const struct fb_copyarea *area)
-{
- if (drm_fbdev_use_iomem(info))
- drm_fb_helper_cfb_copyarea(info, area);
- else
- drm_fb_helper_sys_copyarea(info, area);
-}
-
-static void drm_fbdev_fb_imageblit(struct fb_info *info,
- const struct fb_image *image)
-{
- if (drm_fbdev_use_iomem(info))
- drm_fb_helper_cfb_imageblit(info, image);
- else
- drm_fb_helper_sys_imageblit(info, image);
-}
-
static const struct fb_ops drm_fbdev_fb_ops = {
.owner = THIS_MODULE,
- DRM_FB_HELPER_DEFAULT_OPS,
.fb_open = drm_fbdev_fb_open,
.fb_release = drm_fbdev_fb_release,
+ .fb_read = drm_fb_helper_sys_read,
+ .fb_write = drm_fb_helper_sys_write,
+ DRM_FB_HELPER_DEFAULT_OPS,
+ .fb_fillrect = drm_fb_helper_sys_fillrect,
+ .fb_copyarea = drm_fb_helper_sys_copyarea,
+ .fb_imageblit = drm_fb_helper_sys_imageblit,
+ .fb_mmap = fb_deferred_io_mmap,
.fb_destroy = drm_fbdev_fb_destroy,
- .fb_mmap = drm_fbdev_fb_mmap,
- .fb_read = drm_fbdev_fb_read,
- .fb_write = drm_fbdev_fb_write,
- .fb_fillrect = drm_fbdev_fb_fillrect,
- .fb_copyarea = drm_fbdev_fb_copyarea,
- .fb_imageblit = drm_fbdev_fb_imageblit,
};
/*
@@ -169,7 +77,6 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
struct drm_framebuffer *fb;
struct fb_info *info;
u32 format;
- struct iosys_map map;
int ret;
drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
@@ -197,44 +104,21 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
drm_fb_helper_fill_info(info, fb_helper, sizes);
- if (drm_fbdev_use_shadow_fb(fb_helper)) {
- info->screen_buffer = vzalloc(info->screen_size);
- if (!info->screen_buffer)
- return -ENOMEM;
- info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
+ info->screen_buffer = vzalloc(info->screen_size);
+ if (!info->screen_buffer)
+ return -ENOMEM;
+ info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
- /* Set a default deferred I/O handler */
- fb_helper->fbdefio.delay = HZ / 20;
- fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
+ info->fix.smem_start = page_to_phys(vmalloc_to_page(info->screen_buffer));
Why simply use screen_buffer instead of info->screen_buffer here ?
info->fix.smem_start = page_to_phys(vmalloc_to_page(screen_buffer));
I'm asking because I see you use vfree(screen_buffer) below the err_vfree label in this function.
I also want to ask another question here:
I heard,//the/ /memory/ /allocated/ /by//*//vzalloc//*//is/ /not/ /physically/ /contiguous/. /Why such a virtual address can convert to physical address by//page_to_phys(vmalloc_to_page())?//Does it legal for a GPU without MMU accessing such a physical address leaked to user-space?
- info->fbdefio = &fb_helper->fbdefio;
- ret = fb_deferred_io_init(info);
- if (ret)
- return ret;
- } else {
- /* buffer is mapped for HW framebuffer */
- ret = drm_client_buffer_vmap(fb_helper->buffer, &map);
- if (ret)
- return ret;
- if (map.is_iomem) {
- info->screen_base = map.vaddr_iomem;
- } else {
- info->screen_buffer = map.vaddr;
- info->flags |= FBINFO_VIRTFB;
- }
-
- /*
- * Shamelessly leak the physical address to user-space. As
- * page_to_phys() is undefined for I/O memory, warn in this
- * case.
- */
-#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
- if (fb_helper->hint_leak_smem_start && info->fix.smem_start == 0 &&
- !drm_WARN_ON_ONCE(dev, map.is_iomem))
- info->fix.smem_start =
- page_to_phys(virt_to_page(info->screen_buffer));
-#endif
- }
+ /* Set a default deferred I/O handler */
+ fb_helper->fbdefio.delay = HZ / 20;
+ fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
+
+ info->fbdefio = &fb_helper->fbdefio;
+ ret = fb_deferred_io_init(info);
+ if (ret)
+ return ret;
return 0;
}
@@ -318,18 +202,13 @@ static int drm_fbdev_fb_dirty(struct drm_fb_helper *helper, struct drm_clip_rect
struct drm_device *dev = helper->dev;
int ret;
- if (!drm_fbdev_use_shadow_fb(helper))
- return 0;
-
/* Call damage handlers only if necessary */
if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
return 0;
- if (helper->buffer) {
- ret = drm_fbdev_damage_blit(helper, clip);
- if (drm_WARN_ONCE(dev, ret, "Damage blitter failed: ret=%d\n", ret))
- return ret;
- }
+ ret = drm_fbdev_damage_blit(helper, clip);
+ if (drm_WARN_ONCE(dev, ret, "Damage blitter failed: ret=%d\n", ret))
+ return ret;
if (helper->fb->funcs->dirty) {
ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
@@ -415,12 +294,9 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
* suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
* Simple drivers might use drm_mode_config_helper_suspend().
*
- * 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. 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.
+ * In order to provide fixed mmap-able memory ranges,
I don't understand here, what do you mean about *fixed*?
fixed relative to what? Can you say more?
