On Thu, Feb 20, 2020 at 05:15:58PM -0800, Scott Branden wrote:
Hi Greg,
Thanks for the review. Comments inline.
On 2020-02-19 11:50 p.m., Greg Kroah-Hartman wrote:
On Wed, Feb 19, 2020 at 04:48:23PM -0800, Scott Branden wrote:
Add user space api for bcm-vk driver.
Signed-off-by: Scott Branden scott.branden@broadcom.com
include/uapi/linux/misc/bcm_vk.h | 117 +++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 include/uapi/linux/misc/bcm_vk.h
diff --git a/include/uapi/linux/misc/bcm_vk.h b/include/uapi/linux/misc/bcm_vk.h new file mode 100644 index 000000000000..56a2178e06f5 --- /dev/null +++ b/include/uapi/linux/misc/bcm_vk.h @@ -0,0 +1,117 @@ +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */ +/*
- Copyright 2018-2020 Broadcom.
- */
+#ifndef __UAPI_LINUX_MISC_BCM_VK_H +#define __UAPI_LINUX_MISC_BCM_VK_H
+#include <linux/ioctl.h> +#include <linux/types.h>
+struct vk_image {
- __u32 type; /* Type of image */
+#define VK_IMAGE_TYPE_BOOT1 1 /* 1st stage (load to SRAM) */ +#define VK_IMAGE_TYPE_BOOT2 2 /* 2nd stage (load to DDR) */
- char filename[64]; /* Filename of image */
__u8?
I don't understand why char is not appropriate for a filename. Would like to understand why __u8 is correct to use here vs. char.
Why is __u8 not correct? It's the data type we use for ioctls.
+};
+/* default firmware images names */ +#define VK_BOOT1_DEF_VALKYRIE_FILENAME "vk-boot1.bin" +#define VK_BOOT2_DEF_VALKYRIE_FILENAME "vk-boot2.bin"
+#define VK_BOOT1_DEF_VIPER_FILENAME "vp-boot1.bin" +#define VK_BOOT2_DEF_VIPER_FILENAME "vp-boot2.bin"
Why do these need to be in a uapi .h file? Shouldn't they just be part of the normal MODULE_FIRMWARE() macro in the driver itself?
ioctl VK_IOCTL_LOAD_IMAGE passes in type of image to load and filename. These are the default names used if the images are autoloaded by the driver.
Then put them in the driver, not in the user api file.
But if userspace app wishes to load (or reload) the default images then it needs to know the name of the file to pass in ioctl.
That's up to userspace.
I guess I could change the API at this point to lookup the default filename if NULL filename passed into ioctl.
Yes please.
+struct vk_access {
- __u8 barno; /* BAR number to use */
- __u8 type; /* Type of access */
+#define VK_ACCESS_READ 0 +#define VK_ACCESS_WRITE 1
- __u32 len; /* length of data */
Horrible padding issues, are you sure this all works properly?
Haven't had any issues.
Use pahole to see the holes you have in here and please fix that up.
- __u64 offset; /* offset in BAR */
- __u32 *data; /* where to read/write data to */
Are you _SURE_ you want a pointer here? How do you handle the compat issues with 32/64 user/kernel space?
Don't care about 32-bit user space for this driver.
We all do, see the link that Arnd sent you.
I don't think there isn't even enough memory in such systems for the number of streams of video buffers needed for transcoding.
32bit systems have lots of memory.
This driver is only used in high end 64-bit x86 servers.
For today, what about in 2 years?
But, VK_IOCTL_ACCESS_BAR can go away entirely if standard user space approach already exists as you imply.
Yes, please use that interface, as you should never duplicate existing functionality.
+};
And isn't this just a normal PCI write thing? Can't you do it from userspace using the existing userspace PCI accesses? Why do you need a special ioctl for it?
This follows how pci_endpoint_test reads and writes BARS via ioctl. It also abstracts the accesses all into the device node being opened.
I am not familiar with userspace PCI accesses. Would this be through some sys entries?
Yes, it can read PCI config space that way, and if you use the uio interface, you can read PCI memory.
thanks,
greg k-h