On Wed, Sep 04, 2024 at 11:31:23AM +0200, Thomas Zimmermann wrote:
> Deferred I/O requires struct page for framebuffer memory, which is
> not guaranteed for all DMA ranges. We thus only install deferred I/O
> if we have a framebuffer that requires it.
>
> A reported bug affected the ipu-v3 and pl111 drivers, which have video
> memory in either Normal or HighMem zones
>
> [ 0.000000] Zone ranges:
> [ 0.000000] Normal [mem 0x0000000010000000-0x000000003fffffff]
> [ 0.000000] HighMem [mem 0x0000000040000000-0x000000004fffffff]
>
> where deferred I/O only works correctly with HighMem. See the Closes
> tags for bug reports.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Fixes: 808a40b69468 ("drm/fbdev-dma: Implement damage handling and deferred I/O")
> Reported-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/all/23636953.6Emhk5qWAg@steina-w/
> Reported-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Closes: https://lore.kernel.org/dri-devel/CACRpkdb+hb9AGavbWpY-=uQQ0apY9en_tWJioPKf_fAbXMP4Hg@xxxxxxxxxxxxxx/
> Tested-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Cc: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
I guess yet another reason we should look into vma-interception using
mkwrite and read-only ptes, but that's a lot of typing and I think this
should work interim at least.
> ---
> drivers/gpu/drm/drm_fbdev_dma.c | 71 ++++++++++++++++++++++++---------
> 1 file changed, 52 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c
> index 7ef5a48c8029..455dc48d17a7 100644
> --- a/drivers/gpu/drm/drm_fbdev_dma.c
> +++ b/drivers/gpu/drm/drm_fbdev_dma.c
> @@ -36,20 +36,11 @@ static int drm_fbdev_dma_fb_release(struct fb_info *info, int user)
> return 0;
> }
>
> -FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS(drm_fbdev_dma,
> - drm_fb_helper_damage_range,
> - drm_fb_helper_damage_area);
> -
> static int drm_fbdev_dma_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> {
> struct drm_fb_helper *fb_helper = info->par;
> - struct drm_framebuffer *fb = fb_helper->fb;
> - struct drm_gem_dma_object *dma = drm_fb_dma_get_gem_obj(fb, 0);
>
> - if (!dma->map_noncoherent)
> - vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> -
> - return fb_deferred_io_mmap(info, vma);
> + return drm_gem_prime_mmap(fb_helper->buffer->gem, vma);
> }
>
> static void drm_fbdev_dma_fb_destroy(struct fb_info *info)
> @@ -70,13 +61,40 @@ static void drm_fbdev_dma_fb_destroy(struct fb_info *info)
> }
>
> static const struct fb_ops drm_fbdev_dma_fb_ops = {
> + .owner = THIS_MODULE,
> + .fb_open = drm_fbdev_dma_fb_open,
> + .fb_release = drm_fbdev_dma_fb_release,
> + __FB_DEFAULT_DMAMEM_OPS_RDWR,
> + DRM_FB_HELPER_DEFAULT_OPS,
> + __FB_DEFAULT_DMAMEM_OPS_DRAW,
> + .fb_mmap = drm_fbdev_dma_fb_mmap,
> + .fb_destroy = drm_fbdev_dma_fb_destroy,
> +};
> +
> +FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS(drm_fbdev_dma,
> + drm_fb_helper_damage_range,
> + drm_fb_helper_damage_area);
> +
> +static int drm_fbdev_dma_deferred_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> + struct drm_framebuffer *fb = fb_helper->fb;
> + struct drm_gem_dma_object *dma = drm_fb_dma_get_gem_obj(fb, 0);
> +
> + if (!dma->map_noncoherent)
> + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> +
> + return fb_deferred_io_mmap(info, vma);
> +}
> +
> +static const struct fb_ops drm_fbdev_dma_deferred_fb_ops = {
> .owner = THIS_MODULE,
> .fb_open = drm_fbdev_dma_fb_open,
> .fb_release = drm_fbdev_dma_fb_release,
> __FB_DEFAULT_DEFERRED_OPS_RDWR(drm_fbdev_dma),
> DRM_FB_HELPER_DEFAULT_OPS,
> __FB_DEFAULT_DEFERRED_OPS_DRAW(drm_fbdev_dma),
> - .fb_mmap = drm_fbdev_dma_fb_mmap,
> + .fb_mmap = drm_fbdev_dma_deferred_fb_mmap,
> .fb_destroy = drm_fbdev_dma_fb_destroy,
> };
>
> @@ -89,6 +107,7 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper,
> {
> struct drm_client_dev *client = &fb_helper->client;
> struct drm_device *dev = fb_helper->dev;
> + bool use_deferred_io = false;
> struct drm_client_buffer *buffer;
> struct drm_gem_dma_object *dma_obj;
> struct drm_framebuffer *fb;
> @@ -111,6 +130,15 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper,
>
> fb = buffer->fb;
>
> + /*
> + * Deferred I/O requires struct page for framebuffer memory,
> + * which is not guaranteed for all DMA ranges. We thus only
> + * install deferred I/O if we have a framebuffer that requires
> + * it.
> + */
> + if (fb->funcs->dirty)
> + use_deferred_io = true;
> +
> ret = drm_client_buffer_vmap(buffer, &map);
> if (ret) {
> goto err_drm_client_buffer_delete;
> @@ -130,7 +158,10 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper,
>
> drm_fb_helper_fill_info(info, fb_helper, sizes);
>
> - info->fbops = &drm_fbdev_dma_fb_ops;
> + if (use_deferred_io)
> + info->fbops = &drm_fbdev_dma_deferred_fb_ops;
> + else
> + info->fbops = &drm_fbdev_dma_fb_ops;
>
> /* screen */
> info->flags |= FBINFO_VIRTFB; /* system memory */
> @@ -145,13 +176,15 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper,
> info->fix.smem_len = info->screen_size;
>
> /* deferred I/O */
> - 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)
> - goto err_drm_fb_helper_release_info;
> + if (use_deferred_io) {
I think a check here that roughly matches what fb_deferred_io_get_page
does would be good. Specifically this part
if (is_vmalloc_addr(screen_buffer + offs))
page = vmalloc_to_page(screen_buffer + offs);
else if (info->fix.smem_start)
page = pfn_to_page((info->fix.smem_start + offs) >> PAGE_SHIFT);
So maybe something like:
if (!is_vmalloc_addr(screen_buffer))
if (WARN_ON(!pfn_to_page())))
use_deferred_io = false;
With maye a comment that we assume buffers aren't a hiliarious mix?
I worry about drivers that a) are on very special platforms where our dma
memory might not be page backed and b) use manual upload like over i2c or
spi. That sounds like a rather like embedded use-case combo ...
With something like that added:
Reviewed-by: Simona Vetter <simona.vetter@xxxxxxxx>
Cheers, Sima
> + 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)
> + goto err_drm_fb_helper_release_info;
> + }
>
> return 0;
>
> --
> 2.46.0
>
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
