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

[Zbigniew Kempczyński]

On Fri, Jul 24, 2020 at 01:18:54PM +0100, Chris Wilson wrote:
> Quoting Zbigniew Kempczyński (2020-07-24 10:56:47)
> > 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.
> > 
> > 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 | 106 +++++++++++++++++++++++++++++++++++++++------
> >  lib/intel_bufops.h |  15 ++++++-
> >  2 files changed, 107 insertions(+), 14 deletions(-)
> > 
> > diff --git a/lib/intel_bufops.c b/lib/intel_bufops.c
> > index 2a48fb0c..7ce540fb 100644
> > --- a/lib/intel_bufops.c
> > +++ b/lib/intel_bufops.c
> > @@ -870,6 +870,48 @@ 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);
> > +
> > +       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);
> > +
> 
> Why wouldn't you do the sync here if you did it in the other?

Right.

> 
> Bleugh. Push the synchronisation and flushing to the caller. I have a
> strong dislike for it being magically applied, and believe such control
> should be in the user. (I want to encourage creativity, for we find the
> kernel bugs in the gaps between tests.)
> -Chris

This is helper, using it is optional. Currently there're 21 (cpu), 3 (wc)
of calls in the tests. I would like to keep this in the form it is to keep
compatibility with libdrm code (drm_intel_bo_map()). I think that function
still doesn't kill your creativity, it is just a wrapper. 

--
Zbigniew