RE: [PATCH v2 2/2] drm/virtio: fence created per cursor/plane update

["Kasireddy, Vivek" <vivek.kasireddy@intel.com>]

Hi DW,

> Subject: [PATCH v2 2/2] drm/virtio: fence created per cursor/plane update
> 
> Having one fence for a vgfb would cause conflict in case there are
> multiple planes referencing the same vgfb (e.g. Xorg screen covering
> two displays in extended mode) being flushed simultaneously. So it makes
> sence to use a separated fence for each plane update to prevent this.
> 
> vgfb->fence is not required anymore with the suggested code change so
> both prepare_fb and cleanup_fb are removed since only fence creation/
> freeing are done in there.
> 
> v2: - use the fence always as long as guest_blob is enabled on the
>       scanout object
>     - obj and fence initialized as NULL ptrs to avoid uninitialzed
>       ptr problem (Reported by Dan Carpenter/kernel-test-robot)
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Gurchetan Singh <gurchetansingh@chromium.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_drv.h   |   1 -
>  drivers/gpu/drm/virtio/virtgpu_plane.c | 103 ++++++++++---------------
>  2 files changed, 39 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 0a194aaad419..4c59c1e67ca5 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -186,7 +186,6 @@ struct virtio_gpu_output {
> 
>  struct virtio_gpu_framebuffer {
>  	struct drm_framebuffer base;
> -	struct virtio_gpu_fence *fence;
>  };
>  #define to_virtio_gpu_framebuffer(x) \
>  	container_of(x, struct virtio_gpu_framebuffer, base)
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index 6d3cc9e238a4..821023b7d57d 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -137,29 +137,37 @@ static void virtio_gpu_resource_flush(struct drm_plane *plane,
>  	struct virtio_gpu_device *vgdev = dev->dev_private;
>  	struct virtio_gpu_framebuffer *vgfb;
>  	struct virtio_gpu_object *bo;
> +	struct virtio_gpu_object_array *objs = NULL;
> +	struct virtio_gpu_fence *fence = NULL;
> 
>  	vgfb = to_virtio_gpu_framebuffer(plane->state->fb);
>  	bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
> -	if (vgfb->fence) {
> -		struct virtio_gpu_object_array *objs;
> 
> +	if (!bo)
> +		return;
[Kasireddy, Vivek] I think you can drop the above check as bo is guaranteed
to be valid in resource_flush as the necessary checks are already done early
in virtio_gpu_primary_plane_update().

> +
> +	if (bo->dumb && bo->guest_blob)
> +		fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context,
> +					       0);
> +
> +	if (fence) {
>  		objs = virtio_gpu_array_alloc(1);
> -		if (!objs)
> +		if (!objs) {
> +			kfree(fence);
>  			return;
> +		}
>  		virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
>  		virtio_gpu_array_lock_resv(objs);
> -		virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle, x, y,
> -					      width, height, objs, vgfb->fence);
> -		virtio_gpu_notify(vgdev);
> +	}
> +
> +	virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle, x, y,
> +				      width, height, objs, fence);
> +	virtio_gpu_notify(vgdev);
[Kasireddy, Vivek] I think it is OK to retain the existing style where all the
statements relevant for if (fence) would be lumped together. I do understand that
the above two statements would be redundant in that case but it looks a bit cleaner.

> 
> -		dma_fence_wait_timeout(&vgfb->fence->f, true,
> +	if (fence) {
> +		dma_fence_wait_timeout(&fence->f, true,
>  				       msecs_to_jiffies(50));
> -		dma_fence_put(&vgfb->fence->f);
> -		vgfb->fence = NULL;
> -	} else {
> -		virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle, x, y,
> -					      width, height, NULL, NULL);
> -		virtio_gpu_notify(vgdev);
> +		dma_fence_put(&fence->f);
>  	}
>  }
> 
> @@ -239,47 +247,6 @@ static void virtio_gpu_primary_plane_update(struct drm_plane
> *plane,
>  				  rect.y2 - rect.y1);
>  }
> 
> -static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
> -				       struct drm_plane_state *new_state)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct virtio_gpu_device *vgdev = dev->dev_private;
> -	struct virtio_gpu_framebuffer *vgfb;
> -	struct virtio_gpu_object *bo;
> -
> -	if (!new_state->fb)
> -		return 0;
> -
> -	vgfb = to_virtio_gpu_framebuffer(new_state->fb);
> -	bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
> -	if (!bo || (plane->type == DRM_PLANE_TYPE_PRIMARY && !bo->guest_blob))
> -		return 0;
> -
> -	if (bo->dumb && (plane->state->fb != new_state->fb)) {
> -		vgfb->fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context,
> -						     0);
> -		if (!vgfb->fence)
> -			return -ENOMEM;
> -	}
> -
> -	return 0;
> -}
> -
> -static void virtio_gpu_plane_cleanup_fb(struct drm_plane *plane,
> -					struct drm_plane_state *old_state)
> -{
> -	struct virtio_gpu_framebuffer *vgfb;
> -
> -	if (!plane->state->fb)
> -		return;
> -
> -	vgfb = to_virtio_gpu_framebuffer(plane->state->fb);
> -	if (vgfb->fence) {
> -		dma_fence_put(&vgfb->fence->f);
> -		vgfb->fence = NULL;
> -	}
> -}
> -
>  static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
>  					   struct drm_atomic_state *state)
>  {
> @@ -290,6 +257,8 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane
> *plane,
>  	struct virtio_gpu_output *output = NULL;
>  	struct virtio_gpu_framebuffer *vgfb;
>  	struct virtio_gpu_object *bo = NULL;
> +	struct virtio_gpu_object_array *objs = NULL;
> +	struct virtio_gpu_fence *fence = NULL;
>  	uint32_t handle;
> 
>  	if (plane->state->crtc)
> @@ -309,22 +278,32 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane
> *plane,
> 
>  	if (bo && bo->dumb && (plane->state->fb != old_state->fb)) {
>  		/* new cursor -- update & wait */
> -		struct virtio_gpu_object_array *objs;
> +		fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context,
> +					       0);
> +		if (!fence)
> +			return;
> 
>  		objs = virtio_gpu_array_alloc(1);
> -		if (!objs)
> +		if (!objs) {
> +			if (fence)
[Kasireddy, Vivek] I think you can drop the above check given that you checked it
earlier.

> +				kfree(fence);
> +
>  			return;
> +		}
> +
>  		virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
>  		virtio_gpu_array_lock_resv(objs);
>  		virtio_gpu_cmd_transfer_to_host_2d
>  			(vgdev, 0,
>  			 plane->state->crtc_w,
>  			 plane->state->crtc_h,
> -			 0, 0, objs, vgfb->fence);
> +			 0, 0, objs, fence);
>  		virtio_gpu_notify(vgdev);
> -		dma_fence_wait(&vgfb->fence->f, true);
> -		dma_fence_put(&vgfb->fence->f);
> -		vgfb->fence = NULL;
> +
> +		if (fence) {
[Kasireddy, Vivek] Same as above; i.e, you can drop the if (fence) check as we
wouldn't get here without a valid fence.

I think with the above changes, the diff may get smaller and simpler. Regardless,
this patch is Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com>

> +			dma_fence_wait(&fence->f, true);
> +			dma_fence_put(&fence->f);
> +		}
>  	}
> 
>  	if (plane->state->fb != old_state->fb) {
> @@ -358,15 +337,11 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane
> *plane,
>  }
> 
>  static const struct drm_plane_helper_funcs virtio_gpu_primary_helper_funcs = {
> -	.prepare_fb		= virtio_gpu_plane_prepare_fb,
> -	.cleanup_fb		= virtio_gpu_plane_cleanup_fb,
>  	.atomic_check		= virtio_gpu_plane_atomic_check,
>  	.atomic_update		= virtio_gpu_primary_plane_update,
>  };
> 
>  static const struct drm_plane_helper_funcs virtio_gpu_cursor_helper_funcs = {
> -	.prepare_fb		= virtio_gpu_plane_prepare_fb,
> -	.cleanup_fb		= virtio_gpu_plane_cleanup_fb,
>  	.atomic_check		= virtio_gpu_plane_atomic_check,
>  	.atomic_update		= virtio_gpu_cursor_plane_update,
>  };
> --
> 2.20.1