Re: [PATCH 1/2] drm/amd/display: Reduce stack size for dml31_ModeSupportAndSystemConfigurationFull

[Rodrigo Siqueira Jordao <rjordrigo@xxxxxxx>]

On 2021-12-13 4:46 a.m., Michel Dänzer wrote:
> On 2021-12-11 13:20, Rodrigo Siqueira Jordao wrote:
> > On 2021-12-09 11:46 a.m., Michel Dänzer wrote:
> > > From: Michel Dänzer <mdaenzer@xxxxxxxxxx>
> > >
> > > Move code using the Pipe struct to a new helper function.
> > >
> > > Works around[0] this warning (resulting in failure to build a RHEL debug
> > > kernel with Werror enabled):
> > >
> > > ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn31/display_mode_vba_31.c: In function ‘dml31_ModeSupportAndSystemConfigurationFull’:
> > > ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn31/display_mode_vba_31.c:5740:1: warning: the frame size of 2144 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> > >
> > > The culprit seems to be the Pipe struct, so pull the relevant block out
> > > into its own sub-function. (This is porting
> > > a62427ef9b55 "drm/amd/display: Reduce stack size for dml21_ModeSupportAndSystemConfigurationFull"
> > > from dml31 to dml21)
> > >
> > > [0] AFAICT this doesn't actually reduce the total amount of stack which
> > > can be used, just moves some of it from
> > > dml31_ModeSupportAndSystemConfigurationFull to the new helper function,
> > > so the former happens to no longer exceed the limit for a single
> > > function.
> > >
> > > Signed-off-by: Michel Dänzer <mdaenzer@xxxxxxxxxx>
> > > ---
> > >    .../dc/dml/dcn31/display_mode_vba_31.c        | 185 ++++++++++--------
> > >    1 file changed, 99 insertions(+), 86 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/display/dc/dml/dcn31/display_mode_vba_31.c b/drivers/gpu/drm/amd/display/dc/dml/dcn31/display_mode_vba_31.c
> > > index 7e937bdcea00..8965f9af9d0a 100644
> > > --- a/drivers/gpu/drm/amd/display/dc/dml/dcn31/display_mode_vba_31.c
> > > +++ b/drivers/gpu/drm/amd/display/dc/dml/dcn31/display_mode_vba_31.c
> > > @@ -3949,6 +3949,102 @@ static double TruncToValidBPP(
> > >        return BPP_INVALID;
> > >    }
> > >    +static noinline void CalculatePrefetchSchedulePerPlane(
> > > +        struct display_mode_lib *mode_lib,
> > > +        double HostVMInefficiencyFactor,
> > > +        int i,
> > > +        unsigned j,
> > > +        unsigned k)
> > > +{
> > > +    struct vba_vars_st *v = &mode_lib->vba;
> > > +    Pipe myPipe;
> > > +
> > > +    myPipe.DPPCLK = v->RequiredDPPCLK[i][j][k];
> > > +    myPipe.DISPCLK = v->RequiredDISPCLK[i][j];
> > > +    myPipe.PixelClock = v->PixelClock[k];
> > > +    myPipe.DCFCLKDeepSleep = v->ProjectedDCFCLKDeepSleep[i][j];
> > > +    myPipe.DPPPerPlane = v->NoOfDPP[i][j][k];
> > > +    myPipe.ScalerEnabled = v->ScalerEnabled[k];
> > > +    myPipe.SourceScan = v->SourceScan[k];
> > > +    myPipe.BlockWidth256BytesY = v->Read256BlockWidthY[k];
> > > +    myPipe.BlockHeight256BytesY = v->Read256BlockHeightY[k];
> > > +    myPipe.BlockWidth256BytesC = v->Read256BlockWidthC[k];
> > > +    myPipe.BlockHeight256BytesC = v->Read256BlockHeightC[k];
> > > +    myPipe.InterlaceEnable = v->Interlace[k];
> > > +    myPipe.NumberOfCursors = v->NumberOfCursors[k];
> > > +    myPipe.VBlank = v->VTotal[k] - v->VActive[k];
> > > +    myPipe.HTotal = v->HTotal[k];
> > > +    myPipe.DCCEnable = v->DCCEnable[k];
> > > +    myPipe.ODMCombineIsEnabled = v->ODMCombineEnablePerState[i][k] == dm_odm_combine_mode_4to1
> > > +        || v->ODMCombineEnablePerState[i][k] == dm_odm_combine_mode_2to1;
> > > +    myPipe.SourcePixelFormat = v->SourcePixelFormat[k];
> > > +    myPipe.BytePerPixelY = v->BytePerPixelY[k];
> > > +    myPipe.BytePerPixelC = v->BytePerPixelC[k];
> > > +    myPipe.ProgressiveToInterlaceUnitInOPP = v->ProgressiveToInterlaceUnitInOPP;
> > > +    v->NoTimeForPrefetch[i][j][k] = CalculatePrefetchSchedule(
> > > +        mode_lib,
> > > +        HostVMInefficiencyFactor,
> > > +        &myPipe,
> > > +        v->DSCDelayPerState[i][k],
> > > +        v->DPPCLKDelaySubtotal + v->DPPCLKDelayCNVCFormater,
> > > +        v->DPPCLKDelaySCL,
> > > +        v->DPPCLKDelaySCLLBOnly,
> > > +        v->DPPCLKDelayCNVCCursor,
> > > +        v->DISPCLKDelaySubtotal,
> > > +        v->SwathWidthYThisState[k] / v->HRatio[k],
> > > +        v->OutputFormat[k],
> > > +        v->MaxInterDCNTileRepeaters,
> > > +        dml_min(v->MaxVStartup, v->MaximumVStartup[i][j][k]),
> > > +        v->MaximumVStartup[i][j][k],
> > > +        v->GPUVMMaxPageTableLevels,
> > > +        v->GPUVMEnable,
> > > +        v->HostVMEnable,
> > > +        v->HostVMMaxNonCachedPageTableLevels,
> > > +        v->HostVMMinPageSize,
> > > +        v->DynamicMetadataEnable[k],
> > > +        v->DynamicMetadataVMEnabled,
> > > +        v->DynamicMetadataLinesBeforeActiveRequired[k],
> > > +        v->DynamicMetadataTransmittedBytes[k],
> > > +        v->UrgLatency[i],
> > > +        v->ExtraLatency,
> > > +        v->TimeCalc,
> > > +        v->PDEAndMetaPTEBytesPerFrame[i][j][k],
> > > +        v->MetaRowBytes[i][j][k],
> > > +        v->DPTEBytesPerRow[i][j][k],
> > > +        v->PrefetchLinesY[i][j][k],
> > > +        v->SwathWidthYThisState[k],
> > > +        v->PrefillY[k],
> > > +        v->MaxNumSwY[k],
> > > +        v->PrefetchLinesC[i][j][k],
> > > +        v->SwathWidthCThisState[k],
> > > +        v->PrefillC[k],
> > > +        v->MaxNumSwC[k],
> > > +        v->swath_width_luma_ub_this_state[k],
> > > +        v->swath_width_chroma_ub_this_state[k],
> > > +        v->SwathHeightYThisState[k],
> > > +        v->SwathHeightCThisState[k],
> > > +        v->TWait,
> > > +        &v->DSTXAfterScaler[k],
> > > +        &v->DSTYAfterScaler[k],
> > > +        &v->LineTimesForPrefetch[k],
> > > +        &v->PrefetchBW[k],
> > > +        &v->LinesForMetaPTE[k],
> > > +        &v->LinesForMetaAndDPTERow[k],
> > > +        &v->VRatioPreY[i][j][k],
> > > +        &v->VRatioPreC[i][j][k],
> > > +        &v->RequiredPrefetchPixelDataBWLuma[i][j][k],
> > > +        &v->RequiredPrefetchPixelDataBWChroma[i][j][k],
> > > +        &v->NoTimeForDynamicMetadata[i][j][k],
> > > +        &v->Tno_bw[k],
> > > +        &v->prefetch_vmrow_bw[k],
> > > +        &v->dummy7[k],
> > > +        &v->dummy8[k],
> > > +        &v->dummy13[k],
> > > +        &v->VUpdateOffsetPix[k],
> > > +        &v->VUpdateWidthPix[k],
> > > +        &v->VReadyOffsetPix[k]);
> > > +}
> > > +
> > >    void dml31_ModeSupportAndSystemConfigurationFull(struct display_mode_lib *mode_lib)
> > >    {
> > >        struct vba_vars_st *v = &mode_lib->vba;
> > > @@ -5276,92 +5372,9 @@ void dml31_ModeSupportAndSystemConfigurationFull(struct display_mode_lib *mode_l
> > >                            v->SREnterPlusExitTime);
> > >                      for (k = 0; k < v->NumberOfActivePlanes; k++) {
> > > -                    Pipe myPipe;
> > > -
> > > -                    myPipe.DPPCLK = v->RequiredDPPCLK[i][j][k];
> > > -                    myPipe.DISPCLK = v->RequiredDISPCLK[i][j];
> > > -                    myPipe.PixelClock = v->PixelClock[k];
> > > -                    myPipe.DCFCLKDeepSleep = v->ProjectedDCFCLKDeepSleep[i][j];
> > > -                    myPipe.DPPPerPlane = v->NoOfDPP[i][j][k];
> > > -                    myPipe.ScalerEnabled = v->ScalerEnabled[k];
> > > -                    myPipe.SourceScan = v->SourceScan[k];
> > > -                    myPipe.BlockWidth256BytesY = v->Read256BlockWidthY[k];
> > > -                    myPipe.BlockHeight256BytesY = v->Read256BlockHeightY[k];
> > > -                    myPipe.BlockWidth256BytesC = v->Read256BlockWidthC[k];
> > > -                    myPipe.BlockHeight256BytesC = v->Read256BlockHeightC[k];
> > > -                    myPipe.InterlaceEnable = v->Interlace[k];
> > > -                    myPipe.NumberOfCursors = v->NumberOfCursors[k];
> > > -                    myPipe.VBlank = v->VTotal[k] - v->VActive[k];
> > > -                    myPipe.HTotal = v->HTotal[k];
> > > -                    myPipe.DCCEnable = v->DCCEnable[k];
> > > -                    myPipe.ODMCombineIsEnabled = v->ODMCombineEnablePerState[i][k] == dm_odm_combine_mode_4to1
> > > -                            || v->ODMCombineEnablePerState[i][k] == dm_odm_combine_mode_2to1;
> > > -                    myPipe.SourcePixelFormat = v->SourcePixelFormat[k];
> > > -                    myPipe.BytePerPixelY = v->BytePerPixelY[k];
> > > -                    myPipe.BytePerPixelC = v->BytePerPixelC[k];
> > > -                    myPipe.ProgressiveToInterlaceUnitInOPP = v->ProgressiveToInterlaceUnitInOPP;
> > > -                    v->NoTimeForPrefetch[i][j][k] = CalculatePrefetchSchedule(
> > > -                            mode_lib,
> > > -                            HostVMInefficiencyFactor,
> > > -                            &myPipe,
> > > -                            v->DSCDelayPerState[i][k],
> > > -                            v->DPPCLKDelaySubtotal + v->DPPCLKDelayCNVCFormater,
> > > -                            v->DPPCLKDelaySCL,
> > > -                            v->DPPCLKDelaySCLLBOnly,
> > > -                            v->DPPCLKDelayCNVCCursor,
> > > -                            v->DISPCLKDelaySubtotal,
> > > -                            v->SwathWidthYThisState[k] / v->HRatio[k],
> > > -                            v->OutputFormat[k],
> > > -                            v->MaxInterDCNTileRepeaters,
> > > -                            dml_min(v->MaxVStartup, v->MaximumVStartup[i][j][k]),
> > > -                            v->MaximumVStartup[i][j][k],
> > > -                            v->GPUVMMaxPageTableLevels,
> > > -                            v->GPUVMEnable,
> > > -                            v->HostVMEnable,
> > > -                            v->HostVMMaxNonCachedPageTableLevels,
> > > -                            v->HostVMMinPageSize,
> > > -                            v->DynamicMetadataEnable[k],
> > > -                            v->DynamicMetadataVMEnabled,
> > > -                            v->DynamicMetadataLinesBeforeActiveRequired[k],
> > > -                            v->DynamicMetadataTransmittedBytes[k],
> > > -                            v->UrgLatency[i],
> > > -                            v->ExtraLatency,
> > > -                            v->TimeCalc,
> > > -                            v->PDEAndMetaPTEBytesPerFrame[i][j][k],
> > > -                            v->MetaRowBytes[i][j][k],
> > > -                            v->DPTEBytesPerRow[i][j][k],
> > > -                            v->PrefetchLinesY[i][j][k],
> > > -                            v->SwathWidthYThisState[k],
> > > -                            v->PrefillY[k],
> > > -                            v->MaxNumSwY[k],
> > > -                            v->PrefetchLinesC[i][j][k],
> > > -                            v->SwathWidthCThisState[k],
> > > -                            v->PrefillC[k],
> > > -                            v->MaxNumSwC[k],
> > > -                            v->swath_width_luma_ub_this_state[k],
> > > -                            v->swath_width_chroma_ub_this_state[k],
> > > -                            v->SwathHeightYThisState[k],
> > > -                            v->SwathHeightCThisState[k],
> > > -                            v->TWait,
> > > -                            &v->DSTXAfterScaler[k],
> > > -                            &v->DSTYAfterScaler[k],
> > > -                            &v->LineTimesForPrefetch[k],
> > > -                            &v->PrefetchBW[k],
> > > -                            &v->LinesForMetaPTE[k],
> > > -                            &v->LinesForMetaAndDPTERow[k],
> > > -                            &v->VRatioPreY[i][j][k],
> > > -                            &v->VRatioPreC[i][j][k],
> > > -                            &v->RequiredPrefetchPixelDataBWLuma[i][j][k],
> > > -                            &v->RequiredPrefetchPixelDataBWChroma[i][j][k],
> > > -                            &v->NoTimeForDynamicMetadata[i][j][k],
> > > -                            &v->Tno_bw[k],
> > > -                            &v->prefetch_vmrow_bw[k],
> > > -                            &v->dummy7[k],
> > > -                            &v->dummy8[k],
> > > -                            &v->dummy13[k],
> > > -                            &v->VUpdateOffsetPix[k],
> > > -                            &v->VUpdateWidthPix[k],
> > > -                            &v->VReadyOffsetPix[k]);
> > > +                    CalculatePrefetchSchedulePerPlane(mode_lib,
> > > +                                      HostVMInefficiencyFactor,
> > > +                                      i, j,    k);
> > >                    }
> > >                      for (k = 0; k < v->NumberOfActivePlanes; k++) {
> >
> > Hi Michel,
> >
> > Overwall I think this series is good. I also run it in our internal CI and everything looks fine.
> >
> > Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@xxxxxxx>
> >
> > And applied to amd-staging-drm-next.
>
> Thanks!
>
> Are there any plans for actually reducing the combined amount of stack used by ModeSupportAndSystemConfigurationFull + CalculatePrefetchSchedulePerPlane?

Hi Michel,

Tbh, I'm not fully aware of the problem with the stack size used by 
"ModeSupportAndSystemConfigurationFull + 
CalculatePrefetchSchedulePerPlane". Could you help me to understand it 
better? Could you provide some background? Also, could you help me 
better understand the impact of this stack size issue in the DML code? 
Any information will be helpful.

> Also, did you check that UseMinimumDCFCLK now modifying mode_lib->vba.DCFCLKState[i][j] and possibly other values in mode_lib->vba makes sense?

To check this patch, I submitted it to our Internal CI, where we ran a 
couple of IGT tests in multiple ASICs, and I conducted a simple smoke 
test using 5600XT and a Raven system. Everything was fine.

Finally, I checked Dmytro's opinion about this change, and he agreed 
that your patch is fine.

Thanks
Siqueira