[PATCH i-g-t 1/3] lib/igt_draw: Dont mmap, if the buffer is already mmapped

[Zbigniew Kempczyński]

On Fri, Mar 15, 2024 at 11:41:40AM +0530, Vandita Kulkarni wrote:
> This will help add support to tests which would reuse the
> same fbs and would not want to mmap again and would just
> want to update the fbs.
> 
> Signed-off-by: Vandita Kulkarni <vandita.kulkarni at intel.com>
> ---
>  lib/igt_draw.c | 57 ++++++++++++++++++++++++++++----------------------
>  lib/igt_draw.h |  2 +-
>  2 files changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/lib/igt_draw.c b/lib/igt_draw.c
> index 2c01d7b02..d0aed2b26 100644
> --- a/lib/igt_draw.c
> +++ b/lib/igt_draw.c
> @@ -75,6 +75,7 @@ struct buf_data {
>  	uint32_t handle;
>  	uint32_t size;
>  	uint32_t stride;
> +	void *buf_ptr;
>  	int width;
>  	int height;
>  	int bpp;
> @@ -498,29 +499,32 @@ static void draw_rect_mmap_wc(int fd, struct buf_data *buf, struct rect *rect,
>  			      uint32_t tiling, uint32_t swizzle, uint32_t color)
>  {
>  	uint32_t *ptr;
> -

Keep this line, it's convention to use single empty line between
declaration and the code.

> -	if (is_i915_device(fd)) {
> -		gem_set_domain(fd, buf->handle, I915_GEM_DOMAIN_GTT,
> -			       I915_GEM_DOMAIN_GTT);
> -
> -		/* We didn't implement suport for the older tiling methods yet. */
> -		if (tiling != I915_TILING_NONE)
> -			igt_require(intel_display_ver(intel_get_drm_devid(fd)) >= 5);
> -
> -		if (gem_has_lmem(fd))
> -			ptr = gem_mmap_offset__fixed(fd, buf->handle, 0,
> -						     PAGE_ALIGN(buf->size),
> -						     PROT_READ | PROT_WRITE);
> -		else if (gem_has_legacy_mmap(fd))
> -			ptr = gem_mmap__wc(fd, buf->handle, 0, PAGE_ALIGN(buf->size),
> -					   PROT_READ | PROT_WRITE);
> -		else
> -			ptr = gem_mmap_offset__wc(fd, buf->handle, 0,
> -						  PAGE_ALIGN(buf->size),
> -						  PROT_READ | PROT_WRITE);
> +	if (!buf->buf_ptr) {
> +		if (is_i915_device(fd)) {
> +			gem_set_domain(fd, buf->handle, I915_GEM_DOMAIN_GTT,
> +				       I915_GEM_DOMAIN_GTT);
> +
> +			/* We didn't implement suport for the older tiling methods yet. */
> +			if (tiling != I915_TILING_NONE)
> +				igt_require(intel_display_ver(intel_get_drm_devid(fd)) >= 5);
> +
> +			if (gem_has_lmem(fd))
> +				ptr = gem_mmap_offset__fixed(fd, buf->handle, 0,
> +							     PAGE_ALIGN(buf->size),
> +							     PROT_READ | PROT_WRITE);
> +			else if (gem_has_legacy_mmap(fd))
> +				ptr = gem_mmap__wc(fd, buf->handle, 0, PAGE_ALIGN(buf->size),
> +						   PROT_READ | PROT_WRITE);
> +			else
> +				ptr = gem_mmap_offset__wc(fd, buf->handle, 0,
> +							  PAGE_ALIGN(buf->size),
> +							  PROT_READ | PROT_WRITE);
> +		} else {
> +			ptr = xe_bo_mmap_ext(fd, buf->handle, buf->size,
> +					     PROT_READ | PROT_WRITE);
> +		}
>  	} else {
> -		ptr = xe_bo_mmap_ext(fd, buf->handle, buf->size,
> -				     PROT_READ | PROT_WRITE);
> +		ptr = buf->buf_ptr;
>  	}
>  
>  	switch (tiling) {
> @@ -538,7 +542,8 @@ static void draw_rect_mmap_wc(int fd, struct buf_data *buf, struct rect *rect,
>  		break;
>  	}
>  
> -	igt_assert(gem_munmap(ptr, buf->size) == 0);
> +	if (ptr != buf->buf_ptr)
> +		igt_assert(gem_munmap(ptr, buf->size) == 0);
>  }

I know the reason why you want to reuse pointer to the buffer
when it comes from the caller (to not mmap/munmap over and over again)
and all above is fine for me. But, you should assert in draw_rect_mmap_cpu()
and draw_rect_mmap_gtt() pointing we don't support such behavior on
those functions. I mean igt_assert_neq(buf->buf_ptr, NULL); is enought.

>  
>  static void draw_rect_pwrite_untiled(int fd, struct buf_data *buf,
> @@ -839,6 +844,7 @@ static void draw_rect_render(int fd, struct cmd_data *cmd_data,
>   * @buf_handle: the handle of the buffer where you're going to draw to
>   * @buf_size: the size of the buffer
>   * @buf_stride: the stride of the buffer
> + * @buf_ptr: Pointer to the mmapped buffer

You support this only for WC, so this should be documented here.
Above also suggests you _have_to_ provide mmaped buffer what is
not true.

Generally this code intention to not remap the buffer is fine for
me. Just fix details.

--
Zbigniew

>   * @buf_width: the width of the buffer
>   * @buf_height: the height of the buffer
>   * @tiling: the tiling of the buffer
> @@ -855,7 +861,7 @@ static void draw_rect_render(int fd, struct cmd_data *cmd_data,
>   */
>  void igt_draw_rect(int fd, struct buf_ops *bops, uint32_t ctx,
>  		   uint32_t buf_handle, uint32_t buf_size, uint32_t buf_stride,
> -		   int buf_width, int buf_height,
> +		   void *buf_ptr, int buf_width, int buf_height,
>  		   uint32_t tiling, enum igt_draw_method method,
>  		   int rect_x, int rect_y, int rect_w, int rect_h,
>  		   uint32_t color, int bpp)
> @@ -870,6 +876,7 @@ void igt_draw_rect(int fd, struct buf_ops *bops, uint32_t ctx,
>  		.handle = buf_handle,
>  		.size = buf_size,
>  		.stride = buf_stride,
> +		.buf_ptr = buf_ptr,
>  		.width = buf_width,
>  		.height = buf_height,
>  		.bpp = bpp,
> @@ -935,7 +942,7 @@ void igt_draw_rect_fb(int fd, struct buf_ops *bops,
>  		      int rect_w, int rect_h, uint32_t color)
>  {
>  	igt_draw_rect(fd, bops, ctx, fb->gem_handle, fb->size, fb->strides[0],
> -		      fb->width, fb->height,
> +		      fb->driver_priv, fb->width, fb->height,
>  		      igt_fb_mod_to_tiling(fb->modifier), method,
>  		      rect_x, rect_y, rect_w, rect_h, color,
>  		      igt_drm_format_to_bpp(fb->drm_format));
> diff --git a/lib/igt_draw.h b/lib/igt_draw.h
> index 1dec95e86..865641ef6 100644
> --- a/lib/igt_draw.h
> +++ b/lib/igt_draw.h
> @@ -54,7 +54,7 @@ bool igt_draw_supports_method(int fd, enum igt_draw_method method);
>  
>  void igt_draw_rect(int fd, struct buf_ops *bops, uint32_t ctx,
>  		   uint32_t buf_handle, uint32_t buf_size, uint32_t buf_stride,
> -		   int buf_width, int buf_height,
> +		   void *buf_ptr, int buf_width, int buf_height,
>  		   uint32_t tiling, enum igt_draw_method method,
>  		   int rect_x, int rect_y, int rect_w, int rect_h,
>  		   uint32_t color, int bpp);
> -- 
> 2.34.1
>