| Message ID | 20240925154324.348774-2-mwen@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: increase MAX_SURFACES in line with planes and streams | expand |
[AMD Official Use Only - AMD Internal Distribution Only] Hi Melissa, MAX_SURFACE_NUM and MAX_PLANES both represent the upper bound of planes that are supported by HW. It is best to replace MAX_SURFACE_NUM with MAX_PLANES to remove redundancy. MAX_SURFACES is used to represent the upper bound of surfaces that can be piped to a single CRTC. Keep MAX_SURFACES as is to keep stack size down, and replace MAX_SURFACE_NUM with MAX_PLANES. Thanks, Zaeem -----Original Message----- From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Melissa Wen Sent: Wednesday, September 25, 2024 11:37 AM To: harry.wentland@amd.com; sunpeng.li@amd.com; Rodrigo.Siqueira@amd.com; alexander.deucher@amd.com; christian.koenig@amd.com; Xinhui.Pan@amd.com; airlied@gmail.com; daniel@ffwll.ch Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: [PATCH 1/2] drm/amd/display: increase max surfaces in line with planes and streams 091a97e542cf ("drm/amd/display: Fix warning. Set MAX_SURFACES to 3") reduced the max number of surfaces since, at that time, there was no use for more. However, HW and driver evolves and there are now driver versions that allow two overlay planes (max_slave_planes). Moreover, commit 3cfd03b79425 ("drm/amd/display: update max streams per surface") states 6 is the max surfaces supported asics can have. Therefore, update MAX_SURFACES to match MAX_SURFACE_NUM, MAX_PLANES and MAX_STREAMS. It also addresses array-index-out-of-bounds reported in the link. Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3594 Signed-off-by: Melissa Wen <mwen@igalia.com> --- drivers/gpu/drm/amd/display/dc/dc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h index 3992ad73165b..08b00b263533 100644 --- a/drivers/gpu/drm/amd/display/dc/dc.h +++ b/drivers/gpu/drm/amd/display/dc/dc.h @@ -57,7 +57,7 @@ struct dmub_notification; #define DC_VER "3.2.301" -#define MAX_SURFACES 3 +#define MAX_SURFACES 6 #define MAX_PLANES 6 #define MAX_STREAMS 6 #define MIN_VIEWPORT_SIZE 12 -- 2.45.2
Hi Zaeem, Thanks for explaining their relationship. So IIUC, current DM implementation for dc_surface_updates array is wrong, since it's taking MAX_SURFACES (=3) for allocation but MAX_PLANES (=6) as the upper bound of size of the dc_surface_updates array, as you can see in this allocation and after an iteration from 0 to `plane_count`: https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L9861 Another question to understand what would be the right fix for the implementation above-mentioned: with the cursor overlay mode, it means we are using one of the overlay planes for this cursor overlay mode (one from the "max_slave_planes") or this cursor plane is an extra plane, which means, for some drivers, one primary plane + two overlay planes + one plane for cursor overlay mode (max 4 planes) ? BR, Melissa On 27/09/2024 14:40, Mohamed, Zaeem wrote: > [AMD Official Use Only - AMD Internal Distribution Only] > > Hi Melissa, > > MAX_SURFACE_NUM and MAX_PLANES both represent the upper bound of planes that are supported by HW. It is best to replace MAX_SURFACE_NUM with MAX_PLANES to remove redundancy. MAX_SURFACES is used to represent the upper bound of surfaces that can be piped to a single CRTC. Keep MAX_SURFACES as is to keep stack size down, and replace MAX_SURFACE_NUM with MAX_PLANES. > > Thanks, > Zaeem > > > -----Original Message----- > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Melissa Wen > Sent: Wednesday, September 25, 2024 11:37 AM > To: harry.wentland@amd.com; sunpeng.li@amd.com; Rodrigo.Siqueira@amd.com; alexander.deucher@amd.com; christian.koenig@amd.com; Xinhui.Pan@amd.com; airlied@gmail.com; daniel@ffwll.ch > Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: [PATCH 1/2] drm/amd/display: increase max surfaces in line with planes and streams > > 091a97e542cf ("drm/amd/display: Fix warning. Set MAX_SURFACES to 3") reduced the max number of surfaces since, at that time, there was no use for more. However, HW and driver evolves and there are now driver versions that allow two overlay planes (max_slave_planes). Moreover, commit 3cfd03b79425 ("drm/amd/display: update max streams per surface") states 6 is the max surfaces supported asics can have. Therefore, update MAX_SURFACES to match MAX_SURFACE_NUM, MAX_PLANES and MAX_STREAMS. > > It also addresses array-index-out-of-bounds reported in the link. > > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3594 > Signed-off-by: Melissa Wen <mwen@igalia.com> > --- > drivers/gpu/drm/amd/display/dc/dc.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h > index 3992ad73165b..08b00b263533 100644 > --- a/drivers/gpu/drm/amd/display/dc/dc.h > +++ b/drivers/gpu/drm/amd/display/dc/dc.h > @@ -57,7 +57,7 @@ struct dmub_notification; > > #define DC_VER "3.2.301" > > -#define MAX_SURFACES 3 > +#define MAX_SURFACES 6 > #define MAX_PLANES 6 > #define MAX_STREAMS 6 > #define MIN_VIEWPORT_SIZE 12 > -- > 2.45.2 >
Hi, Gentle ping to land the fix for the kernel crash. If faster, I can send a new version increasing max surfaces to 4 as previously discussed. There are now two bug reports for the same issue: - https://gitlab.freedesktop.org/drm/amd/-/issues/3693 - https://gitlab.freedesktop.org/drm/amd/-/issues/3594 Best Regards, Melissa On 27/09/2024 15:20, Melissa Wen wrote: > Hi Zaeem, > > Thanks for explaining their relationship. > > So IIUC, current DM implementation for dc_surface_updates array is > wrong, since it's taking MAX_SURFACES (=3) for allocation but > MAX_PLANES (=6) as the upper bound of size of the dc_surface_updates > array, as you can see in this allocation and after an iteration from 0 > to `plane_count`: > > https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L9861 > > > Another question to understand what would be the right fix for the > implementation above-mentioned: with the cursor overlay mode, it means > we are using one of the overlay planes for this cursor overlay mode > (one from the "max_slave_planes") or this cursor plane is an extra > plane, which means, for some drivers, one primary plane + two overlay > planes + one plane for cursor overlay mode (max 4 planes) ? > > BR, > > Melissa > > On 27/09/2024 14:40, Mohamed, Zaeem wrote: >> [AMD Official Use Only - AMD Internal Distribution Only] >> >> Hi Melissa, >> >> MAX_SURFACE_NUM and MAX_PLANES both represent the upper bound of >> planes that are supported by HW. It is best to replace >> MAX_SURFACE_NUM with MAX_PLANES to remove redundancy. MAX_SURFACES is >> used to represent the upper bound of surfaces that can be piped to a >> single CRTC. Keep MAX_SURFACES as is to keep stack size down, and >> replace MAX_SURFACE_NUM with MAX_PLANES. >> >> Thanks, >> Zaeem >> >> >> -----Original Message----- >> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf >> Of Melissa Wen >> Sent: Wednesday, September 25, 2024 11:37 AM >> To: harry.wentland@amd.com; sunpeng.li@amd.com; >> Rodrigo.Siqueira@amd.com; alexander.deucher@amd.com; >> christian.koenig@amd.com; Xinhui.Pan@amd.com; airlied@gmail.com; >> daniel@ffwll.ch >> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >> Subject: [PATCH 1/2] drm/amd/display: increase max surfaces in line >> with planes and streams >> >> 091a97e542cf ("drm/amd/display: Fix warning. Set MAX_SURFACES to 3") >> reduced the max number of surfaces since, at that time, there was no >> use for more. However, HW and driver evolves and there are now driver >> versions that allow two overlay planes (max_slave_planes). Moreover, >> commit 3cfd03b79425 ("drm/amd/display: update max streams per >> surface") states 6 is the max surfaces supported asics can have. >> Therefore, update MAX_SURFACES to match MAX_SURFACE_NUM, MAX_PLANES >> and MAX_STREAMS. >> >> It also addresses array-index-out-of-bounds reported in the link. >> >> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3594 >> Signed-off-by: Melissa Wen <mwen@igalia.com> >> --- >> drivers/gpu/drm/amd/display/dc/dc.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/display/dc/dc.h >> b/drivers/gpu/drm/amd/display/dc/dc.h >> index 3992ad73165b..08b00b263533 100644 >> --- a/drivers/gpu/drm/amd/display/dc/dc.h >> +++ b/drivers/gpu/drm/amd/display/dc/dc.h >> @@ -57,7 +57,7 @@ struct dmub_notification; >> >> #define DC_VER "3.2.301" >> >> -#define MAX_SURFACES 3 >> +#define MAX_SURFACES 6 >> #define MAX_PLANES 6 >> #define MAX_STREAMS 6 >> #define MIN_VIEWPORT_SIZE 12 >> -- >> 2.45.2 >> >
[AMD Official Use Only - AMD Internal Distribution Only] Hi, A patch addressing this will be sent out soon. Thanks, Zaeem -----Original Message----- From: Melissa Wen <mwen@igalia.com> Sent: Tuesday, October 22, 2024 11:58 AM To: Mohamed, Zaeem <Zaeem.Mohamed@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; airlied@gmail.com; daniel@ffwll.ch Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH 1/2] drm/amd/display: increase max surfaces in line with planes and streams Hi, Gentle ping to land the fix for the kernel crash. If faster, I can send a new version increasing max surfaces to 4 as previously discussed. There are now two bug reports for the same issue: - https://gitlab.freedesktop.org/drm/amd/-/issues/3693 - https://gitlab.freedesktop.org/drm/amd/-/issues/3594 Best Regards, Melissa On 27/09/2024 15:20, Melissa Wen wrote: > Hi Zaeem, > > Thanks for explaining their relationship. > > So IIUC, current DM implementation for dc_surface_updates array is > wrong, since it's taking MAX_SURFACES (=3) for allocation but > MAX_PLANES (=6) as the upper bound of size of the dc_surface_updates > array, as you can see in this allocation and after an iteration from 0 > to `plane_count`: > > https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/amd/disp > lay/amdgpu_dm/amdgpu_dm.c#L9861 > > > Another question to understand what would be the right fix for the > implementation above-mentioned: with the cursor overlay mode, it means > we are using one of the overlay planes for this cursor overlay mode > (one from the "max_slave_planes") or this cursor plane is an extra > plane, which means, for some drivers, one primary plane + two overlay > planes + one plane for cursor overlay mode (max 4 planes) ? > > BR, > > Melissa > > On 27/09/2024 14:40, Mohamed, Zaeem wrote: >> [AMD Official Use Only - AMD Internal Distribution Only] >> >> Hi Melissa, >> >> MAX_SURFACE_NUM and MAX_PLANES both represent the upper bound of >> planes that are supported by HW. It is best to replace >> MAX_SURFACE_NUM with MAX_PLANES to remove redundancy. MAX_SURFACES is >> used to represent the upper bound of surfaces that can be piped to a >> single CRTC. Keep MAX_SURFACES as is to keep stack size down, and >> replace MAX_SURFACE_NUM with MAX_PLANES. >> >> Thanks, >> Zaeem >> >> >> -----Original Message----- >> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf >> Of Melissa Wen >> Sent: Wednesday, September 25, 2024 11:37 AM >> To: harry.wentland@amd.com; sunpeng.li@amd.com; >> Rodrigo.Siqueira@amd.com; alexander.deucher@amd.com; >> christian.koenig@amd.com; Xinhui.Pan@amd.com; airlied@gmail.com; >> daniel@ffwll.ch >> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >> Subject: [PATCH 1/2] drm/amd/display: increase max surfaces in line >> with planes and streams >> >> 091a97e542cf ("drm/amd/display: Fix warning. Set MAX_SURFACES to 3") >> reduced the max number of surfaces since, at that time, there was no >> use for more. However, HW and driver evolves and there are now driver >> versions that allow two overlay planes (max_slave_planes). Moreover, >> commit 3cfd03b79425 ("drm/amd/display: update max streams per >> surface") states 6 is the max surfaces supported asics can have. >> Therefore, update MAX_SURFACES to match MAX_SURFACE_NUM, MAX_PLANES >> and MAX_STREAMS. >> >> It also addresses array-index-out-of-bounds reported in the link. >> >> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3594 >> Signed-off-by: Melissa Wen <mwen@igalia.com> >> --- >> drivers/gpu/drm/amd/display/dc/dc.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/display/dc/dc.h >> b/drivers/gpu/drm/amd/display/dc/dc.h >> index 3992ad73165b..08b00b263533 100644 >> --- a/drivers/gpu/drm/amd/display/dc/dc.h >> +++ b/drivers/gpu/drm/amd/display/dc/dc.h >> @@ -57,7 +57,7 @@ struct dmub_notification; >> >> #define DC_VER "3.2.301" >> >> -#define MAX_SURFACES 3 >> +#define MAX_SURFACES 6 >> #define MAX_PLANES 6 >> #define MAX_STREAMS 6 >> #define MIN_VIEWPORT_SIZE 12 >> -- >> 2.45.2 >> >
On 23/10/2024 17:36, Mohamed, Zaeem wrote: > [AMD Official Use Only - AMD Internal Distribution Only] > > Hi, > > A patch addressing this will be sent out soon. Great! Thanks for the heads up! Melissa > > Thanks, > Zaeem > > -----Original Message----- > From: Melissa Wen <mwen@igalia.com> > Sent: Tuesday, October 22, 2024 11:58 AM > To: Mohamed, Zaeem <Zaeem.Mohamed@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; airlied@gmail.com; daniel@ffwll.ch > Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: Re: [PATCH 1/2] drm/amd/display: increase max surfaces in line with planes and streams > > Hi, > > Gentle ping to land the fix for the kernel crash. > If faster, I can send a new version increasing max surfaces to 4 as previously discussed. > > There are now two bug reports for the same issue: > - https://gitlab.freedesktop.org/drm/amd/-/issues/3693 > - https://gitlab.freedesktop.org/drm/amd/-/issues/3594 > > Best Regards, > > Melissa > > > On 27/09/2024 15:20, Melissa Wen wrote: >> Hi Zaeem, >> >> Thanks for explaining their relationship. >> >> So IIUC, current DM implementation for dc_surface_updates array is >> wrong, since it's taking MAX_SURFACES (=3) for allocation but >> MAX_PLANES (=6) as the upper bound of size of the dc_surface_updates >> array, as you can see in this allocation and after an iteration from 0 >> to `plane_count`: >> >> https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/amd/disp >> lay/amdgpu_dm/amdgpu_dm.c#L9861 >> >> >> Another question to understand what would be the right fix for the >> implementation above-mentioned: with the cursor overlay mode, it means >> we are using one of the overlay planes for this cursor overlay mode >> (one from the "max_slave_planes") or this cursor plane is an extra >> plane, which means, for some drivers, one primary plane + two overlay >> planes + one plane for cursor overlay mode (max 4 planes) ? >> >> BR, >> >> Melissa >> >> On 27/09/2024 14:40, Mohamed, Zaeem wrote: >>> [AMD Official Use Only - AMD Internal Distribution Only] >>> >>> Hi Melissa, >>> >>> MAX_SURFACE_NUM and MAX_PLANES both represent the upper bound of >>> planes that are supported by HW. It is best to replace >>> MAX_SURFACE_NUM with MAX_PLANES to remove redundancy. MAX_SURFACES is >>> used to represent the upper bound of surfaces that can be piped to a >>> single CRTC. Keep MAX_SURFACES as is to keep stack size down, and >>> replace MAX_SURFACE_NUM with MAX_PLANES. >>> >>> Thanks, >>> Zaeem >>> >>> >>> -----Original Message----- >>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf >>> Of Melissa Wen >>> Sent: Wednesday, September 25, 2024 11:37 AM >>> To: harry.wentland@amd.com; sunpeng.li@amd.com; >>> Rodrigo.Siqueira@amd.com; alexander.deucher@amd.com; >>> christian.koenig@amd.com; Xinhui.Pan@amd.com; airlied@gmail.com; >>> daniel@ffwll.ch >>> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>> Subject: [PATCH 1/2] drm/amd/display: increase max surfaces in line >>> with planes and streams >>> >>> 091a97e542cf ("drm/amd/display: Fix warning. Set MAX_SURFACES to 3") >>> reduced the max number of surfaces since, at that time, there was no >>> use for more. However, HW and driver evolves and there are now driver >>> versions that allow two overlay planes (max_slave_planes). Moreover, >>> commit 3cfd03b79425 ("drm/amd/display: update max streams per >>> surface") states 6 is the max surfaces supported asics can have. >>> Therefore, update MAX_SURFACES to match MAX_SURFACE_NUM, MAX_PLANES >>> and MAX_STREAMS. >>> >>> It also addresses array-index-out-of-bounds reported in the link. >>> >>> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3594 >>> Signed-off-by: Melissa Wen <mwen@igalia.com> >>> --- >>> drivers/gpu/drm/amd/display/dc/dc.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/amd/display/dc/dc.h >>> b/drivers/gpu/drm/amd/display/dc/dc.h >>> index 3992ad73165b..08b00b263533 100644 >>> --- a/drivers/gpu/drm/amd/display/dc/dc.h >>> +++ b/drivers/gpu/drm/amd/display/dc/dc.h >>> @@ -57,7 +57,7 @@ struct dmub_notification; >>> >>> #define DC_VER "3.2.301" >>> >>> -#define MAX_SURFACES 3 >>> +#define MAX_SURFACES 6 >>> #define MAX_PLANES 6 >>> #define MAX_STREAMS 6 >>> #define MIN_VIEWPORT_SIZE 12 >>> -- >>> 2.45.2 >>>
diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h index 3992ad73165b..08b00b263533 100644 --- a/drivers/gpu/drm/amd/display/dc/dc.h +++ b/drivers/gpu/drm/amd/display/dc/dc.h @@ -57,7 +57,7 @@ struct dmub_notification; #define DC_VER "3.2.301" -#define MAX_SURFACES 3 +#define MAX_SURFACES 6 #define MAX_PLANES 6 #define MAX_STREAMS 6 #define MIN_VIEWPORT_SIZE 12
091a97e542cf ("drm/amd/display: Fix warning. Set MAX_SURFACES to 3") reduced the max number of surfaces since, at that time, there was no use for more. However, HW and driver evolves and there are now driver versions that allow two overlay planes (max_slave_planes). Moreover, commit 3cfd03b79425 ("drm/amd/display: update max streams per surface") states 6 is the max surfaces supported asics can have. Therefore, update MAX_SURFACES to match MAX_SURFACE_NUM, MAX_PLANES and MAX_STREAMS. It also addresses array-index-out-of-bounds reported in the link. Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3594 Signed-off-by: Melissa Wen <mwen@igalia.com> --- drivers/gpu/drm/amd/display/dc/dc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)