RE: [PATCH] drm/amd/display: Align dcn314_smu logging with other DCNs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[Public]



> -----Original Message-----
> From: Li, Roman <Roman.Li@xxxxxxx>
> Sent: Tuesday, November 15, 2022 11:52
> To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>; amd-
> gfx@xxxxxxxxxxxxxxxxxxxxx; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Wentland, Harry
> <Harry.Wentland@xxxxxxx>; Rizvi, Saaem <SyedSaaem.Rizvi@xxxxxxx>
> 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@xxxxxxx>
> > Sent: Monday, November 14, 2022 4:16 PM
> > To: Li, Roman <Roman.Li@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Wentland, Harry
> > <Harry.Wentland@xxxxxxx>; Rizvi, Saaem <SyedSaaem.Rizvi@xxxxxxx>
> > Cc: Li, Roman <Roman.Li@xxxxxxx>
> > 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@xxxxxxx <Roman.Li@xxxxxxx>
> > > Sent: Monday, November 14, 2022 15:07
> > > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Deucher, Alexander
> > > <Alexander.Deucher@xxxxxxx>; Wentland, Harry
> > <Harry.Wentland@xxxxxxx>;
> > > Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Rizvi, Saaem
> > > <SyedSaaem.Rizvi@xxxxxxx>
> > > Cc: Li, Roman <Roman.Li@xxxxxxx>
> > > Subject: [PATCH] drm/amd/display: Align dcn314_smu logging with other
> > > DCNs
> > >
> > > From: Roman Li <roman.li@xxxxxxx>
> > >
> > > [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@xxxxxxx>
> > > Signed-off-by: Roman Li <roman.li@xxxxxxx>
> > > ---
> > >  .../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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux