[v4 1/3] lib/igt_draw: Dont mmap, if the buffer is already mmapped
Kulkarni, Vandita
vandita.kulkarni at intel.com
Wed Mar 27 05:26:15 UTC 2024
> -----Original Message-----
> From: Kempczynski, Zbigniew <zbigniew.kempczynski at intel.com>
> Sent: Wednesday, March 27, 2024 10:39 AM
> To: Kulkarni, Vandita <vandita.kulkarni at intel.com>
> Cc: igt-dev at lists.freedesktop.org
> Subject: Re: [v4 1/3] lib/igt_draw: Dont mmap, if the buffer is already
> mmapped
>
> On Tue, Mar 26, 2024 at 07:44:20PM +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.
> >
> > v2: Handle non null buf ptr cases in non WC mmap cases.
> > Squash changes in kms_frontbuffer_tracking which is using
> > this function. (Zbigniew)
> >
> > v3: Fix the buf ptr handling in non WC mmap cases (Zbigniew)
> >
> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni at intel.com>
>
> We discussed about this offline and solution with passing fb mapping in
> driver_priv field seems to be simplest way atm.
Thanks for your feedback, suggestions and reviews.
-Vandita
>
> > ---
> > lib/igt_draw.c | 61 ++++++++++++++++----------
> > lib/igt_draw.h | 2 +-
> > tests/intel/kms_frontbuffer_tracking.c | 2 +-
> > 3 files changed, 39 insertions(+), 26 deletions(-)
> >
> > diff --git a/lib/igt_draw.c b/lib/igt_draw.c index
> > 2c01d7b02..a4d5f2f0e 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;
> > @@ -448,6 +449,8 @@ static void draw_rect_mmap_cpu(int fd, struct
> > buf_data *buf, struct rect *rect, {
> > uint32_t *ptr;
> >
> > + igt_assert(buf->buf_ptr == NULL);
> > +
> > gem_set_domain(fd, buf->handle, I915_GEM_DOMAIN_CPU,
> > I915_GEM_DOMAIN_CPU);
> >
> > @@ -483,6 +486,8 @@ static void draw_rect_mmap_gtt(int fd, struct
> > buf_data *buf, struct rect *rect, {
> > uint32_t *ptr;
> >
> > + igt_assert(buf->buf_ptr == NULL);
> > +
> > gem_set_domain(fd, buf->handle, I915_GEM_DOMAIN_GTT,
> > I915_GEM_DOMAIN_GTT);
> >
> > @@ -499,28 +504,32 @@ static void draw_rect_mmap_wc(int fd, struct
> > buf_data *buf, struct rect *rect, {
> > uint32_t *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);
> > + 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 +547,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);
> > }
> >
> > static void draw_rect_pwrite_untiled(int fd, struct buf_data *buf, @@
> > -839,6 +849,8 @@ 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: if required can be used to pass already mmapped buffer ptr
> > + * only in case of wc mmap, can be NULL in any other case.
>
> I think you should write 'should be NULL' instead 'can be NULL' as you don't
> support cpu/gtt mapping.
Sure, will update.
>
> With this minor fix:
>
> Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
>
> --
> Zbigniew
>
> > * @buf_width: the width of the buffer
> > * @buf_height: the height of the buffer
> > * @tiling: the tiling of the buffer
> > @@ -855,7 +867,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 +882,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 +948,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);
> > diff --git a/tests/intel/kms_frontbuffer_tracking.c
> > b/tests/intel/kms_frontbuffer_tracking.c
> > index db2db0ff1..392539408 100644
> > --- a/tests/intel/kms_frontbuffer_tracking.c
> > +++ b/tests/intel/kms_frontbuffer_tracking.c
> > @@ -2254,7 +2254,7 @@ static void *busy_thread_func(void *data) {
> > while (!busy_thread.stop)
> > igt_draw_rect(drm.fd, drm.bops, 0, busy_thread.handle,
> > - busy_thread.size, busy_thread.stride,
> > + busy_thread.size, busy_thread.stride, NULL,
> > busy_thread.width, busy_thread.height,
> > busy_thread.tiling, IGT_DRAW_BLT, 0, 0,
> > busy_thread.width, busy_thread.height,
> > --
> > 2.43.2
> >
More information about the igt-dev
mailing list