Re: [PATCH v3 2/8] drm/ast: Only map cursor BOs during updates

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Dec 11, 2020 at 11:49:48AM +0100, Thomas Zimmermann wrote:
> 
> 
> Am 11.12.20 um 11:18 schrieb Daniel Vetter:
> > On Wed, Dec 09, 2020 at 03:25:21PM +0100, Thomas Zimmermann wrote:
> > > The HW cursor's BO used to be mapped permanently into the kernel's
> > > address space. GEM's vmap operation will be protected by locks, and
> > > we don't want to lock the BO's for an indefinate period of time.
> > > 
> > > Change the cursor code to map the HW BOs only during updates. The
> > > vmap operation in VRAM helpers is cheap, as a once estabished mapping
> > > is being reused until the BO actually moves. As the HW cursor BOs are
> > > permanently pinned, they never move at all.
> > > 
> > > v2:
> > > 	* fix typos in commit description
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > Acked-by: Christian König <christian.koenig@amd.com>
> > 
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Now there's a pretty big issue here though: We can't take dma_resv_lock in
> > commit_tail, because of possible deadlocks on at least gpus that do real
> > async rendering because of the dma_fences. Unfortunately my annotations
> > patches got stuck a bit, I need to refresh them.
> > 
> > Rules are you can pin and unpin stuff in prepare/cleanup_plane, and also
> > take dma_resv_lock there, but not in commit_tail in-between. So I think
> > our vmap_local needs to loose the unconditional assert_locked and require
> > either that or a pin count.
> 
> I guess my commit description is misleading when it speaks of updates.
> ast_cursor_blit() is actually called from the cursor plane's prepare_fb
> function. [1] The vmap code in ast_cursor_show() could be moved into blit()
> as well, I think.

Oh I failed to check this properly. Even better.

> I guess the clean solution is to integrate the cursor code with the
> modesetting code in ast_mode. From there, locks and mappings can be
> established in prepare_fb and the HW state can be updated in atomic_commit.

Yup. I'll still refresh my series with lockdep annotations, keeps paranoid
me at peace :-)
-Daniel

> 
> Best regards
> Thomas
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/ast/ast_mode.c#n646
> 
> > -Daniel
> > 
> > > ---
> > >   drivers/gpu/drm/ast/ast_cursor.c | 51 ++++++++++++++++++--------------
> > >   drivers/gpu/drm/ast/ast_drv.h    |  2 --
> > >   2 files changed, 28 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
> > > index 68bf3d33f1ed..fac1ee79c372 100644
> > > --- a/drivers/gpu/drm/ast/ast_cursor.c
> > > +++ b/drivers/gpu/drm/ast/ast_cursor.c
> > > @@ -39,7 +39,6 @@ static void ast_cursor_fini(struct ast_private *ast)
> > >   	for (i = 0; i < ARRAY_SIZE(ast->cursor.gbo); ++i) {
> > >   		gbo = ast->cursor.gbo[i];
> > > -		drm_gem_vram_vunmap(gbo, &ast->cursor.map[i]);
> > >   		drm_gem_vram_unpin(gbo);
> > >   		drm_gem_vram_put(gbo);
> > >   	}
> > > @@ -53,14 +52,13 @@ static void ast_cursor_release(struct drm_device *dev, void *ptr)
> > >   }
> > >   /*
> > > - * Allocate cursor BOs and pins them at the end of VRAM.
> > > + * Allocate cursor BOs and pin them at the end of VRAM.
> > >    */
> > >   int ast_cursor_init(struct ast_private *ast)
> > >   {
> > >   	struct drm_device *dev = &ast->base;
> > >   	size_t size, i;
> > >   	struct drm_gem_vram_object *gbo;
> > > -	struct dma_buf_map map;
> > >   	int ret;
> > >   	size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
> > > @@ -77,15 +75,7 @@ int ast_cursor_init(struct ast_private *ast)
> > >   			drm_gem_vram_put(gbo);
> > >   			goto err_drm_gem_vram_put;
> > >   		}
> > > -		ret = drm_gem_vram_vmap(gbo, &map);
> > > -		if (ret) {
> > > -			drm_gem_vram_unpin(gbo);
> > > -			drm_gem_vram_put(gbo);
> > > -			goto err_drm_gem_vram_put;
> > > -		}
> > > -
> > >   		ast->cursor.gbo[i] = gbo;
> > > -		ast->cursor.map[i] = map;
> > >   	}
> > >   	return drmm_add_action_or_reset(dev, ast_cursor_release, NULL);
> > > @@ -94,7 +84,6 @@ int ast_cursor_init(struct ast_private *ast)
> > >   	while (i) {
> > >   		--i;
> > >   		gbo = ast->cursor.gbo[i];
> > > -		drm_gem_vram_vunmap(gbo, &ast->cursor.map[i]);
> > >   		drm_gem_vram_unpin(gbo);
> > >   		drm_gem_vram_put(gbo);
> > >   	}
> > > @@ -168,31 +157,38 @@ static void update_cursor_image(u8 __iomem *dst, const u8 *src, int width, int h
> > >   int ast_cursor_blit(struct ast_private *ast, struct drm_framebuffer *fb)
> > >   {
> > >   	struct drm_device *dev = &ast->base;
> > > -	struct drm_gem_vram_object *gbo;
> > > -	struct dma_buf_map map;
> > > -	int ret;
> > > -	void *src;
> > > +	struct drm_gem_vram_object *dst_gbo = ast->cursor.gbo[ast->cursor.next_index];
> > > +	struct drm_gem_vram_object *src_gbo = drm_gem_vram_of_gem(fb->obj[0]);
> > > +	struct dma_buf_map src_map, dst_map;
> > >   	void __iomem *dst;
> > > +	void *src;
> > > +	int ret;
> > >   	if (drm_WARN_ON_ONCE(dev, fb->width > AST_MAX_HWC_WIDTH) ||
> > >   	    drm_WARN_ON_ONCE(dev, fb->height > AST_MAX_HWC_HEIGHT))
> > >   		return -EINVAL;
> > > -	gbo = drm_gem_vram_of_gem(fb->obj[0]);
> > > -
> > > -	ret = drm_gem_vram_vmap(gbo, &map);
> > > +	ret = drm_gem_vram_vmap(src_gbo, &src_map);
> > >   	if (ret)
> > >   		return ret;
> > > -	src = map.vaddr; /* TODO: Use mapping abstraction properly */
> > > +	src = src_map.vaddr; /* TODO: Use mapping abstraction properly */
> > > -	dst = ast->cursor.map[ast->cursor.next_index].vaddr_iomem;
> > > +	ret = drm_gem_vram_vmap(dst_gbo, &dst_map);
> > > +	if (ret)
> > > +		goto err_drm_gem_vram_vunmap;
> > > +	dst = dst_map.vaddr_iomem; /* TODO: Use mapping abstraction properly */
> > >   	/* do data transfer to cursor BO */
> > >   	update_cursor_image(dst, src, fb->width, fb->height);
> > > -	drm_gem_vram_vunmap(gbo, &map);
> > > +	drm_gem_vram_vunmap(dst_gbo, &dst_map);
> > > +	drm_gem_vram_vunmap(src_gbo, &src_map);
> > >   	return 0;
> > > +
> > > +err_drm_gem_vram_vunmap:
> > > +	drm_gem_vram_vunmap(src_gbo, &src_map);
> > > +	return ret;
> > >   }
> > >   static void ast_cursor_set_base(struct ast_private *ast, u64 address)
> > > @@ -243,17 +239,26 @@ static void ast_cursor_set_location(struct ast_private *ast, u16 x, u16 y,
> > >   void ast_cursor_show(struct ast_private *ast, int x, int y,
> > >   		     unsigned int offset_x, unsigned int offset_y)
> > >   {
> > > +	struct drm_device *dev = &ast->base;
> > > +	struct drm_gem_vram_object *gbo = ast->cursor.gbo[ast->cursor.next_index];
> > > +	struct dma_buf_map map;
> > >   	u8 x_offset, y_offset;
> > >   	u8 __iomem *dst;
> > >   	u8 __iomem *sig;
> > >   	u8 jreg;
> > > +	int ret;
> > > -	dst = ast->cursor.map[ast->cursor.next_index].vaddr;
> > > +	ret = drm_gem_vram_vmap(gbo, &map);
> > > +	if (drm_WARN_ONCE(dev, ret, "drm_gem_vram_vmap() failed, ret=%d\n", ret))
> > > +		return;
> > > +	dst = map.vaddr_iomem; /* TODO: Use mapping abstraction properly */
> > >   	sig = dst + AST_HWC_SIZE;
> > >   	writel(x, sig + AST_HWC_SIGNATURE_X);
> > >   	writel(y, sig + AST_HWC_SIGNATURE_Y);
> > > +	drm_gem_vram_vunmap(gbo, &map);
> > > +
> > >   	if (x < 0) {
> > >   		x_offset = (-x) + offset_x;
> > >   		x = 0;
> > > diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> > > index ccaff81924ee..f871fc36c2f7 100644
> > > --- a/drivers/gpu/drm/ast/ast_drv.h
> > > +++ b/drivers/gpu/drm/ast/ast_drv.h
> > > @@ -28,7 +28,6 @@
> > >   #ifndef __AST_DRV_H__
> > >   #define __AST_DRV_H__
> > > -#include <linux/dma-buf-map.h>
> > >   #include <linux/i2c.h>
> > >   #include <linux/i2c-algo-bit.h>
> > >   #include <linux/io.h>
> > > @@ -133,7 +132,6 @@ struct ast_private {
> > >   	struct {
> > >   		struct drm_gem_vram_object *gbo[AST_DEFAULT_HWC_NUM];
> > > -		struct dma_buf_map map[AST_DEFAULT_HWC_NUM];
> > >   		unsigned int next_index;
> > >   	} cursor;
> > > -- 
> > > 2.29.2
> > > 
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch