On 2021-12-13 15:22, Rodrigo Siqueira Jordao wrote:
>
>
> 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?
The warning which inspired this patch (and the corresponding change to dml21_ModeSupportAndSystemConfigurationFull) is about ModeSupportAndSystemConfigurationFull exceeding a threshold of stack usage. The patch moves some of that stack usage to the new CalculatePrefetchSchedulePerPlane helper function. However, since the former calls the latter, together they still use as much stack (possibly even slightly more, due to the function calling convention) as before. So while we've silenced the warning, we haven't actually improved the situation the warning is about.
> 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.
I don't know the exact reasons for the warning offhand. Presumably the fact that this warning is enabled indicates that stack usage should be minimized though, or at least stack shouldn't be lightly wasted.
>> 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.
Great, thanks.
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
