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.
