Hi,
> > --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> > @@ -120,9 +120,9 @@ struct virtio_gpu_vbuffer {
> >
> > char *resp_buf;
> > int resp_size;
> > -
> > virtio_gpu_resp_cb resp_cb;
> >
> > + struct virtio_gpu_object_array *objs;
> This can use a comment (e.g., objects referenced by the vbuffer)
IMHO this is obvious ...
> > void virtio_gpu_cmd_submit(struct virtio_gpu_device *vgdev,
> > void *data, uint32_t data_size,
> > - uint32_t ctx_id, struct virtio_gpu_fence *fence);
> > + uint32_t ctx_id, struct virtio_gpu_fence *fence,
> > + struct virtio_gpu_object_array *objs);
> Can we keep fence, which is updated, as the last parameter?
Fixed.
> > + if (buflist) {
> > + for (i = 0; i < exbuf->num_bo_handles; i++)
> > + reservation_object_add_excl_fence(buflist->objs[i]->resv,
> > + &out_fence->f);
> > + drm_gem_unlock_reservations(buflist->objs, buflist->nents,
> > + &ticket);
> > + }
> We used to unlock after virtio_gpu_cmd_submit.
>
> I guess, the fence is considered signaled (because its seqno is still
> 0) until after virtio_gpu_cmd_submit. We probably don't want other
> processes to see the semi-initialized fence.
Good point. Fixed.
> > out_memdup:
> > kfree(buf);
> > out_unresv:
> > - ttm_eu_backoff_reservation(&ticket, &validate_list);
> > -out_free:
> > - virtio_gpu_unref_list(&validate_list);
> Keeping out_free to free buflist seems just fine.
We don't need the separate label though ...
> > + drm_gem_unlock_reservations(buflist->objs, buflist->nents, &ticket);
> > out_unused_fd:
> > kvfree(bo_handles);
> > - kvfree(buflist);
> > + if (buflist)
> > + virtio_gpu_array_put_free(buflist);
... and the buflist is released here if needed.
But we need if (buflist) for drm_gem_unlock_reservations too. Fixed.
> > -
> > - list_del(&entry->list);
> > - free_vbuf(vgdev, entry);
> > }
> > wake_up(&vgdev->ctrlq.ack_queue);
> >
> > if (fence_id)
> > virtio_gpu_fence_event_process(vgdev, fence_id);
> > +
> > + list_for_each_entry_safe(entry, tmp, &reclaim_list, list) {
> > + if (entry->objs)
> > + virtio_gpu_array_put_free(entry->objs);
> > + list_del(&entry->list);
> We are clearing the list. I guess list_del is not needed.
> > + free_vbuf(vgdev, entry);
This just shuffles around the code. Dropping list_del() is unrelated
and should be a separate patch.
Beside that I'm not sure it actually can be dropped. free_vbuf() will
not kfree() the vbuf but keep it cached in a freelist instead.
cheers,
Gerd
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
