From: Chia-I Wu <olvaffe@gmail.com>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: David Airlie <airlied@linux.ie>,
open list <linux-kernel@vger.kernel.org>,
ML dri-devel <dri-devel@lists.freedesktop.org>,
"open list:VIRTIO GPU DRIVER"
<virtualization@lists.linux-foundation.org>
Subject: Re: [PATCH v4 08/12] drm/virtio: rework virtio_gpu_execbuffer_ioctl fencing
Date: Sun, 30 Jun 2019 11:47:53 -0700 [thread overview]
Message-ID: <CAPaKu7Qvxp1poDoKz5Rft1uG0-7ijtTJQ-sLibvtewdZyVjXug@mail.gmail.com> (raw)
In-Reply-To: <20190628103412.f2n7ybp3qtxbhdk4@sirius.home.kraxel.org>
On Fri, Jun 28, 2019 at 3:34 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> 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.
Fair point. We now loop the list twice and I was just looking for
chances for micro-optimizations.
>
> 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.
vbuf is created with kmem_cache_zalloc which always zeros the struct.
>
> cheers,
> Gerd
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-06-30 18:48 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-20 6:07 [PATCH v4 00/12] drm/virtio: switch from ttm to gem shmem helpers Gerd Hoffmann
2019-06-20 6:07 ` [PATCH v4 01/12] drm/virtio: pass gem reservation object to ttm init Gerd Hoffmann
2019-06-20 6:07 ` [PATCH v4 02/12] drm/virtio: switch virtio_gpu_wait_ioctl() to gem helper Gerd Hoffmann
2019-06-26 23:55 ` Chia-I Wu
2019-06-28 10:05 ` Gerd Hoffmann
2019-06-30 18:56 ` Chia-I Wu
2019-06-20 6:07 ` [PATCH v4 03/12] drm/virtio: simplify cursor updates Gerd Hoffmann
2019-06-20 6:07 ` [PATCH v4 04/12] drm/virtio: remove virtio_gpu_object_wait Gerd Hoffmann
2019-06-20 6:07 ` [PATCH v4 05/12] drm/virtio: drop no_wait argument from virtio_gpu_object_reserve Gerd Hoffmann
2019-06-20 6:07 ` [PATCH v4 06/12] drm/virtio: remove ttm calls from in virtio_gpu_object_{reserve, unreserve} Gerd Hoffmann
2019-06-20 6:07 ` [PATCH v4 07/12] drm/virtio: add virtio_gpu_object_array & helpers Gerd Hoffmann
2019-06-27 0:49 ` Chia-I Wu
2019-06-20 6:07 ` [PATCH v4 08/12] drm/virtio: rework virtio_gpu_execbuffer_ioctl fencing Gerd Hoffmann
2019-06-27 1:32 ` Chia-I Wu
2019-06-28 10:34 ` Gerd Hoffmann
2019-06-30 18:47 ` Chia-I Wu [this message]
2019-06-20 6:07 ` [PATCH v4 09/12] drm/virtio: rework virtio_gpu_object_create fencing Gerd Hoffmann
2019-06-27 1:44 ` Chia-I Wu
2019-06-20 6:07 ` [PATCH v4 10/12] drm/virtio: drop virtio_gpu_object_list_validate/virtio_gpu_unref_list Gerd Hoffmann
2019-06-20 6:07 ` [PATCH v4 11/12] drm/virtio: switch from ttm to gem shmem helpers Gerd Hoffmann
2019-06-27 4:47 ` Chia-I Wu
2019-06-28 10:49 ` Gerd Hoffmann
2019-06-30 18:42 ` Chia-I Wu
2019-06-20 6:07 ` [PATCH v4 12/12] drm/virtio: remove virtio_gpu_alloc_object Gerd Hoffmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPaKu7Qvxp1poDoKz5Rft1uG0-7ijtTJQ-sLibvtewdZyVjXug@mail.gmail.com \
--to=olvaffe@gmail.com \
--cc=airlied@linux.ie \
--cc=dri-devel@lists.freedesktop.org \
--cc=kraxel@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).