[igt-dev] [PATCH i-g-t 2/3] lib/igt_fb: include lib/intel_blt functions to blitter path
Juha-Pekka Heikkila
juhapekka.heikkila at gmail.com
Thu Jun 29 16:04:45 UTC 2023
On 29.6.2023 14.51, Karolina Stolarek wrote:
> 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).
For time being I think this will be valid also with Xe because Xe shares
display code with i915.
>
>>
>>>
>>>> + 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>
Thanks Karolina. In the mean time this patch was separated into two
parts, one where there were couple of more Xe related changes and other
where these blitting related changes. May I use this R-b here
https://patchwork.freedesktop.org/patch/544642/?series=119886&rev=4
?
Those few alignment fixes and small adjustments I'll apply before
pushing if I will not need another ci round.
/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