[PATCH 2/2] drm/amdgpu: Fix use of interruptible waiting

[Alex Xie]

Hi,

I knew this is part of ioctl. And I intended to fix this interruptible 
waiting in an ioctl.

ioctl is one of driver interfaces, where interruptible waiting is good 
in some scenarios. For example, if ioctl performs IO operations in 
atomic way, we don't want ioctl to be interrupted by a signal.

Interruptible waiting is good when we need to wait for longer time, for 
example, waiting for an input data for indefinite time. We don't want 
the process not responding to signals. Interruptible waitings can be 
good in read/write/ioctl interfaces.

For this patch,

1. The wait is short. There is not much benefit by interruptible 
waiting.  The BO is a frame buffer BO. Are there many competitors to 
lock this BO? If not, the wait is very short. This is my main reason to 
change.
2. In this function and caller functions, the error handling for such 
interrupt is complicated and risky. When the waiting is interrupted by a 
signal, the callers of this function need to handle this interrupt 
error. I traced the calling stack all the way back to function 
drm_mode_page_flip_ioctl. I could not find an issue. But I am not sure 
about even upper layer. Is this IOCTL restartable? How about further 
higher layer? Since I didn't see benefit in point 1. So I thought it was 
a good idea to change.

Anyway, it is up to you. A change might bring other unseen risk. If you 
still have concern, I will drop this patch. This IOCTL is used by 
graphic operation. There may not be a lot of signals to a GUI process 
which uses this IOCTL.

Thanks,
Alex Bin


On 17-04-26 04:34 AM, Christian König wrote:
> Am 26.04.2017 um 03:17 schrieb Michel Dänzer:
>> On 26/04/17 06:25 AM, Alex Xie wrote:
>>> 1. The wait is short. There is not much benefit by
>>> interruptible waiting.
>>> 2. In this function and caller functions, the error
>>> handling for such interrupt is complicated and risky.
>>>
>>> Change-Id: I289674ecd3f5ef20c93fe63e33df6d668b3c2edc
>>> Signed-off-by: Alex Xie <AlexBin.Xie at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> index 43082bf..c006cc4 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> @@ -227,7 +227,7 @@ int amdgpu_crtc_prepare_flip(
>>>       new_abo = gem_to_amdgpu_bo(obj);
>>>         /* pin the new buffer */
>>> -    r = amdgpu_bo_reserve(new_abo, false);
>>> +    r = amdgpu_bo_reserve(new_abo, true);
>>>       if (unlikely(r != 0)) {
>>>           DRM_ERROR("failed to reserve new abo buffer before flip\n");
>>>           goto cleanup;
>>>
>> I'm afraid we have no choice but to use interruptible here, because this
>> code runs as part of an ioctl (DRM_IOCTL_MODE_PAGE_FLIP).
>
> Yes, agree. But the error message is incorrect here and should be 
> removed.
>
> Christian.