Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CAPM=9tzs4n8dDQ_XVVPS_5jrBgsNkhDQvf-B_XmUg+EG_M2i4Q@mail.gmail.com>
 <CAHk-=whUp5Ufur6Bmv=H-rDDTNJJ-KVqOKkL+5FmR01jp0dbcA@mail.gmail.com>
 <CADnq5_Of-8ZyBxee0fZ=0x-eV-2NX_+e9sd-9nmuHdLugSHp2g@mail.gmail.com>
 <CAHk-=wi43xD06UgO2McDT3R=ze_aHgOGjcDOoggSwmQRv2kA+A@mail.gmail.com>
 <CADnq5_N0Ef+1VUoDLdpHfJXqZFuPYbx5Lq+94NWciHcQC+VrMg@mail.gmail.com>
 <CAPM=9tyAOnzwXyyPuoceZ1mimAkzxR-63YmKfVtcQX=swywNvg@mail.gmail.com>
 <CAHk-=wgghR4N-4XWjoK18NDkvjBL7i00ab8+otQg955pNGG_dQ@mail.gmail.com>
 <CAPM=9txE+0EH2Tv_0toDD52j0JO7iDZoJap6qmvMAnRaDRwJNg@mail.gmail.com> <CAPM=9tyjMUxAQnJJBVnXXc0tQTjywiK8PLxbJ_Jz4T_pcEospA@mail.gmail.com>
In-Reply-To: <CAPM=9tyjMUxAQnJJBVnXXc0tQTjywiK8PLxbJ_Jz4T_pcEospA@mail.gmail.com>
From:   Dave Airlie <airlied@gmail.com>
Date:   Fri, 7 Oct 2022 13:03:59 +1000
Message-ID: <CAPM=9tx+mxGphfr7TuUtXz_YgcFDrRi1oq0EhNU+UdmPPGDdUQ@mail.gmail.com>
Subject: Re: [git pull] drm for 6.1-rc1
To:     Linus Torvalds <torvalds@linux-foundation.org>,
        Arvind.Yadav@amd.com
Cc:     Alex Deucher <alexdeucher@gmail.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        LKML <linux-kernel@vger.kernel.org>,
        dri-devel <dri-devel@lists.freedesktop.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, 7 Oct 2022 at 12:54, Dave Airlie <airlied@gmail.com> wrote:
>
> On Fri, 7 Oct 2022 at 12:45, Dave Airlie <airlied@gmail.com> wrote:
> >
> > On Fri, 7 Oct 2022 at 09:45, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > On Thu, Oct 6, 2022 at 1:25 PM Dave Airlie <airlied@gmail.com> wrote:
> > > >
> > > >
> > > > [ 1234.778760] BUG: kernel NULL pointer dereference, address: 00000=
00000000088
> > > > [ 1234.778813] RIP: 0010:drm_sched_job_done.isra.0+0xc/0x140 [gpu_s=
ched]
> > >
> > > As far as I can tell, that's the line
> > >
> > >         struct drm_gpu_scheduler *sched =3D s_fence->sched;
> > >
> > > where 's_fence' is NULL. The code is
> > >
> > >    0: 0f 1f 44 00 00        nopl   0x0(%rax,%rax,1)
> > >    5: 41 54                push   %r12
> > >    7: 55                    push   %rbp
> > >    8: 53                    push   %rbx
> > >    9: 48 89 fb              mov    %rdi,%rbx
> > >    c:* 48 8b af 88 00 00 00 mov    0x88(%rdi),%rbp <-- trapping instr=
uction
> > >   13: f0 ff 8d f0 00 00 00 lock decl 0xf0(%rbp)
> > >   1a: 48 8b 85 80 01 00 00 mov    0x180(%rbp),%rax
> > >
> > > and that next 'lock decl' instruction would have been the
> > >
> > >         atomic_dec(&sched->hw_rq_count);
> > >
> > > at the top of drm_sched_job_done().
> > >
> > > Now, as to *why* you'd have a NULL s_fence, it would seem that
> > > drm_sched_job_cleanup() was called with an active job. Looking at tha=
t
> > > code, it does
> > >
> > >         if (kref_read(&job->s_fence->finished.refcount)) {
> > >                 /* drm_sched_job_arm() has been called */
> > >                 dma_fence_put(&job->s_fence->finished);
> > >         ...
> > >
> > > but then it does
> > >
> > >         job->s_fence =3D NULL;
> > >
> > > anyway, despite the job still being active. The logic of that kind of
> > > "fake refcount" escapes me. The above looks fundamentally racy, not t=
o
> > > say pointless and wrong (a refcount is a _count_, not a flag, so ther=
e
> > > could be multiple references to it, what says that you can just
> > > decrement one of them and say "I'm done").
> > >
> > > Now, _why_ any of that happens, I have no idea. I'm just looking at
> > > the immediate "that pointer is NULL" thing, and reacting to what look=
s
> > > like a completely bogus refcount pattern.
> > >
> > > But that odd refcount pattern isn't new, so it's presumably some user
> > > on the amd gpu side that changed.
> > >
> > > The problem hasn't happened again for me, but that's not saying a lot=
,
> > > since it was very random to begin with.
> >
> > I chased down the culprit to a drm sched patch, I'll send you a pull
> > with a revert in it.
> >
> > commit e4dc45b1848bc6bcac31eb1b4ccdd7f6718b3c86
> > Author: Arvind Yadav <Arvind.Yadav@amd.com>
> > Date:   Wed Sep 14 22:13:20 2022 +0530
> >
> >     drm/sched: Use parent fence instead of finished
> >
> >     Using the parent fence instead of the finished fence
> >     to get the job status. This change is to avoid GPU
> >     scheduler timeout error which can cause GPU reset.
> >
> >     Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com>
> >     Reviewed-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> >     Link: https://patchwork.freedesktop.org/patch/msgid/20220914164321.=
2156-6-Arvind.Yadav@amd.com
> >     Signed-off-by: Christian K=C3=B6nig <christian.koenig@amd.com>
> >
> > I'll let Arvind and Christian maybe work out what is going wrong there.
>
> I do spy two changes queued for -next that might be relevant, so I
> might try just pulling those instead.
>
> I'll send a PR in next hour once I test it.

Okay sent, let me know if you see any further problems.

Dave.