[PATCH] drm/amdgpu: try again kiq access if not in IRQ(v2)

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

 



Hi Monk,

then that was written differently. Maybe "might_sleep()" or something 
like that?

It's just a checker which raises a nice warning when you try to sleep in 
atomic context on debug kernels.

Christian.

Am 05.03.2018 um 12:27 schrieb Liu, Monk:
> Hi Christian
>
> I couln't find this "may_sleep()" in my 4.13kernel , did I miss something ??
>
> Thanks
> /Monk
>
> -----Original Message-----
> From: Christian König [mailto:ckoenig.leichtzumerken at gmail.com]
> Sent: 2018å¹´3æ??5æ?¥ 19:21
> To: Liu, Monk <Monk.Liu at amd.com>; Koenig, Christian <Christian.Koenig at amd.com>; Kuehling, Felix <Felix.Kuehling at amd.com>; amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: try again kiq access if not in IRQ(v2)
>
> Am 05.03.2018 um 09:08 schrieb Liu, Monk:
>> To better approach this issue I suggest to do the following:
>> 1. Revert the original patch.
>>
>> 2. Stop waiting to long for writes. E.g. use a separate timeout (20ms
>> maybe?) to wait for the write. Then do a WARN_ON_ONCE when we timeout.
>> Cannot do that 20ms is not enough, sometimes you need 10 seconds since
>> other VFs may doing bad things like occupying GPU intentionally or
>> they are doing TDR, so I don't think separate read and write is good
>> idea, they should be treated equally
> Well the question is if separating read&writes would actually help.
>
>> 3. To the read function add a "if (!in_intterupt()) may_sleep();" and then retest. That should at least print a nice warning when called from atomic context.
>> Sorry what is may_sleep() ??
>>
>> 4. Test the whole thing and try to fix all warnings about atomic
>> contexts from the may_sleep();
>>
>> 5. Reapply the original patch, but this time only for the read function, not the write function.
>>
>>
>>   From current LKG code, the only one spin lock may wrapping the
>> kiq_rreg/wreg() is the pcie_idx_lock, and this lock is only used during init(), Since init() is run under the case of exclusive mode for SRIOV, which means:
>> 1)  register access is not go through KIQ (see admgpu_mm_reg)
>> 2)  those functions are only in bif_medium_grain_xxx part (vi.c and
>> nbio_v6.c) , and they won't hit under SRIOV ( we return in the head if SRIOV detect) So I don' think this spin_lock may cause trouble...
> Ok in this case let's keep the patch for now, but please provide a new patch which adds "if (!in_intterupt()) may_sleep();" in both the read and write function.
>
> This way we should at least catch problems early on.
>
> Christian.
>
>> /Monk
>>
>>
>>
>> -----Original Message-----
>> From: Christian König [mailto:ckoenig.leichtzumerken at gmail.com]
>> Sent: 2018å¹´3æ??5æ?¥ 15:57
>> To: Liu, Monk <Monk.Liu at amd.com>; Koenig, Christian
>> <Christian.Koenig at amd.com>; Kuehling, Felix <Felix.Kuehling at amd.com>;
>> amd-gfx at lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: try again kiq access if not in
>> IRQ(v2)
>>
>>> otherwise I don't see how it is better by reverting it
>> Well it's better to revert it for now because it seems to create more problems than it solves.
>>
>> To better approach this issue I suggest to do the following:
>> 1. Revert the original patch.
>>
>> 2. Stop waiting to long for writes. E.g. use a separate timeout (20ms
>> maybe?) to wait for the write. Then do a WARN_ON_ONCE when we timeout.
>>
>> 3. To the read function add a "if (!in_intterupt()) may_sleep();" and then retest. That should at least print a nice warning when called from atomic context.
>>
>> 4. Test the whole thing and try to fix all warnings about atomic
>> contexts from the may_sleep();
>>
>> 5. Reapply the original patch, but this time only for the read function, not the write function.
>>
>> Regards,
>> Christian.
>>
>> Am 05.03.2018 um 05:20 schrieb Liu, Monk:
>>> When there are 16 VF/VM on one GPU, we can easily hit "sys lockup to
>>> 22s" kernel error/warning introduced by kiq_rreg/wreg routine That's
>>> why I must use this patch to let thread sleep a while and try again,
>>>
>>> If you insist reverting this patch please give me a solution,
>>> otherwise I don't see how it is better by reverting it
>>>
>>> /Monk
>>>
>>> -----Original Message-----
>>> From: Christian König [mailto:ckoenig.leichtzumerken at gmail.com]
>>> Sent: 2018å¹´3æ??3æ?¥ 21:38
>>> To: Kuehling, Felix <Felix.Kuehling at amd.com>; Liu, Monk
>>> <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/amdgpu: try again kiq access if not in
>>> IRQ(v2)
>>>
>>> Am 02.03.2018 um 21:47 schrieb Felix Kuehling:
>>>> On 2018-03-02 04:29 AM, Liu, Monk wrote:
>>>>> In_atomic() isnot encouraged to be used to judge if sleep is
>>>>> possible, see the macros of it
>>>>>
>>>>> #define in_atomic() (preept_count() != 0)
>>>> OK. But my point is still that you're not testing the right thing
>>>> when you check in_interrupt(). The comment before the in_atomic
>>>> macro definition states the limitations and says "do not use in driver code".
>>>> Unfortunately it doesn't suggest any alternative. I think
>>>> in_interrupt is actually worse, because it misses even more cases than in_atomic.
>>> Thinking about this, Felix seems to be absolutely right.
>>>
>>> So we need to revert this patch since you can't reliable detect in a driver if sleeping is allowed or not.
>>>
>>> Regards,
>>> Christian.
>>>
>>>> Regards,
>>>>       Felix
>>>>
>>>>> /Monk
>>>>>
>>>>> -----Original Message-----
>>>>> From: Kuehling, Felix
>>>>> Sent: 2018å¹´3æ??1æ?¥ 23:50
>>>>> To: amd-gfx at lists.freedesktop.org; Liu, Monk <Monk.Liu at amd.com>
>>>>> Subject: Re: [PATCH] drm/amdgpu: try again kiq access if not in
>>>>> IRQ(v2)
>>>>>
>>>>> On 2018-02-28 02:27 AM, Monk Liu wrote:
>>>>>> sometimes GPU is switched to other VFs and won't swich back soon,
>>>>>> so the kiq reg access will not signal within a short period,
>>>>>> instead of busy waiting a long time(MAX_KEQ_REG_WAIT) and
>>>>>> returning TMO we can istead sleep 5ms and try again later (non irq
>>>>>> context)
>>>>>>
>>>>>> And since the waiting in kiq_r/weg is busy wait, so
>>>>>> MAX_KIQ_REG_WAIT shouldn't set to a long time, set it to 10ms is more appropriate.
>>>>>>
>>>>>> if gpu already in reset state, don't retry the KIQ reg access
>>>>>> otherwise it would always hang because KIQ was already die usually.
>>>>>>
>>>>>> v2:
>>>>>> replace schedule() with msleep() for the wait
>>>>>>
>>>>>> Change-Id: I8fc807ce85a8d30d2b50153f3f3a6eda344ef994
>>>>>> Signed-off-by: Monk Liu <Monk.Liu at amd.com>
>>>>>> ---
>>>>>>      drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 15 +++++++++++++--
>>>>>>      1 file changed, 13 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>>>>>> index b832651..1672f5b 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>>>>>> @@ -22,7 +22,7 @@
>>>>>>       */
>>>>>>      
>>>>>>      #include "amdgpu.h"
>>>>>> -#define MAX_KIQ_REG_WAIT	100000000 /* in usecs */
>>>>>> +#define MAX_KIQ_REG_WAIT	10000 /* in usecs, 10ms */
>>>>>>      
>>>>>>      uint64_t amdgpu_csa_vaddr(struct amdgpu_device *adev)  { @@
>>>>>> -152,9
>>>>>> +152,14 @@ uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device
>>>>>> +*adev, uint32_t reg)
>>>>>>      	amdgpu_ring_commit(ring);
>>>>>>      	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>>>>>>      
>>>>>> +retry_read:
>>>>>>      	r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
>>>>>>      	if (r < 1) {
>>>>>>      		DRM_ERROR("wait for kiq fence error: %ld\n", r);
>>>>>> +		if (!in_interrupt() && !adev->in_gpu_reset) {
>>>>> You should check in_atomic here. Because it's invalid to sleep in atomic context (e.g. while holding a spin lock) even when not in an interrupt.
>>>>> This seems to happen a lot for indirect register access, e.g.
>>>>> soc15_pcie_rreg.
>>>>>
>>>>> Regards,
>>>>>       Felix
>>>>>
>>>>>> +			msleep(5);
>>>>>> +			goto retry_read;
>>>>>> +		}
>>>>>>      		return ~0;
>>>>>>      	}
>>>>>>      	val = adev->wb.wb[adev->virt.reg_val_offs];
>>>>>> @@ -179,9 +184,15 @@ void amdgpu_virt_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v)
>>>>>>      	amdgpu_ring_commit(ring);
>>>>>>      	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>>>>>>      
>>>>>> +retry_write:
>>>>>>      	r = amdgpu_fence_wait_polling(ring, seq, MAX_KIQ_REG_WAIT);
>>>>>> -	if (r < 1)
>>>>>> +	if (r < 1) {
>>>>>>      		DRM_ERROR("wait for kiq fence error: %ld\n", r);
>>>>>> +		if (!in_interrupt() && !adev->in_gpu_reset) {
>>>>>> +			msleep(5);
>>>>>> +			goto retry_write;
>>>>>> +		}
>>>>>> +	}
>>>>>>      }
>>>>>>      
>>>>>>      /**
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx



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

  Powered by Linux