[Public] Conceptually makes sense to me, but please see below comments:
> -----Original Message----- > From: [email protected] <[email protected]> > Sent: Monday, November 14, 2022 15:07 > To: [email protected]; Deucher, Alexander > <[email protected]>; Wentland, Harry > <[email protected]>; Limonciello, Mario > <[email protected]>; Rizvi, Saaem <[email protected]> > Cc: Li, Roman <[email protected]> > Subject: [PATCH] drm/amd/display: Align dcn314_smu logging with other > DCNs > > From: Roman Li <[email protected]> > > [Why] > Assert on non-OK response from SMU is unnecessary. > It was replaced with respective log message on other asics > in the past with commit: > "drm/amd/display: Removing assert statements for Linux" > > [How] > Remove asert and add dbg logging as on other DCNs. > > CC: Saaem Rizvi <[email protected]> > Signed-off-by: Roman Li <[email protected]> > --- > .../drm/amd/display/dc/clk_mgr/dcn314/dcn314_smu.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn314/dcn314_smu.c > b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn314/dcn314_smu.c > index ef0795b14a1f..2db595672a46 100644 > --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn314/dcn314_smu.c > +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn314/dcn314_smu.c > @@ -123,9 +123,10 @@ static int > dcn314_smu_send_msg_with_param(struct clk_mgr_internal *clk_mgr, > uint32_t result; > > result = dcn314_smu_wait_for_response(clk_mgr, 10, 200000); > - ASSERT(result == VBIOSSMC_Result_OK); Does this flow actually happen still? I thought the assertion should have gone away as a result of 83293f7f3d15fc56e86bd5067a2c88b6b233ac3a. Maybe we want to also undo the REG_WRITE() call there if pulling this in. > > - smu_print("SMU response after wait: %d\n", result); > + if (result != VBIOSSMC_Result_OK) > + smu_print("SMU Response was not OK. SMU response after > wait received is: %d\n", > + result); > > if (result == VBIOSSMC_Status_BUSY) > return -1; I think what is missing to clean up recent asserts is actually a little bit further in the code than this. It should be part of the error flow introduced by 03ad3093c7c069d6ab4403730009ebafeea9ee37 > @@ -216,6 +217,12 @@ int dcn314_smu_set_hard_min_dcfclk(struct > clk_mgr_internal *clk_mgr, int request > VBIOSSMC_MSG_SetHardMinDcfclkByFreq, > khz_to_mhz_ceil(requested_dcfclk_khz)); > > +#ifdef DBG > + smu_print("actual_dcfclk_set_mhz %d is set to : %d\n", > + actual_dcfclk_set_mhz, > + actual_dcfclk_set_mhz * 1000); > +#endif > + > return actual_dcfclk_set_mhz * 1000; > } > > -- > 2.17.1
