On 3/23/23 00:25, Rob Clark wrote:
...
>> +static int virtio_gpu_dma_fence_wait(struct virtio_gpu_submit *submit,
>> + struct dma_fence *fence)
>> +{
>> + struct dma_fence *itr;
>> + int idx, err;
>> +
>> + dma_fence_array_for_each(itr, idx, fence) {
>
> I guess unwrapping is for the later step of host waits?
>
> At any rate, I think you should use dma_fence_unwrap_for_each() to
> handle the fence-chain case as well?
Yes, seems so. I actually missed the dma_fence_unwrap, thanks!
...
>> +static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
>> + struct drm_virtgpu_execbuffer *exbuf,
>> + struct drm_device *dev,
>> + struct drm_file *file,
>> + uint64_t fence_ctx, uint32_t ring_idx)
>> +{
>> + struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
>> + struct virtio_gpu_device *vgdev = dev->dev_private;
>> + struct virtio_gpu_fence *out_fence;
>> + int err;
>> +
>> + memset(submit, 0, sizeof(*submit));
>> +
>> + out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
>> + if (!out_fence)
>> + return -ENOMEM;
>> +
>> + err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
>> + if (err) {
>> + dma_fence_put(&out_fence->f);
>> + return err;
>> + }
>
> If we fail at any point after here, where is the out_fence referenced dropped?
Good catch, don't see either where it's dropped. Perhaps the drop got
lost after moving the code around, will fix.
...
>> +/*
>> + * Usage of execbuffer:
>> + * Relocations need to take into account the full VIRTIO_GPUDrawable size.
>> + * However, the command as passed from user space must *not* contain the initial
>> + * VIRTIO_GPUReleaseInfo struct (first XXX bytes)
>> + */
>
> I know this is just getting moved from the old location, but I'm not
> even sure what this comment means ;-)
>
> At least it doesn't make any sense for non-virgl contexts.. I haven't
> looked too closely at virgl protocol itself
Had exactly the same thought :) Well, if nobody will clarify, then I'm
happy with removing it in v3.
--
Best regards,
Dmitry
