diff mbox series

[v2,5/7] bcm-vk: add bcm_vk UAPI

Message ID 20200220004825.23372-6-scott.branden@broadcom.com (mailing list archive)
State New
Headers show
Series firmware: add partial read support in request_firmware_into_buf | expand

Commit Message

Scott Branden Feb. 20, 2020, 12:48 a.m. UTC 
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

Comments

Greg KH Feb. 20, 2020, 7:50 a.m. UTC | #1 
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?

> +};
> +
> +/* 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?

> +
> +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?

> +	__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?

> +};

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?

> +
> +struct vk_reset {
> +	__u32 arg1;
> +	__u32 arg2;
> +};
> +
> +#define VK_MAGIC              0x5E
> +
> +/* Load image to Valkyrie */
> +#define VK_IOCTL_LOAD_IMAGE   _IOW(VK_MAGIC, 0x2, struct vk_image)
> +
> +/* Read data from Valkyrie */
> +#define VK_IOCTL_ACCESS_BAR   _IOWR(VK_MAGIC, 0x3, struct vk_access)
> +
> +/* Send Reset to Valkyrie */
> +#define VK_IOCTL_RESET        _IOW(VK_MAGIC, 0x4, struct vk_reset)
> +
> +/*
> + * message block - basic unit in the message where a message's size is always
> + *		   N x sizeof(basic_block)
> + */
> +struct vk_msg_blk {
> +	__u8 function_id;
> +#define VK_FID_TRANS_BUF 5
> +#define VK_FID_SHUTDOWN  8
> +	__u8 size;
> +	__u16 queue_id:4;
> +	__u16 msg_id:12;

Do not use bitfields in ioctls, they will not work properly on all
systems.  Use masks and shifts instead.

> +	__u32 context_id;
> +	__u32 args[2];
> +#define VK_CMD_PLANES_MASK 0x000F /* number of planes to up/download */
> +#define VK_CMD_UPLOAD      0x0400 /* memory transfer to vk */
> +#define VK_CMD_DOWNLOAD    0x0500 /* memory transfer from vk */
> +#define VK_CMD_MASK        0x0F00 /* command mask */
> +};
> +
> +#define VK_BAR_FWSTS			0x41C
> +/* VK_FWSTS definitions */
> +#define VK_FWSTS_RELOCATION_ENTRY	BIT(0)
> +#define VK_FWSTS_RELOCATION_EXIT	BIT(1)
> +#define VK_FWSTS_INIT_START		BIT(2)
> +#define VK_FWSTS_ARCH_INIT_DONE		BIT(3)
> +#define VK_FWSTS_PRE_KNL1_INIT_DONE	BIT(4)
> +#define VK_FWSTS_PRE_KNL2_INIT_DONE	BIT(5)
> +#define VK_FWSTS_POST_KNL_INIT_DONE	BIT(6)
> +#define VK_FWSTS_INIT_DONE		BIT(7)
> +#define VK_FWSTS_APP_INIT_START		BIT(8)
> +#define VK_FWSTS_APP_INIT_DONE		BIT(9)

I do not think that BIT() is exported to userspace properly, is it
really ok here?

> +#define VK_FWSTS_MASK			0xFFFFFFFF
> +#define VK_FWSTS_READY			(VK_FWSTS_INIT_START | \
> +					 VK_FWSTS_ARCH_INIT_DONE | \
> +					 VK_FWSTS_PRE_KNL1_INIT_DONE | \
> +					 VK_FWSTS_PRE_KNL2_INIT_DONE | \
> +					 VK_FWSTS_POST_KNL_INIT_DONE | \
> +					 VK_FWSTS_INIT_DONE | \
> +					 VK_FWSTS_APP_INIT_START | \
> +					 VK_FWSTS_APP_INIT_DONE)
> +/* Deinit */
> +#define VK_FWSTS_APP_DEINIT_START	BIT(23)
> +#define VK_FWSTS_APP_DEINIT_DONE	BIT(24)
> +#define VK_FWSTS_DRV_DEINIT_START	BIT(25)
> +#define VK_FWSTS_DRV_DEINIT_DONE	BIT(26)
> +#define VK_FWSTS_RESET_DONE		BIT(27)
> +#define VK_FWSTS_DEINIT_TRIGGERED	(VK_FWSTS_APP_DEINIT_START | \
> +					 VK_FWSTS_APP_DEINIT_DONE  | \
> +					 VK_FWSTS_DRV_DEINIT_START | \
> +					 VK_FWSTS_DRV_DEINIT_DONE)
> +/* Last nibble for reboot reason */
> +#define VK_FWSTS_RESET_REASON_SHIFT	28
> +#define VK_FWSTS_RESET_REASON_MASK	(0xF << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_SYS_PWRUP	(0x0 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_MBOX_DB		(0x1 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_M7_WDOG		(0x2 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_TEMP		(0x3 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_PCI_FLR		(0x4 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_PCI_HOT		(0x5 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_PCI_WARM		(0x6 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_PCI_COLD		(0x7 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_L1		(0x8 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_L0		(0x9 << VK_FWSTS_RESET_REASON_SHIFT)
> +#define VK_FWSTS_RESET_UNKNOWN		(0xF << VK_FWSTS_RESET_REASON_SHIFT)

What are all of these #defines doing in an uapi file?  How is userspace
going to use them?

thanks,

greg k-h
Scott Branden Feb. 21, 2020, 1:15 a.m. UTC | #2 
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.
>
>> +};
>> +
>> +/* 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.
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.
I guess I could change the API at this point to lookup the default 
filename if NULL filename passed into ioctl.
>> +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.
>
>> +	__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.
I don't think there isn't even enough memory in such systems for the 
number of streams of video buffers needed for transcoding.
This driver is only used in high end 64-bit x86 servers.
But, VK_IOCTL_ACCESS_BAR can go away entirely if standard user space 
approach already exists as you imply.
>> +};
> 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?
>
>> +
>> +struct vk_reset {
>> +	__u32 arg1;
>> +	__u32 arg2;
>> +};
>> +
>> +#define VK_MAGIC              0x5E
>> +
>> +/* Load image to Valkyrie */
>> +#define VK_IOCTL_LOAD_IMAGE   _IOW(VK_MAGIC, 0x2, struct vk_image)
>> +
>> +/* Read data from Valkyrie */
>> +#define VK_IOCTL_ACCESS_BAR   _IOWR(VK_MAGIC, 0x3, struct vk_access)
>> +
>> +/* Send Reset to Valkyrie */
>> +#define VK_IOCTL_RESET        _IOW(VK_MAGIC, 0x4, struct vk_reset)
>> +
>> +/*
>> + * message block - basic unit in the message where a message's size is always
>> + *		   N x sizeof(basic_block)
>> + */
>> +struct vk_msg_blk {
>> +	__u8 function_id;
>> +#define VK_FID_TRANS_BUF 5
>> +#define VK_FID_SHUTDOWN  8
>> +	__u8 size;
>> +	__u16 queue_id:4;
>> +	__u16 msg_id:12;
> Do not use bitfields in ioctls, they will not work properly on all
> systems.  Use masks and shifts instead.
I don't like the bitfields either - structure inherited from firmware code.
Will work on getting these removed.
>
>> +	__u32 context_id;
>> +	__u32 args[2];
>> +#define VK_CMD_PLANES_MASK 0x000F /* number of planes to up/download */
>> +#define VK_CMD_UPLOAD      0x0400 /* memory transfer to vk */
>> +#define VK_CMD_DOWNLOAD    0x0500 /* memory transfer from vk */
>> +#define VK_CMD_MASK        0x0F00 /* command mask */
>> +};
>> +
>> +#define VK_BAR_FWSTS			0x41C
>> +/* VK_FWSTS definitions */
>> +#define VK_FWSTS_RELOCATION_ENTRY	BIT(0)
>> +#define VK_FWSTS_RELOCATION_EXIT	BIT(1)
>> +#define VK_FWSTS_INIT_START		BIT(2)
>> +#define VK_FWSTS_ARCH_INIT_DONE		BIT(3)
>> +#define VK_FWSTS_PRE_KNL1_INIT_DONE	BIT(4)
>> +#define VK_FWSTS_PRE_KNL2_INIT_DONE	BIT(5)
>> +#define VK_FWSTS_POST_KNL_INIT_DONE	BIT(6)
>> +#define VK_FWSTS_INIT_DONE		BIT(7)
>> +#define VK_FWSTS_APP_INIT_START		BIT(8)
>> +#define VK_FWSTS_APP_INIT_DONE		BIT(9)
> I do not think that BIT() is exported to userspace properly, is it
> really ok here?
Works fine.  Also in uapi/linux/rtc.h.
>
>> +#define VK_FWSTS_MASK			0xFFFFFFFF
>> +#define VK_FWSTS_READY			(VK_FWSTS_INIT_START | \
>> +					 VK_FWSTS_ARCH_INIT_DONE | \
>> +					 VK_FWSTS_PRE_KNL1_INIT_DONE | \
>> +					 VK_FWSTS_PRE_KNL2_INIT_DONE | \
>> +					 VK_FWSTS_POST_KNL_INIT_DONE | \
>> +					 VK_FWSTS_INIT_DONE | \
>> +					 VK_FWSTS_APP_INIT_START | \
>> +					 VK_FWSTS_APP_INIT_DONE)
>> +/* Deinit */
>> +#define VK_FWSTS_APP_DEINIT_START	BIT(23)
>> +#define VK_FWSTS_APP_DEINIT_DONE	BIT(24)
>> +#define VK_FWSTS_DRV_DEINIT_START	BIT(25)
>> +#define VK_FWSTS_DRV_DEINIT_DONE	BIT(26)
>> +#define VK_FWSTS_RESET_DONE		BIT(27)
>> +#define VK_FWSTS_DEINIT_TRIGGERED	(VK_FWSTS_APP_DEINIT_START | \
>> +					 VK_FWSTS_APP_DEINIT_DONE  | \
>> +					 VK_FWSTS_DRV_DEINIT_START | \
>> +					 VK_FWSTS_DRV_DEINIT_DONE)
>> +/* Last nibble for reboot reason */
>> +#define VK_FWSTS_RESET_REASON_SHIFT	28
>> +#define VK_FWSTS_RESET_REASON_MASK	(0xF << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_SYS_PWRUP	(0x0 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_MBOX_DB		(0x1 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_M7_WDOG		(0x2 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_TEMP		(0x3 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_PCI_FLR		(0x4 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_PCI_HOT		(0x5 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_PCI_WARM		(0x6 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_PCI_COLD		(0x7 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_L1		(0x8 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_L0		(0x9 << VK_FWSTS_RESET_REASON_SHIFT)
>> +#define VK_FWSTS_RESET_UNKNOWN		(0xF << VK_FWSTS_RESET_REASON_SHIFT)
> What are all of these #defines doing in an uapi file?  How is userspace
> going to use them?
There are actually 2 linux user spaces that use this header.
One is the x86 host with the bcm-vk PCI driver.
The x86 host user space could use them to check the firmware status and 
find out what state VK is in.

The other user space is a coprocessor inside the VK SOC.
The app running in user space needs to know the state of the FWSTS in 
order to proceed.
It includes this header in its user space app (even though it doesn't 
user the linux driver, it needs access to the same FWSTS register 
directly).
> thanks,
>
> greg k-h
Arnd Bergmann Feb. 21, 2020, 8:34 a.m. UTC | #3 
On Fri, Feb 21, 2020 at 2:16 AM Scott Branden
<scott.branden@broadcom.com> wrote:


> >> +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.
> >
> >> +    __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.
> I don't think there isn't even enough memory in such systems for the
> number of streams of video buffers needed for transcoding.
> This driver is only used in high end 64-bit x86 servers.

Please see Documentation/core-api/ioctl.rst

All ioctl interfaces should be written in a portable way that works with
compat user space and avoids all padding in order to not leak kernel
data into user space.

If the driver is passing video buffers for transcoding, shouldn't the driver
use the existing drivers/media interfaces for that? If it needs features
that are not present there, they can probably be added.

        Arnd
Greg KH Feb. 21, 2020, 9:22 a.m. UTC | #4 
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
Scott Branden Feb. 21, 2020, 6:27 p.m. UTC | #5 
On 2020-02-21 12:34 a.m., Arnd Bergmann wrote:
> On Fri, Feb 21, 2020 at 2:16 AM Scott Branden
> <scott.branden@broadcom.com> wrote:
>
>
>>>> +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.
>>>> +    __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.
>> I don't think there isn't even enough memory in such systems for the
>> number of streams of video buffers needed for transcoding.
>> This driver is only used in high end 64-bit x86 servers.
> Please see Documentation/core-api/ioctl.rst
>
> All ioctl interfaces should be written in a portable way that works with
> compat user space and avoids all padding in order to not leak kernel
> data into user space.
>
> If the driver is passing video buffers for transcoding, shouldn't the driver
> use the existing drivers/media interfaces for that? If it needs features
> that are not present there, they can probably be added.
It doesn't utilize any media interfaces.  It is just an offload engine.  
Really, it could be offloading anything.
There is no infrastructure for other drivers in place that perform such 
transcoding operations.
Perhaps I shouldn't mention it is doing video in this driver.
>
>          Arnd
diff mbox series

Patch

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 */
+};
+
+/* 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"
+
+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 */
+	__u64 offset;   /* offset in BAR */
+	__u32 *data;    /* where to read/write data to */
+};
+
+struct vk_reset {
+	__u32 arg1;
+	__u32 arg2;
+};
+
+#define VK_MAGIC              0x5E
+
+/* Load image to Valkyrie */
+#define VK_IOCTL_LOAD_IMAGE   _IOW(VK_MAGIC, 0x2, struct vk_image)
+
+/* Read data from Valkyrie */
+#define VK_IOCTL_ACCESS_BAR   _IOWR(VK_MAGIC, 0x3, struct vk_access)
+
+/* Send Reset to Valkyrie */
+#define VK_IOCTL_RESET        _IOW(VK_MAGIC, 0x4, struct vk_reset)
+
+/*
+ * message block - basic unit in the message where a message's size is always
+ *		   N x sizeof(basic_block)
+ */
+struct vk_msg_blk {
+	__u8 function_id;
+#define VK_FID_TRANS_BUF 5
+#define VK_FID_SHUTDOWN  8
+	__u8 size;
+	__u16 queue_id:4;
+	__u16 msg_id:12;
+	__u32 context_id;
+	__u32 args[2];
+#define VK_CMD_PLANES_MASK 0x000F /* number of planes to up/download */
+#define VK_CMD_UPLOAD      0x0400 /* memory transfer to vk */
+#define VK_CMD_DOWNLOAD    0x0500 /* memory transfer from vk */
+#define VK_CMD_MASK        0x0F00 /* command mask */
+};
+
+#define VK_BAR_FWSTS			0x41C
+/* VK_FWSTS definitions */
+#define VK_FWSTS_RELOCATION_ENTRY	BIT(0)
+#define VK_FWSTS_RELOCATION_EXIT	BIT(1)
+#define VK_FWSTS_INIT_START		BIT(2)
+#define VK_FWSTS_ARCH_INIT_DONE		BIT(3)
+#define VK_FWSTS_PRE_KNL1_INIT_DONE	BIT(4)
+#define VK_FWSTS_PRE_KNL2_INIT_DONE	BIT(5)
+#define VK_FWSTS_POST_KNL_INIT_DONE	BIT(6)
+#define VK_FWSTS_INIT_DONE		BIT(7)
+#define VK_FWSTS_APP_INIT_START		BIT(8)
+#define VK_FWSTS_APP_INIT_DONE		BIT(9)
+#define VK_FWSTS_MASK			0xFFFFFFFF
+#define VK_FWSTS_READY			(VK_FWSTS_INIT_START | \
+					 VK_FWSTS_ARCH_INIT_DONE | \
+					 VK_FWSTS_PRE_KNL1_INIT_DONE | \
+					 VK_FWSTS_PRE_KNL2_INIT_DONE | \
+					 VK_FWSTS_POST_KNL_INIT_DONE | \
+					 VK_FWSTS_INIT_DONE | \
+					 VK_FWSTS_APP_INIT_START | \
+					 VK_FWSTS_APP_INIT_DONE)
+/* Deinit */
+#define VK_FWSTS_APP_DEINIT_START	BIT(23)
+#define VK_FWSTS_APP_DEINIT_DONE	BIT(24)
+#define VK_FWSTS_DRV_DEINIT_START	BIT(25)
+#define VK_FWSTS_DRV_DEINIT_DONE	BIT(26)
+#define VK_FWSTS_RESET_DONE		BIT(27)
+#define VK_FWSTS_DEINIT_TRIGGERED	(VK_FWSTS_APP_DEINIT_START | \
+					 VK_FWSTS_APP_DEINIT_DONE  | \
+					 VK_FWSTS_DRV_DEINIT_START | \
+					 VK_FWSTS_DRV_DEINIT_DONE)
+/* Last nibble for reboot reason */
+#define VK_FWSTS_RESET_REASON_SHIFT	28
+#define VK_FWSTS_RESET_REASON_MASK	(0xF << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_SYS_PWRUP	(0x0 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_MBOX_DB		(0x1 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_M7_WDOG		(0x2 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_TEMP		(0x3 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_PCI_FLR		(0x4 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_PCI_HOT		(0x5 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_PCI_WARM		(0x6 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_PCI_COLD		(0x7 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_L1		(0x8 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_L0		(0x9 << VK_FWSTS_RESET_REASON_SHIFT)
+#define VK_FWSTS_RESET_UNKNOWN		(0xF << VK_FWSTS_RESET_REASON_SHIFT)
+
+#endif /* __UAPI_LINUX_MISC_BCM_VK_H */