RE: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
[AMD Official Use Only - General]
> -Original Message-
> From: Greg KH
> Sent: Sunday, January 29, 2023 07:32
> To: Limonciello, Mario
> Cc: Linux regressions mailing list ; dri-
> de...@lists.freedesktop.org; sta...@vger.kernel.org;
> stanislav.lisovs...@intel.com; Zuo, Jerry ; amd-
> g...@lists.freedesktop.org; Lin, Wayne ; Guenter
> Roeck ; bske...@redhat.com
> Subject: Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into
> the atomic state"
>
> On Fri, Jan 27, 2023 at 03:02:41PM +, Limonciello, Mario wrote:
> > [Public]
> >
> >
> >
> > > -Original Message-
> > > From: Linux kernel regression tracking (Thorsten Leemhuis)
> > >
> > > Sent: Friday, January 27, 2023 03:15
> > > To: Greg KH ; Limonciello, Mario
> > >
> > > Cc: dri-de...@lists.freedesktop.org; sta...@vger.kernel.org;
> > > stanislav.lisovs...@intel.com; Zuo, Jerry ; amd-
> > > g...@lists.freedesktop.org; Lin, Wayne ; Guenter
> > > Roeck ; bske...@redhat.com
> > > Subject: Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info
> into
> > > the atomic state"
> > >
> > > On 27.01.23 08:39, Greg KH wrote:
> > > > On Fri, Jan 20, 2023 at 11:51:04AM -0600, Limonciello, Mario wrote:
> > > >> On 1/20/2023 11:46, Guenter Roeck wrote:
> > > >>> On Thu, Jan 12, 2023 at 04:50:44PM +0800, Wayne Lin wrote:
> > > >>>> This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f.
> > > >>>>
> > > >>>> [Why]
> > > >>>> Changes cause regression on amdgpu mst.
> > > >>>> E.g.
> > > >>>> In fill_dc_mst_payload_table_from_drm(), amdgpu expects to
> > > add/remove payload
> > > >>>> one by one and call fill_dc_mst_payload_table_from_drm() to
> update
> > > the HW
> > > >>>> maintained payload table. But previous change tries to go through
> all
> > > the
> > > >>>> payloads in mst_state and update amdpug hw maintained table in
> once
> > > everytime
> > > >>>> driver only tries to add/remove a specific payload stream only. The
> > > newly
> > > >>>> design idea conflicts with the implementation in amdgpu nowadays.
> > > >>>>
> > > >>>> [How]
> > > >>>> Revert this patch first. After addressing all regression problems
> caused
> > > by
> > > >>>> this previous patch, will add it back and adjust it.
> > > >>>
> > > >>> Has there been any progress on this revert, or on fixing the
> underlying
> > > >>> problem ?
> > > >>>
> > > >>> Thanks,
> > > >>> Guenter
> > > >>
> > > >> Hi Guenter,
> > > >>
> > > >> Wayne is OOO for CNY, but let me update you.
> > > >>
> > > >> Harry has sent out this series which is a collection of proper fixes.
> > > >> https://patchwork.freedesktop.org/series/113125/
> > > >>
> > > >> Once that's reviewed and accepted, 4 of them are applicable for 6.1.
> > > >
> > > > Any hint on when those will be reviewed and accepted? patchwork
> > > doesn't
> > > > show any activity on them, or at least I can't figure it out...
> > >
> > > I didn't look closer (hence please correct me if I'm wrong), but the
> > > core changes afaics are in the DRM pull airlied send a few hours ago to
> > > Linus (note the "amdgpu […] DP MST fixes" line):
> > >
> > >
> https://lore.kernel.org/all/CAPM%3D9tzuu4xnx6T5v7sKsK%2BA5HEaPOc1ie
> > > myznsyqzgztj%3d...@mail.gmail.com/
> >
> > That's right. There are 4 commits in that PR with the appropriate stable
> > tags
> > that should fix the majority of the MST issues introduced in 6.1 by
> 4d07b0bc40340
> > ("drm/display/dp_mst: Move all payload info into the atomic state"):
> >
> > drm/amdgpu/display/mst: Fix mst_state->pbn_div and slot count
> assignments
> > drm/amdgpu/display/mst: limit payload to be updated one by one
> > drm/amdgpu/display/mst: update mst_mgr relevant variable when long
> HPD
> > drm/display/dp_mst: Correct the kref of port.
> >
> > There will be follow ups for any remaining corner cases.
>
> Great, thanks for this, all are now queued up in the 6.1.y queue.
>
> greg k-h
Greg,
My apologies if this has been covered elsewhere and I missed it but I was
wondering if there was a decision made for whether 6.1.y will be an LTS kernel
release or not?
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
On Fri, Jan 27, 2023 at 03:02:41PM +, Limonciello, Mario wrote:
> [Public]
>
>
>
> > -Original Message-
> > From: Linux kernel regression tracking (Thorsten Leemhuis)
> >
> > Sent: Friday, January 27, 2023 03:15
> > To: Greg KH ; Limonciello, Mario
> >
> > Cc: dri-de...@lists.freedesktop.org; sta...@vger.kernel.org;
> > stanislav.lisovs...@intel.com; Zuo, Jerry ; amd-
> > g...@lists.freedesktop.org; Lin, Wayne ; Guenter
> > Roeck ; bske...@redhat.com
> > Subject: Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into
> > the atomic state"
> >
> > On 27.01.23 08:39, Greg KH wrote:
> > > On Fri, Jan 20, 2023 at 11:51:04AM -0600, Limonciello, Mario wrote:
> > >> On 1/20/2023 11:46, Guenter Roeck wrote:
> > >>> On Thu, Jan 12, 2023 at 04:50:44PM +0800, Wayne Lin wrote:
> > >>>> This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f.
> > >>>>
> > >>>> [Why]
> > >>>> Changes cause regression on amdgpu mst.
> > >>>> E.g.
> > >>>> In fill_dc_mst_payload_table_from_drm(), amdgpu expects to
> > add/remove payload
> > >>>> one by one and call fill_dc_mst_payload_table_from_drm() to update
> > the HW
> > >>>> maintained payload table. But previous change tries to go through all
> > the
> > >>>> payloads in mst_state and update amdpug hw maintained table in once
> > everytime
> > >>>> driver only tries to add/remove a specific payload stream only. The
> > newly
> > >>>> design idea conflicts with the implementation in amdgpu nowadays.
> > >>>>
> > >>>> [How]
> > >>>> Revert this patch first. After addressing all regression problems
> > >>>> caused
> > by
> > >>>> this previous patch, will add it back and adjust it.
> > >>>
> > >>> Has there been any progress on this revert, or on fixing the underlying
> > >>> problem ?
> > >>>
> > >>> Thanks,
> > >>> Guenter
> > >>
> > >> Hi Guenter,
> > >>
> > >> Wayne is OOO for CNY, but let me update you.
> > >>
> > >> Harry has sent out this series which is a collection of proper fixes.
> > >> https://patchwork.freedesktop.org/series/113125/
> > >>
> > >> Once that's reviewed and accepted, 4 of them are applicable for 6.1.
> > >
> > > Any hint on when those will be reviewed and accepted? patchwork
> > doesn't
> > > show any activity on them, or at least I can't figure it out...
> >
> > I didn't look closer (hence please correct me if I'm wrong), but the
> > core changes afaics are in the DRM pull airlied send a few hours ago to
> > Linus (note the "amdgpu […] DP MST fixes" line):
> >
> > https://lore.kernel.org/all/CAPM%3D9tzuu4xnx6T5v7sKsK%2BA5HEaPOc1ie
> > myznsyqzgztj%3d...@mail.gmail.com/
>
> That's right. There are 4 commits in that PR with the appropriate stable tags
> that should fix the majority of the MST issues introduced in 6.1 by
> 4d07b0bc40340
> ("drm/display/dp_mst: Move all payload info into the atomic state"):
>
> drm/amdgpu/display/mst: Fix mst_state->pbn_div and slot count
> assignments
> drm/amdgpu/display/mst: limit payload to be updated one by one
> drm/amdgpu/display/mst: update mst_mgr relevant variable when long HPD
> drm/display/dp_mst: Correct the kref of port.
>
> There will be follow ups for any remaining corner cases.
Great, thanks for this, all are now queued up in the 6.1.y queue.
greg k-h
RE: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
[Public]
> -Original Message-
> From: Linux kernel regression tracking (Thorsten Leemhuis)
>
> Sent: Friday, January 27, 2023 03:15
> To: Greg KH ; Limonciello, Mario
>
> Cc: dri-de...@lists.freedesktop.org; sta...@vger.kernel.org;
> stanislav.lisovs...@intel.com; Zuo, Jerry ; amd-
> g...@lists.freedesktop.org; Lin, Wayne ; Guenter
> Roeck ; bske...@redhat.com
> Subject: Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into
> the atomic state"
>
> On 27.01.23 08:39, Greg KH wrote:
> > On Fri, Jan 20, 2023 at 11:51:04AM -0600, Limonciello, Mario wrote:
> >> On 1/20/2023 11:46, Guenter Roeck wrote:
> >>> On Thu, Jan 12, 2023 at 04:50:44PM +0800, Wayne Lin wrote:
> >>>> This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f.
> >>>>
> >>>> [Why]
> >>>> Changes cause regression on amdgpu mst.
> >>>> E.g.
> >>>> In fill_dc_mst_payload_table_from_drm(), amdgpu expects to
> add/remove payload
> >>>> one by one and call fill_dc_mst_payload_table_from_drm() to update
> the HW
> >>>> maintained payload table. But previous change tries to go through all
> the
> >>>> payloads in mst_state and update amdpug hw maintained table in once
> everytime
> >>>> driver only tries to add/remove a specific payload stream only. The
> newly
> >>>> design idea conflicts with the implementation in amdgpu nowadays.
> >>>>
> >>>> [How]
> >>>> Revert this patch first. After addressing all regression problems caused
> by
> >>>> this previous patch, will add it back and adjust it.
> >>>
> >>> Has there been any progress on this revert, or on fixing the underlying
> >>> problem ?
> >>>
> >>> Thanks,
> >>> Guenter
> >>
> >> Hi Guenter,
> >>
> >> Wayne is OOO for CNY, but let me update you.
> >>
> >> Harry has sent out this series which is a collection of proper fixes.
> >> https://patchwork.freedesktop.org/series/113125/
> >>
> >> Once that's reviewed and accepted, 4 of them are applicable for 6.1.
> >
> > Any hint on when those will be reviewed and accepted? patchwork
> doesn't
> > show any activity on them, or at least I can't figure it out...
>
> I didn't look closer (hence please correct me if I'm wrong), but the
> core changes afaics are in the DRM pull airlied send a few hours ago to
> Linus (note the "amdgpu […] DP MST fixes" line):
>
> https://lore.kernel.org/all/CAPM%3D9tzuu4xnx6T5v7sKsK%2BA5HEaPOc1ie
> myznsyqzgztj%3d...@mail.gmail.com/
That's right. There are 4 commits in that PR with the appropriate stable tags
that should fix the majority of the MST issues introduced in 6.1 by
4d07b0bc40340
("drm/display/dp_mst: Move all payload info into the atomic state"):
drm/amdgpu/display/mst: Fix mst_state->pbn_div and slot count assignments
drm/amdgpu/display/mst: limit payload to be updated one by one
drm/amdgpu/display/mst: update mst_mgr relevant variable when long HPD
drm/display/dp_mst: Correct the kref of port.
There will be follow ups for any remaining corner cases.
>
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
Hey Greg, On 1/27/23 02:39, Greg KH wrote: On Fri, Jan 20, 2023 at 11:51:04AM -0600, Limonciello, Mario wrote: On 1/20/2023 11:46, Guenter Roeck wrote: On Thu, Jan 12, 2023 at 04:50:44PM +0800, Wayne Lin wrote: This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f. [Why] Changes cause regression on amdgpu mst. E.g. In fill_dc_mst_payload_table_from_drm(), amdgpu expects to add/remove payload one by one and call fill_dc_mst_payload_table_from_drm() to update the HW maintained payload table. But previous change tries to go through all the payloads in mst_state and update amdpug hw maintained table in once everytime driver only tries to add/remove a specific payload stream only. The newly design idea conflicts with the implementation in amdgpu nowadays. [How] Revert this patch first. After addressing all regression problems caused by this previous patch, will add it back and adjust it. Has there been any progress on this revert, or on fixing the underlying problem ? Thanks, Guenter Hi Guenter, Wayne is OOO for CNY, but let me update you. Harry has sent out this series which is a collection of proper fixes. https://patchwork.freedesktop.org/series/113125/ Once that's reviewed and accepted, 4 of them are applicable for 6.1. Any hint on when those will be reviewed and accepted? patchwork doesn't show any activity on them, or at least I can't figure it out... These patches have already made it into amd-staging-drm-next and as of https://lore.kernel.org/amd-gfx/20230125220153.320248-1-alexander.deuc...@amd.com/ they should land in drm-next soon if they haven't already. thanks, greg k-h -- Hamza
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
On 27.01.23 08:39, Greg KH wrote: > On Fri, Jan 20, 2023 at 11:51:04AM -0600, Limonciello, Mario wrote: >> On 1/20/2023 11:46, Guenter Roeck wrote: >>> On Thu, Jan 12, 2023 at 04:50:44PM +0800, Wayne Lin wrote: This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f. [Why] Changes cause regression on amdgpu mst. E.g. In fill_dc_mst_payload_table_from_drm(), amdgpu expects to add/remove payload one by one and call fill_dc_mst_payload_table_from_drm() to update the HW maintained payload table. But previous change tries to go through all the payloads in mst_state and update amdpug hw maintained table in once everytime driver only tries to add/remove a specific payload stream only. The newly design idea conflicts with the implementation in amdgpu nowadays. [How] Revert this patch first. After addressing all regression problems caused by this previous patch, will add it back and adjust it. >>> >>> Has there been any progress on this revert, or on fixing the underlying >>> problem ? >>> >>> Thanks, >>> Guenter >> >> Hi Guenter, >> >> Wayne is OOO for CNY, but let me update you. >> >> Harry has sent out this series which is a collection of proper fixes. >> https://patchwork.freedesktop.org/series/113125/ >> >> Once that's reviewed and accepted, 4 of them are applicable for 6.1. > > Any hint on when those will be reviewed and accepted? patchwork doesn't > show any activity on them, or at least I can't figure it out... I didn't look closer (hence please correct me if I'm wrong), but the core changes afaics are in the DRM pull airlied send a few hours ago to Linus (note the "amdgpu […] DP MST fixes" line): https://lore.kernel.org/all/capm%3d9tzuu4xnx6t5v7sksk%2ba5heapoc1iemyznsyqzgztj%3d...@mail.gmail.com/ Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page.
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
On Fri, Jan 20, 2023 at 11:51:04AM -0600, Limonciello, Mario wrote: > On 1/20/2023 11:46, Guenter Roeck wrote: > > On Thu, Jan 12, 2023 at 04:50:44PM +0800, Wayne Lin wrote: > > > This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f. > > > > > > [Why] > > > Changes cause regression on amdgpu mst. > > > E.g. > > > In fill_dc_mst_payload_table_from_drm(), amdgpu expects to add/remove > > > payload > > > one by one and call fill_dc_mst_payload_table_from_drm() to update the HW > > > maintained payload table. But previous change tries to go through all the > > > payloads in mst_state and update amdpug hw maintained table in once > > > everytime > > > driver only tries to add/remove a specific payload stream only. The newly > > > design idea conflicts with the implementation in amdgpu nowadays. > > > > > > [How] > > > Revert this patch first. After addressing all regression problems caused > > > by > > > this previous patch, will add it back and adjust it. > > > > Has there been any progress on this revert, or on fixing the underlying > > problem ? > > > > Thanks, > > Guenter > > Hi Guenter, > > Wayne is OOO for CNY, but let me update you. > > Harry has sent out this series which is a collection of proper fixes. > https://patchwork.freedesktop.org/series/113125/ > > Once that's reviewed and accepted, 4 of them are applicable for 6.1. Any hint on when those will be reviewed and accepted? patchwork doesn't show any activity on them, or at least I can't figure it out... thanks, greg k-h
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
On 1/20/23 10:39, Limonciello, Mario wrote: [ ... ] Wayne is OOO for CNY, but let me update you. Harry has sent out this series which is a collection of proper fixes. https://patchwork.freedesktop.org/series/113125/ Once that's reviewed and accepted, 4 of them are applicable for 6.1. Thanks a lot for the update. There is talk about abandoning v6.1.y as LTS candidate, in large part due to this problem, so it would be great to get the problem fixed before that happens. Any idea how soon that decision is happening? It seems that we have line of sight to a solution including back to 6.1.y pending that review. So perhaps we can put off the decision until those are landed. I honestly don't know. All I know is that Greg is concerned about the number of regressions in v6.1.y, and this problem was one he specifically mentioned to me as potential reason to not designate 6.1.y as LTS kernel. The extensive discussion at [1] may be an indication that there is a problem, though that mostly refers to [lack of] test coverage and does not point to specific regressions. Guenter --- [1] https://lore.kernel.org/lkml/CAPDLWs-Z8pYkwQ13dEgHXqSCjiq4xVnjuAXTy26H3=8nzcp...@mail.gmail.com/
RE: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
[Public] > -Original Message- > From: Guenter Roeck On Behalf Of Guenter Roeck > Sent: Friday, January 20, 2023 12:18 > To: Limonciello, Mario > Cc: Lin, Wayne ; dri-de...@lists.freedesktop.org; > amd-gfx@lists.freedesktop.org; sta...@vger.kernel.org; > stanislav.lisovs...@intel.com; Zuo, Jerry ; > bske...@redhat.com > Subject: Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into > the atomic state" > > Hi Mario, > > On Fri, Jan 20, 2023 at 11:51:04AM -0600, Limonciello, Mario wrote: > > On 1/20/2023 11:46, Guenter Roeck wrote: > > > On Thu, Jan 12, 2023 at 04:50:44PM +0800, Wayne Lin wrote: > > > > This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f. > > > > > > > > [Why] > > > > Changes cause regression on amdgpu mst. > > > > E.g. > > > > In fill_dc_mst_payload_table_from_drm(), amdgpu expects to > add/remove payload > > > > one by one and call fill_dc_mst_payload_table_from_drm() to update > the HW > > > > maintained payload table. But previous change tries to go through all > the > > > > payloads in mst_state and update amdpug hw maintained table in once > everytime > > > > driver only tries to add/remove a specific payload stream only. The > newly > > > > design idea conflicts with the implementation in amdgpu nowadays. > > > > > > > > [How] > > > > Revert this patch first. After addressing all regression problems caused > by > > > > this previous patch, will add it back and adjust it. > > > > > > Has there been any progress on this revert, or on fixing the underlying > > > problem ? > > > > > > Thanks, > > > Guenter > > > > Hi Guenter, > > > > Wayne is OOO for CNY, but let me update you. > > > > Harry has sent out this series which is a collection of proper fixes. > > https://patchwork.freedesktop.org/series/113125/ > > > > Once that's reviewed and accepted, 4 of them are applicable for 6.1. > > Thanks a lot for the update. There is talk about abandoning v6.1.y as > LTS candidate, in large part due to this problem, so it would be great > to get the problem fixed before that happens. Any idea how soon that decision is happening? It seems that we have line of sight to a solution including back to 6.1.y pending that review. So perhaps we can put off the decision until those are landed.
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
Hi Mario, On Fri, Jan 20, 2023 at 11:51:04AM -0600, Limonciello, Mario wrote: > On 1/20/2023 11:46, Guenter Roeck wrote: > > On Thu, Jan 12, 2023 at 04:50:44PM +0800, Wayne Lin wrote: > > > This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f. > > > > > > [Why] > > > Changes cause regression on amdgpu mst. > > > E.g. > > > In fill_dc_mst_payload_table_from_drm(), amdgpu expects to add/remove > > > payload > > > one by one and call fill_dc_mst_payload_table_from_drm() to update the HW > > > maintained payload table. But previous change tries to go through all the > > > payloads in mst_state and update amdpug hw maintained table in once > > > everytime > > > driver only tries to add/remove a specific payload stream only. The newly > > > design idea conflicts with the implementation in amdgpu nowadays. > > > > > > [How] > > > Revert this patch first. After addressing all regression problems caused > > > by > > > this previous patch, will add it back and adjust it. > > > > Has there been any progress on this revert, or on fixing the underlying > > problem ? > > > > Thanks, > > Guenter > > Hi Guenter, > > Wayne is OOO for CNY, but let me update you. > > Harry has sent out this series which is a collection of proper fixes. > https://patchwork.freedesktop.org/series/113125/ > > Once that's reviewed and accepted, 4 of them are applicable for 6.1. Thanks a lot for the update. There is talk about abandoning v6.1.y as LTS candidate, in large part due to this problem, so it would be great to get the problem fixed before that happens. Thanks, Guenter
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
On Thu, Jan 12, 2023 at 04:50:44PM +0800, Wayne Lin wrote: > This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f. > > [Why] > Changes cause regression on amdgpu mst. > E.g. > In fill_dc_mst_payload_table_from_drm(), amdgpu expects to add/remove payload > one by one and call fill_dc_mst_payload_table_from_drm() to update the HW > maintained payload table. But previous change tries to go through all the > payloads in mst_state and update amdpug hw maintained table in once everytime > driver only tries to add/remove a specific payload stream only. The newly > design idea conflicts with the implementation in amdgpu nowadays. > > [How] > Revert this patch first. After addressing all regression problems caused by > this previous patch, will add it back and adjust it. Has there been any progress on this revert, or on fixing the underlying problem ? Thanks, Guenter
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
On 1/20/2023 11:46, Guenter Roeck wrote: On Thu, Jan 12, 2023 at 04:50:44PM +0800, Wayne Lin wrote: This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f. [Why] Changes cause regression on amdgpu mst. E.g. In fill_dc_mst_payload_table_from_drm(), amdgpu expects to add/remove payload one by one and call fill_dc_mst_payload_table_from_drm() to update the HW maintained payload table. But previous change tries to go through all the payloads in mst_state and update amdpug hw maintained table in once everytime driver only tries to add/remove a specific payload stream only. The newly design idea conflicts with the implementation in amdgpu nowadays. [How] Revert this patch first. After addressing all regression problems caused by this previous patch, will add it back and adjust it. Has there been any progress on this revert, or on fixing the underlying problem ? Thanks, Guenter Hi Guenter, Wayne is OOO for CNY, but let me update you. Harry has sent out this series which is a collection of proper fixes. https://patchwork.freedesktop.org/series/113125/ Once that's reviewed and accepted, 4 of them are applicable for 6.1. Thanks,
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
On 1/13/2023 13:28, Lyude Paul wrote:
On Fri, 2023-01-13 at 11:25 +0100, Daniel Vetter wrote:
On Fri, Jan 13, 2023 at 12:16:57PM +0200, Jani Nikula wrote:
Cc: intel-gfx, drm maintainers
Please have the courtesy of Cc'ing us for changes impacting us, and
maybe try to involve us earlier instead of surprising us like
this. Looks like this has been debugged for at least three months, and
the huge revert at this point is going to set us back with what's been
developed on top of it for i915 DP MST and DSC.
tbf I assumed this wont land when I've seen it fly by. It feels a bit much
like living under a rock for half a year and then creating a mess for
everyone else who's been building on top of this is not great.
Like yes it's a regression, but apparently not a blantantly obvious one,
and I think if we just ram this in there's considerable community goodwill
down the drain. Someone needs to get that goodwill up the drain again.
It's a regression, I get that, but this is also going to be really nasty
to deal with. It's a 2500-line commit, plus the dependencies, which I
don't think are accounted for here. (What's the baseline for the revert
anyway?) I don't expect all the dependent commits to be easy to revert
or backport to v6.1 or v6.2.
*sad trombone*
Yeah that's the other thing. 2500 patch revert is not cc stable material.
So this isn't even really helping users all that much.
Unless it also comes with full amounts of backports of the reverts on all
affected drivers for all curent stable trees, fully validated.
The silver lining here is that in terms of how many stable trees this is
broken it's only 6.1.y.
Wayne's revert is against drm-tip.
I found that attempting backporting his revert I run into
conflicts in 6.2-rc3 because of:
831a277ef001 ("Revert "drm/i915: Extract drm_dp_atomic_find_vcpi_slots
cycle to separate function"")
I worked through them and have the result here:
https://gitlab.freedesktop.org/superm1/linux/-/commit/8e926eb77c41e7f32f3d8943cdf7d140ed319b79
and conflicts in 6.1.y from the lack of:
8c7d980da9ba ("drm/nouveau/disp: move DP MST payload config method")
I worked through those as well and the result is here:
https://gitlab.freedesktop.org/superm1/linux/-/commit/2145b4de3fea9908cda6bef0693a797cc7f4ddfc
Affected people in Gitlab #2171 have tested amdgpu works w/ MST again
with 6.1.5 with that applied.
To your point, we do need someone with the appropriate hardware to make
sure we didn't make i915 or nouveau worse by this revert though.
This is bad. I do think we need to have some understanding first of what
"fix this in amdgpu" would look like as plan B. Because plan A does not
look like a happy one at all.
Yeah this whole thing has been a mess, I'm partially to blame here - we should
have reverted earlier, but a lot of this has been me finding out that the
problem here is a lot bigger then I previously imagined - and has not at all
been easy to untangle. I've also dumped so much time into trying to figure it
out that was more or less the only reason I acked this in the first place, I'm
literally just quite tired and exhausted at this point from spinning my wheels
on trying to fix this ;_;.
I am sure there is a real proper fix for this, if anyone wants to help me try
and figure this out I'm happy to setup remote access to the machines I've got
here. I'll also try to push myself to dig further into this next week again.
-Daniel
BR,
Jani.
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
On Fri, 2023-01-13 at 11:25 +0100, Daniel Vetter wrote: > On Fri, Jan 13, 2023 at 12:16:57PM +0200, Jani Nikula wrote: > > > > Cc: intel-gfx, drm maintainers > > > > Please have the courtesy of Cc'ing us for changes impacting us, and > > maybe try to involve us earlier instead of surprising us like > > this. Looks like this has been debugged for at least three months, and > > the huge revert at this point is going to set us back with what's been > > developed on top of it for i915 DP MST and DSC. > > tbf I assumed this wont land when I've seen it fly by. It feels a bit much > like living under a rock for half a year and then creating a mess for > everyone else who's been building on top of this is not great. > > Like yes it's a regression, but apparently not a blantantly obvious one, > and I think if we just ram this in there's considerable community goodwill > down the drain. Someone needs to get that goodwill up the drain again. > > > It's a regression, I get that, but this is also going to be really nasty > > to deal with. It's a 2500-line commit, plus the dependencies, which I > > don't think are accounted for here. (What's the baseline for the revert > > anyway?) I don't expect all the dependent commits to be easy to revert > > or backport to v6.1 or v6.2. > > > > *sad trombone* > > Yeah that's the other thing. 2500 patch revert is not cc stable material. > So this isn't even really helping users all that much. > > Unless it also comes with full amounts of backports of the reverts on all > affected drivers for all curent stable trees, fully validated. > > This is bad. I do think we need to have some understanding first of what > "fix this in amdgpu" would look like as plan B. Because plan A does not > look like a happy one at all. Yeah this whole thing has been a mess, I'm partially to blame here - we should have reverted earlier, but a lot of this has been me finding out that the problem here is a lot bigger then I previously imagined - and has not at all been easy to untangle. I've also dumped so much time into trying to figure it out that was more or less the only reason I acked this in the first place, I'm literally just quite tired and exhausted at this point from spinning my wheels on trying to fix this ;_;. I am sure there is a real proper fix for this, if anyone wants to help me try and figure this out I'm happy to setup remote access to the machines I've got here. I'll also try to push myself to dig further into this next week again. > -Daniel > > > BR, > > Jani. > > > > > > On Thu, 12 Jan 2023, Wayne Lin wrote: > > > This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f. > > > > > > [Why] > > > Changes cause regression on amdgpu mst. > > > E.g. > > > In fill_dc_mst_payload_table_from_drm(), amdgpu expects to add/remove > > > payload > > > one by one and call fill_dc_mst_payload_table_from_drm() to update the HW > > > maintained payload table. But previous change tries to go through all the > > > payloads in mst_state and update amdpug hw maintained table in once > > > everytime > > > driver only tries to add/remove a specific payload stream only. The newly > > > design idea conflicts with the implementation in amdgpu nowadays. > > > > > > [How] > > > Revert this patch first. After addressing all regression problems caused > > > by > > > this previous patch, will add it back and adjust it. > > > > > > Signed-off-by: Wayne Lin > > > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2171 > > > Cc: sta...@vger.kernel.org # 6.1 > > > Cc: Lyude Paul > > > Cc: Harry Wentland > > > Cc: Mario Limonciello > > > Cc: Ville Syrjälä > > > Cc: Ben Skeggs > > > Cc: Stanislav Lisovskiy > > > Cc: Fangzhi Zuo > > > --- > > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 53 +- > > > .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 106 ++- > > > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 87 ++- > > > .../amd/display/include/link_service_types.h | 3 - > > > drivers/gpu/drm/display/drm_dp_mst_topology.c | 724 -- > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 67 +- > > > drivers/gpu/drm/i915/display/intel_hdcp.c | 24 +- > > > drivers/gpu/drm/nouveau/dispnv50/disp.c | 167 ++-- > > > include/drm/display/drm_dp_mst_helper.h | 177 +++-- > > > 9 files changed, 878 insertions(+), 530 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > index 77277d90b6e2..674f5dc1102b 100644 > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > @@ -6548,7 +6548,6 @@ static int dm_encoder_helper_atomic_check(struct > > > drm_encoder *encoder, > > > const struct drm_display_mode *adjusted_mode = > > > _state->adjusted_mode; > > > struct drm_dp_mst_topology_mgr *mst_mgr; > > > struct drm_dp_mst_port *mst_port; > > > - struct drm_dp_mst_topology_state
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
On 1/13/23 05:25, Daniel Vetter wrote: > On Fri, Jan 13, 2023 at 12:16:57PM +0200, Jani Nikula wrote: >> >> Cc: intel-gfx, drm maintainers >> >> Please have the courtesy of Cc'ing us for changes impacting us, and >> maybe try to involve us earlier instead of surprising us like >> this. Looks like this has been debugged for at least three months, and >> the huge revert at this point is going to set us back with what's been >> developed on top of it for i915 DP MST and DSC. > > tbf I assumed this wont land when I've seen it fly by. It feels a bit much > like living under a rock for half a year and then creating a mess for > everyone else who's been building on top of this is not great. > > Like yes it's a regression, but apparently not a blantantly obvious one, > and I think if we just ram this in there's considerable community goodwill > down the drain. Someone needs to get that goodwill up the drain again. > >> It's a regression, I get that, but this is also going to be really nasty >> to deal with. It's a 2500-line commit, plus the dependencies, which I >> don't think are accounted for here. (What's the baseline for the revert >> anyway?) I don't expect all the dependent commits to be easy to revert >> or backport to v6.1 or v6.2. >> >> *sad trombone* > > Yeah that's the other thing. 2500 patch revert is not cc stable material. > So this isn't even really helping users all that much. > > Unless it also comes with full amounts of backports of the reverts on all > affected drivers for all curent stable trees, fully validated. > > This is bad. I do think we need to have some understanding first of what > "fix this in amdgpu" would look like as plan B. Because plan A does not > look like a happy one at all. Agree with your sentiments and apologies for not making this visible to you sooner. This has been on the corner of my radar but I should've given it a higher priority and more visibility sooner. Has anyone looked at the dependencies? I assume there are a number of dependent commits that would be hard/impossible to untangle but haven't had a chance to look for those myself. Could you highlight a couple? Harry > -Daniel > >> BR, >> Jani. >> >> >> On Thu, 12 Jan 2023, Wayne Lin wrote: >>> This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f. >>> >>> [Why] >>> Changes cause regression on amdgpu mst. >>> E.g. >>> In fill_dc_mst_payload_table_from_drm(), amdgpu expects to add/remove >>> payload >>> one by one and call fill_dc_mst_payload_table_from_drm() to update the HW >>> maintained payload table. But previous change tries to go through all the >>> payloads in mst_state and update amdpug hw maintained table in once >>> everytime >>> driver only tries to add/remove a specific payload stream only. The newly >>> design idea conflicts with the implementation in amdgpu nowadays. >>> >>> [How] >>> Revert this patch first. After addressing all regression problems caused by >>> this previous patch, will add it back and adjust it. >>> >>> Signed-off-by: Wayne Lin >>> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2171 >>> Cc: sta...@vger.kernel.org # 6.1 >>> Cc: Lyude Paul >>> Cc: Harry Wentland >>> Cc: Mario Limonciello >>> Cc: Ville Syrjälä >>> Cc: Ben Skeggs >>> Cc: Stanislav Lisovskiy >>> Cc: Fangzhi Zuo >>> --- >>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 53 +- >>> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 106 ++- >>> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 87 ++- >>> .../amd/display/include/link_service_types.h | 3 - >>> drivers/gpu/drm/display/drm_dp_mst_topology.c | 724 -- >>> drivers/gpu/drm/i915/display/intel_dp_mst.c | 67 +- >>> drivers/gpu/drm/i915/display/intel_hdcp.c | 24 +- >>> drivers/gpu/drm/nouveau/dispnv50/disp.c | 167 ++-- >>> include/drm/display/drm_dp_mst_helper.h | 177 +++-- >>> 9 files changed, 878 insertions(+), 530 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> index 77277d90b6e2..674f5dc1102b 100644 >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> @@ -6548,7 +6548,6 @@ static int dm_encoder_helper_atomic_check(struct >>> drm_encoder *encoder, >>> const struct drm_display_mode *adjusted_mode = >>> _state->adjusted_mode; >>> struct drm_dp_mst_topology_mgr *mst_mgr; >>> struct drm_dp_mst_port *mst_port; >>> - struct drm_dp_mst_topology_state *mst_state; >>> enum dc_color_depth color_depth; >>> int clock, bpp = 0; >>> bool is_y420 = false; >>> @@ -6562,13 +6561,6 @@ static int dm_encoder_helper_atomic_check(struct >>> drm_encoder *encoder, >>> if (!crtc_state->connectors_changed && !crtc_state->mode_changed) >>> return 0; >>> >>> - mst_state = drm_atomic_get_mst_topology_state(state, mst_mgr); >>> - if (IS_ERR(mst_state)) >>> - return PTR_ERR(mst_state);
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
On Fri, Jan 13, 2023 at 12:16:57PM +0200, Jani Nikula wrote:
>
> Cc: intel-gfx, drm maintainers
>
> Please have the courtesy of Cc'ing us for changes impacting us, and
> maybe try to involve us earlier instead of surprising us like
> this. Looks like this has been debugged for at least three months, and
> the huge revert at this point is going to set us back with what's been
> developed on top of it for i915 DP MST and DSC.
tbf I assumed this wont land when I've seen it fly by. It feels a bit much
like living under a rock for half a year and then creating a mess for
everyone else who's been building on top of this is not great.
Like yes it's a regression, but apparently not a blantantly obvious one,
and I think if we just ram this in there's considerable community goodwill
down the drain. Someone needs to get that goodwill up the drain again.
> It's a regression, I get that, but this is also going to be really nasty
> to deal with. It's a 2500-line commit, plus the dependencies, which I
> don't think are accounted for here. (What's the baseline for the revert
> anyway?) I don't expect all the dependent commits to be easy to revert
> or backport to v6.1 or v6.2.
>
> *sad trombone*
Yeah that's the other thing. 2500 patch revert is not cc stable material.
So this isn't even really helping users all that much.
Unless it also comes with full amounts of backports of the reverts on all
affected drivers for all curent stable trees, fully validated.
This is bad. I do think we need to have some understanding first of what
"fix this in amdgpu" would look like as plan B. Because plan A does not
look like a happy one at all.
-Daniel
> BR,
> Jani.
>
>
> On Thu, 12 Jan 2023, Wayne Lin wrote:
> > This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f.
> >
> > [Why]
> > Changes cause regression on amdgpu mst.
> > E.g.
> > In fill_dc_mst_payload_table_from_drm(), amdgpu expects to add/remove
> > payload
> > one by one and call fill_dc_mst_payload_table_from_drm() to update the HW
> > maintained payload table. But previous change tries to go through all the
> > payloads in mst_state and update amdpug hw maintained table in once
> > everytime
> > driver only tries to add/remove a specific payload stream only. The newly
> > design idea conflicts with the implementation in amdgpu nowadays.
> >
> > [How]
> > Revert this patch first. After addressing all regression problems caused by
> > this previous patch, will add it back and adjust it.
> >
> > Signed-off-by: Wayne Lin
> > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2171
> > Cc: sta...@vger.kernel.org # 6.1
> > Cc: Lyude Paul
> > Cc: Harry Wentland
> > Cc: Mario Limonciello
> > Cc: Ville Syrjälä
> > Cc: Ben Skeggs
> > Cc: Stanislav Lisovskiy
> > Cc: Fangzhi Zuo
> > ---
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 53 +-
> > .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 106 ++-
> > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 87 ++-
> > .../amd/display/include/link_service_types.h | 3 -
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 724 --
> > drivers/gpu/drm/i915/display/intel_dp_mst.c | 67 +-
> > drivers/gpu/drm/i915/display/intel_hdcp.c | 24 +-
> > drivers/gpu/drm/nouveau/dispnv50/disp.c | 167 ++--
> > include/drm/display/drm_dp_mst_helper.h | 177 +++--
> > 9 files changed, 878 insertions(+), 530 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 77277d90b6e2..674f5dc1102b 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -6548,7 +6548,6 @@ static int dm_encoder_helper_atomic_check(struct
> > drm_encoder *encoder,
> > const struct drm_display_mode *adjusted_mode =
> > _state->adjusted_mode;
> > struct drm_dp_mst_topology_mgr *mst_mgr;
> > struct drm_dp_mst_port *mst_port;
> > - struct drm_dp_mst_topology_state *mst_state;
> > enum dc_color_depth color_depth;
> > int clock, bpp = 0;
> > bool is_y420 = false;
> > @@ -6562,13 +6561,6 @@ static int dm_encoder_helper_atomic_check(struct
> > drm_encoder *encoder,
> > if (!crtc_state->connectors_changed && !crtc_state->mode_changed)
> > return 0;
> >
> > - mst_state = drm_atomic_get_mst_topology_state(state, mst_mgr);
> > - if (IS_ERR(mst_state))
> > - return PTR_ERR(mst_state);
> > -
> > - if (!mst_state->pbn_div)
> > - mst_state->pbn_div =
> > dm_mst_get_pbn_divider(aconnector->mst_port->dc_link);
> > -
> > if (!state->duplicated) {
> > int max_bpc = conn_state->max_requested_bpc;
> > is_y420 = drm_mode_is_420_also(>display_info,
> > adjusted_mode) &&
> > @@ -6580,10 +6572,11 @@ static int dm_encoder_helper_atomic_check(struct
> > drm_encoder *encoder,
> > clock = adjusted_mode->clock;
> > dm_new_connector_state->pbn =
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
Cc: intel-gfx, drm maintainers
Please have the courtesy of Cc'ing us for changes impacting us, and
maybe try to involve us earlier instead of surprising us like
this. Looks like this has been debugged for at least three months, and
the huge revert at this point is going to set us back with what's been
developed on top of it for i915 DP MST and DSC.
It's a regression, I get that, but this is also going to be really nasty
to deal with. It's a 2500-line commit, plus the dependencies, which I
don't think are accounted for here. (What's the baseline for the revert
anyway?) I don't expect all the dependent commits to be easy to revert
or backport to v6.1 or v6.2.
*sad trombone*
BR,
Jani.
On Thu, 12 Jan 2023, Wayne Lin wrote:
> This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f.
>
> [Why]
> Changes cause regression on amdgpu mst.
> E.g.
> In fill_dc_mst_payload_table_from_drm(), amdgpu expects to add/remove payload
> one by one and call fill_dc_mst_payload_table_from_drm() to update the HW
> maintained payload table. But previous change tries to go through all the
> payloads in mst_state and update amdpug hw maintained table in once everytime
> driver only tries to add/remove a specific payload stream only. The newly
> design idea conflicts with the implementation in amdgpu nowadays.
>
> [How]
> Revert this patch first. After addressing all regression problems caused by
> this previous patch, will add it back and adjust it.
>
> Signed-off-by: Wayne Lin
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2171
> Cc: sta...@vger.kernel.org # 6.1
> Cc: Lyude Paul
> Cc: Harry Wentland
> Cc: Mario Limonciello
> Cc: Ville Syrjälä
> Cc: Ben Skeggs
> Cc: Stanislav Lisovskiy
> Cc: Fangzhi Zuo
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 53 +-
> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 106 ++-
> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 87 ++-
> .../amd/display/include/link_service_types.h | 3 -
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 724 --
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 67 +-
> drivers/gpu/drm/i915/display/intel_hdcp.c | 24 +-
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 167 ++--
> include/drm/display/drm_dp_mst_helper.h | 177 +++--
> 9 files changed, 878 insertions(+), 530 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 77277d90b6e2..674f5dc1102b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6548,7 +6548,6 @@ static int dm_encoder_helper_atomic_check(struct
> drm_encoder *encoder,
> const struct drm_display_mode *adjusted_mode =
> _state->adjusted_mode;
> struct drm_dp_mst_topology_mgr *mst_mgr;
> struct drm_dp_mst_port *mst_port;
> - struct drm_dp_mst_topology_state *mst_state;
> enum dc_color_depth color_depth;
> int clock, bpp = 0;
> bool is_y420 = false;
> @@ -6562,13 +6561,6 @@ static int dm_encoder_helper_atomic_check(struct
> drm_encoder *encoder,
> if (!crtc_state->connectors_changed && !crtc_state->mode_changed)
> return 0;
>
> - mst_state = drm_atomic_get_mst_topology_state(state, mst_mgr);
> - if (IS_ERR(mst_state))
> - return PTR_ERR(mst_state);
> -
> - if (!mst_state->pbn_div)
> - mst_state->pbn_div =
> dm_mst_get_pbn_divider(aconnector->mst_port->dc_link);
> -
> if (!state->duplicated) {
> int max_bpc = conn_state->max_requested_bpc;
> is_y420 = drm_mode_is_420_also(>display_info,
> adjusted_mode) &&
> @@ -6580,10 +6572,11 @@ static int dm_encoder_helper_atomic_check(struct
> drm_encoder *encoder,
> clock = adjusted_mode->clock;
> dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp,
> false);
> }
> -
> - dm_new_connector_state->vcpi_slots =
> - drm_dp_atomic_find_time_slots(state, mst_mgr, mst_port,
> - dm_new_connector_state->pbn);
> + dm_new_connector_state->vcpi_slots =
> drm_dp_atomic_find_time_slots(state,
> +
> mst_mgr,
> +
> mst_port,
> +
> dm_new_connector_state->pbn,
> +
> dm_mst_get_pbn_divider(aconnector->dc_link));
> if (dm_new_connector_state->vcpi_slots < 0) {
> DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n",
> (int)dm_new_connector_state->vcpi_slots);
> return dm_new_connector_state->vcpi_slots;
> @@ -6654,14 +6647,17 @@ static int dm_update_mst_vcpi_slots_for_dsc(struct
> drm_atomic_state *state,
>
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
Acked-by: Lyude Paul
On Thu, 2023-01-12 at 16:50 +0800, Wayne Lin wrote:
> This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f.
>
> [Why]
> Changes cause regression on amdgpu mst.
> E.g.
> In fill_dc_mst_payload_table_from_drm(), amdgpu expects to add/remove payload
> one by one and call fill_dc_mst_payload_table_from_drm() to update the HW
> maintained payload table. But previous change tries to go through all the
> payloads in mst_state and update amdpug hw maintained table in once everytime
> driver only tries to add/remove a specific payload stream only. The newly
> design idea conflicts with the implementation in amdgpu nowadays.
>
> [How]
> Revert this patch first. After addressing all regression problems caused by
> this previous patch, will add it back and adjust it.
>
> Signed-off-by: Wayne Lin
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2171
> Cc: sta...@vger.kernel.org # 6.1
> Cc: Lyude Paul
> Cc: Harry Wentland
> Cc: Mario Limonciello
> Cc: Ville Syrjälä
> Cc: Ben Skeggs
> Cc: Stanislav Lisovskiy
> Cc: Fangzhi Zuo
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 53 +-
> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 106 ++-
> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 87 ++-
> .../amd/display/include/link_service_types.h | 3 -
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 724 --
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 67 +-
> drivers/gpu/drm/i915/display/intel_hdcp.c | 24 +-
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 167 ++--
> include/drm/display/drm_dp_mst_helper.h | 177 +++--
> 9 files changed, 878 insertions(+), 530 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 77277d90b6e2..674f5dc1102b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6548,7 +6548,6 @@ static int dm_encoder_helper_atomic_check(struct
> drm_encoder *encoder,
> const struct drm_display_mode *adjusted_mode =
> _state->adjusted_mode;
> struct drm_dp_mst_topology_mgr *mst_mgr;
> struct drm_dp_mst_port *mst_port;
> - struct drm_dp_mst_topology_state *mst_state;
> enum dc_color_depth color_depth;
> int clock, bpp = 0;
> bool is_y420 = false;
> @@ -6562,13 +6561,6 @@ static int dm_encoder_helper_atomic_check(struct
> drm_encoder *encoder,
> if (!crtc_state->connectors_changed && !crtc_state->mode_changed)
> return 0;
>
> - mst_state = drm_atomic_get_mst_topology_state(state, mst_mgr);
> - if (IS_ERR(mst_state))
> - return PTR_ERR(mst_state);
> -
> - if (!mst_state->pbn_div)
> - mst_state->pbn_div =
> dm_mst_get_pbn_divider(aconnector->mst_port->dc_link);
> -
> if (!state->duplicated) {
> int max_bpc = conn_state->max_requested_bpc;
> is_y420 = drm_mode_is_420_also(>display_info,
> adjusted_mode) &&
> @@ -6580,10 +6572,11 @@ static int dm_encoder_helper_atomic_check(struct
> drm_encoder *encoder,
> clock = adjusted_mode->clock;
> dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp,
> false);
> }
> -
> - dm_new_connector_state->vcpi_slots =
> - drm_dp_atomic_find_time_slots(state, mst_mgr, mst_port,
> - dm_new_connector_state->pbn);
> + dm_new_connector_state->vcpi_slots =
> drm_dp_atomic_find_time_slots(state,
> +
> mst_mgr,
> +
> mst_port,
> +
> dm_new_connector_state->pbn,
> +
> dm_mst_get_pbn_divider(aconnector->dc_link));
> if (dm_new_connector_state->vcpi_slots < 0) {
> DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n",
> (int)dm_new_connector_state->vcpi_slots);
> return dm_new_connector_state->vcpi_slots;
> @@ -6654,14 +6647,17 @@ static int dm_update_mst_vcpi_slots_for_dsc(struct
> drm_atomic_state *state,
> dm_conn_state->vcpi_slots = slot_num;
>
> ret = drm_dp_mst_atomic_enable_dsc(state,
> aconnector->port,
> -dm_conn_state->pbn,
> false);
> +dm_conn_state->pbn,
> 0, false);
> if (ret < 0)
> return ret;
>
> continue;
> }
>
> - vcpi = drm_dp_mst_atomic_enable_dsc(state, aconnector->port,
> pbn, true);
> + vcpi = drm_dp_mst_atomic_enable_dsc(state,
> +
Re: [PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
On 1/12/2023 02:50, Wayne Lin wrote:
This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f.
[Why]
Changes cause regression on amdgpu mst.
E.g.
In fill_dc_mst_payload_table_from_drm(), amdgpu expects to add/remove payload
one by one and call fill_dc_mst_payload_table_from_drm() to update the HW
maintained payload table. But previous change tries to go through all the
payloads in mst_state and update amdpug hw maintained table in once everytime
driver only tries to add/remove a specific payload stream only. The newly
design idea conflicts with the implementation in amdgpu nowadays.
[How]
Revert this patch first. After addressing all regression problems caused by
this previous patch, will add it back and adjust it.
Signed-off-by: Wayne Lin
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2171
Cc: sta...@vger.kernel.org # 6.1
Cc: Lyude Paul
Cc: Harry Wentland
Cc: Mario Limonciello
Cc: Ville Syrjälä
Cc: Ben Skeggs
Cc: Stanislav Lisovskiy
Cc: Fangzhi Zuo
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 53 +-
.../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 106 ++-
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 87 ++-
.../amd/display/include/link_service_types.h | 3 -
drivers/gpu/drm/display/drm_dp_mst_topology.c | 724 --
drivers/gpu/drm/i915/display/intel_dp_mst.c | 67 +-
drivers/gpu/drm/i915/display/intel_hdcp.c | 24 +-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 167 ++--
include/drm/display/drm_dp_mst_helper.h | 177 +++--
9 files changed, 878 insertions(+), 530 deletions(-)
Hi Wayne,
What branch is this intended to apply against? I shared that it existed
to reporters in #2171 and they said they couldn't apply it against
drm-next (03a0a1040), v6.2-rc3 or v6.1.5.
I guess it's unclear to me the correct path this is supposed to start.
Should we be reverting in drm-fixes, drm-next, or directly to 6.2-rc?
Thanks,
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 77277d90b6e2..674f5dc1102b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6548,7 +6548,6 @@ static int dm_encoder_helper_atomic_check(struct
drm_encoder *encoder,
const struct drm_display_mode *adjusted_mode =
_state->adjusted_mode;
struct drm_dp_mst_topology_mgr *mst_mgr;
struct drm_dp_mst_port *mst_port;
- struct drm_dp_mst_topology_state *mst_state;
enum dc_color_depth color_depth;
int clock, bpp = 0;
bool is_y420 = false;
@@ -6562,13 +6561,6 @@ static int dm_encoder_helper_atomic_check(struct
drm_encoder *encoder,
if (!crtc_state->connectors_changed && !crtc_state->mode_changed)
return 0;
- mst_state = drm_atomic_get_mst_topology_state(state, mst_mgr);
- if (IS_ERR(mst_state))
- return PTR_ERR(mst_state);
-
- if (!mst_state->pbn_div)
- mst_state->pbn_div =
dm_mst_get_pbn_divider(aconnector->mst_port->dc_link);
-
if (!state->duplicated) {
int max_bpc = conn_state->max_requested_bpc;
is_y420 = drm_mode_is_420_also(>display_info, adjusted_mode)
&&
@@ -6580,10 +6572,11 @@ static int dm_encoder_helper_atomic_check(struct
drm_encoder *encoder,
clock = adjusted_mode->clock;
dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp,
false);
}
-
- dm_new_connector_state->vcpi_slots =
- drm_dp_atomic_find_time_slots(state, mst_mgr, mst_port,
- dm_new_connector_state->pbn);
+ dm_new_connector_state->vcpi_slots =
drm_dp_atomic_find_time_slots(state,
+
mst_mgr,
+
mst_port,
+
dm_new_connector_state->pbn,
+
dm_mst_get_pbn_divider(aconnector->dc_link));
if (dm_new_connector_state->vcpi_slots < 0) {
DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n",
(int)dm_new_connector_state->vcpi_slots);
return dm_new_connector_state->vcpi_slots;
@@ -6654,14 +6647,17 @@ static int dm_update_mst_vcpi_slots_for_dsc(struct
drm_atomic_state *state,
dm_conn_state->vcpi_slots = slot_num;
ret = drm_dp_mst_atomic_enable_dsc(state, aconnector->port,
- dm_conn_state->pbn,
false);
+ dm_conn_state->pbn,
0, false);
if (ret < 0)
return ret;
continue;
}
- vcpi = drm_dp_mst_atomic_enable_dsc(state, aconnector->port,
[PATCH] Revert "drm/display/dp_mst: Move all payload info into the atomic state"
This reverts commit 4d07b0bc403403438d9cf88450506240c5faf92f.
[Why]
Changes cause regression on amdgpu mst.
E.g.
In fill_dc_mst_payload_table_from_drm(), amdgpu expects to add/remove payload
one by one and call fill_dc_mst_payload_table_from_drm() to update the HW
maintained payload table. But previous change tries to go through all the
payloads in mst_state and update amdpug hw maintained table in once everytime
driver only tries to add/remove a specific payload stream only. The newly
design idea conflicts with the implementation in amdgpu nowadays.
[How]
Revert this patch first. After addressing all regression problems caused by
this previous patch, will add it back and adjust it.
Signed-off-by: Wayne Lin
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2171
Cc: sta...@vger.kernel.org # 6.1
Cc: Lyude Paul
Cc: Harry Wentland
Cc: Mario Limonciello
Cc: Ville Syrjälä
Cc: Ben Skeggs
Cc: Stanislav Lisovskiy
Cc: Fangzhi Zuo
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 53 +-
.../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 106 ++-
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 87 ++-
.../amd/display/include/link_service_types.h | 3 -
drivers/gpu/drm/display/drm_dp_mst_topology.c | 724 --
drivers/gpu/drm/i915/display/intel_dp_mst.c | 67 +-
drivers/gpu/drm/i915/display/intel_hdcp.c | 24 +-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 167 ++--
include/drm/display/drm_dp_mst_helper.h | 177 +++--
9 files changed, 878 insertions(+), 530 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 77277d90b6e2..674f5dc1102b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6548,7 +6548,6 @@ static int dm_encoder_helper_atomic_check(struct
drm_encoder *encoder,
const struct drm_display_mode *adjusted_mode =
_state->adjusted_mode;
struct drm_dp_mst_topology_mgr *mst_mgr;
struct drm_dp_mst_port *mst_port;
- struct drm_dp_mst_topology_state *mst_state;
enum dc_color_depth color_depth;
int clock, bpp = 0;
bool is_y420 = false;
@@ -6562,13 +6561,6 @@ static int dm_encoder_helper_atomic_check(struct
drm_encoder *encoder,
if (!crtc_state->connectors_changed && !crtc_state->mode_changed)
return 0;
- mst_state = drm_atomic_get_mst_topology_state(state, mst_mgr);
- if (IS_ERR(mst_state))
- return PTR_ERR(mst_state);
-
- if (!mst_state->pbn_div)
- mst_state->pbn_div =
dm_mst_get_pbn_divider(aconnector->mst_port->dc_link);
-
if (!state->duplicated) {
int max_bpc = conn_state->max_requested_bpc;
is_y420 = drm_mode_is_420_also(>display_info,
adjusted_mode) &&
@@ -6580,10 +6572,11 @@ static int dm_encoder_helper_atomic_check(struct
drm_encoder *encoder,
clock = adjusted_mode->clock;
dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp,
false);
}
-
- dm_new_connector_state->vcpi_slots =
- drm_dp_atomic_find_time_slots(state, mst_mgr, mst_port,
- dm_new_connector_state->pbn);
+ dm_new_connector_state->vcpi_slots =
drm_dp_atomic_find_time_slots(state,
+
mst_mgr,
+
mst_port,
+
dm_new_connector_state->pbn,
+
dm_mst_get_pbn_divider(aconnector->dc_link));
if (dm_new_connector_state->vcpi_slots < 0) {
DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n",
(int)dm_new_connector_state->vcpi_slots);
return dm_new_connector_state->vcpi_slots;
@@ -6654,14 +6647,17 @@ static int dm_update_mst_vcpi_slots_for_dsc(struct
drm_atomic_state *state,
dm_conn_state->vcpi_slots = slot_num;
ret = drm_dp_mst_atomic_enable_dsc(state,
aconnector->port,
- dm_conn_state->pbn,
false);
+ dm_conn_state->pbn,
0, false);
if (ret < 0)
return ret;
continue;
}
- vcpi = drm_dp_mst_atomic_enable_dsc(state, aconnector->port,
pbn, true);
+ vcpi = drm_dp_mst_atomic_enable_dsc(state,
+ aconnector->port,
+ pbn, pbn_div,
+ true);
if (vcpi < 0)
return vcpi;
@@ -9497,6
