| Message ID | 20220722082858.17880-3-yuji2.ishikawa@toshiba.co.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Toshiba Visconti DNN image processing accelerator driver | expand |
On Fri, Jul 22, 2022 at 05:28:55PM +0900, Yuji Ishikawa wrote: > This commit adds common definitions shared among image processing accelerator drivers > for Toshiba Visconti SoCs. Please wrap your changelog text lines properly at 72 columns. And you need to provide a lot more information here as to what this is, it's not enough to be able to properly review this with just a single sentence. > > Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp> > Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > --- > v1 -> v2: > - applied checkpatch.pl --strict > --- > drivers/soc/Kconfig | 1 + > drivers/soc/Makefile | 1 + > drivers/soc/visconti/Kconfig | 1 + > drivers/soc/visconti/Makefile | 6 +++ > drivers/soc/visconti/ipa_common.c | 55 +++++++++++++++++++ > drivers/soc/visconti/ipa_common.h | 18 +++++++ > drivers/soc/visconti/uapi/ipa.h | 90 +++++++++++++++++++++++++++++++ > 7 files changed, 172 insertions(+) > create mode 100644 drivers/soc/visconti/Kconfig > create mode 100644 drivers/soc/visconti/Makefile > create mode 100644 drivers/soc/visconti/ipa_common.c > create mode 100644 drivers/soc/visconti/ipa_common.h > create mode 100644 drivers/soc/visconti/uapi/ipa.h > > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig > index e8a30c4c5..c99139aa8 100644 > --- a/drivers/soc/Kconfig > +++ b/drivers/soc/Kconfig > @@ -22,6 +22,7 @@ source "drivers/soc/tegra/Kconfig" > source "drivers/soc/ti/Kconfig" > source "drivers/soc/ux500/Kconfig" > source "drivers/soc/versatile/Kconfig" > +source "drivers/soc/visconti/Kconfig" > source "drivers/soc/xilinx/Kconfig" > > endmenu > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile > index a05e9fbcd..455b993c2 100644 > --- a/drivers/soc/Makefile > +++ b/drivers/soc/Makefile > @@ -28,4 +28,5 @@ obj-$(CONFIG_ARCH_TEGRA) += tegra/ > obj-y += ti/ > obj-$(CONFIG_ARCH_U8500) += ux500/ > obj-$(CONFIG_PLAT_VERSATILE) += versatile/ > +obj-$(CONFIG_ARCH_VISCONTI) += visconti/ > obj-y += xilinx/ > diff --git a/drivers/soc/visconti/Kconfig b/drivers/soc/visconti/Kconfig > new file mode 100644 > index 000000000..8b1378917 > --- /dev/null > +++ b/drivers/soc/visconti/Kconfig > @@ -0,0 +1 @@ > + > diff --git a/drivers/soc/visconti/Makefile b/drivers/soc/visconti/Makefile > new file mode 100644 > index 000000000..8d710da08 > --- /dev/null > +++ b/drivers/soc/visconti/Makefile > @@ -0,0 +1,6 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Makefile for the Visconti specific device drivers. > +# > + > +obj-y += ipa_common.o > diff --git a/drivers/soc/visconti/ipa_common.c b/drivers/soc/visconti/ipa_common.c > new file mode 100644 > index 000000000..6345f33c5 > --- /dev/null > +++ b/drivers/soc/visconti/ipa_common.c > @@ -0,0 +1,55 @@ > +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause Why is this dual-licensed? I have to ask, and also, have to see some sort of justification as to why this is needed. Doing dual-licensed kernel code is tough and a pain and we need to know that you, and your lawyers, understand the issues involved here. > +/* Toshiba Visconti Image Processing Accelerator Support > + * > + * (C) Copyright 2022 TOSHIBA CORPORATION > + * (C) Copyright 2022 Toshiba Electronic Devices & Storage Corporation > + */ > + > +#include "ipa_common.h" > + > +int ipa_attach_dmabuf(struct device *dev, int fd, struct dma_buf_attachment **a, > + struct sg_table **s, dma_addr_t *addr, enum dma_data_direction dma_dir) > +{ > + struct dma_buf_attachment *attachment; > + struct dma_buf *dmabuf; > + struct sg_table *sgt; > + int ret; > + > + dmabuf = dma_buf_get(fd); > + if (IS_ERR(dmabuf)) { > + dev_err(dev, "Invalid dmabuf FD\n"); > + return PTR_ERR(dmabuf); > + } > + attachment = dma_buf_attach(dmabuf, dev); > + > + if (IS_ERR(attachment)) { > + dev_err(dev, "Failed to attach dmabuf\n"); > + ret = PTR_ERR(attachment); > + goto err_put; > + } > + sgt = dma_buf_map_attachment(attachment, dma_dir); > + if (IS_ERR(sgt)) { > + dev_err(dev, "Failed to get dmabufs sg_table\n"); > + ret = PTR_ERR(sgt); > + goto err_detach; > + } > + if (sgt->nents != 1) { > + dev_err(dev, "Sparse DMA region is unsupported\n"); > + ret = -EINVAL; > + goto err_unmap; > + } > + > + *addr = sg_dma_address(sgt->sgl); > + *a = attachment; > + *s = sgt; > + > + return 0; > + > +err_unmap: > + dma_buf_unmap_attachment(attachment, sgt, dma_dir); > +err_detach: > + dma_buf_detach(dmabuf, attachment); > +err_put: > + dma_buf_put(dmabuf); > + return ret; > +} Why do you have a whole file for one function? That feels unneeded. thanks, greg k-h
Hi Greg, Thank you for your comments. > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Monday, July 25, 2022 9:46 PM > To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) > <yuji2.ishikawa@toshiba.co.jp> > Cc: Rob Herring <robh+dt@kernel.org>; Hans Verkuil <hverkuil@xs4all.nl>; > iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ) > <nobuhiro1.iwamatsu@toshiba.co.jp>; Jonathan Corbet <corbet@lwn.net>; > Sumit Semwal <sumit.semwal@linaro.org>; Christian König > <christian.koenig@amd.com>; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; linux-media@vger.kernel.org; > dri-devel@lists.freedesktop.org; linaro-mm-sig@lists.linaro.org > Subject: Re: [PATCH v2 2/5] soc: visconti: Add Toshiba Visconti image > processing accelerator common source > > On Fri, Jul 22, 2022 at 05:28:55PM +0900, Yuji Ishikawa wrote: > > This commit adds common definitions shared among image processing > > accelerator drivers for Toshiba Visconti SoCs. > > Please wrap your changelog text lines properly at 72 columns. > > And you need to provide a lot more information here as to what this is, it's not > enough to be able to properly review this with just a single sentence. > I'll update changelog. > > > > Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp> > > Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > > --- > > v1 -> v2: > > - applied checkpatch.pl --strict > > --- > > drivers/soc/Kconfig | 1 + > > drivers/soc/Makefile | 1 + > > drivers/soc/visconti/Kconfig | 1 + > > drivers/soc/visconti/Makefile | 6 +++ > > drivers/soc/visconti/ipa_common.c | 55 +++++++++++++++++++ > > drivers/soc/visconti/ipa_common.h | 18 +++++++ > > drivers/soc/visconti/uapi/ipa.h | 90 > +++++++++++++++++++++++++++++++ > > 7 files changed, 172 insertions(+) > > create mode 100644 drivers/soc/visconti/Kconfig create mode 100644 > > drivers/soc/visconti/Makefile create mode 100644 > > drivers/soc/visconti/ipa_common.c create mode 100644 > > drivers/soc/visconti/ipa_common.h create mode 100644 > > drivers/soc/visconti/uapi/ipa.h > > > > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig index > > e8a30c4c5..c99139aa8 100644 > > --- a/drivers/soc/Kconfig > > +++ b/drivers/soc/Kconfig > > @@ -22,6 +22,7 @@ source "drivers/soc/tegra/Kconfig" > > source "drivers/soc/ti/Kconfig" > > source "drivers/soc/ux500/Kconfig" > > source "drivers/soc/versatile/Kconfig" > > +source "drivers/soc/visconti/Kconfig" > > source "drivers/soc/xilinx/Kconfig" > > > > endmenu > > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile index > > a05e9fbcd..455b993c2 100644 > > --- a/drivers/soc/Makefile > > +++ b/drivers/soc/Makefile > > @@ -28,4 +28,5 @@ obj-$(CONFIG_ARCH_TEGRA) += tegra/ > > obj-y += ti/ > > obj-$(CONFIG_ARCH_U8500) += ux500/ > > obj-$(CONFIG_PLAT_VERSATILE) += versatile/ > > +obj-$(CONFIG_ARCH_VISCONTI) += visconti/ > > obj-y += xilinx/ > > diff --git a/drivers/soc/visconti/Kconfig > > b/drivers/soc/visconti/Kconfig new file mode 100644 index > > 000000000..8b1378917 > > --- /dev/null > > +++ b/drivers/soc/visconti/Kconfig > > @@ -0,0 +1 @@ > > + > > diff --git a/drivers/soc/visconti/Makefile > > b/drivers/soc/visconti/Makefile new file mode 100644 index > > 000000000..8d710da08 > > --- /dev/null > > +++ b/drivers/soc/visconti/Makefile > > @@ -0,0 +1,6 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +# > > +# Makefile for the Visconti specific device drivers. > > +# > > + > > +obj-y += ipa_common.o > > diff --git a/drivers/soc/visconti/ipa_common.c > > b/drivers/soc/visconti/ipa_common.c > > new file mode 100644 > > index 000000000..6345f33c5 > > --- /dev/null > > +++ b/drivers/soc/visconti/ipa_common.c > > @@ -0,0 +1,55 @@ > > +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause > > Why is this dual-licensed? I have to ask, and also, have to see some sort of > justification as to why this is needed. Doing dual-licensed kernel code is > tough and a pain and we need to know that you, and your lawyers, understand > the issues involved here. > I'll talk with development members. > > > +/* Toshiba Visconti Image Processing Accelerator Support > > + * > > + * (C) Copyright 2022 TOSHIBA CORPORATION > > + * (C) Copyright 2022 Toshiba Electronic Devices & Storage > > +Corporation */ > > + > > +#include "ipa_common.h" > > + > > +int ipa_attach_dmabuf(struct device *dev, int fd, struct > dma_buf_attachment **a, > > + struct sg_table **s, dma_addr_t *addr, enum > > +dma_data_direction dma_dir) { > > + struct dma_buf_attachment *attachment; > > + struct dma_buf *dmabuf; > > + struct sg_table *sgt; > > + int ret; > > + > > + dmabuf = dma_buf_get(fd); > > + if (IS_ERR(dmabuf)) { > > + dev_err(dev, "Invalid dmabuf FD\n"); > > + return PTR_ERR(dmabuf); > > + } > > + attachment = dma_buf_attach(dmabuf, dev); > > + > > + if (IS_ERR(attachment)) { > > + dev_err(dev, "Failed to attach dmabuf\n"); > > + ret = PTR_ERR(attachment); > > + goto err_put; > > + } > > + sgt = dma_buf_map_attachment(attachment, dma_dir); > > + if (IS_ERR(sgt)) { > > + dev_err(dev, "Failed to get dmabufs sg_table\n"); > > + ret = PTR_ERR(sgt); > > + goto err_detach; > > + } > > + if (sgt->nents != 1) { > > + dev_err(dev, "Sparse DMA region is unsupported\n"); > > + ret = -EINVAL; > > + goto err_unmap; > > + } > > + > > + *addr = sg_dma_address(sgt->sgl); > > + *a = attachment; > > + *s = sgt; > > + > > + return 0; > > + > > +err_unmap: > > + dma_buf_unmap_attachment(attachment, sgt, dma_dir); > > +err_detach: > > + dma_buf_detach(dmabuf, attachment); > > +err_put: > > + dma_buf_put(dmabuf); > > + return ret; > > +} > > Why do you have a whole file for one function? That feels unneeded. > The function ipa_attach_dmabuf() is shared among several accelerator drivers. Visconti has other 8 kinds of accelerators; Affine, Pyramid, DSPIF, ... I should have mentioned detail of how ipa_common.c is used. Sorry. > thanks, > > greg k-h Regards, Yuji
diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig index e8a30c4c5..c99139aa8 100644 --- a/drivers/soc/Kconfig +++ b/drivers/soc/Kconfig @@ -22,6 +22,7 @@ source "drivers/soc/tegra/Kconfig" source "drivers/soc/ti/Kconfig" source "drivers/soc/ux500/Kconfig" source "drivers/soc/versatile/Kconfig" +source "drivers/soc/visconti/Kconfig" source "drivers/soc/xilinx/Kconfig" endmenu diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile index a05e9fbcd..455b993c2 100644 --- a/drivers/soc/Makefile +++ b/drivers/soc/Makefile @@ -28,4 +28,5 @@ obj-$(CONFIG_ARCH_TEGRA) += tegra/ obj-y += ti/ obj-$(CONFIG_ARCH_U8500) += ux500/ obj-$(CONFIG_PLAT_VERSATILE) += versatile/ +obj-$(CONFIG_ARCH_VISCONTI) += visconti/ obj-y += xilinx/ diff --git a/drivers/soc/visconti/Kconfig b/drivers/soc/visconti/Kconfig new file mode 100644 index 000000000..8b1378917 --- /dev/null +++ b/drivers/soc/visconti/Kconfig @@ -0,0 +1 @@ + diff --git a/drivers/soc/visconti/Makefile b/drivers/soc/visconti/Makefile new file mode 100644 index 000000000..8d710da08 --- /dev/null +++ b/drivers/soc/visconti/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# Makefile for the Visconti specific device drivers. +# + +obj-y += ipa_common.o diff --git a/drivers/soc/visconti/ipa_common.c b/drivers/soc/visconti/ipa_common.c new file mode 100644 index 000000000..6345f33c5 --- /dev/null +++ b/drivers/soc/visconti/ipa_common.c @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause +/* Toshiba Visconti Image Processing Accelerator Support + * + * (C) Copyright 2022 TOSHIBA CORPORATION + * (C) Copyright 2022 Toshiba Electronic Devices & Storage Corporation + */ + +#include "ipa_common.h" + +int ipa_attach_dmabuf(struct device *dev, int fd, struct dma_buf_attachment **a, + struct sg_table **s, dma_addr_t *addr, enum dma_data_direction dma_dir) +{ + struct dma_buf_attachment *attachment; + struct dma_buf *dmabuf; + struct sg_table *sgt; + int ret; + + dmabuf = dma_buf_get(fd); + if (IS_ERR(dmabuf)) { + dev_err(dev, "Invalid dmabuf FD\n"); + return PTR_ERR(dmabuf); + } + attachment = dma_buf_attach(dmabuf, dev); + + if (IS_ERR(attachment)) { + dev_err(dev, "Failed to attach dmabuf\n"); + ret = PTR_ERR(attachment); + goto err_put; + } + sgt = dma_buf_map_attachment(attachment, dma_dir); + if (IS_ERR(sgt)) { + dev_err(dev, "Failed to get dmabufs sg_table\n"); + ret = PTR_ERR(sgt); + goto err_detach; + } + if (sgt->nents != 1) { + dev_err(dev, "Sparse DMA region is unsupported\n"); + ret = -EINVAL; + goto err_unmap; + } + + *addr = sg_dma_address(sgt->sgl); + *a = attachment; + *s = sgt; + + return 0; + +err_unmap: + dma_buf_unmap_attachment(attachment, sgt, dma_dir); +err_detach: + dma_buf_detach(dmabuf, attachment); +err_put: + dma_buf_put(dmabuf); + return ret; +} diff --git a/drivers/soc/visconti/ipa_common.h b/drivers/soc/visconti/ipa_common.h new file mode 100644 index 000000000..786988c52 --- /dev/null +++ b/drivers/soc/visconti/ipa_common.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */ +/* Toshiba Visconti Image Processing Accelerator Support + * + * (C) Copyright 2022 TOSHIBA CORPORATION + * (C) Copyright 2022 Toshiba Electronic Devices & Storage Corporation + */ + +#include <linux/dma-mapping.h> +#include <linux/dma-buf.h> + +#define COHERENT_ADDRESS_BIT (0x400000000ULL) +#define IPA_POLL_EVENT_NONE (0) +#define IPA_POLL_EVENT_DONE (1) +#define IPA_POLL_EVENT_ERROR (2) +#define IPA_WAKEUP_RETRY_DELAY (300 * 1000) /*usec*/ + +int ipa_attach_dmabuf(struct device *dev, int fd, struct dma_buf_attachment **a, + struct sg_table **s, dma_addr_t *addr, enum dma_data_direction dma_dir); diff --git a/drivers/soc/visconti/uapi/ipa.h b/drivers/soc/visconti/uapi/ipa.h new file mode 100644 index 000000000..b13bcf9c5 --- /dev/null +++ b/drivers/soc/visconti/uapi/ipa.h @@ -0,0 +1,90 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +/* Toshiba Visconti Image Processing Accelerator Support + * + * (C) Copyright 2022 TOSHIBA CORPORATION + * (C) Copyright 2022 Toshiba Electronic Devices & Storage Corporation + */ + +#ifndef _UAPI_LINUX_IPA_H +#define _UAPI_LINUX_IPA_H + +#include <linux/types.h> + +/** + * enum drv_ipa_state - state of Visconti IPA driver + * + * @DRV_IPA_STATE_IDLE: driver is idle + * @DRV_IPA_STATE_BUSY: device is busy + */ +enum drv_ipa_state { DRV_IPA_STATE_IDLE = 0, DRV_IPA_STATE_BUSY }; + +/** + * enum drv_ipa_buffer_direction - usage of buffer + * + * @DRV_IPA_DIR_BIDIRECTION: cpu writes/reads data + * @DRV_IPA_DIR_TO_DEVICE: cpu writes data + * @DRV_IPA_DIR_FROM_DEVICE: cpu reads data + * @DRV_IPA_DIR_NONE: cpu not access or non-coherent + */ +enum drv_ipa_buffer_direction { + DRV_IPA_DIR_BIDIRECTION = 0, + DRV_IPA_DIR_TO_DEVICE, + DRV_IPA_DIR_FROM_DEVICE, + DRV_IPA_DIR_NONE +}; + +/** + * struct drv_ipa_buffer_info - buffer information for Visconti IPA drivers + * + * @fd: file descriptor of Ion allocated heap + * @coherent: allocated via coherent bus or not + * @direction: buffer direction (to/from device) + * + * note: When the Ion heap is allocated wit ION_FLAG_CACHED, + * &struct drv_ipa_buffer_info.coherent should be true. + * Else, it should be false. + */ +struct drv_ipa_buffer_info { + __u32 fd; + bool coherent; + enum drv_ipa_buffer_direction direction; +}; + +/** + * struct drv_ipa_addr - address structure for Visconti IPA drivers + * + * @buffer_index: array index of &struct drv_ipa_buffer_info + * @offset: offset from the top of Ion heap + */ +struct drv_ipa_addr { + __u32 buffer_index : 4; + __u32 offset : 28; +}; + +#define IPA_BUFFER_INDEX_MAX (16) +#define IPA_INVALID_ADDR \ + { \ + .buffer_index = 0xfu, .offset = 0xfffffffu \ + } + +static inline struct drv_ipa_addr drv_ipa_invalid_addr(void) +{ + struct drv_ipa_addr invalid_addr = IPA_INVALID_ADDR; + return invalid_addr; +} + +static inline bool drv_ipa_is_invalid_addr(struct drv_ipa_addr addr) +{ + struct drv_ipa_addr invalid_addr = IPA_INVALID_ADDR; + + if (addr.buffer_index == invalid_addr.buffer_index && addr.offset == invalid_addr.offset) { + return true; + } + return false; +} + +#define IOC_IPA_MAGIC 'I' +#define IOC_IPA_START _IO(IOC_IPA_MAGIC, 0) +#define IOC_IPA_GET_STATUS _IO(IOC_IPA_MAGIC, 1) + +#endif /* _UAPI_LINUX_IPA_H */