[GIT FIXES for 4.20] Fwnode parsing fix
Hi Mauro, Here's a patch that fixes clearing fwnode flags. The patch that broke it is only in 4.20. Please pull. The following changes since commit 078ab3ea2c3bb69cb989d52346fefa1246055e5b: media: Add a Kconfig option for the Request API (2018-12-05 13:07:43 -0500) are available in the git repository at: ssh://linuxtv.org/git/sailus/media_tree.git tags/fixes-4.20-1-sign for you to fetch changes up to 7add7222d1741afb448a386c3085f38da801fd50: media: v4l2-fwnode: Fix setting V4L2_MBUS_DATA_ACTIVE_HIGH/LOW flag (2018-12-08 00:27:15 +0200) clear correct fwnode flags Ondrej Jirman (1): media: v4l2-fwnode: Fix setting V4L2_MBUS_DATA_ACTIVE_HIGH/LOW flag drivers/media/v4l2-core/v4l2-fwnode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- Sakari Ailus
Re: [PATCH 1/2] media: pxa_camera: don't deferenciate a NULL pointer
Hi Mauro,
On Fri, Dec 07, 2018 at 08:07:54AM -0500, Mauro Carvalho Chehab wrote:
> As warned by smatch:
> drivers/media/platform/pxa_camera.c:2400 pxa_camera_probe() error: we
> previously assumed 'pcdev->pdata' could be null (see line 2397)
>
> It would be possible that neither DT nor platform data would be
> provided. This is a Kernel bug, so warn about that and bail.
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/platform/pxa_camera.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/platform/pxa_camera.c
> b/drivers/media/platform/pxa_camera.c
> index 5f930560eb30..f91f8fd424c4 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -2396,6 +2396,9 @@ static int pxa_camera_probe(struct platform_device
> *pdev)
> pcdev->pdata = pdev->dev.platform_data;
> if (pdev->dev.of_node && !pcdev->pdata) {
> err = pxa_camera_pdata_from_dt(>dev, pcdev, >asd);
> + } else if (!pcdev->pdata) {
This fixes the issue, but the current checks remain a bit odd.
The driver seems to prefer platform data over OF. I wonder if that's
intentional or not.
In that case, I'd roughly write this as:
if (pcdev->pdata) {
...;
} else if (pdev->dev.of_node) {
...;
} else {
return -ENODEV;
}
I'm not sure WARN_ON(1) is necessary. A lot of drivers simply do it this
way without WARN_ON().
> + WARN_ON(1);
> + return -ENODEV;
> } else {
> pcdev->platform_flags = pcdev->pdata->flags;
> pcdev->mclk = pcdev->pdata->mclk_10khz * 1;
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
[PATCH v2 1/1] media: Add a Kconfig option for the Request API
The Request API is now merged to the kernel but the confidence on the
stability of that API is not great, especially regarding the interaction
with V4L2.
Add a Kconfig option for the API, with a scary-looking warning.
The patch itself disables request creation as well as does not advertise
them as buffer flags. The driver requiring requests (cedrus) now depends
on the Kconfig option as well.
Signed-off-by: Sakari Ailus
Acked-by: Hans Verkuil
---
since v1:
- Write out the #ifdef's in request creation
- The option's functionality was reversed in request creation, fixed that
drivers/media/Kconfig | 13 +
drivers/media/common/videobuf2/videobuf2-v4l2.c | 2 ++
drivers/media/media-device.c| 4
drivers/staging/media/sunxi/cedrus/Kconfig | 1 +
4 files changed, 20 insertions(+)
diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
index 8add62a18293..102eb35fcf3f 100644
--- a/drivers/media/Kconfig
+++ b/drivers/media/Kconfig
@@ -110,6 +110,19 @@ config MEDIA_CONTROLLER_DVB
This is currently experimental.
+config MEDIA_CONTROLLER_REQUEST_API
+ bool "Enable Media controller Request API (EXPERIMENTAL)"
+ depends on MEDIA_CONTROLLER && STAGING_MEDIA
+ default n
+ ---help---
+ DO NOT ENABLE THIS OPTION UNLESS YOU KNOW WHAT YOU'RE DOING.
+
+ This option enables the Request API for the Media controller and V4L2
+ interfaces. It is currently needed by a few stateless codec drivers.
+
+ There is currently no intention to provide API or ABI stability for
+ this new API as of yet.
+
#
# Video4Linux support
# Only enables if one of the V4L2 types (ATV, webcam, radio) is selected
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 1244c246d0c4..83c3c0c49e56 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -630,8 +630,10 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
*caps |= V4L2_BUF_CAP_SUPPORTS_USERPTR;
if (q->io_modes & VB2_DMABUF)
*caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF;
+#ifdef CONFIG_MEDIA_CONTROLLER_REQUEST_API
if (q->supports_requests)
*caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
+#endif
}
int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index bed24372e61f..b8ec88612df7 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -381,10 +381,14 @@ static long media_device_get_topology(struct media_device
*mdev, void *arg)
static long media_device_request_alloc(struct media_device *mdev,
int *alloc_fd)
{
+#ifdef CONFIG_MEDIA_CONTROLLER_REQUEST_API
if (!mdev->ops || !mdev->ops->req_validate || !mdev->ops->req_queue)
return -ENOTTY;
return media_request_alloc(mdev, alloc_fd);
+#else
+ return -ENOTTY;
+#endif
}
static long copy_arg_from_user(void *karg, void __user *uarg, unsigned int cmd)
diff --git a/drivers/staging/media/sunxi/cedrus/Kconfig
b/drivers/staging/media/sunxi/cedrus/Kconfig
index a7a34e89c42d..3252efa422f9 100644
--- a/drivers/staging/media/sunxi/cedrus/Kconfig
+++ b/drivers/staging/media/sunxi/cedrus/Kconfig
@@ -3,6 +3,7 @@ config VIDEO_SUNXI_CEDRUS
depends on VIDEO_DEV && VIDEO_V4L2 && MEDIA_CONTROLLER
depends on HAS_DMA
depends on OF
+ depends on MEDIA_CONTROLLER_REQUEST_API
select SUNXI_SRAM
select VIDEOBUF2_DMA_CONTIG
select V4L2_MEM2MEM_DEV
--
2.11.0
[PATCH 1/1] ipu3-cio2: Allow probe to succeed if there are no sensors connected
The device won't be powered off on systems that have no sensors connected unless it has a driver bound to it. Allow that to happen even if there are no sensors connected to cio2. Signed-off-by: Sakari Ailus --- drivers/media/pci/intel/ipu3/ipu3-cio2.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index 447baaebca448..e281e55cdca4a 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c @@ -1810,7 +1810,8 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, /* Register notifier for subdevices we care */ r = cio2_notifier_init(cio2); - if (r) + /* Proceed without sensors connected to allow the device to suspend. */ + if (r && r != -ENODEV) goto fail_cio2_queue_exit; r = devm_request_irq(_dev->dev, pci_dev->irq, cio2_irq, -- 2.11.0
[PATCH 1/1] media: Add a Kconfig option for the Request API
The Request API is now merged to the kernel but the confidence on the
stability of that API is not great, especially regarding the interaction
with V4L2.
Add a Kconfig option for the API, with a scary-looking warning.
The patch itself disables request creation as well as does not advertise
them as buffer flags. The driver requiring requests (cedrus) now depends
on the Kconfig option as well.
Signed-off-by: Sakari Ailus
---
I hope this covers now everything... I was thinking how to make the option
name itself more worrisome but I couldn't come up with a better language
that would be compact enough. The "(EXPERIMENTAL)" notion is a bit too
worn to be effective. :-I
The patch can and should be reverted once we're entirely happy and
confident with the API.
drivers/media/Kconfig | 13 +
drivers/media/common/videobuf2/videobuf2-v4l2.c | 2 ++
drivers/media/media-device.c| 2 ++
drivers/staging/media/sunxi/cedrus/Kconfig | 1 +
4 files changed, 18 insertions(+)
diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
index 8add62a18293..102eb35fcf3f 100644
--- a/drivers/media/Kconfig
+++ b/drivers/media/Kconfig
@@ -110,6 +110,19 @@ config MEDIA_CONTROLLER_DVB
This is currently experimental.
+config MEDIA_CONTROLLER_REQUEST_API
+ bool "Enable Media controller Request API (EXPERIMENTAL)"
+ depends on MEDIA_CONTROLLER && STAGING_MEDIA
+ default n
+ ---help---
+ DO NOT ENABLE THIS OPTION UNLESS YOU KNOW WHAT YOU'RE DOING.
+
+ This option enables the Request API for the Media controller and V4L2
+ interfaces. It is currently needed by a few stateless codec drivers.
+
+ There is currently no intention to provide API or ABI stability for
+ this new API as of yet.
+
#
# Video4Linux support
# Only enables if one of the V4L2 types (ATV, webcam, radio) is selected
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 1244c246d0c4..83c3c0c49e56 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -630,8 +630,10 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
*caps |= V4L2_BUF_CAP_SUPPORTS_USERPTR;
if (q->io_modes & VB2_DMABUF)
*caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF;
+#ifdef CONFIG_MEDIA_CONTROLLER_REQUEST_API
if (q->supports_requests)
*caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
+#endif
}
int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index bed24372e61f..2ef114ce38d0 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -381,7 +381,9 @@ static long media_device_get_topology(struct media_device
*mdev, void *arg)
static long media_device_request_alloc(struct media_device *mdev,
int *alloc_fd)
{
+#ifndef CONFIG_MEDIA_CONTROLLER_REQUEST_API
if (!mdev->ops || !mdev->ops->req_validate || !mdev->ops->req_queue)
+#endif
return -ENOTTY;
return media_request_alloc(mdev, alloc_fd);
diff --git a/drivers/staging/media/sunxi/cedrus/Kconfig
b/drivers/staging/media/sunxi/cedrus/Kconfig
index a7a34e89c42d..3252efa422f9 100644
--- a/drivers/staging/media/sunxi/cedrus/Kconfig
+++ b/drivers/staging/media/sunxi/cedrus/Kconfig
@@ -3,6 +3,7 @@ config VIDEO_SUNXI_CEDRUS
depends on VIDEO_DEV && VIDEO_V4L2 && MEDIA_CONTROLLER
depends on HAS_DMA
depends on OF
+ depends on MEDIA_CONTROLLER_REQUEST_API
select SUNXI_SRAM
select VIDEOBUF2_DMA_CONTIG
select V4L2_MEM2MEM_DEV
--
2.11.0
[GIT PULL for 4.21] More sensor driver patches
Hi Mauro,
Here are improvements for various sensor drivers for 4.21. There area few
trivial V4L2 fwnode and async framework changes as well, plus DT binding
documentation for mt9m111.
Please pull.
The following changes since commit 9b90dc85c718443a3e573a0ccf55900ff4fa73ae:
media: seco-cec: add missing header file to fix build (2018-12-03 15:11:00
-0500)
are available in the git repository at:
ssh://linuxtv.org/git/sailus/media_tree.git tags/for-4.21-4-sign
for you to fetch changes up to bf8da26398f03704bbf7bb10b9c847fd187f4260:
v4l2: async: remove locking when initializing async notifier (2018-12-05
12:10:24 +0200)
sensor driver patches and stuff for 4.21
Bingbu Cao (3):
media: imx319: fix wrong order in test pattern menus
media: imx355: fix wrong order in test pattern menus
media: unify some sony camera sensors pattern naming
Enrico Scholz (1):
media: mt9m111: allow to setup pixclk polarity
Fabio Estevam (1):
media: v4l2-fwnode: Demote warning to debug level
Jacopo Mondi (1):
media: ov5640: Fix set format regression
Luca Ceresoli (3):
media: imx274: fix stack corruption in imx274_read_reg
media: imx274: declare the correct number of controls
media: imx274: select REGMAP_I2C
Marco Felsch (3):
media: mt9m111: add s_stream callback
dt-bindings: media: mt9m111: adapt documentation to be more clear
dt-bindings: media: mt9m111: add pclk-sample property
Maxime Ripard (11):
media: ov5640: Adjust the clock based on the expected rate
media: ov5640: Remove the clocks registers initialization
media: ov5640: Remove redundant defines
media: ov5640: Remove redundant register setup
media: ov5640: Compute the clock rate at runtime
media: ov5640: Remove pixel clock rates
media: ov5640: Enhance FPS handling
media: ov5640: Make the return rate type more explicit
media: ov5640: Make the FPS clamping / rounding more extendable
media: ov5640: Add 60 fps support
media: ov5640: Remove duplicate auto-exposure setup
Michael Grzeschik (2):
media: mt9m111: add streaming check to set_fmt
media: mt9m111: add support to select formats and fps for {Q,SXGA}
Niklas Söderlund (1):
v4l2: async: remove locking when initializing async notifier
.../devicetree/bindings/media/i2c/mt9m111.txt | 13 +-
drivers/media/i2c/Kconfig | 2 +
drivers/media/i2c/imx258.c | 8 +-
drivers/media/i2c/imx274.c | 9 +-
drivers/media/i2c/imx319.c | 8 +-
drivers/media/i2c/imx355.c | 8 +-
drivers/media/i2c/mt9m111.c| 222 +-
drivers/media/i2c/ov5640.c | 764 -
drivers/media/v4l2-core/v4l2-async.c | 4 -
drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
10 files changed, 703 insertions(+), 337 deletions(-)
--
Regards,
Sakari Ailus
[PATCH 1/1] v4l uapi: Make "Vertical Colour Bars" menu item a little more generic
The test pattern could contain a different number of colour bars than
eight, make the entry more useful by removing "Eight " from the name.
Signed-off-by: Sakari Ailus
---
include/uapi/linux/v4l2-controls.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/v4l2-controls.h
b/include/uapi/linux/v4l2-controls.h
index acb2a57fa5d6..88f2759c2ce4 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1016,7 +1016,7 @@ enum v4l2_jpeg_chroma_subsampling {
#define V4L2_CID_TEST_PATTERN (V4L2_CID_IMAGE_PROC_CLASS_BASE
+ 3)
#define V4L2_TEST_PATTERN_DISABLED "Disabled"
#define V4L2_TEST_PATTERN_SOLID_COLOUR "Solid Colour"
-#define V4L2_TEST_PATTERN_VERT_COLOUR_BARS "Eight Vertical Colour Bars"
+#define V4L2_TEST_PATTERN_VERT_COLOUR_BARS "Vertical Colour Bars"
#define V4L2_TEST_PATTERN_VERT_COLOUR_BARS_FADE_TO_GREY "Colour Bars With Fade
to Grey"
#define V4L2_TEST_PATTERN_PN9 "Pseudorandom Sequence (PN9)"
#define V4L2_CID_DEINTERLACING_MODE(V4L2_CID_IMAGE_PROC_CLASS_BASE
+ 4)
--
2.11.0
Re: [PATCH 1/1] media: Use common test pattern menu entries
On Tue, Nov 27, 2018 at 07:19:52PM +0800, Bingbu Cao wrote:
>
> On 11/27/2018 05:33 PM, Sakari Ailus wrote:
> > diff --git a/include/uapi/linux/v4l2-controls.h
> > b/include/uapi/linux/v4l2-controls.h
> > index 998983a6e6b7..a74ff6f1ac88 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -1014,6 +1014,11 @@ enum v4l2_jpeg_chroma_subsampling {
> > #define V4L2_CID_LINK_FREQ
> > (V4L2_CID_IMAGE_PROC_CLASS_BASE + 1)
> > #define V4L2_CID_PIXEL_RATE
> > (V4L2_CID_IMAGE_PROC_CLASS_BASE + 2)
> > #define V4L2_CID_TEST_PATTERN
> > (V4L2_CID_IMAGE_PROC_CLASS_BASE + 3)
> > +#define V4L2_TEST_PATTERN_DISABLED "Disabled"
> > +#define V4L2_TEST_PATTERN_SOLID_COLOUR "Solid Colour"
> > +#define V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS "Eight Vertical
> > Colour Bars"
> > +#define V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY "Colour Bars
> > With Fade to Grey"
> > +#define V4L2_TEST_PATTERN_PN9 "Pseudorandom Sequence
> > (PN9)"
> More padding here for alignment?
Fixed in v2.
--
Sakari Ailus
[PATCH v2 1/1] media: Use common test pattern menu entries
While the test pattern menu itself is not standardised, many devices
support the same test patterns. Aligning the menu entries helps the user
space to use the interface, and adding macros for the menu entry strings
helps to keep them aligned.
Signed-off-by: Sakari Ailus
---
since v1:
- Fix indentation of menu strings
- Remove "8" from the macro names
drivers/media/i2c/imx258.c | 10 +-
drivers/media/i2c/imx319.c | 10 +-
drivers/media/i2c/imx355.c | 10 +-
drivers/media/i2c/ov2640.c | 4 ++--
drivers/media/i2c/smiapp/smiapp-core.c | 10 +-
include/uapi/linux/v4l2-controls.h | 5 +
6 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index f86ae18bc104..df5f016cebd9 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -498,11 +498,11 @@ static const struct imx258_reg mode_1048_780_regs[] = {
};
static const char * const imx258_test_pattern_menu[] = {
- "Disabled",
- "Solid Colour",
- "Eight Vertical Colour Bars",
- "Colour Bars With Fade to Grey",
- "Pseudorandom Sequence (PN9)",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_SOLID_COLOUR,
+ V4L2_TEST_PATTERN_VERT_COLOUR_BARS,
+ V4L2_TEST_PATTERN_VERT_COLOUR_BARS_FADE_TO_GREY,
+ V4L2_TEST_PATTERN_PN9,
};
/* Configurations for supported link frequencies */
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 17c2e4b41221..d9d4176b9d37 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -1647,11 +1647,11 @@ static const struct imx319_reg mode_1280x720_regs[] = {
};
static const char * const imx319_test_pattern_menu[] = {
- "Disabled",
- "Solid Colour",
- "Eight Vertical Colour Bars",
- "Colour Bars With Fade to Grey",
- "Pseudorandom Sequence (PN9)",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_SOLID_COLOUR,
+ V4L2_TEST_PATTERN_VERT_COLOUR_BARS,
+ V4L2_TEST_PATTERN_VERT_COLOUR_BARS_FADE_TO_GREY,
+ V4L2_TEST_PATTERN_PN9,
};
/* supported link frequencies */
diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index bed293b60e50..99138a291cb8 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -875,11 +875,11 @@ static const struct imx355_reg mode_820x616_regs[] = {
};
static const char * const imx355_test_pattern_menu[] = {
- "Disabled",
- "Solid Colour",
- "Eight Vertical Colour Bars",
- "Colour Bars With Fade to Grey",
- "Pseudorandom Sequence (PN9)",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_SOLID_COLOUR,
+ V4L2_TEST_PATTERN_VERT_COLOUR_BARS,
+ V4L2_TEST_PATTERN_VERT_COLOUR_BARS_FADE_TO_GREY,
+ V4L2_TEST_PATTERN_PN9,
};
/* supported link frequencies */
diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index 5d2d6735cc78..65058d9a5d51 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -707,8 +707,8 @@ static int ov2640_reset(struct i2c_client *client)
}
static const char * const ov2640_test_pattern_menu[] = {
- "Disabled",
- "Eight Vertical Colour Bars",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_VERT_COLOUR_BARS,
};
/*
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
b/drivers/media/i2c/smiapp/smiapp-core.c
index 58a45c353e27..5c9bcc9438ec 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -409,11 +409,11 @@ static void smiapp_update_mbus_formats(struct
smiapp_sensor *sensor)
}
static const char * const smiapp_test_patterns[] = {
- "Disabled",
- "Solid Colour",
- "Eight Vertical Colour Bars",
- "Colour Bars With Fade to Grey",
- "Pseudorandom Sequence (PN9)",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_SOLID_COLOUR,
+ V4L2_TEST_PATTERN_VERT_COLOUR_BARS,
+ V4L2_TEST_PATTERN_VERT_COLOUR_BARS_FADE_TO_GREY,
+ V4L2_TEST_PATTERN_PN9,
};
static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
diff --git a/include/uapi/linux/v4l2-controls.h
b/include/uapi/linux/v4l2-controls.h
index 998983a6e6b7..acb2a57fa5d6 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1014,6 +1014,11 @@ enum v4l2_jpeg_chroma_subsampling {
#define V4L2_CID_LINK_FREQ (V4L2_CID_IMAGE_PROC_CLASS_BASE
+ 1)
#define V4L2_CID_PIXEL_RATE(V4L2_CID_IMAGE_PROC_CLASS_BASE
+ 2)
#define V4L2_CID_TEST_PATTERN (V4L2_CID_IMAGE_PROC_CLASS_BASE
+ 3)
+#define V4L2_TEST_PATTERN_DISABLED
Re: [PATCH v2] v4l2-ioctl: Zero v4l2_plane_pix_format reserved fields
On Tue, Nov 27, 2018 at 07:07:56PM -0300, Ezequiel Garcia wrote:
> Make the core set the reserved fields to zero in
> vv4l2_pix_format_mplane.4l2_plane_pix_format,
> for _MPLANE queue types.
>
> Moving this to the core avoids having to do so in each
> and every driver.
>
> Suggested-by: Tomasz Figa
> Signed-off-by: Ezequiel Garcia
> --
> v2:
> * Drop unneeded clear in g_fmt.
> The sturct was already being cleared here.
> * Only zero plane_fmt reserved fields.
> * Use CLEAR_FIELD_MACRO.
>
> drivers/media/v4l2-core/v4l2-ioctl.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index e384142d2826..2506b602481f 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1512,6 +1512,7 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
> struct v4l2_format *p = arg;
> struct video_device *vfd = video_devdata(file);
> int ret = check_fmt(file, p->type);
> + int i;
unsigned int; same below.
With that,
Acked-by: Sakari Ailus
>
> if (ret)
> return ret;
> @@ -1536,6 +1537,8 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
> if (unlikely(!ops->vidioc_s_fmt_vid_cap_mplane))
> break;
> CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
> + for (i = 0; i < p->fmt.pix_mp.num_planes; i++)
> + CLEAR_AFTER_FIELD(p,
> fmt.pix_mp.plane_fmt[i].bytesperline);
> return ops->vidioc_s_fmt_vid_cap_mplane(file, fh, arg);
> case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> if (unlikely(!ops->vidioc_s_fmt_vid_overlay))
> @@ -1564,6 +1567,8 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
> if (unlikely(!ops->vidioc_s_fmt_vid_out_mplane))
> break;
> CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
> + for (i = 0; i < p->fmt.pix_mp.num_planes; i++)
> + CLEAR_AFTER_FIELD(p,
> fmt.pix_mp.plane_fmt[i].bytesperline);
> return ops->vidioc_s_fmt_vid_out_mplane(file, fh, arg);
> case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
> if (unlikely(!ops->vidioc_s_fmt_vid_out_overlay))
> @@ -1604,6 +1609,7 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
> {
> struct v4l2_format *p = arg;
> int ret = check_fmt(file, p->type);
> + int i;
>
> if (ret)
> return ret;
> @@ -1623,6 +1629,8 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
> if (unlikely(!ops->vidioc_try_fmt_vid_cap_mplane))
> break;
> CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
> + for (i = 0; i < p->fmt.pix_mp.num_planes; i++)
> + CLEAR_AFTER_FIELD(p,
> fmt.pix_mp.plane_fmt[i].bytesperline);
> return ops->vidioc_try_fmt_vid_cap_mplane(file, fh, arg);
> case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> if (unlikely(!ops->vidioc_try_fmt_vid_overlay))
> @@ -1651,6 +1659,8 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
> if (unlikely(!ops->vidioc_try_fmt_vid_out_mplane))
> break;
> CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
> + for (i = 0; i < p->fmt.pix_mp.num_planes; i++)
> + CLEAR_AFTER_FIELD(p,
> fmt.pix_mp.plane_fmt[i].bytesperline);
> return ops->vidioc_try_fmt_vid_out_mplane(file, fh, arg);
> case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
> if (unlikely(!ops->vidioc_try_fmt_vid_out_overlay))
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v9 04/13] media: staging/imx7: add MIPI CSI-2 receiver subdev for i.MX7
goto disable_clock;
> +
> + state->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> + state->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_pads_init(>mipi_sd.entity, CSIS_PADS_NUM,
> + state->pads);
> + if (ret < 0)
> + goto unregister_subdev;
> +
> + memcpy(state->events, mipi_csis_events, sizeof(state->events));
> +
> + pm_runtime_enable(dev);
> + if (!pm_runtime_enabled(dev)) {
> + ret = mipi_csis_pm_resume(dev, true);
> + if (ret < 0)
> + goto unregister_all;
> + }
> +
> + dev_info(>dev, "lanes: %d, hs_settle: %d, wclk: %d, freq: %u\n",
> + state->bus.num_data_lanes, state->hs_settle,
> + state->wrap_clk ? 1 : 0, state->clk_frequency);
> + return 0;
> +
> +unregister_all:
> + media_entity_cleanup(>mipi_sd.entity);
> +unregister_subdev:
> + v4l2_async_unregister_subdev(>mipi_sd);
> +disable_clock:
> + mipi_csis_clk_disable(state);
> +
> + return ret;
> +}
> +
> +static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct v4l2_subdev *mipi_sd = platform_get_drvdata(pdev);
> + struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
> + int ret = 0;
> +
> + mutex_lock(>lock);
> + if (state->flags & ST_POWERED) {
> + mipi_csis_stop_stream(state);
> + ret = regulator_disable(state->mipi_phy_regulator);
> + if (ret)
> + goto unlock;
> + mipi_csis_clk_disable(state);
> + state->flags &= ~ST_POWERED;
> + if (!runtime)
> + state->flags |= ST_SUSPENDED;
> + }
> +
> + unlock:
> + mutex_unlock(>lock);
> +
> + return ret ? -EAGAIN : 0;
> +}
> +
> +static int mipi_csis_pm_resume(struct device *dev, bool runtime)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct v4l2_subdev *mipi_sd = platform_get_drvdata(pdev);
> + struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
> + int ret = 0;
> +
> + mutex_lock(>lock);
> + if (!runtime && !(state->flags & ST_SUSPENDED))
> + goto unlock;
> +
> + if (!(state->flags & ST_POWERED)) {
> + ret = regulator_enable(state->mipi_phy_regulator);
> + if (ret)
> + goto unlock;
> +
> + state->flags |= ST_POWERED;
> + mipi_csis_clk_enable(state);
> + }
> + if (state->flags & ST_STREAMING)
> + mipi_csis_start_stream(state);
> +
> + state->flags &= ~ST_SUSPENDED;
> +
> + unlock:
> + mutex_unlock(>lock);
> +
> + return ret ? -EAGAIN : 0;
> +}
> +
> +static int mipi_csis_suspend(struct device *dev)
> +{
> + return mipi_csis_pm_suspend(dev, false);
> +}
> +
> +static int mipi_csis_resume(struct device *dev)
> +{
> + return mipi_csis_pm_resume(dev, false);
> +}
> +
> +static int mipi_csis_runtime_suspend(struct device *dev)
> +{
> + return mipi_csis_pm_suspend(dev, true);
> +}
> +
> +static int mipi_csis_runtime_resume(struct device *dev)
> +{
> + return mipi_csis_pm_resume(dev, true);
> +}
> +
> +static int mipi_csis_remove(struct platform_device *pdev)
> +{
> + struct v4l2_subdev *mipi_sd = platform_get_drvdata(pdev);
> + struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
> +
> + v4l2_async_unregister_subdev(>mipi_sd);
> + v4l2_async_notifier_unregister(>subdev_notifier);
> +
> + pm_runtime_disable(>dev);
> + mipi_csis_pm_suspend(>dev, true);
> + mipi_csis_clk_disable(state);
> + media_entity_cleanup(>mipi_sd.entity);
> + pm_runtime_set_suspended(>dev);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops mipi_csis_pm_ops = {
> + SET_RUNTIME_PM_OPS(mipi_csis_runtime_suspend, mipi_csis_runtime_resume,
> +NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(mipi_csis_suspend, mipi_csis_resume)
> +};
> +
> +static const struct of_device_id mipi_csis_of_match[] = {
> + { .compatible = "fsl,imx7-mipi-csi2", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mipi_csis_of_match);
> +
> +static struct platform_driver mipi_csis_driver = {
> + .probe = mipi_csis_probe,
> + .remove = mipi_csis_remove,
> + .driver = {
> + .of_match_table = mipi_csis_of_match,
> + .name = CSIS_DRIVER_NAME,
> + .pm = _csis_pm_ops,
> + },
> +};
> +
> +module_platform_driver(mipi_csis_driver);
> +
> +MODULE_DESCRIPTION("i.MX7 MIPI CSI-2 Receiver driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:imx7-mipi-csi2");
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH] media: unify some sony camera sensors pattern naming
Hi Bing Bu, Tomasz,
On Mon, Dec 03, 2018 at 10:53:34AM +0800, Bingbu Cao wrote:
>
>
> On 12/01/2018 02:08 AM, Tomasz Figa wrote:
> > Hi Bingbu,
> >
> > On Mon, Nov 26, 2018 at 7:56 PM wrote:
> > > From: Bingbu Cao
> > >
> > > Some Sony camera sensors have same test pattern
> > > definitions, this patch unify the pattern naming
> > > to make it more clear to the userspace.
> > >
> > > Suggested-by: Sakari Ailus
> > > Signed-off-by: Bingbu Cao
> > > ---
> > > drivers/media/i2c/imx258.c | 8
> > > drivers/media/i2c/imx319.c | 8
> > > drivers/media/i2c/imx355.c | 8
> > > 3 files changed, 12 insertions(+), 12 deletions(-)
> > >
> > Thanks for the patch! One comment inline.
> >
> > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > > index 31a1e2294843..a8a2880c6b4e 100644
> > > --- a/drivers/media/i2c/imx258.c
> > > +++ b/drivers/media/i2c/imx258.c
> > > @@ -504,10 +504,10 @@ struct imx258_mode {
> > >
> > > static const char * const imx258_test_pattern_menu[] = {
> > > "Disabled",
> > > - "Color Bars",
> > > - "Solid Color",
> > > - "Grey Color Bars",
> > > - "PN9"
> > > + "Solid Colour",
> > > + "Eight Vertical Colour Bars",
> > Is it just me or "solid color" and "color bars" are being swapped
> > here? Did the driver had the names mixed up before or the order of
> > modes is different between these sensors?
> The test pattern value order of the 3 camera sensors should be same.
> All are:
> 0 - Disabled
> 1 - Solid Colour
> 2 - Eight Vertical Colour Bars
> ...
>
> This patch swapped the first 2 item for imx258 (wrong order before) and use
> unified
> name for all 3 sensors.
I guess this isn't based on Jason's patch (now merged) that fixed the
issue. I'll rebase this; it's trivial.
commit 53f6f81da7db96557fe2bff9b15bd6b83d301f9f
Author: Chen, JasonX Z
Date: Wed Nov 7 21:47:34 2018 -0500
media: imx258: remove test pattern map from driver
change bayer order when using test pattern mode.
remove test pattern mapping method
[Sakari Ailus: Drop extra added newline]
Signed-off-by: Chen, JasonX Z
Signed-off-by: Sakari Ailus
Signed-off-by: Mauro Carvalho Chehab
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
Hi Laurent, On Fri, Nov 30, 2018 at 01:07:53AM +0200, Laurent Pinchart wrote: > Hello Bing, > > On Wednesday, 7 November 2018 06:16:47 EET Bing Bu Cao wrote: > > On 11/01/2018 08:03 PM, Sakari Ailus wrote: > > > On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote: > > [snip] > > > >> ImgU media topology print: > > >> > > >> # media-ctl -d /dev/media0 -p > > >> Media controller API version 4.19.0 > > >> > > >> Media device information > > >> > > >> driver ipu3-imgu > > >> model ipu3-imgu > > >> serial > > >> bus infoPCI::00:05.0 > > >> hw revision 0x80862015 > > >> driver version 4.19.0 > > >> > > >> Device topology > > >> - entity 1: ipu3-imgu 0 (5 pads, 5 links) > > >> type V4L2 subdev subtype Unknown flags 0 > > >> device node name /dev/v4l-subdev0 > > >> pad0: Sink > > >> [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown > > > > > > This doesn't seem right. Which formats can be enumerated from the pad? > > > > > >> crop:(0,0)/1920x1080 > > >> compose:(0,0)/1920x1080] > > > > > > Does the compose rectangle affect the scaling on all outputs? > > > > Sakari, driver use crop and compose targets to help set input-feeder and BDS > > output resolutions which are 2 key block of whole imaging pipeline, not the > > actual ending output, but they will impact the final output. > > > > >> <- "ipu3-imgu 0 input":0 [] > > > > > > Are there links that have no useful link configuration? If so, you should > > > set them enabled and immutable in the driver. > > > > The enabled status of input pads is used to get which pipe that user is > > trying to enable (ipu3_link_setup()), so it could not been set as immutable. > > Each pipe needs an input in order to operate, so from that point of view the > input is mandatory. Why can't we make this link immutable, and use the stream > state (VIDIOC_STREAMON/VIDIOC_STREAMOFF) to enable/disable the pipes ? There are only two options (AFAIK) in choosing the firmware, and by configuring the links this is better visible to the user: the links the state of which can be changed are not immutable. The driver can also obtain the explicit pipeline configuration, which makes the implementation more simple. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
Hi Yong,
On Thu, Nov 29, 2018 at 11:06:23PM +, Zhi, Yong wrote:
> Hi, Sakari,
>
> > -Original Message-
> > From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> > Sent: Thursday, November 29, 2018 4:46 PM
> > To: Zhi, Yong
> > Cc: linux-media@vger.kernel.org; tf...@chromium.org;
> > mche...@kernel.org; hans.verk...@cisco.com;
> > laurent.pinch...@ideasonboard.com; Mani, Rajmohan
> > ; Zheng, Jian Xu ; Hu,
> > Jerry W ; Toivonen, Tuukka
> > ; Qiu, Tian Shu ; Cao,
> > Bingbu ; Li, Chao C
> > Subject: Re: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
> >
> > Hi Yong,
> >
> > On Fri, Nov 16, 2018 at 10:37:00PM +, Zhi, Yong wrote:
> > ...
> > > > > +/**
> > > > > + * struct ipu3_uapi_shd_grid_config - Bayer shading(darkening)
> > > > > +correction
> > > > > + *
> > > > > + * @width: Grid horizontal dimensions, u8, [8, 128], default 73
> > > > > + * @height: Grid vertical dimensions, u8, [8, 128], default 56
> > > > > + * @block_width_log2:Log2 of the width of the grid cell in
> > > > > pixel
> > > > count
> > > > > + * u4, [0, 15], default value 5.
> > > > > + * @__reserved0: reserved
> > > > > + * @block_height_log2: Log2 of the height of the grid cell in
> > > > > pixel
> > > > count
> > > > > + * u4, [0, 15], default value 6.
> > > > > + * @__reserved1: reserved
> > > > > + * @grid_height_per_slice: SHD_MAX_CELLS_PER_SET/width.
> > > > > + * (with SHD_MAX_CELLS_PER_SET =
> > 146).
> > > > > + * @x_start: X value of top left corner of sensor relative to ROI
> > > > > + * u12, [-4096, 0]. default 0, only negative values.
> > > > > + * @y_start: Y value of top left corner of sensor relative to ROI
> > > > > + * u12, [-4096, 0]. default 0, only negative values.
> > > >
> > > > I suppose u12 is incorrect here, if the value is signed --- and
> > > > negative (sign bit) if not 0?
> > > >
> > >
> > > The value will be written to 13 bit register, should use s12.0.
> >
> > If you have s12, that means the most significant bit is the sign bit. So if
> > the
> > smallest value is -4096, you'd need s13.
> >
> > But where is the sign bit, i.e. is this either s13 or s16?
> >
>
> The notation of s12.0 means 13 bit with fraction bit as 0 right?
In s12.0, bit 11 is the sign bit, and bits 10--0 are the integer part. The
smallest number that can be represented is thus -2048 (not -4096).
>
> > >
> > > > > + */
> > > > > +struct ipu3_uapi_shd_grid_config {
> > > > > + /* reg 0 */
> > > > > + __u8 width;
> > > > > + __u8 height;
> > > > > + __u8 block_width_log2:3;
> > > > > + __u8 __reserved0:1;
> > > > > + __u8 block_height_log2:3;
> > > > > + __u8 __reserved1:1;
> > > > > + __u8 grid_height_per_slice;
> > > > > + /* reg 1 */
> > > > > + __s16 x_start;
> > > > > + __s16 y_start;
> > > > > +} __packed;
> >
> > ...
> >
> > > > > +/**
> > > > > + * struct ipu3_uapi_iefd_cux2_1 - Calculate power of non-directed
> > denoise
> > > > > + * element apply.
> > > > > + * @x0: X0 point of Config Unit, u9.0, default 0.
> > > > > + * @x1: X1 point of Config Unit, u9.0, default 0.
> > > > > + * @a01: Slope A of Config Unit, s4.4, default 0.
> > > >
> > > > The field is marked unsigned below. Which one is correct?
> > > >
> > >
> > > They are both correct, however, s4.4 is the internal representation
> > > used by CU, the inputs are unsigned, I will add a note in v8, same
> > > applies to the few other places as you commented.
> >
> > I still find this rather confusing. Is there a sign bit or is there not?
> >
>
> It's unsigned number from driver perspective, all CU inputs are unsigned,
> however, they will be "converted" to signed for FW/HW to use. I have to
> consult FW expert if more clarification is needed.
I think that would be good to have; if you somehow convert an unsigned
integer to a negative number, there's more than just the type cast there.
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
Hi Laurent, On Fri, Nov 30, 2018 at 01:09:37AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > > On Friday, 9 November 2018 12:09:54 EET Sakari Ailus wrote: > > On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote: > > > On 11/01/2018 08:03 PM, Sakari Ailus wrote: > > >> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote: > > [snip] > > > >>> ImgU media topology print: > > >>> > > >>> # media-ctl -d /dev/media0 -p > > >>> Media controller API version 4.19.0 > > >>> > > >>> Media device information > > >>> > > >>> driver ipu3-imgu > > >>> model ipu3-imgu > > >>> serial > > >>> bus infoPCI::00:05.0 > > >>> hw revision 0x80862015 > > >>> driver version 4.19.0 > > >>> > > >>> Device topology > > >>> - entity 1: ipu3-imgu 0 (5 pads, 5 links) > > >>> type V4L2 subdev subtype Unknown flags 0 > > >>> device node name /dev/v4l-subdev0 > > >>> pad0: Sink > > >>> [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown > > >> > > >> This doesn't seem right. Which formats can be enumerated from the pad? > > > > Looking at the code, the OUTPUT video nodes have 10-bit GRBG (or a variant) > > format whereas the CAPTURE video nodes always have NV12. Can you confirm? > > > > If the OUTPUT video node format selection has no effect on the rest of the > > pipeline (device capabilities, which processing blocks are in use, CAPTURE > > video nodes formats etc.), I think you could simply use the FIXED media bus > > code for each pad. That would actually make sense: this device always works > > from memory to memory, and thus does not really have a pixel data bus > > external to the device which is what the media bus codes really are for. > > Isn't the Bayer variant useful information to configure debayering ? I would > expect it to be passed through the format on pad 0. That's already configured on the video node. The FIXED media bus code is intended for links where there's nothing to configure --- which is the case here. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v7 09/16] intel-ipu3: css: Add support for firmware management
Hi Yong, On Mon, Oct 29, 2018 at 03:23:03PM -0700, Yong Zhi wrote: ... > diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-fw.h > b/drivers/media/pci/intel/ipu3/ipu3-css-fw.h > new file mode 100644 > index 000..954bb31 > --- /dev/null > +++ b/drivers/media/pci/intel/ipu3/ipu3-css-fw.h > @@ -0,0 +1,188 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright (C) 2018 Intel Corporation */ > + > +#ifndef __IPU3_CSS_FW_H > +#define __IPU3_CSS_FW_H > + > +/*** Firmware file definitions ***/ > + > +#define IMGU_FW_NAME "ipu3-fw.bin" Shouldn't this be "intel/ipu3-fw.bin"? -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH for v4.20 0/5] vb2 fixes (mostly request API related)
On Wed, Nov 28, 2018 at 09:37:42AM +0100, hverkuil-ci...@xs4all.nl wrote: > From: Hans Verkuil > > While improving the v4l2-compliance tests I came across several vb2 > problems. > > After modifying v4l2-compliance I was now able to use the vivid error > injection feature to test what happens if VIDIOC_STREAMON fails and > a following STREAMON succeeds. > > This generated patches 1/5 and 4/5+5/5. > > Patch 1/5 fixes an issue that was never noticed before since it didn't > result in kernel oopses or warnings, but after the second STREAMON it > won't call start_streaming anymore until you call REQBUFS(0) or close > the device node. > > Patches 4 and 5 are request specific fixes for the same corner case: > the code would unbind (in vb2) or complete (in vivid) a request if > start_streaming fails, but it should just leave things as-is. The > buffer is just put back in the queue, ready for the next attempt at > STREAMON. > > Patch 2/5 was also discovered by v4l2-compliance: the request fd in > v4l2_buffer should be ignored by VIDIOC_PREPARE_BUF, but it wasn't. > > Patch 3/5 fixes a nasty corner case: a buffer with associated request > is queued, and then the request fd is closed by v4l2-compliance. > > When the driver calls vb2_buffer_done, the buffer request object is > unbound, the object is put, and indirectly the associated request > is put as well, and since that was the last references to the request > the whole request is released, which requires the ability to call > mutex_lock. But vb2_buffer_done is atomic (it can be called > from interrupt context), so this shouldn't happen. > > vb2 now takes an extra refcount to the request on qbuf and releases > it on dqbuf and at two other places where an internal dqbuf is done. > > Note that 'skip request checks for VIDIOC_PREPARE_BUF' is a duplicate > of https://patchwork.linuxtv.org/patch/53171/, which is now marked as > superseded. > > I've marked all these patches for 4.20, but I think it is also possible > to apply them for 4.21 since the request API is only used by virtual > drivers and a staging driver. For patches 2--5: Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH for v4.20 1/5] vb2: don't call __vb2_queue_cancel if vb2_start_streaming failed
On Wed, Nov 28, 2018 at 09:37:43AM +0100, hverkuil-ci...@xs4all.nl wrote:
> From: Hans Verkuil
>
> vb2_start_streaming() already rolls back the buffers, so there is no
> need to call __vb2_queue_cancel(). Especially since __vb2_queue_cancel()
> does too much, such as zeroing the q->queued_count value, causing vb2
> to think that no buffers have been queued.
>
> It appears that this call to __vb2_queue_cancel() is a left-over from
> before commit b3379c6201bb3.
>
> Signed-off-by: Hans Verkuil
> Fixes: b3379c6201bb3 ('vb2: only call start_streaming if sufficient buffers
> are queued')
> Cc: # for v4.16 and up
Acked-by: Sakari Ailus
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index 0ca81d495bda..77e2bfe5e722 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1941,10 +1941,8 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned
> int type)
> if (ret)
> return ret;
> ret = vb2_start_streaming(q);
> - if (ret) {
> - __vb2_queue_cancel(q);
> + if (ret)
> return ret;
> - }
> }
>
> q->streaming = 1;
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v3 6/6] media: mt9m111: allow to setup pixclk polarity
On Wed, Nov 28, 2018 at 09:29:01AM +0100, Marco Felsch wrote: > On 18-11-27 23:15, Sakari Ailus wrote: > > On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote: > > > From: Enrico Scholz > > > > > > The chip can be configured to output data transitions on the > > > rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the > > > falling edge. > > > > > > Parsing the fw-node is made in a subfunction to bundle all (future) > > > dt-parsing / fw-parsing stuff. > > > > > > Signed-off-by: Enrico Scholz > > > (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set > > > per default. Set bit to 0 (enable mask bit without value) to enable > > > falling edge sampling.) > > > Signed-off-by: Michael Grzeschik > > > (m.fel...@pengutronix.de: use fwnode helpers) > > > (m.fel...@pengutronix.de: mv fw parsing into own function) > > > (m.fel...@pengutronix.de: adapt commit msg) > > > Signed-off-by: Marco Felsch > > > > This one as well: > > Sorry for that, I forget to adapt the Kconfig to often. Thanks for your > fix. No worries. I hope we'll have automated compile testing in not too distant future... -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v3 6/6] media: mt9m111: allow to setup pixclk polarity
On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote: > From: Enrico Scholz > > The chip can be configured to output data transitions on the > rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the > falling edge. > > Parsing the fw-node is made in a subfunction to bundle all (future) > dt-parsing / fw-parsing stuff. > > Signed-off-by: Enrico Scholz > (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set > per default. Set bit to 0 (enable mask bit without value) to enable > falling edge sampling.) > Signed-off-by: Michael Grzeschik > (m.fel...@pengutronix.de: use fwnode helpers) > (m.fel...@pengutronix.de: mv fw parsing into own function) > (m.fel...@pengutronix.de: adapt commit msg) > Signed-off-by: Marco Felsch This one as well: diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 005fc2bd0d05..902c3cabf44c 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -859,6 +859,7 @@ config VIDEO_MT9M032 config VIDEO_MT9M111 tristate "mt9m111, mt9m112 and mt9m131 support" depends on I2C && VIDEO_V4L2 + select V4L2_FWNODE help This driver supports MT9M111, MT9M112 and MT9M131 cameras from Micron/Aptina -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v3 6/6] media: mt9m111: allow to setup pixclk polarity
On Tue, Nov 27, 2018 at 03:12:29PM +0100, Philipp Zabel wrote:
> Hi Sakari,
>
> On Tue, 2018-11-27 at 15:50 +0200, Sakari Ailus wrote:
> > Hi Philipp,
> >
> > On Tue, Nov 27, 2018 at 02:39:27PM +0100, Philipp Zabel wrote:
> > > Hi Sakari,
> > >
> > > On Tue, 2018-11-27 at 15:19 +0200, Sakari Ailus wrote:
> > > > Hi Marco,
> > > >
> > > > On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote:
> > > > > From: Enrico Scholz
> > > > >
> > > > > The chip can be configured to output data transitions on the
> > > > > rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on
> > > > > the
> > > > > falling edge.
> > > > >
> > > > > Parsing the fw-node is made in a subfunction to bundle all (future)
> > > > > dt-parsing / fw-parsing stuff.
> > > > >
> > > > > Signed-off-by: Enrico Scholz
> > > > > (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit
> > > > > is set
> > > > > per default. Set bit to 0 (enable mask bit without value) to enable
> > > > > falling edge sampling.)
> > > > > Signed-off-by: Michael Grzeschik
> > > > > (m.fel...@pengutronix.de: use fwnode helpers)
> > > > > (m.fel...@pengutronix.de: mv fw parsing into own function)
> > > > > (m.fel...@pengutronix.de: adapt commit msg)
> > > > > Signed-off-by: Marco Felsch
> > > >
> > > > Applied with the following diff:
> > > >
> > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > > index 2ef332b9b914..b6011bfddde8 100644
> > > > --- a/drivers/media/i2c/mt9m111.c
> > > > +++ b/drivers/media/i2c/mt9m111.c
> > > > @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct
> > > > i2c_client *client)
> > > >
> > > > static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111
> > > > *mt9m111)
> > > > {
> > > > - struct v4l2_fwnode_endpoint *bus_cfg;
> > > > + struct v4l2_fwnode_endpoint bus_cfg = {
> > > > + .bus_type = V4L2_MBUS_PARALLEL
> > > > + };
> > > > struct fwnode_handle *np;
> > > > - int ret = 0;
> > > > + int ret;
> > > >
> > > > np = fwnode_graph_get_next_endpoint(dev_fwnode(>dev),
> > > > NULL);
> > > > if (!np)
> > > > return -EINVAL;
> > > >
> > > > - bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> > > > - if (IS_ERR(bus_cfg)) {
> > > > - ret = PTR_ERR(bus_cfg);
> > > > + ret = v4l2_fwnode_endpoint_alloc_parse(np, _cfg);
> > >
> > > Should that be
> > >
> > > + ret = v4l2_fwnode_endpoint_parse(np, _cfg);
> > >
> > > intead?
> >
> > Could be. I'd expect the driver to need the link frequency at some point
> > after which you'd need the variable size properties anyway. But that's not
> > the case now.
>
> I don't think the link-frequencies property will be used, this is just a
> parallel device. But Marco chose to use _alloc_parse because of what the
> v4l2_fwnode_endpoint_parse() documentation says:
>
> /*
> * NOTE: This function does not parse properties the size of which is variable
> * without a low fixed limit. Please use v4l2_fwnode_endpoint_alloc_parse()
> in
> * new drivers instead.
> */
>
> So maybe we want to use v4l2_fwnode_endpoint_alloc_parse() always. There
> is no unnecessary allocation, just a lookup of the non-existing link-
> frequencies property.
There could be other properties in the future.
When I wrote that, I guess I ignored that the link frequency might not be
relevant for some devices such as CSI-2 receivers. I think it'd make sense
to remove the latter sentence; I can send a patch. The first sentence that
tells the limitations of the function is enough IMO.
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v3 5/6] dt-bindings: media: mt9m111: add pclk-sample property
On Tue, Nov 27, 2018 at 03:21:05PM +0100, Marco Felsch wrote:
> Hi Sakari,
>
> On 18-11-27 15:13, Sakari Ailus wrote:
> > Hi Marco,
> >
> > On Tue, Nov 27, 2018 at 11:02:52AM +0100, Marco Felsch wrote:
> > > Add the pclk-sample property to the list of optional properties
> > > for the mt9m111 camera sensor.
> > >
> > > Signed-off-by: Marco Felsch
> > > Reviewed-by: Rob Herring
> > > ---
> > > Documentation/devicetree/bindings/media/i2c/mt9m111.txt | 5 +
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > > b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > > index a431fb45704b..d0bed6fa901a 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > > +++ b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > > @@ -14,6 +14,10 @@ sub-node for its digital output video port, in
> > > accordance with the video
> > > interface bindings defined in:
> > > Documentation/devicetree/bindings/media/video-interfaces.txt
> > >
> > > +Optional endpoint properties:
> > > +- pclk-sample: For information see ../video-interfaces.txt. The value is
> > > set to
> > > + 0 if it isn't specified.
> >
> > How about the data-active, hsync-active and vsync-active properties? Does
> > the hardware have a fixed configuration, or can this be set? It appears the
> > driver assumes active high for all.
>
> As I understood it correctly the data-active, hsync-active and vsync-active
> I/O lines are always ACTIVE_HIGH.
Ack. Thanks!
>
> Kind regards,
> Marco
>
> > If there's something to change, this should be a separate patch IMO.
> >
> > > +
> > > Example:
> > >
> > > i2c_master {
> > > @@ -26,6 +30,7 @@ Example:
> > > port {
> > > mt9m111_1: endpoint {
> > > remote-endpoint = <_camera>;
> > > + pclk-sample = <1>;
> > > };
> > > };
> > > };
> >
> > --
> > Sakari Ailus
> > sakari.ai...@linux.intel.com
> >
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v3 6/6] media: mt9m111: allow to setup pixclk polarity
Hi Philipp,
On Tue, Nov 27, 2018 at 02:39:27PM +0100, Philipp Zabel wrote:
> Hi Sakari,
>
> On Tue, 2018-11-27 at 15:19 +0200, Sakari Ailus wrote:
> > Hi Marco,
> >
> > On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote:
> > > From: Enrico Scholz
> > >
> > > The chip can be configured to output data transitions on the
> > > rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> > > falling edge.
> > >
> > > Parsing the fw-node is made in a subfunction to bundle all (future)
> > > dt-parsing / fw-parsing stuff.
> > >
> > > Signed-off-by: Enrico Scholz
> > > (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> > > per default. Set bit to 0 (enable mask bit without value) to enable
> > > falling edge sampling.)
> > > Signed-off-by: Michael Grzeschik
> > > (m.fel...@pengutronix.de: use fwnode helpers)
> > > (m.fel...@pengutronix.de: mv fw parsing into own function)
> > > (m.fel...@pengutronix.de: adapt commit msg)
> > > Signed-off-by: Marco Felsch
> >
> > Applied with the following diff:
> >
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index 2ef332b9b914..b6011bfddde8 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client
> > *client)
> >
> > static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111
> > *mt9m111)
> > {
> > - struct v4l2_fwnode_endpoint *bus_cfg;
> > + struct v4l2_fwnode_endpoint bus_cfg = {
> > + .bus_type = V4L2_MBUS_PARALLEL
> > + };
> > struct fwnode_handle *np;
> > - int ret = 0;
> > + int ret;
> >
> > np = fwnode_graph_get_next_endpoint(dev_fwnode(>dev), NULL);
> > if (!np)
> > return -EINVAL;
> >
> > - bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> > - if (IS_ERR(bus_cfg)) {
> > - ret = PTR_ERR(bus_cfg);
> > + ret = v4l2_fwnode_endpoint_alloc_parse(np, _cfg);
>
> Should that be
>
> + ret = v4l2_fwnode_endpoint_parse(np, _cfg);
>
> intead?
Could be. I'd expect the driver to need the link frequency at some point
after which you'd need the variable size properties anyway. But that's not
the case now.
With the change, the v4l2_fwnode_endpoint_free() becomes redundant. So the
diff on that diff:
diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index b6011bfddde8..d639b9bcf64a 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -1182,15 +1182,13 @@ static int mt9m111_probe_fw(struct i2c_client *client,
struct mt9m111 *mt9m111)
if (!np)
return -EINVAL;
- ret = v4l2_fwnode_endpoint_alloc_parse(np, _cfg);
+ ret = v4l2_fwnode_endpoint_parse(np, _cfg);
if (ret)
goto out_put_fw;
mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags &
V4L2_MBUS_PCLK_SAMPLE_RISING);
- v4l2_fwnode_endpoint_free(_cfg);
-
out_put_fw:
fwnode_handle_put(np);
return ret;
>
> > + if (ret)
> > goto out_put_fw;
> > - }
> >
> > - mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> > + mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags &
> > V4L2_MBUS_PCLK_SAMPLE_RISING);
> >
> > - v4l2_fwnode_endpoint_free(bus_cfg);
> > + v4l2_fwnode_endpoint_free(_cfg);
> >
> > out_put_fw:
> > fwnode_handle_put(np);
> >
> > Please base on current media tree master on the next time. Thanks.
>
> regards
> Philipp
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v4 2/2] dt-bindings: Document the Rockchip RK1608 bindings
remote-endpoint = <_dphy_in0>;
> + clock-lanes = <0>;
> + data-lanes = <1 2 3 4>;
> + clock-noncontinuous;
> + link-frequencies =
> + /bits/ 64 ;
> + };
> + };
> +
> + port@1 {
> + sensor_in0: endpoint@0 {
reg missing here and below.
> + remote-endpoint = <_out0>;
> + };
> + /* If you have a second sensor,
> + * add the 'endpoint@1' node here.
> + */
> + sensor_in1: endpoint@1 {
> + remote-endpoint = <_out1>;
> + };
> + };
> + };
> + };
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b2a98e3..04d227b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -141,6 +141,7 @@ M:Leo Wen
> S: Maintained
> F: drivers/media/spi/rk1608.c
> F: drivers/media/spi/rk1608.h
> +F: Documentation/devicetree/bindings/media/rk1608.txt
>
> 3C59X NETWORK DRIVER
> M: Steffen Klassert
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v3 6/6] media: mt9m111: allow to setup pixclk polarity
Hi Marco,
On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote:
> From: Enrico Scholz
>
> The chip can be configured to output data transitions on the
> rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> falling edge.
>
> Parsing the fw-node is made in a subfunction to bundle all (future)
> dt-parsing / fw-parsing stuff.
>
> Signed-off-by: Enrico Scholz
> (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> per default. Set bit to 0 (enable mask bit without value) to enable
> falling edge sampling.)
> Signed-off-by: Michael Grzeschik
> (m.fel...@pengutronix.de: use fwnode helpers)
> (m.fel...@pengutronix.de: mv fw parsing into own function)
> (m.fel...@pengutronix.de: adapt commit msg)
> Signed-off-by: Marco Felsch
Applied with the following diff:
diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 2ef332b9b914..b6011bfddde8 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client
*client)
static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111)
{
- struct v4l2_fwnode_endpoint *bus_cfg;
+ struct v4l2_fwnode_endpoint bus_cfg = {
+ .bus_type = V4L2_MBUS_PARALLEL
+ };
struct fwnode_handle *np;
- int ret = 0;
+ int ret;
np = fwnode_graph_get_next_endpoint(dev_fwnode(>dev), NULL);
if (!np)
return -EINVAL;
- bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
- if (IS_ERR(bus_cfg)) {
- ret = PTR_ERR(bus_cfg);
+ ret = v4l2_fwnode_endpoint_alloc_parse(np, _cfg);
+ if (ret)
goto out_put_fw;
- }
- mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
+ mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags &
V4L2_MBUS_PCLK_SAMPLE_RISING);
- v4l2_fwnode_endpoint_free(bus_cfg);
+ v4l2_fwnode_endpoint_free(_cfg);
out_put_fw:
fwnode_handle_put(np);
Please base on current media tree master on the next time. Thanks.
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v3 5/6] dt-bindings: media: mt9m111: add pclk-sample property
Hi Marco,
On Tue, Nov 27, 2018 at 11:02:52AM +0100, Marco Felsch wrote:
> Add the pclk-sample property to the list of optional properties
> for the mt9m111 camera sensor.
>
> Signed-off-by: Marco Felsch
> Reviewed-by: Rob Herring
> ---
> Documentation/devicetree/bindings/media/i2c/mt9m111.txt | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> index a431fb45704b..d0bed6fa901a 100644
> --- a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> @@ -14,6 +14,10 @@ sub-node for its digital output video port, in accordance
> with the video
> interface bindings defined in:
> Documentation/devicetree/bindings/media/video-interfaces.txt
>
> +Optional endpoint properties:
> +- pclk-sample: For information see ../video-interfaces.txt. The value is set
> to
> + 0 if it isn't specified.
How about the data-active, hsync-active and vsync-active properties? Does
the hardware have a fixed configuration, or can this be set? It appears the
driver assumes active high for all.
If there's something to change, this should be a separate patch IMO.
> +
> Example:
>
> i2c_master {
> @@ -26,6 +30,7 @@ Example:
> port {
> mt9m111_1: endpoint {
> remote-endpoint = <_camera>;
> + pclk-sample = <1>;
> };
> };
> };
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [RESEND PATCH 1/1] media: Use common test pattern menu entries
On Tue, Nov 27, 2018 at 01:11:42PM +0100, Luca Ceresoli wrote:
> Hi Sakari, Bingbu,
>
> On 27/11/18 10:34, Sakari Ailus wrote:
> > While the test pattern menu itself is not standardised, many devices
> > support the same test patterns. Aligning the menu entries helps the user
> > space to use the interface, and adding macros for the menu entry strings
> > helps to keep them aligned.
> >
> > Signed-off-by: Sakari Ailus
> > ---
> > Fixed Andy's email.
> >
> > drivers/media/i2c/imx258.c | 10 +-
> > drivers/media/i2c/imx319.c | 10 +-
> > drivers/media/i2c/imx355.c | 10 +-
> > drivers/media/i2c/ov2640.c | 4 ++--
> > drivers/media/i2c/smiapp/smiapp-core.c | 10 +-
> > include/uapi/linux/v4l2-controls.h | 5 +
> > 6 files changed, 27 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index f86ae18bc104..c795d4c4c0e4 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > @@ -498,11 +498,11 @@ static const struct imx258_reg mode_1048_780_regs[] =
> > {
> > };
> >
> > static const char * const imx258_test_pattern_menu[] = {
> > - "Disabled",
> > - "Solid Colour",
> > - "Eight Vertical Colour Bars",
> > - "Colour Bars With Fade to Grey",
> > - "Pseudorandom Sequence (PN9)",
> > + V4L2_TEST_PATTERN_DISABLED,
> > + V4L2_TEST_PATTERN_SOLID_COLOUR,
> > + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> > + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> > + V4L2_TEST_PATTERN_PN9,
> > };
> >
> > /* Configurations for supported link frequencies */
> > diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> > index 17c2e4b41221..eddaf69a67b6 100644
> > --- a/drivers/media/i2c/imx319.c
> > +++ b/drivers/media/i2c/imx319.c
> > @@ -1647,11 +1647,11 @@ static const struct imx319_reg mode_1280x720_regs[]
> > = {
> > };
> >
> > static const char * const imx319_test_pattern_menu[] = {
> > - "Disabled",
> > - "Solid Colour",
> > - "Eight Vertical Colour Bars",
> > - "Colour Bars With Fade to Grey",
> > - "Pseudorandom Sequence (PN9)",
> > + V4L2_TEST_PATTERN_DISABLED,
> > + V4L2_TEST_PATTERN_SOLID_COLOUR,
> > + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> > + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> > + V4L2_TEST_PATTERN_PN9,
> > };
> >
> > /* supported link frequencies */
> > diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> > index bed293b60e50..824d07156f9c 100644
> > --- a/drivers/media/i2c/imx355.c
> > +++ b/drivers/media/i2c/imx355.c
> > @@ -875,11 +875,11 @@ static const struct imx355_reg mode_820x616_regs[] = {
> > };
> >
> > static const char * const imx355_test_pattern_menu[] = {
> > - "Disabled",
> > - "Solid Colour",
> > - "Eight Vertical Colour Bars",
> > - "Colour Bars With Fade to Grey",
> > - "Pseudorandom Sequence (PN9)",
> > + V4L2_TEST_PATTERN_DISABLED,
> > + V4L2_TEST_PATTERN_SOLID_COLOUR,
> > + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> > + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> > + V4L2_TEST_PATTERN_PN9,
> > };
> >
> > /* supported link frequencies */
> > diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> > index 5d2d6735cc78..507ec7176a7d 100644
> > --- a/drivers/media/i2c/ov2640.c
> > +++ b/drivers/media/i2c/ov2640.c
> > @@ -707,8 +707,8 @@ static int ov2640_reset(struct i2c_client *client)
> > }
> >
> > static const char * const ov2640_test_pattern_menu[] = {
> > - "Disabled",
> > - "Eight Vertical Colour Bars",
> > + V4L2_TEST_PATTERN_DISABLED,
> > + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> > };
> >
> > /*
> > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> > b/drivers/media/i2c/smiapp/smiapp-core.c
> > index 58a45c353e27..f6a92b9f178c 100644
> > --- a/drivers/media/i2c/smiapp/smiapp-core.c
> > +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> > @@ -409,11 +409,11 @@ static void smiapp_update_mbus_formats(struct
> > smiapp_sensor *sensor)
> > }
> >
> > static const char * const smiapp_test_patterns[] = {
> > - &q
[RESEND PATCH 1/1] media: Use common test pattern menu entries
While the test pattern menu itself is not standardised, many devices
support the same test patterns. Aligning the menu entries helps the user
space to use the interface, and adding macros for the menu entry strings
helps to keep them aligned.
Signed-off-by: Sakari Ailus
---
Fixed Andy's email.
drivers/media/i2c/imx258.c | 10 +-
drivers/media/i2c/imx319.c | 10 +-
drivers/media/i2c/imx355.c | 10 +-
drivers/media/i2c/ov2640.c | 4 ++--
drivers/media/i2c/smiapp/smiapp-core.c | 10 +-
include/uapi/linux/v4l2-controls.h | 5 +
6 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index f86ae18bc104..c795d4c4c0e4 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -498,11 +498,11 @@ static const struct imx258_reg mode_1048_780_regs[] = {
};
static const char * const imx258_test_pattern_menu[] = {
- "Disabled",
- "Solid Colour",
- "Eight Vertical Colour Bars",
- "Colour Bars With Fade to Grey",
- "Pseudorandom Sequence (PN9)",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_SOLID_COLOUR,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+ V4L2_TEST_PATTERN_PN9,
};
/* Configurations for supported link frequencies */
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 17c2e4b41221..eddaf69a67b6 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -1647,11 +1647,11 @@ static const struct imx319_reg mode_1280x720_regs[] = {
};
static const char * const imx319_test_pattern_menu[] = {
- "Disabled",
- "Solid Colour",
- "Eight Vertical Colour Bars",
- "Colour Bars With Fade to Grey",
- "Pseudorandom Sequence (PN9)",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_SOLID_COLOUR,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+ V4L2_TEST_PATTERN_PN9,
};
/* supported link frequencies */
diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index bed293b60e50..824d07156f9c 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -875,11 +875,11 @@ static const struct imx355_reg mode_820x616_regs[] = {
};
static const char * const imx355_test_pattern_menu[] = {
- "Disabled",
- "Solid Colour",
- "Eight Vertical Colour Bars",
- "Colour Bars With Fade to Grey",
- "Pseudorandom Sequence (PN9)",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_SOLID_COLOUR,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+ V4L2_TEST_PATTERN_PN9,
};
/* supported link frequencies */
diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index 5d2d6735cc78..507ec7176a7d 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -707,8 +707,8 @@ static int ov2640_reset(struct i2c_client *client)
}
static const char * const ov2640_test_pattern_menu[] = {
- "Disabled",
- "Eight Vertical Colour Bars",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
};
/*
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
b/drivers/media/i2c/smiapp/smiapp-core.c
index 58a45c353e27..f6a92b9f178c 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -409,11 +409,11 @@ static void smiapp_update_mbus_formats(struct
smiapp_sensor *sensor)
}
static const char * const smiapp_test_patterns[] = {
- "Disabled",
- "Solid Colour",
- "Eight Vertical Colour Bars",
- "Colour Bars With Fade to Grey",
- "Pseudorandom Sequence (PN9)",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_SOLID_COLOUR,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+ V4L2_TEST_PATTERN_PN9,
};
static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
diff --git a/include/uapi/linux/v4l2-controls.h
b/include/uapi/linux/v4l2-controls.h
index 998983a6e6b7..a74ff6f1ac88 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1014,6 +1014,11 @@ enum v4l2_jpeg_chroma_subsampling {
#define V4L2_CID_LINK_FREQ (V4L2_CID_IMAGE_PROC_CLASS_BASE
+ 1)
#define V4L2_CID_PIXEL_RATE(V4L2_CID_IMAGE_PROC_CLASS_BASE
+ 2)
#define V4L2_CID_TEST_PATTERN (V4L2_CID_IMAGE_PROC_CLASS_BASE
+ 3)
+#define V4L2_TEST_PATTERN_DISABLED "Disabled"
+#define V4L2_TEST_
Re: [PATCH v2] media: unify some sony camera sensors pattern naming
On Tue, Nov 27, 2018 at 05:03:26PM +0800, bingbu@intel.com wrote: > From: Bingbu Cao > > Some Sony camera sensors have same pattern > definitions, this patch unify the pattern naming > to make it more clear to the userspace. > > Suggested-by: Sakari Ailus > Signed-off-by: Bingbu Cao > Reviewed-by: l...@lucaceresoli.net Hi Bing Bu, Sorry, I prefer your v1. :-) -- Sakari Ailus sakari.ai...@linux.intel.com
[PATCH 1/1] media: Use common test pattern menu entries
While the test pattern menu itself is not standardised, many devices
support the same test patterns. Aligning the menu entries helps the user
space to use the interface, and adding macros for the menu entry strings
helps to keep them aligned.
Signed-off-by: Sakari Ailus
---
drivers/media/i2c/imx258.c | 10 +-
drivers/media/i2c/imx319.c | 10 +-
drivers/media/i2c/imx355.c | 10 +-
drivers/media/i2c/ov2640.c | 4 ++--
drivers/media/i2c/smiapp/smiapp-core.c | 10 +-
include/uapi/linux/v4l2-controls.h | 5 +
6 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index f86ae18bc104..c795d4c4c0e4 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -498,11 +498,11 @@ static const struct imx258_reg mode_1048_780_regs[] = {
};
static const char * const imx258_test_pattern_menu[] = {
- "Disabled",
- "Solid Colour",
- "Eight Vertical Colour Bars",
- "Colour Bars With Fade to Grey",
- "Pseudorandom Sequence (PN9)",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_SOLID_COLOUR,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+ V4L2_TEST_PATTERN_PN9,
};
/* Configurations for supported link frequencies */
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 17c2e4b41221..eddaf69a67b6 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -1647,11 +1647,11 @@ static const struct imx319_reg mode_1280x720_regs[] = {
};
static const char * const imx319_test_pattern_menu[] = {
- "Disabled",
- "Solid Colour",
- "Eight Vertical Colour Bars",
- "Colour Bars With Fade to Grey",
- "Pseudorandom Sequence (PN9)",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_SOLID_COLOUR,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+ V4L2_TEST_PATTERN_PN9,
};
/* supported link frequencies */
diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index bed293b60e50..824d07156f9c 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -875,11 +875,11 @@ static const struct imx355_reg mode_820x616_regs[] = {
};
static const char * const imx355_test_pattern_menu[] = {
- "Disabled",
- "Solid Colour",
- "Eight Vertical Colour Bars",
- "Colour Bars With Fade to Grey",
- "Pseudorandom Sequence (PN9)",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_SOLID_COLOUR,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+ V4L2_TEST_PATTERN_PN9,
};
/* supported link frequencies */
diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index 5d2d6735cc78..507ec7176a7d 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -707,8 +707,8 @@ static int ov2640_reset(struct i2c_client *client)
}
static const char * const ov2640_test_pattern_menu[] = {
- "Disabled",
- "Eight Vertical Colour Bars",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
};
/*
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
b/drivers/media/i2c/smiapp/smiapp-core.c
index 58a45c353e27..f6a92b9f178c 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -409,11 +409,11 @@ static void smiapp_update_mbus_formats(struct
smiapp_sensor *sensor)
}
static const char * const smiapp_test_patterns[] = {
- "Disabled",
- "Solid Colour",
- "Eight Vertical Colour Bars",
- "Colour Bars With Fade to Grey",
- "Pseudorandom Sequence (PN9)",
+ V4L2_TEST_PATTERN_DISABLED,
+ V4L2_TEST_PATTERN_SOLID_COLOUR,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+ V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+ V4L2_TEST_PATTERN_PN9,
};
static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
diff --git a/include/uapi/linux/v4l2-controls.h
b/include/uapi/linux/v4l2-controls.h
index 998983a6e6b7..a74ff6f1ac88 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1014,6 +1014,11 @@ enum v4l2_jpeg_chroma_subsampling {
#define V4L2_CID_LINK_FREQ (V4L2_CID_IMAGE_PROC_CLASS_BASE
+ 1)
#define V4L2_CID_PIXEL_RATE(V4L2_CID_IMAGE_PROC_CLASS_BASE
+ 2)
#define V4L2_CID_TEST_PATTERN (V4L2_CID_IMAGE_PROC_CLASS_BASE
+ 3)
+#define V4L2_TEST_PATTERN_DISABLED "Disabled"
+#define V4L2_TEST_PATTERN_SOLID_COLOUR
Re: [PATCH 2/2] media: imx355: fix wrong order in test pattern menus
On Tue, Nov 27, 2018 at 10:45:02AM +0800, Bingbu Cao wrote:
>
>
> On 11/26/2018 04:57 PM, Sakari Ailus wrote:
> > Hi Bing Bu,
> >
> > On Mon, Nov 26, 2018 at 03:43:34PM +0800, bingbu@intel.com wrote:
> > > From: Bingbu Cao
> > >
> > > current imx355 test pattern order in ctrl menu
> > > is not correct, this patch fixes it.
> > >
> > > Signed-off-by: Bingbu Cao
> > > ---
> > > drivers/media/i2c/imx355.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> > > index 20c8eea5db4b..9c9559dfd3dd 100644
> > > --- a/drivers/media/i2c/imx355.c
> > > +++ b/drivers/media/i2c/imx355.c
> > > @@ -876,8 +876,8 @@ struct imx355 {
> > > static const char * const imx355_test_pattern_menu[] = {
> > > "Disabled",
> > > - "100% color bars",
> > > "Solid color",
> > > + "100% color bars",
> > > "Fade to gray color bars",
> > > "PN9"
> > > };
> > While at it, could you use the existing test pattern naming as well for the
> > drivers? That could be a separate patch.
> >
> > >From drivers/media/i2c/smiapp/smiapp-core.c :
> >
> > static const char * const smiapp_test_patterns[] = {
> > "Disabled",
> > "Solid Colour",
> > "Eight Vertical Colour Bars",
> > "Colour Bars With Fade to Grey",
> > "Pseudorandom Sequence (PN9)",
> > };
> >
> > It's not strictly necessary from interface point of view, but for the user
> > space it'd be good to align the naming.
> Sakari, ask a question that not really related to this patch.
> I am wondering whether there are some standardization for the camera sensor
> pattern generator.
> Currently there are varied patterns and every vendor has its own pattern
> types.
Some vendors (and some models) do have their specific patterns, but if you
look at a bunch of drivers, these ones repeat over and over. That's why I
think it'd make sense to have the menu entries aligned.
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH] media: unify some sony camera sensors pattern naming
Hi Luca,
On Tue, Nov 27, 2018 at 09:05:34AM +0100, Luca Ceresoli wrote:
> Hi Bingbu,
>
> On 27/11/18 05:01, bingbu@intel.com wrote:
> > From: Bingbu Cao
> >
> > Some Sony camera sensors have same test pattern
> > definitions, this patch unify the pattern naming
> > to make it more clear to the userspace.
> >
> > Suggested-by: Sakari Ailus
> > Signed-off-by: Bingbu Cao
> > ---
> > drivers/media/i2c/imx258.c | 8
> > drivers/media/i2c/imx319.c | 8
> > drivers/media/i2c/imx355.c | 8
> > 3 files changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index 31a1e2294843..a8a2880c6b4e 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > @@ -504,10 +504,10 @@ struct imx258_mode {
> >
> > static const char * const imx258_test_pattern_menu[] = {
> > "Disabled",
> > - "Color Bars",
> > - "Solid Color",
> > - "Grey Color Bars",
> > - "PN9"
> > + "Solid Colour",
> > + "Eight Vertical Colour Bars",
> > + "Colour Bars With Fade to Grey",
> > + "Pseudorandom Sequence (PN9)",
>
> I had a look at imx274, it has many more values but definitely some
> could be unified too.
>
> However I noticed something strange in that driver: The "Horizontal
> Color Bars" pattern has vertical bars, side-by-side, as in .
> "Vertical Color Bars" are one on top of the other, as in ==. Is it just
> me crazy, or are they swapped?
>
> Only one minor nitpick about your patch. The USA spelling "color" seems
> a lot more frequent in the kernel sources than the UK "colour", so it's
> probably better to be consistent.
This has been around for some seven or so years in the smiapp driver, and
changing strings in uAPI isn't something we prefer to do in general.
I wonder what others think.
If that's changed, it should be a separate patch.
--
Sakari Ailus
sakari.ai...@linux.intel.com
[PATCH 1/1] v4l: ioctl: Allow drivers to fill in the format description
The v4l_fill_fmtdesc() function does a good job in filling in pixelformat description. While generally all drivers should depend on this function doing the job, staging drivers that use their own formats may not. Allow staging drivers to fill in their own formats by checking whether the description begins with a non-nil character before issuing the warning. Signed-off-by: Sakari Ailus Suggested-by: Hans Verkuil --- drivers/media/v4l2-core/v4l2-ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 402b6505502a..7473883ab509 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -1345,9 +1345,9 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) case V4L2_PIX_FMT_MT21C:descr = "Mediatek Compressed Format"; break; case V4L2_PIX_FMT_SUNXI_TILED_NV12: descr = "Sunxi Tiled NV12 Format"; break; default: - WARN(1, "Unknown pixelformat 0x%08x\n", fmt->pixelformat); if (fmt->description[0]) return; + WARN(1, "Unknown pixelformat 0x%08x\n", fmt->pixelformat); flags = 0; snprintf(fmt->description, sz, "%c%c%c%c%s", (char)(fmt->pixelformat & 0x7f), -- 2.11.0
Re: [PATCH 2/2] media: imx355: fix wrong order in test pattern menus
Hi Bing Bu,
On Mon, Nov 26, 2018 at 03:43:34PM +0800, bingbu@intel.com wrote:
> From: Bingbu Cao
>
> current imx355 test pattern order in ctrl menu
> is not correct, this patch fixes it.
>
> Signed-off-by: Bingbu Cao
> ---
> drivers/media/i2c/imx355.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index 20c8eea5db4b..9c9559dfd3dd 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
> @@ -876,8 +876,8 @@ struct imx355 {
>
> static const char * const imx355_test_pattern_menu[] = {
> "Disabled",
> - "100% color bars",
> "Solid color",
> + "100% color bars",
> "Fade to gray color bars",
> "PN9"
> };
While at it, could you use the existing test pattern naming as well for the
drivers? That could be a separate patch.
>From drivers/media/i2c/smiapp/smiapp-core.c :
static const char * const smiapp_test_patterns[] = {
"Disabled",
"Solid Colour",
"Eight Vertical Colour Bars",
"Colour Bars With Fade to Grey",
"Pseudorandom Sequence (PN9)",
};
It's not strictly necessary from interface point of view, but for the user
space it'd be good to align the naming.
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v3] media: video-i2c: check if chip struct has set_power function
Hi Matt,
On Sat, Nov 24, 2018 at 02:03:23PM -0800, Matt Ranostay wrote:
> Not all future supported video chips will always have power management
> support, and so it is important to check before calling set_power() is
> defined.
>
> Cc: Sakari Ailus
> Cc: Hans Verkuil
> Cc: Mauro Carvalho Chehab
> Cc: Akinobu Mita
> Signed-off-by: Matt Ranostay
> ---
>
> Changes from v2:
> - split out from mlx90640 patch series
> - added to Cc list
>
> Changes from v1:
> - none
>
> drivers/media/i2c/video-i2c.c | 22 +-
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index b6ebb8d53e90..01dcf179f203 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -736,9 +736,11 @@ static int video_i2c_probe(struct i2c_client *client,
> video_set_drvdata(>vdev, data);
> i2c_set_clientdata(client, data);
>
> - ret = data->chip->set_power(data, true);
> - if (ret)
> - goto error_unregister_device;
> + if (data->chip->set_power) {
> + ret = data->chip->set_power(data, true);
> + if (ret)
> + goto error_unregister_device;
> + }
How about adding a macro to call the op if it's set? It could be used to
call other ops when they're set, and ignore them when they're not. Just an
idea. See e.g. v4l2_subdev_call() in include/media/v4l2-subdev.h .
>
> pm_runtime_get_noresume(>dev);
> pm_runtime_set_active(>dev);
> @@ -767,7 +769,9 @@ static int video_i2c_probe(struct i2c_client *client,
> pm_runtime_disable(>dev);
> pm_runtime_set_suspended(>dev);
> pm_runtime_put_noidle(>dev);
> - data->chip->set_power(data, false);
> +
> + if (data->chip->set_power)
> + data->chip->set_power(data, false);
>
> error_unregister_device:
> v4l2_device_unregister(v4l2_dev);
> @@ -791,7 +795,9 @@ static int video_i2c_remove(struct i2c_client *client)
> pm_runtime_disable(>dev);
> pm_runtime_set_suspended(>dev);
> pm_runtime_put_noidle(>dev);
> - data->chip->set_power(data, false);
> +
> + if (data->chip->set_power)
> + data->chip->set_power(data, false);
>
> video_unregister_device(>vdev);
>
> @@ -804,6 +810,9 @@ static int video_i2c_pm_runtime_suspend(struct device
> *dev)
> {
> struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
>
> + if (!data->chip->set_power)
> + return 0;
> +
> return data->chip->set_power(data, false);
> }
>
> @@ -811,6 +820,9 @@ static int video_i2c_pm_runtime_resume(struct device *dev)
> {
> struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
>
> + if (!data->chip->set_power)
> + return 0;
> +
> return data->chip->set_power(data, true);
> }
>
> --
> 2.17.1
>
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: 'bad remote port parent' warnings
Hi Fabio, Philipp,
On Thu, Nov 22, 2018 at 05:17:44PM -0200, Fabio Estevam wrote:
> Hi Philipp,
>
> On Thu, Nov 22, 2018 at 2:27 PM Philipp Zabel wrote:
>
> > There are empty endpoint nodes (without remote-endpoint property)
> > labeled ipu1_csi[01]_mux_from_parallel_sensor in the i.MX6 device trees
> > for board DT implementers' convenience. See commit 2539f517acbdc ("ARM:
> > dts: imx6qdl: Add video multiplexers, mipi_csi, and their connections").
> >
> > We had a discussion about this issue in February when this caused a
> > probing error: https://patchwork.kernel.org/patch/10234469/
>
> Thanks for the clarification.
>
> We could demote the warning to a debug message, make the wording a bit
> > less misleading (there is no bad remote port parent, there is just no
> > remote endpoint at all), or we could just accept the error message for
>
> Something like this?
>
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -613,7 +613,7 @@ v4l2_async_notifier_fwnode_parse_endpoint(struct
> device *dev,
> asd->match.fwnode =
> fwnode_graph_get_remote_port_parent(endpoint);
> if (!asd->match.fwnode) {
> - dev_warn(dev, "bad remote port parent\n");
> + dev_dbg(dev, "no remote endpoint found\n");
Makes sense. This is not necessarily a fatal error. Could you send a patch?
> ret = -ENOTCONN;
> goto out_err;
> }
>
> And how should we treat these error probes?
>
> [3.449564] imx-ipuv3 240.ipu: driver could not parse
> port@1/endpoint@0 (-22)
> [3.457342] imx-ipuv3-csi: probe of imx-ipuv3-csi.1 failed with error -22
> [3.464498] imx-ipuv3 280.ipu: driver could not parse
> port@0/endpoint@0 (-22)
> [3.472120] imx-ipuv3-csi: probe of imx-ipuv3-csi.4 failed with error -22
I'm not sure if this is a real problem, I presume it's just that the device
has nothing connected to it, and so cannot work. Steve probably has a
better understanding of this, I'm just guessing here. :-)
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
[yavta PATCH 1/1] Zero dev in main()
From: Sakari Ailus
This is necessary since video_open() may not be always called soon
Signed-off-by: Sakari Ailus
---
yavta.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/yavta.c b/yavta.c
index c7986bd..de5376d 100644
--- a/yavta.c
+++ b/yavta.c
@@ -342,7 +342,6 @@ static bool video_has_valid_buf_type(struct device *dev)
static void video_init(struct device *dev)
{
- memset(dev, 0, sizeof *dev);
dev->fd = -1;
dev->memtype = V4L2_MEMORY_MMAP;
dev->buffers = NULL;
@@ -1903,7 +1902,7 @@ static struct option opts[] = {
int main(int argc, char *argv[])
{
struct sched_param sched;
- struct device dev;
+ struct device dev = { 0 };
int ret;
/* Options parsings */
--
2.11.0
Re: is it possible to use single IOCTL to setup media pipeline?
Hi Tomasz, On Thu, Nov 22, 2018 at 04:06:36PM +0900, Tomasz Figa wrote: > Hi Ning, > > On Thu, Nov 22, 2018 at 11:52 AM Zhang, Ning A wrote: > > > > Hello everyone > > > > when we need to setup media pipeline, eg, for camera capture, media-ctl > > needs to be called multiple time to setup media link and subdev > > formats, or similar code in a single application. this will use > > multiple IOCTLs on "/dev/mediaX" and "/dev/v4l2-subdevY". > > > > to setup media pipeline in userspace requires to fully understanding > > the topology of the media stack. but the fact is only media driver > > developer could know how to setup media pipeline. each time driver > > updates, this would break userspace application if application > > engineers don't know this change. > > That's obviously a bug in the driver. Kernel interfaces must not > change in a way that are not compatible with the userspace. Alternatively, this could result from fixing a bug in a driver. Or adding features that were not previously supported. In this case there are often no perfect solutions to address all the issues at hand. > > > In this case, if a IOCTL is designed > > to setup media pipeline, no need to update applications, after driver > > is updated. > > > > this will not only benefit for design a single IOCTL, this also helps > > to hide the detail of media pipeline, by load a binary blob which holds > > information about how to setup pipeline, or hide it in bootloader/ACPI > > tables/device tree, etc. > > Media pipeline configuration is specific to the use case. If you > hardcode it in the driver or bootloader, the user will not be able to > use any other use case than the hardcoded blob, which is unacceptable > for Linux drivers. > > Instead, it sounds like your userspace should be designed in a way > that the media topology configuration is loaded from a configuration > file that you could either get from your kernel driver developer or > just maintain yourself based on any changes the media developers do. > Of course that's unrelated to the backwards compatibility issue, which > should not happen normally. The configuration file would be helpful > for handling future extensions and new hardware platforms. > > > > > another benefit is save time for setup media pipeline, if there is a > > PKI like "time for open camera". as my test, this will saves hundreds > > of milliseconds. > > For this problem, the proper solution would be to create an ioctl that > can aggregate setting multiple parts of the topology in one go. For > example, V4L2 has VIDIOC_S_CTRL for setting a control, but there is > also VIDIOC_S_EXT_CTRLS, which lets you set multiple controls in one > call. Something like VIDIOC_S_EXT_CTRLS for configuring the media > topology would solve the performance problem. There have been ideas of moving all IOCTL handling to the media device, in a way that would allow issuing them in the same fashion than controls. This was discussed in last year's media summit actually. I think this could be done once the request API is otherwise working for e.g. camera devices. I don't expect this to materialise in near (or even medium) term though. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
[GIT PULL for 4.21] Sensor and CSI driver patches
Hi Mauro, Here are a bunch of sensor driver improvements as well as a driver for the Allwinner CSI parallel bridge. Finally, a small uAPI documentation fix to better document the metadata capture buffers as well as interaction between frame interval and format. Please pull. The following changes since commit 5200ab6a32d6055428896a49ec9e3b1652c1a100: media: vidioc_cropcap -> vidioc_g_pixelaspect (2018-11-20 13:57:21 -0500) are available in the git repository at: ssh://linuxtv.org/git/sailus/media_tree.git tags/for-4.21-3-sign for you to fetch changes up to a5dd7998bbee2275e19d64060d634e901d7533f5: media: ov2680: fix null dereference at power on (2018-11-22 13:35:51 +0200) sensor patches and stuff Akinobu Mita (7): media: mt9m111: support log_status ioctl and event interface media: mt9m111: add V4L2_CID_COLORFX control media: ov2640: add V4L2_CID_TEST_PATTERN control media: ov2640: support log_status ioctl and event interface media: ov5640: support log_status ioctl and event interface media: ov7670: support log_status ioctl and event interface media: ov772x: support log_status ioctl and event interface Chen, JasonX Z (1): media: imx258: remove test pattern map from driver Maxime Ripard (2): dt-bindings: media: sun6i: Add A31 and H3 compatibles media: sun6i: Add A31 compatible Nathan Chancellor (1): media: imx214: Remove unnecessary self assignment in for loop Rui Miguel Silva (1): media: ov2680: fix null dereference at power on Sakari Ailus (2): v4l: uAPI doc: Simplify NATIVE_SIZE selection target documentation v4l: uAPI doc: Changing frame interval won't change format Yong Deng (2): dt-bindings: media: Add Allwinner V3s Camera Sensor Interface (CSI) media: V3s: Add support for Allwinner CSI. .../devicetree/bindings/media/sun6i-csi.txt| 59 ++ .../media/uapi/v4l/v4l2-selection-targets.rst | 7 +- Documentation/media/uapi/v4l/vidioc-g-parm.rst | 3 + .../uapi/v4l/vidioc-subdev-g-frame-interval.rst| 3 + MAINTAINERS| 8 + drivers/media/i2c/imx214.c | 2 +- drivers/media/i2c/imx258.c | 22 +- drivers/media/i2c/mt9m111.c| 44 +- drivers/media/i2c/ov2640.c | 21 +- drivers/media/i2c/ov2680.c | 12 +- drivers/media/i2c/ov5640.c | 7 +- drivers/media/i2c/ov7670.c | 6 +- drivers/media/i2c/ov772x.c | 7 +- drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile| 2 + drivers/media/platform/sunxi/sun6i-csi/Kconfig | 9 + drivers/media/platform/sunxi/sun6i-csi/Makefile| 3 + drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c | 913 + drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h | 135 +++ .../media/platform/sunxi/sun6i-csi/sun6i_csi_reg.h | 196 + .../media/platform/sunxi/sun6i-csi/sun6i_video.c | 678 +++ .../media/platform/sunxi/sun6i-csi/sun6i_video.h | 38 + 22 files changed, 2133 insertions(+), 43 deletions(-) create mode 100644 Documentation/devicetree/bindings/media/sun6i-csi.txt create mode 100644 drivers/media/platform/sunxi/sun6i-csi/Kconfig create mode 100644 drivers/media/platform/sunxi/sun6i-csi/Makefile create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_reg.h create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_video.h -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v2 for v4.4 1/1] v4l: event: Add subscription to list before calling "add" operation
On Tue, Nov 20, 2018 at 09:21:50AM -0200, Mauro Carvalho Chehab wrote: > Em Tue, 20 Nov 2018 12:49:46 +0200 > Sakari Ailus escreveu: > > > Hi Greg, > > > > On Mon, Nov 19, 2018 at 06:46:21PM +0100, Greg Kroah-Hartman wrote: > > > On Mon, Nov 19, 2018 at 07:03:54PM +0200, Sakari Ailus wrote: > > > > Hi Greg, > > > > > > > > On Mon, Nov 19, 2018 at 04:14:00PM +0100, Greg Kroah-Hartman wrote: > > > > > On Wed, Nov 14, 2018 at 11:37:46AM +0200, Sakari Ailus wrote: > > > > > > [ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ] > > > > > > > > > > There is no such git commit id in Linus's tree :( > > > > > > > > Right. At the moment it's in the media tree only. I expect it'll end up > > > > to > > > > Linus's tree once Mauro will send the next pull request from the media > > > > tree > > > > to Linus. > > > > > > Ok, please do not send requests for stable tree inclusion until _AFTER_ > > > the patch is in Linus's tree, otherwise it just wastes the stable tree > > > maintainer's time :( > > > > Apologies for the noise. I'll send you a note once the patches are in > > Linus's tree. > > Btw, just sent a pull request with this patch. > > I wanted to send this two weeks ago, but I had to do two trips > (the final one to be at KS/LPC). This ended by delaying the pull request. The patch is in Linus's tree now. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH] media: ov2680: fix null dereference at power on
Hi Rui,
On Wed, Nov 21, 2018 at 10:59:55AM +, Rui Miguel Silva wrote:
> Swapping the order between v4l2 subdevice registration and checking chip id in
> b7a417628abf ("media: ov2680: don't register the v4l2 subdevice before
> checking chip ID")
> makes the mode restore to use the sensor controls before they are set, so move
> the mode restore call to s_power after the handler setup for controls is done.
>
> This remove also the need for the error code path in power on function.
Could you make sure you wrap the lines in the commit at 76 or so? Otherwise
they'll wrap in git log. I've re-wrapped it this time.
Thanks.
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v8 03/12] media: staging/imx7: add imx7 CSI subdev driver
csi->cc[IMX7_CSI_PAD_SRC] = outcc;
> + }
> +
> + if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY)
> + goto out_unlock;
> +
> + csi->cc[sdformat->pad] = cc;
> +
> + /* propagate output pad format to capture device */
> + imx_media_mbus_fmt_to_pix_fmt(_fmt,
> + >format_mbus[IMX7_CSI_PAD_SRC],
> + csi->cc[IMX7_CSI_PAD_SRC]);
The user is expected to configure the format on the video device; the driver
needs to validate that when starting the stream.
> + mutex_unlock(>lock);
> + imx_media_capture_device_set_format(vdev, _fmt);
> +
> + return 0;
> +
> +out_unlock:
> + mutex_unlock(>lock);
> +
> + return ret;
> +}
> +
> +static int imx7_csi_registered(struct v4l2_subdev *sd)
> +{
> + struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> + int ret;
> + int i;
> +
> + csi->md = dev_get_drvdata(sd->v4l2_dev->dev);
> +
> + for (i = 0; i < IMX7_CSI_PADS_NUM; i++) {
> + csi->pad[i].flags = (i == IMX7_CSI_PAD_SINK) ?
> + MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
> +
> + /* set a default mbus format */
> + ret = imx_media_init_mbus_fmt(>format_mbus[i],
> + 800, 600, 0, V4L2_FIELD_NONE,
> + >cc[i]);
> + if (ret < 0)
> + return ret;
> +
> + /* init default frame interval */
> + csi->frame_interval[i].numerator = 1;
> + csi->frame_interval[i].denominator = 30;
> + }
> +
> + ret = media_entity_pads_init(>entity, IMX7_CSI_PADS_NUM, csi->pad);
> + if (ret < 0)
> + return ret;
> +
> + ret = imx_media_capture_device_register(csi->vdev);
> + if (ret < 0)
> + return ret;
> +
> + ret = imx_media_add_video_device(csi->md, csi->vdev);
> + if (ret < 0) {
> + imx_media_capture_device_unregister(csi->vdev);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void imx7_csi_unregistered(struct v4l2_subdev *sd)
> +{
> + struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> +
> + imx_media_capture_device_unregister(csi->vdev);
> +}
> +
> +static int imx7_csi_init_cfg(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg)
> +{
> + struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> + struct v4l2_mbus_framefmt *mf;
> + int ret;
> + int i;
> +
> + for (i = 0; i < IMX7_CSI_PADS_NUM; i++) {
> + mf = v4l2_subdev_get_try_format(sd, cfg, i);
> +
> + ret = imx_media_init_mbus_fmt(mf, 800, 600, 0, V4L2_FIELD_NONE,
> + >cc[i]);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct media_entity_operations imx7_csi_entity_ops = {
> + .link_setup = imx7_csi_link_setup,
> + .link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static const struct v4l2_subdev_video_ops imx7_csi_video_ops = {
> + .s_stream = imx7_csi_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops imx7_csi_pad_ops = {
> + .init_cfg = imx7_csi_init_cfg,
> + .enum_mbus_code = imx7_csi_enum_mbus_code,
> + .get_fmt = imx7_csi_get_fmt,
> + .set_fmt = imx7_csi_set_fmt,
> + .link_validate =imx7_csi_pad_link_validate,
> +};
> +
> +static const struct v4l2_subdev_ops imx7_csi_subdev_ops = {
> + .video =_csi_video_ops,
> + .pad = _csi_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops imx7_csi_internal_ops = {
> + .registered = imx7_csi_registered,
> + .unregistered = imx7_csi_unregistered,
> +};
> +
> +static int imx7_csi_parse_endpoint(struct device *dev,
> +struct v4l2_fwnode_endpoint *vep,
> +struct v4l2_async_subdev *asd)
> +{
> + return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
> +}
> +
> +static int imx7_csi_parse_dt(struct imx7_csi *csi)
> +{
> + struct device *dev = csi->dev;
> +
> + csi->clk_disp_axi = devm_clk_get(dev, "axi");
> + if (IS_ERR(csi->clk_disp_axi)) {
> + dev_err(dev, "Could not get csi axi clock\n");
> + return -ENODEV;
> + }
> +
> + csi->clk_disp_dcic = devm_clk_get(dev, "dcic");
> + if (IS_ERR(csi->clk_disp_dcic)) {
> + dev_err(dev, "Could not get disp dcic clock\n");
> + return -ENODEV;
> + }
> +
> + csi->clk_csi_mclk = devm_clk_get(dev, "mclk");
> + if (IS_ERR(csi->clk_csi_mclk)) {
> + dev_err(dev, "Could not get csi mclk clock\n");
> + return -ENODEV;
> + }
Could you use devm_clk_bulk_get()?
> +
> + return 0;
> +}
> +
> +static int imx7_csi_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + struct device_node *node = dev->of_node;
> + struct imx7_csi *csi;
> + struct resource *res;
> + int ret;
> +
> + csi = devm_kzalloc(>dev, sizeof(*csi), GFP_KERNEL);
> + if (!csi)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, >sd);
> + csi->dev = dev;
> +
> + ret = imx7_csi_parse_dt(csi);
> + if (ret < 0)
> + return -ENODEV;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + csi->irq = platform_get_irq(pdev, 0);
> + if (!res || csi->irq < 0) {
> + dev_err(dev, "Missing platform resources data\n");
> + return -ENODEV;
> + }
> +
> + csi->regbase = devm_ioremap_resource(dev, res);
> + if (IS_ERR(csi->regbase)) {
> + dev_err(dev, "Failed platform resources map\n");
> + return -ENODEV;
> + }
> +
> + spin_lock_init(>irqlock);
> + mutex_init(>lock);
> +
> + /* install interrupt handler */
> + ret = devm_request_irq(dev, csi->irq, imx7_csi_irq_handler, 0, "csi",
> +(void *)csi);
> + if (ret < 0) {
> + dev_err(dev, "Request CSI IRQ failed.\n");
> + return -ENODEV;
mutex_destroy(); better to add a label below.
> + }
> +
> + /* add media device */
> + csi->imxmd = imx_media_dev_init(dev);
> + if (IS_ERR(csi->imxmd))
> + return PTR_ERR(csi->imxmd);
> +
> + ret = imx_media_of_add_csi(csi->imxmd, node);
> + if (ret < 0)
> + goto media_cleanup;
> +
> + ret = imx_media_dev_notifier_register(csi->imxmd);
> + if (ret < 0)
> + goto media_cleanup;
> +
> + v4l2_subdev_init(>sd, _csi_subdev_ops);
> + v4l2_set_subdevdata(>sd, csi);
> + csi->sd.internal_ops = _csi_internal_ops;
> + csi->sd.entity.ops = _csi_entity_ops;
> + csi->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> + csi->sd.dev = >dev;
> + csi->sd.owner = THIS_MODULE;
> + csi->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> + csi->sd.grp_id = IMX_MEDIA_GRP_ID_CSI;
> + snprintf(csi->sd.name, sizeof(csi->sd.name), "csi");
> +
> + csi->vdev = imx_media_capture_device_init(>sd, IMX7_CSI_PAD_SRC);
> + if (IS_ERR(csi->vdev))
> + return PTR_ERR(csi->vdev);
> +
> + v4l2_ctrl_handler_init(>ctrl_hdlr, 0);
> + csi->sd.ctrl_handler = >ctrl_hdlr;
> +
> + ret = v4l2_async_register_fwnode_subdev(>sd,
> + sizeof(struct v4l2_async_subdev),
> + NULL, 0, imx7_csi_parse_endpoint);
> + if (ret)
> + goto free;
> +
> + return 0;
> +
> +free:
> + v4l2_ctrl_handler_free(>ctrl_hdlr);
> + mutex_destroy(>lock);
> + imx_media_capture_device_remove(csi->vdev);
> +
> +media_cleanup:
> + imx_media_dev_cleanup(csi->imxmd);
> +
> + return ret;
> +}
> +
> +static int imx7_csi_remove(struct platform_device *pdev)
> +{
> + struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> + struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> +
> + v4l2_ctrl_handler_free(>ctrl_hdlr);
> + mutex_destroy(>lock);
Please unregister the devices before the two above lines.
> + imx_media_capture_device_remove(csi->vdev);
> + imx_media_dev_notifier_unregister(csi->imxmd);
> + imx_media_dev_cleanup(csi->imxmd);
> + media_entity_cleanup(>entity);
> + v4l2_async_unregister_subdev(sd);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id imx7_csi_of_match[] = {
> + { .compatible = "fsl,imx7-csi" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
> +
> +static struct platform_driver imx7_csi_driver = {
> + .probe = imx7_csi_probe,
> + .remove = imx7_csi_remove,
> + .driver = {
> + .of_match_table = imx7_csi_of_match,
> + .name = "imx7-csi",
> + },
> +};
> +module_platform_driver(imx7_csi_driver);
> +
> +MODULE_DESCRIPTION("i.MX7 CSI subdev driver");
> +MODULE_AUTHOR("Rui Miguel Silva ");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:imx7-csi");
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v8 05/12] media: dt-bindings: add bindings for i.MX7 media driver
Hi Rui,
On Wed, Nov 21, 2018 at 11:15:51AM +, Rui Miguel Silva wrote:
> Add bindings documentation for i.MX7 media drivers.
> The imx7 MIPI CSI2 and imx7 CMOS Sensor Interface.
>
> Signed-off-by: Rui Miguel Silva
> Reviewed-by: Rob Herring
> Acked-by: Sakari Ailus
> ---
> .../devicetree/bindings/media/imx7-csi.txt| 45 ++
> .../bindings/media/imx7-mipi-csi2.txt | 90 +++
> 2 files changed, 135 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/imx7-csi.txt
> create mode 100644 Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
>
> diff --git a/Documentation/devicetree/bindings/media/imx7-csi.txt
> b/Documentation/devicetree/bindings/media/imx7-csi.txt
> new file mode 100644
> index ..171b089ee91f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/imx7-csi.txt
> @@ -0,0 +1,45 @@
> +Freescale i.MX7 CMOS Sensor Interface
> +=
> +
> +csi node
> +
> +
> +This is device node for the CMOS Sensor Interface (CSI) which enables the
> chip
> +to connect directly to external CMOS image sensors.
> +
> +Required properties:
> +
> +- compatible: "fsl,imx7-csi";
> +- reg : base address and length of the register set for the device;
> +- interrupts: should contain CSI interrupt;
> +- clocks: list of clock specifiers, see
> +Documentation/devicetree/bindings/clock/clock-bindings.txt for
> details;
> +- clock-names : must contain "axi", "mclk" and "dcic" entries, matching
> + entries in the clock property;
> +
> +The device node shall contain one 'port' child node with one child 'endpoint'
> +node, according to the bindings defined in:
> + Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +In the following example a remote endpoint is a video multiplexer.
> +
> +example:
> +
> +csi: csi@3071 {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +compatible = "fsl,imx7-csi";
> +reg = <0x3071 0x1>;
> +interrupts = ;
> +clocks = < IMX7D_CLK_DUMMY>,
> +< IMX7D_CSI_MCLK_ROOT_CLK>,
> +< IMX7D_CLK_DUMMY>;
> +clock-names = "axi", "mclk", "dcic";
> +
> +port {
> +csi_from_csi_mux: endpoint {
> +remote-endpoint = <_mux_to_csi>;
> +};
> +};
> +};
> diff --git a/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
> b/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
> new file mode 100644
> index ..71fd74ed3ec8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
> @@ -0,0 +1,90 @@
> +Freescale i.MX7 Mipi CSI2
> +=
> +
> +mipi_csi2 node
> +--
> +
> +This is the device node for the MIPI CSI-2 receiver core in i.MX7 SoC. It is
> +compatible with previous version of Samsung D-phy.
> +
> +Required properties:
> +
> +- compatible: "fsl,imx7-mipi-csi2";
> +- reg : base address and length of the register set for the device;
> +- interrupts: should contain MIPI CSIS interrupt;
> +- clocks: list of clock specifiers, see
> +Documentation/devicetree/bindings/clock/clock-bindings.txt for
> details;
> +- clock-names : must contain "pclk", "wrap" and "phy" entries, matching
> + entries in the clock property;
> +- power-domains : a phandle to the power domain, see
> + Documentation/devicetree/bindings/power/power_domain.txt for
> details.
> +- reset-names : should include following entry "mrst";
> +- resets: a list of phandle, should contain reset entry of
> + reset-names;
> +- phy-supply: from the generic phy bindings, a phandle to a regulator
> that
> + provides power to MIPI CSIS core;
> +
> +Optional properties:
> +
> +- clock-frequency : The IP's main (system bus) clock frequency in Hz, default
> + value when this property is not specified is 166 MHz;
> +- fsl,csis-hs-settle : differential receiver (HS-RX) settle time;
> +
> +The device node should contain
[PATCH 1/1] v4l: uAPI doc: Changing frame interval won't change format
Document that changing the frame interval has no effect on frame size. While this was the assumption in the API, it was not documented as such. Signed-off-by: Sakari Ailus --- Documentation/media/uapi/v4l/vidioc-g-parm.rst | 3 +++ Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst | 3 +++ 2 files changed, 6 insertions(+) diff --git a/Documentation/media/uapi/v4l/vidioc-g-parm.rst b/Documentation/media/uapi/v4l/vidioc-g-parm.rst index e831fa5512f0..c31585a7701b 100644 --- a/Documentation/media/uapi/v4l/vidioc-g-parm.rst +++ b/Documentation/media/uapi/v4l/vidioc-g-parm.rst @@ -42,6 +42,9 @@ side. This is especially useful when using the :ref:`read() ` or :ref:`write() `, which are not augmented by timestamps or sequence counters, and to avoid unnecessary data copying. +Changing the frame interval shall never change the format. Changing the +format, on the other hand, may change the frame interval. + Further these ioctls can be used to determine the number of buffers used internally by a driver in read/write mode. For implications see the section discussing the :ref:`read() ` function. diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst index 5af0a7179941..f889c20f231c 100644 --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst @@ -63,6 +63,9 @@ doesn't match the device capabilities. They must instead modify the interval to match what the hardware can provide. The modified interval should be as close as possible to the original request. +Changing the frame interval shall never change the format. Changing the +format, on the other hand, may change the frame interval. + Sub-devices that support the frame interval ioctls should implement them on a single pad only. Their behaviour when supported on multiple pads of the same sub-device is not defined. -- 2.11.0
Re: [PATCH] media: video-i2c: don't use msleep for 1ms - 20ms
On Wed, Nov 21, 2018 at 12:27:40AM +0900, Akinobu Mita wrote: > Documentation/timers/timers-howto.txt says: > > "msleep(1~20) may not do what the caller intends, and will often sleep > longer (~20 ms actual sleep for any value given in the 1~20ms range)." > > So replace msleep(2) by usleep_range(2000, 3000). > > Reported-by: Hans Verkuil > Cc: Matt Ranostay > Cc: Sakari Ailus > Cc: Hans Verkuil > Cc: Mauro Carvalho Chehab > Signed-off-by: Akinobu Mita Acked-by: Sakari Ailus -- Sakari Ailus sakari.ai...@linux.intel.com
Re: 'bad remote port parent' warnings
On Tue, Nov 20, 2018 at 10:10:57AM -0200, Fabio Estevam wrote: > Hi, > > On a imx6q-wandboard running linux-next 20181120 there the following warnings: > > [4.327794] video-mux 20e.iomuxc-gpr:ipu1_csi0_mux: bad remote > port parent > [4.336118] video-mux 20e.iomuxc-gpr:ipu2_csi1_mux: bad remote > port parent > > Is there anything we should do to prevent this from happening? Where's the DT source for the board? -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v2 for v4.4 1/1] v4l: event: Add subscription to list before calling "add" operation
Hi Greg, On Mon, Nov 19, 2018 at 06:46:21PM +0100, Greg Kroah-Hartman wrote: > On Mon, Nov 19, 2018 at 07:03:54PM +0200, Sakari Ailus wrote: > > Hi Greg, > > > > On Mon, Nov 19, 2018 at 04:14:00PM +0100, Greg Kroah-Hartman wrote: > > > On Wed, Nov 14, 2018 at 11:37:46AM +0200, Sakari Ailus wrote: > > > > [ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ] > > > > > > There is no such git commit id in Linus's tree :( > > > > Right. At the moment it's in the media tree only. I expect it'll end up to > > Linus's tree once Mauro will send the next pull request from the media tree > > to Linus. > > Ok, please do not send requests for stable tree inclusion until _AFTER_ > the patch is in Linus's tree, otherwise it just wastes the stable tree > maintainer's time :( Apologies for the noise. I'll send you a note once the patches are in Linus's tree. Thanks. -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com
[GIT PULL for 4.21] Sensor and CSI driver patches
Hi Mauro, Here are a bunch of sensor driver improvements as well as a driver for the Allwinner CSI parallel bridge. Finally, a small uAPI documentation fix to better document the metadata capture buffers. Please pull. The following changes since commit fbe57dde7126d1b2712ab5ea93fb9d15f89de708: media: ov7740: constify structures stored in fields of v4l2_subdev_ops structure (2018-11-06 07:17:02 -0500) are available in the git repository at: ssh://linuxtv.org/git/sailus/media_tree.git tags/for-4.21-2-sign for you to fetch changes up to 76bdfdc1cf59b13a204ff18d57549f635956050e: media: V3s: Add support for Allwinner CSI. (2018-11-20 12:31:04 +0200) sensor + CSI patches for 4.21 Akinobu Mita (7): media: mt9m111: support log_status ioctl and event interface media: mt9m111: add V4L2_CID_COLORFX control media: ov2640: add V4L2_CID_TEST_PATTERN control media: ov2640: support log_status ioctl and event interface media: ov5640: support log_status ioctl and event interface media: ov7670: support log_status ioctl and event interface media: ov772x: support log_status ioctl and event interface Chen, JasonX Z (1): media: imx258: remove test pattern map from driver Nathan Chancellor (1): media: imx214: Remove unnecessary self assignment in for loop Sakari Ailus (1): v4l: uAPI doc: Simplify NATIVE_SIZE selection target documentation Yong Deng (2): dt-bindings: media: Add Allwinner V3s Camera Sensor Interface (CSI) media: V3s: Add support for Allwinner CSI. .../devicetree/bindings/media/sun6i-csi.txt| 56 ++ .../media/uapi/v4l/v4l2-selection-targets.rst | 7 +- MAINTAINERS| 8 + drivers/media/i2c/imx214.c | 2 +- drivers/media/i2c/imx258.c | 22 +- drivers/media/i2c/mt9m111.c| 44 +- drivers/media/i2c/ov2640.c | 21 +- drivers/media/i2c/ov5640.c | 7 +- drivers/media/i2c/ov7670.c | 6 +- drivers/media/i2c/ov772x.c | 7 +- drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile| 2 + drivers/media/platform/sunxi/sun6i-csi/Kconfig | 9 + drivers/media/platform/sunxi/sun6i-csi/Makefile| 3 + drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c | 912 + drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h | 135 +++ .../media/platform/sunxi/sun6i-csi/sun6i_csi_reg.h | 196 + .../media/platform/sunxi/sun6i-csi/sun6i_video.c | 678 +++ .../media/platform/sunxi/sun6i-csi/sun6i_video.h | 38 + 19 files changed, 2121 insertions(+), 33 deletions(-) create mode 100644 Documentation/devicetree/bindings/media/sun6i-csi.txt create mode 100644 drivers/media/platform/sunxi/sun6i-csi/Kconfig create mode 100644 drivers/media/platform/sunxi/sun6i-csi/Makefile create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_reg.h create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_video.h -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH] videodev2.h: add V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF/CREATE_BUFS
On Tue, Nov 20, 2018 at 10:41:42AM +0100, Hans Verkuil wrote: > On 11/20/2018 10:27 AM, Sakari Ailus wrote: > > Hi Hans, > > > > On Tue, Nov 20, 2018 at 09:58:43AM +0100, Hans Verkuil wrote: > >> Add new buffer capability flags to indicate if the VIDIOC_PREPARE_BUF or > >> VIDIOC_CREATE_BUFS ioctls are supported. > > > > Are there practical benefits from the change for the user space? > > The more important ioctl to know about is PREPARE_BUF. I noticed this when > working > on v4l2-compliance: the only way to know for an application if PREPARE_BUF > exists > is by trying it, but then you already have prepared a buffer. That's not what > you > want in the application, you need a way to know up front if prepare_buf is > present > or not without having to actually execute it. > > CREATE_BUFS was added because not all drivers support it. It can be dropped > since > it is possible to test for the existence of CREATE_BUFS without actually > allocating > anything, but if I'm adding V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF anyway, then it > is > trivial to add V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS as well to avoid an > additional > ioctl call. > > Hmm, I should have explained this in the commit log. Please add: Acked-by: Sakari Ailus -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH] videodev2.h: add V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF/CREATE_BUFS
Hi Hans,
On Tue, Nov 20, 2018 at 09:58:43AM +0100, Hans Verkuil wrote:
> Add new buffer capability flags to indicate if the VIDIOC_PREPARE_BUF or
> VIDIOC_CREATE_BUFS ioctls are supported.
Are there practical benefits from the change for the user space?
>
> Signed-off-by: Hans Verkuil
> ---
> Note: the flag bits will change since there are two other patches that add
> flags, so the numbering will change.
> ---
> diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> index d40c60e8..abf925484aff 100644
> --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> @@ -112,6 +112,8 @@ any DMA in progress, an implicit
> .. _V4L2-BUF-CAP-SUPPORTS-USERPTR:
> .. _V4L2-BUF-CAP-SUPPORTS-DMABUF:
> .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS:
> +.. _V4L2-BUF-CAP-SUPPORTS-PREPARE-BUF:
> +.. _V4L2-BUF-CAP-SUPPORTS-CREATE-BUFS:
>
> .. cssclass:: longtable
>
> @@ -132,6 +134,12 @@ any DMA in progress, an implicit
> * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS``
>- 0x0008
>- This buffer type supports :ref:`requests `.
> +* - ``V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF``
> + - 0x0010
> + - This buffer type supports :ref:`VIDIOC_PREPARE_BUF`.
> +* - ``V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS``
> + - 0x0020
> + - This buffer type supports :ref:`VIDIOC_CREATE_BUFS`.
>
> Return Value
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index a17033ab2c22..27c0fafca0bf 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -871,6 +871,16 @@ static inline bool vb2_queue_is_busy(struct video_device
> *vdev, struct file *fil
> return vdev->queue->owner && vdev->queue->owner != file->private_data;
> }
>
> +static void fill_buf_caps_vdev(struct video_device *vdev, u32 *caps)
> +{
> + *caps = 0;
> + fill_buf_caps(vdev->queue, caps);
> + if (vdev->ioctl_ops->vidioc_prepare_buf)
> + *caps |= V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF;
> + if (vdev->ioctl_ops->vidioc_create_bufs)
> + *caps |= V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS;
> +}
> +
> /* vb2 ioctl helpers */
>
> int vb2_ioctl_reqbufs(struct file *file, void *priv,
> @@ -879,7 +889,7 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv,
> struct video_device *vdev = video_devdata(file);
> int res = vb2_verify_memory_type(vdev->queue, p->memory, p->type);
>
> - fill_buf_caps(vdev->queue, >capabilities);
> + fill_buf_caps_vdev(vdev, >capabilities);
> if (res)
> return res;
> if (vb2_queue_is_busy(vdev, file))
> @@ -901,7 +911,7 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
> p->format.type);
>
> p->index = vdev->queue->num_buffers;
> - fill_buf_caps(vdev->queue, >capabilities);
> + fill_buf_caps_vdev(vdev, >capabilities);
> /*
>* If count == 0, then just check if memory and type are valid.
>* Any -EBUSY result from vb2_verify_memory_type can be mapped to 0.
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index c8e8ff810190..6648f8ba2277 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -879,6 +879,8 @@ struct v4l2_requestbuffers {
> #define V4L2_BUF_CAP_SUPPORTS_USERPTR(1 << 1)
> #define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
> #define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
Could you align the previous lines to match the ones below?
> +#define V4L2_BUF_CAP_SUPPORTS_PREPARE_BUF(1 << 4)
> +#define V4L2_BUF_CAP_SUPPORTS_CREATE_BUFS(1 << 5)
>
> /**
> * struct v4l2_plane - plane info for multi-planar buffers
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v2 for v4.4 1/1] v4l: event: Add subscription to list before calling "add" operation
Hi Greg, On Mon, Nov 19, 2018 at 04:14:00PM +0100, Greg Kroah-Hartman wrote: > On Wed, Nov 14, 2018 at 11:37:46AM +0200, Sakari Ailus wrote: > > [ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ] > > There is no such git commit id in Linus's tree :( Right. At the moment it's in the media tree only. I expect it'll end up to Linus's tree once Mauro will send the next pull request from the media tree to Linus. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v2 3/9] phy: Add MIPI D-PHY configuration options
gned inths_prepare;
> +
> + /**
> + * @hs_settle:
> + *
> + * Time interval, in nanoseconds, during which the HS receiver
> + * shall ignore any Data Lane HS transitions, starting from
> + * the beginning of @hs_prepare.
> + */
> + unsigned inths_settle;
> +
> + /**
> + * @hs_skip:
> + *
> + * Time interval, in nanoseconds, during which the HS-RX
> + * should ignore any transitions on the Data Lane, following a
> + * HS burst. The end point of the interval is defined as the
> + * beginning of the LP-11 state following the HS burst.
> + */
> + unsigned inths_skip;
> +
> + /**
> + * @hs_trail:
> + *
> + * Time, in nanoseconds, that the transmitter drives the
> + * flipped differential state after last payload data bit of a
> + * HS transmission burst
> + */
> + unsigned inths_trail;
> +
> + /**
> + * @hs_zero:
> + *
> + * Time, in nanoseconds, that the transmitter drives the HS-0
> + * state prior to transmitting the Sync sequence.
> + */
> + unsigned inths_zero;
> +
> + /**
> + * @init:
> + *
> + * Time, in nanoseconds for the initialization period to
> + * complete.
> + */
> + unsigned intinit;
> +
> + /**
> + * @lpx:
> + *
> + * Transmitted length, in nanoseconds, of any Low-Power state
> + * period.
> + */
> + unsigned intlpx;
> +
> + /**
> + * @ta_get:
> + *
> + * Time, in nanoseconds, that the new transmitter drives the
> + * Bridge state (LP-00) after accepting control during a Link
> + * Turnaround.
> + */
> + unsigned intta_get;
> +
> + /**
> + * @ta_go:
> + *
> + * Time, in nanoseconds, that the transmitter drives the
> + * Bridge state (LP-00) before releasing control during a Link
> + * Turnaround.
> + */
> + unsigned intta_go;
> +
> + /**
> + * @ta_sure:
> + *
> + * Time, in nanoseconds, that the new transmitter waits after
> + * the LP-10 state before transmitting the Bridge state
> + * (LP-00) during a Link Turnaround.
> + */
> + unsigned intta_sure;
> +
> + /**
> + * @wakeup:
> + *
> + * Time, in nanoseconds, that a transmitter drives a Mark-1
> + * state prior to a Stop state in order to initiate an exit
> + * from ULPS.
> + */
> + unsigned intwakeup;
> +
> + /**
> + * @hs_clk_rate:
> + *
> + * Clock rate, in Hertz, of the high-speed clock.
> + */
> + unsigned long hs_clk_rate;
> +
> + /**
> + * @lp_clk_rate:
> + *
> + * Clock rate, in Hertz, of the low-power clock.
> + */
> + unsigned long lp_clk_rate;
> +
> + /**
> + * @lanes:
> + *
> + * Number of lanes used for the transmissions.
> + */
> + unsigned char lanes;
This is related to the data_lanes DT property. I assume this is intended to
be the number of active lanes. And presumably the first "lanes" number of
lanes would be used in that case?
> +};
> +
> +#endif /* __PHY_MIPI_DPHY_H_ */
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index 89cf8b685546..d7ea2dc4e2be 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -20,6 +20,8 @@
> #include
> #include
>
> +#include
> +
> struct phy;
>
> enum phy_mode {
> @@ -47,8 +49,12 @@ enum phy_mode {
>
> /**
> * union phy_configure_opts - Opaque generic phy configuration
> + *
> + * @mipi_dphy: Configuration set applicable for phys supporting
> + * the MIPI_DPHY phy mode.
> */
> union phy_configure_opts {
> + struct phy_configure_opts_mipi_dphy mipi_dphy;
> };
>
> /**
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v2 4/9] phy: dphy: Add configuration helpers
cfg->init = 10;
> + cfg->lpx = 60;
> + cfg->ta_get = 5 * cfg->lpx;
> + cfg->ta_go = 4 * cfg->lpx;
> + cfg->ta_sure = 2 * cfg->lpx;
> + cfg->wakeup = 100;
> +
> + cfg->hs_clk_rate = hs_clk_rate;
How about the LP clock?
Frankly, I have worked with MIPI CSI-2 hardware soon a decade, and the very
few cases where software has needed to deal with these values has been in
form of defaults for a receiver, mostly limiting to clk_settle,
clk_term_en, d_term_en as well as hs_settle. On some hardware, the data
lane specific values can be at least in theory configured separately on
different lanes (but perhaps we could ignore that now).
That doesn't say that it'd be useless to convey these values to the PHY
though. What I'm a little worried about though is what could be the effect
of adding support for this for existing drivers? If you have a new driver,
then there is no chance of regressions.
I can't help noticing that many of the above values end up being unused in
the rest of the patches in the set. I guess that's ok, they come from the
standard anyway and some hardware may need them to be configured.
Then there's the question of where should these values originate from.
Some drivers appear to have a need to obtain one of these values via
firmware, see Documentation/devicetree/bindings/media/samsung-mipi-csis.txt
. I presume the defaults should be applicable to most cases, and specific
values would need to be defined in the firmware. That means that the
defaults have effectively the property of firmware API, meaning that they
effectively can never be changed. That suggests we should be pretty sure
the defaults are something that should work for the widest possible set of
the hardware.
> + cfg->lanes = lanes;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(phy_mipi_dphy_get_default_config);
> +
> +/*
> + * Validate D-PHY configuration according to MIPI D-PHY specification
> + * (v1.2, Section Section 6.9 "Global Operation Timing Parameters").
> + */
> +int phy_mipi_dphy_config_validate(struct phy_configure_opts_mipi_dphy *cfg)
> +{
> + unsigned long ui;
> +
> + if (!cfg)
> + return -EINVAL;
> +
> + ui = DIV_ROUND_UP(NSEC_PER_SEC, cfg->hs_clk_rate);
> +
> + if (cfg->clk_miss > 60)
> + return -EINVAL;
> +
> + if (cfg->clk_post < (60 + 52 * ui))
Extra parentheses.
> + return -EINVAL;
> +
> + if (cfg->clk_pre < 8)
> + return -EINVAL;
> +
> + if (cfg->clk_prepare < 38 || cfg->clk_prepare > 95)
> + return -EINVAL;
> +
> + if (cfg->clk_settle < 95 || cfg->clk_settle > 300)
> + return -EINVAL;
> +
> + if (cfg->clk_term_en > 38)
> + return -EINVAL;
> +
> + if (cfg->clk_trail < 60)
> + return -EINVAL;
> +
> + if (cfg->clk_prepare + cfg->clk_zero < 300)
> + return -EINVAL;
> +
> + if (cfg->d_term_en > 35 + 4 * ui)
> + return -EINVAL;
> +
> + if (cfg->eot > 105 + 12 * ui)
> + return -EINVAL;
> +
> + if (cfg->hs_exit < 100)
> + return -EINVAL;
> +
> + if (cfg->hs_prepare < 40 + 4 * ui ||
> + cfg->hs_prepare > 85 + 6 * ui)
> + return -EINVAL;
> +
> + if (cfg->hs_prepare + cfg->hs_zero < 145 + 10 * ui)
> + return -EINVAL;
> +
> + if ((cfg->hs_settle < 85 + 6 * ui) ||
> + (cfg->hs_settle > 145 + 10 * ui))
Ditto.
> + return -EINVAL;
> +
> + if (cfg->hs_skip < 40 || cfg->hs_skip > 55 + 4 * ui)
> + return -EINVAL;
> +
> + if (cfg->hs_trail < max(8 * ui, 60 + 4 * ui))
> + return -EINVAL;
> +
> + if (cfg->init < 10)
> + return -EINVAL;
> +
> + if (cfg->lpx < 50)
> + return -EINVAL;
> +
> + if (cfg->ta_get != 5 * cfg->lpx)
> + return -EINVAL;
> +
> + if (cfg->ta_go != 4 * cfg->lpx)
> + return -EINVAL;
> +
> + if (cfg->ta_sure < cfg->lpx || cfg->ta_sure > 2 * cfg->lpx)
> + return -EINVAL;
> +
> + if (cfg->wakeup < 100)
> + return -EINVAL;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(phy_mipi_dphy_config_validate);
> diff --git a/include/linux/phy/phy-mipi-dphy.h
> b/include/linux/phy/phy-mipi-dphy.h
> index 0b05932916af..5e3673778afa 100644
> --- a/include/linux/phy/phy-mipi-dphy.h
> +++ b/include/linux/phy/phy-mipi-dphy.h
> @@ -229,4 +229,10 @@ struct phy_configure_opts_mipi_dphy {
> unsigned char lanes;
> };
>
> +int phy_mipi_dphy_get_default_config(unsigned long pixel_clock,
> + unsigned int bpp,
> + unsigned int lanes,
> + struct phy_configure_opts_mipi_dphy *cfg);
> +int phy_mipi_dphy_config_validate(struct phy_configure_opts_mipi_dphy *cfg);
> +
> #endif /* __PHY_MIPI_DPHY_H_ */
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCHv2 2/4] vivid: use per-queue mutexes instead of one global mutex.
vb_vbi_out_q;
> + struct mutexvb_vbi_out_q_lock;
> struct list_headvbi_out_active;
>
> /* video loop precalculated rectangles */
> @@ -459,6 +463,7 @@ struct vivid_dev {
>
> /* SDR capture */
> struct vb2_queuevb_sdr_cap_q;
> + struct mutexvb_sdr_cap_q_lock;
> struct list_headsdr_cap_active;
> u32 sdr_pixelformat; /* v4l2 format id */
> unsignedsdr_buffersize;
> diff --git a/drivers/media/platform/vivid/vivid-kthread-cap.c
> b/drivers/media/platform/vivid/vivid-kthread-cap.c
> index eebfff2126be..d8bb59e9bcc7 100644
> --- a/drivers/media/platform/vivid/vivid-kthread-cap.c
> +++ b/drivers/media/platform/vivid/vivid-kthread-cap.c
> @@ -927,8 +927,6 @@ void vivid_stop_generating_vid_cap(struct vivid_dev *dev,
> bool *pstreaming)
>
> /* shutdown control thread */
> vivid_grab_controls(dev, false);
> - mutex_unlock(>mutex);
> kthread_stop(dev->kthread_vid_cap);
> dev->kthread_vid_cap = NULL;
> - mutex_lock(>mutex);
> }
> diff --git a/drivers/media/platform/vivid/vivid-kthread-out.c
> b/drivers/media/platform/vivid/vivid-kthread-out.c
> index 5a14810eeb69..8b864cb0ed52 100644
> --- a/drivers/media/platform/vivid/vivid-kthread-out.c
> +++ b/drivers/media/platform/vivid/vivid-kthread-out.c
> @@ -298,8 +298,6 @@ void vivid_stop_generating_vid_out(struct vivid_dev *dev,
> bool *pstreaming)
>
> /* shutdown control thread */
> vivid_grab_controls(dev, false);
> - mutex_unlock(>mutex);
> kthread_stop(dev->kthread_vid_out);
> dev->kthread_vid_out = NULL;
> - mutex_lock(>mutex);
> }
> diff --git a/drivers/media/platform/vivid/vivid-sdr-cap.c
> b/drivers/media/platform/vivid/vivid-sdr-cap.c
> index dcdc80e272c2..5dfb598af742 100644
> --- a/drivers/media/platform/vivid/vivid-sdr-cap.c
> +++ b/drivers/media/platform/vivid/vivid-sdr-cap.c
> @@ -305,10 +305,8 @@ static void sdr_cap_stop_streaming(struct vb2_queue *vq)
> }
>
> /* shutdown control thread */
> - mutex_unlock(>mutex);
> kthread_stop(dev->kthread_sdr_cap);
> dev->kthread_sdr_cap = NULL;
> - mutex_lock(>mutex);
> }
>
> static void sdr_cap_buf_request_complete(struct vb2_buffer *vb)
> --
> 2.19.1
>
--
Sakari Ailus
sakari.ai...@linux.intel.com
[ANN] Edinburgh Media Summit 2018 meeting report
Hello everyone,
Here's the report on the Media Summit held on 25th October in Edinburgh.
The report is followed by the stateless codec discussion two days earlier.
Note: this is bcc'd to the meeting attendees plus a few others. I didn't
use cc as the list servers tend to reject messages with too many
recipients in cc / to headers.
Most presenters used slides some of which are already available here
(expect more in the near future):
https://www.linuxtv.org/downloads/presentations/media_summit_2018/>
The original announcement for the meeting is here:
https://www.spinics.net/lists/linux-media/msg141095.html>
The raw notes can be found here:
http://www.retiisi.org.uk/~sailus/v4l2/notes/osseu18-media.html>
Attendees
-
Brad Love
Ezequiel Garcia
Gustavo Padovan
Hans Verkuil
Helen Koike
Hidenori Yamaji
Ivan Kalinin
Jacopo Mondi
Kieran Bingham
Laurent Pinchart
Mauro Chebab
Maxime Ripard
Michael Grzeschik
Michael Ira Krufky
Niklas Söderlund
Patrick Lai
Paul Elder
Peter Griffin
Ralph Clark
Ricardo Ribalda
Sakari Ailus
Sean Young
Seung-Woo Kim
Stefan Klug
Vinod Koul
CEC status - Hans Verkuil
-
Hans prensented an update on CEC status. Besides the slides, noteworthy
information is maintained here:
https://hverkuil.home.xs4all.nl/cec-status.txt>
Slides:
https://www.linuxtv.org/downloads/presentations/media_summit_2018/media-cec-status.pdf>
rc-core status report - Sean Young
--
(Contributed by Sean Young)
In the last year all staging lirc drivers have been either removed
or ported to rc-core. Decoding of the more obscure IR protocols and
protocol variants can now be done with BPF, with support in both the
kernel and ir-keytable (which is in v4l-utils). Generally we're in a good
situation wrt IR support.
There is some more ancient hardware (serial or usb-serial) that does not
have support but not sure if anyone cares. kernel-doc is a little sparse
and does not cover BPF IR decoding, so that needs improving. There was a
discussion on enabling builds with CONFIG_RC_CORE=n. Sean suggested we
could have rc_allocate_driver() return NULL and have the drivers deal
with this gracefully, i.e. their probe functions should continue without
IR. Mauro said there should be a per-driver config option (as is done
for saa7134 for example).
No conclusion was reached on this.
Persistent storage of controls - Ricardo Ribalda
Ricardo gave a presentation on a proposed solution for using the V4L2
control framework as an interface for updating control value defaults on
sensor EEPROM.
Sensors commonly come with device specific tuning information that's
embedded in the device EEPROM. Whereas this is also very common for raw
cameras on mobile devices, the discussion this time was concentrated on
industrial cameras.
The EEPROM contents may be written by the sensor vendor but occasionally
may need to be updated by customers. Setting the control default value was
suggested as the exact mechanism to do this.
The proposal was to use controls as the interface to update sensor tuning
information in the EEPROM.
There were arguments for and against the approach:
+ Drivers usually get these things right: relying on an user space program
to do this is an additional dependency.
+ Re-use of an existing interface (root priviledge check may be added).
- Partial solution only: EEPROM contents may need to be updated for other
reasons as well, and a "spotty" implementation for updating certain
EEPROM locations seems very use case specific.
- Changes required to the control framework for this --- defaults are not
settable at the moment.
- The need is very use case specific, and adding support for that in a
generic framework does not seem to fit very well.
The general consensus appears to be not to change the control framework
this way, but to continue to update the EEPROM using a specific user space
program.
Tooling for sub-system tree maintenance - Laurent Pinchart
--
Laurent talked about the DRM tree maintenance model.
The DRM tree has switched to co-maintainer model. This has made it possible
to share the burden of tree maintenance, removing bottlenecks they've had.
The larger number of people having (and using) their commit rights has
created the need for a more strict rules for the tree maintenance, and
subsequently a tool to implement it. It's called "DIM", the DRM Inglorious
Maintenance tool. This is a command line tool that works as a front-end to
execute the workflow.
https://01.org/linuxgraphics/gfx-docs/maintainer-tools/dim.html>
In particular what's worth noting:
- The conflicts are resolved by the committ
Re: [PATCH v2 3/6] media: mt9m111: add support to select formats and fps for {Q,SXGA}
Hi Hans,
On Fri, Nov 16, 2018 at 02:31:01PM +0100, Hans Verkuil wrote:
> On 11/16/2018 02:26 PM, Sakari Ailus wrote:
> > Hi Marco, Michael,
> >
> > On Mon, Oct 29, 2018 at 07:24:07PM +0100, Marco Felsch wrote:
> >> From: Michael Grzeschik
> >>
> >> This patch implements the framerate selection using the skipping and
> >> readout power-modi features. The power-modi cut the framerate by half
> >> and each context has an independent selection bit. The same applies to
> >> the 2x skipping feature.
> >>
> >> Signed-off-by: Michael Grzeschik
> >> Signed-off-by: Marco Felsch
> >>
> >> ---
> >> Changelog
> >>
> >> v2:
> >> - fix updating read mode register, use mt9m111_reg_mask() to update the
> >> relevant bits only. For this purpose add reg_mask field to
> >> struct mt9m111_mode_info.
> >>
> >> drivers/media/i2c/mt9m111.c | 163
> >> 1 file changed, 163 insertions(+)
> >>
> >> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
>
>
>
> >> +static const struct mt9m111_mode_info *
> >> +mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
> >> +unsigned int width, unsigned int height)
> >> +{
> >> + const struct mt9m111_mode_info *mode;
> >> + struct v4l2_rect *sensor_rect = >rect;
> >> + unsigned int gap, gap_best = (unsigned int) -1;
> >> + int i, best_gap_idx = 1;
> >> +
> >> + /* find best matched fps */
> >> + for (i = 0; i < MT9M111_NUM_MODES; i++) {
> >> + unsigned int fps = mt9m111_mode_data[i].max_fps;
> >> +
> >> + gap = abs(fps - req_fps);
> >> + if (gap < gap_best) {
> >> + best_gap_idx = i;
> >> + gap_best = gap;
> >> + }
> >
> > Could you use v4l2_find_nearest_size() instead?
> >
> > Also see below...
> >
> >> + }
> >> +
> >> + /*
> >> + * Use context a/b default timing values instead of calculate blanking
> >> + * timing values.
> >> + */
> >> + mode = _mode_data[best_gap_idx];
> >> + mt9m111->ctx = (best_gap_idx == MT9M111_MODE_QSXGA_30FPS) ? _a :
> >> + _b;
> >> +
> >> + /*
> >> + * Check if current settings support the fps because fps selection is
> >> + * based on the row/col skipping mechanism which has some restriction.
> >> + */
> >> + if (sensor_rect->width != mode->sensor_w ||
> >> + sensor_rect->height != mode->sensor_h ||
> >> + width > mode->max_image_w ||
> >> + height > mode->max_image_h) {
> >> + /* reset sensor window size */
> >> + mt9m111->rect.left = MT9M111_MIN_DARK_COLS;
> >> + mt9m111->rect.top = MT9M111_MIN_DARK_ROWS;
> >> + mt9m111->rect.width = mode->sensor_w;
> >> + mt9m111->rect.height = mode->sensor_h;
> >> +
> >> + /* reset image size */
> >> + mt9m111->width = mode->max_image_w;
> >> + mt9m111->height = mode->max_image_h;
> >> +
> >> + dev_warn(mt9m111->subdev.dev,
> >> + "Warning: update image size %dx%d[%dx%d] ->
> >> %dx%d[%dx%d]\n",
> >> + sensor_rect->width, sensor_rect->height, width, height,
> >> + mode->sensor_w, mode->sensor_h, mode->max_image_w,
> >> + mode->max_image_h);
> >
> > I wouldn't expect requesting a particular frame rate to change the sensor
> > format. The other way around is definitely fine though.
> >
> > Cc Hans.
>
> I agree with Sakari. Changing the framerate should never change the format.
> When you enumerate framerates those framerates are the allowed framerates
> for the mediabus format and the resolution. So changing the framerate should
> never modify the format or resolution. Instead, the framerate should be
> mapped to a framerate that is valid for the format/resolution combo.
I don't think this is actually documented, at least not for the sub-device
API. I can send a patch.
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v2 4/6] media: mt9m111: allow to setup pixclk polarity
Hi Marco, Enrico,
On Mon, Oct 29, 2018 at 07:24:08PM +0100, Marco Felsch wrote:
> From: Enrico Scholz
>
> The chip can be configured to output data transitions on the
> rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> falling edge.
>
> Parsing the fw-node is made in a subfunction to bundle all (future)
> dt-parsing / fw-parsing stuff.
>
> Signed-off-by: Enrico Scholz
> (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> per default. Set bit to 0 (enable mask bit without value) to enable
> falling edge sampling.)
> Signed-off-by: Michael Grzeschik
> (m.fel...@pengutronix.de: use fwnode helpers)
> (m.fel...@pengutronix.de: mv fw parsing into own function)
> (m.fel...@pengutronix.de: adapt commit msg)
> Signed-off-by: Marco Felsch
>
> ---
> Changelog:
>
> v2:
> - make use of fwnode_*() to drop OF dependency and ifdef's
> - mt9m111_g_mbus_config: fix pclk_sample logic which I made due the
> conversion from Enrico's patch.
>
> drivers/media/i2c/mt9m111.c | 46 -
> 1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index e9879e111f58..112eaa5ba917 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -15,12 +15,14 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> #include
> #include
> #include
> +#include
>
> /*
> * MT9M111, MT9M112 and MT9M131:
> @@ -239,6 +241,8 @@ struct mt9m111 {
> const struct mt9m111_datafmt *fmt;
> int lastpage; /* PageMap cache value */
> bool is_streaming;
> + /* user point of view - 0: falling 1: rising edge */
> + unsigned int pclk_sample:1;
You could use a bool. Up to you.
> #ifdef CONFIG_MEDIA_CONTROLLER
> struct media_pad pad;
> #endif
> @@ -591,6 +595,10 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
> return -EINVAL;
> }
>
> + /* receiver samples on falling edge, chip-hw default is rising */
> + if (mt9m111->pclk_sample == 0)
> + mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
> +
> ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> data_outfmt2, mask_outfmt2);
> if (!ret)
> @@ -1051,9 +1059,15 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd,
> int enable)
> static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
> struct v4l2_mbus_config *cfg)
> {
> - cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
> + struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> +
> + cfg->flags = V4L2_MBUS_MASTER |
> V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
> V4L2_MBUS_DATA_ACTIVE_HIGH;
> +
> + cfg->flags |= mt9m111->pclk_sample ? V4L2_MBUS_PCLK_SAMPLE_RISING :
> + V4L2_MBUS_PCLK_SAMPLE_FALLING;
> +
> cfg->type = V4L2_MBUS_PARALLEL;
>
> return 0;
> @@ -1123,6 +1137,32 @@ static int mt9m111_video_probe(struct i2c_client
> *client)
> return ret;
> }
>
> +static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111
> *mt9m111)
> +{
> + struct v4l2_fwnode_endpoint *bus_cfg;
> + struct fwnode_handle *np;
> + int ret = 0;
> +
> + np = fwnode_graph_get_next_endpoint(dev_fwnode(>dev), NULL);
> + if (!np)
> + return -EINVAL;
> +
> + bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> + if (IS_ERR(bus_cfg)) {
> + ret = PTR_ERR(bus_cfg);
> + goto out_put_fw;
> + }
> +
> + mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> + V4L2_MBUS_PCLK_SAMPLE_RISING);
> +
> + v4l2_fwnode_endpoint_free(bus_cfg);
> +
> +out_put_fw:
> + fwnode_handle_put(np);
> + return ret;
> +}
> +
> static int mt9m111_probe(struct i2c_client *client,
>const struct i2c_device_id *did)
> {
> @@ -1147,6 +1187,10 @@ static int mt9m111_probe(struct i2c_client *client,
> /* Default HIGHPOWER context */
> mt9m111->ctx = _b;
>
> + ret = mt9m111_probe_fw(client, mt9m111);
> + if (ret)
> + return ret;
Can you do this before v4l2_clk_get()? That'll go anyway, but for now,
you'd need extra error handling for it.
> +
> v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
> mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>
Please also put this patch after the DT binding changes.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v2 3/6] media: mt9m111: add support to select formats and fps for {Q,SXGA}
best_gap_idx = i;
> + gap_best = gap;
> + }
Could you use v4l2_find_nearest_size() instead?
Also see below...
> + }
> +
> + /*
> + * Use context a/b default timing values instead of calculate blanking
> + * timing values.
> + */
> + mode = _mode_data[best_gap_idx];
> + mt9m111->ctx = (best_gap_idx == MT9M111_MODE_QSXGA_30FPS) ? _a :
> + _b;
> +
> + /*
> + * Check if current settings support the fps because fps selection is
> + * based on the row/col skipping mechanism which has some restriction.
> + */
> + if (sensor_rect->width != mode->sensor_w ||
> + sensor_rect->height != mode->sensor_h ||
> + width > mode->max_image_w ||
> + height > mode->max_image_h) {
> + /* reset sensor window size */
> + mt9m111->rect.left = MT9M111_MIN_DARK_COLS;
> + mt9m111->rect.top = MT9M111_MIN_DARK_ROWS;
> + mt9m111->rect.width = mode->sensor_w;
> + mt9m111->rect.height = mode->sensor_h;
> +
> + /* reset image size */
> + mt9m111->width = mode->max_image_w;
> + mt9m111->height = mode->max_image_h;
> +
> + dev_warn(mt9m111->subdev.dev,
> + "Warning: update image size %dx%d[%dx%d] ->
> %dx%d[%dx%d]\n",
> + sensor_rect->width, sensor_rect->height, width, height,
> + mode->sensor_w, mode->sensor_h, mode->max_image_w,
> + mode->max_image_h);
I wouldn't expect requesting a particular frame rate to change the sensor
format. The other way around is definitely fine though.
Cc Hans.
> + }
> +
> + return mode;
> +}
> +
> #ifdef CONFIG_VIDEO_ADV_DEBUG
> static int mt9m111_g_register(struct v4l2_subdev *sd,
> struct v4l2_dbg_register *reg)
> @@ -775,11 +883,16 @@ static int mt9m111_suspend(struct mt9m111 *mt9m111)
>
> static void mt9m111_restore_state(struct mt9m111 *mt9m111)
> {
> + struct i2c_client *client = v4l2_get_subdevdata(>subdev);
> +
> mt9m111_set_context(mt9m111, mt9m111->ctx);
> mt9m111_set_pixfmt(mt9m111, mt9m111->fmt->code);
> mt9m111_setup_geometry(mt9m111, >rect,
> mt9m111->width, mt9m111->height, mt9m111->fmt->code);
> v4l2_ctrl_handler_setup(>hdl);
> + mt9m111_reg_mask(client, mt9m111->ctx->read_mode,
> + mt9m111->current_mode->reg_val,
> + mt9m111->current_mode->reg_mask);
> }
>
> static int mt9m111_resume(struct mt9m111 *mt9m111)
> @@ -872,6 +985,50 @@ static const struct v4l2_subdev_core_ops
> mt9m111_subdev_core_ops = {
> #endif
> };
>
> +static int mt9m111_g_frame_interval(struct v4l2_subdev *sd,
> +struct v4l2_subdev_frame_interval *fi)
> +{
> + struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> +
> + fi->interval = mt9m111->frame_interval;
> +
> + return 0;
> +}
> +
> +static int mt9m111_s_frame_interval(struct v4l2_subdev *sd,
> +struct v4l2_subdev_frame_interval *fi)
> +{
> + struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> + const struct mt9m111_mode_info *mode;
> + struct v4l2_fract *fract = >interval;
> + int fps;
> +
> + if (mt9m111->is_streaming)
> + return -EBUSY;
> +
> + if (fi->pad != 0)
> + return -EINVAL;
> +
> + if (fract->numerator == 0) {
> + fract->denominator = 30;
> + fract->numerator = 1;
> + }
> +
> + fps = DIV_ROUND_CLOSEST(fract->denominator, fract->numerator);
> +
> + /* find best fitting mode */
> + mode = mt9m111_find_mode(mt9m111, fps, mt9m111->width, mt9m111->height);
> + if (mode->max_fps != fps) {
> + fract->denominator = mode->max_fps;
> + fract->numerator = 1;
> + }
> +
> + mt9m111->current_mode = mode;
> + mt9m111->frame_interval = fi->interval;
> +
> + return 0;
> +}
> +
> static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
> struct v4l2_subdev_pad_config *cfg,
> struct v4l2_subdev_mbus_code_enum *code)
> @@ -905,6 +1062,8 @@ static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
> static const struct v4l2_subdev_video_ops mt9m111_subdev_video_ops = {
> .g_mbus_config = mt9m111_g_mbus_config,
> .s_stream = mt9m111_s_stream,
> + .g_frame_interval = mt9m111_g_frame_interval,
> + .s_frame_interval = mt9m111_s_frame_interval,
> };
>
> static const struct v4l2_subdev_pad_ops mt9m111_subdev_pad_ops = {
> @@ -1021,6 +1180,10 @@ static int mt9m111_probe(struct i2c_client *client,
> goto out_hdlfree;
> #endif
>
> + mt9m111->current_mode = _mode_data[MT9M111_MODE_SXGA_15FPS];
> + mt9m111->frame_interval.numerator = 1;
> + mt9m111->frame_interval.denominator = mt9m111->current_mode->max_fps;
> +
> /* Second stage probe - when a capture adapter is there */
> mt9m111->rect.left = MT9M111_MIN_DARK_COLS;
> mt9m111->rect.top = MT9M111_MIN_DARK_ROWS;
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH 4/4] media: ov5640: Add additional media bus formats
Hi Sam,
On Mon, Oct 08, 2018 at 11:48:02PM -0700, Sam Bobrowicz wrote:
> Add support for 1X16 yuv media bus formats (v4l2_mbus_framefmt).
> These formats are equivalent to the 2X8 formats that are already
> supported, both of which accurately describe the data present on
> the CSI2 interface. This change will increase compatibility with
> CSI2 RX drivers that only advertise support for the 1X16 formats.
>
> Signed-off-by: Sam Bobrowicz
> ---
> drivers/media/i2c/ov5640.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index a50d884..ca9de56 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -125,6 +125,8 @@ static const struct ov5640_pixfmt ov5640_formats[] = {
> { MEDIA_BUS_FMT_JPEG_1X8, V4L2_COLORSPACE_JPEG, },
> { MEDIA_BUS_FMT_UYVY8_2X8, V4L2_COLORSPACE_SRGB, },
> { MEDIA_BUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_SRGB, },
> + { MEDIA_BUS_FMT_UYVY8_1X16, V4L2_COLORSPACE_SRGB, },
> + { MEDIA_BUS_FMT_YUYV8_1X16, V4L2_COLORSPACE_SRGB, },
> { MEDIA_BUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB, },
> { MEDIA_BUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB, },
> };
Can you make these selectable only for the CSI-2 bus? I understand the
driver also supports the parallel interface.
> @@ -2069,10 +2071,12 @@ static int ov5640_set_framefmt(struct ov5640_dev
> *sensor,
>
> switch (format->code) {
> case MEDIA_BUS_FMT_UYVY8_2X8:
> + case MEDIA_BUS_FMT_UYVY8_1X16:
> /* YUV422, UYVY */
> val = 0x3f;
> break;
> case MEDIA_BUS_FMT_YUYV8_2X8:
> + case MEDIA_BUS_FMT_YUYV8_1X16:
> /* YUV422, YUYV */
> val = 0x30;
> break;
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: media: ov8856: Add support for OV8856 sensor
Hi Ben, On Thu, Nov 08, 2018 at 11:41:46AM +0800, Ben Kao wrote: > This patch adds driver for Omnivision's ov8856 sensor, > the driver supports following features: > > - manual exposure/gain(analog and digital) control support > - two link frequencies > - VBLANK/HBLANK support > - test pattern support > - media controller support > - runtime pm support > - supported resolutions > + 3280x2464 at 30FPS > + 1640x1232 at 30FPS > > Signed-off-by: Ben Kao I just realised the driver is missing the MAINTAINERS entry. Could you provide one? Just the diff is fine, I'll then squash it to the patch. Thanks. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v3] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers
On Thu, Nov 15, 2018 at 11:29:35AM +0100, Hans Verkuil wrote: > On 11/14/18 20:59, Laurent Pinchart wrote: > > Hi Philipp, > > > > Thank you for the patch. > > > > On Wednesday, 14 November 2018 17:04:49 EET Philipp Zabel wrote: > >> From: John Sheu > >> > >> Videobuf2 presently does not allow VIDIOC_REQBUFS to destroy outstanding > >> buffers if the queue is of type V4L2_MEMORY_MMAP, and if the buffers are > >> considered "in use". This is different behavior than for other memory > >> types and prevents us from deallocating buffers in following two cases: > >> > >> 1) There are outstanding mmap()ed views on the buffer. However even if > >>we put the buffer in reqbufs(0), there will be remaining references, > >>due to vma .open/close() adjusting vb2 buffer refcount appropriately. > >>This means that the buffer will be in fact freed only when the last > >>mmap()ed view is unmapped. > > > > While I agree that we should remove this restriction, it has helped me in > > the > > past to find missing munmap() in userspace. This patch thus has the > > potential > > of causing memory leaks in userspace. Is there a way we could assist > > application developers with this ? > > Should we just keep the debug message? (rephrased of course) > > That way you can enable debugging and see that this happens. > > It sounds reasonable to me. Makes sense IMO. -- Sakari Ailus e-mail: sakari.ai...@iki.fi
[PATCH v2 for v4.9 1/1] v4l: event: Add subscription to list before calling "add" operation
[ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
Patch ad608fbcf166 changed how events were subscribed to address an issue
elsewhere. As a side effect of that change, the "add" callback was called
before the event subscription was added to the list of subscribed events,
causing the first event queued by the add callback (and possibly other
events arriving soon afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add"
callback, and clean up afterwards if that fails.
Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions
while accessed")
Reported-by: Dave Stevenson
Signed-off-by: Sakari Ailus
Tested-by: Dave Stevenson
Reviewed-by: Hans Verkuil
Tested-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
[Sakari Ailus: Backported to v4.9 stable]
Signed-off-by: Sakari Ailus
---
since v1 (as requested by Sasha):
- Add my final SoB
- Indicate specifically this is a backport
- Remove the extra cc stable
drivers/media/v4l2-core/v4l2-event.c | 43
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index 567d86835f001..1fda2873375f6 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -197,6 +197,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_event_pending);
+static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
+{
+ struct v4l2_fh *fh = sev->fh;
+ unsigned int i;
+
+ lockdep_assert_held(>subscribe_lock);
+ assert_spin_locked(>vdev->fh_lock);
+
+ /* Remove any pending events for this subscription */
+ for (i = 0; i < sev->in_use; i++) {
+ list_del(>events[sev_pos(sev, i)].list);
+ fh->navailable--;
+ }
+ list_del(>list);
+}
+
int v4l2_event_subscribe(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub, unsigned
elems,
const struct v4l2_subscribed_event_ops *ops)
@@ -228,27 +244,23 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
+ if (!found_ev)
+ list_add(>list, >subscribed);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
if (found_ev) {
/* Already listening */
kfree(sev);
- goto out_unlock;
- }
-
- if (sev->ops && sev->ops->add) {
+ } else if (sev->ops && sev->ops->add) {
ret = sev->ops->add(sev, elems);
if (ret) {
+ spin_lock_irqsave(>vdev->fh_lock, flags);
+ __v4l2_event_unsubscribe(sev);
+ spin_unlock_irqrestore(>vdev->fh_lock, flags);
kfree(sev);
- goto out_unlock;
}
}
- spin_lock_irqsave(>vdev->fh_lock, flags);
- list_add(>list, >subscribed);
- spin_unlock_irqrestore(>vdev->fh_lock, flags);
-
-out_unlock:
mutex_unlock(>subscribe_lock);
return ret;
@@ -283,7 +295,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
{
struct v4l2_subscribed_event *sev;
unsigned long flags;
- int i;
if (sub->type == V4L2_EVENT_ALL) {
v4l2_event_unsubscribe_all(fh);
@@ -295,14 +306,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (sev != NULL) {
- /* Remove any pending events for this subscription */
- for (i = 0; i < sev->in_use; i++) {
- list_del(>events[sev_pos(sev, i)].list);
- fh->navailable--;
- }
- list_del(>list);
- }
+ if (sev != NULL)
+ __v4l2_event_unsubscribe(sev);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
--
2.11.0
[PATCH v2 for v4.4 1/1] v4l: event: Add subscription to list before calling "add" operation
[ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
Patch ad608fbcf166 changed how events were subscribed to address an issue
elsewhere. As a side effect of that change, the "add" callback was called
before the event subscription was added to the list of subscribed events,
causing the first event queued by the add callback (and possibly other
events arriving soon afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add"
callback, and clean up afterwards if that fails.
Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions
while accessed")
Reported-by: Dave Stevenson
Signed-off-by: Sakari Ailus
Tested-by: Dave Stevenson
Reviewed-by: Hans Verkuil
Tested-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
[Sakari Ailus: Backported to v4.4 stable]
Signed-off-by: Sakari Ailus
---
since v1 (as requested by Sasha):
- Add my final SoB
- Indicate specifically this is a backport
- Remove the extra cc stable
drivers/media/v4l2-core/v4l2-event.c | 43
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index b47ac4e053d0e..f5c8a952f0aa3 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -197,6 +197,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_event_pending);
+static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
+{
+ struct v4l2_fh *fh = sev->fh;
+ unsigned int i;
+
+ lockdep_assert_held(>subscribe_lock);
+ assert_spin_locked(>vdev->fh_lock);
+
+ /* Remove any pending events for this subscription */
+ for (i = 0; i < sev->in_use; i++) {
+ list_del(>events[sev_pos(sev, i)].list);
+ fh->navailable--;
+ }
+ list_del(>list);
+}
+
int v4l2_event_subscribe(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub, unsigned
elems,
const struct v4l2_subscribed_event_ops *ops)
@@ -228,27 +244,23 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
+ if (!found_ev)
+ list_add(>list, >subscribed);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
if (found_ev) {
/* Already listening */
kfree(sev);
- goto out_unlock;
- }
-
- if (sev->ops && sev->ops->add) {
+ } else if (sev->ops && sev->ops->add) {
ret = sev->ops->add(sev, elems);
if (ret) {
+ spin_lock_irqsave(>vdev->fh_lock, flags);
+ __v4l2_event_unsubscribe(sev);
+ spin_unlock_irqrestore(>vdev->fh_lock, flags);
kfree(sev);
- goto out_unlock;
}
}
- spin_lock_irqsave(>vdev->fh_lock, flags);
- list_add(>list, >subscribed);
- spin_unlock_irqrestore(>vdev->fh_lock, flags);
-
-out_unlock:
mutex_unlock(>subscribe_lock);
return ret;
@@ -283,7 +295,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
{
struct v4l2_subscribed_event *sev;
unsigned long flags;
- int i;
if (sub->type == V4L2_EVENT_ALL) {
v4l2_event_unsubscribe_all(fh);
@@ -295,14 +306,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (sev != NULL) {
- /* Remove any pending events for this subscription */
- for (i = 0; i < sev->in_use; i++) {
- list_del(>events[sev_pos(sev, i)].list);
- fh->navailable--;
- }
- list_del(>list);
- }
+ if (sev != NULL)
+ __v4l2_event_unsubscribe(sev);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
--
2.11.0
Re: [PATCH v4.9 1/1] v4l: event: Add subscription to list before calling "add" operation
Hi Sasha,
On Thu, Nov 08, 2018 at 12:28:53PM -0500, Sasha Levin wrote:
> On Thu, Nov 08, 2018 at 01:46:06PM +0200, Sakari Ailus wrote:
> > [ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
> >
> > Patch ad608fbcf166 changed how events were subscribed to address an issue
> > elsewhere. As a side effect of that change, the "add" callback was called
> > before the event subscription was added to the list of subscribed events,
> > causing the first event queued by the add callback (and possibly other
> > events arriving soon afterwards) to be lost.
> >
> > Fix this by adding the subscription to the list before calling the "add"
> > callback, and clean up afterwards if that fails.
> >
> > Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event
> > subscriptions while accessed")
> >
> > Reported-by: Dave Stevenson
> > Signed-off-by: Sakari Ailus
> > Tested-by: Dave Stevenson
> > Reviewed-by: Hans Verkuil
> > Tested-by: Hans Verkuil
> > Cc: sta...@vger.kernel.org (for 4.14 and up)
> > Signed-off-by: Mauro Carvalho Chehab
>
> Hi Sakari,
>
> For the sake of completeness, can you sign off on the backport too and
> indicate it was backported to 4.9 in the commit messge? Otherwise, this
> commit message says it's for 4.14+ and will suddenly appear in the 4.9
> tree, and if we have issues later it might cause confusion.
Yes; I'll fix the above issues and resend.
Thanks!
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
Hi Jacopo, On Wed, Nov 14, 2018 at 01:25:11AM +0100, jacopo mondi wrote: > On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote: > > Hi, > > > > This series adds support for the Intel IPU3 (Image Processing Unit) > > ImgU which is essentially a modern memory-to-memory ISP. It implements > > raw Bayer to YUV image format conversion as well as a large number of > > other pixel processing algorithms for improving the image quality. > > > > Meta data formats are defined for image statistics (3A, i.e. automatic > > white balance, exposure and focus, histogram and local area contrast > > enhancement) as well as for the pixel processing algorithm parameters. > > The documentation for these formats is currently not included in the > > patchset but will be added in a future version of this set. > > > > The algorithm parameters need to be considered specific to a given frame > > and typically a large number of these parameters change on frame to frame > > basis. Additionally, the parameters are highly structured (and not a flat > > space of independent configuration primitives). They also reflect the > > data structures used by the firmware and the hardware. On top of that, > > the algorithms require highly specialized user space to make meaningful > > use of them. For these reasons it has been chosen video buffers to pass > > the parameters to the device. > > > > On individual patches: > > > > The heart of ImgU is the CSS, or Camera Subsystem, which contains the > > image processors and HW accelerators. > > > > The 3A statistics and other firmware parameter computation related > > functions are implemented in patch 11. > > > > All IPU3 pipeline default settings can be found in patch 10. > > > > Seems to me that patch 10 didn't make it to the mailing list, am I > wrong? > > I'm pointing it out as the same happened on your v6. Thanks for pointing this out. I've uploaded the entire set here: https://git.linuxtv.org/sailus/media_tree.git/log/?h=ipu3-v7> including the 10th patch: https://git.linuxtv.org/sailus/media_tree.git/commit/?h=ipu3-v7=41e2f0d114dbc195efed079202d22748ddedbe83> It's too big to get through the list server. :-( Luckily, it's mostly tables so there's not much to review there. Default settings, effectively. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH 4/4] SoC camera: Tidy the header
Hi Hans, Mauro,
On Wed, Oct 31, 2018 at 07:54:21AM -0300, Mauro Carvalho Chehab wrote:
> Em Wed, 31 Oct 2018 11:00:22 +0100
> Hans Verkuil escreveu:
>
> > On 10/31/2018 10:40 AM, Mauro Carvalho Chehab wrote:
> > > Em Wed, 31 Oct 2018 11:29:45 +0200
> > > Sakari Ailus escreveu:
> > >
> > >> Hi Mauro,
> > >>
> > >> On Tue, Oct 30, 2018 at 09:06:18AM -0300, Mauro Carvalho Chehab wrote:
> > >>> Em Tue, 30 Oct 2018 01:00:29 +0200
> > >>> Sakari Ailus escreveu:
> > >>>
> > >>>> Clean up the SoC camera framework header. It only exists now to keep
> > >>>> board
> > >>>> code compiling. The header can be removed once the board code
> > >>>> dependencies
> > >>>> to it has been removed.
> > >>>>
> > >>>> Signed-off-by: Sakari Ailus
> > >>>> ---
> > >>>> include/media/soc_camera.h | 335
> > >>>> -
> > >>>> 1 file changed, 335 deletions(-)
> > >>>>
> > >>>> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> > >>>> index b7e42a1b0910..14d19da6052a 100644
> > >>>> --- a/include/media/soc_camera.h
> > >>>> +++ b/include/media/soc_camera.h
> > >>>> @@ -22,172 +22,6 @@
> > >>>> #include
> > >>>> #include
> > >>>
> > >>> That doesn't make any sense. soc_camera.h should have the same fate
> > >>> as the entire soc_camera infrastructure: either be removed or moved
> > >>> to staging, and everything else that doesn't have the same fate
> > >>> should get rid of this header.
> > >>
> > >> We can't just remove this; there is board code that depends on it.
> > >>
> > >> The intent is to remove the board code as well but presuming that the
> > >> board
> > >> code would be merged through a different tree, it'd be less hassle to
> > >> wait
> > >> a bit; hence the patch removing any unnecessary stuff here.
> > >
> > > Then we need *first* to remove board code, wait for those changes to be
> > > applied and *then* remove soc_camera (and not the opposite).
> >
> > Please note that the camera support for all the remaining boards using
> > soc_camera has been dead for years. The soc_camera drivers that they
> > depended
> > on have been removed a long time ago.
> >
> > So all they depend on are the header. We can safely remove soc_camera
> > without
> > loss of functionality (and thus prevent others from basing new drivers on
> > soc_camera), while we work to update the board files so we can remove this
> > last
> > header.
> >
> > I have modified some board files here:
> >
> > https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=rm-soc-camera=d7ae2fcf6e447022f0780bb86a2b63d5c7cf4d35
>
> Good! the arch-specific mach-* changes should likely be on separate
> patches, though.
>
> > Only arch/arm/mach-imx/mach-imx27_visstrim_m10.c hasn't been fixed yet in
> > that
> > patch (ENOTIME).
>
> So, the code we don't manage seems to be just 3 archs, right (mach-omap1,
> mach-pxa and mach-imx)?
>
> Btw, the soc_camera dependent code at mach-imx27_visstrim_m10.c is:
>
> static struct soc_camera_link iclink_tvp5150 = {
> .bus_id = 0,
> .board_info = _i2c_camera,
> .i2c_adapter_id = 0,
> .power = visstrim_camera_power,
> .reset = visstrim_camera_reset,
> };
>
> ...
> platform_device_register_resndata(NULL, "soc-camera-pdrv", 0, NULL, 0,
> _tvp5150,
> sizeof(iclink_tvp5150));
>
>
> As tvp5150 is not actually a soc_camera driver, I suspect that
> the conversion here would be to make it to use the tvp5150 driver
> directly, instead of doing it via soc_camera.
Using board files? The IMX27 SoC has plenty of dts files around which
suggests that it's just this particular board that has not been converted.
I don't think this is a good excuse to keep the board file stuff around.
If someone is willing to add dts files for the board, all the better. But
I'm not going to do that.
>
> > The problem is just lack of time to clean this up and figure out who should
> > take these board patches.
>
> No need to rush it.
>
Re: [PATCH] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers
Hi Philipp,
On Tue, Nov 13, 2018 at 04:06:21PM +0100, Philipp Zabel wrote:
> From: John Sheu
>
> Videobuf2 presently does not allow VIDIOC_REQBUFS to destroy outstanding
> buffers if the queue is of type V4L2_MEMORY_MMAP, and if the buffers are
> considered "in use". This is different behavior than for other memory
> types and prevents us from deallocating buffers in following two cases:
>
> 1) There are outstanding mmap()ed views on the buffer. However even if
>we put the buffer in reqbufs(0), there will be remaining references,
>due to vma .open/close() adjusting vb2 buffer refcount appropriately.
>This means that the buffer will be in fact freed only when the last
>mmap()ed view is unmapped.
>
> 2) Buffer has been exported as a DMABUF. Refcount of the vb2 buffer
>is managed properly by VB2 DMABUF ops, i.e. incremented on DMABUF
>get and decremented on DMABUF release. This means that the buffer
>will be alive until all importers release it.
>
> Considering both cases above, there does not seem to be any need to
> prevent reqbufs(0) operation, because buffer lifetime is already
> properly managed by both mmap() and DMABUF code paths. Let's remove it
> and allow userspace freeing the queue (and potentially allocating a new
> one) even though old buffers might be still in processing.
>
> To let userspace know that the kernel now supports orphaning buffers
> that are still in use, add a new V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS
> to be set by reqbufs and create_bufs.
>
> Signed-off-by: John Sheu
> Reviewed-by: Pawel Osciak
> Reviewed-by: Tomasz Figa
> Signed-off-by: Tomasz Figa
> [p.za...@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
> and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
> Signed-off-by: Philipp Zabel
This lets the user to allocate lots of mmap'ed buffers that are pinned in
physical memory. Considering that we don't really have a proper mechanism
to limit that anyway,
Acked-by: Sakari Ailus
That said, the patch must be accompanied by the documentation change in
Documentation/media/uapi/v4l/vidioc-reqbufs.rst .
I wonder what Hans thinks.
> ---
> .../media/common/videobuf2/videobuf2-core.c | 26 +--
> .../media/common/videobuf2/videobuf2-v4l2.c | 2 +-
> include/uapi/linux/videodev2.h| 1 +
> 3 files changed, 3 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index 975ff5669f72..608459450c1e 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -553,20 +553,6 @@ bool vb2_buffer_in_use(struct vb2_queue *q, struct
> vb2_buffer *vb)
> }
> EXPORT_SYMBOL(vb2_buffer_in_use);
>
> -/*
> - * __buffers_in_use() - return true if any buffers on the queue are in use
> and
> - * the queue cannot be freed (by the means of REQBUFS(0)) call
> - */
> -static bool __buffers_in_use(struct vb2_queue *q)
> -{
> - unsigned int buffer;
> - for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> - if (vb2_buffer_in_use(q, q->bufs[buffer]))
> - return true;
> - }
> - return false;
> -}
> -
> void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
> {
> call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
> @@ -674,23 +660,13 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
> vb2_memory memory,
>
> if (*count == 0 || q->num_buffers != 0 ||
> (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
> - /*
> - * We already have buffers allocated, so first check if they
> - * are not in use and can be freed.
> - */
> - mutex_lock(>mmap_lock);
> - if (q->memory == VB2_MEMORY_MMAP && __buffers_in_use(q)) {
> - mutex_unlock(>mmap_lock);
> - dprintk(1, "memory in use, cannot free\n");
> - return -EBUSY;
> - }
> -
> /*
>* Call queue_cancel to clean up any buffers in the
>* QUEUED state which is possible if buffers were prepared or
>* queued without ever calling STREAMON.
>*/
> __vb2_queue_cancel(q);
> + mutex_lock(>mmap_lock);
> ret = __vb2_queue_free(q, q->num_buffers);
> mutex_unlock(>mmap_lock);
> if (ret)
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> b/drivers/media/common/videob
Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
On Tue, Nov 13, 2018 at 07:04:01PM +0800, Bing Bu Cao wrote: > > > On 11/13/2018 06:31 PM, Sakari Ailus wrote: > > Hi Bing Bu, > > > > On Mon, Nov 12, 2018 at 12:31:16PM +0800, Bing Bu Cao wrote: > >> > >> On 11/09/2018 06:09 PM, Sakari Ailus wrote: > >>> Hi Bing Bu, > >>> > >>> On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote: > >>>> On 11/01/2018 08:03 PM, Sakari Ailus wrote: > >>>>> Hi Yong, > >>>>> > >>>>> Thanks for the update! > >>>>> > >>>>> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote: > >>>>>> Hi, > >>>>>> > >>>>>> This series adds support for the Intel IPU3 (Image Processing Unit) > >>>>>> ImgU which is essentially a modern memory-to-memory ISP. It implements > >>>>>> raw Bayer to YUV image format conversion as well as a large number of > >>>>>> other pixel processing algorithms for improving the image quality. > >>>>>> > >>>>>> Meta data formats are defined for image statistics (3A, i.e. automatic > >>>>>> white balance, exposure and focus, histogram and local area contrast > >>>>>> enhancement) as well as for the pixel processing algorithm parameters. > >>>>>> The documentation for these formats is currently not included in the > >>>>>> patchset but will be added in a future version of this set. > >>>>>> > >>>>>> The algorithm parameters need to be considered specific to a given > >>>>>> frame > >>>>>> and typically a large number of these parameters change on frame to > >>>>>> frame > >>>>>> basis. Additionally, the parameters are highly structured (and not a > >>>>>> flat > >>>>>> space of independent configuration primitives). They also reflect the > >>>>>> data structures used by the firmware and the hardware. On top of that, > >>>>>> the algorithms require highly specialized user space to make meaningful > >>>>>> use of them. For these reasons it has been chosen video buffers to pass > >>>>>> the parameters to the device. > >>>>>> > >>>>>> On individual patches: > >>>>>> > >>>>>> The heart of ImgU is the CSS, or Camera Subsystem, which contains the > >>>>>> image processors and HW accelerators. > >>>>>> > >>>>>> The 3A statistics and other firmware parameter computation related > >>>>>> functions are implemented in patch 11. > >>>>>> > >>>>>> All IPU3 pipeline default settings can be found in patch 10. > >>>>>> > >>>>>> To access DDR via ImgU's own memory space, IPU3 is also equipped with > >>>>>> its own MMU unit, the driver is implemented in patch 6. > >>>>>> > >>>>>> Patch 7 uses above driver for DMA mapping operation. > >>>>>> > >>>>>> The communication between IPU3 firmware and driver is implemented with > >>>>>> circular > >>>>>> queues in patch 8. > >>>>>> > >>>>>> Patch 9 provide some utility functions and manage IPU3 fw download and > >>>>>> install. > >>>>>> > >>>>>> The firmware which is called ipu3-fw.bin can be downloaded from: > >>>>>> > >>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git > >>>>>> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f) > >>>>>> > >>>>>> Firmware ABI is defined in patches 4 and 5. > >>>>>> > >>>>>> Patches 12 and 13 are of the same file, the former contains all h/w > >>>>>> programming > >>>>>> related code, the latter implements interface functions for access fw > >>>>>> & hw > >>>>>> capabilities. > >>>>>> > >>>>>> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work: > >>>>>> > >>>>>> https://patchwork.kernel.org/patch/9976295/> > >>>>> I've pushed the latest
Re: [PATCH 3/7] media: ov2640: add V4L2_CID_TEST_PATTERN control
On Tue, Nov 13, 2018 at 10:55:46PM +0900, Akinobu Mita wrote:
> 2018年11月13日(火) 19:37 Sakari Ailus :
> >
> > On Tue, Nov 13, 2018 at 01:00:50AM +0900, Akinobu Mita wrote:
> > > The ov2640 has the test pattern generator features. This makes use of
> > > it through V4L2_CID_TEST_PATTERN control.
> > >
> > > Cc: Sakari Ailus
> > > Cc: Mauro Carvalho Chehab
> > > Signed-off-by: Akinobu Mita
> > > ---
> > > drivers/media/i2c/ov2640.c | 14 +-
> > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> > > index 20a8853..4992d77 100644
> > > --- a/drivers/media/i2c/ov2640.c
> > > +++ b/drivers/media/i2c/ov2640.c
> > > @@ -705,6 +705,11 @@ static int ov2640_reset(struct i2c_client *client)
> > > return ret;
> > > }
> > >
> > > +static const char * const ov2640_test_pattern_menu[] = {
> > > + "Disabled",
> > > + "Color bar",
> >
> > s/b/B/
> >
> > The smiapp driver uses "Eight Vertical Colour Bars", not sure if that'd fit
> > here or not. FYI.
>
> This test pattern shows eight vertical color bars with blending actual
> sensor image, although the datasheet tells just 'Color bar'.
>
> So either is fine for me.
I'll use what the smiapp driver does.
Thanks.
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH 3/7] media: ov2640: add V4L2_CID_TEST_PATTERN control
On Tue, Nov 13, 2018 at 01:00:50AM +0900, Akinobu Mita wrote:
> The ov2640 has the test pattern generator features. This makes use of
> it through V4L2_CID_TEST_PATTERN control.
>
> Cc: Sakari Ailus
> Cc: Mauro Carvalho Chehab
> Signed-off-by: Akinobu Mita
> ---
> drivers/media/i2c/ov2640.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> index 20a8853..4992d77 100644
> --- a/drivers/media/i2c/ov2640.c
> +++ b/drivers/media/i2c/ov2640.c
> @@ -705,6 +705,11 @@ static int ov2640_reset(struct i2c_client *client)
> return ret;
> }
>
> +static const char * const ov2640_test_pattern_menu[] = {
> + "Disabled",
> + "Color bar",
s/b/B/
The smiapp driver uses "Eight Vertical Colour Bars", not sure if that'd fit
here or not. FYI.
> +};
> +
> /*
> * functions
> */
> @@ -740,6 +745,9 @@ static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl)
> case V4L2_CID_HFLIP:
> val = ctrl->val ? REG04_HFLIP_IMG : 0x00;
> return ov2640_mask_set(client, REG04, REG04_HFLIP_IMG, val);
> + case V4L2_CID_TEST_PATTERN:
> + val = ctrl->val ? COM7_COLOR_BAR_TEST : 0x00;
> + return ov2640_mask_set(client, COM7, COM7_COLOR_BAR_TEST, val);
> }
>
> return -EINVAL;
> @@ -1184,12 +1192,16 @@ static int ov2640_probe(struct i2c_client *client,
> v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
> priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> mutex_init(>lock);
> - v4l2_ctrl_handler_init(>hdl, 2);
> + v4l2_ctrl_handler_init(>hdl, 3);
> priv->hdl.lock = >lock;
> v4l2_ctrl_new_std(>hdl, _ctrl_ops,
> V4L2_CID_VFLIP, 0, 1, 1, 0);
> v4l2_ctrl_new_std(>hdl, _ctrl_ops,
> V4L2_CID_HFLIP, 0, 1, 1, 0);
> + v4l2_ctrl_new_std_menu_items(>hdl, _ctrl_ops,
> + V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(ov2640_test_pattern_menu) - 1, 0, 0,
> + ov2640_test_pattern_menu);
> priv->subdev.ctrl_handler = >hdl;
> if (priv->hdl.error) {
> ret = priv->hdl.error;
> --
> 2.7.4
>
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
Hi Bing Bu, On Mon, Nov 12, 2018 at 12:31:16PM +0800, Bing Bu Cao wrote: > > > On 11/09/2018 06:09 PM, Sakari Ailus wrote: > > Hi Bing Bu, > > > > On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote: > >> On 11/01/2018 08:03 PM, Sakari Ailus wrote: > >>> Hi Yong, > >>> > >>> Thanks for the update! > >>> > >>> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote: > >>>> Hi, > >>>> > >>>> This series adds support for the Intel IPU3 (Image Processing Unit) > >>>> ImgU which is essentially a modern memory-to-memory ISP. It implements > >>>> raw Bayer to YUV image format conversion as well as a large number of > >>>> other pixel processing algorithms for improving the image quality. > >>>> > >>>> Meta data formats are defined for image statistics (3A, i.e. automatic > >>>> white balance, exposure and focus, histogram and local area contrast > >>>> enhancement) as well as for the pixel processing algorithm parameters. > >>>> The documentation for these formats is currently not included in the > >>>> patchset but will be added in a future version of this set. > >>>> > >>>> The algorithm parameters need to be considered specific to a given frame > >>>> and typically a large number of these parameters change on frame to frame > >>>> basis. Additionally, the parameters are highly structured (and not a flat > >>>> space of independent configuration primitives). They also reflect the > >>>> data structures used by the firmware and the hardware. On top of that, > >>>> the algorithms require highly specialized user space to make meaningful > >>>> use of them. For these reasons it has been chosen video buffers to pass > >>>> the parameters to the device. > >>>> > >>>> On individual patches: > >>>> > >>>> The heart of ImgU is the CSS, or Camera Subsystem, which contains the > >>>> image processors and HW accelerators. > >>>> > >>>> The 3A statistics and other firmware parameter computation related > >>>> functions are implemented in patch 11. > >>>> > >>>> All IPU3 pipeline default settings can be found in patch 10. > >>>> > >>>> To access DDR via ImgU's own memory space, IPU3 is also equipped with > >>>> its own MMU unit, the driver is implemented in patch 6. > >>>> > >>>> Patch 7 uses above driver for DMA mapping operation. > >>>> > >>>> The communication between IPU3 firmware and driver is implemented with > >>>> circular > >>>> queues in patch 8. > >>>> > >>>> Patch 9 provide some utility functions and manage IPU3 fw download and > >>>> install. > >>>> > >>>> The firmware which is called ipu3-fw.bin can be downloaded from: > >>>> > >>>> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git > >>>> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f) > >>>> > >>>> Firmware ABI is defined in patches 4 and 5. > >>>> > >>>> Patches 12 and 13 are of the same file, the former contains all h/w > >>>> programming > >>>> related code, the latter implements interface functions for access fw & > >>>> hw > >>>> capabilities. > >>>> > >>>> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work: > >>>> > >>>> https://patchwork.kernel.org/patch/9976295/> > >>> I've pushed the latest set here: > >>> > >>> https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output> > >>> > >>> You can just say the entire set depends on those going forward; the > >>> documentation is needed, too. > >>> > >>>> Patch 15 represents the top level that glues all of the other components > >>>> together, > >>>> passing arguments between the components. > >>>> > >>>> Patch 16 is a recent effort to extend v6 for advanced camera features > >>>> like > >>>> Continuous View Finder (CVF) and Snapshot During Video(SDV) support. > >>>> > >>>> Link to user space implem
Re: [PATCH v7 08/16] intel-ipu3: css: Add dma buff pool utility functions
Hi Yong,
On Fri, Nov 09, 2018 at 11:16:44PM +, Zhi, Yong wrote:
> Hi, Sakari,
>
> > -Original Message-
> > From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> > Sent: Thursday, November 8, 2018 9:36 AM
> > To: Zhi, Yong
> > Cc: linux-media@vger.kernel.org; tf...@chromium.org;
> > mche...@kernel.org; hans.verk...@cisco.com;
> > laurent.pinch...@ideasonboard.com; Mani, Rajmohan
> > ; Zheng, Jian Xu ; Hu,
> > Jerry W ; Toivonen, Tuukka
> > ; Qiu, Tian Shu ; Cao,
> > Bingbu
> > Subject: Re: [PATCH v7 08/16] intel-ipu3: css: Add dma buff pool utility
> > functions
> >
> > Hi Yong,
> >
> > On Mon, Oct 29, 2018 at 03:23:02PM -0700, Yong Zhi wrote:
> > > The pools are used to store previous parameters set by user with the
> > > parameter queue. Due to pipelining, there needs to be multiple sets
> > > (up to four) of parameters which are queued in a host-to-sp queue.
> > >
> > > Signed-off-by: Yong Zhi
> > > ---
> > > drivers/media/pci/intel/ipu3/ipu3-css-pool.c | 136
> > > +++
> > > drivers/media/pci/intel/ipu3/ipu3-css-pool.h | 56 +++
> > > 2 files changed, 192 insertions(+)
> > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> > >
> > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > > b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > > new file mode 100644
> > > index 000..eab41c3
> > > --- /dev/null
> > > +++ b/drivers/media/pci/intel/ipu3/ipu3-css-pool.c
> > > @@ -0,0 +1,136 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +// Copyright (C) 2018 Intel Corporation
> > > +
> > > +#include
> > > +
> > > +#include "ipu3.h"
> > > +#include "ipu3-css-pool.h"
> > > +#include "ipu3-dmamap.h"
> > > +
> > > +int ipu3_css_dma_buffer_resize(struct imgu_device *imgu,
> > > +struct ipu3_css_map *map, size_t size) {
> > > + if (map->size < size && map->vaddr) {
> > > + dev_warn(>pci_dev->dev, "dma buf resized from %zu
> > to %zu",
> > > + map->size, size);
> > > +
> > > + ipu3_dmamap_free(imgu, map);
> > > + if (!ipu3_dmamap_alloc(imgu, map, size))
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +void ipu3_css_pool_cleanup(struct imgu_device *imgu, struct
> > > +ipu3_css_pool *pool) {
> > > + unsigned int i;
> > > +
> > > + for (i = 0; i < IPU3_CSS_POOL_SIZE; i++)
> > > + ipu3_dmamap_free(imgu, >entry[i].param); }
> > > +
> > > +int ipu3_css_pool_init(struct imgu_device *imgu, struct ipu3_css_pool
> > *pool,
> > > +size_t size)
> > > +{
> > > + unsigned int i;
> > > +
> > > + for (i = 0; i < IPU3_CSS_POOL_SIZE; i++) {
> > > + /*
> > > + * entry[i].framenum is initialized to INT_MIN so that
> > > + * ipu3_css_pool_check() can treat it as usesable slot.
> > > + */
> > > + pool->entry[i].framenum = INT_MIN;
> > > +
> > > + if (size == 0) {
> > > + pool->entry[i].param.vaddr = NULL;
> > > + continue;
> > > + }
> > > +
> > > + if (!ipu3_dmamap_alloc(imgu, >entry[i].param, size))
> > > + goto fail;
> > > + }
> > > +
> > > + pool->last = IPU3_CSS_POOL_SIZE;
> > > +
> > > + return 0;
> > > +
> > > +fail:
> > > + ipu3_css_pool_cleanup(imgu, pool);
> > > + return -ENOMEM;
> > > +}
> > > +
> > > +/*
> > > + * Check that the following call to pool_get succeeds.
> > > + * Return negative on error.
> > > + */
> > > +static int ipu3_css_pool_check(struct ipu3_css_pool *pool, long
> > > +framenum) {
> > > + /* Get the oldest entry */
> > > + int n = (pool->last + 1) % IPU3_CSS_POOL_SIZE;
> > > + long diff = framenum - pool->entry[n].framenum;
> > > +
> > > + /* if framenum wraps around and becomes smaller than entry n */
> > > + if (diff < 0)
> > > + diff += LONG_MAX;
> >
> > Have you tested the wrap-around? As a result, the value of the diff is
> > between -1 and LONG_MAX - 1 (without considering more than just the two
> > lines above). Is that intended?
> >
>
> Yes, I simulated wrap-around using a smaller limit in v5.
>
> > You seem to be using different types for the frame number; sometimes int,
> > sometimes long. Could you align that, preferrably to an unsigned type? u32
> > would probably be a sound choice.
> >
>
> Will use u32 at places except entry.framenum, which is initialized to
> INT_MIN. This is because the frame is counted from 0 at stream start, and
> the entry.framenum must be smaller enough for the ipu3_css_pool_check()
> to not return -ENOSPC.
You could use another field to tell whether an entry is valid or not.
That'd simplify the code, as well as remove the need to cap the frame
number to an arbitrary value.
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v7 15/16] intel-ipu3: Add imgu top level pci device driver
s a buffer available
> in
> + * master queue, the driver queues a buffer also to all other active nodes.
> + * If user space hasn't provided a buffer to all other video nodes first,
> + * the driver gets an internal dummy buffer and queues it.
> + */
> +#define IMGU_QUEUE_MASTERIPU3_CSS_QUEUE_IN
> +#define IMGU_QUEUE_FIRST_INPUT IPU3_CSS_QUEUE_OUT
> +#define IMGU_MAX_QUEUE_DEPTH (2 + 2)
> +
> +#define IMGU_NODE_IN 0 /* Input RAW image */
> +#define IMGU_NODE_PARAMS 1 /* Input parameters */
> +#define IMGU_NODE_OUT2 /* Main output for still or
> video */
> +#define IMGU_NODE_VF 3 /* Preview */
> +#define IMGU_NODE_PV 4 /* Postview for still capture */
> +#define IMGU_NODE_STAT_3A5 /* 3A statistics */
> +#define IMGU_NODE_NUM6
> +
> +#define file_to_intel_ipu3_node(__file) \
> + container_of(video_devdata(__file), struct imgu_video_device, vdev)
> +
> +#define IPU3_INPUT_MIN_WIDTH 0U
> +#define IPU3_INPUT_MIN_HEIGHT0U
> +#define IPU3_INPUT_MAX_WIDTH 5120U
> +#define IPU3_INPUT_MAX_HEIGHT38404U
> +#define IPU3_OUTPUT_MIN_WIDTH2U
> +#define IPU3_OUTPUT_MIN_HEIGHT 2U
> +#define IPU3_OUTPUT_MAX_WIDTH4480U
> +#define IPU3_OUTPUT_MAX_HEIGHT 34004U
> +
> +struct ipu3_vb2_buffer {
> + /* Public fields */
> + struct vb2_v4l2_buffer vbb; /* Must be the first field */
> +
> + /* Private fields */
> + struct list_head list;
> +};
> +
> +struct imgu_buffer {
> + struct ipu3_vb2_buffer vid_buf; /* Must be the first field */
> + struct ipu3_css_buffer css_buf;
> + struct ipu3_css_map map;
> +};
> +
> +struct imgu_node_mapping {
> + unsigned int css_queue;
> + const char *name;
> +};
> +
> +/**
> + * struct imgu_video_device
> + * each node registers as video device and maintains its
> + * own vb2_queue.
> + */
> +struct imgu_video_device {
> + const char *name;
> + bool output;/* Frames to the driver? */
> + bool immutable; /* Can not be enabled/disabled */
> + bool enabled;
> + int queued; /* Buffers already queued */
The queued field is unused.
> + struct v4l2_format vdev_fmt;/* Currently set format */
> +
> + /* Private fields */
> + struct video_device vdev;
> + struct media_pad vdev_pad;
> + struct v4l2_mbus_framefmt pad_fmt;
> + struct vb2_queue vbq;
> + struct list_head buffers;
> + /* Protect vb2_queue and vdev structs*/
> + struct mutex lock;
> + atomic_t sequence;
> +};
> +
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v7 14/16] intel-ipu3: Add v4l2 driver based on media framework
->enabled)
> + flags |= MEDIA_LNK_FL_ENABLED;
> + if (node->immutable)
> + flags |= MEDIA_LNK_FL_IMMUTABLE;
> + if (node->output) {
> + r = media_create_pad_link(>entity, 0,
> + >subdev.entity,
> + i, flags);
> + } else {
> + r = media_create_pad_link(>subdev.entity,
> + i, >entity, 0, flags);
> + }
> + if (r)
> + goto fail_link;
> + }
> +
> + r = media_device_register(>media_dev);
> + if (r) {
> + dev_err(>pci_dev->dev,
> + "failed to register media device (%d)\n", r);
> + i--;
> + goto fail_link;
> + }
> +
> + return 0;
> +
> + for (; i >= 0; i--) {
> +fail_link:
> + video_unregister_device(>nodes[i].vdev);
> +fail_vdev:
> + media_entity_cleanup(>nodes[i].vdev.entity);
> +fail_vdev_media_entity:
> + mutex_destroy(>nodes[i].lock);
> + }
> +fail_subdevs:
> + v4l2_device_unregister_subdev(>subdev);
> +fail_subdev:
> + media_entity_cleanup(>subdev.entity);
> +fail_media_entity:
> + kfree(imgu->subdev_pads);
> +fail_subdev_pads:
> + v4l2_device_unregister(>v4l2_dev);
> +fail_v4l2_dev:
> + media_device_cleanup(>media_dev);
> +
> + return r;
> +}
> +EXPORT_SYMBOL_GPL(ipu3_v4l2_register);
> +
> +int ipu3_v4l2_unregister(struct imgu_device *imgu)
> +{
> + unsigned int i;
> +
> + media_device_unregister(>media_dev);
> + media_device_cleanup(>media_dev);
media_device_cleanup() should take place after unregistering the
sub-devices.
> +
> + for (i = 0; i < IMGU_NODE_NUM; i++) {
> + video_unregister_device(>nodes[i].vdev);
> + media_entity_cleanup(>nodes[i].vdev.entity);
> + mutex_destroy(>nodes[i].lock);
> + }
> +
> + v4l2_device_unregister_subdev(>subdev);
> + media_entity_cleanup(>subdev.entity);
> + kfree(imgu->subdev_pads);
> + v4l2_device_unregister(>v4l2_dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ipu3_v4l2_unregister);
> +
> +void ipu3_v4l2_buffer_done(struct vb2_buffer *vb,
> +enum vb2_buffer_state state)
> +{
> + struct ipu3_vb2_buffer *b =
> + container_of(vb, struct ipu3_vb2_buffer, vbb.vb2_buf);
> +
> + list_del(>list);
> + vb2_buffer_done(>vbb.vb2_buf, state);
> +}
> +EXPORT_SYMBOL_GPL(ipu3_v4l2_buffer_done);
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v7 12/16] intel-ipu3: css: Initialize css hardware
Hi Yong,
On Mon, Oct 29, 2018 at 03:23:06PM -0700, Yong Zhi wrote:
> This patch implements the functions to initialize
> and configure IPU3 h/w such as clock, irq and power.
>
> Signed-off-by: Yong Zhi
> Signed-off-by: Tomasz Figa
> ---
> drivers/media/pci/intel/ipu3/ipu3-css.c | 537
>
> drivers/media/pci/intel/ipu3/ipu3-css.h | 203
> 2 files changed, 740 insertions(+)
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css.c
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css.h
>
...
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-css.h
> b/drivers/media/pci/intel/ipu3/ipu3-css.h
> new file mode 100644
> index 000..d16d0c4
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-css.h
...
> +/* IPU3 Camera Sub System structure */
> +struct ipu3_css {
> + struct device *dev;
> + void __iomem *base;
> + const struct firmware *fw;
> + struct imgu_fw_header *fwp;
> + int iomem_length;
u32? The same for the length parameter in ccs_init().
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
Hi Yong, On Fri, Nov 09, 2018 at 01:28:27AM +, Zhi, Yong wrote: > Hi, Sakari, > > Thanks for your review and comments. > Bingbu has replied to some of your questions, so I will continue with the > rest. > > > -Original Message- > > From: Bing Bu Cao [mailto:bingbu@linux.intel.com] > > Sent: Tuesday, November 6, 2018 10:17 PM > > To: Sakari Ailus ; Zhi, Yong > > > > Cc: linux-media@vger.kernel.org; tf...@chromium.org; > > mche...@kernel.org; hans.verk...@cisco.com; > > laurent.pinch...@ideasonboard.com; Mani, Rajmohan > > ; Zheng, Jian Xu ; Hu, > > Jerry W ; Toivonen, Tuukka > > ; Qiu, Tian Shu ; Cao, > > Bingbu > > Subject: Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset > > > > > > On 11/01/2018 08:03 PM, Sakari Ailus wrote: > > > Hi Yong, > > > > > > Thanks for the update! > > > > > > On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote: > > >> Hi, > > >> > > >> This series adds support for the Intel IPU3 (Image Processing Unit) > > >> ImgU which is essentially a modern memory-to-memory ISP. It > > >> implements raw Bayer to YUV image format conversion as well as a > > >> large number of other pixel processing algorithms for improving the image > > quality. > > >> > > >> Meta data formats are defined for image statistics (3A, i.e. > > >> automatic white balance, exposure and focus, histogram and local area > > >> contrast > > >> enhancement) as well as for the pixel processing algorithm parameters. > > >> The documentation for these formats is currently not included in the > > >> patchset but will be added in a future version of this set. > > >> > > >> The algorithm parameters need to be considered specific to a given > > >> frame and typically a large number of these parameters change on > > >> frame to frame basis. Additionally, the parameters are highly > > >> structured (and not a flat space of independent configuration > > >> primitives). They also reflect the data structures used by the > > >> firmware and the hardware. On top of that, the algorithms require > > >> highly specialized user space to make meaningful use of them. For > > >> these reasons it has been chosen video buffers to pass the parameters to > > the device. > > >> > > >> On individual patches: > > >> > > >> The heart of ImgU is the CSS, or Camera Subsystem, which contains the > > >> image processors and HW accelerators. > > >> > > >> The 3A statistics and other firmware parameter computation related > > >> functions are implemented in patch 11. > > >> > > >> All IPU3 pipeline default settings can be found in patch 10. > > >> > > >> To access DDR via ImgU's own memory space, IPU3 is also equipped with > > >> its own MMU unit, the driver is implemented in patch 6. > > >> > > >> Patch 7 uses above driver for DMA mapping operation. > > >> > > >> The communication between IPU3 firmware and driver is implemented > > >> with circular queues in patch 8. > > >> > > >> Patch 9 provide some utility functions and manage IPU3 fw download > > >> and install. > > >> > > >> The firmware which is called ipu3-fw.bin can be downloaded from: > > >> > > >> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware > > >> .git (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f) > > >> > > >> Firmware ABI is defined in patches 4 and 5. > > >> > > >> Patches 12 and 13 are of the same file, the former contains all h/w > > >> programming related code, the latter implements interface functions > > >> for access fw & hw capabilities. > > >> > > >> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT > > work: > > >> > > >> https://patchwork.kernel.org/patch/9976295/> > > > I've pushed the latest set here: > > > > > > https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output> > > > > > > You can just say the entire set depends on those going forward; the > > > documentation is needed, too. > > > > > Ack. > > > >> Patch 15 represents the top level that glues all of the other > > >> components together, passing arguments between the components.
Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
Hi Bing Bu, On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote: > > On 11/01/2018 08:03 PM, Sakari Ailus wrote: > > Hi Yong, > > > > Thanks for the update! > > > > On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote: > >> Hi, > >> > >> This series adds support for the Intel IPU3 (Image Processing Unit) > >> ImgU which is essentially a modern memory-to-memory ISP. It implements > >> raw Bayer to YUV image format conversion as well as a large number of > >> other pixel processing algorithms for improving the image quality. > >> > >> Meta data formats are defined for image statistics (3A, i.e. automatic > >> white balance, exposure and focus, histogram and local area contrast > >> enhancement) as well as for the pixel processing algorithm parameters. > >> The documentation for these formats is currently not included in the > >> patchset but will be added in a future version of this set. > >> > >> The algorithm parameters need to be considered specific to a given frame > >> and typically a large number of these parameters change on frame to frame > >> basis. Additionally, the parameters are highly structured (and not a flat > >> space of independent configuration primitives). They also reflect the > >> data structures used by the firmware and the hardware. On top of that, > >> the algorithms require highly specialized user space to make meaningful > >> use of them. For these reasons it has been chosen video buffers to pass > >> the parameters to the device. > >> > >> On individual patches: > >> > >> The heart of ImgU is the CSS, or Camera Subsystem, which contains the > >> image processors and HW accelerators. > >> > >> The 3A statistics and other firmware parameter computation related > >> functions are implemented in patch 11. > >> > >> All IPU3 pipeline default settings can be found in patch 10. > >> > >> To access DDR via ImgU's own memory space, IPU3 is also equipped with > >> its own MMU unit, the driver is implemented in patch 6. > >> > >> Patch 7 uses above driver for DMA mapping operation. > >> > >> The communication between IPU3 firmware and driver is implemented with > >> circular > >> queues in patch 8. > >> > >> Patch 9 provide some utility functions and manage IPU3 fw download and > >> install. > >> > >> The firmware which is called ipu3-fw.bin can be downloaded from: > >> > >> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git > >> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f) > >> > >> Firmware ABI is defined in patches 4 and 5. > >> > >> Patches 12 and 13 are of the same file, the former contains all h/w > >> programming > >> related code, the latter implements interface functions for access fw & hw > >> capabilities. > >> > >> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work: > >> > >> https://patchwork.kernel.org/patch/9976295/> > > I've pushed the latest set here: > > > > https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output> > > > > You can just say the entire set depends on those going forward; the > > documentation is needed, too. > > > >> Patch 15 represents the top level that glues all of the other components > >> together, > >> passing arguments between the components. > >> > >> Patch 16 is a recent effort to extend v6 for advanced camera features like > >> Continuous View Finder (CVF) and Snapshot During Video(SDV) support. > >> > >> Link to user space implementation: > >> > >> git clone https://chromium.googlesource.com/chromiumos/platform/arc-camera > >> > >> ImgU media topology print: > >> > >> # media-ctl -d /dev/media0 -p > >> Media controller API version 4.19.0 > >> > >> Media device information > >> > >> driver ipu3-imgu > >> model ipu3-imgu > >> serial > >> bus infoPCI::00:05.0 > >> hw revision 0x80862015 > >> driver version 4.19.0 > >> > >> Device topology > >> - entity 1: ipu3-imgu 0 (5 pads, 5 links) > >> type V4L2 subdev subtype Unknown flags 0 > >> device node name /dev/v4l-subdev0 > >>pad0: Sink > >>
Re: [PATCH v7 08/16] intel-ipu3: css: Add dma buff pool utility functions
t(struct ipu3_css_pool *pool)
> +{
> + pool->entry[pool->last].framenum = INT_MIN;
> + pool->last = (pool->last + IPU3_CSS_POOL_SIZE - 1) % IPU3_CSS_POOL_SIZE;
> +}
> +
> +/**
> + * ipu3_css_pool_last - Retrieve the nth pool entry from last
> + *
> + * @pool: a pointer to ipu3_css_pool.
> + * @n: the distance to the last index.
> + *
> + * Return: The nth entry from last or null map to indicate no frame stored.
> + */
> +const struct ipu3_css_map *
> +ipu3_css_pool_last(struct ipu3_css_pool *pool, unsigned int n)
> +{
> + static const struct ipu3_css_map null_map = { 0 };
> + int i = (pool->last + IPU3_CSS_POOL_SIZE - n) % IPU3_CSS_POOL_SIZE;
> +
> + WARN_ON(n >= IPU3_CSS_POOL_SIZE);
> +
> + if (pool->entry[i].framenum < 0)
> + return _map;
> +
> + return >entry[i].param;
> +}
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> b/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> new file mode 100644
> index 000..71e48d1
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-css-pool.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2018 Intel Corporation */
> +
> +#ifndef __IPU3_UTIL_H
> +#define __IPU3_UTIL_H
> +
> +struct device;
> +struct imgu_device;
> +
> +#define IPU3_CSS_POOL_SIZE 4
> +
> +/**
> + * ipu3_css_map - store DMA mapping info for buffer
> + *
> + * @size:size of the buffer in bytes.
> + * @vaddr: kernel virtual address.
> + * @daddr: iova dma address to access IPU3.
> + * @vma: private, a pointer to vm_struct,
> + * used for ipu3_dmamap_free.
> + */
> +struct ipu3_css_map {
> + size_t size;
> + void *vaddr;
> + dma_addr_t daddr;
> + struct vm_struct *vma;
> +};
> +
> +/**
> + * ipu3_css_pool - circular buffer pool definition
> + *
> + * @entry: array with IPU3_CSS_POOL_SIZE elements.
> + * @entry.param: a ipu3_css_map for storing the mem mapping.
> + * @entry.framenum: the css frame number, used to determine if the entry
> + * is old enough to be recycled.
> + * @last:write pointer, initialized to IPU3_CSS_POOL_SIZE.
> + */
> +struct ipu3_css_pool {
> + struct {
> + struct ipu3_css_map param;
> + long framenum;
> + } entry[IPU3_CSS_POOL_SIZE];
> + unsigned int last; /* Latest entry */
> +};
> +
> +int ipu3_css_dma_buffer_resize(struct imgu_device *imgu,
> +struct ipu3_css_map *map, size_t size);
> +void ipu3_css_pool_cleanup(struct imgu_device *imgu,
> +struct ipu3_css_pool *pool);
> +int ipu3_css_pool_init(struct imgu_device *imgu, struct ipu3_css_pool *pool,
> +size_t size);
> +int ipu3_css_pool_get(struct ipu3_css_pool *pool, long framenum);
> +void ipu3_css_pool_put(struct ipu3_css_pool *pool);
> +const struct ipu3_css_map *ipu3_css_pool_last(struct ipu3_css_pool *pool,
> + unsigned int last);
> +
> +#endif
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
[PATCH v3.16 0/2] V4L2 event subscription fixes
Hi Ben, The two patches fix a use-after-free issue in V4L2 event handling. The first patch that fixes that issue is already in the other stable trees (as well as Linus's tree) whereas the second that fixes a bug in the first one, is in the media tree only as of yet. https://git.linuxtv.org/media_tree.git/commit/?id=92539d3eda2c090b382699bbb896d4b54e9bdece> Sakari Ailus (2): v4l: event: Prevent freeing event subscriptions while accessed v4l: event: Add subscription to list before calling "add" operation drivers/media/v4l2-core/v4l2-event.c | 63 drivers/media/v4l2-core/v4l2-fh.c| 2 ++ include/media/v4l2-fh.h | 1 + 3 files changed, 38 insertions(+), 28 deletions(-) -- 2.11.0
[PATCH v3.16 1/2] v4l: event: Prevent freeing event subscriptions while accessed
[ upstream commit ad608fbcf166fec809e402d548761768f602702c ]
The event subscriptions are added to the subscribed event list while
holding a spinlock, but that lock is subsequently released while still
accessing the subscription object. This makes it possible to unsubscribe
the event --- and freeing the subscription object's memory --- while
the subscription object is simultaneously accessed.
Prevent this by adding a mutex to serialise the event subscription and
unsubscription. This also gives a guarantee to the callback ops that the
add op has returned before the del op is called.
This change also results in making the elems field less special:
subscriptions are only added to the event list once they are fully
initialised.
Signed-off-by: Sakari Ailus
Reviewed-by: Hans Verkuil
Reviewed-by: Laurent Pinchart
Cc: sta...@vger.kernel.org # for 4.14 and up
Fixes: c3b5b0241f62 ("V4L/DVB: V4L: Events: Add backend")
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/v4l2-core/v4l2-event.c | 38 +++-
drivers/media/v4l2-core/v4l2-fh.c| 2 ++
include/media/v4l2-fh.h | 1 +
3 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index 8761aab99de95..fcf65f131a2ac 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -119,14 +119,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
const struct v4l2_event *e
if (sev == NULL)
return;
- /*
-* If the event has been added to the fh->subscribed list, but its
-* add op has not completed yet elems will be 0, treat this as
-* not being subscribed.
-*/
- if (!sev->elems)
- return;
-
/* Increase event sequence number on fh. */
fh->sequence++;
@@ -209,6 +201,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
struct v4l2_subscribed_event *sev, *found_ev;
unsigned long flags;
unsigned i;
+ int ret = 0;
if (sub->type == V4L2_EVENT_ALL)
return -EINVAL;
@@ -226,31 +219,36 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
sev->flags = sub->flags;
sev->fh = fh;
sev->ops = ops;
+ sev->elems = elems;
+
+ mutex_lock(>subscribe_lock);
spin_lock_irqsave(>vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (!found_ev)
- list_add(>list, >subscribed);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
if (found_ev) {
+ /* Already listening */
kfree(sev);
- return 0; /* Already listening */
+ goto out_unlock;
}
if (sev->ops && sev->ops->add) {
- int ret = sev->ops->add(sev, elems);
+ ret = sev->ops->add(sev, elems);
if (ret) {
- sev->ops = NULL;
- v4l2_event_unsubscribe(fh, sub);
- return ret;
+ kfree(sev);
+ goto out_unlock;
}
}
- /* Mark as ready for use */
- sev->elems = elems;
+ spin_lock_irqsave(>vdev->fh_lock, flags);
+ list_add(>list, >subscribed);
+ spin_unlock_irqrestore(>vdev->fh_lock, flags);
- return 0;
+out_unlock:
+ mutex_unlock(>subscribe_lock);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
@@ -289,6 +287,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
return 0;
}
+ mutex_lock(>subscribe_lock);
+
spin_lock_irqsave(>vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
@@ -306,6 +306,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
if (sev && sev->ops && sev->ops->del)
sev->ops->del(sev);
+ mutex_unlock(>subscribe_lock);
+
kfree(sev);
return 0;
diff --git a/drivers/media/v4l2-core/v4l2-fh.c
b/drivers/media/v4l2-core/v4l2-fh.c
index e57c002b41506..573ec2f192d44 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -42,6 +42,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device
*vdev)
INIT_LIST_HEAD(>available);
INIT_LIST_HEAD(>subscribed);
fh->sequence = -1;
+ mutex_init(>subscribe_lock);
}
EXPORT_SYMBOL_GPL(v4l2_fh_init);
@@ -88,6 +89,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
if (fh->vdev == NULL)
return;
v4l2_event_unsubscribe_all(fh);
+ mutex_destroy(>subscribe_lock);
fh->vdev = NULL;
}
EXPORT_SYMBOL_GPL(v4l2_fh_exit);
diff --git a/include/media/v4
[PATCH v3.16 2/2] v4l: event: Add subscription to list before calling "add" operation
[ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
Patch ad608fbcf166 changed how events were subscribed to address an issue
elsewhere. As a side effect of that change, the "add" callback was called
before the event subscription was added to the list of subscribed events,
causing the first event queued by the add callback (and possibly other
events arriving soon afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add"
callback, and clean up afterwards if that fails.
Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions
while accessed")
Reported-by: Dave Stevenson
Signed-off-by: Sakari Ailus
Tested-by: Dave Stevenson
Reviewed-by: Hans Verkuil
Tested-by: Hans Verkuil
Cc: sta...@vger.kernel.org (for 4.14 and up)
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/v4l2-core/v4l2-event.c | 43
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index fcf65f131a2ac..35901f5b3971b 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -194,6 +194,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_event_pending);
+static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
+{
+ struct v4l2_fh *fh = sev->fh;
+ unsigned int i;
+
+ lockdep_assert_held(>subscribe_lock);
+ assert_spin_locked(>vdev->fh_lock);
+
+ /* Remove any pending events for this subscription */
+ for (i = 0; i < sev->in_use; i++) {
+ list_del(>events[sev_pos(sev, i)].list);
+ fh->navailable--;
+ }
+ list_del(>list);
+}
+
int v4l2_event_subscribe(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub, unsigned
elems,
const struct v4l2_subscribed_event_ops *ops)
@@ -225,27 +241,23 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
+ if (!found_ev)
+ list_add(>list, >subscribed);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
if (found_ev) {
/* Already listening */
kfree(sev);
- goto out_unlock;
- }
-
- if (sev->ops && sev->ops->add) {
+ } else if (sev->ops && sev->ops->add) {
ret = sev->ops->add(sev, elems);
if (ret) {
+ spin_lock_irqsave(>vdev->fh_lock, flags);
+ __v4l2_event_unsubscribe(sev);
+ spin_unlock_irqrestore(>vdev->fh_lock, flags);
kfree(sev);
- goto out_unlock;
}
}
- spin_lock_irqsave(>vdev->fh_lock, flags);
- list_add(>list, >subscribed);
- spin_unlock_irqrestore(>vdev->fh_lock, flags);
-
-out_unlock:
mutex_unlock(>subscribe_lock);
return ret;
@@ -280,7 +292,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
{
struct v4l2_subscribed_event *sev;
unsigned long flags;
- int i;
if (sub->type == V4L2_EVENT_ALL) {
v4l2_event_unsubscribe_all(fh);
@@ -292,14 +303,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (sev != NULL) {
- /* Remove any pending events for this subscription */
- for (i = 0; i < sev->in_use; i++) {
- list_del(>events[sev_pos(sev, i)].list);
- fh->navailable--;
- }
- list_del(>list);
- }
+ if (sev != NULL)
+ __v4l2_event_unsubscribe(sev);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
--
2.11.0
[PATCH v4.4 1/1] v4l: event: Add subscription to list before calling "add" operation
[ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
Patch ad608fbcf166 changed how events were subscribed to address an issue
elsewhere. As a side effect of that change, the "add" callback was called
before the event subscription was added to the list of subscribed events,
causing the first event queued by the add callback (and possibly other
events arriving soon afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add"
callback, and clean up afterwards if that fails.
Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions
while accessed")
Reported-by: Dave Stevenson
Signed-off-by: Sakari Ailus
Tested-by: Dave Stevenson
Reviewed-by: Hans Verkuil
Tested-by: Hans Verkuil
Cc: sta...@vger.kernel.org (for 4.14 and up)
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/v4l2-core/v4l2-event.c | 43
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index b47ac4e053d0e..f5c8a952f0aa3 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -197,6 +197,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_event_pending);
+static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
+{
+ struct v4l2_fh *fh = sev->fh;
+ unsigned int i;
+
+ lockdep_assert_held(>subscribe_lock);
+ assert_spin_locked(>vdev->fh_lock);
+
+ /* Remove any pending events for this subscription */
+ for (i = 0; i < sev->in_use; i++) {
+ list_del(>events[sev_pos(sev, i)].list);
+ fh->navailable--;
+ }
+ list_del(>list);
+}
+
int v4l2_event_subscribe(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub, unsigned
elems,
const struct v4l2_subscribed_event_ops *ops)
@@ -228,27 +244,23 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
+ if (!found_ev)
+ list_add(>list, >subscribed);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
if (found_ev) {
/* Already listening */
kfree(sev);
- goto out_unlock;
- }
-
- if (sev->ops && sev->ops->add) {
+ } else if (sev->ops && sev->ops->add) {
ret = sev->ops->add(sev, elems);
if (ret) {
+ spin_lock_irqsave(>vdev->fh_lock, flags);
+ __v4l2_event_unsubscribe(sev);
+ spin_unlock_irqrestore(>vdev->fh_lock, flags);
kfree(sev);
- goto out_unlock;
}
}
- spin_lock_irqsave(>vdev->fh_lock, flags);
- list_add(>list, >subscribed);
- spin_unlock_irqrestore(>vdev->fh_lock, flags);
-
-out_unlock:
mutex_unlock(>subscribe_lock);
return ret;
@@ -283,7 +295,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
{
struct v4l2_subscribed_event *sev;
unsigned long flags;
- int i;
if (sub->type == V4L2_EVENT_ALL) {
v4l2_event_unsubscribe_all(fh);
@@ -295,14 +306,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (sev != NULL) {
- /* Remove any pending events for this subscription */
- for (i = 0; i < sev->in_use; i++) {
- list_del(>events[sev_pos(sev, i)].list);
- fh->navailable--;
- }
- list_del(>list);
- }
+ if (sev != NULL)
+ __v4l2_event_unsubscribe(sev);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
--
2.11.0
[PATCH v4.9 1/1] v4l: event: Add subscription to list before calling "add" operation
[ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
Patch ad608fbcf166 changed how events were subscribed to address an issue
elsewhere. As a side effect of that change, the "add" callback was called
before the event subscription was added to the list of subscribed events,
causing the first event queued by the add callback (and possibly other
events arriving soon afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add"
callback, and clean up afterwards if that fails.
Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions
while accessed")
Reported-by: Dave Stevenson
Signed-off-by: Sakari Ailus
Tested-by: Dave Stevenson
Reviewed-by: Hans Verkuil
Tested-by: Hans Verkuil
Cc: sta...@vger.kernel.org (for 4.14 and up)
Signed-off-by: Mauro Carvalho Chehab
---
Hi Greg,
This is a backport of a fix in the media tree for the 4.9 stable series.
https://git.linuxtv.org/media_tree.git/commit/?id=92539d3eda2c090b382699bbb896d4b54e9bdece>
I'll send patches for 4.4 and 3.16 (to Ben H.) kernels shortly.
drivers/media/v4l2-core/v4l2-event.c | 43
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index 567d86835f001..1fda2873375f6 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -197,6 +197,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_event_pending);
+static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
+{
+ struct v4l2_fh *fh = sev->fh;
+ unsigned int i;
+
+ lockdep_assert_held(>subscribe_lock);
+ assert_spin_locked(>vdev->fh_lock);
+
+ /* Remove any pending events for this subscription */
+ for (i = 0; i < sev->in_use; i++) {
+ list_del(>events[sev_pos(sev, i)].list);
+ fh->navailable--;
+ }
+ list_del(>list);
+}
+
int v4l2_event_subscribe(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub, unsigned
elems,
const struct v4l2_subscribed_event_ops *ops)
@@ -228,27 +244,23 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
+ if (!found_ev)
+ list_add(>list, >subscribed);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
if (found_ev) {
/* Already listening */
kfree(sev);
- goto out_unlock;
- }
-
- if (sev->ops && sev->ops->add) {
+ } else if (sev->ops && sev->ops->add) {
ret = sev->ops->add(sev, elems);
if (ret) {
+ spin_lock_irqsave(>vdev->fh_lock, flags);
+ __v4l2_event_unsubscribe(sev);
+ spin_unlock_irqrestore(>vdev->fh_lock, flags);
kfree(sev);
- goto out_unlock;
}
}
- spin_lock_irqsave(>vdev->fh_lock, flags);
- list_add(>list, >subscribed);
- spin_unlock_irqrestore(>vdev->fh_lock, flags);
-
-out_unlock:
mutex_unlock(>subscribe_lock);
return ret;
@@ -283,7 +295,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
{
struct v4l2_subscribed_event *sev;
unsigned long flags;
- int i;
if (sub->type == V4L2_EVENT_ALL) {
v4l2_event_unsubscribe_all(fh);
@@ -295,14 +306,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (sev != NULL) {
- /* Remove any pending events for this subscription */
- for (i = 0; i < sev->in_use; i++) {
- list_del(>events[sev_pos(sev, i)].list);
- fh->navailable--;
- }
- list_del(>list);
- }
+ if (sev != NULL)
+ __v4l2_event_unsubscribe(sev);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
--
2.11.0
[PATCH 1/1] v4l: uAPI doc: Simplify NATIVE_SIZE selection target documentation
The NATIVE_SIZE target is documented for mem2mem devices but no driver has ever apparently used it. It may be never needed; remove it for now. Suggested-by: Hans Verkuil Signed-off-by: Sakari Ailus --- Documentation/media/uapi/v4l/v4l2-selection-targets.rst | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/Documentation/media/uapi/v4l/v4l2-selection-targets.rst b/Documentation/media/uapi/v4l/v4l2-selection-targets.rst index 87433ec76c6b..bee31611947e 100644 --- a/Documentation/media/uapi/v4l/v4l2-selection-targets.rst +++ b/Documentation/media/uapi/v4l/v4l2-selection-targets.rst @@ -42,12 +42,7 @@ of the two interfaces they are used. * - ``V4L2_SEL_TGT_NATIVE_SIZE`` - 0x0003 - The native size of the device, e.g. a sensor's pixel array. - ``left`` and ``top`` fields are zero for this target. Setting the - native size will generally only make sense for memory to memory - devices where the software can create a canvas of a given size in - which for example a video frame can be composed. In that case - V4L2_SEL_TGT_NATIVE_SIZE can be used to configure the size of - that canvas. + ``left`` and ``top`` fields are zero for this target. - Yes - Yes * - ``V4L2_SEL_TGT_COMPOSE`` -- 2.11.0
Re: [PATCH v4l-utils] keytable: fix compilation warning
On Wed, Nov 07, 2018 at 12:02:14PM +, Sean Young wrote: > keytable.c: In function ‘parse_opt’: > keytable.c:835:7: warning: ‘param’ may be used uninitialized in this function > [-Wuninitialized] > > Signed-off-by: Sean Young Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH 1/3] media: imx: add capture compose rectangle
Hi Philipp,
On Tue, Nov 06, 2018 at 03:44:07PM +0100, Philipp Zabel wrote:
> Hi Sakari,
>
> On Tue, 2018-11-06 at 16:01 +0200, Sakari Ailus wrote:
> [...]
> > @@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh,
> > v4l2_std_id std)
> > > return v4l2_subdev_call(priv->src_sd, video, s_std, std);
> > > }
> > >
> > > +static int capture_g_selection(struct file *file, void *fh,
> > > +struct v4l2_selection *s)
> > > +{
> > > + struct capture_priv *priv = video_drvdata(file);
> > > +
> > > + switch (s->target) {
> > > + case V4L2_SEL_TGT_CROP:
> > > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > > + case V4L2_SEL_TGT_NATIVE_SIZE:
> >
> > The NATIVE_SIZE is for devices such as sensors. It doesn't make sense here.
>
> Should this be documented in Documentation/media/uapi/v4l/v4l2-
> selection-targets.rst ? There it only mentions when to make it
> writeable.
This seems to have originated from the documentation before the ReST
conversion and I had hard time to figure out where the current text (apart
from sensor pixel array) came from. There is also no driver using it in
that meaning, and I doubt if the use is not already been covered by the
compose rectangle.
This indeed requires some follow-up, but that's out of scope of your set.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v7 05/12] media: dt-bindings: add bindings for i.MX7 media driver
On Fri, Aug 10, 2018 at 03:20:38PM +0100, Rui Miguel Silva wrote: > Add bindings documentation for i.MX7 media drivers. > The imx7 MIPI CSI2 and imx7 CMOS Sensor Interface. > > Reviewed-by: Rob Herring > Signed-off-by: Rui Miguel Silva Acked-by: Sakari Ailus - -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH] media: imx258: remove test pattern map from driver
Hi Jason,
Thanks for the patch.
On Wed, Nov 07, 2018 at 03:22:23PM +0800, jasonx.z.c...@intel.com wrote:
> From: "Chen, JasonX Z"
>
> Test Pattern mode be picked at HAL instead of driver.
> do a FLIP when userspace use test pattern mode.
> add entity_ops for validating imx258 link.
>
> Signed-off-by: Chen, JasonX Z
> ---
> drivers/media/i2c/imx258.c | 28
> 1 file changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 31a1e22..71f9875 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -62,11 +62,6 @@
>
> /* Test Pattern Control */
> #define IMX258_REG_TEST_PATTERN 0x0600
> -#define IMX258_TEST_PATTERN_DISABLE 0
> -#define IMX258_TEST_PATTERN_SOLID_COLOR 1
> -#define IMX258_TEST_PATTERN_COLOR_BARS 2
> -#define IMX258_TEST_PATTERN_GREY_COLOR 3
> -#define IMX258_TEST_PATTERN_PN9 4
>
> /* Orientation */
> #define REG_MIRROR_FLIP_CONTROL 0x0101
> @@ -504,20 +499,12 @@ struct imx258_mode {
>
> static const char * const imx258_test_pattern_menu[] = {
> "Disabled",
> - "Color Bars",
> "Solid Color",
> + "Color Bars",
> "Grey Color Bars",
> "PN9"
> };
>
> -static const int imx258_test_pattern_val[] = {
> - IMX258_TEST_PATTERN_DISABLE,
> - IMX258_TEST_PATTERN_COLOR_BARS,
> - IMX258_TEST_PATTERN_SOLID_COLOR,
> - IMX258_TEST_PATTERN_GREY_COLOR,
> - IMX258_TEST_PATTERN_PN9,
> -};
> -
> /* Configurations for supported link frequencies */
> #define IMX258_LINK_FREQ_634MHZ 63360ULL
> #define IMX258_LINK_FREQ_320MHZ 32000ULL
> @@ -752,7 +739,6 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> container_of(ctrl->handler, struct imx258, ctrl_handler);
> struct i2c_client *client = v4l2_get_subdevdata(>sd);
> int ret = 0;
> -
This seems like an unrelated change, and it's a common (and usually good)
practice to add an empty line after variable declarations.
> /*
>* Applying V4L2 control value only happens
>* when power is up for streaming
> @@ -778,13 +764,10 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> case V4L2_CID_TEST_PATTERN:
> ret = imx258_write_reg(imx258, IMX258_REG_TEST_PATTERN,
> IMX258_REG_VALUE_16BIT,
> - imx258_test_pattern_val[ctrl->val]);
> -
> + ctrl->val);
> ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> IMX258_REG_VALUE_08BIT,
> - ctrl->val == imx258_test_pattern_val
> - [IMX258_TEST_PATTERN_DISABLE] ?
> - REG_CONFIG_MIRROR_FLIP :
> + !ctrl->val?REG_CONFIG_MIRROR_FLIP :
> REG_CONFIG_FLIP_TEST_PATTERN);
> break;
> default:
> @@ -1105,6 +1088,10 @@ static int imx258_identify_module(struct imx258
> *imx258)
> .pad = _pad_ops,
> };
>
> +static const struct media_entity_operations imx258_subdev_entity_ops = {
> + .link_validate = v4l2_subdev_link_validate,
> +};
> +
This seems unrelated as well. Do you need the link validation for something?
As far as I understand, the driver exposes a single source pad; therefore
the link_validate op will never be called.
> static const struct v4l2_subdev_internal_ops imx258_internal_ops = {
> .open = imx258_open,
> };
> @@ -1250,6 +1237,7 @@ static int imx258_probe(struct i2c_client *client)
>
> /* Initialize subdev */
> imx258->sd.internal_ops = _internal_ops;
> + imx258->sd.entity.ops = _subdev_entity_ops;
Ditto.
> imx258->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> imx258->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH] [v4l-utils] libv4l2subdev: Add MEDIA_BUS_FMT_FIXED to mbus_formats[]
On Tue, Nov 06, 2018 at 09:12:56AM -0800, Yong Zhi wrote:
> Also add V4L2_COLORSPACE_RAW to the colorspaces[].
>
> Signed-off-by: Yong Zhi
> ---
> utils/media-ctl/libv4l2subdev.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
> index a989efb..46668eb 100644
> --- a/utils/media-ctl/libv4l2subdev.c
> +++ b/utils/media-ctl/libv4l2subdev.c
> @@ -855,6 +855,7 @@ static const struct {
> enum v4l2_mbus_pixelcode code;
> } mbus_formats[] = {
> #include "media-bus-format-names.h"
> + { "FIXED", MEDIA_BUS_FMT_FIXED},
> { "Y8", MEDIA_BUS_FMT_Y8_1X8},
> { "Y10", MEDIA_BUS_FMT_Y10_1X10 },
> { "Y12", MEDIA_BUS_FMT_Y12_1X12 },
> @@ -965,7 +966,9 @@ static struct {
> { "srgb", V4L2_COLORSPACE_SRGB },
> { "oprgb", V4L2_COLORSPACE_OPRGB },
> { "bt2020", V4L2_COLORSPACE_BT2020 },
> + { "raw", V4L2_COLORSPACE_RAW },
> { "dcip3", V4L2_COLORSPACE_DCI_P3 },
> +
> };
>
> const char *v4l2_subdev_colorspace_to_string(enum v4l2_colorspace colorspace)
The diff became:
diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
index 46668eb5..0d0afbe7 100644
--- a/utils/media-ctl/libv4l2subdev.c
+++ b/utils/media-ctl/libv4l2subdev.c
@@ -855,8 +855,8 @@ static const struct {
enum v4l2_mbus_pixelcode code;
} mbus_formats[] = {
#include "media-bus-format-names.h"
- { "FIXED", MEDIA_BUS_FMT_FIXED},
- { "Y8", MEDIA_BUS_FMT_Y8_1X8},
+ { "FIXED", MEDIA_BUS_FMT_FIXED },
+ { "Y8", MEDIA_BUS_FMT_Y8_1X8 },
{ "Y10", MEDIA_BUS_FMT_Y10_1X10 },
{ "Y12", MEDIA_BUS_FMT_Y12_1X12 },
{ "YUYV", MEDIA_BUS_FMT_YUYV8_1X16 },
@@ -968,7 +968,6 @@ static struct {
{ "bt2020", V4L2_COLORSPACE_BT2020 },
{ "raw", V4L2_COLORSPACE_RAW },
{ "dcip3", V4L2_COLORSPACE_DCI_P3 },
-
};
const char *v4l2_subdev_colorspace_to_string(enum v4l2_colorspace colorspace)
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH] [v4l-utils] libv4l2subdev: Add MEDIA_BUS_FMT_FIXED to mbus_formats[]
Hi Yong,
On Tue, Nov 06, 2018 at 09:12:56AM -0800, Yong Zhi wrote:
> Also add V4L2_COLORSPACE_RAW to the colorspaces[].
>
> Signed-off-by: Yong Zhi
> ---
> utils/media-ctl/libv4l2subdev.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
> index a989efb..46668eb 100644
> --- a/utils/media-ctl/libv4l2subdev.c
> +++ b/utils/media-ctl/libv4l2subdev.c
> @@ -855,6 +855,7 @@ static const struct {
> enum v4l2_mbus_pixelcode code;
> } mbus_formats[] = {
> #include "media-bus-format-names.h"
> + { "FIXED", MEDIA_BUS_FMT_FIXED},
> { "Y8", MEDIA_BUS_FMT_Y8_1X8},
> { "Y10", MEDIA_BUS_FMT_Y10_1X10 },
> { "Y12", MEDIA_BUS_FMT_Y12_1X12 },
> @@ -965,7 +966,9 @@ static struct {
> { "srgb", V4L2_COLORSPACE_SRGB },
> { "oprgb", V4L2_COLORSPACE_OPRGB },
> { "bt2020", V4L2_COLORSPACE_BT2020 },
> + { "raw", V4L2_COLORSPACE_RAW },
> { "dcip3", V4L2_COLORSPACE_DCI_P3 },
> +
Extra newlne.
I can remove it as well while applying the patch.
> };
>
> const char *v4l2_subdev_colorspace_to_string(enum v4l2_colorspace colorspace)
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH 1/3] media: imx: add capture compose rectangle
Hi Philipp,
On Mon, Nov 05, 2018 at 04:20:53PM +0100, Philipp Zabel wrote:
> Allowing to compose captured images into larger memory buffers
> will let us lift alignment restrictions on CSI crop width.
>
> Signed-off-by: Philipp Zabel
> ---
> drivers/staging/media/imx/imx-ic-prpencvf.c | 3 +-
> drivers/staging/media/imx/imx-media-capture.c | 38 +++
> drivers/staging/media/imx/imx-media-csi.c | 3 +-
> drivers/staging/media/imx/imx-media-vdic.c| 4 +-
> drivers/staging/media/imx/imx-media.h | 2 +
> 5 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c
> b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index 28f41caba05d..fe5a77baa592 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -366,8 +366,7 @@ static int prp_setup_channel(struct prp_priv *priv,
>
> memset(, 0, sizeof(image));
> image.pix = vdev->fmt.fmt.pix;
> - image.rect.width = image.pix.width;
> - image.rect.height = image.pix.height;
> + image.rect = vdev->compose;
>
> if (rot_swap_width_height) {
> swap(image.pix.width, image.pix.height);
> diff --git a/drivers/staging/media/imx/imx-media-capture.c
> b/drivers/staging/media/imx/imx-media-capture.c
> index b37e1186eb2f..cace8a51aca8 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -262,6 +262,10 @@ static int capture_s_fmt_vid_cap(struct file *file, void
> *fh,
> priv->vdev.fmt.fmt.pix = f->fmt.pix;
> priv->vdev.cc = imx_media_find_format(f->fmt.pix.pixelformat,
> CS_SEL_ANY, true);
> + priv->vdev.compose.left = 0;
> + priv->vdev.compose.top = 0;
> + priv->vdev.compose.width = f->fmt.fmt.pix.width;
> + priv->vdev.compose.height = f->fmt.fmt.pix.height;
>
> return 0;
> }
> @@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh,
> v4l2_std_id std)
> return v4l2_subdev_call(priv->src_sd, video, s_std, std);
> }
>
> +static int capture_g_selection(struct file *file, void *fh,
> +struct v4l2_selection *s)
> +{
> + struct capture_priv *priv = video_drvdata(file);
> +
> + switch (s->target) {
> + case V4L2_SEL_TGT_CROP:
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + case V4L2_SEL_TGT_NATIVE_SIZE:
The NATIVE_SIZE is for devices such as sensors. It doesn't make sense here.
With that removed,
Acked-by: Sakari Ailus
> + case V4L2_SEL_TGT_COMPOSE:
> + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> + case V4L2_SEL_TGT_COMPOSE_PADDED:
> + s->r = priv->vdev.compose;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int capture_s_selection(struct file *file, void *fh,
> +struct v4l2_selection *s)
> +{
> + return capture_g_selection(file, fh, s);
> +}
> +
> static int capture_g_parm(struct file *file, void *fh,
> struct v4l2_streamparm *a)
> {
> @@ -350,6 +383,9 @@ static const struct v4l2_ioctl_ops capture_ioctl_ops = {
> .vidioc_g_std = capture_g_std,
> .vidioc_s_std = capture_s_std,
>
> + .vidioc_g_selection = capture_g_selection,
> + .vidioc_s_selection = capture_s_selection,
> +
> .vidioc_g_parm = capture_g_parm,
> .vidioc_s_parm = capture_s_parm,
>
> @@ -687,6 +723,8 @@ int imx_media_capture_device_register(struct
> imx_media_video_dev *vdev)
> vdev->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> imx_media_mbus_fmt_to_pix_fmt(>fmt.fmt.pix,
> _src.format, NULL);
> + vdev->compose.width = fmt_src.format.width;
> + vdev->compose.height = fmt_src.format.height;
> vdev->cc = imx_media_find_format(vdev->fmt.fmt.pix.pixelformat,
>CS_SEL_ANY, false);
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index 4223f8d418ae..c4523afe7b48 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -413,8 +413,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
>
> memset(, 0, sizeof(image));
> image.pix = vdev->fmt.fmt.pix;
> - image.rect.width =
[GIT PULL for 4.20] Sensor and ISP driver patches for 4.21
Hi Mauro, Here are a few sensor and ISP driver patches for 4.21, plus a documentation fix. The noteworthy change in the sea of bugfixes is the imx214 driver. Please pull. The following changes since commit dafb7f9aef2fd44991ff1691721ff765a23be27b: v4l2-controls: add a missing include (2018-11-02 06:36:32 -0400) are available in the git repository at: ssh://linuxtv.org/git/sailus/media_tree.git tags/for-4.21-1-sign for you to fetch changes up to aaa886f8404b6ae39aad984c8b826c092ebe0092: media: ov7740: constify structures stored in fields of v4l2_subdev_ops structure (2018-11-06 13:33:19 +0200) Patches or 4.21 Chiranjeevi Rapolu (1): media: ov13858: Check for possible null pointer Julia Lawall (2): media: ov5645: constify v4l2_ctrl_ops structure media: ov7740: constify structures stored in fields of v4l2_subdev_ops structure Rajmohan Mani (1): media: intel-ipu3: cio2: Remove redundant definitions Ricardo Ribalda Delgado (2): imx214: device tree binding imx214: Add imx214 camera sensor driver Sakari Ailus (4): media: docs: Document metadata format in struct v4l2_format omap3isp: Unregister media device as first ipu3-cio2: Unregister device nodes first, then release resources ipu3-cio2: Use cio2_queues_exit .../devicetree/bindings/media/i2c/sony,imx214.txt | 53 + Documentation/media/uapi/v4l/dev-meta.rst |2 +- Documentation/media/uapi/v4l/vidioc-g-fmt.rst |5 + MAINTAINERS|8 + drivers/media/i2c/Kconfig | 12 + drivers/media/i2c/Makefile |1 + drivers/media/i2c/imx214.c | 1118 drivers/media/i2c/ov13858.c|6 +- drivers/media/i2c/ov5645.c |2 +- drivers/media/i2c/ov7740.c |4 +- drivers/media/pci/intel/ipu3/ipu3-cio2.c |6 +- drivers/media/pci/intel/ipu3/ipu3-cio2.h |2 - drivers/media/platform/omap3isp/isp.c |3 +- 13 files changed, 1209 insertions(+), 13 deletions(-) create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx214.txt create mode 100644 drivers/media/i2c/imx214.c -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [RFC] Create test script(s?) for regression testing
Hi Hans, On Tue, Nov 06, 2018 at 09:37:07AM +0100, Hans Verkuil wrote: > Hi all, > > After the media summit (heavy on test discussions) and the V4L2 event > regression > we just found it is clear we need to do a better job with testing. > > All the pieces are in place, so what is needed is to combine it and create a > script that anyone of us as core developers can run to check for regressions. > The same script can be run as part of the kernelci regression testing. I'd say that *some* pieces are in place. Of course, the more there is, the better. The more there are tests, the more important it would be they're automated, preferrably without the developer having to run them on his/her own machine. > > We have four virtual drivers: vivid, vim2m, vimc and vicodec. The last one > is IMHO not quite good enough yet for testing: it is not fully compliant to > the > upcoming stateful codec spec. Work for that is planned as part of an Outreachy > project. > > My idea is to create a script that is maintained as part of v4l-utils that > loads the drivers and runs v4l2-compliance and possibly other tests against > the virtual drivers. How about spending a little time to pick a suitable framework for running the tests? It could be useful to get more informative reports than just pass / fail. Do note that for different hardware the tests would be likely different as well although there are classes of devices for which the exact same tests would be applicable. > > It should be simple to use and require very little in the way of dependencies. > Ideally no dependencies other than what is in v4l-utils so it can easily be > run > on an embedded system as well. > > For a 64-bit kernel it should run the tests both with 32-bit and 64-bit > applications. > > It should also test with both single and multiplanar modes where available. > > Since vivid emulates CEC as well, it should run CEC tests too. > > As core developers we should have an environment where we can easily test > our patches with this script (I use a VM for that). > > I think maintaining the script (or perhaps scripts) in v4l-utils is best since > that keeps it in sync with the latest kernel and v4l-utils developments. Makes sense --- and that can be always changed later on if there's a need to. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
[GIT PULL FOR 4.20] Fix first event delivery
Hi Mauro, There turns out to have been an issue in the event subscription fix; in particular the first control event is missed due to a subtle bug in the patch. This patch fixes it. Once it's in, I'll submit the corresponding patches to the stable kernels. Please pull. The following changes since commit dafb7f9aef2fd44991ff1691721ff765a23be27b: v4l2-controls: add a missing include (2018-11-02 06:36:32 -0400) are available in the git repository at: ssh://linuxtv.org/git/sailus/media_tree.git tags/event-sub-fix-sign for you to fetch changes up to cbafeff167c91243f336e1703d7f86aa019b973e: v4l: event: Add subscription to list before calling "add" operation (2018-11-06 10:57:34 +0200) fix event subscription -------- Sakari Ailus (1): v4l: event: Add subscription to list before calling "add" operation drivers/media/v4l2-core/v4l2-event.c | 43 1 file changed, 24 insertions(+), 19 deletions(-) -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v7 05/16] intel-ipu3: abi: Add structs
Hi Raj,
On Mon, Nov 05, 2018 at 07:05:53PM +, Mani, Rajmohan wrote:
> Hi Sakari,
>
> > -Original Message-
> > From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> > Sent: Monday, November 05, 2018 12:28 AM
> > To: Zhi, Yong
> > Cc: linux-media@vger.kernel.org; tf...@chromium.org; mche...@kernel.org;
> > hans.verk...@cisco.com; laurent.pinch...@ideasonboard.com; Mani,
> > Rajmohan ; Zheng, Jian Xu
> > ; Hu, Jerry W ; Toivonen,
> > Tuukka ; Qiu, Tian Shu
> > ; Cao, Bingbu
> > Subject: Re: [PATCH v7 05/16] intel-ipu3: abi: Add structs
> >
> > Hi Yong,
> >
> > On Mon, Oct 29, 2018 at 03:22:59PM -0700, Yong Zhi wrote:
> > > This add all the structs of IPU3 firmware ABI.
> > >
> > > Signed-off-by: Yong Zhi
> > > Signed-off-by: Rajmohan Mani
> >
> > ...
> >
> > > +struct imgu_abi_shd_intra_frame_operations_data {
> > > + struct imgu_abi_acc_operation
> > > + operation_list[IMGU_ABI_SHD_MAX_OPERATIONS]
> > __attribute__((aligned(32)));
> > > + struct imgu_abi_acc_process_lines_cmd_data
> > > + process_lines_data[IMGU_ABI_SHD_MAX_PROCESS_LINES]
> > __attribute__((aligned(32)));
> > > + struct imgu_abi_shd_transfer_luts_set_data
> > > + transfer_data[IMGU_ABI_SHD_MAX_TRANSFERS]
> > > +__attribute__((aligned(32)));
> >
> > Could you replace this wth __aligned(32), please? The same for the rest of
> > the
> > header.
> >
>
> Using __aligned(32) in the uAPI header resulted in compilation errors in
> user space / camera HAL code.
>
> e.g
> ../../../../../../../../usr/include/linux/intel-ipu3.h:464:57: error:
> expected ';'
> at end of declaration list
> __u8 bayer_table[IPU3_UAPI_AWB_FR_BAYER_TABLE_MAX_SIZE] __aligned(32);
>
> So we ended up using __attribute__((aligned(32))) format in uAPI header and
> to be consistent, we followed the same format in ABI header as well.
>
> Let us know if it's okay to deviate between uAPI and ABI header for this
> alignment qualifier.
There's a reason for using __attribute__((aligned(32))) in the uAPI header,
but not in the in-kernel headers where __aligned(32) is preferred.
I have a patch for addressing this for the uAPI headers as well so
__aligned(32) could be used there, too; I'll submit it soon. Let's see...
there are kerneldoc issues still in this area.
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
[PATCH v3 1/1] v4l: event: Add subscription to list before calling "add" operation
Patch ad608fbcf166 changed how events were subscribed to address an issue
elsewhere. As a side effect of that change, the "add" callback was called
before the event subscription was added to the list of subscribed events,
causing the first event queued by the add callback (and possibly other
events arriving soon afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add"
callback, and clean up afterwards if that fails.
Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions
while accessed")
Reported-by: Dave Stevenson
Signed-off-by: Sakari Ailus
---
since v2:
- More accurate commit message. No other changes.
drivers/media/v4l2-core/v4l2-event.c | 43
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index a3ef1f50a4b3..481e3c65cf97 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -193,6 +193,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_event_pending);
+static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
+{
+ struct v4l2_fh *fh = sev->fh;
+ unsigned int i;
+
+ lockdep_assert_held(>subscribe_lock);
+ assert_spin_locked(>vdev->fh_lock);
+
+ /* Remove any pending events for this subscription */
+ for (i = 0; i < sev->in_use; i++) {
+ list_del(>events[sev_pos(sev, i)].list);
+ fh->navailable--;
+ }
+ list_del(>list);
+}
+
int v4l2_event_subscribe(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub, unsigned
elems,
const struct v4l2_subscribed_event_ops *ops)
@@ -224,27 +240,23 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
+ if (!found_ev)
+ list_add(>list, >subscribed);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
if (found_ev) {
/* Already listening */
kvfree(sev);
- goto out_unlock;
- }
-
- if (sev->ops && sev->ops->add) {
+ } else if (sev->ops && sev->ops->add) {
ret = sev->ops->add(sev, elems);
if (ret) {
+ spin_lock_irqsave(>vdev->fh_lock, flags);
+ __v4l2_event_unsubscribe(sev);
+ spin_unlock_irqrestore(>vdev->fh_lock, flags);
kvfree(sev);
- goto out_unlock;
}
}
- spin_lock_irqsave(>vdev->fh_lock, flags);
- list_add(>list, >subscribed);
- spin_unlock_irqrestore(>vdev->fh_lock, flags);
-
-out_unlock:
mutex_unlock(>subscribe_lock);
return ret;
@@ -279,7 +291,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
{
struct v4l2_subscribed_event *sev;
unsigned long flags;
- int i;
if (sub->type == V4L2_EVENT_ALL) {
v4l2_event_unsubscribe_all(fh);
@@ -291,14 +302,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (sev != NULL) {
- /* Remove any pending events for this subscription */
- for (i = 0; i < sev->in_use; i++) {
- list_del(>events[sev_pos(sev, i)].list);
- fh->navailable--;
- }
- list_del(>list);
- }
+ if (sev != NULL)
+ __v4l2_event_unsubscribe(sev);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
--
2.11.0
[PATCH v2 1/1] v4l: event: Add subscription to list before calling "add" operation
Patch ad608fbcf166 changed how events were subscribed to address an issue
elsewhere. As a side effect of that change, the "add" callback was called
before the event subscription was added to the list of subscribed events,
causing the first event (and possibly other events arriving soon
afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add"
callback, and clean up afterwards if that fails.
Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions
while accessed")
Reported-by: Dave Stevenson
Signed-off-by: Sakari Ailus
---
Hi Dave, Hans,
I figured there was room for some refactoring... so I did that.
Functionality-wise it should be equivalent.
drivers/media/v4l2-core/v4l2-event.c | 43
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index a3ef1f50a4b3..481e3c65cf97 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -193,6 +193,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_event_pending);
+static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
+{
+ struct v4l2_fh *fh = sev->fh;
+ unsigned int i;
+
+ lockdep_assert_held(>subscribe_lock);
+ assert_spin_locked(>vdev->fh_lock);
+
+ /* Remove any pending events for this subscription */
+ for (i = 0; i < sev->in_use; i++) {
+ list_del(>events[sev_pos(sev, i)].list);
+ fh->navailable--;
+ }
+ list_del(>list);
+}
+
int v4l2_event_subscribe(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub, unsigned
elems,
const struct v4l2_subscribed_event_ops *ops)
@@ -224,27 +240,23 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
+ if (!found_ev)
+ list_add(>list, >subscribed);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
if (found_ev) {
/* Already listening */
kvfree(sev);
- goto out_unlock;
- }
-
- if (sev->ops && sev->ops->add) {
+ } else if (sev->ops && sev->ops->add) {
ret = sev->ops->add(sev, elems);
if (ret) {
+ spin_lock_irqsave(>vdev->fh_lock, flags);
+ __v4l2_event_unsubscribe(sev);
+ spin_unlock_irqrestore(>vdev->fh_lock, flags);
kvfree(sev);
- goto out_unlock;
}
}
- spin_lock_irqsave(>vdev->fh_lock, flags);
- list_add(>list, >subscribed);
- spin_unlock_irqrestore(>vdev->fh_lock, flags);
-
-out_unlock:
mutex_unlock(>subscribe_lock);
return ret;
@@ -279,7 +291,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
{
struct v4l2_subscribed_event *sev;
unsigned long flags;
- int i;
if (sub->type == V4L2_EVENT_ALL) {
v4l2_event_unsubscribe_all(fh);
@@ -291,14 +302,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (sev != NULL) {
- /* Remove any pending events for this subscription */
- for (i = 0; i < sev->in_use; i++) {
- list_del(>events[sev_pos(sev, i)].list);
- fh->navailable--;
- }
- list_del(>list);
- }
+ if (sev != NULL)
+ __v4l2_event_unsubscribe(sev);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
--
2.11.0
[PATCH 1/1] v4l: event: Add subscription to list before calling "add" operation
Patch ad608fbcf166 changed how events were subscribed to address an issue
elsewhere. As a side effect of that change, the "add" callback was called
before the event subscription was added to the list of subscribed events,
causing the first event (and possibly other events arriving soon
afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add"
callback, and clean up afterwards if that fails.
Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions
while accessed")
Reported-by: Dave Stevenson
Signed-off-by: Sakari Ailus
---
Hi Dave, Hans,
This should address the issue.
The functions can (and probably should) be re-arranged later but let's get
the fix right first.
I haven't tested this using vivid yet as it crashes where I was testing
it. I'll see later if it works elsewhere but I'm sending the patch for
review in the meantime.
drivers/media/v4l2-core/v4l2-event.c | 39 +++-
1 file changed, 25 insertions(+), 14 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index a3ef1f50a4b3..2b6651aeb89b 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -193,6 +193,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_event_pending);
+static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
+{
+ struct v4l2_fh *fh = sev->fh;
+ unsigned int i;
+
+ lockdep_assert_held(>subscribe_lock);
+ assert_spin_locked(>vdev->fh_lock);
+
+ /* Remove any pending events for this subscription */
+ for (i = 0; i < sev->in_use; i++) {
+ list_del(>events[sev_pos(sev, i)].list);
+ fh->navailable--;
+ }
+ list_del(>list);
+}
+
int v4l2_event_subscribe(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub, unsigned
elems,
const struct v4l2_subscribed_event_ops *ops)
@@ -232,18 +248,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
goto out_unlock;
}
+ spin_lock_irqsave(>vdev->fh_lock, flags);
+ list_add(>list, >subscribed);
+ spin_unlock_irqrestore(>vdev->fh_lock, flags);
+
if (sev->ops && sev->ops->add) {
ret = sev->ops->add(sev, elems);
if (ret) {
+ spin_lock_irqsave(>vdev->fh_lock, flags);
+ __v4l2_event_unsubscribe(sev);
+ spin_unlock_irqrestore(>vdev->fh_lock, flags);
kvfree(sev);
- goto out_unlock;
}
}
- spin_lock_irqsave(>vdev->fh_lock, flags);
- list_add(>list, >subscribed);
- spin_unlock_irqrestore(>vdev->fh_lock, flags);
-
out_unlock:
mutex_unlock(>subscribe_lock);
@@ -279,7 +297,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
{
struct v4l2_subscribed_event *sev;
unsigned long flags;
- int i;
if (sub->type == V4L2_EVENT_ALL) {
v4l2_event_unsubscribe_all(fh);
@@ -291,14 +308,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
spin_lock_irqsave(>vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (sev != NULL) {
- /* Remove any pending events for this subscription */
- for (i = 0; i < sev->in_use; i++) {
- list_del(>events[sev_pos(sev, i)].list);
- fh->navailable--;
- }
- list_del(>list);
- }
+ if (sev != NULL)
+ __v4l2_event_unsubscribe(sev);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
--
2.11.0
Re: VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?
out_unlock; > } > } > > - spin_lock_irqsave(>vdev->fh_lock, flags); > - list_add(>list, >subscribed); > - spin_unlock_irqrestore(>vdev->fh_lock, flags); > - > out_unlock: > mutex_unlock(>subscribe_lock); > > Is there a need to iterate the sev->events list and free any > potentially pending events there in the ops->add error path? > We still have the subscribe_lock held, so I don't think that it > reintroduces the issue that the patch was fixing of unsubscribing > before subscribed. The events queued during that time need to be removed. Indeed the issue appears to be that the add op is called before the event is added to the list, and therefore the check whether it's subscribed fails. > > This patch has also been merged to stable, so 4.14 is affected as well. Yes, and the earlier stable kernels as well. -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v7 06/16] intel-ipu3: mmu: Implement driver
Hi Yong,
On Mon, Oct 29, 2018 at 03:23:00PM -0700, Yong Zhi wrote:
> From: Tomasz Figa
>
> This driver translates IO virtual address to physical
> address based on two levels page tables.
>
> Signed-off-by: Tomasz Figa
> Signed-off-by: Yong Zhi
> ---
...
> +static void call_if_ipu3_is_powered(struct ipu3_mmu *mmu,
> + void (*func)(struct ipu3_mmu *mmu))
> +{
> + pm_runtime_get_noresume(mmu->dev);
> + if (pm_runtime_active(mmu->dev))
> + func(mmu);
> + pm_runtime_put(mmu->dev);
How about:
if (!pm_runtime_get_if_in_use(mmu->dev))
return;
func(mmu);
pm_runtime_put(mmu->dev);
> +}
--
Sakari Ailus
sakari.ai...@linux.intel.com
