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 <l...@intel.com>
> Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
> Cc: Gurchetan Singh <gurchetansi...@chromium.org>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> Cc: Vivek Kasireddy <vivek.kasire...@intel.com>
> Signed-off-by: Dongwon Kim <dongwon....@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.kasire...@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
