On Wed, Apr 3, 2019 at 8:17 PM Dave Airlie <airl...@gmail.com> wrote:
> On Thu, 4 Apr 2019 at 06:54, Chia-I Wu <olva...@gmail.com> wrote:
> >
> > You could end up having two virgl_hw_res with two different GEM handles
> pointing to the same kernel GEM object. That might break some assumptions
> about dependency tracking.
> >
> > For example, when the cmdbuf being built uses a buffer and you want to
> transfer some more data into the buffer, you normally need to submit the
> cmdbuf first before starting the transfer. The current code detects that
> with virgl_drm_res_is_ref, which assumes each kernel GEM object has a
> unique virgl_hw_res.
> >
> > On Mon, Apr 1, 2019 at 12:37 PM Lepton Wu <lep...@chromium.org> wrote:
> >>
> >>
> >>
> >>
> >> On Wed, Mar 20, 2019 at 3:03 PM Chia-I Wu <olva...@gmail.com> wrote:
> >>>
> >>> On Mon, Mar 18, 2019 at 2:22 PM Lepton Wu <lep...@chromium.org> wrote:
> >>>>
> >>>> The old code could use gem name as key when inserting it to bo_handles
> >>>> hash table while trying to remove it from hash table with bo_handle as
> >>>> key in virgl_hw_res_destroy. This triggers use after free. Also, we
> >>>> should only reuse resource from bo_handle hash when the handle type is
> >>>> FD.
> >>>
> >>> Reuse is not very accurate. Opening a shared handle (flink name)
> twice gives two GEM handles. Importing an fd handle (prime fd) twice gives
> the same GEM handle. In all cases, within a virgl_winsys, we want only one
> GEM handle and only one virgl_resource for each kernel GEM object.
> >>>
> >>> I think the logic should go like:
> >>>
> >>> if (HANDLE_TYPE_SHARED) {
> >>> if (bo_names.has(flink_name))
> >>> return bo_names[flink_name];
> >>> gem_handle = gem_open(flink_name);
> >>> } else {
> >>> gem_handle = drmPrimeFDToHandle(prime_fd);
> >>> }
> >>>
> >>>
> >>> if (bo_handles.has(gem_handle))
> >>> return bo_handles[gem_handle];
> >>> bo_handles[gem_handle] = create_new_resource();
> >>>
> >> Hi, the current patch did most of what you said with only one
> difference: it didn't insert to bo_handles[] hash when the type is
> HANDLE_TYPE_SHARED.
> >> I think this is reasonable since opening a shared handle always get a
> new gem handle very time and I think it doesn't worth to insert it to
> bo_handles[] hash.
> >> What do you think?
>
> Just to reinforce this, we can only have one GEM handle for a kernel
> object, validation will go wrong and deadlock if we submit two handles
> pointing at the same bo.
>
> Opening a shared handle should not get a new gem handle, if should
> return any gem handle that already exists.
>
I just tried and that is not the case. Sounds like a kernel bug?
>
> Dave.
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
