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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux