Re: [PATCH] drm/fbdev-dma: Only install deferred I/O if necessary

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 04.09.24 um 11:59 schrieb Simona Vetter:
> 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?

Ok.

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

The solution here is just a quick-fix and luckily we already have a 
solution. You've probably seen the rework of the client setup in [1]. 
That series adds struct drm_driver.fbdev_probe, which will then 
implement fbdev buffer creation. In the next iteration, I plan to 
provide an implementation for DMA BOs with deferred I/O and another one 
without deferred I/O, so each driver can select the one it requires. In 
the worst case, a driver can provide its own implementation.

[1] https://patchwork.freedesktop.org/series/137391/

> With something like that added:
>
> Reviewed-by: Simona Vetter <simona.vetter@xxxxxxxx>

Thanks for reviewing.

Best regards
Thomas

> 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

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)