From a9006ebde025698334293ea5d148e81ee9be8a32 Mon Sep 17 00:00:00 2001 From: Yiwei Zhang Date: Wed, 4 Jun 2025 12:58:10 -0700 Subject: [PATCH] pan/layout: document and prepare to fix planar plane offset This change: 1. Rename pan_image_wsi_layout to pan_image_layout_constraints. 1. Document that pan_image_layout_constraints::offset_B will be used to pass planar plane offset for native images as well. Update array_stride_B to exclude such offset accordingly. 2. Document that whether explicit layout is used further depends on if the passed wsi_row_pitch_B is non-zero or not. Updated the checks to base on the new boolean use_explicit_layout. 3. Update and document the intended slice offset_B behavior so that native and imported images are aligned. No behavior change from this commit since the client (panvk) hasn't set the pan_image_layout_constraints->offset_B for planar plane yet. Reviewed-by: Boris Brezillon Part-of: --- src/gallium/drivers/panfrost/pan_resource.c | 4 +- src/panfrost/lib/pan_layout.c | 73 ++++++++++++--------- src/panfrost/lib/pan_layout.h | 36 ++++++++-- src/panfrost/lib/tests/test-layout.cpp | 22 ++++--- src/panfrost/vulkan/panvk_image.c | 6 +- 5 files changed, 89 insertions(+), 52 deletions(-) diff --git a/src/gallium/drivers/panfrost/pan_resource.c b/src/gallium/drivers/panfrost/pan_resource.c index 172de042573..2289356913f 100644 --- a/src/gallium/drivers/panfrost/pan_resource.c +++ b/src/gallium/drivers/panfrost/pan_resource.c @@ -159,9 +159,9 @@ panfrost_resource_from_handle(struct pipe_screen *pscreen, : whandle->modifier; enum mali_texture_dimension dim = panfrost_translate_texture_dimension(templat->target); - struct pan_image_wsi_layout explicit_layout = { + struct pan_image_layout_constraints explicit_layout = { .offset_B = whandle->offset, - .row_pitch_B = whandle->stride, + .wsi_row_pitch_B = whandle->stride, .strict = dev->debug & PAN_DBG_STRICT_IMPORT, }; diff --git a/src/panfrost/lib/pan_layout.c b/src/panfrost/lib/pan_layout.c index fbc3ef80c30..a4eb556e088 100644 --- a/src/panfrost/lib/pan_layout.c +++ b/src/panfrost/lib/pan_layout.c @@ -241,13 +241,13 @@ pan_image_get_wsi_row_pitch(const struct pan_image_props *props, } static bool -wsi_row_pitch_to_row_stride(unsigned arch, const struct pan_image_props *props, - unsigned plane_idx, - const struct pan_image_wsi_layout *wsi_layout, - unsigned *row_stride_B) +wsi_row_pitch_to_row_stride( + unsigned arch, const struct pan_image_props *props, unsigned plane_idx, + const struct pan_image_layout_constraints *layout_constraints, + unsigned *row_stride_B) { unsigned row_align_mask, offset_align_mask, width_px; - unsigned wsi_row_pitch_B = wsi_layout->row_pitch_B; + unsigned wsi_row_pitch_B = layout_constraints->wsi_row_pitch_B; enum pipe_format format = props->format; uint64_t modifier = props->modifier; @@ -283,7 +283,7 @@ wsi_row_pitch_to_row_stride(unsigned arch, const struct pan_image_props *props, * This is something we can't change without risking breaking existing * users, so we enforce this explicit tile alignment only if we were * asked to. */ - if (wsi_layout->strict && + if (layout_constraints->strict && (afbc_tile_payload_row_stride_B % afbc_tile_payload_size_B)) { mesa_loge("WSI pitch is not aligned on an AFBC tile"); return false; @@ -335,7 +335,7 @@ wsi_row_pitch_to_row_stride(unsigned arch, const struct pan_image_props *props, return false; } - if (wsi_layout->offset_B & offset_align_mask) { + if (layout_constraints->offset_B & offset_align_mask) { mesa_loge("WSI offset not properly aligned"); return false; } @@ -363,15 +363,19 @@ pan_image_surface_offset(const struct pan_image_layout *layout, unsigned level, } bool -pan_image_layout_init(unsigned arch, const struct pan_image_props *props, - unsigned plane_idx, - const struct pan_image_wsi_layout *wsi_layout, - struct pan_image_layout *layout) +pan_image_layout_init( + unsigned arch, const struct pan_image_props *props, unsigned plane_idx, + const struct pan_image_layout_constraints *layout_constraints, + struct pan_image_layout *layout) { + /* Use explicit layout only when wsi_row_pitch_B is non-zero */ + const bool use_explicit_layout = + layout_constraints && layout_constraints->wsi_row_pitch_B; + /* Explicit stride only work with non-mipmap, non-array, single-sample * 2D image without CRC. */ - if (wsi_layout && + if (use_explicit_layout && (props->extent_px.depth > 1 || props->nr_samples > 1 || props->array_size > 1 || props->dim != MALI_TEXTURE_DIMENSION_2D || props->nr_slices > 1 || props->crc)) @@ -385,19 +389,16 @@ pan_image_layout_init(unsigned arch, const struct pan_image_props *props, const int align_req_B = afbc ? pan_afbc_header_row_stride_align(arch, props->format, props->modifier) - : afrc - ? pan_afrc_buffer_alignment_from_modifier(props->modifier) - : linear_or_tiled_row_align_req(arch, props->format, props->modifier); - unsigned wsi_row_stride_B = 0; - uint64_t offset_B = 0; + : (afrc ? pan_afrc_buffer_alignment_from_modifier(props->modifier) + : linear_or_tiled_row_align_req(arch, props->format, + props->modifier)); /* Mandate alignment */ - if (wsi_layout) { - if (!wsi_row_pitch_to_row_stride(arch, props, plane_idx, wsi_layout, - &wsi_row_stride_B)) + unsigned wsi_row_stride_B = 0; + if (use_explicit_layout) { + if (!wsi_row_pitch_to_row_stride(arch, props, plane_idx, + layout_constraints, &wsi_row_stride_B)) return false; - - offset_B = wsi_layout->offset_B; } const unsigned fmt_blocksize_B = @@ -433,6 +434,7 @@ pan_image_layout_init(unsigned arch, const struct pan_image_props *props, align_h_el *= pan_afbc_tile_size(props->modifier); } + uint64_t offset_B = layout_constraints ? layout_constraints->offset_B : 0; for (unsigned l = 0; l < props->nr_slices; ++l) { struct pan_image_slice_layout *slice = &layout->slices[l]; @@ -447,7 +449,7 @@ pan_image_layout_init(unsigned arch, const struct pan_image_props *props, /* Align levels to cache-line as a performance improvement for * linear/tiled and as a requirement for AFBC */ - if (!wsi_layout) + if (!use_explicit_layout) offset_B = ALIGN_POT(offset_B, pan_image_slice_align(props->modifier)); slice->offset_B = offset_B; @@ -465,12 +467,12 @@ pan_image_layout_init(unsigned arch, const struct pan_image_props *props, row_stride_B = ALIGN_POT(row_stride_B, align_req_B); } - if (wsi_layout && !afbc) { + if (use_explicit_layout && !afbc) { /* Explicit stride should be rejected by wsi_row_pitch_to_row_stride() * if it's too small. */ assert(wsi_row_stride_B >= row_stride_B); - if (!afrc || wsi_layout->strict) + if (!afrc || layout_constraints->strict) row_stride_B = wsi_row_stride_B; } else if (linear) { /* Keep lines alignment on 64 byte for performance */ @@ -487,11 +489,13 @@ pan_image_layout_init(unsigned arch, const struct pan_image_props *props, /* Explicit stride should be rejected by wsi_row_pitch_to_row_stride() * if it's too small. */ - assert(!wsi_layout || wsi_row_stride_B >= slice->row_stride_B); + assert(!use_explicit_layout || + wsi_row_stride_B >= slice->row_stride_B); - if (wsi_layout && wsi_layout->strict) { + if (use_explicit_layout && layout_constraints->strict) { slice->row_stride_B = wsi_row_stride_B; - slice_one_size_B = (uint64_t)wsi_layout->row_pitch_B * effective_height_el; + slice_one_size_B = (uint64_t)layout_constraints->wsi_row_pitch_B * + effective_height_el; } slice->afbc.stride_sb = @@ -545,10 +549,17 @@ pan_image_layout_init(unsigned arch, const struct pan_image_props *props, } /* Arrays and cubemaps have the entire miptree duplicated */ - layout->array_stride_B = ALIGN_POT(offset_B, 64); - if (wsi_layout) { - layout->data_size_B = offset_B - wsi_layout->offset_B; + layout->array_stride_B = + ALIGN_POT(offset_B - layout->slices[0].offset_B, 64); + if (use_explicit_layout) { + layout->data_size_B = offset_B - layout_constraints->offset_B; } else { + /* Native images start from offset 0, and the planar plane offset has + * been at least 4K page aligned below. So the base level slice offset + * should always be the same with the plane offset. + */ + assert(!layout_constraints || + layout_constraints->offset_B == layout->slices[0].offset_B); layout->data_size_B = ALIGN_POT( (uint64_t)layout->array_stride_B * (uint64_t)props->array_size, 4096); } diff --git a/src/panfrost/lib/pan_layout.h b/src/panfrost/lib/pan_layout.h index e7cc6f3a690..422ea054e11 100644 --- a/src/panfrost/lib/pan_layout.h +++ b/src/panfrost/lib/pan_layout.h @@ -22,6 +22,20 @@ extern "C" { #define MAX_IMAGE_PLANES 3 struct pan_image_slice_layout { + /* Offset in bytes relative to the base bo bound. + * + * Unlike gallium, vulkan has to report explicit image subres layout which + * disallows to hide the planar plane offset into the bo mapping. So we let + * the slice offsets to include the plane offset of the native multi-planar + * images to be consistent with the imported ones via explicit layout info. + * Doing so allows us to use a single code path to correctly: + * - report image subres layout and memory requirement + * - bind image memory + * + * XXX However, for native non-disjoint multi-planar images in panvk, this + * offset_B is currently relative to the planar plane offset instead of base + * bo offset. To be fixed in the follow up. + */ unsigned offset_B; /* For AFBC images, the number of bytes between two rows of AFBC @@ -94,9 +108,19 @@ struct pan_image_layout { uint64_t array_stride_B; }; -struct pan_image_wsi_layout { +struct pan_image_layout_constraints { + /* + * Plane offset in bytes + * - For native images, it's the planar plane offset. + * - For imported images, it's the user specified explicit offset. + * + * To be noted, this offset might be adjusted to choose an optimal alignment, + * unless the layout constraints are explicit (wsi_row_patch_B != 0). + */ unsigned offset_B; - unsigned row_pitch_B; + + /* Row pitch in bytes. Non-zero if layout is explicit. */ + unsigned wsi_row_pitch_B; bool strict; }; @@ -151,10 +175,10 @@ pan_image_mip_level_size(const struct pan_image_props *props, return size; } -bool pan_image_layout_init(unsigned arch, const struct pan_image_props *props, - unsigned plane_idx, - const struct pan_image_wsi_layout *wsi_layout, - struct pan_image_layout *layout); +bool pan_image_layout_init( + unsigned arch, const struct pan_image_props *props, unsigned plane_idx, + const struct pan_image_layout_constraints *layout_constraints, + struct pan_image_layout *layout); static inline unsigned pan_image_get_wsi_offset(const struct pan_image_layout *layout, unsigned level) diff --git a/src/panfrost/lib/tests/test-layout.cpp b/src/panfrost/lib/tests/test-layout.cpp index ee17bae1ee8..a9154e0d5d7 100644 --- a/src/panfrost/lib/tests/test-layout.cpp +++ b/src/panfrost/lib/tests/test-layout.cpp @@ -519,7 +519,7 @@ static unsigned archs[] = {4, 5, 6, 7, 9, 12, 13}; unsigned __export_row_pitch_B = \ pan_image_get_wsi_row_pitch(&iprops, __plane, &layout, 0); \ unsigned __export_offset_B = pan_image_get_wsi_offset(&layout, 0); \ - EXPECT_TRUE(__export_row_pitch_B == (__wsi_layout)->row_pitch_B && \ + EXPECT_TRUE(__export_row_pitch_B == (__wsi_layout)->wsi_row_pitch_B && \ __export_offset_B == (__wsi_layout)->offset_B) \ << " mismatch between import and export for vk.format, plane); - struct pan_image_wsi_layout plane_layout; + struct pan_image_layout_constraints plane_layout; if (explicit_info) { - plane_layout = (struct pan_image_wsi_layout){ + plane_layout = (struct pan_image_layout_constraints){ .offset_B = explicit_info->pPlaneLayouts[plane].offset, - .row_pitch_B = explicit_info->pPlaneLayouts[plane].rowPitch, + .wsi_row_pitch_B = explicit_info->pPlaneLayouts[plane].rowPitch, }; }