Re: Radeon regression in 6.6 kernel

[Alex Deucher]

On Wed, Nov 29, 2023 at 11:41 AM Luben Tuikov <ltuikov89@xxxxxxxxx> wrote:
>
> On 2023-11-29 10:22, Alex Deucher wrote:
> > On Wed, Nov 29, 2023 at 8:50 AM Alex Deucher <alexdeucher@xxxxxxxxx> wrote:
> >>
> >> On Tue, Nov 28, 2023 at 11:45 PM Luben Tuikov <ltuikov89@xxxxxxxxx> wrote:
> >>>
> >>> On 2023-11-28 17:13, Alex Deucher wrote:
> >>>> On Mon, Nov 27, 2023 at 6:24 PM Phillip Susi <phill@xxxxxxxxxxxx> wrote:
> >>>>>
> >>>>> Alex Deucher <alexdeucher@xxxxxxxxx> writes:
> >>>>>
> >>>>>>> In that case those are the already known problems with the scheduler
> >>>>>>> changes, aren't they?
> >>>>>>
> >>>>>> Yes.  Those changes went into 6.7 though, not 6.6 AFAIK.  Maybe I'm
> >>>>>> misunderstanding what the original report was actually testing.  If it
> >>>>>> was 6.7, then try reverting:
> >>>>>> 56e449603f0ac580700621a356d35d5716a62ce5
> >>>>>> b70438004a14f4d0f9890b3297cd66248728546c
> >>>>>
> >>>>> At some point it was suggested that I file a gitlab issue, but I took
> >>>>> this to mean it was already known and being worked on.  -rc3 came out
> >>>>> today and still has the problem.  Is there a known issue I could track?
> >>>>>
> >>>>
> >>>> At this point, unless there are any objections, I think we should just
> >>>> revert the two patches
> >>> Uhm, no.
> >>>
> >>> Why "the two" patches?
> >>>
> >>> This email, part of this thread,
> >>>
> >>> https://lore.kernel.org/all/87r0kircdo.fsf@xxxxxxxxxxxxxxxx/
> >>>
> >>> clearly states that reverting *only* this commit,
> >>> 56e449603f0ac5 drm/sched: Convert the GPU scheduler to variable number of run-queues
> >>> *does not* mitigate the failed suspend. (Furthermore, this commit doesn't really change
> >>> anything operational, other than using an allocated array, instead of a static one, in DRM,
> >>> while the 2nd patch is solely contained within the amdgpu driver code.)
> >>>
> >>> Leaving us with only this change,
> >>> b70438004a14f4 drm/amdgpu: move buffer funcs setting up a level
> >>> to be at fault, as the kernel log attached in the linked email above shows.
> >>>
> >>> The conclusion is that only b70438004a14f4 needs reverting.
> >>
> >> b70438004a14f4 was a fix for 56e449603f0ac5.  Without b70438004a14f4,
> >> 56e449603f0ac5 breaks amdgpu.
> >
> > We can try and re-enable it in the next kernel.  I'm just not sure
> > we'll be able to fix this in time for 6.7 with the holidays and all
> > and I don't want to cause a lot of scheduler churn at the end of the
> > 6.7 cycle if we hold off and try and fix it.  Reverting seems like the
> > best short term solution.
>
> A lot of subsequent code has come in since commit 56e449603f0ac5, as it opened
> the opportunity for a 1-to-1 relationship between an entity and a scheduler.
> (Should've always been the case, from the outset. Not sure why it was coded as
> a fixed-size array.)
>
> Given that commit 56e449603f0ac5 has nothing to do with amdgpu, and the problem
> is wholly contained in amdgpu, and no other driver has this problem, there is
> no reason to have to "churn", i.e. go back and forth in DRM, only to cover up
> an init bug in amdgpu. See the response I just sent in @this thread:
> https://lore.kernel.org/r/05007cb0-871e-4dc7-af58-1351f4ba43e2@xxxxxxxxx
>
> And it's not like this issue is unknown. I first posted about it on 2023-10-16.
>
> Ideally, amdgpu would just fix their init code.

You can't make changes to core code that break other drivers.
Arguably 56e449603f0ac5 should not have gone in in the first place if
it broke amdgpu.  b70438004a14f4 was the code to fix amdgpu's init
code, but as a side effect it seems to have broken suspend for some
users.

Alex