| Message ID | 20240126210807.320671-4-juhapekka.heikkila@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable ccs compressed framebuffers on Xe2 | expand |
On 26/01/2024 21:08, Juha-Pekka Heikkila wrote: > Store pat index from xe_vma to xe_bo and check if bo was pinned > as framebuffer and verify pat index is not changing for pinned > framebuffers. > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > --- > drivers/gpu/drm/xe/xe_pt.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c > index de1030a47588..0a5d7c7543b1 100644 > --- a/drivers/gpu/drm/xe/xe_pt.c > +++ b/drivers/gpu/drm/xe/xe_pt.c > @@ -1208,10 +1208,11 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue > struct dma_fence *fence; > struct invalidation_fence *ifence = NULL; > struct xe_range_fence *rfence; > + struct xe_bo *bo = xe_vma_bo(vma); > int err; > > bind_pt_update.locked = false; > - xe_bo_assert_held(xe_vma_bo(vma)); > + xe_bo_assert_held(bo); > xe_vm_assert_held(vm); > > vm_dbg(&xe_vma_vm(vma)->xe->drm, > @@ -1252,8 +1253,22 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue > return ERR_PTR(-ENOMEM); > } > > + /* > + * BO which has XE_BO_SCANOUT_BIT set and was pinned as framebuffer > + * before with different PAT index cannot be bound with different PAT > + * index. This is to prevent switching CCS on/off from framebuffers > + * on the fly. > + */ > + if (bo) { > + if (bo->flags & XE_BO_SCANOUT_BIT && bo->pat_index_scanout && Note that pat_index = 0 is usually a valid index... > + bo->pat_index_scanout != vma->pat_index) > + return ERR_PTR(-EINVAL); > + > + bo->pat_index = vma->pat_index; > + } ...what about something like: if (bo.has_sealed_pat_index && bo.sealed_pat_index != vma->pat_index) return ERR_PTR(); else if (SCANOUT) { bo.has_sealed_pat_index = true; bo.sealed_pat_index = vma->pat_index; } Also, this and the previous patch should probably be squashed together? Other question is if we should only apply this on xe2? > + > fence = xe_migrate_update_pgtables(tile->migrate, > - vm, xe_vma_bo(vma), q, > + vm, bo, q, > entries, num_entries, > syncs, num_syncs, > &bind_pt_update.base); > @@ -1287,8 +1302,8 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue > DMA_RESV_USAGE_KERNEL : > DMA_RESV_USAGE_BOOKKEEP); > > - if (!xe_vma_has_no_bo(vma) && !xe_vma_bo(vma)->vm) > - dma_resv_add_fence(xe_vma_bo(vma)->ttm.base.resv, fence, > + if (!xe_vma_has_no_bo(vma) && !bo->vm) > + dma_resv_add_fence(bo->ttm.base.resv, fence, > DMA_RESV_USAGE_BOOKKEEP); > xe_pt_commit_bind(vma, entries, num_entries, rebind, > bind_pt_update.locked ? &deferred : NULL);
On 29.1.2024 13.33, Matthew Auld wrote: > On 26/01/2024 21:08, Juha-Pekka Heikkila wrote: >> Store pat index from xe_vma to xe_bo and check if bo was pinned >> as framebuffer and verify pat index is not changing for pinned >> framebuffers. >> >> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> >> --- >> drivers/gpu/drm/xe/xe_pt.c | 23 +++++++++++++++++++---- >> 1 file changed, 19 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c >> index de1030a47588..0a5d7c7543b1 100644 >> --- a/drivers/gpu/drm/xe/xe_pt.c >> +++ b/drivers/gpu/drm/xe/xe_pt.c >> @@ -1208,10 +1208,11 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct >> xe_vma *vma, struct xe_exec_queue >> struct dma_fence *fence; >> struct invalidation_fence *ifence = NULL; >> struct xe_range_fence *rfence; >> + struct xe_bo *bo = xe_vma_bo(vma); >> int err; >> bind_pt_update.locked = false; >> - xe_bo_assert_held(xe_vma_bo(vma)); >> + xe_bo_assert_held(bo); >> xe_vm_assert_held(vm); >> vm_dbg(&xe_vma_vm(vma)->xe->drm, >> @@ -1252,8 +1253,22 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct >> xe_vma *vma, struct xe_exec_queue >> return ERR_PTR(-ENOMEM); >> } >> + /* >> + * BO which has XE_BO_SCANOUT_BIT set and was pinned as framebuffer >> + * before with different PAT index cannot be bound with different >> PAT >> + * index. This is to prevent switching CCS on/off from framebuffers >> + * on the fly. >> + */ >> + if (bo) { >> + if (bo->flags & XE_BO_SCANOUT_BIT && bo->pat_index_scanout && > > Note that pat_index = 0 is usually a valid index... > >> + bo->pat_index_scanout != vma->pat_index) >> + return ERR_PTR(-EINVAL); >> + >> + bo->pat_index = vma->pat_index; >> + } > > ...what about something like: > > if (bo.has_sealed_pat_index && bo.sealed_pat_index != vma->pat_index) > return ERR_PTR(); > else if (SCANOUT) { > bo.has_sealed_pat_index = true; > bo.sealed_pat_index = vma->pat_index; > } > > Also, this and the previous patch should probably be squashed together? > Other question is if we should only apply this on xe2? Hi Matthew, thanks for the comments. I went ahead with making has_sealed_pat_index and it did make things much more clean. It's good idea, thanks. I'll soon send new version. Here I didn't go limit this to xe2 as the limit is coming from framebuffer code, if there come other use for this pat index sealing it doesn't need to be about xe2 on this part. > >> + >> fence = xe_migrate_update_pgtables(tile->migrate, >> - vm, xe_vma_bo(vma), q, >> + vm, bo, q, >> entries, num_entries, >> syncs, num_syncs, >> &bind_pt_update.base); >> @@ -1287,8 +1302,8 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct >> xe_vma *vma, struct xe_exec_queue >> DMA_RESV_USAGE_KERNEL : >> DMA_RESV_USAGE_BOOKKEEP); >> - if (!xe_vma_has_no_bo(vma) && !xe_vma_bo(vma)->vm) >> - dma_resv_add_fence(xe_vma_bo(vma)->ttm.base.resv, fence, >> + if (!xe_vma_has_no_bo(vma) && !bo->vm) >> + dma_resv_add_fence(bo->ttm.base.resv, fence, >> DMA_RESV_USAGE_BOOKKEEP); >> xe_pt_commit_bind(vma, entries, num_entries, rebind, >> bind_pt_update.locked ? &deferred : NULL);
Hi Juha-Pekka,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Juha-Pekka-Heikkila/drm-xe-pat-annotate-pat-index-table-with-compression-information/20240127-091231
base: https://gitlab.freedesktop.org/drm/xe/kernel.git drm-xe-next
patch link: https://lore.kernel.org/r/20240126210807.320671-4-juhapekka.heikkila%40gmail.com
patch subject: [PATCH 3/5] drm/xe: store bind time pat index to xe_bo
config: sparc-randconfig-r081-20240128 (https://download.01.org/0day-ci/archive/20240131/202401311604.1pLlAxeK-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202401311604.1pLlAxeK-lkp@intel.com/
New smatch warnings:
drivers/gpu/drm/xe/xe_pt.c:1265 __xe_pt_bind_vma() warn: possible memory leak of 'ifence'
drivers/gpu/drm/xe/xe_pt.c:1265 __xe_pt_bind_vma() warn: possible memory leak of 'rfence'
vim +/ifence +1265 drivers/gpu/drm/xe/xe_pt.c
dd08ebf6c3525a Matthew Brost 2023-03-30 1192 struct dma_fence *
9b9529ce379a08 Francois Dugast 2023-07-31 1193 __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue *q,
dd08ebf6c3525a Matthew Brost 2023-03-30 1194 struct xe_sync_entry *syncs, u32 num_syncs,
dd08ebf6c3525a Matthew Brost 2023-03-30 1195 bool rebind)
dd08ebf6c3525a Matthew Brost 2023-03-30 1196 {
dd08ebf6c3525a Matthew Brost 2023-03-30 1197 struct xe_vm_pgtable_update entries[XE_VM_MAX_LEVEL * 2 + 1];
dd08ebf6c3525a Matthew Brost 2023-03-30 1198 struct xe_pt_migrate_pt_update bind_pt_update = {
dd08ebf6c3525a Matthew Brost 2023-03-30 1199 .base = {
dd08ebf6c3525a Matthew Brost 2023-03-30 1200 .ops = xe_vma_is_userptr(vma) ? &userptr_bind_ops : &bind_ops,
dd08ebf6c3525a Matthew Brost 2023-03-30 1201 .vma = vma,
fd84041d094ce8 Matthew Brost 2023-07-19 1202 .tile_id = tile->id,
dd08ebf6c3525a Matthew Brost 2023-03-30 1203 },
dd08ebf6c3525a Matthew Brost 2023-03-30 1204 .bind = true,
dd08ebf6c3525a Matthew Brost 2023-03-30 1205 };
21ed3327e388c2 Matthew Brost 2023-06-22 1206 struct xe_vm *vm = xe_vma_vm(vma);
dd08ebf6c3525a Matthew Brost 2023-03-30 1207 u32 num_entries;
dd08ebf6c3525a Matthew Brost 2023-03-30 1208 struct dma_fence *fence;
5387e865d90e92 Matthew Brost 2023-01-27 1209 struct invalidation_fence *ifence = NULL;
fd84041d094ce8 Matthew Brost 2023-07-19 1210 struct xe_range_fence *rfence;
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1211 struct xe_bo *bo = xe_vma_bo(vma);
dd08ebf6c3525a Matthew Brost 2023-03-30 1212 int err;
dd08ebf6c3525a Matthew Brost 2023-03-30 1213
dd08ebf6c3525a Matthew Brost 2023-03-30 1214 bind_pt_update.locked = false;
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1215 xe_bo_assert_held(bo);
dd08ebf6c3525a Matthew Brost 2023-03-30 1216 xe_vm_assert_held(vm);
dd08ebf6c3525a Matthew Brost 2023-03-30 1217
21ed3327e388c2 Matthew Brost 2023-06-22 1218 vm_dbg(&xe_vma_vm(vma)->xe->drm,
dd08ebf6c3525a Matthew Brost 2023-03-30 1219 "Preparing bind, with range [%llx...%llx) engine %p.\n",
0e1a234618a86c Paulo Zanoni 2023-09-29 1220 xe_vma_start(vma), xe_vma_end(vma), q);
dd08ebf6c3525a Matthew Brost 2023-03-30 1221
876611c2b75689 Matt Roper 2023-06-01 1222 err = xe_pt_prepare_bind(tile, vma, entries, &num_entries, rebind);
dd08ebf6c3525a Matthew Brost 2023-03-30 1223 if (err)
dd08ebf6c3525a Matthew Brost 2023-03-30 1224 goto err;
c73acc1eeba5e3 Francois Dugast 2023-09-12 1225 xe_tile_assert(tile, num_entries <= ARRAY_SIZE(entries));
dd08ebf6c3525a Matthew Brost 2023-03-30 1226
876611c2b75689 Matt Roper 2023-06-01 1227 xe_vm_dbg_print_entries(tile_to_xe(tile), entries, num_entries);
fd84041d094ce8 Matthew Brost 2023-07-19 1228 xe_pt_calc_rfence_interval(vma, &bind_pt_update, entries,
fd84041d094ce8 Matthew Brost 2023-07-19 1229 num_entries);
dd08ebf6c3525a Matthew Brost 2023-03-30 1230
85dbfe47d07cdd Thomas Hellström 2023-06-05 1231 /*
85dbfe47d07cdd Thomas Hellström 2023-06-05 1232 * If rebind, we have to invalidate TLB on !LR vms to invalidate
85dbfe47d07cdd Thomas Hellström 2023-06-05 1233 * cached PTEs point to freed memory. on LR vms this is done
85dbfe47d07cdd Thomas Hellström 2023-06-05 1234 * automatically when the context is re-enabled by the rebind worker,
85dbfe47d07cdd Thomas Hellström 2023-06-05 1235 * or in fault mode it was invalidated on PTE zapping.
85dbfe47d07cdd Thomas Hellström 2023-06-05 1236 *
85dbfe47d07cdd Thomas Hellström 2023-06-05 1237 * If !rebind, and scratch enabled VMs, there is a chance the scratch
85dbfe47d07cdd Thomas Hellström 2023-06-05 1238 * PTE is already cached in the TLB so it needs to be invalidated.
85dbfe47d07cdd Thomas Hellström 2023-06-05 1239 * on !LR VMs this is done in the ring ops preceding a batch, but on
85dbfe47d07cdd Thomas Hellström 2023-06-05 1240 * non-faulting LR, in particular on user-space batch buffer chaining,
85dbfe47d07cdd Thomas Hellström 2023-06-05 1241 * it needs to be done here.
85dbfe47d07cdd Thomas Hellström 2023-06-05 1242 */
fdb6a05383fab3 Thomas Hellström 2023-11-27 1243 if ((rebind && !xe_vm_in_lr_mode(vm) && !vm->batch_invalidate_tlb) ||
06951c2ee72df2 Thomas Hellström 2023-12-09 1244 (!rebind && xe_vm_has_scratch(vm) && xe_vm_in_preempt_fence_mode(vm))) {
5387e865d90e92 Matthew Brost 2023-01-27 1245 ifence = kzalloc(sizeof(*ifence), GFP_KERNEL);
5387e865d90e92 Matthew Brost 2023-01-27 1246 if (!ifence)
5387e865d90e92 Matthew Brost 2023-01-27 1247 return ERR_PTR(-ENOMEM);
5387e865d90e92 Matthew Brost 2023-01-27 1248 }
5387e865d90e92 Matthew Brost 2023-01-27 1249
fd84041d094ce8 Matthew Brost 2023-07-19 1250 rfence = kzalloc(sizeof(*rfence), GFP_KERNEL);
fd84041d094ce8 Matthew Brost 2023-07-19 1251 if (!rfence) {
fd84041d094ce8 Matthew Brost 2023-07-19 1252 kfree(ifence);
fd84041d094ce8 Matthew Brost 2023-07-19 1253 return ERR_PTR(-ENOMEM);
fd84041d094ce8 Matthew Brost 2023-07-19 1254 }
fd84041d094ce8 Matthew Brost 2023-07-19 1255
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1256 /*
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1257 * BO which has XE_BO_SCANOUT_BIT set and was pinned as framebuffer
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1258 * before with different PAT index cannot be bound with different PAT
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1259 * index. This is to prevent switching CCS on/off from framebuffers
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1260 * on the fly.
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1261 */
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 @1262 if (bo) {
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1263 if (bo->flags & XE_BO_SCANOUT_BIT && bo->pat_index_scanout &&
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1264 bo->pat_index_scanout != vma->pat_index)
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 @1265 return ERR_PTR(-EINVAL);
Smatch wants a kfree(ifence) and kfree(rfence) before the return.
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1266
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1267 bo->pat_index = vma->pat_index;
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1268 }
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1269
08dea7674533cf Matt Roper 2023-06-01 1270 fence = xe_migrate_update_pgtables(tile->migrate,
6fb884b76bd164 Juha-Pekka Heikkila 2024-01-26 1271 vm, bo, q,
dd08ebf6c3525a Matthew Brost 2023-03-30 1272 entries, num_entries,
dd08ebf6c3525a Matthew Brost 2023-03-30 1273 syncs, num_syncs,
dd08ebf6c3525a Matthew Brost 2023-03-30 1274 &bind_pt_update.base);
diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c index de1030a47588..0a5d7c7543b1 100644 --- a/drivers/gpu/drm/xe/xe_pt.c +++ b/drivers/gpu/drm/xe/xe_pt.c @@ -1208,10 +1208,11 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue struct dma_fence *fence; struct invalidation_fence *ifence = NULL; struct xe_range_fence *rfence; + struct xe_bo *bo = xe_vma_bo(vma); int err; bind_pt_update.locked = false; - xe_bo_assert_held(xe_vma_bo(vma)); + xe_bo_assert_held(bo); xe_vm_assert_held(vm); vm_dbg(&xe_vma_vm(vma)->xe->drm, @@ -1252,8 +1253,22 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue return ERR_PTR(-ENOMEM); } + /* + * BO which has XE_BO_SCANOUT_BIT set and was pinned as framebuffer + * before with different PAT index cannot be bound with different PAT + * index. This is to prevent switching CCS on/off from framebuffers + * on the fly. + */ + if (bo) { + if (bo->flags & XE_BO_SCANOUT_BIT && bo->pat_index_scanout && + bo->pat_index_scanout != vma->pat_index) + return ERR_PTR(-EINVAL); + + bo->pat_index = vma->pat_index; + } + fence = xe_migrate_update_pgtables(tile->migrate, - vm, xe_vma_bo(vma), q, + vm, bo, q, entries, num_entries, syncs, num_syncs, &bind_pt_update.base); @@ -1287,8 +1302,8 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue DMA_RESV_USAGE_KERNEL : DMA_RESV_USAGE_BOOKKEEP); - if (!xe_vma_has_no_bo(vma) && !xe_vma_bo(vma)->vm) - dma_resv_add_fence(xe_vma_bo(vma)->ttm.base.resv, fence, + if (!xe_vma_has_no_bo(vma) && !bo->vm) + dma_resv_add_fence(bo->ttm.base.resv, fence, DMA_RESV_USAGE_BOOKKEEP); xe_pt_commit_bind(vma, entries, num_entries, rebind, bind_pt_update.locked ? &deferred : NULL);
Store pat index from xe_vma to xe_bo and check if bo was pinned as framebuffer and verify pat index is not changing for pinned framebuffers. Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> --- drivers/gpu/drm/xe/xe_pt.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)