On Wed, Feb 12, 2020 at 3:13 AM Gerd Hoffmann <kraxel at redhat.com>
wrote:>
> Drop the virtio_gpu_{disable,enable}_notify(). Add a new
> virtio_gpu_notify() call instead, which must be called whenever
> the driver wants make sure the host is notified needed.
>
> Drop notification from command submission. Add virtio_gpu_notify()
> calls everywhere instead. This results in more batching because we now
> notify only once for a series of commands. We already had that for page
> flips, now we also batch resource creation (create + attach-backing),
> display updates (edid + display-info) and device initialization. With
> this in place it is also possible to make notification optional for
> userspace ioctls.
Can you separate out the batching changes (create + attach-backing,
edid + display-info, etc.)? This patch can mechanically append a
virtio_gpu_notify call to every virtio_gpu_cmd_* call site. A second
patch can make the batching changes.
>
> v2:
> - rebase to latest drm-misc-next.
> - use "if (!atomic_read())".
> - add review & test tags.
>
> Signed-off-by: Gerd Hoffmann <kraxel at redhat.com>
> Reviewed-by: Gurchetan Singh <gurchetansingh at chromium.org>
> Tested-by: Gurchetan Singh <gurchetansingh at chromium.org>
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.h | 6 ++---
> drivers/gpu/drm/virtio/virtgpu_display.c | 2 ++
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 4 ++++
> drivers/gpu/drm/virtio/virtgpu_kms.c | 3 +++
> drivers/gpu/drm/virtio/virtgpu_object.c | 1 +
> drivers/gpu/drm/virtio/virtgpu_plane.c | 5 ++--
> drivers/gpu/drm/virtio/virtgpu_vq.c | 30 ++++++++++--------------
> 7 files changed, 26 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index af9403e1cf78..2f6c4ccbfd14 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -179,8 +179,7 @@ struct virtio_gpu_device {
> struct virtio_gpu_queue cursorq;
> struct kmem_cache *vbufs;
>
> - bool disable_notify;
> - bool pending_notify;
> + atomic_t pending_commands;
nit: this could be named ctrlq_pending_commands or be moved to virtio_gpu_queue.
>
> struct ida resource_ida;
>
> @@ -335,8 +334,7 @@ void virtio_gpu_dequeue_ctrl_func(struct work_struct
*work);
> void virtio_gpu_dequeue_cursor_func(struct work_struct *work);
> void virtio_gpu_dequeue_fence_func(struct work_struct *work);
>
> -void virtio_gpu_disable_notify(struct virtio_gpu_device *vgdev);
> -void virtio_gpu_enable_notify(struct virtio_gpu_device *vgdev);
> +void virtio_gpu_notify(struct virtio_gpu_device *vgdev);
>
> /* virtio_gpu_display.c */
> void virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c
b/drivers/gpu/drm/virtio/virtgpu_display.c
> index af953db4a0c9..2b7e6ae65546 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
> @@ -90,6 +90,7 @@ static void virtio_gpu_crtc_mode_set_nofb(struct drm_crtc
*crtc)
> virtio_gpu_cmd_set_scanout(vgdev, output->index, 0,
> crtc->mode.hdisplay,
> crtc->mode.vdisplay, 0, 0);
> + virtio_gpu_notify(vgdev);
> }
>
> static void virtio_gpu_crtc_atomic_enable(struct drm_crtc *crtc,
> @@ -108,6 +109,7 @@ static void virtio_gpu_crtc_atomic_disable(struct
drm_crtc *crtc,
> struct virtio_gpu_output *output =
drm_crtc_to_virtio_gpu_output(crtc);
>
> virtio_gpu_cmd_set_scanout(vgdev, output->index, 0, 0, 0, 0, 0);
> + virtio_gpu_notify(vgdev);
> output->enabled = false;
> }
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 205ec4abae2b..75d818d707e6 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -158,6 +158,7 @@ static int virtio_gpu_execbuffer_ioctl(struct
drm_device *dev, void *data,
>
> virtio_gpu_cmd_submit(vgdev, buf, exbuf->size,
> vfpriv->ctx_id, buflist, out_fence);
> + virtio_gpu_notify(vgdev);
> return 0;
>
> out_memdup:
> @@ -314,6 +315,7 @@ static int virtio_gpu_transfer_from_host_ioctl(struct
drm_device *dev,
> (vgdev, vfpriv->ctx_id, offset, args->level,
> &args->box, objs, fence);
> dma_fence_put(&fence->f);
> + virtio_gpu_notify(vgdev);
> return 0;
>
> err_unlock:
> @@ -359,6 +361,7 @@ static int virtio_gpu_transfer_to_host_ioctl(struct
drm_device *dev, void *data,
> args->level, &args->box, objs, fence);
> dma_fence_put(&fence->f);
> }
> + virtio_gpu_notify(vgdev);
> return 0;
>
> err_unlock:
> @@ -445,6 +448,7 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device
*dev,
> /* not in cache - need to talk to hw */
> virtio_gpu_cmd_get_capset(vgdev, found_valid, args->cap_set_ver,
> &cache_ent);
> + virtio_gpu_notify(vgdev);
>
> copy_exit:
> ret = wait_event_timeout(vgdev->resp_wq,
> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c
b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index 4009c2f97d08..7eabcf1ca424 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -44,6 +44,7 @@ static void virtio_gpu_config_changed_work_func(struct
work_struct *work)
> if (vgdev->has_edid)
> virtio_gpu_cmd_get_edids(vgdev);
> virtio_gpu_cmd_get_display_info(vgdev);
> + virtio_gpu_notify(vgdev);
> drm_helper_hpd_irq_event(vgdev->ddev);
> events_clear |= VIRTIO_GPU_EVENT_DISPLAY;
> }
> @@ -92,6 +93,7 @@ static void virtio_gpu_get_capsets(struct
virtio_gpu_device *vgdev,
> }
> for (i = 0; i < num_capsets; i++) {
> virtio_gpu_cmd_get_capset_info(vgdev, i);
> + virtio_gpu_notify(vgdev);
> ret = wait_event_timeout(vgdev->resp_wq,
> vgdev->capsets[i].id > 0, 5
* HZ);
> if (ret == 0) {
> @@ -205,6 +207,7 @@ int virtio_gpu_init(struct drm_device *dev)
> if (vgdev->has_edid)
> virtio_gpu_cmd_get_edids(vgdev);
> virtio_gpu_cmd_get_display_info(vgdev);
> + virtio_gpu_notify(vgdev);
> wait_event_timeout(vgdev->resp_wq,
!vgdev->display_info_pending,
> 5 * HZ);
> return 0;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c
b/drivers/gpu/drm/virtio/virtgpu_object.c
> index 8870ee23ff2b..65d6834d3c74 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -224,6 +224,7 @@ int virtio_gpu_object_create(struct virtio_gpu_device
*vgdev,
> return ret;
> }
>
> + virtio_gpu_notify(vgdev);
> *bo_ptr = bo;
> return 0;
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c
b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index ac42c84d2d7f..fd6487fb0855 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -154,8 +154,6 @@ static void virtio_gpu_primary_plane_update(struct
drm_plane *plane,
> if (!drm_atomic_helper_damage_merged(old_state, plane->state,
&rect))
> return;
>
> - virtio_gpu_disable_notify(vgdev);
> -
> bo = gem_to_virtio_gpu_obj(plane->state->fb->obj[0]);
> if (bo->dumb)
> virtio_gpu_update_dumb_bo(vgdev, plane->state,
&rect);
> @@ -187,7 +185,7 @@ static void virtio_gpu_primary_plane_update(struct
drm_plane *plane,
> rect.x2 - rect.x1,
> rect.y2 - rect.y1);
>
> - virtio_gpu_enable_notify(vgdev);
> + virtio_gpu_notify(vgdev);
> }
>
> static int virtio_gpu_cursor_prepare_fb(struct drm_plane *plane,
> @@ -265,6 +263,7 @@ static void virtio_gpu_cursor_plane_update(struct
drm_plane *plane,
> plane->state->crtc_w,
> plane->state->crtc_h,
> 0, 0, objs, vgfb->fence);
> + virtio_gpu_notify(vgdev);
> dma_fence_wait(&vgfb->fence->f, true);
> dma_fence_put(&vgfb->fence->f);
> vgfb->fence = NULL;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c
b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index cfe9c54f87a3..357fef8197dc 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -329,7 +329,6 @@ static void virtio_gpu_queue_ctrl_sgs(struct
virtio_gpu_device *vgdev,
> int incnt)
> {
> struct virtqueue *vq = vgdev->ctrlq.vq;
> - bool notify = false;
> int ret, idx;
>
> if (!drm_dev_enter(vgdev->ddev, &idx)) {
> @@ -368,16 +367,10 @@ static void virtio_gpu_queue_ctrl_sgs(struct
virtio_gpu_device *vgdev,
>
> trace_virtio_gpu_cmd_queue(vq, virtio_gpu_vbuf_ctrl_hdr(vbuf));
>
> - notify = virtqueue_kick_prepare(vq);
> + atomic_inc(&vgdev->pending_commands);
>
> spin_unlock(&vgdev->ctrlq.qlock);
>
> - if (notify) {
> - if (vgdev->disable_notify)
> - vgdev->pending_notify = true;
> - else
> - virtqueue_notify(vq);
> - }
> drm_dev_exit(idx);
> }
>
> @@ -434,19 +427,20 @@ static void
virtio_gpu_queue_fenced_ctrl_buffer(struct virtio_gpu_device *vgdev,
> }
> }
>
> -void virtio_gpu_disable_notify(struct virtio_gpu_device *vgdev)
> +void virtio_gpu_notify(struct virtio_gpu_device *vgdev)
> {
> - vgdev->disable_notify = true;
> -}
> + bool notify;
>
> -void virtio_gpu_enable_notify(struct virtio_gpu_device *vgdev)
> -{
> - vgdev->disable_notify = false;
> -
> - if (!vgdev->pending_notify)
> + if (!atomic_read(&vgdev->pending_commands))
> return;
> - vgdev->pending_notify = false;
> - virtqueue_notify(vgdev->ctrlq.vq);
> +
> + spin_lock(&vgdev->ctrlq.qlock);
> + atomic_set(&vgdev->pending_commands, 0);
> + notify = virtqueue_kick_prepare(vgdev->ctrlq.vq);
> + spin_unlock(&vgdev->ctrlq.qlock);
> +
> + if (notify)
> + virtqueue_notify(vgdev->ctrlq.vq);
> }
>
> static void virtio_gpu_queue_ctrl_buffer(struct virtio_gpu_device *vgdev,
> --
> 2.18.2
>