[PATCH 1/4] drm/virtio: pass gem reservation object to ttm init

[PATCH 1/4] drm/virtio: pass gem reservation object to ttm init

[Gerd Hoffmann]

With this gem and ttm will use the same reservation object,
so mixing and matching ttm / gem reservation helpers should
work fine.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_object.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index b2da31310d24..242766d644a7 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -132,7 +132,8 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
 	virtio_gpu_init_ttm_placement(bo);
 	ret = ttm_bo_init(&vgdev->mman.bdev, &bo->tbo, params->size,
 			  ttm_bo_type_device, &bo->placement, 0,
-			  true, acc_size, NULL, NULL,
+			  true, acc_size, NULL,
+			  bo->gem_base.resv,
 			  &virtio_gpu_ttm_bo_destroy);
 	/* ttm_bo_init failure will call the destroy */
 	if (ret != 0)
-- 
2.18.1

[PATCH 2/4] drm/virtio: switch virtio_gpu_wait_ioctl() to gem helper.

[Gerd Hoffmann]

Use drm_gem_reservation_object_wait() in virtio_gpu_wait_ioctl().

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index c0ba1ead740f..e38a6bb46cc7 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -464,23 +464,13 @@ 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;
-	int ret;
-	bool nowait = false;
-
-	gobj = drm_gem_object_lookup(file, args->handle);
-	if (gobj == NULL)
-		return -ENOENT;
-
-	qobj = gem_to_virtio_gpu_obj(gobj);
+	long timeout = 15 * HZ;
 
 	if (args->flags & VIRTGPU_WAIT_NOWAIT)
-		nowait = true;
-	ret = virtio_gpu_object_wait(qobj, nowait);
+		timeout = 0;
 
-	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

[PATCH 3/4] drm/virtio: simplify cursor updates

[Gerd Hoffmann]

No need to do the reservation dance,
we can just wait on the fence directly.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_plane.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index 024c2aa0c929..4b805bf466d3 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -184,7 +184,6 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
 	struct virtio_gpu_framebuffer *vgfb;
 	struct virtio_gpu_object *bo = NULL;
 	uint32_t handle;
-	int ret = 0;
 
 	if (plane->state->crtc)
 		output = drm_crtc_to_virtio_gpu_output(plane->state->crtc);
@@ -208,15 +207,9 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
 			 cpu_to_le32(plane->state->crtc_w),
 			 cpu_to_le32(plane->state->crtc_h),
 			 0, 0, vgfb->fence);
-		ret = virtio_gpu_object_reserve(bo, false);
-		if (!ret) {
-			reservation_object_add_excl_fence(bo->tbo.resv,
-							  &vgfb->fence->f);
-			dma_fence_put(&vgfb->fence->f);
-			vgfb->fence = NULL;
-			virtio_gpu_object_unreserve(bo);
-			virtio_gpu_object_wait(bo, false);
-		}
+		dma_fence_wait(&vgfb->fence->f, true);
+		dma_fence_put(&vgfb->fence->f);
+		vgfb->fence = NULL;
 	}
 
 	if (plane->state->fb != old_state->fb) {
-- 
2.18.1

[PATCH 4/4] drm/virtio: remove virtio_gpu_object_wait

[Gerd Hoffmann]

No users left.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h    |  1 -
 drivers/gpu/drm/virtio/virtgpu_object.c | 13 -------------
 2 files changed, 14 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 9e2d3062b01d..2cd96256ba37 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -364,7 +364,6 @@ int virtio_gpu_object_kmap(struct virtio_gpu_object *bo);
 int virtio_gpu_object_get_sg_table(struct virtio_gpu_device *qdev,
 				   struct virtio_gpu_object *bo);
 void virtio_gpu_object_free_sg_table(struct virtio_gpu_object *bo);
-int virtio_gpu_object_wait(struct virtio_gpu_object *bo, bool no_wait);
 
 /* virtgpu_prime.c */
 struct sg_table *virtgpu_gem_prime_get_sg_table(struct drm_gem_object *obj);
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index 242766d644a7..82bfbf983fd2 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -233,16 +233,3 @@ void virtio_gpu_object_free_sg_table(struct virtio_gpu_object *bo)
 	kfree(bo->pages);
 	bo->pages = NULL;
 }
-
-int virtio_gpu_object_wait(struct virtio_gpu_object *bo, bool no_wait)
-{
-	int r;
-
-	r = ttm_bo_reserve(&bo->tbo, true, no_wait, NULL);
-	if (unlikely(r != 0))
-		return r;
-	r = ttm_bo_wait(&bo->tbo, true, no_wait);
-	ttm_bo_unreserve(&bo->tbo);
-	return r;
-}
-
-- 
2.18.1

Re: [PATCH 1/4] drm/virtio: pass gem reservation object to ttm init

[Daniel Vetter]

On Mon, Jun 17, 2019 at 01:14:03PM +0200, Gerd Hoffmann wrote:
> With this gem and ttm will use the same reservation object,
> so mixing and matching ttm / gem reservation helpers should
> work fine.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

While doing my prime doc+cleanup series I wondered whether we should do
this for everyone, and perhaps even remove ttm_bo.ttm_resv. Only driver
which doesn't yet have a gem_bo embedded in the same allocation is vmwgfx,
and that would be easy to fix by adding a vmwgfx_resv somehwere.

Anyway, looks like a solid start into the convergence story.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/virtio/virtgpu_object.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
> index b2da31310d24..242766d644a7 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -132,7 +132,8 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
>  	virtio_gpu_init_ttm_placement(bo);
>  	ret = ttm_bo_init(&vgdev->mman.bdev, &bo->tbo, params->size,
>  			  ttm_bo_type_device, &bo->placement, 0,
> -			  true, acc_size, NULL, NULL,
> +			  true, acc_size, NULL,
> +			  bo->gem_base.resv,
>  			  &virtio_gpu_ttm_bo_destroy);
>  	/* ttm_bo_init failure will call the destroy */
>  	if (ret != 0)
> -- 
> 2.18.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/4] drm/virtio: pass gem reservation object to ttm init

[Daniel Vetter]

On Mon, Jun 17, 2019 at 04:08:25PM +0200, Daniel Vetter wrote:
> On Mon, Jun 17, 2019 at 01:14:03PM +0200, Gerd Hoffmann wrote:
> > With this gem and ttm will use the same reservation object,
> > so mixing and matching ttm / gem reservation helpers should
> > work fine.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> While doing my prime doc+cleanup series I wondered whether we should do
> this for everyone, and perhaps even remove ttm_bo.ttm_resv. Only driver
> which doesn't yet have a gem_bo embedded in the same allocation is vmwgfx,
> and that would be easy to fix by adding a vmwgfx_resv somehwere.
> 
> Anyway, looks like a solid start into the convergence story.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Aside: if virtio ever allows dma-buf sharing with something else (or
multiple virtio-gpu instances), then together with my patch series this
will fix dma-buf import. Atm virtio ignores the reservation object of the
imported dma-buf, which for foreing objects really isn't correct.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/virtio/virtgpu_object.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
> > index b2da31310d24..242766d644a7 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> > @@ -132,7 +132,8 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
> >  	virtio_gpu_init_ttm_placement(bo);
> >  	ret = ttm_bo_init(&vgdev->mman.bdev, &bo->tbo, params->size,
> >  			  ttm_bo_type_device, &bo->placement, 0,
> > -			  true, acc_size, NULL, NULL,
> > +			  true, acc_size, NULL,
> > +			  bo->gem_base.resv,
> >  			  &virtio_gpu_ttm_bo_destroy);
> >  	/* ttm_bo_init failure will call the destroy */
> >  	if (ret != 0)
> > -- 
> > 2.18.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/4] drm/virtio: switch virtio_gpu_wait_ioctl() to gem helper.

[Daniel Vetter]

On Mon, Jun 17, 2019 at 01:14:04PM +0200, Gerd Hoffmann wrote:
> Use drm_gem_reservation_object_wait() in virtio_gpu_wait_ioctl().

Would be good to mention here that with this the wait becomes lockless, we
don't call ttm_bo_reserve/unreserve anymore.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index c0ba1ead740f..e38a6bb46cc7 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -464,23 +464,13 @@ 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;
> -	int ret;
> -	bool nowait = false;
> -
> -	gobj = drm_gem_object_lookup(file, args->handle);
> -	if (gobj == NULL)
> -		return -ENOENT;
> -
> -	qobj = gem_to_virtio_gpu_obj(gobj);
> +	long timeout = 15 * HZ;
>  
>  	if (args->flags & VIRTGPU_WAIT_NOWAIT)
> -		nowait = true;
> -	ret = virtio_gpu_object_wait(qobj, nowait);
> +		timeout = 0;

timeout of 0 gets upgrade to a 1 jiffy wait, which is probably not what
you want here. You need to open-code ttm_bo_wait here and call
reservation_object_test_signaled_rcu() for this case.

With that fixed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  
> -	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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/4] drm/virtio: simplify cursor updates

[Daniel Vetter]

On Mon, Jun 17, 2019 at 01:14:05PM +0200, Gerd Hoffmann wrote:
> No need to do the reservation dance,
> we can just wait on the fence directly.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_plane.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index 024c2aa0c929..4b805bf466d3 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -184,7 +184,6 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
>  	struct virtio_gpu_framebuffer *vgfb;
>  	struct virtio_gpu_object *bo = NULL;
>  	uint32_t handle;
> -	int ret = 0;
>  
>  	if (plane->state->crtc)
>  		output = drm_crtc_to_virtio_gpu_output(plane->state->crtc);
> @@ -208,15 +207,9 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
>  			 cpu_to_le32(plane->state->crtc_w),
>  			 cpu_to_le32(plane->state->crtc_h),
>  			 0, 0, vgfb->fence);
> -		ret = virtio_gpu_object_reserve(bo, false);
> -		if (!ret) {
> -			reservation_object_add_excl_fence(bo->tbo.resv,
> -							  &vgfb->fence->f);
> -			dma_fence_put(&vgfb->fence->f);
> -			vgfb->fence = NULL;
> -			virtio_gpu_object_unreserve(bo);
> -			virtio_gpu_object_wait(bo, false);
> -		}
> +		dma_fence_wait(&vgfb->fence->f, true);
> +		dma_fence_put(&vgfb->fence->f);
> +		vgfb->fence = NULL;

Even nicer would be to add the fence using
drm_atomic_set_fence_for_plane() in the prepare_fb hook. Assuming this
isn't necessary for correctness (but then I kinda have questions about
races and stuff).

But this gets the job done too I think, so:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  	}
>  
>  	if (plane->state->fb != old_state->fb) {
> -- 
> 2.18.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 4/4] drm/virtio: remove virtio_gpu_object_wait

[Daniel Vetter]

On Mon, Jun 17, 2019 at 01:14:06PM +0200, Gerd Hoffmann wrote:
> No users left.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/virtio/virtgpu_drv.h    |  1 -
>  drivers/gpu/drm/virtio/virtgpu_object.c | 13 -------------
>  2 files changed, 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 9e2d3062b01d..2cd96256ba37 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -364,7 +364,6 @@ int virtio_gpu_object_kmap(struct virtio_gpu_object *bo);
>  int virtio_gpu_object_get_sg_table(struct virtio_gpu_device *qdev,
>  				   struct virtio_gpu_object *bo);
>  void virtio_gpu_object_free_sg_table(struct virtio_gpu_object *bo);
> -int virtio_gpu_object_wait(struct virtio_gpu_object *bo, bool no_wait);
>  
>  /* virtgpu_prime.c */
>  struct sg_table *virtgpu_gem_prime_get_sg_table(struct drm_gem_object *obj);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
> index 242766d644a7..82bfbf983fd2 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -233,16 +233,3 @@ void virtio_gpu_object_free_sg_table(struct virtio_gpu_object *bo)
>  	kfree(bo->pages);
>  	bo->pages = NULL;
>  }
> -
> -int virtio_gpu_object_wait(struct virtio_gpu_object *bo, bool no_wait)
> -{
> -	int r;
> -
> -	r = ttm_bo_reserve(&bo->tbo, true, no_wait, NULL);
> -	if (unlikely(r != 0))
> -		return r;
> -	r = ttm_bo_wait(&bo->tbo, true, no_wait);
> -	ttm_bo_unreserve(&bo->tbo);
> -	return r;
> -}
> -
> -- 
> 2.18.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/4] drm/virtio: simplify cursor updates

[Gerd Hoffmann]

> Even nicer would be to add the fence using
> drm_atomic_set_fence_for_plane() in the prepare_fb hook. Assuming this
> isn't necessary for correctness (but then I kinda have questions about
> races and stuff).

I'll have a look.  Maybe this way I can drop struct
virtio_gpu_framebuffer (where the fence is the only
thing beside struct drm_framebuffer).

cheers,
  Gerd