From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id 49BDC10E98C for ; Fri, 10 Nov 2023 10:59:57 +0000 (UTC) Message-ID: Date: Fri, 10 Nov 2023 10:59:51 +0000 MIME-Version: 1.0 Content-Language: en-GB To: Akshata Jahagirdar References: <7970ae652cdefe8026863cfd16c82d57e46693a3.1699647258.git.akshata.jahagirdar@intel.com> From: Matthew Auld In-Reply-To: <7970ae652cdefe8026863cfd16c82d57e46693a3.1699647258.git.akshata.jahagirdar@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [igt-dev] [PATCH i-g-t 2/5] lib/intel_blt: Update calculation of ccs_size and size_of_ctrl_copy List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: igt-dev@lists.freedesktop.org, ayaz.siddiqui@intel.com Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On 10/11/2023 21:24, Akshata Jahagirdar wrote: > The Main-to-CCS Ratio for XE2 has been changed to 512:1. Update the CCS_RATIO macro to select relevant ratio based on platform. > Since the PAGE_SIZE of sysmem is 4K, update the size of ctrl_copy to reflect this change. Nit: very long line. > > Signed-off-by: Akshata Jahagirdar > --- > lib/intel_blt.c | 19 +++++++++++-------- > lib/intel_blt.h | 2 +- > 2 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/lib/intel_blt.c b/lib/intel_blt.c > index c0593930c..8168ce7b5 100644 > --- a/lib/intel_blt.c > +++ b/lib/intel_blt.c > @@ -948,15 +948,16 @@ int blt_block_copy(int fd, > return ret; > } > > -static uint16_t __ccs_size(const struct blt_ctrl_surf_copy_data *surf) > +static uint16_t __ccs_size(int fd, const struct blt_ctrl_surf_copy_data *surf) > { > uint32_t src_size, dst_size; > + uint16_t ccsratio = CCS_RATIO(fd); > > src_size = surf->src.access_type == DIRECT_ACCESS ? > - surf->src.size : surf->src.size / CCS_RATIO; > + surf->src.size : surf->src.size / ccsratio; > > dst_size = surf->dst.access_type == DIRECT_ACCESS ? > - surf->dst.size : surf->dst.size / CCS_RATIO; > + surf->dst.size : surf->dst.size / ccsratio; > > igt_assert_f(src_size <= dst_size, "dst size must be >= src size for CCS copy\n"); > > @@ -1118,6 +1119,7 @@ uint64_t emit_blt_ctrl_surf_copy(int fd, > uint64_t dst_offset, src_offset, bb_offset, alignment; > uint32_t bbe = MI_BATCH_BUFFER_END; > uint32_t *bb; > + uint16_t num_ccs_blocks = xe_get_default_alignment(fd) / (CCS_RATIO(fd)); > > igt_assert_f(ahnd, "ctrl-surf-copy supports softpin only\n"); > igt_assert_f(surf, "ctrl-surf-copy requires data to do ctrl-surf-copy blit\n"); > @@ -1136,7 +1138,7 @@ uint64_t emit_blt_ctrl_surf_copy(int fd, > data.xe2.dw00.dst_access_type = surf->dst.access_type; > > /* Ensure dst has size capable to keep src ccs aux */ > - data.xe2.dw00.size_of_ctrl_copy = __ccs_size(surf) / CCS_RATIO - 1; > + data.xe2.dw00.size_of_ctrl_copy = __ccs_size(fd, surf) / num_ccs_blocks - 1; > data.xe2.dw00.length = 0x3; > > data.xe2.dw01.src_address_lo = src_offset; > @@ -1155,7 +1157,7 @@ uint64_t emit_blt_ctrl_surf_copy(int fd, > data.gen12.dw00.dst_access_type = surf->dst.access_type; > > /* Ensure dst has size capable to keep src ccs aux */ > - data.gen12.dw00.size_of_ctrl_copy = __ccs_size(surf) / CCS_RATIO - 1; > + data.gen12.dw00.size_of_ctrl_copy = __ccs_size(fd,surf) / num_ccs_blocks - 1; > data.gen12.dw00.length = 0x3; > > data.gen12.dw01.src_address_lo = src_offset; > @@ -1794,7 +1796,8 @@ blt_create_object(const struct blt_copy_data *blt, uint32_t region, > uint64_t size = width * height * bpp / 8; > uint32_t stride = tiling == T_LINEAR ? width * 4 : width; > uint32_t handle; > - > + uint8_t pat_index = compression ? intel_get_pat_idx_uc_comp(blt->fd) : intel_get_pat_idx_uc(blt->fd); intel_get_pat_idx_uc_comp() has not been defined yet in the series? > + uint16_t cpu_caching = DRM_XE_GEM_CPU_CACHING_WC; Maybe make this conditional on xe2+ and compression = true? I figure we still want wb (if possible) for non-compressed stuff. Also commit doesn't really explain this change in behaviour? Should this change be separated? > igt_assert_f(blt->driver, "Driver isn't set, have you called blt_copy_init()?\n"); > > obj = calloc(1, sizeof(*obj)); > @@ -1808,13 +1811,13 @@ blt_create_object(const struct blt_copy_data *blt, uint32_t region, > flags |= XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM; > > size = ALIGN(size, xe_get_default_alignment(blt->fd)); > - handle = xe_bo_create_flags(blt->fd, 0, size, flags); > + handle = xe_bo_create_caching(blt->fd, 0, size, region, cpu_caching); > } else { > igt_assert(__gem_create_in_memory_regions(blt->fd, &handle, > &size, region) == 0); > } > > - blt_set_object(obj, handle, size, region, mocs_index, DEFAULT_PAT_INDEX, tiling, > + blt_set_object(obj, handle, size, region, mocs_index, pat_index, tiling, > compression, compression_type); > blt_set_geom(obj, stride, 0, 0, width, height, 0, 0); > > diff --git a/lib/intel_blt.h b/lib/intel_blt.h > index 5934ccd67..a7aeab595 100644 > --- a/lib/intel_blt.h > +++ b/lib/intel_blt.h > @@ -52,7 +52,7 @@ > #include "igt.h" > #include "intel_cmds_info.h" > > -#define CCS_RATIO 256 > +#define CCS_RATIO(xe) AT_LEAST_GEN(intel_get_drm_devid(xe), 20) ? 512 : 256 > > enum blt_color_depth { > CD_8bit,