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.
The problem is that all those waits can block the MM subsystem from
reclaiming memory in an out of memory situation, e.g. when the process
is terminated with a SIGKILL.
That's why the usual policy used in the drivers is to wait interruptible
unless you absolutely need the wait uninterrupted (e.g. in a cleanup
path for example).
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.
The page flip IOCTL should be restartable. I think all drm IOCTLs with
driver callbacks are restartable, but I'm not 100% sure, Michel probably
knows that better.
There is even the CONFIG_DEBUG_WW_MUTEX_SLOWPATH option to throw random
backoff cases into reserving the BO for testing. But I think that only
applies to when multiple BOs are reserved at the same time.
Might be a good idea to create something similar for all interruptible
lock acquires to test if they are really restartable. That will most
likely yield quite a bunch of cases where the handling isn't correct.
Regards,
Christian.
Am 26.04.2017 um 21:19 schrieb 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....@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.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
