2020-02-06 11:44:07

by Gerd Hoffmann

Subject: [PATCH] drm/virtio: fix ring free check

If the virtio device supports indirect ring descriptors we need only one
ring entry for the whole command. Take that into account when checking
whenever the virtqueue has enough free entries for our command.

Signed-off-by: Gerd Hoffmann <[email protected]>
---
drivers/gpu/drm/virtio/virtgpu_vq.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 41e475fbd67b..a2ec09dba530 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -328,7 +328,8 @@ static void virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
{
struct virtqueue *vq = vgdev->ctrlq.vq;
bool notify = false;
- int ret;
+ bool indirect;
+ int vqcnt, ret;

again:
spin_lock(&vgdev->ctrlq.qlock);
@@ -341,9 +342,11 @@ static void virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
return;
}

- if (vq->num_free < elemcnt) {
+ indirect = virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC);
+ vqcnt = indirect ? 1 : elemcnt;
+ if (vq->num_free < vqcnt) {
spin_unlock(&vgdev->ctrlq.qlock);
- wait_event(vgdev->ctrlq.ack_queue, vq->num_free >= elemcnt);
+ wait_event(vgdev->ctrlq.ack_queue, vq->num_free >= vqcnt);
goto again;
}

--
2.18.1

2020-02-06 18:29:42

by Chia-I Wu

Subject: Re: [PATCH] drm/virtio: fix ring free check

On Thu, Feb 6, 2020 at 3:14 AM Gerd Hoffmann <[email protected]> wrote:
>
> If the virtio device supports indirect ring descriptors we need only one
> ring entry for the whole command. Take that into account when checking
> whenever the virtqueue has enough free entries for our command.
>
> Signed-off-by: Gerd Hoffmann <[email protected]>
> ---
> drivers/gpu/drm/virtio/virtgpu_vq.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index 41e475fbd67b..a2ec09dba530 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -328,7 +328,8 @@ static void virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
> {
> struct virtqueue *vq = vgdev->ctrlq.vq;
> bool notify = false;
> - int ret;
> + bool indirect;
> + int vqcnt, ret;
>
> again:
> spin_lock(&vgdev->ctrlq.qlock);
> @@ -341,9 +342,11 @@ static void virtio_gpu_queue_ctrl_sgs(struct virtio_gpu_device *vgdev,
> return;
> }
>
> - if (vq->num_free < elemcnt) {
> + indirect = virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC);
> + vqcnt = indirect ? 1 : elemcnt;
Is the feature dynamic and require the lock held? If not, the result
can be cached and the fixup can happen before grabbing the lock

if (vgdev->has_indirect_desc)
elemcnt = 1;

Either way, patch is

Reviewed-by: Chia-I Wu <[email protected]>


> + if (vq->num_free < vqcnt) {
> spin_unlock(&vgdev->ctrlq.qlock);
> - wait_event(vgdev->ctrlq.ack_queue, vq->num_free >= elemcnt);
> + wait_event(vgdev->ctrlq.ack_queue, vq->num_free >= vqcnt);
> goto again;
> }
>
> --
> 2.18.1
>

2020-02-07 06:47:47

by Gerd Hoffmann

Subject: Re: [PATCH] drm/virtio: fix ring free check

Hi,

> > + indirect = virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC);
> > + vqcnt = indirect ? 1 : elemcnt;
> Is the feature dynamic and require the lock held? If not, the result
> can be cached and the fixup can happen before grabbing the lock

Not dynamic, so yes, caching makes sense.

cheers,
Gerd