Use drm_gem_reservation_object_wait() in virtio_gpu_wait_ioctl().
This also makes the ioctl run lockless.
v2: use reservation_object_test_signaled_rcu for VIRTGPU_WAIT_NOWAIT.
Signed-off-by: Gerd Hoffmann <[email protected]>
Reviewed-by: Daniel Vetter <[email protected]>
---
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index ac60be9b5c19..313c770ea2c5 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -464,23 +464,19 @@ static int virtio_gpu_wait_ioctl(struct drm_device *dev, void *data,
struct drm_file *file)
{
struct drm_virtgpu_3d_wait *args = data;
- struct drm_gem_object *gobj = NULL;
- struct virtio_gpu_object *qobj = NULL;
+ struct drm_gem_object *obj;
+ long timeout = 15 * HZ;
int ret;
- bool nowait = false;
- gobj = drm_gem_object_lookup(file, args->handle);
- if (gobj == NULL)
- return -ENOENT;
+ if (args->flags & VIRTGPU_WAIT_NOWAIT) {
+ obj = drm_gem_object_lookup(file, args->handle);
+ ret = reservation_object_test_signaled_rcu(obj->resv, true);
+ drm_gem_object_put_unlocked(obj);
+ return ret ? 0 : -EBUSY;
+ }
- qobj = gem_to_virtio_gpu_obj(gobj);
-
- if (args->flags & VIRTGPU_WAIT_NOWAIT)
- nowait = true;
- ret = virtio_gpu_object_wait(qobj, nowait);
-
- drm_gem_object_put_unlocked(gobj);
- return ret;
+ return drm_gem_reservation_object_wait(file, args->handle,
+ true, timeout);
}
static int virtio_gpu_get_caps_ioctl(struct drm_device *dev,
--
2.18.1
On Wed, Jun 19, 2019 at 11:07 PM Gerd Hoffmann <[email protected]> wrote:
>
> Use drm_gem_reservation_object_wait() in virtio_gpu_wait_ioctl().
> This also makes the ioctl run lockless.
The userspace has a BO cache to avoid freeing BOs immediately but to
reuse them on next allocations. The BO cache checks if a BO is busy
before reuse, and I am seeing a big negative perf impact because of
slow virtio_gpu_wait_ioctl. I wonder if this helps.
>
> v2: use reservation_object_test_signaled_rcu for VIRTGPU_WAIT_NOWAIT.
>
> Signed-off-by: Gerd Hoffmann <[email protected]>
> Reviewed-by: Daniel Vetter <[email protected]>
> ---
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 24 ++++++++++--------------
> 1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index ac60be9b5c19..313c770ea2c5 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -464,23 +464,19 @@ static int virtio_gpu_wait_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file)
> {
> struct drm_virtgpu_3d_wait *args = data;
> - struct drm_gem_object *gobj = NULL;
> - struct virtio_gpu_object *qobj = NULL;
> + struct drm_gem_object *obj;
> + long timeout = 15 * HZ;
> int ret;
> - bool nowait = false;
>
> - gobj = drm_gem_object_lookup(file, args->handle);
> - if (gobj == NULL)
> - return -ENOENT;
> + if (args->flags & VIRTGPU_WAIT_NOWAIT) {
> + obj = drm_gem_object_lookup(file, args->handle);
Don't we need a NULL check here?
> + ret = reservation_object_test_signaled_rcu(obj->resv, true);
> + drm_gem_object_put_unlocked(obj);
> + return ret ? 0 : -EBUSY;
> + }
>
> - qobj = gem_to_virtio_gpu_obj(gobj);
> -
> - if (args->flags & VIRTGPU_WAIT_NOWAIT)
> - nowait = true;
> - ret = virtio_gpu_object_wait(qobj, nowait);
> -
> - drm_gem_object_put_unlocked(gobj);
> - return ret;
> + return drm_gem_reservation_object_wait(file, args->handle,
> + true, timeout);
> }
>
> static int virtio_gpu_get_caps_ioctl(struct drm_device *dev,
> --
> 2.18.1
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Jun 26, 2019 at 04:55:20PM -0700, Chia-I Wu wrote:
> On Wed, Jun 19, 2019 at 11:07 PM Gerd Hoffmann <[email protected]> wrote:
> >
> > Use drm_gem_reservation_object_wait() in virtio_gpu_wait_ioctl().
> > This also makes the ioctl run lockless.
> The userspace has a BO cache to avoid freeing BOs immediately but to
> reuse them on next allocations. The BO cache checks if a BO is busy
> before reuse, and I am seeing a big negative perf impact because of
> slow virtio_gpu_wait_ioctl. I wonder if this helps.
Could help indeed (assuming it checks with NOWAIT).
How many objects does userspace check in one go typically? Maybe it
makes sense to add an ioctl which checks a list, to reduce the system
call overhead.
> > + if (args->flags & VIRTGPU_WAIT_NOWAIT) {
> > + obj = drm_gem_object_lookup(file, args->handle);
> Don't we need a NULL check here?
Yes, we do. Will fix.
thanks,
Gerd
On Fri, Jun 28, 2019 at 3:05 AM Gerd Hoffmann <[email protected]> wrote:
>
> On Wed, Jun 26, 2019 at 04:55:20PM -0700, Chia-I Wu wrote:
> > On Wed, Jun 19, 2019 at 11:07 PM Gerd Hoffmann <[email protected]> wrote:
> > >
> > > Use drm_gem_reservation_object_wait() in virtio_gpu_wait_ioctl().
> > > This also makes the ioctl run lockless.
> > The userspace has a BO cache to avoid freeing BOs immediately but to
> > reuse them on next allocations. The BO cache checks if a BO is busy
> > before reuse, and I am seeing a big negative perf impact because of
> > slow virtio_gpu_wait_ioctl. I wonder if this helps.
>
> Could help indeed (assuming it checks with NOWAIT).
Yeah, that is the case.
>
> How many objects does userspace check in one go typically? Maybe it
> makes sense to add an ioctl which checks a list, to reduce the system
> call overhead.
One. The cache sorts BOs by the time they are freed, and checks only
the first (compatible) BO. If it is idle, cache hit. Otherwise,
cache miss. A new ioctl probably won't help.
>
> > > + if (args->flags & VIRTGPU_WAIT_NOWAIT) {
> > > + obj = drm_gem_object_lookup(file, args->handle);
> > Don't we need a NULL check here?
>
> Yes, we do. Will fix.
>
> thanks,
> Gerd
>