[PATCH v2] drm/xe: Change pcode timeout to 50msec while polling again

[Ghimiray, Himal Prasad]

On 08-05-2024 18:29, Rodrigo Vivi wrote:
> On Wed, May 08, 2024 at 07:55:10AM -0500, Lucas De Marchi wrote:
>> On Wed, May 08, 2024 at 09:13:28AM GMT, Himal Prasad Ghimiray wrote:
>>> Polling is initially attempted with timeout_base_ms enabled for
>>> preemption, and if it exceeds this timeframe, another attempt is made
>>> without preemption, allowing an additional 50 ms before timing out.
>>>
>>> Display driver can request for timeout of 3 msec, hence modify the
>>> warning condition.
>> this seems like a separate fix?
>>
>> afaics below we have:
>>
>> 1) fix the warning to consider the display case ... and shouldn't the
>>     warning be before the first pcode_try_request()? Why do we want to
>>     warn before a fixed-duration rather than before the variable duration
>>     passed as argument?
>>
>> 2) change the second timeout to be a fixed 50 msec rather than 1msec so
>>     it follows what was actually documented in that comment.
> (for the 1 and 2 I was okay with having the squashed patch tbh, but
> yeap, probably better a separate one)

Sure. Will separate out.


>
> 3) status = 0;
> (this was not even mentioned in the commit message, and I noticed
> after the fact when I was about to merge it. Sorry about that.)

Oops, I missed removing this in the patch. I'll make sure to address it 
in the next version. Thanks for pointing it out!

I have also added assert |xe_gt_assert(gt, timeout_us > 0);| to warn if 
the user sets the timeout to 0.

Do you think we should include this check in patch 1 or patch 2, or do 
you think it's unnecessary?

>
>> or maybe I'm missing something here... could you clarify?
>>
>> Lucas De Marchi
>>
>>> v2
>>> - Rebase
>>>
>>> Cc: Rodrigo Vivi<rodrigo.vivi at intel.com>
>>> Signed-off-by: Himal Prasad Ghimiray<himal.prasad.ghimiray at intel.com>
>>> ---
>>> drivers/gpu/drm/xe/xe_pcode.c | 9 ++++++---
>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
>>> index c010ef16fbf5..2771eed1e45f 100644
>>> --- a/drivers/gpu/drm/xe/xe_pcode.c
>>> +++ b/drivers/gpu/drm/xe/xe_pcode.c
>>> @@ -10,6 +10,7 @@
>>>
>>> #include <drm/drm_managed.h>
>>>
>>> +#include "xe_assert.h"
>>> #include "xe_device.h"
>>> #include "xe_gt.h"
>>> #include "xe_mmio.h"
>>> @@ -124,6 +125,8 @@ static int pcode_try_request(struct xe_gt *gt, u32 mbox,
>>> {
>>> 	int slept, wait = 10;
>>>
>>> +	xe_gt_assert(gt, timeout_us > 0);
>>> +
>>> 	for (slept = 0; slept < timeout_us; slept += wait) {
>>> 		if (locked)
>>> 			*status = pcode_mailbox_rw(gt, mbox, &request, NULL, 1, true,
>>> @@ -166,7 +169,7 @@ static int pcode_try_request(struct xe_gt *gt, u32 mbox,
>>> int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
>>> 		      u32 reply_mask, u32 reply, int timeout_base_ms)
>>> {
>>> -	u32 status;
>>> +	u32 status = 0;
>>> 	int ret;
>>>
>>> 	mutex_lock(&gt->pcode.lock);
>>> @@ -188,10 +191,10 @@ int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
>>> 	 */
>>> 	drm_err(&gt_to_xe(gt)->drm,
>>> 		"PCODE timeout, retrying with preemption disabled\n");
>>> -	drm_WARN_ON_ONCE(&gt_to_xe(gt)->drm, timeout_base_ms > 1);
>>> +	drm_WARN_ON_ONCE(&gt_to_xe(gt)->drm, timeout_base_ms > 3);
>>> 	preempt_disable();
>>> 	ret = pcode_try_request(gt, mbox, request, reply_mask, reply, &status,
>>> -				true, timeout_base_ms * 1000, true);
>>> +				true, 50 * 1000, true);
>>> 	preempt_enable();
>>>
>>> out:
>>> -- 
>>> 2.25.1
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20240508/3b52baaa/attachment-0001.htm>