From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9EF0610E164 for ; Thu, 29 Jun 2023 11:51:37 +0000 (UTC) Message-ID: <8af0e399-4a30-c919-32c9-472539318b99@intel.com> Date: Thu, 29 Jun 2023 13:51:25 +0200 Content-Language: en-US To: References: <20230627172210.4599-1-juhapekka.heikkila@gmail.com> <20230627172210.4599-3-juhapekka.heikkila@gmail.com> <41adde8f-53ce-856e-e355-8939f13988f2@intel.com> <744da5cd-3b56-1504-da6b-193e26e4c605@gmail.com> From: Karolina Stolarek In-Reply-To: <744da5cd-3b56-1504-da6b-193e26e4c605@gmail.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t 2/3] lib/igt_fb: include lib/intel_blt functions to blitter path List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: igt-dev@lists.freedesktop.org Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On 29.06.2023 13:17, Juha-Pekka Heikkila wrote: > On 29.6.2023 11.45, Karolina Stolarek wrote: >> Hi Juha-Pekka, >> >> On 27.06.2023 19:22, Juha-Pekka Heikkila wrote: >>> Use intel_blt functions on blitter path and on i915 devices with >>> flat ccs use blitter instead of render copy. >>> >>> Signed-off-by: Juha-Pekka Heikkila >>> --- >>>   lib/igt_fb.c | 209 +++++++++++++++++++++++++++++++++++++++++++++------ >>>   1 file changed, 186 insertions(+), 23 deletions(-) >>> >>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c >>> index 402fadf41..d8f2cc640 100644 >>> --- a/lib/igt_fb.c >>> +++ b/lib/igt_fb.c >>> @@ -35,6 +35,8 @@ >>>   #include "drmtest.h" >>>   #include "i915/gem_create.h" >>>   #include "i915/gem_mman.h" >>> +#include "intel_blt.h" >>> +#include "intel_mocs.h" >>>   #include "igt_aux.h" >>>   #include "igt_color_encoding.h" >>>   #include "igt_fb.h" >>> @@ -445,7 +447,7 @@ void igt_get_fb_tile_size(int fd, uint64_t >>> modifier, int fb_bpp, >>>           *height_ret = 1; >>>           break; >>>       case I915_FORMAT_MOD_X_TILED: >>> -        igt_require_i915(fd); >>> +        igt_require_intel(fd); >>>           if (intel_display_ver(intel_get_drm_devid(fd)) == 2) { >>>               *width_ret = 128; >>>               *height_ret = 16; >>> @@ -466,7 +468,7 @@ void igt_get_fb_tile_size(int fd, uint64_t >>> modifier, int fb_bpp, >>>       case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS: >>>       case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS: >>>       case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC: >>> -        igt_require_i915(fd); >>> +        igt_require_intel(fd); >>>           if (intel_display_ver(intel_get_drm_devid(fd)) == 2) { >>>               *width_ret = 128; >>>               *height_ret = 16; >>> @@ -2453,29 +2455,47 @@ struct fb_blit_upload { >>>       struct intel_bb *ibb; >>>   }; >>> -static bool fast_blit_ok(const struct igt_fb *fb) >>> +static enum blt_tiling_type fb_tile_to_blt_tile(uint64_t tile) >> >> Could we promote this function to intel_blt.c? These enums are not >> just fb-specific, they are legacy stuff, and I think that tests such >> as gem_blits would make use of such converter. > > We can do that but I'd rather commit this as is and then add form of > clean up on top with this type changes as it will then need all those > other places patched as well. Sure thing > >> >>>   { >>> -    int dev_id = intel_get_drm_devid(fb->fd); >>> -    int ver = intel_display_ver(dev_id); >>> - >>> -    if (ver < 9) >>> -        return false; >>> - >>> -    if (ver < 12) >>> -        return true; >>> +    switch (igt_fb_mod_to_tiling(tile)) { >>> +    case I915_TILING_NONE: >>> +        return T_LINEAR; >>> +    case I915_TILING_X: >>> +        return T_XMAJOR; >>> +    case I915_TILING_Y: >>> +        return T_YMAJOR; >>> +    case I915_TILING_4: >>> +        return T_TILE4; >>> +    case I915_TILING_Yf: >>> +        return T_YFMAJOR; >>> +    default: >>> +        igt_assert_f(0, "Unknown tiling!\n"); >>> +    } >>> +} >>> -    if (ver >= 13 && !IS_ALDERLAKE_P(dev_id)) >>> -        return true; >>> +static bool fast_blit_ok(const struct igt_fb *fb) >>> +{ >>> +    return blt_has_fast_copy(fb->fd) && >>> +        !is_ccs_modifier(fb->modifier) && >>> +        blt_fast_copy_supports_tiling(fb->fd, >>> +                           fb_tile_to_blt_tile(fb->modifier)); >> >> Nit: this line is slightly misaligned >> >>> +} >>> -    return fb->modifier != I915_FORMAT_MOD_X_TILED; >>> +static bool block_copy_ok(const struct igt_fb *fb) >>> +{ >>> +    return blt_has_block_copy(fb->fd) && >>> +        blt_block_copy_supports_tiling(fb->fd, >>> +                           fb_tile_to_blt_tile(fb->modifier)); >>>   } >>>   static bool blitter_ok(const struct igt_fb *fb) >>>   { >>> -    if (!is_i915_device(fb->fd)) >>> +    if (!is_intel_device(fb->fd)) >>>           return false; >>> -    if (is_ccs_modifier(fb->modifier)) >>> +    if ((is_ccs_modifier(fb->modifier) && >>> +         !HAS_FLATCCS(intel_get_drm_devid(fb->fd))) >>> +         || is_gen12_mc_ccs_modifier(fb->modifier)) >> >> Nit: This || should go line up >> >>>           return false; >>>       for (int i = 0; i < fb->num_planes; i++) { >>> @@ -2510,8 +2530,8 @@ static bool use_enginecopy(const struct igt_fb >>> *fb) >>>           return false; >>>       return fb->modifier == I915_FORMAT_MOD_Yf_TILED || >>> -           is_ccs_modifier(fb->modifier) || >>> -           (is_i915_device(fb->fd) && !gem_has_mappable_ggtt(fb->fd)); >>> +           (!HAS_FLATCCS(intel_get_drm_devid(fb->fd)) && >>> is_ccs_modifier(fb->modifier)) || >>> +           is_gen12_mc_ccs_modifier(fb->modifier); >>>   } >>>   static bool use_blitter(const struct igt_fb *fb) >>> @@ -2520,6 +2540,7 @@ static bool use_blitter(const struct igt_fb *fb) >>>           return false; >>>       return fb->modifier == I915_FORMAT_MOD_Y_TILED || >>> +           fb->modifier == I915_FORMAT_MOD_4_TILED || >>>              fb->modifier == I915_FORMAT_MOD_Yf_TILED || >>>              (is_i915_device(fb->fd) && !gem_has_mappable_ggtt(fb->fd)); >>>   } >>> @@ -2711,12 +2732,115 @@ static void copy_with_engine(struct >>> fb_blit_upload *blit, >>>       fini_buf(src); >>>   } >>> +static struct blt_copy_object *blt_fb_init(const struct igt_fb *fb, >>> +                       uint32_t plane, uint32_t memregion) >>> +{ >>> +    uint32_t name, handle; >>> +    struct blt_copy_object *blt; >>> +    enum blt_tiling_type blt_tile; >>> +    uint64_t stride; >>> + >>> +    blt = malloc(sizeof(*blt)); >>> +    igt_assert(blt); >>> + >>> +    name = gem_flink(fb->fd, fb->gem_handle); >>> +    handle = gem_open(fb->fd, name); >>> + >>> +    blt_tile = fb_tile_to_blt_tile(fb->modifier); >>> +    stride = blt_tile == T_LINEAR ? fb->strides[plane] : >>> fb->strides[plane] / 4; >>> + >>> +    blt_set_object(blt, handle, fb->size, memregion, >>> +               intel_get_uc_mocs(fb->fd), >>> +               blt_tile, >>> +               is_ccs_modifier(fb->modifier) ? COMPRESSION_ENABLED : >>> COMPRESSION_DISABLED, >>> +               is_gen12_mc_ccs_modifier(fb->modifier) ? >>> COMPRESSION_TYPE_MEDIA : COMPRESSION_TYPE_3D); >>> + >>> +    blt_set_geom(blt, stride, 0, 0, fb->width, >>> fb->plane_height[plane], 0, 0); >> >> Why do we go with fb->width instead of fb->plane_width[plane]? > > For blitter with planar formats will need to use fb->width, otherwise > part of uv plane would not get copied. > OK, I clearly lack the context :) Thank you for the explaination. >> >> Also, in blt_set_geom, you're passing 0s instead of x_offset and >> y_offset, which have to be set in dword6 and dword11. I'm not sure if >> that's correct. > > Here those offsets will always be zero, they cannot be anything else. > There are no x_offset or y_offset parameters that I could pass which > would make sense in this copy. Framebuffer will always be copied in full > when calling this function. Right, I was asking because I saw some offsets defined, and wasn't sure if these 0s were intentional. > >> >>> + >>> +    blt->plane_offset = fb->offsets[plane]; >>> + >>> +    blt->ptr = gem_mmap__device_coherent(fb->fd, handle, 0, fb->size, >>> +                         PROT_READ | PROT_WRITE); >>> +    return blt; >>> +} >>> + >>> +static enum blt_color_depth blt_get_bpp(const struct igt_fb *fb) >>> +{ >>> +    switch (fb->plane_bpp[0]) { >>> +    case 8: >>> +        return CD_8bit; >>> +    case 16: >>> +        return CD_16bit; >>> +    case 32: >>> +        return CD_32bit; >>> +    case 64: >>> +        return CD_64bit; >>> +    case 96: >>> +        return CD_96bit; >>> +    case 128: >>> +        return CD_128bit; >>> +    default: >>> +        igt_assert(0); >>> +    } >>> +} >>> + >>> +#define BLT_TARGET_RC(x) (x.compression == COMPRESSION_ENABLED && \ >>> +              x.compression_type == COMPRESSION_TYPE_3D) >>> + >>> +#define BLT_TARGET_MC(x) (x.compression == COMPRESSION_ENABLED && \ >>> +              x.compression_type == COMPRESSION_TYPE_MEDIA) >>> + >>> +static uint32_t blt_compression_format(struct blt_copy_data *blt, >>> +                       const struct igt_fb *fb) >>> +{ >>> +    if (blt->src.compression == COMPRESSION_DISABLED && >>> +        blt->dst.compression == COMPRESSION_DISABLED) >>> +        return 0; >>> + >>> +    if (BLT_TARGET_RC(blt->src) || BLT_TARGET_RC(blt->dst)) { >>> +        switch (blt->color_depth) { >>> +        case CD_32bit: >>> +            return 8; >>> +        default: >>> +            igt_assert_f(0, "COMPRESSION_TYPE_3D unknown color >>> depth\n"); >>> +        } >>> +    } else if (BLT_TARGET_MC(blt->src)) { >>> +        switch (fb->drm_format) { >>> +        case DRM_FORMAT_XRGB8888: >>> +            return 8; >>> +        case DRM_FORMAT_XYUV8888: >>> +            return 9; >>> +        case DRM_FORMAT_NV12: >>> +            return 9; >>> +        case DRM_FORMAT_P010: >>> +        case DRM_FORMAT_P012: >>> +        case DRM_FORMAT_P016: >>> +            return 8; >>> +        default: >>> +            igt_assert_f(0, "COMPRESSION_TYPE_MEDIA unknown format\n"); >>> +        } >>> +    } else if (BLT_TARGET_MC(blt->dst)) { >>> +        igt_assert_f(0, "Destination compression not supported on mc >>> ccs\n"); >>> +    } else { >>> +        igt_assert_f(0, "unknown compression\n"); >>> +    } >>> +} >>> + >>>   static void blitcopy(const struct igt_fb *dst_fb, >>>                const struct igt_fb *src_fb) >>>   { >>>       uint32_t src_tiling, dst_tiling; >>>       uint32_t ctx = 0; >>>       uint64_t ahnd = 0; >>> +    const intel_ctx_t *ictx = NULL; >>> +    struct intel_execution_engine2 *e; >>> +    uint32_t bb; >>> +    uint64_t bb_size = 4096; >>> +    struct blt_copy_data blt = {}; >>> +    struct blt_copy_object *src, *dst; >>> +    struct blt_block_copy_data_ext ext = {}, *pext = NULL; >>> +    uint32_t mem_region = HAS_FLATCCS(intel_get_drm_devid(src_fb->fd)) >>> +               ? REGION_LMEM(0) : REGION_SMEM; >>>       igt_assert_eq(dst_fb->fd, src_fb->fd); >>>       igt_assert_eq(dst_fb->num_planes, src_fb->num_planes); >>> @@ -2726,19 +2850,21 @@ static void blitcopy(const struct igt_fb >>> *dst_fb, >>>       if (is_i915_device(dst_fb->fd) && >>> !gem_has_relocations(dst_fb->fd)) { >>>           igt_require(gem_has_contexts(dst_fb->fd)); >>> +        ictx = intel_ctx_create_all_physical(src_fb->fd); >> >> Why do we take src_fb when we have dst_fb everywhere else? Maybe it >> doesn't matter much, but wanted to check it anyway. > > This I don't know why it is so, I saw some other code take this from > source hence I did the same. Right, I think that both should work, but you can leave it as it is. > >> >>>           ctx = gem_context_create(dst_fb->fd); >>>           ahnd = get_reloc_ahnd(dst_fb->fd, ctx); >>> + >>> +        igt_assert(__gem_create_in_memory_regions(src_fb->fd, >>> +                              &bb, >>> +                              &bb_size, >>> +                              mem_region) == 0); >>>       } >>>       for (int i = 0; i < dst_fb->num_planes; i++) { >>>           igt_assert_eq(dst_fb->plane_bpp[i], src_fb->plane_bpp[i]); >>>           igt_assert_eq(dst_fb->plane_width[i], src_fb->plane_width[i]); >>>           igt_assert_eq(dst_fb->plane_height[i], >>> src_fb->plane_height[i]); >>> -        /* >>> -         * On GEN12+ X-tiled format support is removed from the fast >>> -         * blit command, so use the XY_SRC blit command for it >>> -         * instead. >>> -         */ >>> + >>>           if (fast_blit_ok(src_fb) && fast_blit_ok(dst_fb)) { >> >> I know it's beyond the scope of this patch, but do you plan to switch >> to intel_blt.c's fast copy as well? > > No, that fast copy igt_blitter_fast_copy__raw() is here also for > platforms where relocations are supported. If this was changed there > would come need to add another blitting case or go around need for ahnd. Hmm, adding support for relocations to intel_blt.c would be a good move, noted. > >> >>>               igt_blitter_fast_copy__raw(dst_fb->fd, >>>                              ahnd, ctx, NULL, >>> @@ -2757,6 +2883,42 @@ static void blitcopy(const struct igt_fb *dst_fb, >>>                              dst_tiling, >>>                              0, 0 /* dst_x, dst_y */, >>>                              dst_fb->size); >>> +        } else if (ahnd && block_copy_ok(src_fb) && >>> block_copy_ok(dst_fb)) { >>> +            for_each_ctx_engine(src_fb->fd, ictx, e) { >>> +                if (gem_engine_can_block_copy(src_fb->fd, e)) >>> +                    break; >>> +            } >>> +            igt_assert_f(e, "No block copy capable engine found!\n"); >>> + >>> +            src = blt_fb_init(src_fb, i, mem_region); >>> +            dst = blt_fb_init(dst_fb, i, mem_region); >>> + >>> +            memset(&blt, 0, sizeof(blt)); >>> +            blt.color_depth = blt_get_bpp(src_fb); >> >> It gets bpp only from fb->plane_bpp[0], whereas in src_copy we take >> (dst)_fb->plane_bpp[i]. Is this bpp still correct? > > This is what is generally used. Currently when handling framebuffer > surfaces with pixel data information their bpp is always the same on i915. I wonder if this will be also the case for Xe (but that's a problem for future selves). > >> >>> +            blt_set_copy_object(&blt.src, src); >>> +            blt_set_copy_object(&blt.dst, dst); >>> + >>> +            if (HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))) { >> >> This check might not be correct on some platforms. To check if we need >> to fill in ext part, you could do a similar check as in commit >> 45da871dd268 ("tests/gem_ccs: Check for extended block-copy and >> compression support"). >> >> +Zbigniew, what is the status of HAS_FLATCCS? How is it useful in the >> context of blitter copy? I believe we want to switch to using command >> flags, like BLT_CMD_EXTENDED? Or am I misremembering something? >> > > This is bit double edged situation. Even if for MTL there would be added > support for compression into intel_blt, all the related handling for aux > ccs would still not be here on igt_fb.c. For flat ccs everything that is > needed is now in place and for time being it seems correct, everything > matches. Do note this in not only controlled by HAS_FLATCCS() but also > by modifiers which actually arrive onto this part of code. Hmm, I see, let's leave it for now and we can revisit it in the future. Thanks for answering my questions. Like I said, there are two nits that would be good to tidy up, but everything else looks good to me: Reviewed-by: Karolina Stolarek All the best, Karolina > > /Juha-Pekka > >> >>> +                blt_set_object_ext(&ext.src, >>> +                           blt_compression_format(&blt, src_fb), >>> +                           src_fb->width, src_fb->height, >>> +                           SURFACE_TYPE_2D); >>> + >>> +                blt_set_object_ext(&ext.dst, >>> +                           blt_compression_format(&blt, dst_fb), >>> +                           dst_fb->width, dst_fb->height, >>> +                           SURFACE_TYPE_2D); >>> + >>> +                pext = &ext; >>> +            } >>> + >>> +            blt_set_batch(&blt.bb, bb, bb_size, mem_region); >>> + >>> +            blt_block_copy(src_fb->fd, ictx, e, ahnd, &blt, pext); >>> +            gem_sync(src_fb->fd, blt.dst.handle); >>> + >>> +            blt_destroy_object(src_fb->fd, src); >>> +            blt_destroy_object(dst_fb->fd, dst); >>>           } else { >>>               igt_blitter_src_copy(dst_fb->fd, >>>                            ahnd, ctx, NULL, >>> @@ -2781,6 +2943,7 @@ static void blitcopy(const struct igt_fb *dst_fb, >>>       if (ctx) >>>           gem_context_destroy(dst_fb->fd, ctx); >>>       put_ahnd(ahnd); >>> +    intel_ctx_destroy(src_fb->fd, ictx); >>>   } >>>   static void free_linear_mapping(struct fb_blit_upload *blit) >