Re: [PATCH v3 11/14] media: platform: Add Sunxi-Cedrus VPU decoder driver
Hi Paul, I love your patch! Yet something to improve: [auto build test ERROR on linuxtv-media/master] [also build test ERROR on v4.17-rc4 next-20180507] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Paul-Kocialkowski/Sunxi-Cedrus-driver-for-the-Allwinner-Video-Engine-using-media-requests/20180508-004955 base: git://linuxtv.org/media_tree.git master config: arm-allmodconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm All error/warnings (new ones prefixed by >>): >> drivers/media//platform/sunxi/cedrus/sunxi_cedrus.c:205:3: error: 'const >> struct media_device_ops' has no member named 'req_validate' .req_validate = vb2_request_validate, ^~~~ >> drivers/media//platform/sunxi/cedrus/sunxi_cedrus.c:205:18: error: >> 'vb2_request_validate' undeclared here (not in a function); did you mean >> 'vb2_queue_release'? .req_validate = vb2_request_validate, ^~~~ vb2_queue_release >> drivers/media//platform/sunxi/cedrus/sunxi_cedrus.c:206:3: error: 'const >> struct media_device_ops' has no member named 'req_queue' .req_queue = vb2_m2m_request_queue, ^ >> drivers/media//platform/sunxi/cedrus/sunxi_cedrus.c:206:15: error: >> 'vb2_m2m_request_queue' undeclared here (not in a function); did you mean >> 'v4l2_m2m_buf_queue'? .req_queue = vb2_m2m_request_queue, ^ v4l2_m2m_buf_queue >> drivers/media//platform/sunxi/cedrus/sunxi_cedrus.c:206:15: warning: excess >> elements in struct initializer drivers/media//platform/sunxi/cedrus/sunxi_cedrus.c:206:15: note: (near initialization for 'sunxi_cedrus_m2m_media_ops') -- drivers/media//platform/sunxi/cedrus/sunxi_cedrus_video.c: In function 'sunxi_cedrus_stop_streaming': >> drivers/media//platform/sunxi/cedrus/sunxi_cedrus_video.c:437:3: error: >> implicit declaration of function 'v4l2_ctrl_request_complete'; did you mean >> 'v4l2_ctrl_replace'? [-Werror=implicit-function-declaration] v4l2_ctrl_request_complete(vbuf->vb2_buf.req_obj.req, ^~ v4l2_ctrl_replace >> drivers/media//platform/sunxi/cedrus/sunxi_cedrus_video.c:437:43: error: >> 'struct vb2_buffer' has no member named 'req_obj' v4l2_ctrl_request_complete(vbuf->vb2_buf.req_obj.req, ^ drivers/media//platform/sunxi/cedrus/sunxi_cedrus_video.c: In function 'sunxi_cedrus_buf_request_complete': drivers/media//platform/sunxi/cedrus/sunxi_cedrus_video.c:455:31: error: 'struct vb2_buffer' has no member named 'req_obj' v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl); ^~ drivers/media//platform/sunxi/cedrus/sunxi_cedrus_video.c: At top level: >> drivers/media//platform/sunxi/cedrus/sunxi_cedrus_video.c:464:3: error: >> 'struct vb2_ops' has no member named 'buf_request_complete' .buf_request_complete = sunxi_cedrus_buf_request_complete, ^~~~ >> drivers/media//platform/sunxi/cedrus/sunxi_cedrus_video.c:464:26: warning: >> excess elements in struct initializer .buf_request_complete = sunxi_cedrus_buf_request_complete, ^ drivers/media//platform/sunxi/cedrus/sunxi_cedrus_video.c:464:26: note: (near initialization for 'sunxi_cedrus_qops') cc1: some warnings being treated as errors -- drivers/media//platform/sunxi/cedrus/sunxi_cedrus_dec.c: In function 'sunxi_cedrus_device_run': >> drivers/media//platform/sunxi/cedrus/sunxi_cedrus_dec.c:100:28: error: >> 'struct vb2_buffer' has no member named 'req_obj' src_req = run.src->vb2_buf.req_obj.req; ^ drivers/media//platform/sunxi/cedrus/sunxi_cedrus_dec.c:101:28: error: 'struct vb2_buffer' has no member named 'req_obj' dst_req = run.dst->vb2_buf.req_obj.req; ^ >> drivers/media//platform/sunxi/cedrus/sunxi_cedrus_dec.c:104:3: error: >> implicit declaration of function 'v4l2_ctrl_request_setup'; did you mean >> 'v4l2_ctrl_handler_setup'? [-Werror=implicit-function-declaration] v4l2_ctrl_request_setup(src_req, &ctx->hdl); ^~~ v4l2_ctrl_handler_setup >> drivers/media//platform/sunxi/cedrus/sunxi_cedrus_dec.c:138:3: error: >> implicit declaration of function 'v4l2_ctrl_request_complete'; did you mean >> 'v4l2_ctrl_replace'? [-Werror=implicit-function-declaration] v4l2_ctrl_request_complete(src_req, &ctx->hdl); ^~ v4l2_ctrl_replace cc1: some
Re: [PATCH v3 11/14] media: platform: Add Sunxi-Cedrus VPU decoder driver
On Mon, May 07, 2018 at 02:44:57PM +0200, Paul Kocialkowski wrote:
> +#define SYSCON_SRAM_CTRL_REG00x0
> +#define SYSCON_SRAM_C1_MAP_VE0x7fff
This isn't needed anymore
> + dev->ahb_clk = devm_clk_get(dev->dev, "ahb");
> + if (IS_ERR(dev->ahb_clk)) {
> + dev_err(dev->dev, "failed to get ahb clock\n");
> + return PTR_ERR(dev->ahb_clk);
> + }
> + dev->mod_clk = devm_clk_get(dev->dev, "mod");
> + if (IS_ERR(dev->mod_clk)) {
> + dev_err(dev->dev, "failed to get mod clock\n");
> + return PTR_ERR(dev->mod_clk);
> + }
> + dev->ram_clk = devm_clk_get(dev->dev, "ram");
> + if (IS_ERR(dev->ram_clk)) {
> + dev_err(dev->dev, "failed to get ram clock\n");
> + return PTR_ERR(dev->ram_clk);
> + }
Please add some blank lines between those blocks
> + dev->rstc = devm_reset_control_get(dev->dev, NULL);
You're not checking the error code here
> + dev->syscon = syscon_regmap_lookup_by_phandle(dev->dev->of_node,
> + "syscon");
> + if (IS_ERR(dev->syscon)) {
> + dev->syscon = NULL;
> + } else {
> + regmap_write_bits(dev->syscon, SYSCON_SRAM_CTRL_REG0,
> + SYSCON_SRAM_C1_MAP_VE,
> + SYSCON_SRAM_C1_MAP_VE);
> + }
You don't need the syscon part anymore either
> + ret = clk_prepare_enable(dev->ahb_clk);
> + if (ret) {
> + dev_err(dev->dev, "could not enable ahb clock\n");
> + return -EFAULT;
> + }
> + ret = clk_prepare_enable(dev->mod_clk);
> + if (ret) {
> + clk_disable_unprepare(dev->ahb_clk);
> + dev_err(dev->dev, "could not enable mod clock\n");
> + return -EFAULT;
> + }
> + ret = clk_prepare_enable(dev->ram_clk);
> + if (ret) {
> + clk_disable_unprepare(dev->mod_clk);
> + clk_disable_unprepare(dev->ahb_clk);
> + dev_err(dev->dev, "could not enable ram clock\n");
> + return -EFAULT;
> + }
> +
> + ret = reset_control_reset(dev->rstc);
> + if (ret) {
> + clk_disable_unprepare(dev->ram_clk);
> + clk_disable_unprepare(dev->mod_clk);
> + clk_disable_unprepare(dev->ahb_clk);
> + dev_err(dev->dev, "could not reset device\n");
> + return -EFAULT;
labels would simplify this greatly, and you should also release the
sram and the memory region here.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH v3 11/14] media: platform: Add Sunxi-Cedrus VPU decoder driver
On 07/05/18 14:44, Paul Kocialkowski wrote: > This introduces the Sunxi-Cedrus VPU driver that supports the VPU found > in Allwinner SoCs, also known as Video Engine. It is implemented through > a v4l2 m2m decoder device and a media device (used for media requests). > So far, it only supports MPEG2 decoding. > > Since this VPU is stateless, synchronization with media requests is > required in order to ensure consistency between frame headers that > contain metadata about the frame to process and the raw slice data that > is used to generate the frame. > > This driver was made possible thanks to the long-standing effort > carried out by the linux-sunxi community in the interest of reverse > engineering, documenting and implementing support for Allwinner VPU. > > Signed-off-by: Paul Kocialkowski > --- > MAINTAINERS| 7 + > drivers/media/platform/Kconfig | 15 + > drivers/media/platform/Makefile| 1 + > drivers/media/platform/sunxi/cedrus/Makefile | 4 + > drivers/media/platform/sunxi/cedrus/sunxi_cedrus.c | 333 ++ > .../platform/sunxi/cedrus/sunxi_cedrus_common.h| 128 ++ > .../media/platform/sunxi/cedrus/sunxi_cedrus_dec.c | 188 > .../media/platform/sunxi/cedrus/sunxi_cedrus_dec.h | 35 ++ > .../media/platform/sunxi/cedrus/sunxi_cedrus_hw.c | 240 ++ > .../media/platform/sunxi/cedrus/sunxi_cedrus_hw.h | 37 ++ > .../platform/sunxi/cedrus/sunxi_cedrus_mpeg2.c | 160 +++ > .../platform/sunxi/cedrus/sunxi_cedrus_mpeg2.h | 33 ++ > .../platform/sunxi/cedrus/sunxi_cedrus_regs.h | 175 +++ > .../platform/sunxi/cedrus/sunxi_cedrus_video.c | 505 > + > .../platform/sunxi/cedrus/sunxi_cedrus_video.h | 31 ++ > 15 files changed, 1892 insertions(+) > create mode 100644 drivers/media/platform/sunxi/cedrus/Makefile > create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus.c > create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_common.h > create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_dec.c > create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_dec.h > create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_hw.c > create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_hw.h > create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_mpeg2.c > create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_mpeg2.h > create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_regs.h > create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_video.c > create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_video.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 79bb02ff812f..489f1dccc810 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -656,6 +656,13 @@ L: linux-cry...@vger.kernel.org > S: Maintained > F: drivers/crypto/sunxi-ss/ > > +ALLWINNER VPU DRIVER > +M: Maxime Ripard > +M: Paul Kocialkowski > +L: linux-media@vger.kernel.org > +S: Maintained > +F: drivers/media/platform/sunxi/cedrus/ > + > ALPHA PORT > M: Richard Henderson > M: Ivan Kokshaysky > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > index 5af07b620094..72d37cd2f7a2 100644 > --- a/drivers/media/platform/Kconfig > +++ b/drivers/media/platform/Kconfig > @@ -476,6 +476,21 @@ config VIDEO_TI_VPE > Support for the TI VPE(Video Processing Engine) block > found on DRA7XX SoC. > > +config VIDEO_SUNXI_CEDRUS > + tristate "Sunxi-Cedrus VPU driver" > + depends on VIDEO_DEV && VIDEO_V4L2 && MEDIA_CONTROLLER > + depends on ARCH_SUNXI > + depends on HAS_DMA > + select VIDEOBUF2_DMA_CONTIG > + select MEDIA_REQUEST_API > + select V4L2_MEM2MEM_DEV > + ---help--- > + Support for the VPU found in Allwinner SoCs, also known as the Cedar > + video engine. > + > + To compile this driver as a module, choose M here: the module > + will be called sunxi-cedrus. > + > config VIDEO_TI_VPE_DEBUG > bool "VPE debug messages" > depends on VIDEO_TI_VPE > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile > index 932515df4477..444b995424a5 100644 > --- a/drivers/media/platform/Makefile > +++ b/drivers/media/platform/Makefile > @@ -69,6 +69,7 @@ obj-$(CONFIG_VIDEO_ROCKCHIP_RGA)+= rockchip/rga/ > obj-y+= omap/ > > obj-$(CONFIG_VIDEO_AM437X_VPFE) += am437x/ > +obj-$(CONFIG_VIDEO_SUNXI_CEDRUS) += sunxi/cedrus/ > > obj-$(CONFIG_VIDEO_XILINX) += xilinx/ > > diff --git a/drivers/media/platform/sunxi/cedrus/Makefile > b/drivers/media/platform/sunxi/cedrus/Makefile > new file mode 100644 > index ..98f30df626a9 > --- /dev/null > +++ b/drivers/media/platform/sunxi/cedrus/Makefile > @@ -0,0 +1,4 @@ > +obj-$(CONFIG_VIDEO_SUNXI_C
[PATCH v3 11/14] media: platform: Add Sunxi-Cedrus VPU decoder driver
This introduces the Sunxi-Cedrus VPU driver that supports the VPU found in Allwinner SoCs, also known as Video Engine. It is implemented through a v4l2 m2m decoder device and a media device (used for media requests). So far, it only supports MPEG2 decoding. Since this VPU is stateless, synchronization with media requests is required in order to ensure consistency between frame headers that contain metadata about the frame to process and the raw slice data that is used to generate the frame. This driver was made possible thanks to the long-standing effort carried out by the linux-sunxi community in the interest of reverse engineering, documenting and implementing support for Allwinner VPU. Signed-off-by: Paul Kocialkowski --- MAINTAINERS| 7 + drivers/media/platform/Kconfig | 15 + drivers/media/platform/Makefile| 1 + drivers/media/platform/sunxi/cedrus/Makefile | 4 + drivers/media/platform/sunxi/cedrus/sunxi_cedrus.c | 333 ++ .../platform/sunxi/cedrus/sunxi_cedrus_common.h| 128 ++ .../media/platform/sunxi/cedrus/sunxi_cedrus_dec.c | 188 .../media/platform/sunxi/cedrus/sunxi_cedrus_dec.h | 35 ++ .../media/platform/sunxi/cedrus/sunxi_cedrus_hw.c | 240 ++ .../media/platform/sunxi/cedrus/sunxi_cedrus_hw.h | 37 ++ .../platform/sunxi/cedrus/sunxi_cedrus_mpeg2.c | 160 +++ .../platform/sunxi/cedrus/sunxi_cedrus_mpeg2.h | 33 ++ .../platform/sunxi/cedrus/sunxi_cedrus_regs.h | 175 +++ .../platform/sunxi/cedrus/sunxi_cedrus_video.c | 505 + .../platform/sunxi/cedrus/sunxi_cedrus_video.h | 31 ++ 15 files changed, 1892 insertions(+) create mode 100644 drivers/media/platform/sunxi/cedrus/Makefile create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus.c create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_common.h create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_dec.c create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_dec.h create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_hw.c create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_hw.h create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_mpeg2.c create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_mpeg2.h create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_regs.h create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_video.c create mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_video.h diff --git a/MAINTAINERS b/MAINTAINERS index 79bb02ff812f..489f1dccc810 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -656,6 +656,13 @@ L: linux-cry...@vger.kernel.org S: Maintained F: drivers/crypto/sunxi-ss/ +ALLWINNER VPU DRIVER +M: Maxime Ripard +M: Paul Kocialkowski +L: linux-media@vger.kernel.org +S: Maintained +F: drivers/media/platform/sunxi/cedrus/ + ALPHA PORT M: Richard Henderson M: Ivan Kokshaysky diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig index 5af07b620094..72d37cd2f7a2 100644 --- a/drivers/media/platform/Kconfig +++ b/drivers/media/platform/Kconfig @@ -476,6 +476,21 @@ config VIDEO_TI_VPE Support for the TI VPE(Video Processing Engine) block found on DRA7XX SoC. +config VIDEO_SUNXI_CEDRUS + tristate "Sunxi-Cedrus VPU driver" + depends on VIDEO_DEV && VIDEO_V4L2 && MEDIA_CONTROLLER + depends on ARCH_SUNXI + depends on HAS_DMA + select VIDEOBUF2_DMA_CONTIG + select MEDIA_REQUEST_API + select V4L2_MEM2MEM_DEV + ---help--- + Support for the VPU found in Allwinner SoCs, also known as the Cedar + video engine. + + To compile this driver as a module, choose M here: the module + will be called sunxi-cedrus. + config VIDEO_TI_VPE_DEBUG bool "VPE debug messages" depends on VIDEO_TI_VPE diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile index 932515df4477..444b995424a5 100644 --- a/drivers/media/platform/Makefile +++ b/drivers/media/platform/Makefile @@ -69,6 +69,7 @@ obj-$(CONFIG_VIDEO_ROCKCHIP_RGA) += rockchip/rga/ obj-y += omap/ obj-$(CONFIG_VIDEO_AM437X_VPFE)+= am437x/ +obj-$(CONFIG_VIDEO_SUNXI_CEDRUS) += sunxi/cedrus/ obj-$(CONFIG_VIDEO_XILINX) += xilinx/ diff --git a/drivers/media/platform/sunxi/cedrus/Makefile b/drivers/media/platform/sunxi/cedrus/Makefile new file mode 100644 index ..98f30df626a9 --- /dev/null +++ b/drivers/media/platform/sunxi/cedrus/Makefile @@ -0,0 +1,4 @@ +obj-$(CONFIG_VIDEO_SUNXI_CEDRUS) += sunxi-cedrus.o + +sunxi-cedrus-y = sunxi_cedrus.o sunxi_cedrus_video.o sunxi_cedrus_hw.o \ +sunxi_cedrus_dec.o sunxi_cedrus_mpeg2.o diff --git a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus
