[igt-dev] [PATCH i-g-t v31 01/32] lib/intel_bufops: add mapping on cpu / device

[Chris Wilson]

Quoting Zbigniew Kempczyński (2020-08-20 07:29:59)
> Simplify mapping intel_buf. To be extended with ref counting.
> Add intel_buf dump function for easy dump buffer to the file.
> Fixing returned type of bo size function.
> 
> Idempotency selftest is now skipped for default buf_ops creation
> to avoid time consuming comparison of HW and SW tiled buffers
> (this can be the problem for forked tests). Additional function
> buf_ops_create_with_selftest() was added to allow perform
> verification step where it is required.
> 
> Changing alignment from 4->2 (required for 16bpp render).
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> Cc: Dominik Grzegorzek <dominik.grzegorzek at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  lib/intel_bufops.c | 114 +++++++++++++++++++++++++++++++++++++++------
>  lib/intel_bufops.h |  15 +++++-
>  2 files changed, 113 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/intel_bufops.c b/lib/intel_bufops.c
> index 2a48fb0c..09433bed 100644
> --- a/lib/intel_bufops.c
> +++ b/lib/intel_bufops.c
> @@ -704,7 +704,7 @@ static void __intel_buf_init(struct buf_ops *bops,
>         igt_assert(buf);
>         igt_assert(width > 0 && height > 0);
>         igt_assert(bpp == 8 || bpp == 16 || bpp == 32);
> -       igt_assert(alignment % 4 == 0);
> +       igt_assert(alignment % 2 == 0);
>  
>         memset(buf, 0, sizeof(*buf));
>  
> @@ -757,7 +757,7 @@ static void __intel_buf_init(struct buf_ops *bops,
>  
>                         buf->surface[0].stride = ALIGN(width * (bpp / 8), tile_width);
>                 } else {
> -                       buf->surface[0].stride = ALIGN(width * (bpp / 8), alignment ?: 4);
> +                       buf->surface[0].stride = ALIGN(width * (bpp / 8), alignment ?: 2);

Just worried in case some one decides to use an 8bpp surface and we have
to change the restriction again :)

>                 }
>  
>                 buf->surface[0].size = buf->surface[0].stride * height;
> @@ -870,6 +870,52 @@ void intel_buf_destroy(struct intel_buf *buf)
>         free(buf);
>  }
>  
> +void *intel_buf_cpu_map(struct intel_buf *buf, bool write)
> +{
> +       int i915 = buf_ops_get_fd(buf->bops);
> +
> +       igt_assert(buf->ptr == NULL); /* already mapped */
> +
> +       buf->cpu_write = write;
> +       buf->ptr = gem_mmap__cpu_coherent(i915, buf->handle, 0,
> +                                         buf->surface[0].size,
> +                                         write ? PROT_WRITE : PROT_READ);
> +
> +       gem_set_domain(i915, buf->handle,
> +                      I915_GEM_DOMAIN_CPU,
> +                      write ? I915_GEM_DOMAIN_CPU : 0);

I still have mixed feeling about the automatic sync, but one thing that
stands out from the assert is that this interface is for temporary
short-lived mmaps. Do not start to see issues with the cost of the mmap()
outweighing the cost of the operation?

Definitely if we have to mmap a 4GiB object to write a single byte...

> +
> +       return buf->ptr;
> +}
> +
> +void *intel_buf_device_map(struct intel_buf *buf, bool write)
> +{
> +       int i915 = buf_ops_get_fd(buf->bops);
> +
> +       igt_assert(buf->ptr == NULL); /* already mapped */
> +
> +       buf->ptr = gem_mmap__device_coherent(i915, buf->handle, 0,
> +                                            buf->surface[0].size,
> +                                            write ? PROT_WRITE : PROT_READ);
> +
> +       gem_set_domain(i915, buf->handle,
> +                      I915_GEM_DOMAIN_WC,
> +                      write ? I915_GEM_DOMAIN_WC : 0);
> +
> +       return buf->ptr;
> +}
> +
> +void intel_buf_unmap(struct intel_buf *buf)
> +{
> +       igt_assert(buf->ptr);
> +
> +       if (buf->cpu_write)
> +               gem_sw_finish(buf_ops_get_fd(buf->bops), buf->handle);

Not sure if this should be done automatically; it may prevent the
igt_draw style of tests looking for the coherency issues around
framebuffers. Certainly wants a comment to say that it's consumers should
be reviewed to see if it is not better done by them, under their control.

So, wrap this an intel_buf_flush(bu); ?


> +
> +       munmap(buf->ptr, buf->surface[0].size);
> +       buf->ptr = NULL;

Do we have an intel_buf_destroy() where we could assert the buf->ptr is
not leaked?
-Chris