The kernel code will create a new gem handle for SHARED handle every time, see code here:
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_gem.c#L796 On Mon, Apr 8, 2019 at 11:12 AM Chia-I Wu <olva...@gmail.com> wrote: > > > > 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? The kernel code will create a new gem handle for SHARED handle every time, see code here: https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_gem.c#L796 > >> >> >> Dave. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
