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

[Zbigniew Kempczyński]

On Wed, Aug 26, 2020 at 04:37:12PM +0100, Chris Wilson wrote:
> 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 :)

Agree, for 8bpp we would have to change the alignment again. I'll relax
it to 1 if alignment is not set.

> 
> >                 }
> >  
> >                 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?

To be honest Dominik has noticed some slowdown when multiple map/unmap
is done in some test/benchmark. Not sure which, I can ask him on Monday.

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

Generally I wanted to keep compatibility with drm_intel_bo_map() / unmap().
For other mappings test code author always can do mapping on its own
(gem_mmap__.*_coherent()) and not rely on wrapper.

> 
> > +
> > +       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); ?

Do you mean we can save cpu for unnecessary ioctl() call to GEM_SW_FINISH
when buffer is not framebuffer? In that case diverging intel_buf_unmap()/
intel_buf_flush_and_unmap() makes sense intel_buf_flush() doesn't itself
informs it unmaps the buffer.
 
> 
> 
> > +
> > +       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

Sure, agree, we should assert that in intel_buf_destroy().

Thanks for the review.
--
Zbigniew