[PATCH i-g-t 1/2] lib/intel_blt: Add functions which extract and check object ccs data
Zbigniew Kempczyński
zbigniew.kempczynski at intel.com
Wed Apr 10 08:33:31 UTC 2024
On Fri, Mar 29, 2024 at 02:31:36PM +0100, Karolina Stolarek wrote:
> On 29.03.2024 10:16, Zbigniew Kempczyński wrote:
> > In tests which use compression we did very naive check in which we
> > compare source and destination surface. For linear we could expect
> > differences are related to compression, unfortunately for tiled data
> > this attitude is not appropriate due to natural data layout difference.
> >
> > Lets add helpers to extract surface ccs data and check if such
> > surface is compressed. Function which extracts ccs data is defined
> > as public to dump such data to png in the future.
> >
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Karolina Stolarek <karolina.stolarek at intel.com>
> > Cc: Akshata Jahagirdar <akshata.jahagirdar at intel.com>
> > ---
> > lib/intel_blt.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++
> > lib/intel_blt.h | 11 +++++
> > 2 files changed, 127 insertions(+)
> >
> > diff --git a/lib/intel_blt.c b/lib/intel_blt.c
> > index fe0a45cb8e..fa8e61732c 100644
> > --- a/lib/intel_blt.c
> > +++ b/lib/intel_blt.c
> > @@ -2094,6 +2094,122 @@ void blt_surface_info(const char *info, const struct blt_copy_object *obj)
> > obj->x1, obj->y1, obj->x2, obj->y2);
> > }
> > +/**
> > + * blt_surface_get_flatccs_data:
> > + * @fd: drm fd
> > + * @ctx: intel_ctx_t context
> > + * @e: blitter engine for @ctx
> > + * @ahnd: allocator handle
> > + * @obj: object from which flatccs data will be extracted
> > + *
> > + * Function executes ctrl-surf-copy to extract object ccs data from flatccs
> > + * area. Memory for the result ccs data are allocated in the function and must
> > + * be freed by the caller.
> > + */
> > +void blt_surface_get_flatccs_data(int fd,
> > + intel_ctx_t *ctx,
> > + const struct intel_execution_engine2 *e,
> > + uint64_t ahnd,
> > + const struct blt_copy_object *obj,
> > + uint32_t **ccsptr, uint64_t *sizeptr)
> > +{
> > + struct blt_ctrl_surf_copy_data surf = {};
> > + uint32_t bb, ccs, *ccsmap;
> > + uint64_t bb_size, ccssize = obj->size / CCS_RATIO(fd);
> > + uint32_t *ccscopy;
> > + uint32_t sysmem;
> > + uint8_t uc_mocs = intel_get_uc_mocs_index(fd);
> > + uint8_t comp_pat_index = DEFAULT_PAT_INDEX;
> > +
> > + igt_assert(ccsptr != NULL);
> > + igt_assert(sizeptr != NULL);
>
> I think we can drop these "!= NULL"
Ack.
>
> > +
> > + blt_ctrl_surf_copy_init(fd, &surf);
> > +
> > + ccscopy = (uint32_t *)malloc(ccssize);
> > + igt_assert(ccscopy);
> > +
> > + if (surf.driver == INTEL_DRIVER_XE) {
> > + uint16_t cpu_caching = __xe_default_cpu_caching(fd, sysmem, 0);
>
> sysmem is undefined here, I'd move the definition from below to here (i.e.,
> to the line before cpu_caching).
Eh, well spotted. I've added xe path first and most variables were
defined on the function beginning. When I've extended and added
i915 path I've missed that.
>
> Apart from that, the patch looks good (+ one nit below). I haven't test it
> on my machine, but I trust you played with your changes to verify their
> correctness :)
I've tested it on xe/xe2 machines and I didn't notice any issues.
>
> > + uint64_t ccs_bo_size = ALIGN(ccssize, xe_get_default_alignment(fd));
> > +
> > + if (AT_LEAST_GEN(intel_get_drm_devid(fd), 20) && obj->compression) {
> > + comp_pat_index = intel_get_pat_idx_uc_comp(fd);
> > + cpu_caching = DRM_XE_GEM_CPU_CACHING_WC;
> > + }
> > + sysmem = system_memory(fd);
> > + ccs = xe_bo_create_caching(fd, 0, ccs_bo_size, sysmem, 0, cpu_caching);
> > + bb_size = xe_bb_size(fd, SZ_4K);
> > + bb = xe_bo_create(fd, 0, bb_size, sysmem, 0);
> > + } else {
> > + sysmem = REGION_SMEM;
> > + ccs = gem_create(fd, ccssize);
> > + bb_size = 4096;
> > + igt_assert_eq(__gem_create(fd, &bb_size, &bb), 0);
> > + }
> > + blt_set_ctrl_surf_object(&surf.src, obj->handle, obj->region, obj->size,
> > + uc_mocs, comp_pat_index, BLT_INDIRECT_ACCESS);
> > + blt_set_ctrl_surf_object(&surf.dst, ccs, sysmem, ccssize, uc_mocs,
> > + DEFAULT_PAT_INDEX, DIRECT_ACCESS);
> > + blt_set_batch(&surf.bb, bb, bb_size, sysmem);
> > + blt_ctrl_surf_copy(fd, ctx, e, ahnd, &surf);
> > +
> > + if (surf.driver == INTEL_DRIVER_XE) {
> > + intel_ctx_xe_sync(ctx, true);
> > + ccsmap = xe_bo_map(fd, ccs, surf.dst.size);
> > + } else {
> > + gem_sync(fd, surf.dst.handle);
> > + ccsmap = gem_mmap__device_coherent(fd, ccs, 0, surf.dst.size,
> > + PROT_READ | PROT_WRITE);
> > + }
> > + memcpy(ccscopy, ccsmap, ccssize);
>
> The promised nit -- how about adding a blank line after else to improve
> readability? That would probably also mean adding one for "surf.driver ==
> INTEL_DRIVER_XE" else to keep it consistent.
I guess you mean INTEL_DRIVER_I915. I didn't added it explicitly as
get_intel_driver asserts on different values.
According to blank line, sure, I'll add it.
--
Zbigniew
>
> All the best,
> Karolina
>
> > + munmap(ccsmap, surf.dst.size);
> > +
> > + gem_close(fd, ccs);
> > + gem_close(fd, bb);
> > + put_offset(ahnd, ccs);
> > + put_offset(ahnd, bb);
> > + if (surf.driver == INTEL_DRIVER_XE)
> > + intel_allocator_bind(ahnd, 0, 0);
> > +
> > + *ccsptr = ccscopy;
> > + *sizeptr = ccssize;
> > +}
> > +
> > +/**
> > + * blt_surface_is_compressed:
> > + * @fd: drm fd
> > + * @ctx: intel_ctx_t context
> > + * @e: blitter engine for @ctx
> > + * @ahnd: allocator handle
> > + * @obj: object to check
> > + *
> > + * Function extracts object ccs data and check it contains any non-zero
> > + * value what means surface is compressed. Returns true if it is, otherwise
> > + * false.
> > + */
> > +bool blt_surface_is_compressed(int fd,
> > + intel_ctx_t *ctx,
> > + const struct intel_execution_engine2 *e,
> > + uint64_t ahnd,
> > + const struct blt_copy_object *obj)
> > +{
> > + uint64_t size;
> > + uint32_t *ptr;
> > + bool is_compressed = false;
> > +
> > + blt_surface_get_flatccs_data(fd, ctx, e, ahnd, obj, &ptr, &size);
> > + for (int i = 0; i < size / sizeof(*ptr); i++) {
> > + if (ptr[i] != 0) {
> > + is_compressed = true;
> > + break;
> > + }
> > + }
> > + free(ptr);
> > +
> > + return is_compressed;
> > +}
> > +
> > /**
> > * blt_surface_to_png:
> > * @fd: drm fd
> > diff --git a/lib/intel_blt.h b/lib/intel_blt.h
> > index 1f6c713596..d9c4d107f1 100644
> > --- a/lib/intel_blt.h
> > +++ b/lib/intel_blt.h
> > @@ -311,6 +311,17 @@ void blt_set_ctrl_surf_object(struct blt_ctrl_surf_copy_object *obj,
> > uint8_t mocs_index, uint8_t pat_index,
> > enum blt_access_type access_type);
> > +void blt_surface_get_flatccs_data(int fd,
> > + intel_ctx_t *ctx,
> > + const struct intel_execution_engine2 *e,
> > + uint64_t ahnd,
> > + const struct blt_copy_object *obj,
> > + uint32_t **ccsptr, uint64_t *sizeptr);
> > +bool blt_surface_is_compressed(int fd,
> > + intel_ctx_t *ctx,
> > + const struct intel_execution_engine2 *e,
> > + uint64_t ahnd,
> > + const struct blt_copy_object *obj);
> > void blt_surface_info(const char *info,
> > const struct blt_copy_object *obj);
> > void blt_surface_fill_rect(int fd, const struct blt_copy_object *obj,
More information about the igt-dev
mailing list