Re: [Nouveau] linux-next: Tree for Jun 10 (drivers/gpu/drm/nouveau/dispnv50/disp.c)
On 6/10/21 2:48 AM, Stephen Rothwell wrote:
Hi all,
Changes since 20210609:
on x86_64:
../drivers/gpu/drm/nouveau/dispnv50/disp.c: In function
‘nv50_sor_atomic_disable’:
../drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:52: error: ‘struct
nouveau_connector’ has no member named ‘backlight’
struct nouveau_backlight *backlight = nv_connector->backlight;
^~
../drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:28: error: dereferencing
pointer to incomplete type ‘struct nouveau_backlight’
if (backlight && backlight->uses_dpcd) {
^~
Full randconfig fil is attached.
config-r8561.gz
Description: application/gzip
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v10 07/10] mm: Device exclusive memory access
On Friday, 11 June 2021 11:00:34 AM AEST Peter Xu wrote: > On Fri, Jun 11, 2021 at 09:17:14AM +1000, Alistair Popple wrote: > > On Friday, 11 June 2021 9:04:19 AM AEST Peter Xu wrote: > > > On Fri, Jun 11, 2021 at 12:21:26AM +1000, Alistair Popple wrote: > > > > > Hmm, the thing is.. to me FOLL_SPLIT_PMD should have similar effect > > > > > to explicit > > > > > call split_huge_pmd_address(), afaict. Since both of them use > > > > > __split_huge_pmd() > > > > > internally which will generate that unwanted CLEAR notify. > > > > > > > > Agree that gup calls __split_huge_pmd() via split_huge_pmd_address() > > > > which will always CLEAR. However gup only calls > > > > split_huge_pmd_address() if it > > > > finds a thp pmd. In follow_pmd_mask() we have: > > > > > > > > if (likely(!pmd_trans_huge(pmdval))) > > > > return follow_page_pte(vma, address, pmd, flags, > > > > >pgmap); > > > > > > > > So I don't think we have a problem here. > > > > > > Sorry I didn't follow here.. We do FOLL_SPLIT_PMD after this check, > > > right? I > > > mean, if it's a thp for the current mm, afaict pmd_trans_huge() should > > > return > > > true above, so we'll skip follow_page_pte(); then we'll check > > > FOLL_SPLIT_PMD > > > and do the split, then the CLEAR notify. Hmm.. Did I miss something? > > > > That seems correct - if the thp is not mapped with a pmd we won't split and > > we > > won't CLEAR. If there is a thp pmd we will split and CLEAR, but in that > > case it > > is fine - we will retry, but the retry will won't CLEAR because the pmd has > > already been split. > > Aha! > > > > > The issue arises with doing it unconditionally in make device exclusive is > > that > > you *always* CLEAR even if there is no thp pmd to split. Or at least that's > > my > > understanding, please let me know if it doesn't make sense. > > Exactly. But if you see what I meant here, even if it can work like this, it > sounds still fragile, isn't it? I just feel something is slightly off there.. > > IMHO split_huge_pmd() checked pmd before calling __split_huge_pmd() for > performance, afaict, because if it's not a thp even without locking, then it > won't be, so further __split_huge_pmd() is not necessary. > > IOW, it's very legal if someday we'd like to let split_huge_pmd() call > __split_huge_pmd() directly, then AFAIU device exclusive API will be the 1st > one to be broken with that seems-to-be-irrelevant change I'm afraid.. Well I would argue the performance of memory notifiers is becoming increasingly important, and a change that causes them to be called unnecessarily is therefore not very legal. Likely the correct fix here is to optimise __split_huge_pmd() to only call the notifier if it's actually going to split a pmd. As you said though that's a completely different story which I think would be best done as a separate series. > This lets me goes back a step to think about why do we need this notifier at > all to cover this whole range of make_device_exclusive() procedure.. > > What I am thinking is, we're afraid some CPU accesses this page so the pte got > quickly restored when device atomic operation is carrying on. Then with this > notifier we'll be able to cancel it. Makes perfect sense. > > However do we really need to register this notifier so early? The thing is > the > GPU driver still has all the page locks, so even if there's a race to restore > the ptes, they'll block at taking the page lock until the driver releases it. > > IOW, I'm wondering whether the "non-fragile" way to do this is not do > mmu_interval_notifier_insert() that early: what if we register that notifier > after make_device_exclusive_range() returns but before page_unlock() somehow? > So before page_unlock(), race is protected fully by the lock itself; after > that, it's done by mmu notifier. Then maybe we don't need to worry about all > these notifications during marking exclusive (while we shouldn't)? The notifier is needed to protect against races with pte changes. Once a page has been marked for exclusive access the driver will update it's page tables to allow atomic access to the page. However in the meantime the page could become unmapped entirely or write protected. As I understand things the page lock won't protect against these kind of pte changes, hence the need for mmu_interval_read_begin/retry which allows the driver to hold a mutex protecting against invalidations via blocking the notifier until the device page tables have been updated. > Sorry in advance if I overlooked anything as I know little on device side > (even > less than mm itself). Also sorry to know that this series got marked > to-be-update in -mm; hopefully it'll still land soon even if it still needs > some rebase to other more important bugfixes - I definitely jumped in too late > even if to mess this all up. :-) I was thinking that was probably coming anyway, but I'm still hoping it will be just a rebase on Hugh's work
Re: [Nouveau] [PATCH v10 07/10] mm: Device exclusive memory access
On Friday, 11 June 2021 9:04:19 AM AEST Peter Xu wrote: > External email: Use caution opening links or attachments > > > On Fri, Jun 11, 2021 at 12:21:26AM +1000, Alistair Popple wrote: > > > Hmm, the thing is.. to me FOLL_SPLIT_PMD should have similar effect to > > > explicit > > > call split_huge_pmd_address(), afaict. Since both of them use > > > __split_huge_pmd() > > > internally which will generate that unwanted CLEAR notify. > > > > Agree that gup calls __split_huge_pmd() via split_huge_pmd_address() > > which will always CLEAR. However gup only calls split_huge_pmd_address() if > > it > > finds a thp pmd. In follow_pmd_mask() we have: > > > > if (likely(!pmd_trans_huge(pmdval))) > > return follow_page_pte(vma, address, pmd, flags, >pgmap); > > > > So I don't think we have a problem here. > > Sorry I didn't follow here.. We do FOLL_SPLIT_PMD after this check, right? I > mean, if it's a thp for the current mm, afaict pmd_trans_huge() should return > true above, so we'll skip follow_page_pte(); then we'll check FOLL_SPLIT_PMD > and do the split, then the CLEAR notify. Hmm.. Did I miss something? That seems correct - if the thp is not mapped with a pmd we won't split and we won't CLEAR. If there is a thp pmd we will split and CLEAR, but in that case it is fine - we will retry, but the retry will won't CLEAR because the pmd has already been split. The issue arises with doing it unconditionally in make device exclusive is that you *always* CLEAR even if there is no thp pmd to split. Or at least that's my understanding, please let me know if it doesn't make sense. - Alistair > -- > Peter Xu > ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v10 07/10] mm: Device exclusive memory access
On Friday, 11 June 2021 4:04:35 AM AEST Peter Xu wrote: > External email: Use caution opening links or attachments > > > On Thu, Jun 10, 2021 at 10:18:25AM +1000, Alistair Popple wrote: > > > > The main problem is split_huge_pmd_address() unconditionally calls a mmu > > > > notifier so I would need to plumb in passing an owner everywhere which > > > > could > > > > get messy. > > > > > > Could I ask why? split_huge_pmd_address() will notify with CLEAR, so I'm > > > a bit > > > confused why we need to pass over the owner. > > > > Sure, it is the same reason we need to pass it for the exclusive notifier. > > Any invalidation during the make exclusive operation will break the mmu read > > side critical section forcing a retry of the operation. The owner field is > > what > > is used to filter out invalidations (such as the exclusive invalidation) > > that > > don't need to be retried. > > Do you mean the mmu_interval_read_begin|retry() calls? Yep. > Hmm, the thing is.. to me FOLL_SPLIT_PMD should have similar effect to > explicit > call split_huge_pmd_address(), afaict. Since both of them use > __split_huge_pmd() > internally which will generate that unwanted CLEAR notify. Agree that gup calls __split_huge_pmd() via split_huge_pmd_address() which will always CLEAR. However gup only calls split_huge_pmd_address() if it finds a thp pmd. In follow_pmd_mask() we have: if (likely(!pmd_trans_huge(pmdval))) return follow_page_pte(vma, address, pmd, flags, >pgmap); So I don't think we have a problem here. > If that's the case, I think it fails because split_huge_pmd_address() will > trigger that CLEAR notify unconditionally (even if it's not a thp; not sure > whether it should be optimized to not notify at all... definitely another > story), while FOLL_SPLIT_PMD will skip the notify as it calls split_huge_pmd() > instead, who checks the pmd before calling __split_huge_pmd(). > > Does it also mean that if there's a real THP it won't really work? As then > FOLL_SPLIT_PMD will start to trigger that CLEAR notify too, I think.. > > -- > Peter Xu > ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] nouveau broken on Riva TNT2 in 5.13.0-rc4: NULL pointer dereference in nouveau_bo_sync_for_device
Am 10.06.21 um 19:50 schrieb Ondrej Zary: On Thursday 10 June 2021 08:43:06 Christian König wrote: Am 09.06.21 um 22:00 schrieb Ondrej Zary: On Wednesday 09 June 2021 11:21:05 Christian König wrote: Am 09.06.21 um 09:10 schrieb Ondrej Zary: On Wednesday 09 June 2021, Christian König wrote: Am 09.06.21 um 08:57 schrieb Ondrej Zary: [SNIP] Thanks for the heads up. So the problem with my patch is already fixed, isn't it? The NULL pointer dereference in nouveau_bo_wr16 introduced in 141b15e59175aa174ca1f7596188bd15a7ca17ba was fixed by aea656b0d05ec5b8ed5beb2f94c4dd42ea834e9d. That's the bug I hit when bisecting the original problem: NULL pointer dereference in nouveau_bo_sync_for_device It's caused by: # first bad commit: [e34b8feeaa4b65725b25f49c9b08a0f8707e8e86] drm/ttm: merge ttm_dma_tt back into ttm_tt Good that I've asked :) Ok that's a bit strange. e34b8feeaa4b65725b25f49c9b08a0f8707e8e86 was created mostly automated. Do you have the original backtrace of that NULL pointer deref once more? The original backtrace is here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2021%2F6%2F5%2F350data=04%7C01%7Cchristian.koenig%40amd.com%7C657222345e3242e7a6a608d92c383f66%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637589442963348551%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=ZkJs%2FR8MeQKUxwhJUC%2FG4Hi3T%2FMIftt%2FWRh%2B1%2BU5rUE%3Dreserved=0 And the problem is that ttm_dma->dma_address is NULL, right? Mhm, I don't see how that can happen since nouveau is using ttm_sg_tt_init(). Apart from that what nouveau does here is rather questionable since you need a coherent architecture for most things anyway, but that's not what we are trying to fix here. Can you try to narrow down if ttm_sg_tt_init is called before calling this function for the tt object in question? ttm_sg_tt_init is not called: [ 12.150124] nouveau :01:00.0: DRM: VRAM: 31 MiB [ 12.150133] nouveau :01:00.0: DRM: GART: 128 MiB [ 12.150143] nouveau :01:00.0: DRM: BMP version 5.6 [ 12.150151] nouveau :01:00.0: DRM: No DCB data found in VBIOS [ 12.151362] ttm_tt_init [ 12.151370] ttm_tt_init_fields [ 12.151374] ttm_tt_alloc_page_directory [ 12.151615] BUG: kernel NULL pointer dereference, address: Please add dump_stack(); to ttm_tt_init() and report back with the backtrace. I can't see how this is called from the nouveau code, only possibility I see is that it is maybe called through the AGP code somehow. Yes, you're right: [ 13.192663] Call Trace: [ 13.192678] dump_stack+0x54/0x68 [ 13.192690] ttm_tt_init+0x11/0x8a [ttm] [ 13.192699] ttm_agp_tt_create+0x39/0x51 [ttm] [ 13.192840] nouveau_ttm_tt_create+0x17/0x22 [nouveau] [ 13.192856] ttm_tt_create+0x78/0x8c [ttm] [ 13.192864] ttm_bo_handle_move_mem+0x7d/0xca [ttm] [ 13.192873] ttm_bo_validate+0x92/0xc8 [ttm] [ 13.192883] ttm_bo_init_reserved+0x216/0x243 [ttm] [ 13.192892] ttm_bo_init+0x45/0x65 [ttm] [ 13.193018] ? nouveau_bo_del_io_reserve_lru+0x48/0x48 [nouveau] [ 13.193150] nouveau_bo_init+0x8c/0x94 [nouveau] [ 13.193273] ? nouveau_bo_del_io_reserve_lru+0x48/0x48 [nouveau] [ 13.193407] nouveau_bo_new+0x44/0x57 [nouveau] [ 13.193537] nouveau_channel_prep+0xa3/0x269 [nouveau] [ 13.193665] nouveau_channel_new+0x3c/0x5f7 [nouveau] [ 13.193679] ? slab_free_freelist_hook+0x3b/0xa7 [ 13.193686] ? kfree+0x9e/0x11a [ 13.193781] ? nvif_object_sclass_put+0xd/0x16 [nouveau] [ 13.193908] nouveau_drm_device_init+0x2e2/0x646 [nouveau] [ 13.193924] ? pci_enable_device_flags+0x1e/0xac [ 13.194052] nouveau_drm_probe+0xeb/0x188 [nouveau] [ 13.194182] ? nouveau_drm_device_init+0x646/0x646 [nouveau] [ 13.194195] pci_device_probe+0x89/0xe9 [ 13.194205] really_probe+0x127/0x2a7 [ 13.194212] driver_probe_device+0x5b/0x87 [ 13.194219] device_driver_attach+0x2e/0x41 [ 13.194226] __driver_attach+0x7c/0x83 [ 13.194232] bus_for_each_dev+0x4c/0x66 [ 13.194238] driver_attach+0x14/0x16 [ 13.194244] ? device_driver_attach+0x41/0x41 [ 13.194251] bus_add_driver+0xc5/0x16c [ 13.194258] driver_register+0x87/0xb9 [ 13.194265] __pci_register_driver+0x38/0x3b [ 13.194271] ? 0xf0c0d000 [ 13.194362] nouveau_drm_init+0x14c/0x1000 [nouveau] How is ttm_dma_tt->dma_address allocated? Mhm, I need to double check how AGP is supposed to work. Since barely anybody is using it these days it is something which breaks from time to time. Thanks for the backtrace, Christian. I cannot find any assignment executed (in the working code): $ git grep dma_address\ = drivers/gpu/ drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c: sg->sgl->dma_address = addr; drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c:dma_address = >dma_address[offset >> PAGE_SHIFT]; drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c:dma_address = (mm_node->start << PAGE_SHIFT) + offset;
Re: [Nouveau] nouveau broken on Riva TNT2 in 5.13.0-rc4: NULL pointer dereference in nouveau_bo_sync_for_device
On Thursday 10 June 2021 08:43:06 Christian König wrote: > > Am 09.06.21 um 22:00 schrieb Ondrej Zary: > > On Wednesday 09 June 2021 11:21:05 Christian König wrote: > >> Am 09.06.21 um 09:10 schrieb Ondrej Zary: > >>> On Wednesday 09 June 2021, Christian König wrote: > Am 09.06.21 um 08:57 schrieb Ondrej Zary: > > [SNIP] > >> Thanks for the heads up. So the problem with my patch is already fixed, > >> isn't it? > > The NULL pointer dereference in nouveau_bo_wr16 introduced in > > 141b15e59175aa174ca1f7596188bd15a7ca17ba was fixed by > > aea656b0d05ec5b8ed5beb2f94c4dd42ea834e9d. > > > > That's the bug I hit when bisecting the original problem: > > NULL pointer dereference in nouveau_bo_sync_for_device > > It's caused by: > > # first bad commit: [e34b8feeaa4b65725b25f49c9b08a0f8707e8e86] drm/ttm: > > merge ttm_dma_tt back into ttm_tt > Good that I've asked :) > > Ok that's a bit strange. e34b8feeaa4b65725b25f49c9b08a0f8707e8e86 was > created mostly automated. > > Do you have the original backtrace of that NULL pointer deref once more? > >>> The original backtrace is here: > >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2021%2F6%2F5%2F350data=04%7C01%7Cchristian.koenig%40amd.com%7C4309ff021d5e4cbe948b08d92b813106%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637588657045383056%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=t70c9ktzPJzDaEAcO4wpQMv3TUo5b53cUy66AkLeVwE%3Dreserved=0 > >> And the problem is that ttm_dma->dma_address is NULL, right? Mhm, I > >> don't see how that can happen since nouveau is using ttm_sg_tt_init(). > >> > >> Apart from that what nouveau does here is rather questionable since you > >> need a coherent architecture for most things anyway, but that's not what > >> we are trying to fix here. > >> > >> Can you try to narrow down if ttm_sg_tt_init is called before calling > >> this function for the tt object in question? > > ttm_sg_tt_init is not called: > > [ 12.150124] nouveau :01:00.0: DRM: VRAM: 31 MiB > > [ 12.150133] nouveau :01:00.0: DRM: GART: 128 MiB > > [ 12.150143] nouveau :01:00.0: DRM: BMP version 5.6 > > [ 12.150151] nouveau :01:00.0: DRM: No DCB data found in VBIOS > > [ 12.151362] ttm_tt_init > > [ 12.151370] ttm_tt_init_fields > > [ 12.151374] ttm_tt_alloc_page_directory > > [ 12.151615] BUG: kernel NULL pointer dereference, address: > > Please add dump_stack(); to ttm_tt_init() and report back with the > backtrace. > > I can't see how this is called from the nouveau code, only possibility I > see is that it is maybe called through the AGP code somehow. Yes, you're right: [ 13.192663] Call Trace: [ 13.192678] dump_stack+0x54/0x68 [ 13.192690] ttm_tt_init+0x11/0x8a [ttm] [ 13.192699] ttm_agp_tt_create+0x39/0x51 [ttm] [ 13.192840] nouveau_ttm_tt_create+0x17/0x22 [nouveau] [ 13.192856] ttm_tt_create+0x78/0x8c [ttm] [ 13.192864] ttm_bo_handle_move_mem+0x7d/0xca [ttm] [ 13.192873] ttm_bo_validate+0x92/0xc8 [ttm] [ 13.192883] ttm_bo_init_reserved+0x216/0x243 [ttm] [ 13.192892] ttm_bo_init+0x45/0x65 [ttm] [ 13.193018] ? nouveau_bo_del_io_reserve_lru+0x48/0x48 [nouveau] [ 13.193150] nouveau_bo_init+0x8c/0x94 [nouveau] [ 13.193273] ? nouveau_bo_del_io_reserve_lru+0x48/0x48 [nouveau] [ 13.193407] nouveau_bo_new+0x44/0x57 [nouveau] [ 13.193537] nouveau_channel_prep+0xa3/0x269 [nouveau] [ 13.193665] nouveau_channel_new+0x3c/0x5f7 [nouveau] [ 13.193679] ? slab_free_freelist_hook+0x3b/0xa7 [ 13.193686] ? kfree+0x9e/0x11a [ 13.193781] ? nvif_object_sclass_put+0xd/0x16 [nouveau] [ 13.193908] nouveau_drm_device_init+0x2e2/0x646 [nouveau] [ 13.193924] ? pci_enable_device_flags+0x1e/0xac [ 13.194052] nouveau_drm_probe+0xeb/0x188 [nouveau] [ 13.194182] ? nouveau_drm_device_init+0x646/0x646 [nouveau] [ 13.194195] pci_device_probe+0x89/0xe9 [ 13.194205] really_probe+0x127/0x2a7 [ 13.194212] driver_probe_device+0x5b/0x87 [ 13.194219] device_driver_attach+0x2e/0x41 [ 13.194226] __driver_attach+0x7c/0x83 [ 13.194232] bus_for_each_dev+0x4c/0x66 [ 13.194238] driver_attach+0x14/0x16 [ 13.194244] ? device_driver_attach+0x41/0x41 [ 13.194251] bus_add_driver+0xc5/0x16c [ 13.194258] driver_register+0x87/0xb9 [ 13.194265] __pci_register_driver+0x38/0x3b [ 13.194271] ? 0xf0c0d000 [ 13.194362] nouveau_drm_init+0x14c/0x1000 [nouveau] How is ttm_dma_tt->dma_address allocated? I cannot find any assignment executed (in the working code): $ git grep dma_address\ = drivers/gpu/ drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c: sg->sgl->dma_address = addr; drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c:dma_address = >dma_address[offset >> PAGE_SHIFT]; drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c:dma_address = (mm_node->start << PAGE_SHIFT) + offset;
Re: [Nouveau] [PATCH] drm/nouveau: init the base GEM fields for internal BOs
Am 09.06.21 um 19:45 schrieb Mikko Perttunen: On 6/9/21 8:29 PM, Christian König wrote: TTMs buffer objects are based on GEM objects for quite a while and rely on initializing those fields before initializing the TTM BO. Noveau now doesn't init the GEM object for internally allocated BOs, Nouveau so make sure that we at least initialize some necessary fields. Signed-off-by: Christian König --- drivers/gpu/drm/nouveau/nouveau_bo.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c index 520b1ea9d16c..085023624fb0 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.c +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c @@ -149,6 +149,8 @@ nouveau_bo_del_ttm(struct ttm_buffer_object *bo) */ if (bo->base.dev) drm_gem_object_release(>base); + else + dma_resv_fini(>base._resv); kfree(nvbo); } @@ -330,6 +332,10 @@ nouveau_bo_new(struct nouveau_cli *cli, u64 size, int align, if (IS_ERR(nvbo)) return PTR_ERR(nvbo); + nvbo->bo.base.size = size; + dma_resv_init(>bo.base._resv); + drm_vma_node_reset(>bo.base.vma_node); + ret = nouveau_bo_init(nvbo, size, align, domain, sg, robj); if (ret) return ret; That works, thanks for the fix! Tested-by: Mikko Perttunen Thanks. Can anybody give me an rb that I can push this to drm-misc-next before the weekend? Regards, Christian. Mikko ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] nouveau broken on Riva TNT2 in 5.13.0-rc4: NULL pointer dereference in nouveau_bo_sync_for_device
Am 09.06.21 um 22:00 schrieb Ondrej Zary: On Wednesday 09 June 2021 11:21:05 Christian König wrote: Am 09.06.21 um 09:10 schrieb Ondrej Zary: On Wednesday 09 June 2021, Christian König wrote: Am 09.06.21 um 08:57 schrieb Ondrej Zary: [SNIP] Thanks for the heads up. So the problem with my patch is already fixed, isn't it? The NULL pointer dereference in nouveau_bo_wr16 introduced in 141b15e59175aa174ca1f7596188bd15a7ca17ba was fixed by aea656b0d05ec5b8ed5beb2f94c4dd42ea834e9d. That's the bug I hit when bisecting the original problem: NULL pointer dereference in nouveau_bo_sync_for_device It's caused by: # first bad commit: [e34b8feeaa4b65725b25f49c9b08a0f8707e8e86] drm/ttm: merge ttm_dma_tt back into ttm_tt Good that I've asked :) Ok that's a bit strange. e34b8feeaa4b65725b25f49c9b08a0f8707e8e86 was created mostly automated. Do you have the original backtrace of that NULL pointer deref once more? The original backtrace is here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2021%2F6%2F5%2F350data=04%7C01%7Cchristian.koenig%40amd.com%7C4309ff021d5e4cbe948b08d92b813106%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637588657045383056%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=t70c9ktzPJzDaEAcO4wpQMv3TUo5b53cUy66AkLeVwE%3Dreserved=0 And the problem is that ttm_dma->dma_address is NULL, right? Mhm, I don't see how that can happen since nouveau is using ttm_sg_tt_init(). Apart from that what nouveau does here is rather questionable since you need a coherent architecture for most things anyway, but that's not what we are trying to fix here. Can you try to narrow down if ttm_sg_tt_init is called before calling this function for the tt object in question? ttm_sg_tt_init is not called: [ 12.150124] nouveau :01:00.0: DRM: VRAM: 31 MiB [ 12.150133] nouveau :01:00.0: DRM: GART: 128 MiB [ 12.150143] nouveau :01:00.0: DRM: BMP version 5.6 [ 12.150151] nouveau :01:00.0: DRM: No DCB data found in VBIOS [ 12.151362] ttm_tt_init [ 12.151370] ttm_tt_init_fields [ 12.151374] ttm_tt_alloc_page_directory [ 12.151615] BUG: kernel NULL pointer dereference, address: Please add dump_stack(); to ttm_tt_init() and report back with the backtrace. I can't see how this is called from the nouveau code, only possibility I see is that it is maybe called through the AGP code somehow. Christian. ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
