<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="blue" vlink="#954F72" style="word-wrap:break-word"> [Public] <div> <div class="WordSection1"> Reviewed-by: Saaem Rizvi <SyedSaaem.Rizvi@amd.com> <div style="mso-element:para-border-div;border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: <a href="mailto:Mario.Limonciello@amd.com">Limonciello, Mario</a> Sent: Tuesday, November 15, 2022 1:02 PM To: <a href="mailto:Roman.Li@amd.com">Li, Roman</a>; <a href="mailto:amd-gfx@lists.freedesktop.org"> amd-gfx@lists.freedesktop.org</a>; <a href="mailto:Alexander.Deucher@amd.com">Deucher, Alexander</a>; <a href="mailto:Harry.Wentland@amd.com">Wentland, Harry</a>; <a href="mailto:SyedSaaem.Rizvi@amd.com"> Rizvi, Saaem</a> Subject: RE: [PATCH] drm/amd/display: Align dcn314_smu logging with other DCNs </div> <o:p> </o:p> [Public] > -----Original Message----- > From: Li, Roman <Roman.Li@amd.com> > Sent: Tuesday, November 15, 2022 11:52 > To: Limonciello, Mario <Mario.Limonciello@amd.com>; amd- > gfx@lists.freedesktop.org; Deucher, Alexander > <Alexander.Deucher@amd.com>; Wentland, Harry > <Harry.Wentland@amd.com>; Rizvi, Saaem <SyedSaaem.Rizvi@amd.com> > Subject: RE: [PATCH] drm/amd/display: Align dcn314_smu logging with other > DCNs > > Hi Mario, > > Thanks for your comments. > I replied inline. > > > -----Original Message----- > > From: Limonciello, Mario <Mario.Limonciello@amd.com> > > Sent: Monday, November 14, 2022 4:16 PM > > To: Li, Roman <Roman.Li@amd.com>; amd-gfx@lists.freedesktop.org; > > Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry > > <Harry.Wentland@amd.com>; Rizvi, Saaem <SyedSaaem.Rizvi@amd.com> > > Cc: Li, Roman <Roman.Li@amd.com> > > Subject: RE: [PATCH] drm/amd/display: Align dcn314_smu logging with > other > > DCNs > > > > [Public] > > > > Conceptually makes sense to me, but please see below comments: > > > > > -----Original Message----- > > > From: Roman.Li@amd.com <Roman.Li@amd.com> > > > Sent: Monday, November 14, 2022 15:07 > > > To: amd-gfx@lists.freedesktop.org; Deucher, Alexander > > > <Alexander.Deucher@amd.com>; Wentland, Harry > > <Harry.Wentland@amd.com>; > > > Limonciello, Mario <Mario.Limonciello@amd.com>; Rizvi, Saaem > > > <SyedSaaem.Rizvi@amd.com> > > > Cc: Li, Roman <Roman.Li@amd.com> > > > Subject: [PATCH] drm/amd/display: Align dcn314_smu logging with other > > > DCNs > > > > > > From: Roman Li <roman.li@amd.com> > > > > > > [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. > > I will fix "assert" spelling before merging. > > > > > > > CC: Saaem Rizvi <SyedSaaem.Rizvi@amd.com> > > > Signed-off-by: Roman Li <roman.li@amd.com> > > > --- > > > .../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. > > > Happens or not, we don't assert here on other asics. > I don't try to fix any bugs with this patch, just align the dcn314 logging/bug > reporting with other asics. Got it thanks. > > > 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 > > 03ad3093c7c069d6a is for dcn3.1 initially. > If there's an issue with it (which I didn't experience) it should be addressed > on all dcn3x, that reuse it, in a separate patch. OK. > > > > > > > @@ -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 > > Thanks, > Roman<o:p></o:p> <o:p> </o:p> </div> </div> </body> </html>