cron job: media_tree daily build: ERRORS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Fri Aug 3 05:00:11 CEST 2018 media-tree git hash:2c3449fb95c318920ca8dc645d918d408db219ac media_build git hash: e75fbc93fc427f769c3ce5a020bf204e02a45852 v4l-utils git hash: 70b13df426d30ca58c79cf8a366e73463bb22cbb edid-decode git hash: ab18befbcacd6cd4dff63faa82e32700369d6f25 gcc version:i686-linux-gcc (GCC) 8.1.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.16.0-1-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: WARNINGS linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: WARNINGS Check COMPILE_TEST: OK linux-2.6.36.4-i686: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-i686: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-i686: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-i686: OK linux-2.6.39.4-x86_64: OK linux-3.0.101-i686: OK linux-3.0.101-x86_64: OK linux-3.1.10-i686: ERRORS linux-3.1.10-x86_64: ERRORS linux-3.2.102-i686: ERRORS linux-3.2.102-x86_64: ERRORS linux-3.3.8-i686: ERRORS linux-3.3.8-x86_64: ERRORS linux-3.4.113-i686: ERRORS linux-3.4.113-x86_64: ERRORS linux-3.5.7-i686: ERRORS linux-3.5.7-x86_64: ERRORS linux-3.6.11-i686: ERRORS linux-3.6.11-x86_64: ERRORS linux-3.7.10-i686: ERRORS linux-3.7.10-x86_64: ERRORS linux-3.8.13-i686: ERRORS linux-3.8.13-x86_64: ERRORS linux-3.9.11-i686: ERRORS linux-3.9.11-x86_64: ERRORS linux-3.10.108-i686: ERRORS linux-3.10.108-x86_64: ERRORS linux-3.11.10-i686: ERRORS linux-3.11.10-x86_64: ERRORS linux-3.12.74-i686: ERRORS linux-3.12.74-x86_64: ERRORS linux-3.13.11-i686: ERRORS linux-3.13.11-x86_64: ERRORS linux-3.14.79-i686: ERRORS linux-3.14.79-x86_64: ERRORS linux-3.15.10-i686: ERRORS linux-3.15.10-x86_64: ERRORS linux-3.16.57-i686: ERRORS linux-3.16.57-x86_64: ERRORS linux-3.17.8-i686: ERRORS linux-3.17.8-x86_64: ERRORS linux-3.18.115-i686: ERRORS linux-3.18.115-x86_64: ERRORS linux-3.19.8-i686: ERRORS linux-3.19.8-x86_64: ERRORS linux-4.0.9-i686: ERRORS linux-4.0.9-x86_64: ERRORS linux-4.1.52-i686: ERRORS linux-4.1.52-x86_64: ERRORS linux-4.2.8-i686: ERRORS linux-4.2.8-x86_64: ERRORS linux-4.3.6-i686: ERRORS linux-4.3.6-x86_64: ERRORS linux-4.4.140-i686: ERRORS linux-4.4.140-x86_64: ERRORS linux-4.5.7-i686: ERRORS linux-4.5.7-x86_64: ERRORS linux-4.6.7-i686: ERRORS linux-4.6.7-x86_64: ERRORS linux-4.7.10-i686: ERRORS linux-4.7.10-x86_64: ERRORS linux-4.8.17-i686: ERRORS linux-4.8.17-x86_64: ERRORS linux-4.9.112-i686: OK linux-4.9.112-x86_64: OK linux-4.10.17-i686: OK linux-4.10.17-x86_64: OK linux-4.11.12-i686: OK linux-4.11.12-x86_64: OK linux-4.12.14-i686: OK linux-4.12.14-x86_64: OK linux-4.13.16-i686: OK linux-4.13.16-x86_64: OK linux-4.14.55-i686: OK linux-4.14.55-x86_64: OK linux-4.15.18-i686: OK linux-4.15.18-x86_64: OK linux-4.16.18-i686: OK linux-4.16.18-x86_64: OK linux-4.17.6-i686: OK linux-4.17.6-x86_64: OK linux-4.18-rc4-i686: OK linux-4.18-rc4-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Friday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Friday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
RE: [RESEND PATCH v4] media: imx208: Add imx208 camera sensor driver
Hi Tomasz,
>On Fri, Aug 3, 2018 at 11:57 AM Ping-chung Chen
>wrote:
>
> From: "Chen, Ping-chung"
>
> Add a V4L2 sub-device driver for the Sony IMX208 image sensor.
> This is a camera sensor using the I2C bus for control and the
> CSI-2 bus for data.
>
> Signed-off-by: Ping-Chung Chen
> ---
> since v1:
> -- Update the function media_entity_pads_init for upstreaming.
> -- Change the structure name mutex as imx208_mx.
> -- Refine the control flow of test pattern function.
> -- vflip/hflip control support (will impact the output bayer order)
> -- support 4 bayer orders output (via change v/hflip)
> - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10
> -- Simplify error handling in the set_stream function.
> since v2:
> -- Refine coding style.
> -- Fix the if statement to use pm_runtime_get_if_in_use().
> -- Print more error log during error handling.
> -- Remove mutex_destroy() from imx208_free_controls().
> -- Add more comments.
> since v3:
> -- Set explicit indices to link frequencies.
>
[snip]
> +/* Menu items for LINK_FREQ V4L2 control */ static const s64
> +link_freq_menu_items[] = {
> + [IMX208_LINK_FREQ_384MHZ_INDEX] = IMX208_LINK_FREQ_384MHZ,
> + [IMX208_LINK_FREQ_384MHZ_INDEX] = IMX208_LINK_FREQ_96MHZ,
>IMX208_LINK_FREQ_96MHZ_INDEX?
Oh... It's my mistake. Fixed.
>With this fixed (and if there are no other changes), feel free to add
>Reviewed-by: Tomasz Figa
Sure.
>Best regards,
>Tomasz
Re: [RESEND PATCH v4] media: imx208: Add imx208 camera sensor driver
On Fri, Aug 3, 2018 at 11:57 AM Ping-chung Chen
wrote:
>
> From: "Chen, Ping-chung"
>
> Add a V4L2 sub-device driver for the Sony IMX208 image sensor.
> This is a camera sensor using the I2C bus for control and the
> CSI-2 bus for data.
>
> Signed-off-by: Ping-Chung Chen
> ---
> since v1:
> -- Update the function media_entity_pads_init for upstreaming.
> -- Change the structure name mutex as imx208_mx.
> -- Refine the control flow of test pattern function.
> -- vflip/hflip control support (will impact the output bayer order)
> -- support 4 bayer orders output (via change v/hflip)
> - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10
> -- Simplify error handling in the set_stream function.
> since v2:
> -- Refine coding style.
> -- Fix the if statement to use pm_runtime_get_if_in_use().
> -- Print more error log during error handling.
> -- Remove mutex_destroy() from imx208_free_controls().
> -- Add more comments.
> since v3:
> -- Set explicit indices to link frequencies.
>
[snip]
> +/* Menu items for LINK_FREQ V4L2 control */
> +static const s64 link_freq_menu_items[] = {
> + [IMX208_LINK_FREQ_384MHZ_INDEX] = IMX208_LINK_FREQ_384MHZ,
> + [IMX208_LINK_FREQ_384MHZ_INDEX] = IMX208_LINK_FREQ_96MHZ,
IMX208_LINK_FREQ_96MHZ_INDEX?
With this fixed (and if there are no other changes), feel free to add
Reviewed-by: Tomasz Figa
Best regards,
Tomasz
[RESEND PATCH v4] media: imx208: Add imx208 camera sensor driver
From: "Chen, Ping-chung" Add a V4L2 sub-device driver for the Sony IMX208 image sensor. This is a camera sensor using the I2C bus for control and the CSI-2 bus for data. Signed-off-by: Ping-Chung Chen --- since v1: -- Update the function media_entity_pads_init for upstreaming. -- Change the structure name mutex as imx208_mx. -- Refine the control flow of test pattern function. -- vflip/hflip control support (will impact the output bayer order) -- support 4 bayer orders output (via change v/hflip) - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 -- Simplify error handling in the set_stream function. since v2: -- Refine coding style. -- Fix the if statement to use pm_runtime_get_if_in_use(). -- Print more error log during error handling. -- Remove mutex_destroy() from imx208_free_controls(). -- Add more comments. since v3: -- Set explicit indices to link frequencies. MAINTAINERS| 7 + drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/imx208.c | 995 + 4 files changed, 1014 insertions(+) create mode 100644 drivers/media/i2c/imx208.c diff --git a/MAINTAINERS b/MAINTAINERS index bbd9b9b..896c1df 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13268,6 +13268,13 @@ S: Maintained F: drivers/ssb/ F: include/linux/ssb/ +SONY IMX208 SENSOR DRIVER +M: Sakari Ailus +L: linux-media@vger.kernel.org +T: git git://linuxtv.org/media_tree.git +S: Maintained +F: drivers/media/i2c/imx208.c + SONY IMX258 SENSOR DRIVER M: Sakari Ailus L: linux-media@vger.kernel.org diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 8669853..ae11f1e 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -595,6 +595,17 @@ config VIDEO_APTINA_PLL config VIDEO_SMIAPP_PLL tristate +config VIDEO_IMX208 + tristate "Sony IMX208 sensor support" + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API + depends on MEDIA_CAMERA_SUPPORT + ---help--- + This is a Video4Linux2 sensor driver for the Sony + IMX208 camera. + + To compile this driver as a module, choose M here: the + module will be called imx208. + config VIDEO_IMX258 tristate "Sony IMX258 sensor support" depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 837c428..47604c2 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -104,6 +104,7 @@ obj-$(CONFIG_VIDEO_I2C) += video-i2c.o obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o obj-$(CONFIG_VIDEO_OV2659) += ov2659.o obj-$(CONFIG_VIDEO_TC358743) += tc358743.o +obj-$(CONFIG_VIDEO_IMX208) += imx208.o obj-$(CONFIG_VIDEO_IMX258) += imx258.o obj-$(CONFIG_VIDEO_IMX274) += imx274.o diff --git a/drivers/media/i2c/imx208.c b/drivers/media/i2c/imx208.c new file mode 100644 index 000..9e374c1 --- /dev/null +++ b/drivers/media/i2c/imx208.c @@ -0,0 +1,995 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2018 Intel Corporation + +#include +#include +#include +#include +#include +#include +#include +#include + +#define IMX208_REG_MODE_SELECT 0x0100 +#define IMX208_MODE_STANDBY0x00 +#define IMX208_MODE_STREAMING 0x01 + +/* Chip ID */ +#define IMX208_REG_CHIP_ID 0x +#define IMX208_CHIP_ID 0x0208 + +/* V_TIMING internal */ +#define IMX208_REG_VTS 0x0340 +#define IMX208_VTS_60FPS 0x0472 +#define IMX208_VTS_BINNING 0x0239 +#define IMX208_VTS_60FPS_MIN 0x0458 +#define IMX208_VTS_BINNING_MIN 0x0230 +#define IMX208_VTS_MAX 0x + +/* HBLANK control - read only */ +#define IMX208_PPL_384MHZ 2248 +#define IMX208_PPL_96MHZ 2248 + +/* Exposure control */ +#define IMX208_REG_EXPOSURE0x0202 +#define IMX208_EXPOSURE_MIN4 +#define IMX208_EXPOSURE_STEP 1 +#define IMX208_EXPOSURE_DEFAULT0x190 +#define IMX208_EXPOSURE_MAX65535 + +/* Analog gain control */ +#define IMX208_REG_ANALOG_GAIN 0x0204 +#define IMX208_ANA_GAIN_MIN0 +#define IMX208_ANA_GAIN_MAX0x00e0 +#define IMX208_ANA_GAIN_STEP 1 +#define IMX208_ANA_GAIN_DEFAULT0x0 + +/* Digital gain control */ +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e +#define IMX208_REG_R_DIGITAL_GAIN 0x0210 +#define IMX208_REG_B_DIGITAL_GAIN 0x0212 +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214 +#define IMX208_DGTL_GAIN_MIN 0 +#define IMX208_DGTL_GAIN_MAX 4096 +#define IMX208_DGTL_GAIN_DEFAULT 0x100 +#define IMX208_DGTL_GAIN_STEP 1 + +/* Orientation */ +#define IMX208_REG_ORIENTATION_CONTROL 0x0101 + +/* Test Pattern Control */ +#define IMX208_REG_TEST_PATTERN_MODE 0x0600 +#define
[PATCH v4] media: imx208: Add imx208 camera sensor driver
From: "Chen, Ping-chung" Add a V4L2 sub-device driver for the Sony IMX208 image sensor. This is a camera sensor using the I2C bus for control and the CSI-2 bus for data. Signed-off-by: Ping-Chung Chen --- since v1: -- Update the function media_entity_pads_init for upstreaming. -- Change the structure name mutex as imx208_mx. -- Refine the control flow of test pattern function. -- vflip/hflip control support (will impact the output bayer order) -- support 4 bayer orders output (via change v/hflip) - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 -- Simplify error handling in the set_stream function. since v2: -- Refine coding style. -- Fix the if statement to use pm_runtime_get_if_in_use(). -- Print more error log during error handling. -- Remove mutex_destroy() from imx208_free_controls(). -- Add more comments. MAINTAINERS| 7 + drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/imx208.c | 995 + 4 files changed, 1014 insertions(+) create mode 100644 drivers/media/i2c/imx208.c diff --git a/MAINTAINERS b/MAINTAINERS index bbd9b9b..896c1df 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13268,6 +13268,13 @@ S: Maintained F: drivers/ssb/ F: include/linux/ssb/ +SONY IMX208 SENSOR DRIVER +M: Sakari Ailus +L: linux-media@vger.kernel.org +T: git git://linuxtv.org/media_tree.git +S: Maintained +F: drivers/media/i2c/imx208.c + SONY IMX258 SENSOR DRIVER M: Sakari Ailus L: linux-media@vger.kernel.org diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 8669853..ae11f1e 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -595,6 +595,17 @@ config VIDEO_APTINA_PLL config VIDEO_SMIAPP_PLL tristate +config VIDEO_IMX208 + tristate "Sony IMX208 sensor support" + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API + depends on MEDIA_CAMERA_SUPPORT + ---help--- + This is a Video4Linux2 sensor driver for the Sony + IMX208 camera. + + To compile this driver as a module, choose M here: the + module will be called imx208. + config VIDEO_IMX258 tristate "Sony IMX258 sensor support" depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 837c428..47604c2 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -104,6 +104,7 @@ obj-$(CONFIG_VIDEO_I2C) += video-i2c.o obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o obj-$(CONFIG_VIDEO_OV2659) += ov2659.o obj-$(CONFIG_VIDEO_TC358743) += tc358743.o +obj-$(CONFIG_VIDEO_IMX208) += imx208.o obj-$(CONFIG_VIDEO_IMX258) += imx258.o obj-$(CONFIG_VIDEO_IMX274) += imx274.o diff --git a/drivers/media/i2c/imx208.c b/drivers/media/i2c/imx208.c new file mode 100644 index 000..9e374c1 --- /dev/null +++ b/drivers/media/i2c/imx208.c @@ -0,0 +1,995 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2018 Intel Corporation + +#include +#include +#include +#include +#include +#include +#include +#include + +#define IMX208_REG_MODE_SELECT 0x0100 +#define IMX208_MODE_STANDBY0x00 +#define IMX208_MODE_STREAMING 0x01 + +/* Chip ID */ +#define IMX208_REG_CHIP_ID 0x +#define IMX208_CHIP_ID 0x0208 + +/* V_TIMING internal */ +#define IMX208_REG_VTS 0x0340 +#define IMX208_VTS_60FPS 0x0472 +#define IMX208_VTS_BINNING 0x0239 +#define IMX208_VTS_60FPS_MIN 0x0458 +#define IMX208_VTS_BINNING_MIN 0x0230 +#define IMX208_VTS_MAX 0x + +/* HBLANK control - read only */ +#define IMX208_PPL_384MHZ 2248 +#define IMX208_PPL_96MHZ 2248 + +/* Exposure control */ +#define IMX208_REG_EXPOSURE0x0202 +#define IMX208_EXPOSURE_MIN4 +#define IMX208_EXPOSURE_STEP 1 +#define IMX208_EXPOSURE_DEFAULT0x190 +#define IMX208_EXPOSURE_MAX65535 + +/* Analog gain control */ +#define IMX208_REG_ANALOG_GAIN 0x0204 +#define IMX208_ANA_GAIN_MIN0 +#define IMX208_ANA_GAIN_MAX0x00e0 +#define IMX208_ANA_GAIN_STEP 1 +#define IMX208_ANA_GAIN_DEFAULT0x0 + +/* Digital gain control */ +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e +#define IMX208_REG_R_DIGITAL_GAIN 0x0210 +#define IMX208_REG_B_DIGITAL_GAIN 0x0212 +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214 +#define IMX208_DGTL_GAIN_MIN 0 +#define IMX208_DGTL_GAIN_MAX 4096 +#define IMX208_DGTL_GAIN_DEFAULT 0x100 +#define IMX208_DGTL_GAIN_STEP 1 + +/* Orientation */ +#define IMX208_REG_ORIENTATION_CONTROL 0x0101 + +/* Test Pattern Control */ +#define IMX208_REG_TEST_PATTERN_MODE 0x0600 +#define IMX208_TEST_PATTERN_DISABLE0x0 +#define
edit your photos
As a boutique team, we work personally with our clients to ensure the good results. Having over a decade of hands-on experience in photography and retouching, we have been inspired to expand our services to the public. We are team of artists who have excelled in fields such as art, photography, retouching, graphics and design. No matter what your field of interest is in, we can learn to work with your style to give you the best product. We provide below image editing services: Clipping path Image cut out Image shadow creation Image masking Photo retouching Beauty retouching (skin, face, body retouching) Glamour retouching Product retouching Color correction and others We provide testing for our services. Thanks, Sam
[PATCH v4 3/6] v4l2-mem2mem: Avoid v4l2_m2m_prepare_buf from scheduling a job
There is no need for v4l2_m2m_prepare_buf to try to schedule a job,
as it only prepares a buffer, but does not queue or changes the
state of the queue.
Remove the call to v4l2_m2m_try_schedule from v4l2_m2m_prepare_buf.
Signed-off-by: Ezequiel Garcia
---
drivers/media/v4l2-core/v4l2-mem2mem.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
b/drivers/media/v4l2-core/v4l2-mem2mem.c
index b7005894292c..6bdbdbfa8e6c 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -481,14 +481,9 @@ int v4l2_m2m_prepare_buf(struct file *file, struct
v4l2_m2m_ctx *m2m_ctx,
struct v4l2_buffer *buf)
{
struct vb2_queue *vq;
- int ret;
vq = v4l2_m2m_get_vq(m2m_ctx, buf->type);
- ret = vb2_prepare_buf(vq, buf);
- if (!ret)
- v4l2_m2m_try_schedule(m2m_ctx);
-
- return ret;
+ return vb2_prepare_buf(vq, buf);
}
EXPORT_SYMBOL_GPL(v4l2_m2m_prepare_buf);
--
2.18.0
[PATCH v4 5/6] v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
v4l2_m2m_job_finish() is typically called in interrupt context.
Some implementation of .device_run might sleep, and so it's
desirable to avoid calling it directly from
v4l2_m2m_job_finish(), thus avoiding .device_run from running
in interrupt context.
Implement a deferred context that calls v4l2_m2m_try_run,
and gets scheduled by v4l2_m2m_job_finish().
Signed-off-by: Ezequiel Garcia
---
drivers/media/v4l2-core/v4l2-mem2mem.c | 46 +++---
1 file changed, 42 insertions(+), 4 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 04e2c8357863..020b2d8621d0 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -69,6 +69,7 @@ static const char * const m2m_entity_name[] = {
* @curr_ctx: currently running instance
* @job_queue: instances queued to run
* @job_spinlock: protects job_queue
+ * @job_work: worker to run queued jobs.
* @m2m_ops: driver callbacks
*/
struct v4l2_m2m_dev {
@@ -85,6 +86,7 @@ struct v4l2_m2m_dev {
struct list_headjob_queue;
spinlock_t job_spinlock;
+ struct work_struct job_work;
const struct v4l2_m2m_ops *m2m_ops;
};
@@ -224,10 +226,11 @@ EXPORT_SYMBOL(v4l2_m2m_get_curr_priv);
/**
* v4l2_m2m_try_run() - select next job to perform and run it if possible
* @m2m_dev: per-device context
+ * @try_lock: indicates if the queue lock should be taken
*
* Get next transaction (if present) from the waiting jobs list and run it.
*/
-static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
+static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev, bool try_lock)
{
unsigned long flags;
@@ -250,7 +253,20 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
spin_unlock_irqrestore(_dev->job_spinlock, flags);
dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
+
+ /*
+* A m2m context lock is taken only after a m2m context
+* is picked from the queue and marked as running.
+* The lock is only needed if v4l2_m2m_try_run is called
+* from the async worker.
+*/
+ if (try_lock && m2m_dev->curr_ctx->q_lock)
+ mutex_lock(m2m_dev->curr_ctx->q_lock);
+
m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
+
+ if (try_lock && m2m_dev->curr_ctx->q_lock)
+ mutex_unlock(m2m_dev->curr_ctx->q_lock);
}
/*
@@ -330,7 +346,8 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev
*m2m_dev,
* Check if this context is ready to queue a job. If suitable,
* run the next queued job on the mem2mem device.
*
- * This function shouldn't run in interrupt context.
+ * This function shouldn't run in interrupt context, and must be called
+ * with the v4l2_m2m_ctx.q_lock mutex held.
*
* Note that v4l2_m2m_try_schedule() can schedule one job for this context,
* and then run another job for another context.
@@ -339,11 +356,26 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
{
struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
+ if (m2m_ctx->q_lock)
+ WARN_ON(!mutex_is_locked(m2m_ctx->q_lock));
+
__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
- v4l2_m2m_try_run(m2m_dev);
+ v4l2_m2m_try_run(m2m_dev, false);
}
EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
+/**
+ * v4l2_m2m_device_run_work() - run pending jobs for the context
+ * @work: Work structure used for scheduling the execution of this function.
+ */
+static void v4l2_m2m_device_run_work(struct work_struct *work)
+{
+ struct v4l2_m2m_dev *m2m_dev =
+ container_of(work, struct v4l2_m2m_dev, job_work);
+
+ v4l2_m2m_try_run(m2m_dev, true);
+}
+
/**
* v4l2_m2m_cancel_job() - cancel pending jobs for the context
* @m2m_ctx: m2m context with jobs to be canceled
@@ -403,7 +435,12 @@ void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
/* This instance might have more buffers ready, but since we do not
* allow more than one job on the job_queue per instance, each has
* to be scheduled separately after the previous one finishes. */
- v4l2_m2m_try_schedule(m2m_ctx);
+ __v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
+
+ /* We might be running in atomic context,
+* but the job must be run in non-atomic context.
+*/
+ schedule_work(_dev->job_work);
}
EXPORT_SYMBOL(v4l2_m2m_job_finish);
@@ -837,6 +874,7 @@ struct v4l2_m2m_dev *v4l2_m2m_init(const struct
v4l2_m2m_ops *m2m_ops)
m2m_dev->m2m_ops = m2m_ops;
INIT_LIST_HEAD(_dev->job_queue);
spin_lock_init(_dev->job_spinlock);
+ INIT_WORK(_dev->job_work, v4l2_m2m_device_run_work);
return m2m_dev;
}
--
2.18.0
[PATCH v4 6/6] selftests: media_tests: Add a memory-to-memory concurrent stress test
Add a test for the memory-to-memory framework, to exercise the
scheduling of concurrent jobs, using multiple contexts.
This test needs to be run using the vim2m virtual driver,
and so needs no hardware.
While here, rework the media_tests suite in order to make it
useful for automatic tools. Those tests that need human intervention
are now separated from those that can run automatically, needing
only virtual drivers to work.
Signed-off-by: Ezequiel Garcia
---
.../testing/selftests/media_tests/.gitignore | 1 +
tools/testing/selftests/media_tests/Makefile | 5 +-
.../selftests/media_tests/m2m_job_test.c | 287 ++
.../selftests/media_tests/m2m_job_test.sh | 32 ++
4 files changed, 324 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/media_tests/m2m_job_test.c
create mode 100755 tools/testing/selftests/media_tests/m2m_job_test.sh
diff --git a/tools/testing/selftests/media_tests/.gitignore
b/tools/testing/selftests/media_tests/.gitignore
index 8745eba39012..71c6508348ce 100644
--- a/tools/testing/selftests/media_tests/.gitignore
+++ b/tools/testing/selftests/media_tests/.gitignore
@@ -1,3 +1,4 @@
media_device_test
media_device_open
video_device_test
+m2m_job_test
diff --git a/tools/testing/selftests/media_tests/Makefile
b/tools/testing/selftests/media_tests/Makefile
index 60826d7d37d4..d25d4c3eb7d2 100644
--- a/tools/testing/selftests/media_tests/Makefile
+++ b/tools/testing/selftests/media_tests/Makefile
@@ -1,6 +1,9 @@
# SPDX-License-Identifier: GPL-2.0
#
CFLAGS += -I../ -I../../../../usr/include/
-TEST_GEN_PROGS := media_device_test media_device_open video_device_test
+TEST_GEN_PROGS_EXTENDED := media_device_test media_device_open
video_device_test m2m_job_test
+TEST_PROGS := m2m_job_test.sh
include ../lib.mk
+
+LDLIBS += -lpthread
diff --git a/tools/testing/selftests/media_tests/m2m_job_test.c
b/tools/testing/selftests/media_tests/m2m_job_test.c
new file mode 100644
index ..5800269567e6
--- /dev/null
+++ b/tools/testing/selftests/media_tests/m2m_job_test.c
@@ -0,0 +1,287 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) Collabora, Ltd.
+
+/*
+ * This file adds a test for the memory-to-memory
+ * framework.
+ *
+ * This test opens a user specified video device and then
+ * queues concurrent jobs. The jobs are totally dummy,
+ * its purpose is only to verify that each of the queued
+ * jobs is run, and is run only once.
+ *
+ * The vim2m driver is needed in order to run the test.
+ *
+ * Usage:
+ * ./m2m-job-test -d /dev/videoX
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#include "../kselftest.h"
+
+#define V4L2_CID_TRANS_TIME_MSEC(V4L2_CID_USER_BASE + 0x1000)
+#define V4L2_CID_TRANS_NUM_BUFS (V4L2_CID_USER_BASE + 0x1001)
+
+#define MAX_TRANS_TIME_MSEC 500
+#define MAX_COUNT 50
+#define MAX_BUFFERS 5
+#define W 10
+#define H 10
+
+#ifndef DEBUG
+#define dprintf(fmt, arg...) \
+ do {\
+ } while (0)
+#else
+#define dprintf(fmt, arg...) printf(fmt, ## arg)
+#endif
+
+static char video_device[256];
+static int thread_count;
+
+struct context {
+ int vfd;
+ unsigned int width;
+ unsigned int height;
+ int buffers;
+};
+
+static int req_src_buf(struct context *ctx, int buffers)
+{
+ struct v4l2_requestbuffers reqbuf;
+ struct v4l2_buffer buf;
+ int i, ret;
+
+ memset(, 0, sizeof(reqbuf));
+ memset(, 0, sizeof(buf));
+
+ reqbuf.count= buffers;
+ reqbuf.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+ reqbuf.memory = V4L2_MEMORY_MMAP;
+ ret = ioctl(ctx->vfd, VIDIOC_REQBUFS, );
+ if (ret)
+ return ret;
+
+ for (i = 0; i < buffers; i++) {
+ buf.type= V4L2_BUF_TYPE_VIDEO_OUTPUT;
+ buf.memory = V4L2_MEMORY_MMAP;
+ buf.index = i;
+ ret = ioctl(ctx->vfd, VIDIOC_QUERYBUF, );
+ if (ret)
+ return ret;
+ buf.bytesused = W*H*2;
+ ret = ioctl(ctx->vfd, VIDIOC_QBUF, );
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int req_dst_buf(struct context *ctx, int buffers)
+{
+ struct v4l2_requestbuffers reqbuf;
+ struct v4l2_buffer buf;
+ int i, ret;
+
+ memset(, 0, sizeof(reqbuf));
+ memset(, 0, sizeof(buf));
+
+ reqbuf.count= buffers;
+ reqbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+ reqbuf.memory = V4L2_MEMORY_MMAP;
+
+ ret = ioctl(ctx->vfd, VIDIOC_REQBUFS, );
+ if (ret)
+ return ret;
+
+ for (i = 0; i < buffers; i++) {
+ buf.type= V4L2_BUF_TYPE_VIDEO_CAPTURE;
+ buf.memory = V4L2_MEMORY_MMAP;
+ buf.index
[PATCH v4 4/6] v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule
From: Sakari Ailus
The __v4l2_m2m_try_schedule function acquires and releases multiple
spinlocks. Simplify unlocking the job lock by adding labels to unlock
the lock and exit the function.
Signed-off-by: Sakari Ailus
Signed-off-by: Ezequiel Garcia
---
drivers/media/v4l2-core/v4l2-mem2mem.c | 29 --
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 6bdbdbfa8e6c..04e2c8357863 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -279,51 +279,48 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev
*m2m_dev,
/* If the context is aborted then don't schedule it */
if (m2m_ctx->job_flags & TRANS_ABORT) {
- spin_unlock_irqrestore(_dev->job_spinlock, flags_job);
dprintk("Aborted context\n");
- return;
+ goto job_unlock;
}
if (m2m_ctx->job_flags & TRANS_QUEUED) {
- spin_unlock_irqrestore(_dev->job_spinlock, flags_job);
dprintk("On job queue already\n");
- return;
+ goto job_unlock;
}
spin_lock_irqsave(_ctx->out_q_ctx.rdy_spinlock, flags_out);
if (list_empty(_ctx->out_q_ctx.rdy_queue)
&& !m2m_ctx->out_q_ctx.buffered) {
- spin_unlock_irqrestore(_ctx->out_q_ctx.rdy_spinlock,
- flags_out);
- spin_unlock_irqrestore(_dev->job_spinlock, flags_job);
dprintk("No input buffers available\n");
- return;
+ goto out_unlock;
}
spin_lock_irqsave(_ctx->cap_q_ctx.rdy_spinlock, flags_cap);
if (list_empty(_ctx->cap_q_ctx.rdy_queue)
&& !m2m_ctx->cap_q_ctx.buffered) {
- spin_unlock_irqrestore(_ctx->cap_q_ctx.rdy_spinlock,
- flags_cap);
- spin_unlock_irqrestore(_ctx->out_q_ctx.rdy_spinlock,
- flags_out);
- spin_unlock_irqrestore(_dev->job_spinlock, flags_job);
dprintk("No output buffers available\n");
- return;
+ goto cap_unlock;
}
spin_unlock_irqrestore(_ctx->cap_q_ctx.rdy_spinlock, flags_cap);
spin_unlock_irqrestore(_ctx->out_q_ctx.rdy_spinlock, flags_out);
if (m2m_dev->m2m_ops->job_ready
&& (!m2m_dev->m2m_ops->job_ready(m2m_ctx->priv))) {
- spin_unlock_irqrestore(_dev->job_spinlock, flags_job);
dprintk("Driver not ready\n");
- return;
+ goto job_unlock;
}
list_add_tail(_ctx->queue, _dev->job_queue);
m2m_ctx->job_flags |= TRANS_QUEUED;
spin_unlock_irqrestore(_dev->job_spinlock, flags_job);
+ return;
+
+cap_unlock:
+ spin_unlock_irqrestore(_ctx->cap_q_ctx.rdy_spinlock, flags_cap);
+out_unlock:
+ spin_unlock_irqrestore(_ctx->out_q_ctx.rdy_spinlock, flags_out);
+job_unlock:
+ spin_unlock_irqrestore(_dev->job_spinlock, flags_job);
}
/**
--
2.18.0
[PATCH v4 2/6] v4l2-ioctl.c: simplify locking for m2m devices
Now that the mutexes for output and capture vb2 queues match,
it is possible to refer to the context q_lock as the
m2m lock for a given m2m context.
Remove the output/capture lock selection.
Signed-off-by: Ezequiel Garcia
---
drivers/media/v4l2-core/v4l2-ioctl.c | 47 ++--
1 file changed, 2 insertions(+), 45 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
b/drivers/media/v4l2-core/v4l2-ioctl.c
index 54afc9c7ee6e..c46c455df652 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2677,45 +2677,6 @@ static bool v4l2_is_known_ioctl(unsigned int cmd)
return v4l2_ioctls[_IOC_NR(cmd)].ioctl == cmd;
}
-#if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
-static bool v4l2_ioctl_m2m_queue_is_output(unsigned int cmd, void *arg)
-{
- switch (cmd) {
- case VIDIOC_CREATE_BUFS: {
- struct v4l2_create_buffers *cbufs = arg;
-
- return V4L2_TYPE_IS_OUTPUT(cbufs->format.type);
- }
- case VIDIOC_REQBUFS: {
- struct v4l2_requestbuffers *rbufs = arg;
-
- return V4L2_TYPE_IS_OUTPUT(rbufs->type);
- }
- case VIDIOC_QBUF:
- case VIDIOC_DQBUF:
- case VIDIOC_QUERYBUF:
- case VIDIOC_PREPARE_BUF: {
- struct v4l2_buffer *buf = arg;
-
- return V4L2_TYPE_IS_OUTPUT(buf->type);
- }
- case VIDIOC_EXPBUF: {
- struct v4l2_exportbuffer *expbuf = arg;
-
- return V4L2_TYPE_IS_OUTPUT(expbuf->type);
- }
- case VIDIOC_STREAMON:
- case VIDIOC_STREAMOFF: {
- int *type = arg;
-
- return V4L2_TYPE_IS_OUTPUT(*type);
- }
- default:
- return false;
- }
-}
-#endif
-
static struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev,
struct v4l2_fh *vfh, unsigned int cmd,
void *arg)
@@ -2725,12 +2686,8 @@ static struct mutex *v4l2_ioctl_get_lock(struct
video_device *vdev,
#if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
if (vfh && vfh->m2m_ctx &&
(v4l2_ioctls[_IOC_NR(cmd)].flags & INFO_FL_QUEUE)) {
- bool is_output = v4l2_ioctl_m2m_queue_is_output(cmd, arg);
- struct v4l2_m2m_queue_ctx *ctx = is_output ?
- >m2m_ctx->out_q_ctx : >m2m_ctx->cap_q_ctx;
-
- if (ctx->q.lock)
- return ctx->q.lock;
+ if (vfh->m2m_ctx->q_lock)
+ return vfh->m2m_ctx->q_lock;
}
#endif
if (vdev->queue && vdev->queue->lock &&
--
2.18.0
[PATCH v4 1/6] mem2mem: Require capture and output mutexes to match
Currently, all the mem2mem driver either use a single mutex
to lock the capture and output videobuf2 queues, or don't
set any mutex.
This means the mutexes match, and so the mem2mem framework
is able to set the m2m context lock.
Enforce this by making it mandatory for drivers to set
the same capture and output mutex, or not set any mutex at all.
Signed-off-by: Ezequiel Garcia
---
drivers/media/v4l2-core/v4l2-mem2mem.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 0a93c5b173c2..b7005894292c 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -887,12 +887,14 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct
v4l2_m2m_dev *m2m_dev,
if (ret)
goto err;
/*
-* If both queues use same mutex assign it as the common buffer
-* queues lock to the m2m context. This lock is used in the
-* v4l2_m2m_ioctl_* helpers.
+* Both queues should use same the mutex to lock the m2m context.
+* This lock is used in some v4l2_m2m_* helpers.
*/
- if (out_q_ctx->q.lock == cap_q_ctx->q.lock)
- m2m_ctx->q_lock = out_q_ctx->q.lock;
+ if (WARN_ON(out_q_ctx->q.lock != cap_q_ctx->q.lock)) {
+ ret = -EINVAL;
+ goto err;
+ }
+ m2m_ctx->q_lock = out_q_ctx->q.lock;
return m2m_ctx;
err:
--
2.18.0
[PATCH v4 0/6] Make sure .device_run is always called in non-atomic context
v4: * Add patches and 1 and 2, to make q_lock mandatory, in other words, require output and capture locks to match. * Add WARN_ON if the lock is not held in v4l2_m2m_try_schedule, and also document the requirement. * Add a comment explaining why the job is scheduled. This series goal is to avoid drivers from having ad-hoc code to call .device_run in non-atomic context. Currently, .device_run can be called via v4l2_m2m_job_finish(), potentially running in interrupt context. This series will be useful for the upcoming Request API, where drivers typically require .device_run to be called in non-atomic context for v4l2_ctrl_request_setup() calls. The solution is to add a per-device worker that is scheduled by v4l2_m2m_job_finish, which replaces drivers having a threaded interrupt or similar. This change allows v4l2_m2m_job_finish() to be called in interrupt context, separating .device_run and v4l2_m2m_job_finish() contexts. It's worth mentioning that v4l2_m2m_cancel_job() doesn't need to flush or cancel the new worker, because the job_spinlock synchronizes both and also because the core prevents simultaneous jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the worker will be unable to run a new job. Testing --- In order to test the change, and make sure no regressions are introduced, a kselftest test is added to stress the mem2mem framework. Note that this series rework the kselftest media_tests target. Those tests that need hardware and human intervention are now marked as _EXTENDED, and a frontend script is added to run those tests that can run without hardware or human intervention. This will allow the media_tests target to be included in automatic regression testing setups. Hopefully, we will be able to introduce more and more automatic regression tests. Currently, our self-test run looks like: $ make TARGETS=media_tests kselftest make[1]: Entering directory '/home/zeta/repos/builds/virtme-x86_64' make[3]: warning: jobserver unavailable: using -j1. Add '+' to parent make rule. make[3]: Nothing to be done for 'all'. make[3]: warning: jobserver unavailable: using -j1. Add '+' to parent make rule. TAP version 13 selftests: media_tests: m2m_job_test.sh --- running media tests --- media_device : no video4linux drivers loaded, vim2m is needed not ok 1..1 selftests: media_tests: m2m_job_test.sh [SKIP] make[1]: Leaving directory '/home/zeta/repos/builds/virtme-x86_64' Ezequiel Garcia (5): mem2mem: Require capture and output mutexes to match v4l2-ioctl.c: simplify locking for m2m devices v4l2-mem2mem: Avoid v4l2_m2m_prepare_buf from scheduling a job v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish selftests: media_tests: Add a memory-to-memory concurrent stress test Sakari Ailus (1): v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule drivers/media/v4l2-core/v4l2-ioctl.c | 47 +-- drivers/media/v4l2-core/v4l2-mem2mem.c| 94 -- .../testing/selftests/media_tests/.gitignore | 1 + tools/testing/selftests/media_tests/Makefile | 5 +- .../selftests/media_tests/m2m_job_test.c | 287 ++ .../selftests/media_tests/m2m_job_test.sh | 32 ++ 6 files changed, 389 insertions(+), 77 deletions(-) create mode 100644 tools/testing/selftests/media_tests/m2m_job_test.c create mode 100755 tools/testing/selftests/media_tests/m2m_job_test.sh -- 2.18.0
[PATCH v2 2/6] ARM: dts: rockchip: add VPU device node for RK3288
Add the Video Processing Unit node for RK3288 SoC.
Also, enable the VPU IOMMU node, as it's needed
for any user that wants to use the VPU with the IOMMU.
Signed-off-by: Ezequiel Garcia
---
arch/arm/boot/dts/rk3288.dtsi | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 0840ffb3205c..a86f0c7d8e71 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -1223,6 +1223,18 @@
};
};
+ vpu: video-codec@ff9a {
+ compatible = "rockchip,rk3288-vpu";
+ reg = <0x0 0xff9a 0x0 0x800>;
+ interrupts = ,
+;
+ interrupt-names = "vepu", "vdpu";
+ clocks = < ACLK_VCODEC>, < HCLK_VCODEC>;
+ clock-names = "aclk", "hclk";
+ power-domains = < RK3288_PD_VIDEO>;
+ iommus = <_mmu>;
+ };
+
vpu_mmu: iommu@ff9a0800 {
compatible = "rockchip,iommu";
reg = <0x0 0xff9a0800 0x0 0x100>;
@@ -1231,7 +1243,6 @@
clocks = < ACLK_VCODEC>, < HCLK_VCODEC>;
clock-names = "aclk", "iface";
#iommu-cells = <0>;
- status = "disabled";
};
hevc_mmu: iommu@ff9c0440 {
--
2.18.0
[PATCH v2 1/6] dt-bindings: Document the Rockchip VPU bindings
Add devicetree binding documentation for Rockchip Video Processing
Unit IP.
Signed-off-by: Ezequiel Garcia
---
.../bindings/media/rockchip-vpu.txt | 30 +++
1 file changed, 30 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/rockchip-vpu.txt
diff --git a/Documentation/devicetree/bindings/media/rockchip-vpu.txt
b/Documentation/devicetree/bindings/media/rockchip-vpu.txt
new file mode 100644
index ..5e0d421301ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/rockchip-vpu.txt
@@ -0,0 +1,30 @@
+device-tree bindings for rockchip VPU codec
+
+Rockchip (Video Processing Unit) present in various Rockchip platforms,
+such as RK3288 and RK3399.
+
+Required properties:
+- compatible: value should be one of the following
+ "rockchip,rk3288-vpu";
+ "rockchip,rk3399-vpu";
+- interrupts: encoding and decoding interrupt specifiers
+- interrupt-names: should be "vepu" and "vdpu"
+- clocks: phandle to VPU aclk, hclk clocks
+- clock-names: should be "aclk" and "hclk"
+- power-domains: phandle to power domain node
+- iommus: phandle to a iommu node
+
+Example:
+SoC-specific DT entry:
+ vpu: video-codec@ff9a {
+ compatible = "rockchip,rk3288-vpu";
+ reg = <0x0 0xff9a 0x0 0x800>;
+ interrupts = ,
+;
+ interrupt-names = "vepu", "vdpu";
+ clocks = < ACLK_VCODEC>, < HCLK_VCODEC>;
+ clock-names = "aclk", "hclk";
+ power-domains = < RK3288_PD_VIDEO>;
+ iommus = <_mmu>;
+ };
+
--
2.18.0
[PATCH v2 6/6] media: add Rockchip VPU driver
Add a mem2mem driver for the VPU available on Rockchip SoCs. Currently only JPEG encoding is supported, for RK3399 and RK3288 platforms. Signed-off-by: Ezequiel Garcia --- MAINTAINERS | 7 + drivers/media/platform/Kconfig| 13 + drivers/media/platform/Makefile | 1 + drivers/media/platform/rockchip/vpu/Makefile | 8 + .../platform/rockchip/vpu/rk3288_vpu_hw.c | 122 +++ .../rockchip/vpu/rk3288_vpu_hw_jpege.c| 154 .../platform/rockchip/vpu/rk3288_vpu_regs.h | 442 +++ .../platform/rockchip/vpu/rk3399_vpu_hw.c | 122 +++ .../rockchip/vpu/rk3399_vpu_hw_jpege.c| 181 + .../platform/rockchip/vpu/rk3399_vpu_regs.h | 601 +++ .../platform/rockchip/vpu/rockchip_vpu.h | 272 +++ .../platform/rockchip/vpu/rockchip_vpu_drv.c | 404 ++ .../platform/rockchip/vpu/rockchip_vpu_enc.c | 715 ++ .../platform/rockchip/vpu/rockchip_vpu_enc.h | 25 + .../platform/rockchip/vpu/rockchip_vpu_hw.h | 67 ++ 15 files changed, 3134 insertions(+) create mode 100644 drivers/media/platform/rockchip/vpu/Makefile create mode 100644 drivers/media/platform/rockchip/vpu/rk3288_vpu_hw.c create mode 100644 drivers/media/platform/rockchip/vpu/rk3288_vpu_hw_jpege.c create mode 100644 drivers/media/platform/rockchip/vpu/rk3288_vpu_regs.h create mode 100644 drivers/media/platform/rockchip/vpu/rk3399_vpu_hw.c create mode 100644 drivers/media/platform/rockchip/vpu/rk3399_vpu_hw_jpege.c create mode 100644 drivers/media/platform/rockchip/vpu/rk3399_vpu_regs.h create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu.h create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu_drv.c create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu_enc.c create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu_enc.h create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu_hw.h diff --git a/MAINTAINERS b/MAINTAINERS index 0f2cce4b73d7..71b034160b51 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12414,6 +12414,13 @@ S: Maintained F: drivers/media/platform/rockchip/rga/ F: Documentation/devicetree/bindings/media/rockchip-rga.txt +ROCKCHIP VPU CODEC DRIVER +M: Ezequiel Garcia +L: linux-media@vger.kernel.org +S: Maintained +F: drivers/media/platform/rockchip/vpu/ +F: Documentation/devicetree/bindings/media/rockchip-vpu.txt + ROCKER DRIVER M: Jiri Pirko L: net...@vger.kernel.org diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig index 9fa260090bf4..5b88d3de0490 100644 --- a/drivers/media/platform/Kconfig +++ b/drivers/media/platform/Kconfig @@ -448,6 +448,19 @@ config VIDEO_ROCKCHIP_RGA To compile this driver as a module choose m here. +config VIDEO_ROCKCHIP_VPU + tristate "Rockchip VPU driver" + depends on VIDEO_DEV && VIDEO_V4L2 + depends on ARCH_ROCKCHIP || COMPILE_TEST + select VIDEOBUF2_DMA_CONTIG + select V4L2_MEM2MEM_DEV + default n + help + Support for the Video Processing Unit present on Rockchip SoC, + which accelerates video and image encoding and decoding. + To compile this driver as a module, choose M here: the module + will be called rockchip-vpu. + config VIDEO_TI_VPE tristate "TI VPE (Video Processing Engine) driver" depends on VIDEO_DEV && VIDEO_V4L2 diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile index 2a9d82d1f984..513e8cef39ac 100644 --- a/drivers/media/platform/Makefile +++ b/drivers/media/platform/Makefile @@ -67,6 +67,7 @@ obj-$(CONFIG_VIDEO_RENESAS_JPU) += rcar_jpu.o obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1/ obj-$(CONFIG_VIDEO_ROCKCHIP_RGA) += rockchip/rga/ +obj-$(CONFIG_VIDEO_ROCKCHIP_VPU)+= rockchip/vpu/ obj-y += omap/ diff --git a/drivers/media/platform/rockchip/vpu/Makefile b/drivers/media/platform/rockchip/vpu/Makefile new file mode 100644 index ..cab0123c49d4 --- /dev/null +++ b/drivers/media/platform/rockchip/vpu/Makefile @@ -0,0 +1,8 @@ +obj-$(CONFIG_VIDEO_ROCKCHIP_VPU) += rockchip-vpu.o + +rockchip-vpu-y += rockchip_vpu_drv.o \ + rockchip_vpu_enc.o \ + rk3288_vpu_hw.o \ + rk3288_vpu_hw_jpege.o \ + rk3399_vpu_hw.o \ + rk3399_vpu_hw_jpege.o diff --git a/drivers/media/platform/rockchip/vpu/rk3288_vpu_hw.c b/drivers/media/platform/rockchip/vpu/rk3288_vpu_hw.c new file mode 100644 index ..747a82b69820 --- /dev/null +++ b/drivers/media/platform/rockchip/vpu/rk3288_vpu_hw.c @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Rockchip VPU codec driver + * + * Copyright (C) 2018 Rockchip Electronics Co., Ltd. + * Jeffy Chen + */ + +#include + +#include "rockchip_vpu.h" +#include "rk3288_vpu_regs.h" + +#define RK3288_ACLK_MAX_FREQ (400 * 1000 * 1000)
[PATCH v2 5/6] media: Add controls for jpeg quantization tables
From: Shunqian Zheng
Add V4L2_CID_JPEG_LUMA/CHROMA_QUANTIZATION controls to allow userspace
configure the JPEG quantization tables.
Signed-off-by: Shunqian Zheng
Signed-off-by: Ezequiel Garcia
---
Documentation/media/uapi/v4l/extended-controls.rst | 9 +
drivers/media/v4l2-core/v4l2-ctrls.c | 4
include/uapi/linux/v4l2-controls.h | 3 +++
3 files changed, 16 insertions(+)
diff --git a/Documentation/media/uapi/v4l/extended-controls.rst
b/Documentation/media/uapi/v4l/extended-controls.rst
index 9f7312bf3365..80e26f81900b 100644
--- a/Documentation/media/uapi/v4l/extended-controls.rst
+++ b/Documentation/media/uapi/v4l/extended-controls.rst
@@ -3354,6 +3354,15 @@ JPEG Control IDs
Specify which JPEG markers are included in compressed stream. This
control is valid only for encoders.
+.. _jpeg-quant-tables-control:
+
+``V4L2_CID_JPEG_LUMA_QUANTIZATION (__u8 matrix)``
+Sets the luma quantization table to be used for encoding
+or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. This table is
+expected to be in JPEG zigzag order, as per the JPEG specification.
+
+``V4L2_CID_JPEG_CHROMA_QUANTIZATION (__u8 matrix)``
+Sets the chroma quantization table.
.. flat-table::
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 599c1cbff3b9..5c62c3101851 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -999,6 +999,8 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_JPEG_RESTART_INTERVAL:return "Restart Interval";
case V4L2_CID_JPEG_COMPRESSION_QUALITY: return "Compression Quality";
case V4L2_CID_JPEG_ACTIVE_MARKER: return "Active Markers";
+ case V4L2_CID_JPEG_LUMA_QUANTIZATION: return "Luminance Quantization
Matrix";
+ case V4L2_CID_JPEG_CHROMA_QUANTIZATION: return "Chrominance
Quantization Matrix";
/* Image source controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1284,6 +1286,8 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
v4l2_ctrl_type *type,
*flags |= V4L2_CTRL_FLAG_READ_ONLY;
break;
case V4L2_CID_DETECT_MD_REGION_GRID:
+ case V4L2_CID_JPEG_LUMA_QUANTIZATION:
+ case V4L2_CID_JPEG_CHROMA_QUANTIZATION:
*type = V4L2_CTRL_TYPE_U8;
break;
case V4L2_CID_DETECT_MD_THRESHOLD_GRID:
diff --git a/include/uapi/linux/v4l2-controls.h
b/include/uapi/linux/v4l2-controls.h
index e4ee10ee917d..a466acf40625 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -987,6 +987,9 @@ enum v4l2_jpeg_chroma_subsampling {
#defineV4L2_JPEG_ACTIVE_MARKER_DQT (1 << 17)
#defineV4L2_JPEG_ACTIVE_MARKER_DHT (1 << 18)
+#define V4L2_CID_JPEG_LUMA_QUANTIZATION
(V4L2_CID_JPEG_CLASS_BASE + 5)
+#define V4L2_CID_JPEG_CHROMA_QUANTIZATION (V4L2_CID_JPEG_CLASS_BASE + 6)
+
/* Image source controls */
#define V4L2_CID_IMAGE_SOURCE_CLASS_BASE (V4L2_CTRL_CLASS_IMAGE_SOURCE |
0x900)
--
2.18.0
[PATCH v2 3/6] arm64: dts: rockchip: add VPU device node for RK3399
Add the Video Processing Unit node for the RK3399 SoC.
Signed-off-by: Ezequiel Garcia
---
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 12
1 file changed, 12 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index c88e603396f6..11137a06dd62 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -1198,6 +1198,18 @@
status = "disabled";
};
+ vpu: video-codec@ff65 {
+ compatible = "rockchip,rk3399-vpu";
+ reg = <0x0 0xff65 0x0 0x800>;
+ interrupts = ,
+;
+ interrupt-names = "vepu", "vdpu";
+ clocks = < ACLK_VCODEC>, < HCLK_VCODEC>;
+ clock-names = "aclk", "hclk";
+ power-domains = < RK3399_PD_VCODEC>;
+ iommus = <_mmu>;
+ };
+
vpu_mmu: iommu@ff650800 {
compatible = "rockchip,iommu";
reg = <0x0 0xff650800 0x0 0x40>;
--
2.18.0
[PATCH v2 4/6] media: Add JPEG_RAW format
From: Shunqian Zheng
Add V4L2_PIX_FMT_JPEG_RAW format that does not contain
JPEG header in the output frame.
Signed-off-by: Shunqian Zheng
Signed-off-by: Ezequiel Garcia
---
Documentation/media/uapi/v4l/pixfmt-compressed.rst | 9 +
drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
include/uapi/linux/videodev2.h | 1 +
3 files changed, 11 insertions(+)
diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst
b/Documentation/media/uapi/v4l/pixfmt-compressed.rst
index d382e7a5c38e..4dffe40097f2 100644
--- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst
+++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst
@@ -23,6 +23,15 @@ Compressed Formats
- 'JPEG'
- TBD. See also :ref:`VIDIOC_G_JPEGCOMP `,
:ref:`VIDIOC_S_JPEGCOMP `.
+* .. _V4L2-PIX-FMT-JPEG-RAW:
+
+ - ``V4L2_PIX_FMT_JPEG_RAW``
+ - 'Raw JPEG'
+ - Raw JPEG bitstream, containing a compressed payload. This format
+contains an image scan, i.e. without any metadata or headers.
+The user is expected to set the needed metadata such as
+quantization and entropy encoding tables, via ``V4L2_CID_JPEG``
+controls, see :ref:`jpeg-control-id`.
* .. _V4L2-PIX-FMT-MPEG:
- ``V4L2_PIX_FMT_MPEG``
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
b/drivers/media/v4l2-core/v4l2-ioctl.c
index 26d9702069fd..7eac5e39ddac 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1296,6 +1296,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
/* Max description length mask: descr =
"0123456789012345678901234567890" */
case V4L2_PIX_FMT_MJPEG:descr = "Motion-JPEG"; break;
case V4L2_PIX_FMT_JPEG: descr = "JFIF JPEG"; break;
+ case V4L2_PIX_FMT_JPEG_RAW: descr = "Raw JPEG"; break;
case V4L2_PIX_FMT_DV: descr = "1394"; break;
case V4L2_PIX_FMT_MPEG: descr = "MPEG-1/2/4"; break;
case V4L2_PIX_FMT_H264: descr = "H.264"; break;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index d8b33095abe0..72b458c8f49f 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -621,6 +621,7 @@ struct v4l2_pix_format {
/* compressed formats */
#define V4L2_PIX_FMT_MJPEGv4l2_fourcc('M', 'J', 'P', 'G') /* Motion-JPEG
*/
#define V4L2_PIX_FMT_JPEG v4l2_fourcc('J', 'P', 'E', 'G') /* JFIF JPEG
*/
+#define V4L2_PIX_FMT_JPEG_RAW v4l2_fourcc('J', 'P', 'G', 'R') /* JFIF JPEG RAW
without headers */
#define V4L2_PIX_FMT_DV v4l2_fourcc('d', 'v', 's', 'd') /* 1394
*/
#define V4L2_PIX_FMT_MPEG v4l2_fourcc('M', 'P', 'E', 'G') /* MPEG-1/2/4
Multiplexed */
#define V4L2_PIX_FMT_H264 v4l2_fourcc('H', '2', '6', '4') /* H264 with
start codes */
--
2.18.0
[PATCH v2 0/6] Add Rockchip VPU JPEG encoder
This series adds support for JPEG encoding via the VPU block
present in Rockchip platforms. Currently, support for RK3288
and RK3399 is included.
The hardware produces a Raw JPEG format (i.e. works as a
JPEG accelerator). It requires quantization tables provided
by the application, and uses standard huffman tables,
as recommended by the JPEG specification.
In order to support this, the series introduces a new pixel format,
and a new pair of controls, V4L2_CID_JPEG_{LUMA,CHROMA}_QUANTIZATION
allowing userspace to specify the quantization tables.
Userspace is then responsible to add the required headers
and tables to the produced raw payload, to produce a JPEG image.
Compliance
==
There is an outstanding compliance issue, related to a blocking
dqbuf, but I cannot see where is the issue, nor if the issue is
in the core or in the driver.
# v4l2-compliance -d 0 -s
v4l2-compliance SHA: 81ea4a243b63d7bb1fec580910c553af4ae072c1, 64 bits
Compliance test for device /dev/video0:
Driver Info:
Driver name : rockchip-vpu
Card type: rockchip-vpu
Bus info : platform: rockchip-vpu
Driver version : 4.18.0
Capabilities : 0x84204000
Video Memory-to-Memory Multiplanar
Streaming
Extended Pix Format
Device Capabilities
Device Caps : 0x04204000
Video Memory-to-Memory Multiplanar
Streaming
Extended Pix Format
Required ioctls:
test VIDIOC_QUERYCAP: OK
Allow for multiple opens:
test second /dev/video0 open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK
Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK (Not Supported)
Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 0 Audio Inputs: 0 Tuners: 0
Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0
Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)
Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: OK
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 2 Private Controls: 0
Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK (Not Supported)
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK
Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
Buffer ioctls:
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK
Test input 0:
Streaming ioctls:
test read/write: OK (Not Supported)
fail: v4l2-test-buffers.cpp(1225): pid != pid_streamoff
fail: v4l2-test-buffers.cpp(1258): testBlockingDQBuf(node, q)
test blocking wait: FAIL
test MMAP: OK
test USERPTR: OK (Not Supported)
test DMABUF: Cannot test, specify --expbuf-device
Total: 47, Succeeded: 46, Failed: 1, Warnings: 0
v2:
- Add devicetree binding documentation and devicetree changes
- Add documentation to added pixel format and controls
- Address Hans' review comments
- Get rid of unused running_ctx field
- Fix wrong planar pixel format depths
- Other minor changes for v4l2-compliance
- Drop .crop support, we will add support for the
selector API later, if needed.
Ezequiel Garcia (4):
dt-bindings: Document the Rockchip VPU bindings
ARM: dts: rockchip: add VPU device node for RK3288
arm64: dts: rockchip: add VPU device node for RK3399
media: add Rockchip VPU driver
Shunqian Zheng (2):
media: Add JPEG_RAW format
Re: [PATCH 3/3] media: add Rockchip VPU driver
On Thu, 2018-08-02 at 09:30 +0200, Hans Verkuil wrote:
> On 07/27/2018 07:13 PM, Ezequiel Garcia wrote:
> > Hi Hans,
> >
> > Thanks a lot for the review.
> >
> > On Wed, 2018-07-18 at 11:58 +0200, Hans Verkuil wrote:
> > > >
> > > > +
> > > > +static int
> > > > +queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue
> > > > *dst_vq)
> > > > +{
> > > > + struct rockchip_vpu_ctx *ctx = priv;
> > > > + int ret;
> > > > +
> > > > + src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > > > + src_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
> > >
> > > Any reason for setting USERPTR?
> > >
> > > > + src_vq->drv_priv = ctx;
> > > > + src_vq->ops = _vpu_enc_queue_ops;
> > > > + src_vq->mem_ops = _dma_contig_memops;
> > >
> > > It isn't really useful in combination with dma_contig.
> > >
> >
> > Right! I think I just missed it.
> >
> > > >
> > > > +
> > > > +fallback:
> > > > + /* Default to full frame for incorrect settings. */
> > > > + ctx->src_crop.width = fmt->width;
> > > > + ctx->src_crop.height = fmt->height;
> > > > + return 0;
> > > > +}
> > >
> > > Replace crop by the selection API. The old crop API is no longer allowed
> > > in new drivers.
> >
> > I have a question about the selection API. There is a comment that says
> > MPLANE types shouldn't be used:
> >
> > /**
> > * struct v4l2_selection - selection info
> > * @type: buffer type (do not use *_MPLANE types)
> >
> > What is the meaning of that?
>
> Easiest is to look in v4l2-ioctl.c. Search for g_selection. You'll see that
> if the user passes in an _MPLANE buftype it is replaced by the regular
> non-mplane
> buftype. So drivers only see that type.
>
> It used to be that applications also had to be specific about what buftype to
> pass to G/S_SELECTION, but these days either type can be passed in.
>
> >
> > [..]
> > >
> > > I skipped the review of the colorspace handling. I'll see if I can come
> > > back
> > > to that later today. I'm not sure if it is correct, but to be honest I
> > > doubt
> > > that there is any JPEG encoder that does this right anyway.
> > >
> >
> > And I'd say it's probably wrong, since we let the user change the
> > colorspace,
> > but we do not use that for anything.
>
> So strictly speaking a JPEG encoder doesn't care about colorspace, xfer_func
> and
> ycbcr_enc. It might care about quantization. It is my understanding that a
> JPEG
> encoder expects full range Y'CbCr instead of limited range Y'CbCr. Does the HW
> support limited range as well? I.e. can it convert from limited to full range
> in hardware?
>
> It might also be that it doesn't care so passing a limited range Y'CbCr image
> will
> create a limited range JPEG file and software will have to know that it
> contains
> limited range data when decoding it.
>
> In any case, colorspace, xfer_func and ycbcr_enc can just be passed from the
> output to the capture side, just like other codecs. What to do with
> 'quantization'
> depends on the hardware: if it can convert from limited to full range on the
> fly,
> then it should be handled by the driver. If not, then copy it like the other
> fields.
>
I see no mention of the encoding range in the TRM, but reviewing the registers
it doesn't seem it's supporting conversion from limited to full range, so it
should
be fine to pass-thru the output value.
Thanks,
Eze
Re: [RFC 1/4] media: meson: add v4l2 m2m video decoder driver
2018-08-02 12:30 GMT+02:00 Jerome Brunet :
>
> Ouch!
>
> Your architecture seems pretty modular. Maybe you could split this patch a
> little ? One patch of the 'backbone' then one for each codec ?
Hehe, it's a big baby. Sure I'll split it codec by codec.
>
> I suppose this will go away with :
> https://lkml.kernel.org/r/20180801185128.23440-1-maxi.jour...@wanadoo.fr
>
> ? If yes, it would have been better to send a version using it directly.
Indeed they will, my bad.
>> + while (readl_relaxed(core->dos_base + DCAC_DMA_CTRL) & 0x8000) { }
>> + while (readl_relaxed(core->dos_base + LMEM_DMA_CTRL) & 0x8000) { }
>
> Is this guarantee to exit at some point or is there a risk of staying stuck
> here
> forever ?
>
> I think regmap has some helper function for polling with a timeout.
Totally forgot about those since they caused no problem so far, good catch.
I'll see with regmap.
>> +
>> + writel_relaxed((1<<7) | (1<<6) | (1<<4), core->dos_base +
>> DOS_SW_RESET0);
>> + writel_relaxed(0, core->dos_base + DOS_SW_RESET0);
>> + readl_relaxed(core->dos_base + DOS_SW_RESET0);
>> +
>> + writel_relaxed((1<<7) | (1<<6) | (1<<4), core->dos_base +
>> DOS_SW_RESET0);
>> + writel_relaxed(0, core->dos_base + DOS_SW_RESET0);
>> + writel_relaxed((1<<9) | (1<<8), core->dos_base + DOS_SW_RESET0);
>> + writel_relaxed(0, core->dos_base + DOS_SW_RESET0);
>> + readl_relaxed(core->dos_base + DOS_SW_RESET0);
>> +
>> + writel_relaxed(readl_relaxed(core->dos_base + POWER_CTL_VLD) | (1 <<
>> 9) | (1 << 6), core->dos_base + POWER_CTL_VLD);
>> +
>> + writel_relaxed(0, core->dos_base + PSCALE_CTRL);
>> + writel_relaxed(0, core->dos_base + AV_SCRATCH_0);
>> +
>> + workspace_offset = h264->workspace_paddr - DEF_BUF_START_ADDR;
>> + writel_relaxed(workspace_offset, core->dos_base + AV_SCRATCH_1);
>> + writel_relaxed(h264->ext_fw_paddr, core->dos_base + AV_SCRATCH_G);
>> + writel_relaxed(h264->sei_paddr - workspace_offset, core->dos_base +
>> AV_SCRATCH_I);
>
> Do we have any idea what all the above does ? I suppose, it mostly comes from
> reverse engineering the vendor kernel. If possible, a few comments would be
> nice.
>
> In general, instead of (1 << x), you could write BIT(x)
>
The resets and power_ctl, not really. The last 4 lines set the phy
addr for the "workspace" which is a memory region where the decoder
keeps some stuff, unfortunately I don't know what,
the extended firmware data and the SEI dumps.
I'll try to add some comments and see if I can find more info in the aml kernel.
I'll also change all the (1 << x) to BIT(x).
>
> What the about defining the filter shift and width, using GENMASK() maybe ?
>
>> + mb_total = (parsed_info >> 8) & 0x;
>> +
>> + /* Size of Motion Vector per macroblock ? */
>> + mb_mv_byte = (parsed_info & 0x8000) ? 24 : 96;
>
> #define FOO_MB_SIZE BIT(28)
> (parsed_info & FOO_MB_SIZE) ?
>> + writel_relaxed((max_reference_size << 24) | (actual_dpb_size << 16) |
>> (max_dpb_size << 8), core->dos_base + AV_SCRATCH_0);
>
> I know it is not always easy, especially with very little documentation, but
> could you #define a bit more the content of the registers:
>
> something like
> #define BAR_MAX_REF_SHIFT 24
>
> You get the idea ..
>> + cmd = status & 0xff;
>> +
>> + if (cmd == 1) {
>
> Same kind of comment, could define those command somewhere to this a bit more
> readable ?
>
>> + for (i = 0; i < 16; i++) {
>> + u16 cur_rps = params->p.CUR_RPS[i];
>> + int delt = cur_rps & ((1 << (RPS_USED_BIT - 1)) - 1);
>
> Looks like using GENMASK would make things a bit more readable here.
>
>> +
>> + if (cur_rps & 0x8000)
>
> BIT(15) ? any idea what this is ? could we define this ?
>
> Same comment in general for the rest of the patch. If you can replace mask and
> bit calculation with related macro and define things a bit more, it would be
> nice.
>> + for (i = 0; i < num_ref_idx_l0_active; i++) {
>> + int cIdx;
>
> Could we avoid cAmEl case ?
>
>> + //writel_relaxed(buf_uv_paddr >> 5, core->dos_base +
>> HEVCD_MPP_ANC2AXI_TBL_DATA);
>
> Clean up ?
>
Thanks for all the advice above.
>> +#define DOS_SW_RESET00xfc00
>
> I think this is not the first time you've defined this.
> Maybe this (and other re-used register offsets) needs to be in some header ?
Yes at present each file has their set of registers declared within
the .c but there are some redundancies.
I'll cater them in a header.
>> +#define HEVC_ASSIST_AFIFO_CTRL (0x3001 * 4)
>
> Defining register this way is something amlogic does in their.
> We they submitted the AXG clock controller we kindly asked them
> to directly put what the offset is and drop the calculation.
>
> Could please do the same ?
Sure
>> + core->dos_parser_clk = devm_clk_get(dev, "dos_parser");
>> + if (IS_ERR(core->dos_parser_clk)) {
>
> You should handle EPROBE_DEFER and not
Re: [PATCH v6 07/13] media: dt-bindings: add bindings for i.MX7 media driver
Hi Sakari,
Thanks for the review.
I will take this in account when preparing the v7, all your
comments
bellow look reasonable to me.
---
Cheers,
Rui
On Thu 02 Aug 2018 at 14:00, Sakari Ailus wrote:
Hi Rui,
On Tue, May 22, 2018 at 03:52:39PM +0100, 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
---
.../devicetree/bindings/media/imx7-csi.txt| 44 ++
.../bindings/media/imx7-mipi-csi2.txt | 82
+++
2 files changed, 126 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 ..aab4f5d72390
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/imx7-csi.txt
@@ -0,0 +1,44 @@
+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 should contain one 'port' child node with one
child 'endpoint'
"should" or "shall"?
+node, according to the bindings defined in
Documentation/devicetree/bindings/
+media/video-interfaces.txt. In the following example a remote
endpoint is a
I wouldn't split the path as it breaks copy-paste; up to you.
+video multiplexer.
+
+example:
+
+csi: csi@3071 {
+#address-cells = <1>;
+#size-cells = <0>;
+
+compatible = "fsl,imx7-csi";
+reg = <0x3071 0x1>;
+interrupts = IRQ_TYPE_LEVEL_HIGH>;
+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 ..7c5f20863724
--- /dev/null
+++
b/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
@@ -0,0 +1,82 @@
+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;
+
+port node
+-
+
+- reg : (required) can take the values 0 or 1,
where 0 is the
+ related sink port and port 1 should be
the source one;
Should and is -> shall?
I think you should also elaborate whether or not the port (as
well as the
endpoint) nodes themselves are mandatory.
+
+endpoint node
+-
+
+- data-lanes: (required) an array specifying active
physical MIPI-CSI2
+ data input lanes and their mapping to logical
lanes; the
+
Re: [PATCH v6 00/13] media: staging/imx7: add i.MX7 media driver
Hi Hans, On Thu 02 Aug 2018 at 13:37, Hans Verkuil wrote: Hi Rui, On 05/22/18 16:52, Rui Miguel Silva wrote: Hi, This series introduces the Media driver to work with the i.MX7 SoC. it uses the already existing imx media core drivers but since the i.MX7, contrary to i.MX5/6, do not have an IPU and because of that some changes in the imx media core are made along this series to make it support that case. This patches adds CSI and MIPI-CSI2 drivers for i.MX7, along with several configurations changes for this to work as a capture subsystem. Some bugs are also fixed along the line. And necessary documentation. For a more detailed view of the capture paths, pads links in the i.MX7 please take a look at the documentation in PATCH 14. The system used to test and develop this was the Warp7 board with an OV2680 sensor, which output format is 10-bit bayer. So, only MIPI interface was tested, a scenario with an parallel input would nice to have. *Important note*, this code depends on Steve Longerbeam series [0]: [PATCH v4 00/13] media: imx: Switch to subdev notifiers which the merging status is not clear to me, but the changes in there make senses to this series Bellow goes an example of the output of the pads and links and the output of v4l2-compliance testing. The v4l-utils version used is: v4l2-compliance SHA : 47d43b130dc6e9e0edc900759fb37649208371e4 from Apr 4th. The Media Driver fail some tests but this failures are coming from code out of scope of this series (video-mux, imx-capture), and some from the sensor OV2680 but that I think not related with the sensor driver but with the testing and core. The csi and mipi-csi entities pass all compliance tests. Cheers, Rui [0]: https://www.mail-archive.com/linux-media@vger.kernel.org/msg131186.html This patch series was delayed quite a bit since the patch series above it depends on is still not merged. But the v6 version of that series will be merged once the 4.20 cycle opens: https://www.mail-archive.com/linux-media@vger.kernel.org/msg133391.html Good news. Sakari has a branch with that series on top of the latest media_tree master: https://git.linuxtv.org/sailus/media_tree.git/log/?h=v4l2-fwnode Can you rebase this imx7 series on top of that? And test it again with the *latest* v4l2-compliance? (I've added new checks recently, so you need to update this utility) Please post the output of the v4l2-compliance test (after fixing any issues it raises of course), either as a reply to this post or in the cover letter of a v7 version of this series if you had to make changes. Sure, I will rebase on top of Sakari tree and will update the compliance tests and run them again. This should expedite merging this series for 4.20. Thanks! Hans Ok, thanks for this. I will try to do it soon. --- Cheers, Rui
Re: [PATCH v3 3/4] v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
On Thu, 2018-08-02 at 10:02 +0200, Hans Verkuil wrote:
> On 08/01/2018 11:50 PM, Ezequiel Garcia wrote:
> > v4l2_m2m_job_finish() is typically called in interrupt context.
> >
> > Some implementation of .device_run might sleep, and so it's
> > desirable to avoid calling it directly from
> > v4l2_m2m_job_finish(), thus avoiding .device_run from running
> > in interrupt context.
> >
> > Implement a deferred context that calls v4l2_m2m_try_run,
> > and gets scheduled by v4l2_m2m_job_finish().
> >
> > Signed-off-by: Ezequiel Garcia
> > ---
> > drivers/media/v4l2-core/v4l2-mem2mem.c | 36 +++---
> > 1 file changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > index da82d151dd20..0bf4deefa899 100644
> > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > @@ -69,6 +69,7 @@ static const char * const m2m_entity_name[] = {
> > * @curr_ctx: currently running instance
> > * @job_queue: instances queued to run
> > * @job_spinlock: protects job_queue
> > + * @job_work: worker to run queued jobs.
> > * @m2m_ops: driver callbacks
> > */
> > struct v4l2_m2m_dev {
> > @@ -85,6 +86,7 @@ struct v4l2_m2m_dev {
> >
> > struct list_headjob_queue;
> > spinlock_t job_spinlock;
> > + struct work_struct job_work;
> >
> > const struct v4l2_m2m_ops *m2m_ops;
> > };
> > @@ -224,10 +226,11 @@ EXPORT_SYMBOL(v4l2_m2m_get_curr_priv);
> > /**
> > * v4l2_m2m_try_run() - select next job to perform and run it if possible
> > * @m2m_dev: per-device context
> > + * @try_lock: indicates if the queue lock should be taken
>
> I don't like this bool. See more below.
>
Me neither. In fact, I've spent a lot of time trying to avoid it!
However...
> > *
> > * Get next transaction (if present) from the waiting jobs list and run it.
> > */
> > -static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
> > +static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev, bool try_lock)
> > {
> > unsigned long flags;
> >
> > @@ -250,7 +253,20 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev
> > *m2m_dev)
> > spin_unlock_irqrestore(_dev->job_spinlock, flags);
> >
> > dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
> > +
> > + /*
> > +* A m2m context lock is taken only after a m2m context
> > +* is picked from the queue and marked as running.
> > +* The lock is only needed if v4l2_m2m_try_run is called
> > +* from the async worker.
> > +*/
> > + if (try_lock && m2m_dev->curr_ctx->q_lock)
> > + mutex_lock(m2m_dev->curr_ctx->q_lock);
> > +
Note that only after a context has been chosen, and curr_ctx is assigned,
it's possible to take the mutex.
> > m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
> > +
> > + if (try_lock && m2m_dev->curr_ctx->q_lock)
> > + mutex_unlock(m2m_dev->curr_ctx->q_lock);
> > }
> >
> > /*
> > @@ -340,10 +356,22 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx
> > *m2m_ctx)
> > struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
> >
> > __v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
> > - v4l2_m2m_try_run(m2m_dev);
> > + v4l2_m2m_try_run(m2m_dev, false);
>
> I would like to see a WARN_ON where you verify that q_lock is actually locked
> (and this needs to be documented as well).
>
OK.
> > }
> > EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
> >
> > +/**
> > + * v4l2_m2m_device_run_work() - run pending jobs for the context
> > + * @work: Work structure used for scheduling the execution of this
> > function.
> > + */
> > +static void v4l2_m2m_device_run_work(struct work_struct *work)
> > +{
> > + struct v4l2_m2m_dev *m2m_dev =
> > + container_of(work, struct v4l2_m2m_dev, job_work);
> > +
> > + v4l2_m2m_try_run(m2m_dev, true);
>
> Just lock q_lock here around the try_run call. That's consistent with how
> try_schedule works. No need to add an extra argument to try_run.
>
As I mentioned above, we might not have any lock to take at this point.
> > +}
> > +
> > /**
> > * v4l2_m2m_cancel_job() - cancel pending jobs for the context
> > * @m2m_ctx: m2m context with jobs to be canceled
> > @@ -403,7 +431,8 @@ void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
> > /* This instance might have more buffers ready, but since we do not
> > * allow more than one job on the job_queue per instance, each has
> > * to be scheduled separately after the previous one finishes. */
> > - v4l2_m2m_try_schedule(m2m_ctx);
> > + __v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
> > + schedule_work(_dev->job_work);
>
> You might want to add a comment here explaining why you need 'work' here.
>
OK.
> > }
> > EXPORT_SYMBOL(v4l2_m2m_job_finish);
> >
> > @@ -837,6 +866,7 @@ struct v4l2_m2m_dev *v4l2_m2m_init(const struct
> > v4l2_m2m_ops
Re: [PATCH 16/22] [media] tvp5150: add querystd
Em Thu, 2 Aug 2018 12:16:41 +0200 Marco Felsch escreveu: > > > Did you drop the DT of_graph support patch? It was there on your first > > > tvp5150 branch. > > > > Yes. As discussed, I'm waiting for a replacement patch from you. So, > > after testing, I removed it, in order to make simpler to add your > > replacement patch. > > > > IMO, the proper mapping is one input linked to (up to) 3 connectors. > > I tought it would be okay to have more than 1 input pad since the > .sig_type pad property. So the tvp5150 media entity can be represented > like the physical tvp5150 chip. As I said, tvp5150 has internally two physical inputs only: AIP1A and AIP1B. IMO, it should be creating 3 PADS (two inputs and the output one), e. g. something like (names here are just a suggestion): TVP5150_PAD_IN_AIP1A, TVP5150_PAD_IN_AIP1B, TVP5150_PAD_OUT The S-video connector (if present) should be linked to both inputs. I discussed this with other core maintainers: we all have the same opinion about that. I'll post a separate e-mail from the discussions for you and others to comment. It would need some logic that will enforce that just one connector link will be active on any given time (e. g. when a link is enabled, it should disable the other links). > > I will fix it, if it isn't the right way. Ok. Thanks, Mauro
Re: [PATCH v5 08/11] media: vsp1: Add support for extended display list headers
Hi Kieran,
Thank you for the patch.
On Tuesday, 17 July 2018 23:35:50 EEST Kieran Bingham wrote:
> From: Kieran Bingham
>
> Extended display list headers allow pre and post command lists to be
> executed by the VSP pipeline. This provides the base support for
> features such as AUTO_FLD (for interlaced support) and AUTO_DISP (for
> supporting continuous camera preview pipelines.
>
> Signed-off-by: Kieran Bingham
>
> ---
>
> v2:
> - remove __packed attributes
>
> v5:
> - Rename vsp1_dl_ext_header field names
> - Rename @extended -> @extension
> - Remove unnecessary VI6_DL_SWAP changes
> - Rename @cmd_opcode -> @opcode
> - Drop unused @data_size field
> - Move iteration of WPF's inside vsp1_dlm_setup
> - Rename vsp1_dl_ext_cmd_header -> vsp1_pre_ext_dl_body
> - Rename vsp1_pre_ext_dl_body->cmd to vsp1_pre_ext_dl_body->opcode
> - Rename vsp1_dl_ext_header->reserved0 to vsp1_dl_ext_header->padding
> - vsp1_pre_ext_dl_body: Rename 'data' to 'address_set'
> - vsp1_pre_ext_dl_body: Add struct documentation
> - Document ordering of 16bit accesses for flags in vsp1_dl_ext_header
>
> drivers/media/platform/vsp1/vsp1.h | 1 +-
> drivers/media/platform/vsp1/vsp1_dl.c | 104 -
> drivers/media/platform/vsp1/vsp1_dl.h | 25 ++-
> drivers/media/platform/vsp1/vsp1_drv.c | 4 +-
> drivers/media/platform/vsp1/vsp1_regs.h | 2 +-
> 5 files changed, 132 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1.h
> b/drivers/media/platform/vsp1/vsp1.h index f0d21cc8e9ab..56c62122a81a
> 100644
> --- a/drivers/media/platform/vsp1/vsp1.h
> +++ b/drivers/media/platform/vsp1/vsp1.h
> @@ -53,6 +53,7 @@ struct vsp1_uif;
> #define VSP1_HAS_HGO (1 << 7)
> #define VSP1_HAS_HGT (1 << 8)
> #define VSP1_HAS_BRS (1 << 9)
> +#define VSP1_HAS_EXT_DL (1 << 10)
>
> struct vsp1_device_info {
> u32 version;
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c index 90e0a11c29b5..2fffe977aa35
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -22,6 +22,9 @@
> #define VSP1_DLH_INT_ENABLE (1 << 1)
> #define VSP1_DLH_AUTO_START (1 << 0)
>
> +#define VSP1_DLH_EXT_PRE_CMD_EXEC(1 << 9)
> +#define VSP1_DLH_EXT_POST_CMD_EXEC (1 << 8)
> +
> struct vsp1_dl_header_list {
> u32 num_bytes;
> u32 addr;
> @@ -34,12 +37,59 @@ struct vsp1_dl_header {
> u32 flags;
> };
>
> +/**
> + * struct vsp1_dl_ext_header - Extended display list header
> + * @padding: padding zero bytes for alignment
> + * @pre_ext_dl_num_cmd: number of pre-extended command bodies to parse
> + * @flags: enables or disables execution of the pre and post command
> + * @pre_ext_dl_plist: start address of pre-extended display list bodies
> + * @post_ext_dl_num_cmd: number of post-extended command bodies to parse
> + * @post_ext_dl_plist: start address of post-extended display list bodies
> + */
> +struct vsp1_dl_ext_header {
> + u32 padding;
> +
> + /*
> + * The datasheet represents flags as stored before pre_ext_dl_num_cmd,
> + * expecting 32-bit accesses. The flags are appropriate to the whole
> + * header, not just the pre_ext command, and thus warrant being
> + * separated out. Due to byte ordering, and representing as 16 bit
> + * values here, the flags must be positioned after the
> + * pre_ext_dl_num_cmd.
> + */
> + u16 pre_ext_dl_num_cmd;
> + u16 flags;
> + u32 pre_ext_dl_plist;
> +
> + u32 post_ext_dl_num_cmd;
> + u32 post_ext_dl_plist;
> +};
> +
> +struct vsp1_dl_header_extended {
> + struct vsp1_dl_header header;
> + struct vsp1_dl_ext_header ext;
> +};
> +
> struct vsp1_dl_entry {
> u32 addr;
> u32 data;
> };
>
> /**
> + * struct vsp1_pre_ext_dl_body - Pre Extended Display List Body
> + * @opcode: Extended display list command operation code
> + * @flags: Pre-extended command flags. These are specific to each command
> + * @address_set: Source address set pointer. Must have 16 byte alignment
s/byte/bytes/
> + * @reserved: Zero bits for alignment.
> + */
> +struct vsp1_pre_ext_dl_body {
> + u32 opcode;
> + u32 flags;
> + u32 address_set;
> + u32 reserved;
> +};
> +
> +/**
> * struct vsp1_dl_body - Display list body
> * @list: entry in the display list list of bodies
> * @free: entry in the pool free body list
> @@ -95,9 +145,12 @@ struct vsp1_dl_body_pool {
> * @list: entry in the display list manager lists
> * @dlm: the display list manager
> * @header: display list header
> + * @extension: extended display list header. NULL for normal lists
> * @dma: DMA address for the header
> * @body0: first display list body
> * @bodies: list of extra display list bodies
> + * @pre_cmd: pre command to be issued through extended dl header
> + * @post_cmd: post command to be issued through extended
your photos need edit
As a boutique team, we work personally with our clients to ensure the good results. Having over a decade of hands-on experience in photography and retouching, we have been inspired to expand our services to the public. We are team of artists who have excelled in fields such as art, photography, retouching, graphics and design. No matter what your field of interest is in, we can learn to work with your style to give you the best product. We provide below image editing services: Clipping path Image cut out Image shadow creation Image masking Photo retouching Beauty retouching (skin, face, body retouching) Glamour retouching Product retouching Color correction and others We provide testing for our services. Thanks, Sam
for the retouching
As a boutique team, we work personally with our clients to ensure the good results. Having over a decade of hands-on experience in photography and retouching, we have been inspired to expand our services to the public. We are team of artists who have excelled in fields such as art, photography, retouching, graphics and design. No matter what your field of interest is in, we can learn to work with your style to give you the best product. We provide below image editing services: Clipping path Image cut out Image shadow creation Image masking Photo retouching Beauty retouching (skin, face, body retouching) Glamour retouching Product retouching Color correction and others We provide testing for our services. Thanks, Sam
Re: [PATCH v6 07/13] media: dt-bindings: add bindings for i.MX7 media driver
Hi Rui,
On Tue, May 22, 2018 at 03:52:39PM +0100, 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
> ---
> .../devicetree/bindings/media/imx7-csi.txt| 44 ++
> .../bindings/media/imx7-mipi-csi2.txt | 82 +++
> 2 files changed, 126 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 ..aab4f5d72390
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/imx7-csi.txt
> @@ -0,0 +1,44 @@
> +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 should contain one 'port' child node with one child
> 'endpoint'
"should" or "shall"?
> +node, according to the bindings defined in Documentation/devicetree/bindings/
> +media/video-interfaces.txt. In the following example a remote endpoint is a
I wouldn't split the path as it breaks copy-paste; up to you.
> +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 ..7c5f20863724
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
> @@ -0,0 +1,82 @@
> +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;
> +
> +port node
> +-
> +
> +- reg : (required) can take the values 0 or 1, where 0 is
> the
> + related sink port and port 1 should be the source one;
Should and is -> shall?
I think you should also elaborate whether or not the port (as well as the
endpoint) nodes themselves are mandatory.
> +
> +endpoint node
> +-
> +
> +- data-lanes: (required) an array specifying active physical MIPI-CSI2
> + data input lanes and their mapping to logical lanes;
Re: [RFC 0/4] media: meson: add video decoder driver
Hi Jerome, Neil, Hans, Thanks a lot for all the insights. 2018-08-02 8:59 GMT+02:00 Hans Verkuil : >> fail: >> ../../../v4l-utils-1.12.3/utils/v4l2-compliance/v4l2-test-buffers.cpp(571): >> q.has_expbuf(node) >> test VIDIOC_EXPBUF: FAIL > > Not sure, might well be a knock-on result of the 'one open' problem. > > BTW, always get the latest code from the v4l-utils git repo, don't use a > released > version for v4l2-compliance: it's always evolving and you don't want to use an > old version. Also for the next version of this patch series add the output of > v4l2-compliance to this cover letter, I want to see it. Will do. > Finally, are you aware of the work Tomasz Figa on specifying the codec > behavior? > > https://lkml.org/lkml/2018/7/24/539 > > The final version will be close to what was posted there. I wasn't, thanks for pointing it out.
Re: [RFC 4/4] dt-bindings: media: add Amlogic Meson Video Decoder Bindings
Hi Martin & Jerome, 2018-08-02 12:33 GMT+02:00 Jerome Brunet : > Maxime, when formatting your patchset, remember to put the bindings > documentation before actually using them. This patch could be the first one of > your series. Noted, thanks. 2018-08-01 22:13 GMT+02:00 Martin Blumenstingl : >> +- VDEC_2 is used as a helper for corner cases like H.264 4K on older SoCs. >> +It is not handled by this driver. > is it currently not handled or will it never be? I don't think it will ever be, at least from me. This VDEC unit is rarely used and only for a few corner cases on SoCs like meson8b, and I have no intention of supporting them for now as there are other limitations. > any reason why you are not using the DMC syscon (as added in your > patch "dt-bindings: soc: amlogic: add meson-canvas documentation") > instead of mapping the DMC region again? To answer you and Jerome, I didn't use it because I wanted to keep both patchsets separate in case of testing. In hindsight though, I should have used the canvas module in the vdec in the RFC. So yeah, this will definitely be used by the final product. >> +- interrupts: should contain the vdec and esparser IRQs. > are these two IRQs the "currently supported" ones or are there more > for the whole IP block (but just not implemented yet)? There are more IRQs within the VDEC but they are not used at the moment. Some are for the demuxer, VDEC_2, etc.. > AFAIK the "correct" format is (just like you've done for the clocks below): >reg = <0x0 0xc882 0x0 0x1>, > <0x0 0xc110a580 0x0 0xe4>, > <0x0 0xc8838000 0x0 0x60>; > > AFAIK the "correct" format is (just like you've done for the clocks below): >interrupts = , >; > >> + amlogic,ao-sysctrl = <_AO>; > this is not documented above - is it needed? Duly noted, thanks.
Re: [PATCH] media: imx258: remove test pattern map from driver
Hi Jason,
On Thu, Aug 02, 2018 at 04:17:00PM +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.
Hmm. I think this would be changed based on my comments anyway, but please
explain what you're doing and *why*. HAL is not relevant in this context
I'd say.
>
> 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;
> -
I think this newline is where it should be.
> /*
>* 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 :
Spaces around "?".
> 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,
The sensor only has a source pad while the link validate is only needed for
sink pads.
> +};
> +
> 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;
> imx258->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> imx258->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v6 00/13] media: staging/imx7: add i.MX7 media driver
Hi Rui, On 05/22/18 16:52, Rui Miguel Silva wrote: > Hi, > This series introduces the Media driver to work with the i.MX7 SoC. it uses > the > already existing imx media core drivers but since the i.MX7, contrary to > i.MX5/6, do not have an IPU and because of that some changes in the imx media > core are made along this series to make it support that case. > > This patches adds CSI and MIPI-CSI2 drivers for i.MX7, along with several > configurations changes for this to work as a capture subsystem. Some bugs are > also fixed along the line. And necessary documentation. > > For a more detailed view of the capture paths, pads links in the i.MX7 please > take a look at the documentation in PATCH 14. > > The system used to test and develop this was the Warp7 board with an OV2680 > sensor, which output format is 10-bit bayer. So, only MIPI interface was > tested, a scenario with an parallel input would nice to have. > > *Important note*, this code depends on Steve Longerbeam series [0]: > [PATCH v4 00/13] media: imx: Switch to subdev notifiers > which the merging status is not clear to me, but the changes in there make > senses to this series > > Bellow goes an example of the output of the pads and links and the output of > v4l2-compliance testing. > > The v4l-utils version used is: > v4l2-compliance SHA : 47d43b130dc6e9e0edc900759fb37649208371e4 from Apr 4th. > > The Media Driver fail some tests but this failures are coming from code out of > scope of this series (video-mux, imx-capture), and some from the sensor OV2680 > but that I think not related with the sensor driver but with the testing and > core. > > The csi and mipi-csi entities pass all compliance tests. > > Cheers, > Rui > > [0]: https://www.mail-archive.com/linux-media@vger.kernel.org/msg131186.html This patch series was delayed quite a bit since the patch series above it depends on is still not merged. But the v6 version of that series will be merged once the 4.20 cycle opens: https://www.mail-archive.com/linux-media@vger.kernel.org/msg133391.html Sakari has a branch with that series on top of the latest media_tree master: https://git.linuxtv.org/sailus/media_tree.git/log/?h=v4l2-fwnode Can you rebase this imx7 series on top of that? And test it again with the *latest* v4l2-compliance? (I've added new checks recently, so you need to update this utility) Please post the output of the v4l2-compliance test (after fixing any issues it raises of course), either as a reply to this post or in the cover letter of a v7 version of this series if you had to make changes. This should expedite merging this series for 4.20. Thanks! Hans
Re: [PATCH 3/3] media: add Rockchip VPU driver
On 2 August 2018 at 05:54, Hans Verkuil wrote: > On 08/01/18 23:07, Ezequiel Garcia wrote: >> Add a mem2mem driver for the VPU available on Rockchip SoCs. >> Currently only JPEG encoding is supported, for RK3399 and RK3288 >> platforms. >> >> Signed-off-by: Ezequiel Garcia [..] > > I stop reviewing here since I wonder if this is really the v2 source code. > I see too many things I've commented about in v1. Did you accidentally > post the v1 again? > Yes, that seems to be the case! -- Ezequiel GarcÃa, VanguardiaSur www.vanguardiasur.com.ar
Re: [PATCH 16/22] [media] tvp5150: add querystd
Hi Ian,
On 18-08-02 11:54, Ian Arkver wrote:
> On 02/08/18 10:49, Mauro Carvalho Chehab wrote:
> > Em Thu, 2 Aug 2018 10:01:01 +0200
> > Marco Felsch escreveu:
> >
> > > Hi Mauro,
> > >
> > > On 18-08-01 12:50, Mauro Carvalho Chehab wrote:
> > > > Em Wed, 1 Aug 2018 16:49:26 +0200
> > > > Marco Felsch escreveu:
> > > > > Hi Mauro,
> > > > >
> > > > > On 18-08-01 11:22, Mauro Carvalho Chehab wrote:
> > > > > > Em Wed, 1 Aug 2018 15:21:25 +0200
> > > > > > Marco Felsch escreveu:
> > > > > > > Hi Mauro,
> > > > > > >
> > > > > > > On 18-07-30 15:09, Mauro Carvalho Chehab wrote:
> > > > > > > > Em Thu, 28 Jun 2018 18:20:48 +0200
> > > > > > > > Marco Felsch escreveu:
> > > > > > > > > From: Philipp Zabel
> > > > > > > > >
> > > > > > > > > Add the querystd video_op and make it return V4L2_STD_UNKNOWN
> > > > > > > > > while the
> > > > > > > > > TVP5150 is not locked to a signal.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Philipp Zabel
> > > > > > > > > Signed-off-by: Marco Felsch
> > > > > > > > > ---
> > > > > > > > > drivers/media/i2c/tvp5150.c | 10 ++
> > > > > > > > > 1 file changed, 10 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/media/i2c/tvp5150.c
> > > > > > > > > b/drivers/media/i2c/tvp5150.c
> > > > > > > > > index 99d887936ea0..1990aaa17749 100644
> > > > > > > > > --- a/drivers/media/i2c/tvp5150.c
> > > > > > > > > +++ b/drivers/media/i2c/tvp5150.c
> > > > > > > > > @@ -796,6 +796,15 @@ static v4l2_std_id
> > > > > > > > > tvp5150_read_std(struct v4l2_subdev *sd)
> > > > > > > > > }
> > > > > > > > > }
> > > > > > > > > +static int tvp5150_querystd(struct v4l2_subdev *sd,
> > > > > > > > > v4l2_std_id *std_id)
> > > > > > > > > +{
> > > > > > > > > + struct tvp5150 *decoder = to_tvp5150(sd);
> > > > > > > > > +
> > > > > > > > > + *std_id = decoder->lock ? tvp5150_read_std(sd) :
> > > > > > > > > V4L2_STD_UNKNOWN;
> > > > > > > >
> > > > > > > > This patch requires rework. What happens when a device doesn't
> > > > > > > > have
> > > > > > > > IRQ enabled? Perhaps it should, instead, read some register in
> > > > > > > > order
> > > > > > > > to check for the locking status, as this would work on both
> > > > > > > > cases.
> > > > > > >
> > > > > > > If IRQ isn't enabled, decoder->lock is set to always true during
> > > > > > > probe(). So this case should be fine.
> > > > > >
> > > > > > Not sure if tvp5150_read_std() will do the right thing. If it does,
> > > > > > the above could simply be:
> > > > > > std_id = tvp5150_read_std(sd);
> > > > > >
> > > > > > But, as there are 3 variants of this chipset, it sounds safer to
> > > > > > check
> > > > > > if the device is locked before calling tvp5150_read_std().
> > > > >
> > > > > Yes, I'm with you.
> > > > > >
> > > > > > IMHO, the best would be to have a patch like the one below.
> > > > > >
> > > > > > Regards,
> > > > > > Mauro
> > > > > >
> > > > > > [PATCH] media: tvp5150: implement decoder lock when irq is not used
> > > > > >
> > > > > > When irq is used, the lock is set via IRQ code. When it isn't,
> > > > > > the driver just assumes it is always locked. Instead, read the
> > > > > > lock status from the status register.
> > > > >
> > > > > Yes, that is a better solution.
> > > > > >
> > > > > > Compile-tested only.
> > > > > >
> > > > > > Signed-off-by: Mauro Carvalho Chehab
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/tvp5150.c
> > > > > > b/drivers/media/i2c/tvp5150.c
> > > > > > index 75e5ffc6573d..e07020d4053d 100644
> > > > > > --- a/drivers/media/i2c/tvp5150.c
> > > > > > +++ b/drivers/media/i2c/tvp5150.c
> > > > > > @@ -811,11 +811,24 @@ static v4l2_std_id tvp5150_read_std(struct
> > > > > > v4l2_subdev *sd)
> > > > > > }
> > > > > > }
> > > > > > +static int query_lock(struct v4l2_subdev *sd)
> > > > > > +{
> > > > > > + struct tvp5150 *decoder = to_tvp5150(sd);
> > > > > > + int status;
> > > > > > +
> > > > > > + if (decoder->irq)
> > > > > > + return decoder->lock;
> > > > > > +
> > > > > > + regmap_read(map, TVP5150_INT_STATUS_REG_A, );
> > > > > > +
> > > > > > + return (status & 0x06) == 0x06;
> > > > >
> > > > > Typo? It should be 0x80, as described in the datasheet (SLES209E) or
> > > > > just use the TVP5150_INT_A_LOCK_STATUS define. This avoid datasheet
> > > > > cross check during reading.
> > > >
> > > > Yes, it is a typo, but at the other line... I meant to use the register
> > > > 0x88, e. g.:
> > > >
> > > > regmap_read(decoder->regmap, TVP5150_STATUS_REG_1, );
> > >
> > > During my development I tried this status register too, as descibed on
> > > the community website [1]. But that wasn't that good, because the look
> > > will be lost very often. Bit7 of Interrupt Status Reg A (0xc0) is more
> > > robust for that kind of work, since it covers the whole signal.
> > >
> > > [1] http://e2e.ti.com/support/data_converters/videoconverters/f/918/p/ \
> > >
Re: [PATCH 16/22] [media] tvp5150: add querystd
On 02/08/18 10:49, Mauro Carvalho Chehab wrote:
Em Thu, 2 Aug 2018 10:01:01 +0200
Marco Felsch escreveu:
Hi Mauro,
On 18-08-01 12:50, Mauro Carvalho Chehab wrote:
Em Wed, 1 Aug 2018 16:49:26 +0200
Marco Felsch escreveu:
Hi Mauro,
On 18-08-01 11:22, Mauro Carvalho Chehab wrote:
Em Wed, 1 Aug 2018 15:21:25 +0200
Marco Felsch escreveu:
Hi Mauro,
On 18-07-30 15:09, Mauro Carvalho Chehab wrote:
Em Thu, 28 Jun 2018 18:20:48 +0200
Marco Felsch escreveu:
From: Philipp Zabel
Add the querystd video_op and make it return V4L2_STD_UNKNOWN while the
TVP5150 is not locked to a signal.
Signed-off-by: Philipp Zabel
Signed-off-by: Marco Felsch
---
drivers/media/i2c/tvp5150.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 99d887936ea0..1990aaa17749 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -796,6 +796,15 @@ static v4l2_std_id tvp5150_read_std(struct v4l2_subdev *sd)
}
}
+static int tvp5150_querystd(struct v4l2_subdev *sd, v4l2_std_id *std_id)
+{
+ struct tvp5150 *decoder = to_tvp5150(sd);
+
+ *std_id = decoder->lock ? tvp5150_read_std(sd) : V4L2_STD_UNKNOWN;
This patch requires rework. What happens when a device doesn't have
IRQ enabled? Perhaps it should, instead, read some register in order
to check for the locking status, as this would work on both cases.
If IRQ isn't enabled, decoder->lock is set to always true during
probe(). So this case should be fine.
Not sure if tvp5150_read_std() will do the right thing. If it does,
the above could simply be:
std_id = tvp5150_read_std(sd);
But, as there are 3 variants of this chipset, it sounds safer to check
if the device is locked before calling tvp5150_read_std().
Yes, I'm with you.
IMHO, the best would be to have a patch like the one below.
Regards,
Mauro
[PATCH] media: tvp5150: implement decoder lock when irq is not used
When irq is used, the lock is set via IRQ code. When it isn't,
the driver just assumes it is always locked. Instead, read the
lock status from the status register.
Yes, that is a better solution.
Compile-tested only.
Signed-off-by: Mauro Carvalho Chehab
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 75e5ffc6573d..e07020d4053d 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -811,11 +811,24 @@ static v4l2_std_id tvp5150_read_std(struct v4l2_subdev
*sd)
}
}
+static int query_lock(struct v4l2_subdev *sd)
+{
+ struct tvp5150 *decoder = to_tvp5150(sd);
+ int status;
+
+ if (decoder->irq)
+ return decoder->lock;
+
+ regmap_read(map, TVP5150_INT_STATUS_REG_A, );
+
+ return (status & 0x06) == 0x06;
Typo? It should be 0x80, as described in the datasheet (SLES209E) or
just use the TVP5150_INT_A_LOCK_STATUS define. This avoid datasheet
cross check during reading.
Yes, it is a typo, but at the other line... I meant to use the register
0x88, e. g.:
regmap_read(decoder->regmap, TVP5150_STATUS_REG_1, );
During my development I tried this status register too, as descibed on
the community website [1]. But that wasn't that good, because the look
will be lost very often. Bit7 of Interrupt Status Reg A (0xc0) is more
robust for that kind of work, since it covers the whole signal.
[1] http://e2e.ti.com/support/data_converters/videoconverters/f/918/p/ \
617120/2273276?keyMatch=tvp5150%20lock%20lost=Search-EN-Support
Bit 4 is not reliable for such purpose, but, on my tests, when video is
present, bits 1, 2 and 3 are present when there is a proper signal.
Basically, all the times I issued a std query ioctl, it reads 0x1e.
When the signal is removed, I get a 0x00.
Here, reading int status reg A returns 0x40 with or without signal,
probably because IRQ is not enabled. See, for USB devices like em28xx,
we don't have direct access to tvp5051 IRQ line. If the IRQ lines is
somewhat wired to em28xx, it could be possible that the em28xx would
handle it, but we would need to know a way to setup em28xx to handle irqs.
I don't know if this is possible, and, if so, how em28xx would be
notifying such interrupts via some URB packet.
The interrupt status register bits are latched and need a 1 written to
clear them. Normally this would be done by the interrupt handler. Maybe
this is why you're seeing the LOCK bit stuck?
Regards,
Ian
I ran some tests here: the int status reg is not updated.
Also, after thinking a little bit, I opted to not use the query_lock()
at s_stream. It makes no sense there without adding a status polling
logic. I also opted to remove initializing decoder->lock to true, as
this is very counter-intuitive. Instead, I'm adding a test at s_stream
if decoder->irq is set. This makes easier to understand the code.
Yes, you're right.
Btw, on my tests here, I noticed a problem with S-Video... at least with
Re: [RFC 4/4] dt-bindings: media: add Amlogic Meson Video Decoder Bindings
On Wed, 2018-08-01 at 21:33 +0200, Maxime Jourdan wrote: > Add documentation for the meson vdec dts node. > > Signed-off-by: Maxime Jourdan > --- > .../bindings/media/amlogic,meson-vdec.txt | 60 +++ > 1 file changed, 60 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/media/amlogic,meson-vdec.txt Maxime, when formatting your patchset, remember to put the bindings documentation before actually using them. This patch could be the first one of your series.
Re: [PATCH 16/22] [media] tvp5150: add querystd
Hi Mauro,
On 18-08-02 06:49, Mauro Carvalho Chehab wrote:
> Em Thu, 2 Aug 2018 10:01:01 +0200
> Marco Felsch escreveu:
>
> > Hi Mauro,
> >
> > On 18-08-01 12:50, Mauro Carvalho Chehab wrote:
> > > Em Wed, 1 Aug 2018 16:49:26 +0200
> > > Marco Felsch escreveu:
> > >
> > > > Hi Mauro,
> > > >
> > > > On 18-08-01 11:22, Mauro Carvalho Chehab wrote:
> > > > > Em Wed, 1 Aug 2018 15:21:25 +0200
> > > > > Marco Felsch escreveu:
> > > > >
> > > > > > Hi Mauro,
> > > > > >
> > > > > > On 18-07-30 15:09, Mauro Carvalho Chehab wrote:
> > > > > > > Em Thu, 28 Jun 2018 18:20:48 +0200
> > > > > > > Marco Felsch escreveu:
> > > > > > >
> > > > > > > > From: Philipp Zabel
> > > > > > > >
> > > > > > > > Add the querystd video_op and make it return V4L2_STD_UNKNOWN
> > > > > > > > while the
> > > > > > > > TVP5150 is not locked to a signal.
> > > > > > > >
> > > > > > > > Signed-off-by: Philipp Zabel
> > > > > > > > Signed-off-by: Marco Felsch
> > > > > > > > ---
> > > > > > > > drivers/media/i2c/tvp5150.c | 10 ++
> > > > > > > > 1 file changed, 10 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/i2c/tvp5150.c
> > > > > > > > b/drivers/media/i2c/tvp5150.c
> > > > > > > > index 99d887936ea0..1990aaa17749 100644
> > > > > > > > --- a/drivers/media/i2c/tvp5150.c
> > > > > > > > +++ b/drivers/media/i2c/tvp5150.c
> > > > > > > > @@ -796,6 +796,15 @@ static v4l2_std_id tvp5150_read_std(struct
> > > > > > > > v4l2_subdev *sd)
> > > > > > > > }
> > > > > > > > }
> > > > > > > >
> > > > > > > > +static int tvp5150_querystd(struct v4l2_subdev *sd,
> > > > > > > > v4l2_std_id *std_id)
> > > > > > > > +{
> > > > > > > > + struct tvp5150 *decoder = to_tvp5150(sd);
> > > > > > > > +
> > > > > > > > + *std_id = decoder->lock ? tvp5150_read_std(sd) :
> > > > > > > > V4L2_STD_UNKNOWN;
> > > > > > >
> > > > > > > This patch requires rework. What happens when a device doesn't
> > > > > > > have
> > > > > > > IRQ enabled? Perhaps it should, instead, read some register in
> > > > > > > order
> > > > > > > to check for the locking status, as this would work on both
> > > > > > > cases.
> > > > > >
> > > > > > If IRQ isn't enabled, decoder->lock is set to always true during
> > > > > > probe(). So this case should be fine.
> > > > >
> > > > > Not sure if tvp5150_read_std() will do the right thing. If it does,
> > > > > the above could simply be:
> > > > > std_id = tvp5150_read_std(sd);
> > > > >
> > > > > But, as there are 3 variants of this chipset, it sounds safer to check
> > > > > if the device is locked before calling tvp5150_read_std().
> > > >
> > > > Yes, I'm with you.
> > > >
> > > > >
> > > > > IMHO, the best would be to have a patch like the one below.
> > > > >
> > > > > Regards,
> > > > > Mauro
> > > > >
> > > > > [PATCH] media: tvp5150: implement decoder lock when irq is not used
> > > > >
> > > > > When irq is used, the lock is set via IRQ code. When it isn't,
> > > > > the driver just assumes it is always locked. Instead, read the
> > > > > lock status from the status register.
> > > >
> > > > Yes, that is a better solution.
> > > >
> > > > >
> > > > > Compile-tested only.
> > > > >
> > > > > Signed-off-by: Mauro Carvalho Chehab
> > > > >
> > > > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > > > > index 75e5ffc6573d..e07020d4053d 100644
> > > > > --- a/drivers/media/i2c/tvp5150.c
> > > > > +++ b/drivers/media/i2c/tvp5150.c
> > > > > @@ -811,11 +811,24 @@ static v4l2_std_id tvp5150_read_std(struct
> > > > > v4l2_subdev *sd)
> > > > > }
> > > > > }
> > > > >
> > > > > +static int query_lock(struct v4l2_subdev *sd)
> > > > > +{
> > > > > + struct tvp5150 *decoder = to_tvp5150(sd);
> > > > > + int status;
> > > > > +
> > > > > + if (decoder->irq)
> > > > > + return decoder->lock;
> > > > > +
> > > > > + regmap_read(map, TVP5150_INT_STATUS_REG_A, );
> > > > > +
> > > > > + return (status & 0x06) == 0x06;
> > > >
> > > > Typo? It should be 0x80, as described in the datasheet (SLES209E) or
> > > > just use the TVP5150_INT_A_LOCK_STATUS define. This avoid datasheet
> > > > cross check during reading.
> > >
> > > Yes, it is a typo, but at the other line... I meant to use the register
> > > 0x88, e. g.:
> > >
> > > regmap_read(decoder->regmap, TVP5150_STATUS_REG_1, );
> >
> > During my development I tried this status register too, as descibed on
> > the community website [1]. But that wasn't that good, because the look
> > will be lost very often. Bit7 of Interrupt Status Reg A (0xc0) is more
> > robust for that kind of work, since it covers the whole signal.
> >
> > [1] http://e2e.ti.com/support/data_converters/videoconverters/f/918/p/ \
> > 617120/2273276?keyMatch=tvp5150%20lock%20lost=Search-EN-Support
>
> Bit 4 is not reliable for such purpose, but, on my tests, when video is
> present, bits 1,
Re: [PATCH 16/22] [media] tvp5150: add querystd
Em Thu, 2 Aug 2018 10:01:01 +0200
Marco Felsch escreveu:
> Hi Mauro,
>
> On 18-08-01 12:50, Mauro Carvalho Chehab wrote:
> > Em Wed, 1 Aug 2018 16:49:26 +0200
> > Marco Felsch escreveu:
> >
> > > Hi Mauro,
> > >
> > > On 18-08-01 11:22, Mauro Carvalho Chehab wrote:
> > > > Em Wed, 1 Aug 2018 15:21:25 +0200
> > > > Marco Felsch escreveu:
> > > >
> > > > > Hi Mauro,
> > > > >
> > > > > On 18-07-30 15:09, Mauro Carvalho Chehab wrote:
> > > > > > Em Thu, 28 Jun 2018 18:20:48 +0200
> > > > > > Marco Felsch escreveu:
> > > > > >
> > > > > > > From: Philipp Zabel
> > > > > > >
> > > > > > > Add the querystd video_op and make it return V4L2_STD_UNKNOWN
> > > > > > > while the
> > > > > > > TVP5150 is not locked to a signal.
> > > > > > >
> > > > > > > Signed-off-by: Philipp Zabel
> > > > > > > Signed-off-by: Marco Felsch
> > > > > > > ---
> > > > > > > drivers/media/i2c/tvp5150.c | 10 ++
> > > > > > > 1 file changed, 10 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/media/i2c/tvp5150.c
> > > > > > > b/drivers/media/i2c/tvp5150.c
> > > > > > > index 99d887936ea0..1990aaa17749 100644
> > > > > > > --- a/drivers/media/i2c/tvp5150.c
> > > > > > > +++ b/drivers/media/i2c/tvp5150.c
> > > > > > > @@ -796,6 +796,15 @@ static v4l2_std_id tvp5150_read_std(struct
> > > > > > > v4l2_subdev *sd)
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > > +static int tvp5150_querystd(struct v4l2_subdev *sd, v4l2_std_id
> > > > > > > *std_id)
> > > > > > > +{
> > > > > > > + struct tvp5150 *decoder = to_tvp5150(sd);
> > > > > > > +
> > > > > > > + *std_id = decoder->lock ? tvp5150_read_std(sd) :
> > > > > > > V4L2_STD_UNKNOWN;
> > > > > >
> > > > > > This patch requires rework. What happens when a device doesn't have
> > > > > > IRQ enabled? Perhaps it should, instead, read some register in order
> > > > > > to check for the locking status, as this would work on both cases.
> > > > > >
> > > > >
> > > > > If IRQ isn't enabled, decoder->lock is set to always true during
> > > > > probe(). So this case should be fine.
> > > >
> > > > Not sure if tvp5150_read_std() will do the right thing. If it does,
> > > > the above could simply be:
> > > > std_id = tvp5150_read_std(sd);
> > > >
> > > > But, as there are 3 variants of this chipset, it sounds safer to check
> > > > if the device is locked before calling tvp5150_read_std().
> > >
> > > Yes, I'm with you.
> > >
> > > >
> > > > IMHO, the best would be to have a patch like the one below.
> > > >
> > > > Regards,
> > > > Mauro
> > > >
> > > > [PATCH] media: tvp5150: implement decoder lock when irq is not used
> > > >
> > > > When irq is used, the lock is set via IRQ code. When it isn't,
> > > > the driver just assumes it is always locked. Instead, read the
> > > > lock status from the status register.
> > >
> > > Yes, that is a better solution.
> > >
> > > >
> > > > Compile-tested only.
> > > >
> > > > Signed-off-by: Mauro Carvalho Chehab
> > > >
> > > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > > > index 75e5ffc6573d..e07020d4053d 100644
> > > > --- a/drivers/media/i2c/tvp5150.c
> > > > +++ b/drivers/media/i2c/tvp5150.c
> > > > @@ -811,11 +811,24 @@ static v4l2_std_id tvp5150_read_std(struct
> > > > v4l2_subdev *sd)
> > > > }
> > > > }
> > > >
> > > > +static int query_lock(struct v4l2_subdev *sd)
> > > > +{
> > > > + struct tvp5150 *decoder = to_tvp5150(sd);
> > > > + int status;
> > > > +
> > > > + if (decoder->irq)
> > > > + return decoder->lock;
> > > > +
> > > > + regmap_read(map, TVP5150_INT_STATUS_REG_A, );
> > > > +
> > > > + return (status & 0x06) == 0x06;
> > >
> > > Typo? It should be 0x80, as described in the datasheet (SLES209E) or
> > > just use the TVP5150_INT_A_LOCK_STATUS define. This avoid datasheet
> > > cross check during reading.
> >
> > Yes, it is a typo, but at the other line... I meant to use the register
> > 0x88, e. g.:
> >
> > regmap_read(decoder->regmap, TVP5150_STATUS_REG_1, );
>
> During my development I tried this status register too, as descibed on
> the community website [1]. But that wasn't that good, because the look
> will be lost very often. Bit7 of Interrupt Status Reg A (0xc0) is more
> robust for that kind of work, since it covers the whole signal.
>
> [1] http://e2e.ti.com/support/data_converters/videoconverters/f/918/p/ \
> 617120/2273276?keyMatch=tvp5150%20lock%20lost=Search-EN-Support
Bit 4 is not reliable for such purpose, but, on my tests, when video is
present, bits 1, 2 and 3 are present when there is a proper signal.
Basically, all the times I issued a std query ioctl, it reads 0x1e.
When the signal is removed, I get a 0x00.
Here, reading int status reg A returns 0x40 with or without signal,
probably because IRQ is not enabled. See, for USB devices like em28xx,
we don't have direct access to tvp5051
Re: [PATCH 00/13] Better handle pads for tuning/decoder part of the devices
Em Thu, 2 Aug 2018 11:12:23 +0200 Hans Verkuil escreveu: > On 08/01/18 17:55, Mauro Carvalho Chehab wrote: > > At PC consumer devices, it is very common that the bridge same driver > > to be attached to different types of tuners and demods. We need a way > > for the Kernel to properly identify what kind of signal is provided by each > > PAD, in order to properly setup the pipelines. > > > > The previous approach were to hardcode a fixed number of PADs for all > > elements of the same type. This is not good, as different devices may > > actually have a different number of pads. > > > > It was acceptable in the past, as there were a promisse of adding "soon" > > a properties API that would allow to identify the type for each PADs, but > > this was never merged (or even a patchset got submitted). > > > > So, replace this approach by another one: add a "taint" mark to pads that > > contain different types of signals. > > > > I tried to minimize the number of signals, in order to make it simpler to > > convert from the past way. > > > > For now, it is tested only with a simple grabber device. I intend to do > > more tests before merging it, but it would be interesting to have this > > merged for Kernel 4.19, as we'll now be exposing the pad index via > > the MC API version 2. > > Other than a small comment for the last patch I didn't see anything > problematical in this series. It doesn't touch on the public API or > on any of the non-tuner drivers. So for patches 1-12: > > Acked-by: Hans Verkuil > > And after adding back the documentation for the enums in patch 13 you > can add my Ack to that one as well. Thank you! I changed patch 13 to keep the documentation and added your ack: https://git.linuxtv.org/mchehab/experimental.git/log/?h=pad-fix-3 Thanks, Mauro
Re: [PATCH 13/13] media: v4l2-mc: get rid of global pad indexes
Em Thu, 2 Aug 2018 11:08:52 +0200
Hans Verkuil escreveu:
> On 08/01/18 17:55, Mauro Carvalho Chehab wrote:
> > Now that all drivers are using pad signal types, we can get
> > rid of the global static definition, as routes are stablished
> > using the pad signal type.
> >
> > The tuner and IF-PLL pads are now used only by the tuner core,
> > so move the definitions to be there.
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > ---
> > drivers/media/v4l2-core/tuner-core.c | 13 +
> > include/media/v4l2-mc.h | 76
> > 2 files changed, 13 insertions(+), 76 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/tuner-core.c
> > b/drivers/media/v4l2-core/tuner-core.c
> > index d4c32ccd0930..e35438ca0b50 100644
> > --- a/drivers/media/v4l2-core/tuner-core.c
> > +++ b/drivers/media/v4l2-core/tuner-core.c
> > @@ -97,6 +97,19 @@ static const struct v4l2_subdev_ops tuner_ops;
> > * Internal struct used inside the driver
> > */
> >
> > +enum tuner_pad_index {
> > + TUNER_PAD_RF_INPUT,
> > + TUNER_PAD_OUTPUT,
> > + TUNER_PAD_AUD_OUT,
> > + TUNER_NUM_PADS
> > +};
> > +
> > +enum if_vid_dec_pad_index {
> > + IF_VID_DEC_PAD_IF_INPUT,
> > + IF_VID_DEC_PAD_OUT,
> > + IF_VID_DEC_PAD_NUM_PADS
> > +};
>
> Shouldn't the enum documentation be copied as well instead of just the
> enums themselves?
I was in doubt about that too :-)
When this was global, documentation was a need, as all drivers should
do the same. Now that it is local, and all external parties use are
the sig_types, the information became less relevant, being internal to
tuner-core.
Yet, it doesn't hurt copying the documentation here, so I'll add it.
See enclosed.
Thanks,
Mauro
[PATCHv2 13/13] media: v4l2-mc: get rid of global pad indexes
Now that all drivers are using pad signal types, we can get
rid of the global static definition, as routes are stablished
using the pad signal type.
The tuner and IF-PLL pads are now used only by the tuner core,
so move the definitions to be there.
Signed-off-by: Mauro Carvalho Chehab
diff --git a/drivers/media/v4l2-core/tuner-core.c
b/drivers/media/v4l2-core/tuner-core.c
index d4c32ccd0930..47228145473f 100644
--- a/drivers/media/v4l2-core/tuner-core.c
+++ b/drivers/media/v4l2-core/tuner-core.c
@@ -94,9 +94,56 @@ static const struct v4l2_subdev_ops tuner_ops;
} while (0)
/*
- * Internal struct used inside the driver
+ * Internal enums/struct used inside the driver
*/
+/**
+ * enum tuner_pad_index - tuner pad index for MEDIA_ENT_F_TUNER
+ *
+ * @TUNER_PAD_RF_INPUT:
+ * Radiofrequency (RF) sink pad, usually linked to a RF connector entity.
+ * @TUNER_PAD_OUTPUT:
+ * tuner video output source pad. Contains the video chrominance
+ * and luminance or the hole bandwidth of the signal converted to
+ * an Intermediate Frequency (IF) or to baseband (on zero-IF tuners).
+ * @TUNER_PAD_AUD_OUT:
+ * Tuner audio output source pad. Tuners used to decode analog TV
+ * signals have an extra pad for audio output. Old tuners use an
+ * analog stage with a saw filter for the audio IF frequency. The
+ * output of the pad is, in this case, the audio IF, with should be
+ * decoded either by the bridge chipset (that's the case of cx2388x
+ * chipsets) or may require an external IF sound processor, like
+ * msp34xx. On modern silicon tuners, the audio IF decoder is usually
+ * incorporated at the tuner. On such case, the output of this pad
+ * is an audio sampled data.
+ * @TUNER_NUM_PADS:
+ * Number of pads of the tuner.
+ */
+enum tuner_pad_index {
+ TUNER_PAD_RF_INPUT,
+ TUNER_PAD_OUTPUT,
+ TUNER_PAD_AUD_OUT,
+ TUNER_NUM_PADS
+};
+
+/**
+ * enum if_vid_dec_pad_index - video IF-PLL pad index
+ * for MEDIA_ENT_F_IF_VID_DECODER
+ *
+ * @IF_VID_DEC_PAD_IF_INPUT:
+ * video Intermediate Frequency (IF) sink pad
+ * @IF_VID_DEC_PAD_OUT:
+ * IF-PLL video output source pad. Contains the video chrominance
+ * and luminance IF signals.
+ * @IF_VID_DEC_PAD_NUM_PADS:
+ * Number of pads of the video IF-PLL.
+ */
+enum if_vid_dec_pad_index {
+ IF_VID_DEC_PAD_IF_INPUT,
+ IF_VID_DEC_PAD_OUT,
+ IF_VID_DEC_PAD_NUM_PADS
+};
+
struct tuner {
/* device */
struct dvb_frontend fe;
diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
index 7c9c781b16a9..bf5043c1ab6b 100644
--- a/include/media/v4l2-mc.h
+++ b/include/media/v4l2-mc.h
@@ -23,82 +23,6 @@
#include
#include
-/**
- * enum tuner_pad_index - tuner pad index for MEDIA_ENT_F_TUNER
- *
- * @TUNER_PAD_RF_INPUT:Radiofrequency (RF) sink pad, usually linked to
a
- * RF connector entity.
- * @TUNER_PAD_OUTPUT: Tuner video output source pad. Contains the video
- * chrominance and luminance or the hole bandwidth
- * of the signal converted to an Intermediate Frequency
- * (IF) or to baseband
Re: [RFC 0/4] media: meson: add video decoder driver
On Wed, 2018-08-01 at 21:33 +0200, Maxime Jourdan wrote: > This is a Request for Comments for the amlogic (meson) video decoder driver. > It is written around the V4L2 M2M framework without using the Request > API as there are a hardware bitstream parser and firmwares. > > It features decoding for: > - MPEG 1/2/4, H.263, H.264, MJPEG, HEVC 8-bit (partial) > > Even though they are supported in hardware, it doesn't leverage support for: > - HEVC 10-bit, VP9, VC1 (all those are in TODOs) > > The output is multiplanar NV12 (V4L2_PIX_FMT_NV12M). > Supported SoCs are: GXBB (S905), GXL (S905X/W/D), GXM (S912) > It was tested primarily with FFmpeg, GStreamer and kodi. > > The file hierarchy can be boiled down to: > > | codec_h264.c > | codec_mjpeg.c > | codec_mpeg4.c > | vdec_1.c -->| codec_mpeg12.c > vdec.c -->| vdec_hevc.c -->| codec_hevc.c > | esparser.c > > The V4L2 code is handled mostly in vdec.c. > Each VDEC and CODEC unit is accessed via ops structs to facilitate the code. > > The arrangement between vdecs and codecs can be seen in vdec_platform.c > This file also declares things like pixfmts, min/max buffers and firmware > paths > for each SoC. > > Specific questions about the code: > > - While I do use the platform's general clks and resets tied to the vdec in > a nice way (dts + clock/reset controller with clk/reset frameworks), > there are some subclocks and resets that I use in the driver by writing > directly to registers. e.g: > > - writel_relaxed((1<<7) | (1<<6), core->dos_base + DOS_SW_RESET0); > - writel_relaxed(0x3ff, core->dos_base + DOS_GCLK_EN0); Answering for the clocks here: - There is no obligation to use the clock framework to model your clocks. It is just very convenient when we need to handle clk parenting (rate, enable) because you don't need to worry about setting the whole tree (It is not the only reason to use CCF of course) ... but this is assuming you have been able to model this tree in CCF. If you know nothing about the clock and you just know that you need to flip this bit to 'make it work' then don't bother with CCF for now. > > and a few other instances where that happens. > > Is it okay to not create specific controllers for those ? It depends what you mean by controller: - Any device may add a clk to CCF (call clk(_hw)_register()). the clk pointer will just be available to the related driver. As described above, this might be useful to propagate your changes to the rest of the tree. - A device may also be a provider. After registering, it may call of_clk_add_hw_provider() (or any similar function). Other devices will then be able to request the clocks this device has registered. This is useful when the clock is in region by one device and used by another device. > The main issue is > the lack of documentation so I don't know which resets/clocks are impacted by > those writes. > The only thing I'm certain of is that they only apply to the vdec/esparser. > > - I tend to call vdec_* functions from the codec handlers. > > For instance, codec_h264 will call vdec_dst_buf_done_idx to DONE > a capture buffer. vdec_dst_buf_done_idx is as such a public symbol. > > Should I use an ops struct for those instead, so that the codec handlers > don't depend directly on the vdec general code ? > > - Naming: my public symbols either start with vdec_* or esparser_* > > Should I change that to something meson/amlogic specific ? That what we tend to do yes. If the symbol is not static, you should probably have a prefix like 'meson_gx_' ... maybe 'meson_gx_vdec'. Another (amlogic) platform may come along someday, with another 'type' of vdec. If the symbol is static, then maybe vdec_ is enough. > > - I have a _lot_ of writel_relaxed calls. > > Can I leave them be or is there a nicer way to do it ? > > - Since the decoder is single instance, I only allow one _open at a time. > > However the v4l2 compliance suite complains about this. > How should I safely make it single instance ? Not allowing multiple > start_streaming ? > > - I am getting these 2 fails, but unsure what they are about: > > Buffer ioctls: > fail: > ../../../v4l-utils-1.12.3/utils/v4l2-compliance/v4l2-test-buffers.cpp(428): > node->node2 == NULL > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL > fail: > ../../../v4l-utils-1.12.3/utils/v4l2-compliance/v4l2-test-buffers.cpp(571): > q.has_expbuf(node) > test VIDIOC_EXPBUF: FAIL > > > > And of course, I will gladly accept any kind of other feedback you would have. > > Thanks! > > > Maxime Jourdan (4): > media: meson: add v4l2 m2m video decoder driver > ARM64: dts: meson-gx: add vdec entry > ARM64: dts: meson: add vdec entries > dt-bindings: media: add Amlogic Meson Video Decoder Bindings > > .../bindings/media/amlogic,meson-vdec.txt | 60 + > arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 14 + >
Re: [PATCH 00/13] Better handle pads for tuning/decoder part of the devices
On 08/01/18 17:55, Mauro Carvalho Chehab wrote: > At PC consumer devices, it is very common that the bridge same driver > to be attached to different types of tuners and demods. We need a way > for the Kernel to properly identify what kind of signal is provided by each > PAD, in order to properly setup the pipelines. > > The previous approach were to hardcode a fixed number of PADs for all > elements of the same type. This is not good, as different devices may > actually have a different number of pads. > > It was acceptable in the past, as there were a promisse of adding "soon" > a properties API that would allow to identify the type for each PADs, but > this was never merged (or even a patchset got submitted). > > So, replace this approach by another one: add a "taint" mark to pads that > contain different types of signals. > > I tried to minimize the number of signals, in order to make it simpler to > convert from the past way. > > For now, it is tested only with a simple grabber device. I intend to do > more tests before merging it, but it would be interesting to have this > merged for Kernel 4.19, as we'll now be exposing the pad index via > the MC API version 2. Other than a small comment for the last patch I didn't see anything problematical in this series. It doesn't touch on the public API or on any of the non-tuner drivers. So for patches 1-12: Acked-by: Hans Verkuil And after adding back the documentation for the enums in patch 13 you can add my Ack to that one as well. Regards, Hans > > Mauro Carvalho Chehab (13): > media: v4l2: remove VBI output pad > media: v4l2: taint pads with the signal types for consumer devices > v4l2-mc: switch it to use the new approach to setup pipelines > media: dvb: use signals to discover pads > media: au0828: use signals instead of hardcoding a pad number > media: au8522: declare its own pads > media: msp3400: declare its own pads > media: saa7115: declare its own pads > media: tvp5150: declare its own pads > media: si2157: declare its own pads > media: saa7134: declare its own pads > media: mxl111sf: declare its own pads > media: v4l2-mc: get rid of global pad indexes > > drivers/media/dvb-core/dvbdev.c | 19 +++-- > drivers/media/dvb-frontends/au8522_decoder.c | 10 ++- > drivers/media/dvb-frontends/au8522_priv.h| 9 ++- > drivers/media/i2c/msp3400-driver.c | 6 +- > drivers/media/i2c/msp3400-driver.h | 8 +- > drivers/media/i2c/saa7115.c | 18 +++-- > drivers/media/i2c/tvp5150.c | 21 -- > drivers/media/media-entity.c | 26 +++ > drivers/media/pci/saa7134/saa7134-core.c | 9 ++- > drivers/media/pci/saa7134/saa7134.h | 8 +- > drivers/media/tuners/si2157.c| 11 ++- > drivers/media/tuners/si2157_priv.h | 9 ++- > drivers/media/usb/au0828/au0828-core.c | 12 +-- > drivers/media/usb/dvb-usb-v2/mxl111sf.c | 8 +- > drivers/media/usb/dvb-usb-v2/mxl111sf.h | 8 +- > drivers/media/v4l2-core/tuner-core.c | 18 + > drivers/media/v4l2-core/v4l2-mc.c| 73 +- > include/media/media-entity.h | 54 ++ > include/media/v4l2-mc.h | 78 > 19 files changed, 266 insertions(+), 139 deletions(-) >
Re: [PATCH 13/13] media: v4l2-mc: get rid of global pad indexes
On 08/01/18 17:55, Mauro Carvalho Chehab wrote:
> Now that all drivers are using pad signal types, we can get
> rid of the global static definition, as routes are stablished
> using the pad signal type.
>
> The tuner and IF-PLL pads are now used only by the tuner core,
> so move the definitions to be there.
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/v4l2-core/tuner-core.c | 13 +
> include/media/v4l2-mc.h | 76
> 2 files changed, 13 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/tuner-core.c
> b/drivers/media/v4l2-core/tuner-core.c
> index d4c32ccd0930..e35438ca0b50 100644
> --- a/drivers/media/v4l2-core/tuner-core.c
> +++ b/drivers/media/v4l2-core/tuner-core.c
> @@ -97,6 +97,19 @@ static const struct v4l2_subdev_ops tuner_ops;
> * Internal struct used inside the driver
> */
>
> +enum tuner_pad_index {
> + TUNER_PAD_RF_INPUT,
> + TUNER_PAD_OUTPUT,
> + TUNER_PAD_AUD_OUT,
> + TUNER_NUM_PADS
> +};
> +
> +enum if_vid_dec_pad_index {
> + IF_VID_DEC_PAD_IF_INPUT,
> + IF_VID_DEC_PAD_OUT,
> + IF_VID_DEC_PAD_NUM_PADS
> +};
Shouldn't the enum documentation be copied as well instead of just the
enums themselves?
Regards,
Hans
> +
> struct tuner {
> /* device */
> struct dvb_frontend fe;
> diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> index 7c9c781b16a9..bf5043c1ab6b 100644
> --- a/include/media/v4l2-mc.h
> +++ b/include/media/v4l2-mc.h
> @@ -23,82 +23,6 @@
> #include
> #include
>
> -/**
> - * enum tuner_pad_index - tuner pad index for MEDIA_ENT_F_TUNER
> - *
> - * @TUNER_PAD_RF_INPUT: Radiofrequency (RF) sink pad, usually linked to
> a
> - * RF connector entity.
> - * @TUNER_PAD_OUTPUT:Tuner video output source pad. Contains the
> video
> - * chrominance and luminance or the hole bandwidth
> - * of the signal converted to an Intermediate Frequency
> - * (IF) or to baseband (on zero-IF tuners).
> - * @TUNER_PAD_AUD_OUT: Tuner audio output source pad. Tuners used to
> decode
> - * analog TV signals have an extra pad for audio output.
> - * Old tuners use an analog stage with a saw filter for
> - * the audio IF frequency. The output of the pad is, in
> - * this case, the audio IF, with should be decoded either
> - * by the bridge chipset (that's the case of cx2388x
> - * chipsets) or may require an external IF sound
> - * processor, like msp34xx. On modern silicon tuners,
> - * the audio IF decoder is usually incorporated at the
> - * tuner. On such case, the output of this pad is an
> - * audio sampled data.
> - * @TUNER_NUM_PADS: Number of pads of the tuner.
> - */
> -enum tuner_pad_index {
> - TUNER_PAD_RF_INPUT,
> - TUNER_PAD_OUTPUT,
> - TUNER_PAD_AUD_OUT,
> - TUNER_NUM_PADS
> -};
> -
> -/**
> - * enum if_vid_dec_pad_index - video IF-PLL pad index for
> - * MEDIA_ENT_F_IF_VID_DECODER
> - *
> - * @IF_VID_DEC_PAD_IF_INPUT: video Intermediate Frequency (IF) sink pad
> - * @IF_VID_DEC_PAD_OUT: IF-PLL video output source pad.
> Contains the
> - * video chrominance and luminance IF signals.
> - * @IF_VID_DEC_PAD_NUM_PADS: Number of pads of the video IF-PLL.
> - */
> -enum if_vid_dec_pad_index {
> - IF_VID_DEC_PAD_IF_INPUT,
> - IF_VID_DEC_PAD_OUT,
> - IF_VID_DEC_PAD_NUM_PADS
> -};
> -
> -/**
> - * enum if_aud_dec_pad_index - audio/sound IF-PLL pad index for
> - * MEDIA_ENT_F_IF_AUD_DECODER
> - *
> - * @IF_AUD_DEC_PAD_IF_INPUT: audio Intermediate Frequency (IF) sink pad
> - * @IF_AUD_DEC_PAD_OUT: IF-PLL audio output source pad.
> Contains the
> - * audio sampled stream data, usually connected
> - * to the bridge bus via an Inter-IC Sound (I2S)
> - * bus.
> - * @IF_AUD_DEC_PAD_NUM_PADS: Number of pads of the audio IF-PLL.
> - */
> -enum if_aud_dec_pad_index {
> - IF_AUD_DEC_PAD_IF_INPUT,
> - IF_AUD_DEC_PAD_OUT,
> - IF_AUD_DEC_PAD_NUM_PADS
> -};
> -
> -/**
> - * enum demod_pad_index - analog TV pad index for MEDIA_ENT_F_ATV_DECODER
> - *
> - * @DEMOD_PAD_IF_INPUT: IF input sink pad.
> - * @DEMOD_PAD_VID_OUT: Video output source pad.
> - * @DEMOD_PAD_AUDIO_OUT: Audio output source pad.
> - * @DEMOD_NUM_PADS: Maximum number of output pads.
> - */
> -enum demod_pad_index {
> - DEMOD_PAD_IF_INPUT,
> - DEMOD_PAD_VID_OUT,
> - DEMOD_PAD_AUDIO_OUT,
> - DEMOD_NUM_PADS
> -};
> -
> /* We don't need to include pci.h or usb.h here */
> struct pci_dev;
> struct usb_device;
>
Re: [PATCH 3/3] media: add Rockchip VPU driver
On 08/01/18 23:07, Ezequiel Garcia wrote:
> Add a mem2mem driver for the VPU available on Rockchip SoCs.
> Currently only JPEG encoding is supported, for RK3399 and RK3288
> platforms.
>
> Signed-off-by: Ezequiel Garcia
> ---
> drivers/media/platform/Kconfig| 12 +
> drivers/media/platform/Makefile | 1 +
> drivers/media/platform/rockchip/vpu/Makefile | 8 +
> .../platform/rockchip/vpu/rk3288_vpu_hw.c | 127 +++
> .../rockchip/vpu/rk3288_vpu_hw_jpege.c| 156
> .../platform/rockchip/vpu/rk3288_vpu_regs.h | 442 ++
> .../platform/rockchip/vpu/rk3399_vpu_hw.c | 127 +++
> .../rockchip/vpu/rk3399_vpu_hw_jpege.c| 165
> .../platform/rockchip/vpu/rk3399_vpu_regs.h | 601 ++
> .../platform/rockchip/vpu/rockchip_vpu.h | 270 +++
> .../platform/rockchip/vpu/rockchip_vpu_drv.c | 416 ++
> .../platform/rockchip/vpu/rockchip_vpu_enc.c | 763 ++
> .../platform/rockchip/vpu/rockchip_vpu_enc.h | 25 +
> .../platform/rockchip/vpu/rockchip_vpu_hw.h | 67 ++
There is no update for the MAINTAINERS file, please add an entry there.
> 14 files changed, 3180 insertions(+)
> create mode 100644 drivers/media/platform/rockchip/vpu/Makefile
> create mode 100644 drivers/media/platform/rockchip/vpu/rk3288_vpu_hw.c
> create mode 100644 drivers/media/platform/rockchip/vpu/rk3288_vpu_hw_jpege.c
> create mode 100644 drivers/media/platform/rockchip/vpu/rk3288_vpu_regs.h
> create mode 100644 drivers/media/platform/rockchip/vpu/rk3399_vpu_hw.c
> create mode 100644 drivers/media/platform/rockchip/vpu/rk3399_vpu_hw_jpege.c
> create mode 100644 drivers/media/platform/rockchip/vpu/rk3399_vpu_regs.h
> create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu.h
> create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu_drv.c
> create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu_enc.c
> create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu_enc.h
> create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu_hw.h
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 2728376b04b5..7244a2360732 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -448,6 +448,18 @@ config VIDEO_ROCKCHIP_RGA
>
> To compile this driver as a module choose m here.
>
> +config VIDEO_ROCKCHIP_VPU
> + tristate "Rockchip VPU driver"
> + depends on VIDEO_DEV && VIDEO_V4L2
> + depends on ARCH_ROCKCHIP || COMPILE_TEST
> + select VIDEOBUF2_DMA_CONTIG
> + select V4L2_MEM2MEM_DEV
> + default n
> + ---help---
> + Support for the VPU video codec found on Rockchip SoC.
> + To compile this driver as a module, choose M here: the module
> + will be called rockchip-vpu.
> +
> config VIDEO_TI_VPE
> tristate "TI VPE (Video Processing Engine) driver"
> depends on VIDEO_DEV && VIDEO_V4L2
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 04bc1502a30e..83378180d8ac 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_VIDEO_RENESAS_JPU) += rcar_jpu.o
> obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1/
>
> obj-$(CONFIG_VIDEO_ROCKCHIP_RGA) += rockchip/rga/
> +obj-$(CONFIG_VIDEO_ROCKCHIP_VPU)+= rockchip/vpu/
>
> obj-y+= omap/
>
> diff --git a/drivers/media/platform/rockchip/vpu/Makefile
> b/drivers/media/platform/rockchip/vpu/Makefile
> new file mode 100644
> index ..cab0123c49d4
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/vpu/Makefile
> @@ -0,0 +1,8 @@
> +obj-$(CONFIG_VIDEO_ROCKCHIP_VPU) += rockchip-vpu.o
> +
> +rockchip-vpu-y += rockchip_vpu_drv.o \
> + rockchip_vpu_enc.o \
> + rk3288_vpu_hw.o \
> + rk3288_vpu_hw_jpege.o \
> + rk3399_vpu_hw.o \
> + rk3399_vpu_hw_jpege.o
> diff --git a/drivers/media/platform/rockchip/vpu/rk3288_vpu_hw.c
> b/drivers/media/platform/rockchip/vpu/rk3288_vpu_hw.c
> new file mode 100644
> index ..0caff8ecf343
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/vpu/rk3288_vpu_hw.c
> @@ -0,0 +1,127 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip VPU codec driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + * Jeffy Chen
> + */
> +
> +#include
> +
> +#include "rockchip_vpu.h"
> +#include "rk3288_vpu_regs.h"
> +
> +#define RK3288_ACLK_MAX_FREQ (400 * 1000 * 1000)
> +
> +/*
> + * Supported formats.
> + */
> +
> +static const struct rockchip_vpu_fmt rk3288_vpu_enc_fmts[] = {
> + /* Source formats. */
> + {
> + .name = "4:2:0 3 planes Y/Cb/Cr",
Please drop the name field. This is filled in by v4l2-ioctl.c in its
enum_fmt implementation.
> + .fourcc = V4L2_PIX_FMT_YUV420M,
> + .codec_mode =
Re: [RFC 0/4] media: meson: add video decoder driver
On 01/08/2018 21:33, Maxime Jourdan wrote: > This is a Request for Comments for the amlogic (meson) video decoder driver. > It is written around the V4L2 M2M framework without using the Request > API as there are a hardware bitstream parser and firmwares. > > It features decoding for: > - MPEG 1/2/4, H.263, H.264, MJPEG, HEVC 8-bit (partial) > > Even though they are supported in hardware, it doesn't leverage support for: > - HEVC 10-bit, VP9, VC1 (all those are in TODOs) > > The output is multiplanar NV12 (V4L2_PIX_FMT_NV12M). > Supported SoCs are: GXBB (S905), GXL (S905X/W/D), GXM (S912) > It was tested primarily with FFmpeg, GStreamer and kodi. > > The file hierarchy can be boiled down to: > > | codec_h264.c > | codec_mjpeg.c > | codec_mpeg4.c > | vdec_1.c -->| codec_mpeg12.c > vdec.c -->| vdec_hevc.c -->| codec_hevc.c > | esparser.c > > The V4L2 code is handled mostly in vdec.c. > Each VDEC and CODEC unit is accessed via ops structs to facilitate the code. > > The arrangement between vdecs and codecs can be seen in vdec_platform.c > This file also declares things like pixfmts, min/max buffers and firmware > paths > for each SoC. > > Specific questions about the code: > > - While I do use the platform's general clks and resets tied to the vdec in > a nice way (dts + clock/reset controller with clk/reset frameworks), > there are some subclocks and resets that I use in the driver by writing > directly to registers. e.g: > > - writel_relaxed((1<<7) | (1<<6), core->dos_base + DOS_SW_RESET0); This seems to be internal resets > - writel_relaxed(0x3ff, core->dos_base + DOS_GCLK_EN0); This seems to be internal gates If it's in the VDEC/DO registers space, you can keep this internaly, no need to use the clk or reset framework. Neil > > and a few other instances where that happens. > > Is it okay to not create specific controllers for those ? The main issue is > the lack of documentation so I don't know which resets/clocks are impacted by > those writes. > The only thing I'm certain of is that they only apply to the vdec/esparser. > > - I tend to call vdec_* functions from the codec handlers. > > For instance, codec_h264 will call vdec_dst_buf_done_idx to DONE > a capture buffer. vdec_dst_buf_done_idx is as such a public symbol. > > Should I use an ops struct for those instead, so that the codec handlers > don't depend directly on the vdec general code ? > > - Naming: my public symbols either start with vdec_* or esparser_* > > Should I change that to something meson/amlogic specific ? > > - I have a _lot_ of writel_relaxed calls. > > Can I leave them be or is there a nicer way to do it ? > > - Since the decoder is single instance, I only allow one _open at a time. > > However the v4l2 compliance suite complains about this. > How should I safely make it single instance ? Not allowing multiple > start_streaming ? > > - I am getting these 2 fails, but unsure what they are about: > > Buffer ioctls: > fail: > ../../../v4l-utils-1.12.3/utils/v4l2-compliance/v4l2-test-buffers.cpp(428): > node->node2 == NULL > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL > fail: > ../../../v4l-utils-1.12.3/utils/v4l2-compliance/v4l2-test-buffers.cpp(571): > q.has_expbuf(node) > test VIDIOC_EXPBUF: FAIL > > > > And of course, I will gladly accept any kind of other feedback you would have. > > Thanks! > > > Maxime Jourdan (4): > media: meson: add v4l2 m2m video decoder driver > ARM64: dts: meson-gx: add vdec entry > ARM64: dts: meson: add vdec entries > dt-bindings: media: add Amlogic Meson Video Decoder Bindings > > .../bindings/media/amlogic,meson-vdec.txt | 60 + > arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 14 + > arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi |8 + > arch/arm64/boot/dts/amlogic/meson-gxl.dtsi|8 + > arch/arm64/boot/dts/amlogic/meson-gxm.dtsi|4 + > drivers/media/platform/Kconfig| 10 + > drivers/media/platform/meson/Makefile |1 + > drivers/media/platform/meson/vdec/Makefile|7 + > drivers/media/platform/meson/vdec/canvas.c| 69 + > drivers/media/platform/meson/vdec/canvas.h| 42 + > .../media/platform/meson/vdec/codec_h264.c| 376 + > .../media/platform/meson/vdec/codec_h264.h| 13 + > .../media/platform/meson/vdec/codec_helpers.c | 45 + > .../media/platform/meson/vdec/codec_helpers.h |8 + > .../media/platform/meson/vdec/codec_hevc.c| 1383 + > .../media/platform/meson/vdec/codec_hevc.h| 13 + > .../media/platform/meson/vdec/codec_mjpeg.c | 203 +++ > .../media/platform/meson/vdec/codec_mjpeg.h | 13 + > .../media/platform/meson/vdec/codec_mpeg12.c | 183 +++ > .../media/platform/meson/vdec/codec_mpeg12.h | 13 + > .../media/platform/meson/vdec/codec_mpeg4.c | 213 +++ >
[PATCH] media: imx258: remove test pattern map from driver
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_PATTERN0x0600
-#define IMX258_TEST_PATTERN_DISABLE0
-#define IMX258_TEST_PATTERN_SOLID_COLOR1
-#define IMX258_TEST_PATTERN_COLOR_BARS 2
-#define IMX258_TEST_PATTERN_GREY_COLOR 3
-#define IMX258_TEST_PATTERN_PN94
/* Orientation */
#define REG_MIRROR_FLIP_CONTROL0x0101
@@ -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_634MHZ63360ULL
#define IMX258_LINK_FREQ_320MHZ32000ULL
@@ -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;
-
/*
* 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,
+};
+
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;
imx258->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
imx258->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
--
1.9.1
Re: [PATCH v3 3/4] v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
On 08/01/2018 11:50 PM, Ezequiel Garcia wrote:
> v4l2_m2m_job_finish() is typically called in interrupt context.
>
> Some implementation of .device_run might sleep, and so it's
> desirable to avoid calling it directly from
> v4l2_m2m_job_finish(), thus avoiding .device_run from running
> in interrupt context.
>
> Implement a deferred context that calls v4l2_m2m_try_run,
> and gets scheduled by v4l2_m2m_job_finish().
>
> Signed-off-by: Ezequiel Garcia
> ---
> drivers/media/v4l2-core/v4l2-mem2mem.c | 36 +++---
> 1 file changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
> b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index da82d151dd20..0bf4deefa899 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -69,6 +69,7 @@ static const char * const m2m_entity_name[] = {
> * @curr_ctx:currently running instance
> * @job_queue: instances queued to run
> * @job_spinlock:protects job_queue
> + * @job_work:worker to run queued jobs.
> * @m2m_ops: driver callbacks
> */
> struct v4l2_m2m_dev {
> @@ -85,6 +86,7 @@ struct v4l2_m2m_dev {
>
> struct list_headjob_queue;
> spinlock_t job_spinlock;
> + struct work_struct job_work;
>
> const struct v4l2_m2m_ops *m2m_ops;
> };
> @@ -224,10 +226,11 @@ EXPORT_SYMBOL(v4l2_m2m_get_curr_priv);
> /**
> * v4l2_m2m_try_run() - select next job to perform and run it if possible
> * @m2m_dev: per-device context
> + * @try_lock: indicates if the queue lock should be taken
I don't like this bool. See more below.
> *
> * Get next transaction (if present) from the waiting jobs list and run it.
> */
> -static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
> +static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev, bool try_lock)
> {
> unsigned long flags;
>
> @@ -250,7 +253,20 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev
> *m2m_dev)
> spin_unlock_irqrestore(_dev->job_spinlock, flags);
>
> dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
> +
> + /*
> + * A m2m context lock is taken only after a m2m context
> + * is picked from the queue and marked as running.
> + * The lock is only needed if v4l2_m2m_try_run is called
> + * from the async worker.
> + */
> + if (try_lock && m2m_dev->curr_ctx->q_lock)
> + mutex_lock(m2m_dev->curr_ctx->q_lock);
> +
> m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
> +
> + if (try_lock && m2m_dev->curr_ctx->q_lock)
> + mutex_unlock(m2m_dev->curr_ctx->q_lock);
> }
>
> /*
> @@ -340,10 +356,22 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
>
> __v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
> - v4l2_m2m_try_run(m2m_dev);
> + v4l2_m2m_try_run(m2m_dev, false);
I would like to see a WARN_ON where you verify that q_lock is actually locked
(and this needs to be documented as well).
> }
> EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
>
> +/**
> + * v4l2_m2m_device_run_work() - run pending jobs for the context
> + * @work: Work structure used for scheduling the execution of this function.
> + */
> +static void v4l2_m2m_device_run_work(struct work_struct *work)
> +{
> + struct v4l2_m2m_dev *m2m_dev =
> + container_of(work, struct v4l2_m2m_dev, job_work);
> +
> + v4l2_m2m_try_run(m2m_dev, true);
Just lock q_lock here around the try_run call. That's consistent with how
try_schedule works. No need to add an extra argument to try_run.
> +}
> +
> /**
> * v4l2_m2m_cancel_job() - cancel pending jobs for the context
> * @m2m_ctx: m2m context with jobs to be canceled
> @@ -403,7 +431,8 @@ void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
> /* This instance might have more buffers ready, but since we do not
>* allow more than one job on the job_queue per instance, each has
>* to be scheduled separately after the previous one finishes. */
> - v4l2_m2m_try_schedule(m2m_ctx);
> + __v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
> + schedule_work(_dev->job_work);
You might want to add a comment here explaining why you need 'work' here.
> }
> EXPORT_SYMBOL(v4l2_m2m_job_finish);
>
> @@ -837,6 +866,7 @@ struct v4l2_m2m_dev *v4l2_m2m_init(const struct
> v4l2_m2m_ops *m2m_ops)
> m2m_dev->m2m_ops = m2m_ops;
> INIT_LIST_HEAD(_dev->job_queue);
> spin_lock_init(_dev->job_spinlock);
> + INIT_WORK(_dev->job_work, v4l2_m2m_device_run_work);
>
> return m2m_dev;
> }
>
Regarding q_lock: I would like to see that made compulsory. So v4l2_mem2mem
should check
that both queue lock pointers point to the same mutex and return an error if
that's not
the case (I believe all m2m drivers use the same mutex already).
Also make sure that there are no drivers that
Re: [PATCH 16/22] [media] tvp5150: add querystd
Hi Mauro,
On 18-08-01 12:50, Mauro Carvalho Chehab wrote:
> Em Wed, 1 Aug 2018 16:49:26 +0200
> Marco Felsch escreveu:
>
> > Hi Mauro,
> >
> > On 18-08-01 11:22, Mauro Carvalho Chehab wrote:
> > > Em Wed, 1 Aug 2018 15:21:25 +0200
> > > Marco Felsch escreveu:
> > >
> > > > Hi Mauro,
> > > >
> > > > On 18-07-30 15:09, Mauro Carvalho Chehab wrote:
> > > > > Em Thu, 28 Jun 2018 18:20:48 +0200
> > > > > Marco Felsch escreveu:
> > > > >
> > > > > > From: Philipp Zabel
> > > > > >
> > > > > > Add the querystd video_op and make it return V4L2_STD_UNKNOWN while
> > > > > > the
> > > > > > TVP5150 is not locked to a signal.
> > > > > >
> > > > > > Signed-off-by: Philipp Zabel
> > > > > > Signed-off-by: Marco Felsch
> > > > > > ---
> > > > > > drivers/media/i2c/tvp5150.c | 10 ++
> > > > > > 1 file changed, 10 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/tvp5150.c
> > > > > > b/drivers/media/i2c/tvp5150.c
> > > > > > index 99d887936ea0..1990aaa17749 100644
> > > > > > --- a/drivers/media/i2c/tvp5150.c
> > > > > > +++ b/drivers/media/i2c/tvp5150.c
> > > > > > @@ -796,6 +796,15 @@ static v4l2_std_id tvp5150_read_std(struct
> > > > > > v4l2_subdev *sd)
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > +static int tvp5150_querystd(struct v4l2_subdev *sd, v4l2_std_id
> > > > > > *std_id)
> > > > > > +{
> > > > > > + struct tvp5150 *decoder = to_tvp5150(sd);
> > > > > > +
> > > > > > + *std_id = decoder->lock ? tvp5150_read_std(sd) :
> > > > > > V4L2_STD_UNKNOWN;
> > > > >
> > > > > This patch requires rework. What happens when a device doesn't have
> > > > > IRQ enabled? Perhaps it should, instead, read some register in order
> > > > > to check for the locking status, as this would work on both cases.
> > > >
> > > > If IRQ isn't enabled, decoder->lock is set to always true during
> > > > probe(). So this case should be fine.
> > >
> > > Not sure if tvp5150_read_std() will do the right thing. If it does,
> > > the above could simply be:
> > > std_id = tvp5150_read_std(sd);
> > >
> > > But, as there are 3 variants of this chipset, it sounds safer to check
> > > if the device is locked before calling tvp5150_read_std().
> >
> > Yes, I'm with you.
> >
> > >
> > > IMHO, the best would be to have a patch like the one below.
> > >
> > > Regards,
> > > Mauro
> > >
> > > [PATCH] media: tvp5150: implement decoder lock when irq is not used
> > >
> > > When irq is used, the lock is set via IRQ code. When it isn't,
> > > the driver just assumes it is always locked. Instead, read the
> > > lock status from the status register.
> >
> > Yes, that is a better solution.
> >
> > >
> > > Compile-tested only.
> > >
> > > Signed-off-by: Mauro Carvalho Chehab
> > >
> > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > > index 75e5ffc6573d..e07020d4053d 100644
> > > --- a/drivers/media/i2c/tvp5150.c
> > > +++ b/drivers/media/i2c/tvp5150.c
> > > @@ -811,11 +811,24 @@ static v4l2_std_id tvp5150_read_std(struct
> > > v4l2_subdev *sd)
> > > }
> > > }
> > >
> > > +static int query_lock(struct v4l2_subdev *sd)
> > > +{
> > > + struct tvp5150 *decoder = to_tvp5150(sd);
> > > + int status;
> > > +
> > > + if (decoder->irq)
> > > + return decoder->lock;
> > > +
> > > + regmap_read(map, TVP5150_INT_STATUS_REG_A, );
> > > +
> > > + return (status & 0x06) == 0x06;
> >
> > Typo? It should be 0x80, as described in the datasheet (SLES209E) or
> > just use the TVP5150_INT_A_LOCK_STATUS define. This avoid datasheet
> > cross check during reading.
>
> Yes, it is a typo, but at the other line... I meant to use the register
> 0x88, e. g.:
>
> regmap_read(decoder->regmap, TVP5150_STATUS_REG_1, );
During my development I tried this status register too, as descibed on
the community website [1]. But that wasn't that good, because the look
will be lost very often. Bit7 of Interrupt Status Reg A (0xc0) is more
robust for that kind of work, since it covers the whole signal.
[1] http://e2e.ti.com/support/data_converters/videoconverters/f/918/p/ \
617120/2273276?keyMatch=tvp5150%20lock%20lost=Search-EN-Support
>
> I ran some tests here: the int status reg is not updated.
>
> Also, after thinking a little bit, I opted to not use the query_lock()
> at s_stream. It makes no sense there without adding a status polling
> logic. I also opted to remove initializing decoder->lock to true, as
> this is very counter-intuitive. Instead, I'm adding a test at s_stream
> if decoder->irq is set. This makes easier to understand the code.
Yes, you're right.
> Btw, on my tests here, I noticed a problem with S-Video... at least with
> AV-350 grabber, composite is only working when S-Video is connected.
Unfortunately I have only a custom board with one composite connection.
>
> This bug also happens before your patchset, so this is not a regression
> caused by your patches.
>
> Anyway, patch enclosed. I added it
Re: [PATCH 3/3] media: add Rockchip VPU driver
On 07/27/2018 07:13 PM, Ezequiel Garcia wrote:
> Hi Hans,
>
> Thanks a lot for the review.
>
> On Wed, 2018-07-18 at 11:58 +0200, Hans Verkuil wrote:
>>>
>>> +
>>> +static int
>>> +queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
>>> +{
>>> + struct rockchip_vpu_ctx *ctx = priv;
>>> + int ret;
>>> +
>>> + src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
>>> + src_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
>>
>> Any reason for setting USERPTR?
>>
>>> + src_vq->drv_priv = ctx;
>>> + src_vq->ops = _vpu_enc_queue_ops;
>>> + src_vq->mem_ops = _dma_contig_memops;
>>
>> It isn't really useful in combination with dma_contig.
>>
>
> Right! I think I just missed it.
>
>>>
>>> +
>>> +fallback:
>>> + /* Default to full frame for incorrect settings. */
>>> + ctx->src_crop.width = fmt->width;
>>> + ctx->src_crop.height = fmt->height;
>>> + return 0;
>>> +}
>>
>> Replace crop by the selection API. The old crop API is no longer allowed
>> in new drivers.
>
> I have a question about the selection API. There is a comment that says
> MPLANE types shouldn't be used:
>
> /**
> * struct v4l2_selection - selection info
> * @type: buffer type (do not use *_MPLANE types)
>
> What is the meaning of that?
Easiest is to look in v4l2-ioctl.c. Search for g_selection. You'll see that
if the user passes in an _MPLANE buftype it is replaced by the regular
non-mplane
buftype. So drivers only see that type.
It used to be that applications also had to be specific about what buftype to
pass to G/S_SELECTION, but these days either type can be passed in.
>
> [..]
>>
>> I skipped the review of the colorspace handling. I'll see if I can come back
>> to that later today. I'm not sure if it is correct, but to be honest I doubt
>> that there is any JPEG encoder that does this right anyway.
>>
>
> And I'd say it's probably wrong, since we let the user change the colorspace,
> but we do not use that for anything.
So strictly speaking a JPEG encoder doesn't care about colorspace, xfer_func and
ycbcr_enc. It might care about quantization. It is my understanding that a JPEG
encoder expects full range Y'CbCr instead of limited range Y'CbCr. Does the HW
support limited range as well? I.e. can it convert from limited to full range
in hardware?
It might also be that it doesn't care so passing a limited range Y'CbCr image
will
create a limited range JPEG file and software will have to know that it contains
limited range data when decoding it.
In any case, colorspace, xfer_func and ycbcr_enc can just be passed from the
output to the capture side, just like other codecs. What to do with
'quantization'
depends on the hardware: if it can convert from limited to full range on the
fly,
then it should be handled by the driver. If not, then copy it like the other
fields.
>
>> BTW, please show the 'v4l2-compliance -s' output in the cover letter for the
>> next version.
>>
>
> OK, no problem.
>
> Thanks!
> Eze
>
Regards,
Hans
Re: [RFC 0/4] media: meson: add video decoder driver
Hi Maxime! Thanks for working on this, much appreciated. Some high-level comments below: On 08/01/2018 09:33 PM, Maxime Jourdan wrote: > This is a Request for Comments for the amlogic (meson) video decoder driver. > It is written around the V4L2 M2M framework without using the Request > API as there are a hardware bitstream parser and firmwares. > > It features decoding for: > - MPEG 1/2/4, H.263, H.264, MJPEG, HEVC 8-bit (partial) > > Even though they are supported in hardware, it doesn't leverage support for: > - HEVC 10-bit, VP9, VC1 (all those are in TODOs) > > The output is multiplanar NV12 (V4L2_PIX_FMT_NV12M). > Supported SoCs are: GXBB (S905), GXL (S905X/W/D), GXM (S912) > It was tested primarily with FFmpeg, GStreamer and kodi. > > The file hierarchy can be boiled down to: > > | codec_h264.c > | codec_mjpeg.c > | codec_mpeg4.c > | vdec_1.c -->| codec_mpeg12.c > vdec.c -->| vdec_hevc.c -->| codec_hevc.c > | esparser.c > > The V4L2 code is handled mostly in vdec.c. > Each VDEC and CODEC unit is accessed via ops structs to facilitate the code. > > The arrangement between vdecs and codecs can be seen in vdec_platform.c > This file also declares things like pixfmts, min/max buffers and firmware > paths > for each SoC. > > Specific questions about the code: > > - While I do use the platform's general clks and resets tied to the vdec in > a nice way (dts + clock/reset controller with clk/reset frameworks), > there are some subclocks and resets that I use in the driver by writing > directly to registers. e.g: > > - writel_relaxed((1<<7) | (1<<6), core->dos_base + DOS_SW_RESET0); > - writel_relaxed(0x3ff, core->dos_base + DOS_GCLK_EN0); > > and a few other instances where that happens. > > Is it okay to not create specific controllers for those ? The main issue is > the lack of documentation so I don't know which resets/clocks are impacted by > those writes. > The only thing I'm certain of is that they only apply to the vdec/esparser. > > - I tend to call vdec_* functions from the codec handlers. > > For instance, codec_h264 will call vdec_dst_buf_done_idx to DONE > a capture buffer. vdec_dst_buf_done_idx is as such a public symbol. > > Should I use an ops struct for those instead, so that the codec handlers > don't depend directly on the vdec general code ? > > - Naming: my public symbols either start with vdec_* or esparser_* > > Should I change that to something meson/amlogic specific ? Yes please. > > - I have a _lot_ of writel_relaxed calls. > > Can I leave them be or is there a nicer way to do it ? > > - Since the decoder is single instance, I only allow one _open at a time. > > However the v4l2 compliance suite complains about this. > How should I safely make it single instance ? Not allowing multiple > start_streaming ? The vb2 queue_setup operation is a good place for that: let it return EBUSY if there is a decoder active already. This op is called when allocating buffers and that's what locks things in place. Just be aware that it can be called multiple times if the user calls VIDIOC_CREATE_BUFS, so you need to keep track of the struct file that is currently 'owning' the decoder. > > - I am getting these 2 fails, but unsure what they are about: > > Buffer ioctls: > fail: > ../../../v4l-utils-1.12.3/utils/v4l2-compliance/v4l2-test-buffers.cpp(428): > node->node2 == NULL > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL This is since you only allow one open. > fail: > ../../../v4l-utils-1.12.3/utils/v4l2-compliance/v4l2-test-buffers.cpp(571): > q.has_expbuf(node) > test VIDIOC_EXPBUF: FAIL Not sure, might well be a knock-on result of the 'one open' problem. BTW, always get the latest code from the v4l-utils git repo, don't use a released version for v4l2-compliance: it's always evolving and you don't want to use an old version. Also for the next version of this patch series add the output of v4l2-compliance to this cover letter, I want to see it. Finally, are you aware of the work Tomasz Figa on specifying the codec behavior? https://lkml.org/lkml/2018/7/24/539 The final version will be close to what was posted there. Regards, Hans > > > > And of course, I will gladly accept any kind of other feedback you would have. > > Thanks! > > > Maxime Jourdan (4): > media: meson: add v4l2 m2m video decoder driver > ARM64: dts: meson-gx: add vdec entry > ARM64: dts: meson: add vdec entries > dt-bindings: media: add Amlogic Meson Video Decoder Bindings > > .../bindings/media/amlogic,meson-vdec.txt | 60 + > arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 14 + > arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi |8 + > arch/arm64/boot/dts/amlogic/meson-gxl.dtsi|8 + > arch/arm64/boot/dts/amlogic/meson-gxm.dtsi|4 + > drivers/media/platform/Kconfig| 10 + >
