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.
Dave.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
