* [PATCH v4] Add v4l2 subdev driver for S5P MIPI-CSI receivers
@ 2011-04-21 15:21 Sylwester Nawrocki
2011-04-21 15:21 ` [PATCH 1/3] v4l: Add V4L2_MBUS_FMT_JPEG_1X8 media bus format Sylwester Nawrocki
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Sylwester Nawrocki @ 2011-04-21 15:21 UTC (permalink / raw)
To: linux-media
Cc: linux-samsung-soc, kyungmin.park, m.szyprowski, riverful.kim,
kgene.kim, sungchun.kang, jonghun.han
Hello,
this is a fourth version of the subdev driver for MIPI-CSI2 receivers
present on S5PVx10 and EXYNOS4 SoCs.
This patch set also adds a V4L2_MBUS_FMT_JPEG_1X8 media bus format
and moves the s5p-fimc driver to Video Capture Devices kconfig group.
I don't expect this driver to undergo any major changes and hope to have
it added in kernel 2.6.40.
Any comments are welcome!
Changes since v1:
- added runtime PM support
- conversion to the pad ops
Changes since v2:
- added reference counting in s_stream op to allow the mipi-csi subdev
to be shared by multiple FIMC instances
- added support for TRY format in pad get_fmt op
- added pm_runtime* calls in s_stream op to avoid a need for explicit
s_power(1) call
- corrected locking around the pad ops, minor bug fixes
Changes since v3:
- slighty reworked the power management part
- removed a reference counting in s_stream op as this should be handled
on a media device and the pipeline level
- s5p_csis_ prefix renamed to s5pcsis_
- updated the help text in Kconfig
[PATCH 1/3] v4l: Add V4L2_MBUS_FMT_JPEG_1X8 media bus format
[PATCH v4 2/3] v4l: Move S5P FIMC driver into Video Capture Devices
[PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI Receiver
Documentation/DocBook/v4l/subdev-formats.xml | 46 ++
drivers/media/video/Kconfig | 28 +-
drivers/media/video/s5p-fimc/Makefile | 6 +-
drivers/media/video/s5p-fimc/mipi-csis.c | 745 ++++++++++++++++++++++++++
drivers/media/video/s5p-fimc/mipi-csis.h | 22 +
include/linux/v4l2-mediabus.h | 3 +
6 files changed, 840 insertions(+), 10 deletions(-)
--
Regards,
Sylwester Nawrocki
Samsung Poland R&D Center
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] v4l: Add V4L2_MBUS_FMT_JPEG_1X8 media bus format 2011-04-21 15:21 [PATCH v4] Add v4l2 subdev driver for S5P MIPI-CSI receivers Sylwester Nawrocki @ 2011-04-21 15:21 ` Sylwester Nawrocki 2011-04-21 15:21 ` [PATCH v4 2/3] v4l: Move S5P FIMC driver into Video Capture Devices Sylwester Nawrocki 2011-04-21 15:21 ` [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers Sylwester Nawrocki 2 siblings, 0 replies; 11+ messages in thread From: Sylwester Nawrocki @ 2011-04-21 15:21 UTC (permalink / raw) To: linux-media Cc: linux-samsung-soc, kyungmin.park, m.szyprowski, riverful.kim, kgene.kim, sungchun.kang, jonghun.han, Sylwester Nawrocki Add V4L2_MBUS_FMT_JPEG_1X8 format and the corresponding Docbook documentation. Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Documentation/DocBook/v4l/subdev-formats.xml | 46 ++++++++++++++++++++++++++ include/linux/v4l2-mediabus.h | 3 ++ 2 files changed, 49 insertions(+), 0 deletions(-) diff --git a/Documentation/DocBook/v4l/subdev-formats.xml b/Documentation/DocBook/v4l/subdev-formats.xml index 7041127..df2d7d3 100644 --- a/Documentation/DocBook/v4l/subdev-formats.xml +++ b/Documentation/DocBook/v4l/subdev-formats.xml @@ -2463,5 +2463,51 @@ </tgroup> </table> </section> + + <section> + <title>JPEG Compressed Formats</title> + + <para>Those data formats consist of an ordered sequence of 8-bit bytes + obtained from JPEG compression process. Additionally to the + <constant>_JPEG</constant> prefix the format code is made of + the following information. + <itemizedlist> + <listitem>The number of bus samples per entropy encoded byte.</listitem> + <listitem>The bus width.</listitem> + </itemizedlist> + + <para>For instance, for a JPEG baseline process and an 8-bit bus width + the format will be named <constant>V4L2_MBUS_FMT_JPEG_1X8</constant>. + </para> + </para> + + <para>The following table lists existing JPEG compressed formats.</para> + + <table pgwide="0" frame="none" id="v4l2-mbus-pixelcode-jpeg"> + <title>JPEG Formats</title> + <tgroup cols="3"> + <colspec colname="id" align="left" /> + <colspec colname="code" align="left"/> + <colspec colname="remarks" align="left"/> + <thead> + <row> + <entry>Identifier</entry> + <entry>Code</entry> + <entry>Remarks</entry> + </row> + </thead> + <tbody valign="top"> + <row id="V4L2-MBUS-FMT-JPEG-1X8"> + <entry>V4L2_MBUS_FMT_JPEG_1X8</entry> + <entry>0x4001</entry> + <entry>Besides of its usage for the parallel bus this format is + recommended for transmission of JPEG data over MIPI CSI bus + using the User Defined 8-bit Data types. + </entry> + </row> + </tbody> + </tgroup> + </table> + </section> </section> </section> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h index 7054a7a..15d6cda 100644 --- a/include/linux/v4l2-mediabus.h +++ b/include/linux/v4l2-mediabus.h @@ -86,6 +86,9 @@ enum v4l2_mbus_pixelcode { V4L2_MBUS_FMT_SGBRG12_1X12 = 0x3010, V4L2_MBUS_FMT_SGRBG12_1X12 = 0x3011, V4L2_MBUS_FMT_SRGGB12_1X12 = 0x3012, + + /* JPEG compressed formats - next is 0x4002 */ + V4L2_MBUS_FMT_JPEG_1X8 = 0x4001, }; /** -- 1.7.4.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v4 2/3] v4l: Move S5P FIMC driver into Video Capture Devices 2011-04-21 15:21 [PATCH v4] Add v4l2 subdev driver for S5P MIPI-CSI receivers Sylwester Nawrocki 2011-04-21 15:21 ` [PATCH 1/3] v4l: Add V4L2_MBUS_FMT_JPEG_1X8 media bus format Sylwester Nawrocki @ 2011-04-21 15:21 ` Sylwester Nawrocki 2011-05-03 8:24 ` Laurent Pinchart 2011-04-21 15:21 ` [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers Sylwester Nawrocki 2 siblings, 1 reply; 11+ messages in thread From: Sylwester Nawrocki @ 2011-04-21 15:21 UTC (permalink / raw) To: linux-media Cc: linux-samsung-soc, kyungmin.park, m.szyprowski, riverful.kim, kgene.kim, sungchun.kang, jonghun.han, Sylwester Nawrocki s5p-fimc now also implements a camera capture video node so move it under the "Video capture devices" Kconfig menu. Also update the entry to reflect the driver's coverage of EXYNOS4 SoCs. Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> --- drivers/media/video/Kconfig | 19 +++++++++++-------- 1 files changed, 11 insertions(+), 8 deletions(-) diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 4498b94..4f0ac2d 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -927,6 +927,17 @@ config VIDEO_MX2 This is a v4l2 driver for the i.MX27 and the i.MX25 Camera Sensor Interface +config VIDEO_SAMSUNG_S5P_FIMC + tristate "S5P and EXYNOS4 camera host interface driver" + depends on VIDEO_DEV && VIDEO_V4L2 && PLAT_S5P + select VIDEOBUF2_DMA_CONTIG + select V4L2_MEM2MEM_DEV + help + This is a v4l2 driver for the S5P and EXYNOS4 camera host interface + and video postprocessor. + + To compile this driver as a module, choose M here: the + module will be called s5p-fimc. # # USB Multimedia device configuration @@ -1022,13 +1033,5 @@ config VIDEO_MEM2MEM_TESTDEV This is a virtual test device for the memory-to-memory driver framework. -config VIDEO_SAMSUNG_S5P_FIMC - tristate "Samsung S5P FIMC (video postprocessor) driver" - depends on VIDEO_DEV && VIDEO_V4L2 && PLAT_S5P - select VIDEOBUF2_DMA_CONTIG - select V4L2_MEM2MEM_DEV - help - This is a v4l2 driver for the S5P camera interface - (video postprocessor) endif # V4L_MEM2MEM_DRIVERS -- 1.7.4.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/3] v4l: Move S5P FIMC driver into Video Capture Devices 2011-04-21 15:21 ` [PATCH v4 2/3] v4l: Move S5P FIMC driver into Video Capture Devices Sylwester Nawrocki @ 2011-05-03 8:24 ` Laurent Pinchart 0 siblings, 0 replies; 11+ messages in thread From: Laurent Pinchart @ 2011-05-03 8:24 UTC (permalink / raw) To: Sylwester Nawrocki Cc: linux-media, linux-samsung-soc, kyungmin.park, m.szyprowski, riverful.kim, kgene.kim, sungchun.kang, jonghun.han Hi Sylwester, Thanks for the patch. On Thursday 21 April 2011 17:21:03 Sylwester Nawrocki wrote: > s5p-fimc now also implements a camera capture video node so move > it under the "Video capture devices" Kconfig menu. Also update > the entry to reflect the driver's coverage of EXYNOS4 SoCs. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > drivers/media/video/Kconfig | 19 +++++++++++-------- > 1 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig > index 4498b94..4f0ac2d 100644 > --- a/drivers/media/video/Kconfig > +++ b/drivers/media/video/Kconfig > @@ -927,6 +927,17 @@ config VIDEO_MX2 > This is a v4l2 driver for the i.MX27 and the i.MX25 Camera Sensor > Interface > > +config VIDEO_SAMSUNG_S5P_FIMC > + tristate "S5P and EXYNOS4 camera host interface driver" Should this read "Samsung S5P and EXYNOS4" ? Without the brand name kernel config entries are a bit cryptic. > + depends on VIDEO_DEV && VIDEO_V4L2 && PLAT_S5P > + select VIDEOBUF2_DMA_CONTIG > + select V4L2_MEM2MEM_DEV > + help > + This is a v4l2 driver for the S5P and EXYNOS4 camera host interface > + and video postprocessor. > + > + To compile this driver as a module, choose M here: the > + module will be called s5p-fimc. > > # > # USB Multimedia device configuration > @@ -1022,13 +1033,5 @@ config VIDEO_MEM2MEM_TESTDEV > This is a virtual test device for the memory-to-memory driver > framework. > > -config VIDEO_SAMSUNG_S5P_FIMC > - tristate "Samsung S5P FIMC (video postprocessor) driver" > - depends on VIDEO_DEV && VIDEO_V4L2 && PLAT_S5P > - select VIDEOBUF2_DMA_CONTIG > - select V4L2_MEM2MEM_DEV > - help > - This is a v4l2 driver for the S5P camera interface > - (video postprocessor) > > endif # V4L_MEM2MEM_DRIVERS -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers 2011-04-21 15:21 [PATCH v4] Add v4l2 subdev driver for S5P MIPI-CSI receivers Sylwester Nawrocki 2011-04-21 15:21 ` [PATCH 1/3] v4l: Add V4L2_MBUS_FMT_JPEG_1X8 media bus format Sylwester Nawrocki 2011-04-21 15:21 ` [PATCH v4 2/3] v4l: Move S5P FIMC driver into Video Capture Devices Sylwester Nawrocki @ 2011-04-21 15:21 ` Sylwester Nawrocki 2011-05-03 9:16 ` Laurent Pinchart 2 siblings, 1 reply; 11+ messages in thread From: Sylwester Nawrocki @ 2011-04-21 15:21 UTC (permalink / raw) To: linux-media Cc: linux-samsung-soc, kyungmin.park, m.szyprowski, riverful.kim, kgene.kim, sungchun.kang, jonghun.han, Sylwester Nawrocki Add the subdev driver for the MIPI CSIS units available in S5P and Exynos4 SoC series. This driver supports both CSIS0 and CSIS1 MIPI-CSI2 receivers. The driver requires Runtime PM to be enabled for proper operation. Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> --- drivers/media/video/Kconfig | 9 + drivers/media/video/s5p-fimc/Makefile | 6 +- drivers/media/video/s5p-fimc/mipi-csis.c | 745 ++++++++++++++++++++++++++++++ drivers/media/video/s5p-fimc/mipi-csis.h | 22 + 4 files changed, 780 insertions(+), 2 deletions(-) create mode 100644 drivers/media/video/s5p-fimc/mipi-csis.c create mode 100644 drivers/media/video/s5p-fimc/mipi-csis.h diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 4f0ac2d..1da961a 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -939,6 +939,15 @@ config VIDEO_SAMSUNG_S5P_FIMC To compile this driver as a module, choose M here: the module will be called s5p-fimc. +config VIDEO_S5P_MIPI_CSIS + tristate "S5P and EXYNOS4 MIPI CSI Receiver driver" + depends on VIDEO_V4L2 && VIDEO_SAMSUNG_S5P_FIMC && MEDIA_CONTROLLER + ---help--- + This is a v4l2 driver for the S5P/EXYNOS4 MIPI-CSI Receiver. + + To compile this driver as a module, choose M here: the + module will be called s5p-csis. + # # USB Multimedia device configuration # diff --git a/drivers/media/video/s5p-fimc/Makefile b/drivers/media/video/s5p-fimc/Makefile index 7ea1b14..df6954a 100644 --- a/drivers/media/video/s5p-fimc/Makefile +++ b/drivers/media/video/s5p-fimc/Makefile @@ -1,3 +1,5 @@ +s5p-fimc-objs := fimc-core.o fimc-reg.o fimc-capture.o +s5p-csis-objs := mipi-csis.o -obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) := s5p-fimc.o -s5p-fimc-y := fimc-core.o fimc-reg.o fimc-capture.o +obj-$(CONFIG_VIDEO_S5P_MIPI_CSIS) += s5p-csis.o +obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc.o diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c b/drivers/media/video/s5p-fimc/mipi-csis.c new file mode 100644 index 0000000..6219754 --- /dev/null +++ b/drivers/media/video/s5p-fimc/mipi-csis.c @@ -0,0 +1,745 @@ +/* + * Samsung S5P SoC series MIPI-CSI receiver driver + * + * Copyright (C) 2011 Samsung Electronics Co., Ltd. + * Contact: Sylwester Nawrocki, <s.nawrocki@samsung.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/errno.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/kernel.h> +#include <linux/memory.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/regulator/consumer.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/videodev2.h> +#include <media/v4l2-subdev.h> +#include <plat/mipi_csis.h> +#include "mipi-csis.h" + +static int debug; +module_param(debug, int, 0644); +MODULE_PARM_DESC(debug, "Debug level (0-1)"); + +/* Register map definition */ + +/* CSIS global control */ +#define S5PCSIS_CTRL 0x00 +#define S5PCSIS_CTRL_DPDN_DEFAULT (0 << 31) +#define S5PCSIS_CTRL_DPDN_SWAP (1 << 31) +#define S5PCSIS_CTRL_ALIGN_32BIT (1 << 20) +#define S5PCSIS_CTRL_UPDATE_SHADOW (1 << 16) +#define S5PCSIS_CTRL_WCLK_EXTCLK (1 << 8) +#define S5PCSIS_CTRL_RESET (1 << 4) +#define S5PCSIS_CTRL_ENABLE (1 << 0) + +/* D-PHY control */ +#define S5PCSIS_DPHYCTRL 0x04 +#define S5PCSIS_DPHYCTRL_HSS_MASK (0x1f << 27) +#define S5PCSIS_DPHYCTRL_ENABLE (0x1f << 0) + +#define S5PCSIS_CONFIG 0x08 +#define S5PCSIS_CFG_FMT_YCBCR422_8BIT (0x1e << 2) +#define S5PCSIS_CFG_FMT_RAW8 (0x2a << 2) +#define S5PCSIS_CFG_FMT_RAW10 (0x2b << 2) +#define S5PCSIS_CFG_FMT_RAW12 (0x2c << 2) +/* User defined formats, x = 1...4 */ +#define S5PCSIS_CFG_FMT_USER(x) ((0x30 + x - 1) << 2) +#define S5PCSIS_CFG_FMT_MASK (0x3f << 2) +#define S5PCSIS_CFG_NR_LANE_MASK 3 + +/* Interrupt mask. */ +#define S5PCSIS_INTMSK 0x10 +#define S5PCSIS_INTMSK_EN_ALL 0xf000003f +#define S5PCSIS_INTSRC 0x14 + +/* Pixel resolution */ +#define S5PCSIS_RESOL 0x2c +#define CSIS_MAX_PIX_WIDTH 0xffff +#define CSIS_MAX_PIX_HEIGHT 0xffff + +enum { + CSIS_CLK_MUX, + CSIS_CLK_GATE, +}; + +static char *csi_clock_name[] = { + [CSIS_CLK_MUX] = "sclk_csis", + [CSIS_CLK_GATE] = "csis", +}; +#define NUM_CSIS_CLOCKS ARRAY_SIZE(csi_clock_name) + +enum { + ST_POWERED = 1, + ST_STREAMING = 2, + ST_SUSPENDED = 4, +}; + +enum { + CSIS_FMT_TRY, + CSIS_FMT_ACTIVE, + CSIS_NUM_FMTS +}; + +struct csis_state { + struct mutex lock; + struct media_pad pads[CSIS_PADS_NUM]; + struct v4l2_subdev sd; + struct platform_device *pdev; + struct resource *regs_res; + void __iomem *regs; + struct clk *clock[NUM_CSIS_CLOCKS]; + int irq; + struct regulator *supply; + u32 flags; + /* Common format for the source and sink pad. */ + const struct csis_pix_format *csis_fmt; + struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS]; +}; + +struct csis_pix_format { + enum v4l2_mbus_pixelcode code; + u32 fmt_reg; + u16 pix_hor_align; +}; + +static const struct csis_pix_format s5pcsis_formats[] = { + { + .code = V4L2_MBUS_FMT_VYUY8_2X8, + .fmt_reg = S5PCSIS_CFG_FMT_YCBCR422_8BIT, + .pix_hor_align = 1, + }, + { + .code = V4L2_MBUS_FMT_JPEG_1X8, + .fmt_reg = S5PCSIS_CFG_FMT_USER(1), + .pix_hor_align = 1, + }, +}; + +#define csis_pad_valid(pad) (pad == CSIS_PAD_SOURCE || pad == CSIS_PAD_SINK) + +static struct csis_state *sd_to_csis_state(struct v4l2_subdev *sdev) +{ + return container_of(sdev, struct csis_state, sd); +} + +static const struct csis_pix_format *find_csis_format( + struct v4l2_mbus_framefmt *mf) +{ + int i = ARRAY_SIZE(s5pcsis_formats); + + while (--i >= 0) + if (mf->code == s5pcsis_formats[i].code) + return &s5pcsis_formats[i]; + + return NULL; +} + +static void s5pcsis_enable_interrupts(struct csis_state *state, bool on) +{ + u32 reg = readl(state->regs + S5PCSIS_CTRL); + + if (on) + reg |= S5PCSIS_INTMSK_EN_ALL; + else + reg &= ~S5PCSIS_INTMSK_EN_ALL; + writel(reg, state->regs + S5PCSIS_INTMSK); +} + +static void s5pcsis_reset(struct csis_state *state) +{ + u32 reg = readl(state->regs + S5PCSIS_CTRL); + + writel(reg | S5PCSIS_CTRL_RESET, state->regs + S5PCSIS_CTRL); + udelay(10); +} + +static void s5pcsis_system_enable(struct csis_state *state, int on) +{ + u32 reg; + + reg = readl(state->regs + S5PCSIS_CTRL); + if (on) + reg |= S5PCSIS_CTRL_ENABLE; + else + reg &= ~S5PCSIS_CTRL_ENABLE; + writel(reg, state->regs + S5PCSIS_CTRL); + + reg = readl(state->regs + S5PCSIS_DPHYCTRL); + if (on) + reg |= S5PCSIS_DPHYCTRL_ENABLE; + else + reg &= ~S5PCSIS_DPHYCTRL_ENABLE; + writel(reg, state->regs + S5PCSIS_DPHYCTRL); +} + +static int __s5pcsis_set_format(struct csis_state *state) +{ + struct v4l2_mbus_framefmt *mf = &state->mf[CSIS_FMT_ACTIVE]; + u32 reg; + + v4l2_dbg(1, debug, &state->sd, "fmt: %d, %d x %d\n", + mf->code, mf->width, mf->height); + + if (WARN_ON(state->csis_fmt == NULL)) + return -EINVAL; + + /* Color format */ + reg = readl(state->regs + S5PCSIS_CONFIG); + reg = (reg & ~S5PCSIS_CFG_FMT_MASK) | state->csis_fmt->fmt_reg; + writel(reg, state->regs + S5PCSIS_CONFIG); + + /* Pixel resolution */ + reg = (mf->width << 16) | mf->height; + writel(reg, state->regs + S5PCSIS_RESOL); + + return 0; +} + +static void s5pcsis_set_hsync_settle(struct csis_state *state, + int settle) +{ + u32 reg = readl(state->regs + S5PCSIS_DPHYCTRL); + + reg &= ~S5PCSIS_DPHYCTRL_HSS_MASK; + reg |= (settle << 27); + writel(reg, state->regs + S5PCSIS_DPHYCTRL); +} + +static int s5pcsis_set_params(struct csis_state *state) +{ + struct s5p_platform_mipi_csis *pdata = state->pdev->dev.platform_data; + u32 reg, tmp; + int ret; + + reg = readl(state->regs + S5PCSIS_CONFIG); + reg &= ~S5PCSIS_CFG_NR_LANE_MASK; + tmp = (pdata->lanes - 1) & 0x3; + writel(reg | tmp, state->regs + S5PCSIS_CONFIG); + + ret = __s5pcsis_set_format(state); + if (ret) + return ret; + + s5pcsis_set_hsync_settle(state, pdata->hs_settle); + + reg = readl(state->regs + S5PCSIS_CTRL); + + if (pdata->alignment == 32) + reg |= S5PCSIS_CTRL_ALIGN_32BIT; + else /* 24-bits */ + reg &= ~S5PCSIS_CTRL_ALIGN_32BIT; + + /* Not using external clock. */ + reg &= ~S5PCSIS_CTRL_WCLK_EXTCLK; + + writel(reg, state->regs + S5PCSIS_CTRL); + + /* Update the shadow register. */ + reg = readl(state->regs + S5PCSIS_CTRL); + writel(reg | S5PCSIS_CTRL_UPDATE_SHADOW, + state->regs + S5PCSIS_CTRL); + + return 0; +} + +static void s5pcsis_clk_put(struct csis_state *state) +{ + int i; + + for (i = 0; i < NUM_CSIS_CLOCKS; i++) + if (!IS_ERR(state->clock[i])) + clk_put(state->clock[i]); +} + +static int s5pcsis_clk_get(struct csis_state *state) +{ + struct device *dev = &state->pdev->dev; + int i; + + for (i = 0; i < NUM_CSIS_CLOCKS; i++) { + state->clock[i] = clk_get(dev, csi_clock_name[i]); + + if (IS_ERR(state->clock[i])) { + s5pcsis_clk_put(state); + dev_err(dev, "failed to get clock: %s\n", + csi_clock_name[i]); + return -ENXIO; + } + } + return 0; +} + +static int s5pcsis_s_power(struct v4l2_subdev *sd, int on) +{ + struct csis_state *state = sd_to_csis_state(sd); + struct device *dev = &state->pdev->dev; + + v4l2_dbg(1, debug, sd, "%s: on: %d, flags: 0x%x\n", + __func__, on, state->flags); + + if (on) + return pm_runtime_get_sync(dev); + + return pm_runtime_put_sync(dev); +} + +static int s5pcsis_start_stream(struct csis_state *state) +{ + int ret; + + s5pcsis_reset(state); + ret = s5pcsis_set_params(state); + if (ret) + return ret; + + s5pcsis_system_enable(state, true); + s5pcsis_enable_interrupts(state, true); + + return 0; +} + +static void s5pcsis_stop_stream(struct csis_state *state) +{ + s5pcsis_enable_interrupts(state, false); + s5pcsis_system_enable(state, false); +} + +/* Subdev operations */ +static int s5pcsis_s_stream(struct v4l2_subdev *sd, int enable) +{ + struct csis_state *state = sd_to_csis_state(sd); + int ret = 0; + + v4l2_dbg(1, debug, sd, "%s: %d, state: 0x%x\n", + __func__, enable, state->flags); + + if (enable) { + ret = pm_runtime_get_sync(&state->pdev->dev); + if (ret && ret != 1) + return ret; + } + + mutex_lock(&state->lock); + if (enable) { + if (state->flags & ST_SUSPENDED) { + ret = -EBUSY; + goto unlock; + } + ret = s5pcsis_start_stream(state); + if (!ret) + state->flags |= ST_STREAMING; + } else { + s5pcsis_stop_stream(state); + state->flags &= ~ST_STREAMING; + } +unlock: + mutex_unlock(&state->lock); + if (!enable) + pm_runtime_put(&state->pdev->dev); + return ret; +} + +static int s5pcsis_enum_mbus_code(struct v4l2_subdev *sd, + struct v4l2_subdev_fh *fh, + struct v4l2_subdev_mbus_code_enum *code) +{ + if (code->index >= ARRAY_SIZE(s5pcsis_formats)) + return -EINVAL; + + code->code = s5pcsis_formats[code->index].code; + return 0; +} + +static void s5pcsis_try_format(struct v4l2_mbus_framefmt *mf) +{ + struct csis_pix_format const *csis_fmt; + + csis_fmt = find_csis_format(mf); + if (csis_fmt == NULL) + csis_fmt = &s5pcsis_formats[0]; + + mf->code = csis_fmt->code; + v4l_bound_align_image(&mf->width, 1, CSIS_MAX_PIX_WIDTH, + csis_fmt->pix_hor_align, + &mf->height, 1, CSIS_MAX_PIX_HEIGHT, 1, + 0); +} + +static int s5pcsis_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, + struct v4l2_subdev_format *fmt) +{ + struct csis_state *state = sd_to_csis_state(sd); + struct v4l2_mbus_framefmt *mf = &fmt->format; + struct csis_pix_format const *csis_fmt = find_csis_format(mf); + + v4l2_dbg(1, debug, sd, "%s: %dx%d, code: %x, csis_fmt: %p\n", + __func__, mf->width, mf->height, mf->code, csis_fmt); + + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { + s5pcsis_try_format(mf); + state->mf[CSIS_FMT_TRY] = *mf; + return 0; + } + + /* Both source and sink pad have always same format. */ + if (!csis_pad_valid(fmt->pad) || + csis_fmt == NULL || + mf->width > CSIS_MAX_PIX_WIDTH || + mf->height > CSIS_MAX_PIX_HEIGHT || + mf->width & (u32)(csis_fmt->pix_hor_align - 1)) + return -EINVAL; + + mutex_lock(&state->lock); + state->mf[CSIS_FMT_ACTIVE] = *mf; + state->csis_fmt = csis_fmt; + mutex_unlock(&state->lock); + + return 0; +} + +static int s5pcsis_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, + struct v4l2_subdev_format *fmt) +{ + struct csis_state *state = sd_to_csis_state(sd); + int index = fmt->which == V4L2_SUBDEV_FORMAT_TRY ? + CSIS_FMT_TRY : CSIS_FMT_ACTIVE; + + if (!csis_pad_valid(fmt->pad)) + return -EINVAL; + + mutex_lock(&state->lock); + fmt->format = state->mf[index]; + mutex_unlock(&state->lock); + + return 0; +} + +static struct v4l2_subdev_core_ops s5pcsis_core_ops = { + .s_power = s5pcsis_s_power, +}; + +static struct v4l2_subdev_pad_ops s5pcsis_pad_ops = { + .enum_mbus_code = s5pcsis_enum_mbus_code, + .get_fmt = s5pcsis_get_fmt, + .set_fmt = s5pcsis_set_fmt, +}; + +static struct v4l2_subdev_video_ops s5pcsis_video_ops = { + .s_stream = s5pcsis_s_stream, +}; + +static struct v4l2_subdev_ops s5pcsis_subdev_ops = { + .core = &s5pcsis_core_ops, + .pad = &s5pcsis_pad_ops, + .video = &s5pcsis_video_ops, +}; + +/* Media operations */ +static int csis_link_setup(struct media_entity *entity, + const struct media_pad *local, + const struct media_pad *remote, u32 flags) +{ + v4l2_dbg(1, debug, entity, "%s: entity: %s, flags: 0x%x\n", + __func__, entity->name, flags); + + return 0; +} + +static const struct media_entity_operations csis_media_ops = { + .link_setup = csis_link_setup, +}; + +static irqreturn_t s5pcsis_irq_handler(int irq, void *dev_id) +{ + struct csis_state *state = dev_id; + u32 reg; + + /* Just clear the interrupt pending bits. */ + reg = readl(state->regs + S5PCSIS_INTSRC); + writel(reg, state->regs + S5PCSIS_INTSRC); + + return IRQ_HANDLED; +} + +static int s5pcsis_probe(struct platform_device *pdev) +{ + struct s5p_platform_mipi_csis *pdata; + struct resource *mem_res; + struct resource *regs_res; + struct csis_state *state; + struct media_entity *entity; + int ret = -ENOMEM; + + state = kzalloc(sizeof(*state), GFP_KERNEL); + if (!state) + return -ENOMEM; + + mutex_init(&state->lock); + state->pdev = pdev; + + pdata = pdev->dev.platform_data; + if (pdata == NULL || pdata->phy_enable == NULL) { + dev_err(&pdev->dev, "Platform data not fully specified\n"); + goto e_free; + } + + if ((pdev->id == 1 && pdata->lanes > CSIS1_MAX_LANES) || + pdata->lanes > CSIS0_MAX_LANES) { + ret = -EINVAL; + dev_err(&pdev->dev, "Unsupported number of data lanes: %d\n", + pdata->lanes); + goto e_free; + } + + mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!mem_res) { + dev_err(&pdev->dev, "Failed to get IO memory region\n"); + goto e_free; + } + + regs_res = request_mem_region(mem_res->start, resource_size(mem_res), + pdev->name); + if (!regs_res) { + dev_err(&pdev->dev, "Failed to request IO memory region\n"); + goto e_free; + } + state->regs_res = regs_res; + + state->regs = ioremap(mem_res->start, resource_size(mem_res)); + if (!state->regs) { + dev_err(&pdev->dev, "Failed to remap IO region\n"); + goto e_reqmem; + } + + ret = s5pcsis_clk_get(state) + if (ret) + goto e_unmap; + + clk_enable(state->clock[CSIS_CLK_MUX]); + if (pdata->clk_rate) + clk_set_rate(state->clock[CSIS_CLK_MUX], pdata->clk_rate); + else + dev_WARN(&pdev->dev, "No clock frequency specified!\n"); + + state->irq = platform_get_irq(pdev, 0); + if (state->irq < 0) { + ret = state->irq; + dev_err(&pdev->dev, "Failed to get irq\n"); + goto e_clkput; + } + + if (!pdata->fixed_phy_vdd) { + state->supply = regulator_get(&pdev->dev, "vdd"); + if (IS_ERR(state->supply)) { + ret = PTR_ERR(state->supply); + state->supply = NULL; + goto e_clkput; + } + } + + ret = request_irq(state->irq, s5pcsis_irq_handler, 0, + dev_name(&pdev->dev), state); + if (ret) { + dev_err(&pdev->dev, "request_irq failed\n"); + goto e_regput; + } + + v4l2_subdev_init(&state->sd, &s5pcsis_subdev_ops); + state->sd.owner = THIS_MODULE; + strlcpy(state->sd.name, dev_name(&pdev->dev), sizeof(state->sd.name)); + + entity = &state->sd.entity; + state->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK; + state->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; + ret = media_entity_init(&state->sd.entity, + CSIS_PADS_NUM, state->pads, 0); + if (ret < 0) + goto e_irqfree; + + /* This allows to retrieve the platform device id by the host driver */ + v4l2_set_subdevdata(&state->sd, pdev); + + /* .. and a pointer to the subdev. */ + platform_set_drvdata(pdev, &state->sd); + + state->flags = ST_SUSPENDED; + pm_runtime_enable(&pdev->dev); + + return 0; + +e_irqfree: + free_irq(state->irq, state); +e_regput: + if (state->supply) + regulator_put(state->supply); +e_clkput: + clk_disable(state->clock[CSIS_CLK_MUX]); + s5pcsis_clk_put(state); +e_unmap: + iounmap(state->regs); +e_reqmem: + release_mem_region(regs_res->start, resource_size(regs_res)); +e_free: + kfree(state); + return ret; +} + +static int s5pcsis_suspend(struct device *dev) +{ + struct s5p_platform_mipi_csis *pdata = dev->platform_data; + struct platform_device *pdev = to_platform_device(dev); + struct v4l2_subdev *sd = platform_get_drvdata(pdev); + struct csis_state *state = sd_to_csis_state(sd); + int ret; + + v4l2_dbg(1, debug, sd, "%s: flags: 0x%x\n", + __func__, state->flags); + + mutex_lock(&state->lock); + if (state->flags & ST_POWERED) { + s5pcsis_stop_stream(state); + ret = pdata->phy_enable(state->pdev, false); + if (ret) + goto unlock; + + if (state->supply && regulator_disable(state->supply)) + goto unlock; + + clk_disable(state->clock[CSIS_CLK_GATE]); + state->flags &= ~ST_POWERED; + } + state->flags |= ST_SUSPENDED; + unlock: + mutex_unlock(&state->lock); + return ret ? -EAGAIN : 0; +} + +static int s5pcsis_resume(struct device *dev) +{ + struct s5p_platform_mipi_csis *pdata = dev->platform_data; + struct platform_device *pdev = to_platform_device(dev); + struct v4l2_subdev *sd = platform_get_drvdata(pdev); + struct csis_state *state = sd_to_csis_state(sd); + int ret = 0; + + v4l2_dbg(1, debug, sd, "%s: flags: 0x%x\n", + __func__, state->flags); + + mutex_lock(&state->lock); + if (!(state->flags & ST_SUSPENDED)) + goto unlock; + + if (!(state->flags & ST_POWERED)) { + if (state->supply) + ret = regulator_enable(state->supply); + if (ret) + goto unlock; + + ret = pdata->phy_enable(state->pdev, true); + if (!ret) { + state->flags |= ST_POWERED; + } else { + regulator_disable(state->supply); + goto unlock; + } + clk_enable(state->clock[CSIS_CLK_GATE]); + } + if (state->flags & ST_STREAMING) + s5pcsis_start_stream(state); + + state->flags &= ~ST_SUSPENDED; + unlock: + mutex_unlock(&state->lock); + return ret ? -EAGAIN : 0; +} + +#ifdef CONFIG_PM_SLEEP +static int s5pcsis_pm_suspend(struct device *dev) +{ + return s5pcsis_suspend(dev); +} + +static int s5pcsis_pm_resume(struct device *dev) +{ + int ret; + + ret = s5pcsis_resume(dev); + + if (!ret) { + pm_runtime_disable(dev); + ret = pm_runtime_set_active(dev); + pm_runtime_enable(dev); + } + + return ret; +} +#endif + +static int s5pcsis_remove(struct platform_device *pdev) +{ + struct v4l2_subdev *sd = platform_get_drvdata(pdev); + struct csis_state *state = sd_to_csis_state(sd); + struct resource *res = state->regs_res; + + pm_runtime_disable(&pdev->dev); + s5pcsis_suspend(&pdev->dev); + clk_disable(state->clock[CSIS_CLK_MUX]); + pm_runtime_set_suspended(&pdev->dev); + + s5pcsis_clk_put(state); + if (state->supply) + regulator_put(state->supply); + + media_entity_cleanup(&state->sd.entity); + free_irq(state->irq, state); + iounmap(state->regs); + release_mem_region(res->start, resource_size(res)); + kfree(state); + + return 0; +} + +static const struct dev_pm_ops s5pcsis_pm_ops = { + SET_RUNTIME_PM_OPS(s5pcsis_suspend, s5pcsis_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(s5pcsis_pm_suspend, s5pcsis_pm_resume) +}; + +static struct platform_driver s5pcsis_driver = { + .probe = s5pcsis_probe, + .remove = s5pcsis_remove, + .driver = { + .name = CSIS_MODULE_NAME, + .owner = THIS_MODULE, + .pm = &s5pcsis_pm_ops, + }, +}; + +static int __init s5pcsis_init(void) +{ + return platform_driver_probe(&s5pcsis_driver, s5pcsis_probe); +} + +static void __exit s5pcsis_exit(void) +{ + platform_driver_unregister(&s5pcsis_driver); +} + +module_init(s5pcsis_init); +module_exit(s5pcsis_exit); + +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>"); +MODULE_DESCRIPTION("S5P/EXYNOS4 MIPI CSI receiver driver"); +MODULE_LICENSE("GPL"); diff --git a/drivers/media/video/s5p-fimc/mipi-csis.h b/drivers/media/video/s5p-fimc/mipi-csis.h new file mode 100644 index 0000000..a1e3a38 --- /dev/null +++ b/drivers/media/video/s5p-fimc/mipi-csis.h @@ -0,0 +1,22 @@ +/* + * Samsung S5P/EXYNOS4 SoC series MIPI-CSI receiver driver + * + * Copyright (C) 2011 Samsung Electronics Co., Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#ifndef S5P_MIPI_CSIS_H_ +#define S5P_MIPI_CSIS_H_ + +#define CSIS_MODULE_NAME "s5p-mipi-csis" +#define CSIS_MAX_ENTITIES 2 +#define CSIS0_MAX_LANES 4 +#define CSIS1_MAX_LANES 2 + +#define CSIS_PAD_SINK 0 +#define CSIS_PAD_SOURCE 1 +#define CSIS_PADS_NUM 2 + +#endif -- 1.7.4.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers 2011-04-21 15:21 ` [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers Sylwester Nawrocki @ 2011-05-03 9:16 ` Laurent Pinchart 2011-05-03 18:07 ` Sylwester Nawrocki 0 siblings, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2011-05-03 9:16 UTC (permalink / raw) To: Sylwester Nawrocki Cc: linux-media, linux-samsung-soc, kyungmin.park, m.szyprowski, riverful.kim, kgene.kim, sungchun.kang, jonghun.han Hi Sylwester, Thanks for the patch. On Thursday 21 April 2011 17:21:04 Sylwester Nawrocki wrote: > Add the subdev driver for the MIPI CSIS units available in > S5P and Exynos4 SoC series. This driver supports both CSIS0 > and CSIS1 MIPI-CSI2 receivers. > The driver requires Runtime PM to be enabled for proper operation. Then maybe it should depend on runtime PM ? > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> [snip] > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig > index 4f0ac2d..1da961a 100644 > --- a/drivers/media/video/Kconfig > +++ b/drivers/media/video/Kconfig > @@ -939,6 +939,15 @@ config VIDEO_SAMSUNG_S5P_FIMC > To compile this driver as a module, choose M here: the > module will be called s5p-fimc. > > +config VIDEO_S5P_MIPI_CSIS > + tristate "S5P and EXYNOS4 MIPI CSI Receiver driver" Maybe "Samsung S5P and EXYNOS4" as well ? > + depends on VIDEO_V4L2 && VIDEO_SAMSUNG_S5P_FIMC && MEDIA_CONTROLLER > + ---help--- > + This is a v4l2 driver for the S5P/EXYNOS4 MIPI-CSI Receiver. > + > + To compile this driver as a module, choose M here: the > + module will be called s5p-csis. > + [snip] > diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c > b/drivers/media/video/s5p-fimc/mipi-csis.c new file mode 100644 > index 0000000..6219754 > --- /dev/null > +++ b/drivers/media/video/s5p-fimc/mipi-csis.c [snip] > +enum { > + CSIS_FMT_TRY, > + CSIS_FMT_ACTIVE, > + CSIS_NUM_FMTS > +} There's no need to define new TRY/ACTIVE constants, use the existing V4L2_SUBDEV_FORMAT_TRY and V4L2_SUBDEV_FORMAT_ACTIVE values. > +struct csis_state { > + struct mutex lock; checkpatch.pl requires mutexes to be documented. You should add a comment (here, or even better, before the structure, with the documentation of all memebers) that explains what the mutex protects. > + struct media_pad pads[CSIS_PADS_NUM]; > + struct v4l2_subdev sd; > + struct platform_device *pdev; > + struct resource *regs_res; > + void __iomem *regs; > + struct clk *clock[NUM_CSIS_CLOCKS]; > + int irq; > + struct regulator *supply; > + u32 flags; > + /* Common format for the source and sink pad. */ > + const struct csis_pix_format *csis_fmt; > + struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS]; As try formats are stored in the file handle, and as the formats on the sink and source pads are identical, a single v4l2_mbus_framefmt will do here. > +}; [snip] > +struct csis_pix_format { > + enum v4l2_mbus_pixelcode code; > + u32 fmt_reg; > + u16 pix_hor_align; You won't save memory by using a 16-bit integer here, and the code might be slower. I would go for a regular unsigned int. > +}; > + > +static const struct csis_pix_format s5pcsis_formats[] = { > + { > + .code = V4L2_MBUS_FMT_VYUY8_2X8, > + .fmt_reg = S5PCSIS_CFG_FMT_YCBCR422_8BIT, > + .pix_hor_align = 1, > + }, > + { > + .code = V4L2_MBUS_FMT_JPEG_1X8, > + .fmt_reg = S5PCSIS_CFG_FMT_USER(1), > + .pix_hor_align = 1, > + }, > +}; Do you plan to add formats with pix_hor_align != 1 ? If not you could remove the field completely. > +#define csis_pad_valid(pad) (pad == CSIS_PAD_SOURCE || pad == > CSIS_PAD_SINK) + > +static struct csis_state *sd_to_csis_state(struct v4l2_subdev *sdev) > +{ > + return container_of(sdev, struct csis_state, sd); > +} > + > +static const struct csis_pix_format *find_csis_format( > + struct v4l2_mbus_framefmt *mf) > +{ > + int i = ARRAY_SIZE(s5pcsis_formats); > + > + while (--i >= 0) I'm curious, why do you search backward instead of doing the usual for (i = 0; i < ARRAY_SIZE(s5pcsis_formats); ++i) (in that case 'i' could be unsigned) ? > + if (mf->code == s5pcsis_formats[i].code) > + return &s5pcsis_formats[i]; > + > + return NULL; > +} > + > +static void s5pcsis_enable_interrupts(struct csis_state *state, bool on) > +{ > + u32 reg = readl(state->regs + S5PCSIS_CTRL); I haven't read the hardware spec, but shouldn't the register be S5PCSIS_INTMSK here ? > + > + if (on) > + reg |= S5PCSIS_INTMSK_EN_ALL; > + else > + reg &= ~S5PCSIS_INTMSK_EN_ALL; > + writel(reg, state->regs + S5PCSIS_INTMSK); > +} > + > +static void s5pcsis_reset(struct csis_state *state) > +{ > + u32 reg = readl(state->regs + S5PCSIS_CTRL); > + > + writel(reg | S5PCSIS_CTRL_RESET, state->regs + S5PCSIS_CTRL); Would the code be more readable if you defined macros or inline functions for the read, write and read-modify-write register operations ? Something like s5pcsis_read(struct csis_state *state, u32 reg); s5pcsis_write(struct csis_state *state, u32 reg, u32 value); s5pcsis_clr(struct csis_state *state, u32 reg, u32 clr); s5pcsis_set(struct csis_state *state, u32 reg, u32 set); s5pcsis_clr_set(struct csis_state *state, u32 reg, u32 clr, u32 set); > + udelay(10); > +} > + > +static void s5pcsis_system_enable(struct csis_state *state, int on) s/int on/bool on/ ? I have no preference for int or bool in this case, it's just for consistency reasons as you use bool in s5pcsis_enable_interrupts(). > +{ > + u32 reg; > + > + reg = readl(state->regs + S5PCSIS_CTRL); > + if (on) > + reg |= S5PCSIS_CTRL_ENABLE; > + else > + reg &= ~S5PCSIS_CTRL_ENABLE; > + writel(reg, state->regs + S5PCSIS_CTRL); > + > + reg = readl(state->regs + S5PCSIS_DPHYCTRL); > + if (on) > + reg |= S5PCSIS_DPHYCTRL_ENABLE; > + else > + reg &= ~S5PCSIS_DPHYCTRL_ENABLE; > + writel(reg, state->regs + S5PCSIS_DPHYCTRL); > +} > + > +static int __s5pcsis_set_format(struct csis_state *state) > +{ > + struct v4l2_mbus_framefmt *mf = &state->mf[CSIS_FMT_ACTIVE]; > + u32 reg; > + > + v4l2_dbg(1, debug, &state->sd, "fmt: %d, %d x %d\n", > + mf->code, mf->width, mf->height); > + > + if (WARN_ON(state->csis_fmt == NULL)) > + return -EINVAL; This will happen if __s5pcsis_set_format() is called before s5pcsis_set_fmt(). You should set state->csis_fmt to a default value at initialization time and remove the check. > + /* Color format */ > + reg = readl(state->regs + S5PCSIS_CONFIG); > + reg = (reg & ~S5PCSIS_CFG_FMT_MASK) | state->csis_fmt->fmt_reg; > + writel(reg, state->regs + S5PCSIS_CONFIG); > + > + /* Pixel resolution */ Do you need to protect read access to state->mf, or do you guarantee that the functions that read and write it will be serialized by the caller ? > + reg = (mf->width << 16) | mf->height; > + writel(reg, state->regs + S5PCSIS_RESOL); > + > + return 0; > +} [snip] > +static int s5pcsis_set_params(struct csis_state *state) > +{ > + struct s5p_platform_mipi_csis *pdata = state->pdev->dev.platform_data; > + u32 reg, tmp; > + int ret; > + > + reg = readl(state->regs + S5PCSIS_CONFIG); > + reg &= ~S5PCSIS_CFG_NR_LANE_MASK; > + tmp = (pdata->lanes - 1) & 0x3; tmp is a bad name, you could do reg |= (pdata->lanes - 1) & S5PCSIS_CFG_NR_LANE_MASK; and get rid of tmp completely. > + writel(reg | tmp, state->regs + S5PCSIS_CONFIG); > + > + ret = __s5pcsis_set_format(state); > + if (ret) > + return ret; > + > + s5pcsis_set_hsync_settle(state, pdata->hs_settle); > + > + reg = readl(state->regs + S5PCSIS_CTRL); > + > + if (pdata->alignment == 32) > + reg |= S5PCSIS_CTRL_ALIGN_32BIT; > + else /* 24-bits */ > + reg &= ~S5PCSIS_CTRL_ALIGN_32BIT; > + > + /* Not using external clock. */ > + reg &= ~S5PCSIS_CTRL_WCLK_EXTCLK; > + > + writel(reg, state->regs + S5PCSIS_CTRL); > + > + /* Update the shadow register. */ > + reg = readl(state->regs + S5PCSIS_CTRL); > + writel(reg | S5PCSIS_CTRL_UPDATE_SHADOW, > + state->regs + S5PCSIS_CTRL); > + > + return 0; > +} [snip] > +static void s5pcsis_clk_put(struct csis_state *state) > +{ > + int i; > + > + for (i = 0; i < NUM_CSIS_CLOCKS; i++) > + if (!IS_ERR(state->clock[i])) > + clk_put(state->clock[i]); > +} > + > +static int s5pcsis_clk_get(struct csis_state *state) > +{ > + struct device *dev = &state->pdev->dev; > + int i; > + > + for (i = 0; i < NUM_CSIS_CLOCKS; i++) { > + state->clock[i] = clk_get(dev, csi_clock_name[i]); > + > + if (IS_ERR(state->clock[i])) { > + s5pcsis_clk_put(state); If an error occurs here, some clock will be equal to NULL. This won't be caught by the IS_ERR check in s5pcsis_clk_put(), so clk_put() will be called with a NULL argument. Is that guaranteed to be safe ? > + dev_err(dev, "failed to get clock: %s\n", > + csi_clock_name[i]); > + return -ENXIO; > + } > + } > + return 0; > +} [snip] > +static void s5pcsis_try_format(struct v4l2_mbus_framefmt *mf) > +{ > + struct csis_pix_format const *csis_fmt; > + > + csis_fmt = find_csis_format(mf); > + if (csis_fmt == NULL) > + csis_fmt = &s5pcsis_formats[0]; > + > + mf->code = csis_fmt->code; > + v4l_bound_align_image(&mf->width, 1, CSIS_MAX_PIX_WIDTH, > + csis_fmt->pix_hor_align, > + &mf->height, 1, CSIS_MAX_PIX_HEIGHT, 1, > + 0); > +} > + > +static int s5pcsis_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh > *fh, + struct v4l2_subdev_format *fmt) > +{ > + struct csis_state *state = sd_to_csis_state(sd); > + struct v4l2_mbus_framefmt *mf = &fmt->format; > + struct csis_pix_format const *csis_fmt = find_csis_format(mf); > + > + v4l2_dbg(1, debug, sd, "%s: %dx%d, code: %x, csis_fmt: %p\n", > + __func__, mf->width, mf->height, mf->code, csis_fmt); > + > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > + s5pcsis_try_format(mf); You need to take the pad into account here. As you mention below, source and sink formats are identical. When the user tries to set the source format, the driver should just return the sink format without performing any modification. > + state->mf[CSIS_FMT_TRY] = *mf; > + return 0; > + } > + > + /* Both source and sink pad have always same format. */ > + if (!csis_pad_valid(fmt->pad) || > + csis_fmt == NULL || > + mf->width > CSIS_MAX_PIX_WIDTH || > + mf->height > CSIS_MAX_PIX_HEIGHT || > + mf->width & (u32)(csis_fmt->pix_hor_align - 1)) > + return -EINVAL; Don't return an error, adjust the user supplied format instead. > + > + mutex_lock(&state->lock); > + state->mf[CSIS_FMT_ACTIVE] = *mf; > + state->csis_fmt = csis_fmt; > + mutex_unlock(&state->lock); The logic in this function is not correct. First of all, you need to adjust the user-supplied format in all cases, regardless of the format type (try/active). Then, as the formats on the sink and source pads are always identical, you should return the sink pad format when the user tries to set the source pad format. Setting the source pad format will have no effect. Finally, you should store try formats in the subdev file handle, not in the csis_state structure (see ccp2_set_format() in drivers/media/video/omap3isp/ispccp2.c for an example, in your case you don't need to propagate the format change from sink to source, as the source always has the same format as the sink). > + > + return 0; > +} > + > +static int s5pcsis_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh > *fh, + struct v4l2_subdev_format *fmt) > +{ > + struct csis_state *state = sd_to_csis_state(sd); > + int index = fmt->which == V4L2_SUBDEV_FORMAT_TRY ? > + CSIS_FMT_TRY : CSIS_FMT_ACTIVE; > + > + if (!csis_pad_valid(fmt->pad)) > + return -EINVAL; > + > + mutex_lock(&state->lock); > + fmt->format = state->mf[index]; > + mutex_unlock(&state->lock); Try formats should be stored in the subdev file handle. If your caller guarantees that get/set format calls are serialized, you don't need to use a mutex. > + > + return 0; > +} [snip] > +/* Media operations */ > +static int csis_link_setup(struct media_entity *entity, > + const struct media_pad *local, > + const struct media_pad *remote, u32 flags) > +{ > + v4l2_dbg(1, debug, entity, "%s: entity: %s, flags: 0x%x\n", > + __func__, entity->name, flags); > + > + return 0; > +} > + > +static const struct media_entity_operations csis_media_ops = { > + .link_setup = csis_link_setup, > +}; If you define the link as immutable, the link_setup operation isn't required. > +static irqreturn_t s5pcsis_irq_handler(int irq, void *dev_id) > +{ > + struct csis_state *state = dev_id; > + u32 reg; > + > + /* Just clear the interrupt pending bits. */ What is the IRQ for then ? > + reg = readl(state->regs + S5PCSIS_INTSRC); > + writel(reg, state->regs + S5PCSIS_INTSRC); > + > + return IRQ_HANDLED; > +} [snip] -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers 2011-05-03 9:16 ` Laurent Pinchart @ 2011-05-03 18:07 ` Sylwester Nawrocki 2011-05-04 12:00 ` Laurent Pinchart 0 siblings, 1 reply; 11+ messages in thread From: Sylwester Nawrocki @ 2011-05-03 18:07 UTC (permalink / raw) To: Laurent Pinchart Cc: Sylwester Nawrocki, linux-media, linux-samsung-soc, Kyungmin Park, Marek Szyprowski, Heungjun Kim, Sungchun Kang, Jonghun Han Hi Laurent, thank you for the review. On 05/03/2011 11:16 AM, Laurent Pinchart wrote: > Hi Sylwester, > > Thanks for the patch. > > On Thursday 21 April 2011 17:21:04 Sylwester Nawrocki wrote: >> Add the subdev driver for the MIPI CSIS units available in >> S5P and Exynos4 SoC series. This driver supports both CSIS0 >> and CSIS1 MIPI-CSI2 receivers. >> The driver requires Runtime PM to be enabled for proper operation. > > Then maybe it should depend on runtime PM ? Yes, it seem the right thing to do. I hesitated to do this, not sure now why. > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com> >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> > > [snip] > >> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig >> index 4f0ac2d..1da961a 100644 >> --- a/drivers/media/video/Kconfig >> +++ b/drivers/media/video/Kconfig >> @@ -939,6 +939,15 @@ config VIDEO_SAMSUNG_S5P_FIMC >> To compile this driver as a module, choose M here: the >> module will be called s5p-fimc. >> >> +config VIDEO_S5P_MIPI_CSIS >> + tristate "S5P and EXYNOS4 MIPI CSI Receiver driver" > > Maybe "Samsung S5P and EXYNOS4" as well ? Agreed. I'll change it in the patch 2/3 as well. > >> + depends on VIDEO_V4L2&& VIDEO_SAMSUNG_S5P_FIMC&& MEDIA_CONTROLLER >> + ---help--- >> + This is a v4l2 driver for the S5P/EXYNOS4 MIPI-CSI Receiver. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called s5p-csis. >> + > > [snip] > >> diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c >> b/drivers/media/video/s5p-fimc/mipi-csis.c new file mode 100644 >> index 0000000..6219754 >> --- /dev/null >> +++ b/drivers/media/video/s5p-fimc/mipi-csis.c > > [snip] > >> +enum { >> + CSIS_FMT_TRY, >> + CSIS_FMT_ACTIVE, >> + CSIS_NUM_FMTS >> +} > > There's no need to define new TRY/ACTIVE constants, use the existing > V4L2_SUBDEV_FORMAT_TRY and V4L2_SUBDEV_FORMAT_ACTIVE values. > >> +struct csis_state { >> + struct mutex lock; > > checkpatch.pl requires mutexes to be documented. You should add a comment > (here, or even better, before the structure, with the documentation of all > members) that explains what the mutex protects. Didn't know about that.. I'll add a full description of all the structure's members. > >> + struct media_pad pads[CSIS_PADS_NUM]; >> + struct v4l2_subdev sd; >> + struct platform_device *pdev; >> + struct resource *regs_res; >> + void __iomem *regs; >> + struct clk *clock[NUM_CSIS_CLOCKS]; >> + int irq; >> + struct regulator *supply; >> + u32 flags; >> + /* Common format for the source and sink pad. */ >> + const struct csis_pix_format *csis_fmt; >> + struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS]; > > As try formats are stored in the file handle, and as the formats on the sink > and source pads are identical, a single v4l2_mbus_framefmt will do here. Ok. How about a situation when the caller never provides a file handle? Is it not supposed to happen? For V4L2_SUBDEV_FORMAT_TRY, should set_fmt just abandon storing the format and should get_fmt just return -EINVAL when passed fh == NULL ? Or should the host driver allocate the file handle just for the sake of set_fmt/get_fmt calls (assuming that cropping ops are not supported by the subdev) ? It's not my intention to create a broken implementation but it would be nice to be able to drop functionality which would never be used. As a note, I wanted to avoid bothering user space with setting up the MIPI CSI receiver sub-device. There wouldn't be any gain from it, just more things to care about for the applications. Moreover I don't see a good usage for the stored TRY format (yet). So I originally thought this subdev could be configurable by the host driver which wouldn't provide a file handle. > >> +}; > > [snip] > >> +struct csis_pix_format { >> + enum v4l2_mbus_pixelcode code; >> + u32 fmt_reg; >> + u16 pix_hor_align; > > You won't save memory by using a 16-bit integer here, and the code might be > slower. I would go for a regular unsigned int. Agreed. > >> +}; >> + >> +static const struct csis_pix_format s5pcsis_formats[] = { >> + { >> + .code = V4L2_MBUS_FMT_VYUY8_2X8, >> + .fmt_reg = S5PCSIS_CFG_FMT_YCBCR422_8BIT, >> + .pix_hor_align = 1, >> + }, >> + { >> + .code = V4L2_MBUS_FMT_JPEG_1X8, >> + .fmt_reg = S5PCSIS_CFG_FMT_USER(1), >> + .pix_hor_align = 1, >> + }, >> +}; > > Do you plan to add formats with pix_hor_align != 1 ? If not you could remove > the field completely. Yes, there is more formats that need different alignment, like RAW10, RAW12. The documentation for s5pc110, for instance, can be downloaded after registration from http://www.aesop.or.kr/?mid=PageMain_Documents_Tips Actually it is supposed to be 2^pix_hor_align pixel width alignment. I need to amend the usage of this alignment property in s5pcsis_set_fmt(). > >> +#define csis_pad_valid(pad) (pad == CSIS_PAD_SOURCE || pad == >> CSIS_PAD_SINK) + >> +static struct csis_state *sd_to_csis_state(struct v4l2_subdev *sdev) >> +{ >> + return container_of(sdev, struct csis_state, sd); >> +} >> + >> +static const struct csis_pix_format *find_csis_format( >> + struct v4l2_mbus_framefmt *mf) >> +{ >> + int i = ARRAY_SIZE(s5pcsis_formats); >> + >> + while (--i>= 0) > > I'm curious, why do you search backward instead of doing the usual > > for (i = 0; i< ARRAY_SIZE(s5pcsis_formats); ++i) > > (in that case 'i' could be unsigned) ? Perhaps doing it either way does not make any difference with the toolchains we use, but the loops with test for 0 are supposed to be faster on ARM. > >> + if (mf->code == s5pcsis_formats[i].code) >> + return&s5pcsis_formats[i]; >> + >> + return NULL; >> +} >> + >> +static void s5pcsis_enable_interrupts(struct csis_state *state, bool on) >> +{ >> + u32 reg = readl(state->regs + S5PCSIS_CTRL); > > I haven't read the hardware spec, but shouldn't the register be S5PCSIS_INTMSK > here ? That for sure wasn't deliberate. I just wonder how it went unnoticed for so long :| > >> + >> + if (on) >> + reg |= S5PCSIS_INTMSK_EN_ALL; >> + else >> + reg&= ~S5PCSIS_INTMSK_EN_ALL; >> + writel(reg, state->regs + S5PCSIS_INTMSK); >> +} >> + >> +static void s5pcsis_reset(struct csis_state *state) >> +{ >> + u32 reg = readl(state->regs + S5PCSIS_CTRL); >> + >> + writel(reg | S5PCSIS_CTRL_RESET, state->regs + S5PCSIS_CTRL); > > Would the code be more readable if you defined macros or inline functions for > the read, write and read-modify-write register operations ? Something like > > s5pcsis_read(struct csis_state *state, u32 reg); > s5pcsis_write(struct csis_state *state, u32 reg, u32 value); > s5pcsis_clr(struct csis_state *state, u32 reg, u32 clr); > s5pcsis_set(struct csis_state *state, u32 reg, u32 set); > s5pcsis_clr_set(struct csis_state *state, u32 reg, u32 clr, u32 set); Yes, I suppose so. However there is only 9 control/status registers and those functions touching the registers are really small. They touch 2 registers at most so IMHO it was not worth to create such utility macro/inline functions. Nevertheless I'll try and see how it goes. > >> + udelay(10); >> +} >> + >> +static void s5pcsis_system_enable(struct csis_state *state, int on) > > s/int on/bool on/ ? I have no preference for int or bool in this case, it's > just for consistency reasons as you use bool in s5pcsis_enable_interrupts(). All right, I'm going to use bool. Maybe it's because I wasn't really convinced whether to use int or bool.... :) > >> +{ >> + u32 reg; >> + >> + reg = readl(state->regs + S5PCSIS_CTRL); >> + if (on) >> + reg |= S5PCSIS_CTRL_ENABLE; >> + else >> + reg&= ~S5PCSIS_CTRL_ENABLE; >> + writel(reg, state->regs + S5PCSIS_CTRL); >> + >> + reg = readl(state->regs + S5PCSIS_DPHYCTRL); >> + if (on) >> + reg |= S5PCSIS_DPHYCTRL_ENABLE; >> + else >> + reg&= ~S5PCSIS_DPHYCTRL_ENABLE; >> + writel(reg, state->regs + S5PCSIS_DPHYCTRL); >> +} >> + >> +static int __s5pcsis_set_format(struct csis_state *state) >> +{ >> + struct v4l2_mbus_framefmt *mf =&state->mf[CSIS_FMT_ACTIVE]; >> + u32 reg; >> + >> + v4l2_dbg(1, debug,&state->sd, "fmt: %d, %d x %d\n", >> + mf->code, mf->width, mf->height); >> + >> + if (WARN_ON(state->csis_fmt == NULL)) >> + return -EINVAL; > > This will happen if __s5pcsis_set_format() is called before s5pcsis_set_fmt(). > You should set state->csis_fmt to a default value at initialization time and > remove the check. So s_stream before set_fmt. This would never happen in my setup, nevertheless I'll get rid of the check, this way whole function could be converted to return void. > >> + /* Color format */ >> + reg = readl(state->regs + S5PCSIS_CONFIG); >> + reg = (reg& ~S5PCSIS_CFG_FMT_MASK) | state->csis_fmt->fmt_reg; >> + writel(reg, state->regs + S5PCSIS_CONFIG); >> + >> + /* Pixel resolution */ > > Do you need to protect read access to state->mf, or do you guarantee that the > functions that read and write it will be serialized by the caller ? This function is guaranteed to be called with the state->lock mutex held. I'll add relevant comment. > >> + reg = (mf->width<< 16) | mf->height; >> + writel(reg, state->regs + S5PCSIS_RESOL); >> + >> + return 0; >> +} > > [snip] > >> +static int s5pcsis_set_params(struct csis_state *state) >> +{ >> + struct s5p_platform_mipi_csis *pdata = state->pdev->dev.platform_data; >> + u32 reg, tmp; >> + int ret; >> + >> + reg = readl(state->regs + S5PCSIS_CONFIG); >> + reg&= ~S5PCSIS_CFG_NR_LANE_MASK; >> + tmp = (pdata->lanes - 1)& 0x3; > > tmp is a bad name, you could do > > reg |= (pdata->lanes - 1)& S5PCSIS_CFG_NR_LANE_MASK; > > and get rid of tmp completely. Thanks, looks much better that way. I'm not a fan of variable names like 'tmp' either, but CodingStyle does not condemn them. > >> + writel(reg | tmp, state->regs + S5PCSIS_CONFIG); >> + >> + ret = __s5pcsis_set_format(state); >> + if (ret) >> + return ret; >> + >> + s5pcsis_set_hsync_settle(state, pdata->hs_settle); >> + >> + reg = readl(state->regs + S5PCSIS_CTRL); >> + >> + if (pdata->alignment == 32) >> + reg |= S5PCSIS_CTRL_ALIGN_32BIT; >> + else /* 24-bits */ >> + reg&= ~S5PCSIS_CTRL_ALIGN_32BIT; >> + >> + /* Not using external clock. */ >> + reg&= ~S5PCSIS_CTRL_WCLK_EXTCLK; >> + >> + writel(reg, state->regs + S5PCSIS_CTRL); >> + >> + /* Update the shadow register. */ >> + reg = readl(state->regs + S5PCSIS_CTRL); >> + writel(reg | S5PCSIS_CTRL_UPDATE_SHADOW, >> + state->regs + S5PCSIS_CTRL); >> + >> + return 0; >> +} > > [snip] > >> +static void s5pcsis_clk_put(struct csis_state *state) >> +{ >> + int i; >> + >> + for (i = 0; i< NUM_CSIS_CLOCKS; i++) >> + if (!IS_ERR(state->clock[i])) >> + clk_put(state->clock[i]); >> +} >> + >> +static int s5pcsis_clk_get(struct csis_state *state) >> +{ >> + struct device *dev =&state->pdev->dev; >> + int i; >> + >> + for (i = 0; i< NUM_CSIS_CLOCKS; i++) { >> + state->clock[i] = clk_get(dev, csi_clock_name[i]); >> + >> + if (IS_ERR(state->clock[i])) { >> + s5pcsis_clk_put(state); > > If an error occurs here, some clock will be equal to NULL. This won't be > caught by the IS_ERR check in s5pcsis_clk_put(), so clk_put() will be called > with a NULL argument. Is that guaranteed to be safe ? Thanks, good catch! No, the clk_put routine will just offhand dereference what is passed to it. Then IS_ERR_OR_NULL should be used instead in s5pcsis_clk_put(). > >> + dev_err(dev, "failed to get clock: %s\n", >> + csi_clock_name[i]); >> + return -ENXIO; >> + } >> + } >> + return 0; >> +} > > [snip] > >> +static void s5pcsis_try_format(struct v4l2_mbus_framefmt *mf) >> +{ >> + struct csis_pix_format const *csis_fmt; >> + >> + csis_fmt = find_csis_format(mf); >> + if (csis_fmt == NULL) >> + csis_fmt =&s5pcsis_formats[0]; >> + >> + mf->code = csis_fmt->code; >> + v4l_bound_align_image(&mf->width, 1, CSIS_MAX_PIX_WIDTH, >> + csis_fmt->pix_hor_align, >> + &mf->height, 1, CSIS_MAX_PIX_HEIGHT, 1, >> + 0); >> +} >> + >> +static int s5pcsis_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh >> *fh, + struct v4l2_subdev_format *fmt) >> +{ >> + struct csis_state *state = sd_to_csis_state(sd); >> + struct v4l2_mbus_framefmt *mf =&fmt->format; >> + struct csis_pix_format const *csis_fmt = find_csis_format(mf); >> + >> + v4l2_dbg(1, debug, sd, "%s: %dx%d, code: %x, csis_fmt: %p\n", >> + __func__, mf->width, mf->height, mf->code, csis_fmt); >> + >> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { >> + s5pcsis_try_format(mf); > > You need to take the pad into account here. As you mention below, source and > sink formats are identical. When the user tries to set the source format, the > driver should just return the sink format without performing any modification. > >> + state->mf[CSIS_FMT_TRY] = *mf; >> + return 0; >> + } >> + >> + /* Both source and sink pad have always same format. */ >> + if (!csis_pad_valid(fmt->pad) || >> + csis_fmt == NULL || >> + mf->width> CSIS_MAX_PIX_WIDTH || >> + mf->height> CSIS_MAX_PIX_HEIGHT || >> + mf->width& (u32)(csis_fmt->pix_hor_align - 1)) >> + return -EINVAL; > > Don't return an error, adjust the user supplied format instead. > >> + >> + mutex_lock(&state->lock); >> + state->mf[CSIS_FMT_ACTIVE] = *mf; >> + state->csis_fmt = csis_fmt; >> + mutex_unlock(&state->lock); > > The logic in this function is not correct. First of all, you need to adjust > the user-supplied format in all cases, regardless of the format type > (try/active). Then, as the formats on the sink and source pads are always > identical, you should return the sink pad format when the user tries to set > the source pad format. Setting the source pad format will have no effect. > Finally, you should store try formats in the subdev file handle, not in the > csis_state structure (see ccp2_set_format() in > drivers/media/video/omap3isp/ispccp2.c for an example, in your case you don't > need to propagate the format change from sink to source, as the source always > has the same format as the sink). Thanks, that's all very useful. The only thing I am concerned is what should be done when the file handle is null? Is it not allowed by design? I've initially reworked s5pcsis_set_fmt to something like this: static int s5pcsis_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, struct v4l2_subdev_format *fmt) { struct csis_state *state = sd_to_csis_state(sd); struct v4l2_mbus_framefmt *mf = &fmt->format; struct csis_pix_format const *csis_fmt; v4l2_dbg(1, debug, sd, "%s: %dx%d, code: %x\n", __func__, mf->width, mf->height, mf->code); if (!csis_pad_valid(fmt->pad)) return -EINVAL; mutex_lock(&state->lock); if (fmt->pad == CSIS_PAD_SOURCE) { fmt->format = state->format; goto unlock; } csis_fmt = s5pcsis_try_format(&fmt->format); if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { /* TODO: store format in *fh */ goto unlock; } /* Common format for the source and the sink pad */ state->format = fmt->format; state->csis_fmt = csis_fmt; unlock: mutex_unlock(&state->lock); return 0; } > >> + >> + return 0; >> +} >> + >> +static int s5pcsis_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh >> *fh, + struct v4l2_subdev_format *fmt) >> +{ >> + struct csis_state *state = sd_to_csis_state(sd); >> + int index = fmt->which == V4L2_SUBDEV_FORMAT_TRY ? >> + CSIS_FMT_TRY : CSIS_FMT_ACTIVE; >> + >> + if (!csis_pad_valid(fmt->pad)) >> + return -EINVAL; >> + >> + mutex_lock(&state->lock); >> + fmt->format = state->mf[index]; >> + mutex_unlock(&state->lock); > > Try formats should be stored in the subdev file handle. > > If your caller guarantees that get/set format calls are serialized, you don't > need to use a mutex. No, there is no guarantee the pad operations are serialized. The MIPI CSIS subdev is a member of two pipelines each of which is terminated by a different FIMC entity (video capture node). x--- FIMC_0 (/dev/video1) SENSOR -> MIPI_CSIS --| x--- FIMC_1 (/dev/video3) So MIPI CSIS ops can be called from both host drivers. Of course at any time either FIMC_0 or FIMC_1 is streaming. S Not sure how I could get rid of the mutex... > >> + >> + return 0; >> +} > > [snip] > >> +/* Media operations */ >> +static int csis_link_setup(struct media_entity *entity, >> + const struct media_pad *local, >> + const struct media_pad *remote, u32 flags) >> +{ >> + v4l2_dbg(1, debug, entity, "%s: entity: %s, flags: 0x%x\n", >> + __func__, entity->name, flags); >> + >> + return 0; >> +} >> + >> +static const struct media_entity_operations csis_media_ops = { >> + .link_setup = csis_link_setup, >> +}; > > If you define the link as immutable, the link_setup operation isn't required. OK, thanks for the hint. I've got one immutable link (from sensor subdev to the SINK pad) but the other ones aren't immutable (from SOURCE pad there are 4 links to FIMC entities, of which any number can potentially be active at any time). I assume the link_setup could be omitted in that case too, as it would really do nothing. > >> +static irqreturn_t s5pcsis_irq_handler(int irq, void *dev_id) >> +{ >> + struct csis_state *state = dev_id; >> + u32 reg; >> + >> + /* Just clear the interrupt pending bits. */ > > What is the IRQ for then ? It's for reporting various MIPI CSI bus errors, like CRC, FIFO overflows, packet's fragment loss etc. I plan to support this in the future even though it is not clear from the specs what could be done with all those error events except of printing them in the log. > >> + reg = readl(state->regs + S5PCSIS_INTSRC); >> + writel(reg, state->regs + S5PCSIS_INTSRC); >> + >> + return IRQ_HANDLED; >> +} > > [snip] -- Regards, Sylwester Nawrocki ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers 2011-05-03 18:07 ` Sylwester Nawrocki @ 2011-05-04 12:00 ` Laurent Pinchart 2011-05-05 9:33 ` Sylwester Nawrocki 0 siblings, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2011-05-04 12:00 UTC (permalink / raw) To: Sylwester Nawrocki Cc: Sylwester Nawrocki, linux-media, linux-samsung-soc, Kyungmin Park, Marek Szyprowski, Heungjun Kim, Sungchun Kang, Jonghun Han Hi Sylwester, On Tuesday 03 May 2011 20:07:43 Sylwester Nawrocki wrote: > On 05/03/2011 11:16 AM, Laurent Pinchart wrote: > > On Thursday 21 April 2011 17:21:04 Sylwester Nawrocki wrote: [snip] > >> + struct media_pad pads[CSIS_PADS_NUM]; > >> + struct v4l2_subdev sd; > >> + struct platform_device *pdev; > >> + struct resource *regs_res; > >> + void __iomem *regs; > >> + struct clk *clock[NUM_CSIS_CLOCKS]; > >> + int irq; > >> + struct regulator *supply; > >> + u32 flags; > >> + /* Common format for the source and sink pad. */ > >> + const struct csis_pix_format *csis_fmt; > >> + struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS]; > > > > As try formats are stored in the file handle, and as the formats on the > > sink and source pads are identical, a single v4l2_mbus_framefmt will do > > here. > > Ok. How about a situation when the caller never provides a file handle? > Is it not supposed to happen? Good question :-) The subdev pad-level operations have been designed with a userspace interface in mind, so they require a file handle to store try the formats (and crop rectangles). > For V4L2_SUBDEV_FORMAT_TRY, should set_fmt just abandon storing the format > and should get_fmt just return -EINVAL when passed fh == NULL ? For such a simple subdev, that should work as a workaround, yes. You can use it as a temporary solution at least. > Or should the host driver allocate the file handle just for the sake of > set_fmt/get_fmt calls (assuming that cropping ops are not supported > by the subdev) ? That's another solution. We could also pass an internal structure that contains formats and crop rectangles to the pad operations handlers, instead of passing the whole file handle. Do you think that would be better ? > It's not my intention to create a broken implementation but it would > be nice to be able to drop functionality which would never be used. > > As a note, I wanted to avoid bothering user space with setting up the MIPI > CSI receiver sub-device. There wouldn't be any gain from it, just more > things to care about for the applications. Quoting one of your comments below, x--- FIMC_0 (/dev/video1) SENSOR -> MIPI_CSIS --| x--- FIMC_1 (/dev/video3) How do you expect to configure the MIPI_CSIS block from the FIMC_0 and FIMC_1 blocks, without any help from userspace ? Conflicts will need to be handled, and the best way to handle them is to have userspace configuring the MIPI_CSIS explicitly. > Moreover I don't see a good usage for the stored TRY format (yet). So I > originally thought this subdev could be configurable by the host driver > which wouldn't provide a file handle. [snip] > >> +#define csis_pad_valid(pad) (pad == CSIS_PAD_SOURCE || pad == > >> CSIS_PAD_SINK) + > >> +static struct csis_state *sd_to_csis_state(struct v4l2_subdev *sdev) > >> +{ > >> + return container_of(sdev, struct csis_state, sd); > >> +} > >> + > >> +static const struct csis_pix_format *find_csis_format( > >> + struct v4l2_mbus_framefmt *mf) > >> +{ > >> + int i = ARRAY_SIZE(s5pcsis_formats); > >> + > >> + while (--i>= 0) > > > > I'm curious, why do you search backward instead of doing the usual > > > > for (i = 0; i< ARRAY_SIZE(s5pcsis_formats); ++i) > > > > (in that case 'i' could be unsigned) ? > > Perhaps doing it either way does not make any difference with the > toolchains we use, but the loops with test for 0 are supposed to be faster > on ARM. I didn't know that. I wonder if it makes a real difference with gcc. [snip] > >> +static void s5pcsis_try_format(struct v4l2_mbus_framefmt *mf) > >> +{ > >> + struct csis_pix_format const *csis_fmt; > >> + > >> + csis_fmt = find_csis_format(mf); > >> + if (csis_fmt == NULL) > >> + csis_fmt =&s5pcsis_formats[0]; > >> + > >> + mf->code = csis_fmt->code; > >> + v4l_bound_align_image(&mf->width, 1, CSIS_MAX_PIX_WIDTH, > >> + csis_fmt->pix_hor_align, > >> + &mf->height, 1, CSIS_MAX_PIX_HEIGHT, 1, > >> + 0); > >> +} > >> + > >> +static int s5pcsis_set_fmt(struct v4l2_subdev *sd, struct > >> v4l2_subdev_fh *fh, + struct v4l2_subdev_format *fmt) > >> +{ > >> + struct csis_state *state = sd_to_csis_state(sd); > >> + struct v4l2_mbus_framefmt *mf =&fmt->format; > >> + struct csis_pix_format const *csis_fmt = find_csis_format(mf); > >> + > >> + v4l2_dbg(1, debug, sd, "%s: %dx%d, code: %x, csis_fmt: %p\n", > >> + __func__, mf->width, mf->height, mf->code, csis_fmt); > >> + > >> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > >> + s5pcsis_try_format(mf); > > > > You need to take the pad into account here. As you mention below, source > > and sink formats are identical. When the user tries to set the source > > format, the driver should just return the sink format without performing > > any modification. > > > >> + state->mf[CSIS_FMT_TRY] = *mf; > >> + return 0; > >> + } > >> + > >> + /* Both source and sink pad have always same format. */ > >> + if (!csis_pad_valid(fmt->pad) || > >> + csis_fmt == NULL || > >> + mf->width> CSIS_MAX_PIX_WIDTH || > >> + mf->height> CSIS_MAX_PIX_HEIGHT || > >> + mf->width& (u32)(csis_fmt->pix_hor_align - 1)) > >> + return -EINVAL; > > > > Don't return an error, adjust the user supplied format instead. > > > >> + > >> + mutex_lock(&state->lock); > >> + state->mf[CSIS_FMT_ACTIVE] = *mf; > >> + state->csis_fmt = csis_fmt; > >> + mutex_unlock(&state->lock); > > > > The logic in this function is not correct. First of all, you need to > > adjust the user-supplied format in all cases, regardless of the format > > type (try/active). Then, as the formats on the sink and source pads are > > always identical, you should return the sink pad format when the user > > tries to set the source pad format. Setting the source pad format will > > have no effect. Finally, you should store try formats in the subdev file > > handle, not in the csis_state structure (see ccp2_set_format() in > > drivers/media/video/omap3isp/ispccp2.c for an example, in your case you > > don't need to propagate the format change from sink to source, as the > > source always has the same format as the sink). > > Thanks, that's all very useful. The only thing I am concerned is what > should be done when the file handle is null? Is it not allowed by design? It wasn't allowed by design. As explained above, we could replace the file handle with another structure, and have the caller allocate it. In your case you could just skip storing try formats. This needs to be thought about some more. > I've initially reworked s5pcsis_set_fmt to something like this: > > static int s5pcsis_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh > *fh, struct v4l2_subdev_format *fmt) > { > struct csis_state *state = sd_to_csis_state(sd); > struct v4l2_mbus_framefmt *mf = &fmt->format; > struct csis_pix_format const *csis_fmt; > > v4l2_dbg(1, debug, sd, "%s: %dx%d, code: %x\n", > __func__, mf->width, mf->height, mf->code); > > if (!csis_pad_valid(fmt->pad)) > return -EINVAL; > > mutex_lock(&state->lock); > if (fmt->pad == CSIS_PAD_SOURCE) { > fmt->format = state->format; > goto unlock; > } > csis_fmt = s5pcsis_try_format(&fmt->format); Try try_format call doesn't require the mutex to be locked. You can lock/unlock the mutex around fmt->format = state->format; above, and lock/unlock it around state->formt = fmt->format; state->csis_fmt = csis_fmt; below. > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > /* TODO: store format in *fh */ > goto unlock; > } > > /* Common format for the source and the sink pad */ > state->format = fmt->format; > state->csis_fmt = csis_fmt; > unlock: > mutex_unlock(&state->lock); > return 0; > } [snip] -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers 2011-05-04 12:00 ` Laurent Pinchart @ 2011-05-05 9:33 ` Sylwester Nawrocki 2011-05-05 12:25 ` Laurent Pinchart 0 siblings, 1 reply; 11+ messages in thread From: Sylwester Nawrocki @ 2011-05-05 9:33 UTC (permalink / raw) To: Laurent Pinchart Cc: Sylwester Nawrocki, linux-media, linux-samsung-soc, Kyungmin Park, Marek Szyprowski, Heungjun Kim, Sungchun Kang, Jonghun Han Hi Laurent, On 05/04/2011 02:00 PM, Laurent Pinchart wrote: > Hi Sylwester, > > On Tuesday 03 May 2011 20:07:43 Sylwester Nawrocki wrote: >> On 05/03/2011 11:16 AM, Laurent Pinchart wrote: >>> On Thursday 21 April 2011 17:21:04 Sylwester Nawrocki wrote: > > [snip] > >>>> + struct media_pad pads[CSIS_PADS_NUM]; >>>> + struct v4l2_subdev sd; >>>> + struct platform_device *pdev; >>>> + struct resource *regs_res; >>>> + void __iomem *regs; >>>> + struct clk *clock[NUM_CSIS_CLOCKS]; >>>> + int irq; >>>> + struct regulator *supply; >>>> + u32 flags; >>>> + /* Common format for the source and sink pad. */ >>>> + const struct csis_pix_format *csis_fmt; >>>> + struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS]; >>> >>> As try formats are stored in the file handle, and as the formats on the >>> sink and source pads are identical, a single v4l2_mbus_framefmt will do >>> here. >> >> Ok. How about a situation when the caller never provides a file handle? >> Is it not supposed to happen? > > Good question :-) The subdev pad-level operations have been designed with a > userspace interface in mind, so they require a file handle to store try the > formats (and crop rectangles). > >> For V4L2_SUBDEV_FORMAT_TRY, should set_fmt just abandon storing the format >> and should get_fmt just return -EINVAL when passed fh == NULL ? > > For such a simple subdev, that should work as a workaround, yes. You can use > it as a temporary solution at least. > >> Or should the host driver allocate the file handle just for the sake of >> set_fmt/get_fmt calls (assuming that cropping ops are not supported >> by the subdev) ? > > That's another solution. We could also pass an internal structure that > contains formats and crop rectangles to the pad operations handlers, instead > of passing the whole file handle. Do you think that would be better ? So it would then be an additional argument for the pad-level operations ? Perhaps that would make sense when both format and crop information is needed at the same time in the subdev driver. However this would only be required for TRY formats/crop rectangles which wouldn't be supported anyway because of missing file handle.. or I missed something? Furthermore I assume more complex pipelines will be handled in user space and the host drivers could store format and crop information locally directly from v4l2_subdev_format and v4l2_subdev_crop data structures. [snip] > Quoting one of your comments below, > > x--- FIMC_0 (/dev/video1) > SENSOR -> MIPI_CSIS --| > x--- FIMC_1 (/dev/video3) > > How do you expect to configure the MIPI_CSIS block from the FIMC_0 and FIMC_1 > blocks, without any help from userspace ? Conflicts will need to be handled, > and the best way to handle them is to have userspace configuring the MIPI_CSIS > explicitly. My priority is to make work the configurations without device nodes at sensor and MIPI CSIS subdevs. I agree it would be best to leave the negotiation logic to user space, however I need to assure the regular V4L2 application also can use the driver. My idea was to only try format at CSI slave and sensor subdevs when S_FMT is called on a video node. So the sensor and CSIS constraints are taken into account. Then from VIDIOC_STREAMON, formats at pipeline elements would be set on subdevs without device node or validated on subdevs providing a device node. Another issue is v4l2 controls. The subdevs are now in my case registered to a v4l2_device instance embedded in the media device driver. The video node drivers (FIMC0...FIMC3) have their own v4l2_device instances. So the control inheritance between video node and a subdevs is gone :/, i.e. I couldn't find a standard way to remove back from a parent control handler the controls which have been added to it with v4l2_ctrl_handler_add(). I've had similar issue with subdev -> v4l2_device notify callback. Before, when the subdev was directly registered to a v4l2_instance associated with a video node, v4l2_subdev_notify had been propagated straight to FIMC{N} device the subdev was attached to. Now I just redirect notifications ending up in the media device driver to relevant FIMC{N} device instance depending on link configuration. [snip] >>>> +#define csis_pad_valid(pad) (pad == CSIS_PAD_SOURCE || pad == >>>> CSIS_PAD_SINK) + >>>> +static struct csis_state *sd_to_csis_state(struct v4l2_subdev *sdev) >>>> +{ >>>> + return container_of(sdev, struct csis_state, sd); >>>> +} >>>> + >>>> +static const struct csis_pix_format *find_csis_format( >>>> + struct v4l2_mbus_framefmt *mf) >>>> +{ >>>> + int i = ARRAY_SIZE(s5pcsis_formats); >>>> + >>>> + while (--i>= 0) >>> >>> I'm curious, why do you search backward instead of doing the usual >>> >>> for (i = 0; i< ARRAY_SIZE(s5pcsis_formats); ++i) >>> >>> (in that case 'i' could be unsigned) ? >> >> Perhaps doing it either way does not make any difference with the >> toolchains we use, but the loops with test for 0 are supposed to be faster >> on ARM. > > I didn't know that. I wonder if it makes a real difference with gcc. I've checked it and gcc 4.5 seem to produce identical number of instructions for both statements, regardless of the optimization level. Although it may also depend on the architecture version. The topic is presented in http://infocenter.arm.com/help/topic/com.arm.doc.dui0056d/DUI0056.pdf chapter 5.3. However this is really old book now :-) > [snip] > >>>> +static void s5pcsis_try_format(struct v4l2_mbus_framefmt *mf) >>>> +{ >>>> + struct csis_pix_format const *csis_fmt; >>>> + >>>> + csis_fmt = find_csis_format(mf); >>>> + if (csis_fmt == NULL) >>>> + csis_fmt =&s5pcsis_formats[0]; >>>> + >>>> + mf->code = csis_fmt->code; >>>> + v4l_bound_align_image(&mf->width, 1, CSIS_MAX_PIX_WIDTH, >>>> + csis_fmt->pix_hor_align, >>>> + &mf->height, 1, CSIS_MAX_PIX_HEIGHT, 1, >>>> + 0); >>>> +} >>>> + >>>> +static int s5pcsis_set_fmt(struct v4l2_subdev *sd, struct >>>> v4l2_subdev_fh *fh, + struct v4l2_subdev_format *fmt) >>>> +{ >>>> + struct csis_state *state = sd_to_csis_state(sd); >>>> + struct v4l2_mbus_framefmt *mf =&fmt->format; >>>> + struct csis_pix_format const *csis_fmt = find_csis_format(mf); >>>> + >>>> + v4l2_dbg(1, debug, sd, "%s: %dx%d, code: %x, csis_fmt: %p\n", >>>> + __func__, mf->width, mf->height, mf->code, csis_fmt); >>>> + >>>> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { >>>> + s5pcsis_try_format(mf); >>> >>> You need to take the pad into account here. As you mention below, source >>> and sink formats are identical. When the user tries to set the source >>> format, the driver should just return the sink format without performing >>> any modification. Yeah, that's right. Thanks for the hint. >>>> + state->mf[CSIS_FMT_TRY] = *mf; >>>> + return 0; >>>> + } >>>> + >>>> + /* Both source and sink pad have always same format. */ >>>> + if (!csis_pad_valid(fmt->pad) || >>>> + csis_fmt == NULL || >>>> + mf->width> CSIS_MAX_PIX_WIDTH || >>>> + mf->height> CSIS_MAX_PIX_HEIGHT || >>>> + mf->width& (u32)(csis_fmt->pix_hor_align - 1)) >>>> + return -EINVAL; >>> >>> Don't return an error, adjust the user supplied format instead. >>> >>>> + >>>> + mutex_lock(&state->lock); >>>> + state->mf[CSIS_FMT_ACTIVE] = *mf; >>>> + state->csis_fmt = csis_fmt; >>>> + mutex_unlock(&state->lock); >>> >>> The logic in this function is not correct. First of all, you need to >>> adjust the user-supplied format in all cases, regardless of the format >>> type (try/active). Then, as the formats on the sink and source pads are >>> always identical, you should return the sink pad format when the user >>> tries to set the source pad format. Setting the source pad format will >>> have no effect. Finally, you should store try formats in the subdev file >>> handle, not in the csis_state structure (see ccp2_set_format() in >>> drivers/media/video/omap3isp/ispccp2.c for an example, in your case you >>> don't need to propagate the format change from sink to source, as the >>> source always has the same format as the sink). >> >> Thanks, that's all very useful. The only thing I am concerned is what >> should be done when the file handle is null? Is it not allowed by design? > > It wasn't allowed by design. As explained above, we could replace the file > handle with another structure, and have the caller allocate it. In your case > you could just skip storing try formats. This needs to be thought about some > more. > >> I've initially reworked s5pcsis_set_fmt to something like this: >> >> static int s5pcsis_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh >> *fh, struct v4l2_subdev_format *fmt) >> { >> struct csis_state *state = sd_to_csis_state(sd); >> struct v4l2_mbus_framefmt *mf = &fmt->format; >> struct csis_pix_format const *csis_fmt; >> >> v4l2_dbg(1, debug, sd, "%s: %dx%d, code: %x\n", >> __func__, mf->width, mf->height, mf->code); >> >> if (!csis_pad_valid(fmt->pad)) >> return -EINVAL; >> >> mutex_lock(&state->lock); >> if (fmt->pad == CSIS_PAD_SOURCE) { >> fmt->format = state->format; >> goto unlock; >> } >> csis_fmt = s5pcsis_try_format(&fmt->format); > > Try try_format call doesn't require the mutex to be locked. You can > lock/unlock the mutex around fmt->format = state->format; above, and > lock/unlock it around state->formt = fmt->format; state->csis_fmt = csis_fmt; > below. > >> if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { >> /* TODO: store format in *fh */ >> goto unlock; >> } >> >> /* Common format for the source and the sink pad */ >> state->format = fmt->format; >> state->csis_fmt = csis_fmt; >> unlock: >> mutex_unlock(&state->lock); >> return 0; >> } Regards, -- Sylwester Nawrocki Samsung Poland R&D Center ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers 2011-05-05 9:33 ` Sylwester Nawrocki @ 2011-05-05 12:25 ` Laurent Pinchart 2011-05-05 16:06 ` Sylwester Nawrocki 0 siblings, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2011-05-05 12:25 UTC (permalink / raw) To: Sylwester Nawrocki Cc: Sylwester Nawrocki, linux-media, linux-samsung-soc, Kyungmin Park, Marek Szyprowski, Heungjun Kim, Sungchun Kang, Jonghun Han Hi Sylwester, On Thursday 05 May 2011 11:33:27 Sylwester Nawrocki wrote: > On 05/04/2011 02:00 PM, Laurent Pinchart wrote: > > On Tuesday 03 May 2011 20:07:43 Sylwester Nawrocki wrote: > >> On 05/03/2011 11:16 AM, Laurent Pinchart wrote: > >>> On Thursday 21 April 2011 17:21:04 Sylwester Nawrocki wrote: > > [snip] > > > >>>> + struct media_pad pads[CSIS_PADS_NUM]; > >>>> + struct v4l2_subdev sd; > >>>> + struct platform_device *pdev; > >>>> + struct resource *regs_res; > >>>> + void __iomem *regs; > >>>> + struct clk *clock[NUM_CSIS_CLOCKS]; > >>>> + int irq; > >>>> + struct regulator *supply; > >>>> + u32 flags; > >>>> + /* Common format for the source and sink pad. */ > >>>> + const struct csis_pix_format *csis_fmt; > >>>> + struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS]; > >>> > >>> As try formats are stored in the file handle, and as the formats on the > >>> sink and source pads are identical, a single v4l2_mbus_framefmt will do > >>> here. > >> > >> Ok. How about a situation when the caller never provides a file handle? > >> Is it not supposed to happen? > > > > Good question :-) The subdev pad-level operations have been designed with > > a userspace interface in mind, so they require a file handle to store > > try the formats (and crop rectangles). > > > >> For V4L2_SUBDEV_FORMAT_TRY, should set_fmt just abandon storing the > >> format and should get_fmt just return -EINVAL when passed fh == NULL ? > > > > For such a simple subdev, that should work as a workaround, yes. You can > > use it as a temporary solution at least. > > > >> Or should the host driver allocate the file handle just for the sake of > >> set_fmt/get_fmt calls (assuming that cropping ops are not supported > >> by the subdev) ? > > > > That's another solution. We could also pass an internal structure that > > contains formats and crop rectangles to the pad operations handlers, > > instead of passing the whole file handle. Do you think that would be > > better ? > > So it would then be an additional argument for the pad-level operations ? It would replace the file handle argument. > Perhaps that would make sense when both format and crop information is > needed at the same time in the subdev driver. However this would only be > required for TRY formats/crop rectangles which wouldn't be supported anyway > because of missing file handle.. or I missed something? The reason why we pass the file handle to the pad operations is to let drivers access formats/crop try settings that are stored in the file handle. If we moved those settings to a separate structure, and embedded that structure into the file handle structure, we could pass &fh->settings instead of fh to the pad operations. Drivers that want to call pad operations would then need to allocate a settings structure, instead of a complete fake fh. > Furthermore I assume more complex pipelines will be handled in user space The pad-level API has been designed to get/set formats/crop settings directly from userspace, not from inside the kernel, so that would certainly work. > and the host drivers could store format and crop information locally > directly from v4l2_subdev_format and v4l2_subdev_crop data structures. I'm not sure to understand what you mean there. > > Quoting one of your comments below, > > > > x--- FIMC_0 (/dev/video1) > > > > SENSOR -> MIPI_CSIS --| > > > > x--- FIMC_1 (/dev/video3) > > > > How do you expect to configure the MIPI_CSIS block from the FIMC_0 and > > FIMC_1 blocks, without any help from userspace ? Conflicts will need to > > be handled, and the best way to handle them is to have userspace > > configuring the MIPI_CSIS explicitly. > > My priority is to make work the configurations without device nodes at > sensor and MIPI CSIS subdevs. > > I agree it would be best to leave the negotiation logic to user space, > however I need to assure the regular V4L2 application also can use the > driver. > > My idea was to only try format at CSI slave and sensor subdevs when S_FMT > is called on a video node. So the sensor and CSIS constraints are taken > into account. > > Then from VIDIOC_STREAMON, formats at pipeline elements would be set on > subdevs without device node or validated on subdevs providing a device > node. For subdevs without device nodes, why don't you set the active format directly when S_FMT is called, instead of postponing the operation until VIDIOC_STREAMON time ? You wouldn't need to use TRY formats then. > Another issue is v4l2 controls. The subdevs are now in my case registered > to a v4l2_device instance embedded in the media device driver. The video > node drivers (FIMC0...FIMC3) have their own v4l2_device instances. So the > control inheritance between video node and a subdevs is gone :/, i.e. I > couldn't find a standard way to remove back from a parent control handler > the controls which have been added to it with v4l2_ctrl_handler_add(). I think you should expose sensor controls through subdev devices nodes, not through the V4L2 device node. > I've had similar issue with subdev -> v4l2_device notify callback. Before, > when the subdev was directly registered to a v4l2_instance associated with > a video node, v4l2_subdev_notify had been propagated straight to FIMC{N} > device the subdev was attached to. > Now I just redirect notifications ending up in the media device driver to > relevant FIMC{N} device instance depending on link configuration. > > >>>> +#define csis_pad_valid(pad) (pad == CSIS_PAD_SOURCE || pad == > >>>> CSIS_PAD_SINK) + > >>>> +static struct csis_state *sd_to_csis_state(struct v4l2_subdev *sdev) > >>>> +{ > >>>> + return container_of(sdev, struct csis_state, sd); > >>>> +} > >>>> + > >>>> +static const struct csis_pix_format *find_csis_format( > >>>> + struct v4l2_mbus_framefmt *mf) > >>>> +{ > >>>> + int i = ARRAY_SIZE(s5pcsis_formats); > >>>> + > >>>> + while (--i>= 0) > >>> > >>> I'm curious, why do you search backward instead of doing the usual > >>> > >>> for (i = 0; i< ARRAY_SIZE(s5pcsis_formats); ++i) > >>> > >>> (in that case 'i' could be unsigned) ? > >> > >> Perhaps doing it either way does not make any difference with the > >> toolchains we use, but the loops with test for 0 are supposed to be > >> faster on ARM. > > > > I didn't know that. I wonder if it makes a real difference with gcc. > > I've checked it and gcc 4.5 seem to produce identical number of > instructions for both statements, regardless of the optimization level. > Although it may also depend on the architecture version. > > The topic is presented in > http://infocenter.arm.com/help/topic/com.arm.doc.dui0056d/DUI0056.pdf > chapter 5.3. > However this is really old book now :-) Yes it is :-) (and I can't find this topic in chapter 5.3) -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers 2011-05-05 12:25 ` Laurent Pinchart @ 2011-05-05 16:06 ` Sylwester Nawrocki 0 siblings, 0 replies; 11+ messages in thread From: Sylwester Nawrocki @ 2011-05-05 16:06 UTC (permalink / raw) To: Laurent Pinchart Cc: Sylwester Nawrocki, linux-media, linux-samsung-soc, Kyungmin Park, Marek Szyprowski, Heungjun Kim, Sungchun Kang, Jonghun Han Hi Laurent, On 05/05/2011 02:25 PM, Laurent Pinchart wrote: > On Thursday 05 May 2011 11:33:27 Sylwester Nawrocki wrote: >> On 05/04/2011 02:00 PM, Laurent Pinchart wrote: >>> On Tuesday 03 May 2011 20:07:43 Sylwester Nawrocki wrote: >>>> On 05/03/2011 11:16 AM, Laurent Pinchart wrote: >>>>> On Thursday 21 April 2011 17:21:04 Sylwester Nawrocki wrote: >>> [snip] >>> >>>>>> + struct media_pad pads[CSIS_PADS_NUM]; >>>>>> + struct v4l2_subdev sd; >>>>>> + struct platform_device *pdev; >>>>>> + struct resource *regs_res; >>>>>> + void __iomem *regs; >>>>>> + struct clk *clock[NUM_CSIS_CLOCKS]; >>>>>> + int irq; >>>>>> + struct regulator *supply; >>>>>> + u32 flags; >>>>>> + /* Common format for the source and sink pad. */ >>>>>> + const struct csis_pix_format *csis_fmt; >>>>>> + struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS]; >>>>> >>>>> As try formats are stored in the file handle, and as the formats on the >>>>> sink and source pads are identical, a single v4l2_mbus_framefmt will do >>>>> here. >>>> >>>> Ok. How about a situation when the caller never provides a file handle? >>>> Is it not supposed to happen? >>> >>> Good question :-) The subdev pad-level operations have been designed with >>> a userspace interface in mind, so they require a file handle to store >>> try the formats (and crop rectangles). >>> >>>> For V4L2_SUBDEV_FORMAT_TRY, should set_fmt just abandon storing the >>>> format and should get_fmt just return -EINVAL when passed fh == NULL ? >>> >>> For such a simple subdev, that should work as a workaround, yes. You can >>> use it as a temporary solution at least. >>> >>>> Or should the host driver allocate the file handle just for the sake of >>>> set_fmt/get_fmt calls (assuming that cropping ops are not supported >>>> by the subdev) ? >>> >>> That's another solution. We could also pass an internal structure that >>> contains formats and crop rectangles to the pad operations handlers, >>> instead of passing the whole file handle. Do you think that would be >>> better ? >> >> So it would then be an additional argument for the pad-level operations ? > > It would replace the file handle argument. > >> Perhaps that would make sense when both format and crop information is >> needed at the same time in the subdev driver. However this would only be >> required for TRY formats/crop rectangles which wouldn't be supported anyway >> because of missing file handle.. or I missed something? > > The reason why we pass the file handle to the pad operations is to let drivers > access formats/crop try settings that are stored in the file handle. If we > moved those settings to a separate structure, and embedded that structure into > the file handle structure, we could pass &fh->settings instead of fh to the > pad operations. Drivers that want to call pad operations would then need to > allocate a settings structure, instead of a complete fake fh. I see, that sounds like a cleanest solution of the problem. > >> Furthermore I assume more complex pipelines will be handled in user space > > The pad-level API has been designed to get/set formats/crop settings directly > from userspace, not from inside the kernel, so that would certainly work. > >> and the host drivers could store format and crop information locally >> directly from v4l2_subdev_format and v4l2_subdev_crop data structures. > > I'm not sure to understand what you mean there. I meant that the adjusted format/crop rectangle is still returned in the pad operations, through the last argument; in struct v4l2_subdev_format::format or struct v4l2_subdev_crop::rect and these can be queried and interpreted by a host driver. But as you explain purpose of the fh is to aid subdev, not the host drivers. > >>> Quoting one of your comments below, >>> >>> x--- FIMC_0 (/dev/video1) >>> >>> SENSOR -> MIPI_CSIS --| >>> >>> x--- FIMC_1 (/dev/video3) >>> >>> How do you expect to configure the MIPI_CSIS block from the FIMC_0 and >>> FIMC_1 blocks, without any help from userspace ? Conflicts will need to >>> be handled, and the best way to handle them is to have userspace >>> configuring the MIPI_CSIS explicitly. >> >> My priority is to make work the configurations without device nodes at >> sensor and MIPI CSIS subdevs. >> >> I agree it would be best to leave the negotiation logic to user space, >> however I need to assure the regular V4L2 application also can use the >> driver. >> >> My idea was to only try format at CSI slave and sensor subdevs when S_FMT >> is called on a video node. So the sensor and CSIS constraints are taken >> into account. >> >> Then from VIDIOC_STREAMON, formats at pipeline elements would be set on >> subdevs without device node or validated on subdevs providing a device >> node. > > For subdevs without device nodes, why don't you set the active format directly > when S_FMT is called, instead of postponing the operation until > VIDIOC_STREAMON time ? You wouldn't need to use TRY formats then. In the configuration with two FIMC devices linked to MIPI CSIS and sensor as above, if one of the FIMC nodes is streaming, in e.g. camera preview mode, it would not be possible to set format and allocate buffers in the other (idle) video node for snapshot capture. On the other hand I would expect subdevs without a device node to be relatively simple, supporting only preview/monitor mode. Thus they would be used only with single FIMC, and format at them could be set directly from VIDIOC_S_FMT as you suggested. > >> Another issue is v4l2 controls. The subdevs are now in my case registered >> to a v4l2_device instance embedded in the media device driver. The video >> node drivers (FIMC0...FIMC3) have their own v4l2_device instances. So the >> control inheritance between video node and a subdevs is gone :/, i.e. I >> couldn't find a standard way to remove back from a parent control handler >> the controls which have been added to it with v4l2_ctrl_handler_add(). > > I think you should expose sensor controls through subdev devices nodes, not > through the V4L2 device node. OK, I am in favour of that. Yet I've got some persons to be convinced ;-) It might be a bit pointless to try to make a solution for set/get_fmt/crop pad operations without fh (because there is no subdev device node) and require a device node for the control operations, might it not? I think the controls could be handled quite cleanly with the control handler magic as proposed by Hans on IRC. > >> I've had similar issue with subdev -> v4l2_device notify callback. Before, >> when the subdev was directly registered to a v4l2_instance associated with >> a video node, v4l2_subdev_notify had been propagated straight to FIMC{N} >> device the subdev was attached to. >> Now I just redirect notifications ending up in the media device driver to >> relevant FIMC{N} device instance depending on link configuration. >> >>>>>> +#define csis_pad_valid(pad) (pad == CSIS_PAD_SOURCE || pad == >>>>>> CSIS_PAD_SINK) + >>>>>> +static struct csis_state *sd_to_csis_state(struct v4l2_subdev *sdev) >>>>>> +{ >>>>>> + return container_of(sdev, struct csis_state, sd); >>>>>> +} >>>>>> + >>>>>> +static const struct csis_pix_format *find_csis_format( >>>>>> + struct v4l2_mbus_framefmt *mf) >>>>>> +{ >>>>>> + int i = ARRAY_SIZE(s5pcsis_formats); >>>>>> + >>>>>> + while (--i>= 0) >>>>> >>>>> I'm curious, why do you search backward instead of doing the usual >>>>> >>>>> for (i = 0; i< ARRAY_SIZE(s5pcsis_formats); ++i) >>>>> >>>>> (in that case 'i' could be unsigned) ? >>>> >>>> Perhaps doing it either way does not make any difference with the >>>> toolchains we use, but the loops with test for 0 are supposed to be >>>> faster on ARM. >>> >>> I didn't know that. I wonder if it makes a real difference with gcc. >> >> I've checked it and gcc 4.5 seem to produce identical number of >> instructions for both statements, regardless of the optimization level. >> Although it may also depend on the architecture version. >> >> The topic is presented in >> http://infocenter.arm.com/help/topic/com.arm.doc.dui0056d/DUI0056.pdf >> chapter 5.3. >> However this is really old book now :-) > > Yes it is :-) > > (and I can't find this topic in chapter 5.3) Oops, that was a wrong link, sorry about that. Here is the right one: http://tinyurl.com/68l3ntx Best regards, -- Sylwester Nawrocki ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-05-05 16:07 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-04-21 15:21 [PATCH v4] Add v4l2 subdev driver for S5P MIPI-CSI receivers Sylwester Nawrocki 2011-04-21 15:21 ` [PATCH 1/3] v4l: Add V4L2_MBUS_FMT_JPEG_1X8 media bus format Sylwester Nawrocki 2011-04-21 15:21 ` [PATCH v4 2/3] v4l: Move S5P FIMC driver into Video Capture Devices Sylwester Nawrocki 2011-05-03 8:24 ` Laurent Pinchart 2011-04-21 15:21 ` [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers Sylwester Nawrocki 2011-05-03 9:16 ` Laurent Pinchart 2011-05-03 18:07 ` Sylwester Nawrocki 2011-05-04 12:00 ` Laurent Pinchart 2011-05-05 9:33 ` Sylwester Nawrocki 2011-05-05 12:25 ` Laurent Pinchart 2011-05-05 16:06 ` Sylwester Nawrocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox