loader: Pass the device dispatch table handle to the ICD before CreateDevice

[karl-lunarg]

Issue by jdrouan-goog (MIGRATED)
Saturday Apr 14, 2018 at 01:37 GMT
Originally opened as KhronosGroup/Vulkan-LoaderAndValidationLayers#2578

---

Our ICD implementation requires that the future address of the dispatch table be in scope during CreateDevice.

[karl-lunarg]

Comment by polarina (MIGRATED)
Saturday Apr 14, 2018 at 17:26 GMT

---

For what purpose does your ICD require this?

[karl-lunarg]

Comment by jdrouan-goog (MIGRATED)
Monday Apr 16, 2018 at 15:39 GMT

---

Our platform requires this the way it is implemented. We think this is a relatively small change that should be compatible with the current ecosystem of ICDs.

cc: @jfroy

[karl-lunarg]

Comment by lenny-lunarg (MIGRATED)
Monday Apr 16, 2018 at 20:10 GMT

---

You can see the linked PR for an explanation of why I don't want to pass the entire dispatch table to the ICD, but is there any reason why you actually need the dispatch table? Generally, it's much easier to just pass function pointers to the vkGet*ProcAddr functions. Would this still address your needs or is there some reason that you actually need the dispatch table.

I still find it odd that you would need these pointers at all, but I would be far more receptive to sharing them through that method, rather than just passing the dispatch table.

[karl-lunarg]

Comment by ianelliottus (MIGRATED)
Tuesday Apr 17, 2018 at 21:02 GMT

---

I just started looking at this. I will try to get some additional info to see if I can help. I will follow-up in the PR.

[karl-lunarg]

Comment by jfroy (MIGRATED)
Monday Apr 23, 2018 at 22:44 GMT

---

I am helping @jdrouan-goog on this PR. We think the change we're making here will be transparent to all existing ICDs because of the established rules for scanning and parsing extension structures, e.g. ignore and skip what you do not recognize.

Without going into all the technical details and motivations, we are building a virtual ICD that forwards a portion of its work to a "delegate" ICD. As part of this, we want to data to dispatchable objects, much like layers do. Unfortunately, with the current loader implementation, when vkCreateDevice executes in an ICD, the final dispatch table pointer has not been set by the loader. This is our attempt at solving this problem.

It occurs to me an alternative may be possible. Looking at https://github.com/KhronosGroup/Vulkan-LoaderAndValidationLayers/blob/5de1ff1ff66aa41e271b87dcd0b47701711f6e17/loader/loader.c#L5577, could we move that line above the fpCreateDevice call?

[karl-lunarg]

Comment by lenny-lunarg (MIGRATED)
Monday Apr 23, 2018 at 23:39 GMT

---

We think the change we're making here will be transparent to all existing ICDs because of the established rules for scanning and parsing extension structures, e.g. ignore and skip what you do not recognize.

The problem I have with passing the dispatch table to the ICD isn't about using the pNext chain for this purpose — it's that the dispatch table is an internal struct. As such, no attempt is made to preserve compatibility because it is never used in an interface. Preserving compatibility would be possible, but would definitely be an inconvenience, as the items in that struct get reordered occasionally. If a system were to have a newer loader that reordered the contents of that table, while the ICD was built against an older loader, the result would be that the ICD would get the function pointers mixed up and would probably crash.

I don't want to lose the ability to reorder the members of that struct. This is why I suggested passing a pointer to vkGetDeviceProcAddr instead. Doing so avoids the compatibility concerns of passing the pointer to a table. This is also partly why Vulkan as a whole returns individual function pointers, instead of one or two big tables.

could we move that line above the fpCreateDevice call?

Maybe. The call to fpCreateDevice takes a VkDevice as a parameter. While it is legal to pass null to the function, doing so will result in a pointer that is valid for any device. This could have adverse performance implications. But even if performance isn't a problem, the only way an ICD could access that pointer would be to dereference the loader_device struct from the VkDevice argument. Don't do that. It would have all the problems of passing the dispatch pointer and then some. If you're including anything other than vk_icd.h in the interface to your ICD, then you're probably using private info that you should not count on staying the same.

If you want to see an example of what I'm suggesting, look at the interface between the loader and the layers. No dispatch tables are ever passed. Instead, at device creation the loader passes a VkLayerDeviceLink, which is defined in vk_layer.h as:

typedef struct VkLayerDeviceLink_ {
    struct VkLayerDeviceLink_ *pNext;
    PFN_vkGetInstanceProcAddr pfnNextGetInstanceProcAddr;
    PFN_vkGetDeviceProcAddr pfnNextGetDeviceProcAddr;
} VkLayerDeviceLink;

This does not include any dispatch table. Instead, the layer is responsible for building its own dispatch table from the two function pointers. While the loader and the layers do use the same code for building dispatch tables, this is purely an implementation detail. The dispatch tables are never passed between the two because different versions of the dispatch table are incompatible. This is the basic mechanism I would want to use to pass the dispatch information.

[lenny-lunarg]

I'm going to close this issue right now, since we're not going to do the change the way that was suggested, and because there's been no activity on this for a while. If people want to reopen discussion again, feel free to create a new issue and we can discuss how to make any changes.