Re: [PATCH 1/3] drm/virtio: set seqno for dma-fence
On Mon, Apr 29, 2019 at 03:08:23PM -0700, Chia-I Wu wrote: > This is motivated by having meaningful ftrace events, but it also > fixes use cases where dma_fence_is_later is called, such as in > sync_file_merge. > > In other drivers, fence creation and cmdbuf submission normally > happen atomically, > > mutex_lock(); > fence = dma_fence_create(..., ++timeline->seqno); > submit_cmdbuf(); > mutex_unlock(); > > and have no such issue. But in our driver, because most ioctls > queue commands into ctrlq, we do not want to grab a lock. Instead, > we set seqno to 0 when a fence is created, and update it when the > command is finally queued and the seqno is known. Series pushed to drm-misc-next. thanks, Gerd ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/virtio: set seqno for dma-fence
Hi Chia-I, On Mon, 29 Apr 2019 at 23:08, Chia-I Wu wrote: > > This is motivated by having meaningful ftrace events, but it also > fixes use cases where dma_fence_is_later is called, such as in > sync_file_merge. > > In other drivers, fence creation and cmdbuf submission normally > happen atomically, > > mutex_lock(); > fence = dma_fence_create(..., ++timeline->seqno); > submit_cmdbuf(); > mutex_unlock(); > > and have no such issue. But in our driver, because most ioctls > queue commands into ctrlq, we do not want to grab a lock. Instead, > we set seqno to 0 when a fence is created, and update it when the > command is finally queued and the seqno is known. > > Signed-off-by: Chia-I Wu The series looks great. For the lot: Reviewed-by: Emil Velikov -Emil ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/virtio: set seqno for dma-fence
(Add missing CCs)
On Mon, Apr 29, 2019 at 3:08 PM Chia-I Wu wrote:
>
> This is motivated by having meaningful ftrace events, but it also
> fixes use cases where dma_fence_is_later is called, such as in
> sync_file_merge.
>
> In other drivers, fence creation and cmdbuf submission normally
> happen atomically,
>
> mutex_lock();
> fence = dma_fence_create(..., ++timeline->seqno);
> submit_cmdbuf();
> mutex_unlock();
>
> and have no such issue. But in our driver, because most ioctls
> queue commands into ctrlq, we do not want to grab a lock. Instead,
> we set seqno to 0 when a fence is created, and update it when the
> command is finally queued and the seqno is known.
>
> Signed-off-by: Chia-I Wu
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.h | 1 -
> drivers/gpu/drm/virtio/virtgpu_fence.c | 17 ++---
> 2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
> b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 491dec0712b3..90461feeafdb 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -102,7 +102,6 @@ struct virtio_gpu_fence {
> struct dma_fence f;
> struct virtio_gpu_fence_driver *drv;
> struct list_head node;
> - uint64_t seq;
> };
> #define to_virtio_fence(x) \
> container_of(x, struct virtio_gpu_fence, f)
> diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c
> b/drivers/gpu/drm/virtio/virtgpu_fence.c
> index 87d1966192f4..72b4f7561432 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_fence.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
> @@ -40,16 +40,14 @@ bool virtio_fence_signaled(struct dma_fence *f)
> {
> struct virtio_gpu_fence *fence = to_virtio_fence(f);
>
> - if (atomic64_read(>drv->last_seq) >= fence->seq)
> + if (atomic64_read(>drv->last_seq) >= fence->f.seqno)
> return true;
> return false;
> }
>
> static void virtio_fence_value_str(struct dma_fence *f, char *str, int size)
> {
> - struct virtio_gpu_fence *fence = to_virtio_fence(f);
> -
> - snprintf(str, size, "%llu", fence->seq);
> + snprintf(str, size, "%llu", f->seqno);
> }
>
> static void virtio_timeline_value_str(struct dma_fence *f, char *str, int
> size)
> @@ -76,6 +74,11 @@ struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct
> virtio_gpu_device *vgdev)
> return fence;
>
> fence->drv = drv;
> +
> + /* This only partially initializes the fence because the seqno is
> +* unknown yet. The fence must not be used outside of the driver
> +* until virtio_gpu_fence_emit is called.
> +*/
> dma_fence_init(>f, _fence_ops, >lock,
> drv->context, 0);
>
> return fence;
> @@ -89,13 +92,13 @@ int virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
> unsigned long irq_flags;
>
> spin_lock_irqsave(>lock, irq_flags);
> - fence->seq = ++drv->sync_seq;
> + fence->f.seqno = ++drv->sync_seq;
> dma_fence_get(>f);
> list_add_tail(>node, >fences);
> spin_unlock_irqrestore(>lock, irq_flags);
>
> cmd_hdr->flags |= cpu_to_le32(VIRTIO_GPU_FLAG_FENCE);
> - cmd_hdr->fence_id = cpu_to_le64(fence->seq);
> + cmd_hdr->fence_id = cpu_to_le64(fence->f.seqno);
> return 0;
> }
>
> @@ -109,7 +112,7 @@ void virtio_gpu_fence_event_process(struct
> virtio_gpu_device *vgdev,
> spin_lock_irqsave(>lock, irq_flags);
> atomic64_set(>fence_drv.last_seq, last_seq);
> list_for_each_entry_safe(fence, tmp, >fences, node) {
> - if (last_seq < fence->seq)
> + if (last_seq < fence->f.seqno)
> continue;
> dma_fence_signal_locked(>f);
> list_del(>node);
> --
> 2.21.0.593.g511ec345e18-goog
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] drm/virtio: set seqno for dma-fence
This is motivated by having meaningful ftrace events, but it also
fixes use cases where dma_fence_is_later is called, such as in
sync_file_merge.
In other drivers, fence creation and cmdbuf submission normally
happen atomically,
mutex_lock();
fence = dma_fence_create(..., ++timeline->seqno);
submit_cmdbuf();
mutex_unlock();
and have no such issue. But in our driver, because most ioctls
queue commands into ctrlq, we do not want to grab a lock. Instead,
we set seqno to 0 when a fence is created, and update it when the
command is finally queued and the seqno is known.
Signed-off-by: Chia-I Wu
---
drivers/gpu/drm/virtio/virtgpu_drv.h | 1 -
drivers/gpu/drm/virtio/virtgpu_fence.c | 17 ++---
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 491dec0712b3..90461feeafdb 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -102,7 +102,6 @@ struct virtio_gpu_fence {
struct dma_fence f;
struct virtio_gpu_fence_driver *drv;
struct list_head node;
- uint64_t seq;
};
#define to_virtio_fence(x) \
container_of(x, struct virtio_gpu_fence, f)
diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c
b/drivers/gpu/drm/virtio/virtgpu_fence.c
index 87d1966192f4..72b4f7561432 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -40,16 +40,14 @@ bool virtio_fence_signaled(struct dma_fence *f)
{
struct virtio_gpu_fence *fence = to_virtio_fence(f);
- if (atomic64_read(>drv->last_seq) >= fence->seq)
+ if (atomic64_read(>drv->last_seq) >= fence->f.seqno)
return true;
return false;
}
static void virtio_fence_value_str(struct dma_fence *f, char *str, int size)
{
- struct virtio_gpu_fence *fence = to_virtio_fence(f);
-
- snprintf(str, size, "%llu", fence->seq);
+ snprintf(str, size, "%llu", f->seqno);
}
static void virtio_timeline_value_str(struct dma_fence *f, char *str, int size)
@@ -76,6 +74,11 @@ struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct
virtio_gpu_device *vgdev)
return fence;
fence->drv = drv;
+
+ /* This only partially initializes the fence because the seqno is
+* unknown yet. The fence must not be used outside of the driver
+* until virtio_gpu_fence_emit is called.
+*/
dma_fence_init(>f, _fence_ops, >lock, drv->context,
0);
return fence;
@@ -89,13 +92,13 @@ int virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
unsigned long irq_flags;
spin_lock_irqsave(>lock, irq_flags);
- fence->seq = ++drv->sync_seq;
+ fence->f.seqno = ++drv->sync_seq;
dma_fence_get(>f);
list_add_tail(>node, >fences);
spin_unlock_irqrestore(>lock, irq_flags);
cmd_hdr->flags |= cpu_to_le32(VIRTIO_GPU_FLAG_FENCE);
- cmd_hdr->fence_id = cpu_to_le64(fence->seq);
+ cmd_hdr->fence_id = cpu_to_le64(fence->f.seqno);
return 0;
}
@@ -109,7 +112,7 @@ void virtio_gpu_fence_event_process(struct
virtio_gpu_device *vgdev,
spin_lock_irqsave(>lock, irq_flags);
atomic64_set(>fence_drv.last_seq, last_seq);
list_for_each_entry_safe(fence, tmp, >fences, node) {
- if (last_seq < fence->seq)
+ if (last_seq < fence->f.seqno)
continue;
dma_fence_signal_locked(>f);
list_del(>node);
--
2.21.0.593.g511ec345e18-goog
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
