[igt-dev] [PATCH i-g-t 2/3] lib/igt_fb: include lib/intel_blt functions to blitter path
Karolina Stolarek
karolina.stolarek at intel.com
Thu Jun 29 11:51:25 UTC 2023
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 <juhapekka.heikkila at gmail.com>
>>> ---
>>> 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 <karolina.stolarek at intel.com>
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)
>
More information about the igt-dev
mailing list