Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

Iouri Tarassov
2020-08-28T00:25:28Z

Hi Pavel,

Thanks for your time reviewing the driver.

On 8/21/2020 6:53 AM, Pavel Machek wrote:
> On Fri 2020-08-14 08:38:53, Sasha Levin wrote:
> > Add support for a Hyper-V based vGPU implementation that exposes the
> > DirectX API to Linux userspace.
>
> NAK.
>
> Kernel APIs should be documented. ... in tree and with suitable license.
We are considering to add required documentation. The design is that the
driver IOCTLs will not be called directly by user mode drivers or
applications. They will instead link with libdxcore.so and use the
D3DKMT* interface to send requests to the driver.  libdxcore will
translate the request to driver IOCTLs and do some additional work. For
example, it will translate the path of the user mode driver name to the
Linux namespace. The D3DKMTinterface is documented on MSDN. Do you
suggest that the IOCTL interface needs to be documented or the D3DKMT
interface?
>
> > +int dxgadapter_init(struct dxgadapter *adapter, struct hv_device *hdev)
> > +{
> > + int ret = 0;
> > + char s[80];
> > +
> > + UNUSED(s);
>
> What is going on here?
This is a mistake, which will be fixed.
>
> > +{
> > + struct dxgprocess_adapter *adapter_info = dxgmem_alloc(process,
> > + DXGMEM_PROCESS_ADAPTER,
> > + sizeof
> > + (*adapter_info));
>
> We normally use kernel functions in kernel code.
Using a custom memory allocation function allows us to track memory
allocations per DXGPROCESS and catch memory leaks when a DXGPROCESS is
destroyed or when the driver is unloaded. It also allows to easily
change the memory allocation implementation if needed.
>
> > + dxgmutex_unlock(&device->adapter_info->device_list_mutex);
>
> And you should not have private locking primitives, either.
This is done to allow the implementation of custom lock order
verification. We learnt recently about lock dependency checking in
kernel and will evaluate if it can replace the custom lock order
verification.
>
> > +bool dxghwqueue_acquire_reference(struct dxghwqueue *hwqueue)
> > +{
> > + return refcount_inc_not_zero(&hwqueue->refcount);
> > +}
>
> Midlayers are evil.
I strongly agree in general, but think that in our case the layers are
very small. It allows to quickly find all places where a
reference/dereference on an object is done and addition of debug tracing
to catch errors.
>
> Best regards,
> Pavel

Thank you
Iouri