cron job: media_tree daily build: WARNINGS
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: Sat Sep 15 04:00:49 CEST 2018 media-tree git hash:78cf8c842c111df656c63b5d04997ea4e40ef26a media_build git hash: 73a39eb63460f29cbe9bc056ae0b05ce9e813b11 v4l-utils git hash: 22a3113e373d0845fee555f1818c81d2fdc9fc20 edid-decode git hash: b2da1516df3cc2756bfe8d1fa06d7bf2562ba1f4 gcc version:i686-linux-gcc (GCC) 8.2.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.17.0-3-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: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK Check COMPILE_TEST: OK linux-3.0.101-i686: WARNINGS linux-3.0.101-x86_64: WARNINGS linux-3.1.10-i686: WARNINGS linux-3.1.10-x86_64: WARNINGS linux-3.2.102-i686: WARNINGS linux-3.2.102-x86_64: WARNINGS linux-3.3.8-i686: WARNINGS linux-3.3.8-x86_64: WARNINGS linux-3.4.113-i686: WARNINGS linux-3.4.113-x86_64: WARNINGS linux-3.5.7-i686: WARNINGS linux-3.5.7-x86_64: WARNINGS linux-3.6.11-i686: WARNINGS linux-3.6.11-x86_64: WARNINGS linux-3.7.10-i686: WARNINGS linux-3.7.10-x86_64: WARNINGS linux-3.8.13-i686: WARNINGS linux-3.8.13-x86_64: WARNINGS linux-3.9.11-i686: WARNINGS linux-3.9.11-x86_64: WARNINGS linux-3.10.108-i686: WARNINGS linux-3.10.108-x86_64: WARNINGS linux-3.11.10-i686: WARNINGS linux-3.11.10-x86_64: WARNINGS linux-3.12.74-i686: WARNINGS linux-3.12.74-x86_64: WARNINGS linux-3.13.11-i686: WARNINGS linux-3.13.11-x86_64: WARNINGS linux-3.14.79-i686: WARNINGS linux-3.14.79-x86_64: WARNINGS linux-3.15.10-i686: WARNINGS linux-3.15.10-x86_64: WARNINGS linux-3.16.57-i686: WARNINGS linux-3.16.57-x86_64: WARNINGS linux-3.17.8-i686: WARNINGS linux-3.17.8-x86_64: WARNINGS linux-3.18.119-i686: OK linux-3.18.119-x86_64: OK linux-3.19.8-i686: WARNINGS linux-3.19.8-x86_64: WARNINGS linux-4.0.9-i686: WARNINGS linux-4.0.9-x86_64: WARNINGS linux-4.1.52-i686: WARNINGS linux-4.1.52-x86_64: WARNINGS linux-4.2.8-i686: WARNINGS linux-4.2.8-x86_64: WARNINGS linux-4.3.6-i686: OK linux-4.3.6-x86_64: OK linux-4.4.152-i686: OK linux-4.4.152-x86_64: OK linux-4.5.7-i686: OK linux-4.5.7-x86_64: OK linux-4.6.7-i686: OK linux-4.6.7-x86_64: OK linux-4.7.10-i686: OK linux-4.7.10-x86_64: OK linux-4.8.17-i686: OK linux-4.8.17-x86_64: OK linux-4.9.124-i686: OK linux-4.9.124-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.67-i686: OK linux-4.14.67-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.19-i686: OK linux-4.17.19-x86_64: OK linux-4.18.5-i686: OK linux-4.18.5-x86_64: OK linux-4.19-rc1-i686: OK linux-4.19-rc1-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Logs weren't copied as they are too large (2112 kB) The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
Re: cx23885 - regression between 4.17.x and 4.18.x
On Thu, Sep 13, 2018 at 06:39:57PM +0100, David R wrote: > Hi > > Just a heads up. I'm having to revert cx23885-core.c to the 4.17 version > to obtain stability with my old AMD Phenom/ASUS M4A785TD and Hauppauge > WinTV-HVR-5525. The latest code drops out and refuses to return video > streams in hours or a few days max. The 4.17 version is fine and stable > over weeks/months. > > 04:00.0 Multimedia video controller: Conexant Systems, Inc. CX23887/8 > PCIe Broadcast Audio and Video Decoder with 3D Comb (rev 04) > Subsystem: Hauppauge computer works Inc. Device f038 > Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- > Stepping- SERR+ FastB2B- DisINTx- > Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- > SERR- Latency: 0, Cache Line Size: 64 bytes > Interrupt: pin A routed to IRQ 17 > Region 0: Memory at fe80 (64-bit, non-prefetchable) [size=2M] > Capabilities: > Kernel driver in use: cx23885 Interesting. cx88 also seems to have regressed. Are you able to give a git commit range for what works & doesn't work? I asked Adam to try building the modules with media-build but have not heard anything further. Date: Sat, 8 Sep 2018 20:49:22 -0400 From: Adam Stylinski To: linux-media@vger.kernel.org Subject: 4.18 regression Hello, I haven't done a thorough bisection of kernel revisions, but moving from kernel 4.17.19 to 4.18.6 results in mythtv being unable to tune in any channel with a pic hdtv 5500 tuner (cx88 based device with an LG frontend). I get an error back from the channel scanner about not being able to measure signal strength and getting back an error unknown (errno 254). I was able to use dvbtools with get-atsc to get a channel, but I don't think any of the forward error correction was applied to it. Let me know if you need more details. Vince
Re: [PATCH] media: imx-pxp: fix compilation on i386 or x86_64
On 9/14/18 12:10 AM, Philipp Zabel wrote:
> Include the missing interrupt.h header to fix compilation on i386 or
> x86_64:
>
> ../drivers/media/platform/imx-pxp.c:988:1: error: unknown type name
> 'irqreturn_t'
> static irqreturn_t pxp_irq_handler(int irq, void *dev_id)
> ^
> ../drivers/media/platform/imx-pxp.c: In function 'pxp_irq_handler':
> ../drivers/media/platform/imx-pxp.c:1012:9: error: 'IRQ_HANDLED' undeclared
> (first use in this function)
>return IRQ_HANDLED;
> ^
> ../drivers/media/platform/imx-pxp.c:1012:9: note: each undeclared identifier
> is reported only once for each function it appears in
> ../drivers/media/platform/imx-pxp.c: In function 'pxp_probe':
> ../drivers/media/platform/imx-pxp.c:1660:2: error: implicit declaration of
> function 'devm_request_threaded_irq' [-Werror=implicit-function-declaration]
>ret = devm_request_threaded_irq(>dev, irq, NULL, pxp_irq_handler,
>^
> ../drivers/media/platform/imx-pxp.c:1661:4: error: 'IRQF_ONESHOT' undeclared
> (first use in this function)
> IRQF_ONESHOT, dev_name(>dev), dev);
>
> Fixes: 51abcf7fdb70 ("media: imx-pxp: add i.MX Pixel Pipeline driver")
> Reported-by: Randy Dunlap
> Signed-off-by: Philipp Zabel
Thanks. You can choose/add:
Acked-by: Randy Dunlap
Build-tested-by: Randy Dunlap
> ---
> drivers/media/platform/imx-pxp.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/platform/imx-pxp.c
> b/drivers/media/platform/imx-pxp.c
> index 68ecfed7098b..229c23ae4029 100644
> --- a/drivers/media/platform/imx-pxp.c
> +++ b/drivers/media/platform/imx-pxp.c
> @@ -13,6 +13,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
>
--
~Randy
Re: [PATCH v2 0/4] media: soc_camera: ov9640: switch driver to v4l2_async
Dne 14.9.2018 v 14:59 Sakari Ailus napsal(a): > Hi Petr, > > On Wed, Aug 15, 2018 at 03:30:23PM +0200, petrcve...@gmail.com wrote: >> From: Petr Cvek >> >> This patch series transfer the ov9640 driver from the soc_camera subsystem >> into a standalone v4l2 driver. There is no changes except the required >> v4l2_async calls, GPIO allocation, deletion of now unused variables, >> a change from mdelay() to msleep() and an addition of SPDX identifiers >> (as suggested in the v1 version RFC). >> >> The config symbol has been changed from CONFIG_SOC_CAMERA_OV9640 to >> CONFIG_VIDEO_OV9640. >> >> Also as the drivers of the soc_camera seems to be orphaned I'm volunteering >> as a maintainer of the driver (I own the hardware). > > Thanks for the set. The patches seem good to me as such but there's some > more work to do there. For one, the depedency to v4l2_clk should be > removed; the common clock framework has supported clocks from random > devices for many, many years now. > > The PXA camera driver does still depend on v4l2_clk so I guess this is > better to do later on in a different patchset. > Yeah I too would like to remove the v4l2_clk from both of them. We've had the discussion about clock dependency around v1 patch set: "[BUG, RFC] media: Wrong module gets acquired" cheers, Petr
Re: [PATCH v2 1/4] media: soc_camera: ov9640: move ov9640 out of soc_camera
Dne 14.9.2018 v 14:59 Sakari Ailus napsal(a):
> Hi Petr,
>
> Thanks for the patchset, and my apologies for reviewing it so late!
>
> I'm commenting this one but feel free to add patches to address the
> comments.
>
Hi and thanks for the review. I would like to have this patch set to be
as much as possible only a conversion from soc-camera, but I guess I can
fix the error handling in probe and the missing newlines. For the
enhanced functionality, I would like to have a new patch set after I'll
patch the controller (pxa camera) on my testing platform.
>> +/* Start/Stop streaming from the device */
>> +static int ov9640_s_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> +return 0;
>
> Doesn't the sensor provide any control over the streaming state? Just
> wondering...
>
Before the PXA camera switch from soc-camera I've found some
deficiencies in register settings so I'm planning to test them all. With
the current state of vanilla I wouldn't be able to test the change.
After the quick search in datasheet I wasn't able to find any (stream,
capture, start) flag. It may be controlled by just setting the output
format flags, but these registers are some of those I will be changing
in the future.
>> +static int ov9640_s_power(struct v4l2_subdev *sd, int on)
>> +{
>> +struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>> +struct ov9640_priv *priv = to_ov9640_sensor(sd);
>> +
>> +return soc_camera_set_power(>dev, ssdd, priv->clk, on);
>
> Runtime PM support would be nice --- but not needed in this set IMO.
>
If I remember correctly a suspend to mem will freeze the whole machine,
so in the future :-/.
>> +}
>> +
>> +/* select nearest higher resolution for capture */
>> +static void ov9640_res_roundup(u32 *width, u32 *height)
>> +{
>> +int i;
>
> unsigned int
>
> Same for other loops where no negative values or test below zero are
> needed (or where the value which is compared against is signed).
>
Just re-declaring: unsigned int i; ... OK
>> +
>> +cfg->try_fmt = *mf;
>
> Newline here?
>
>> +return 0;
>> +}
>> +
>> +static int ov9640_enum_mbus_code(struct v4l2_subdev *sd,
>> +struct v4l2_subdev_pad_config *cfg,
>> +struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> +if (code->pad || code->index >= ARRAY_SIZE(ov9640_codes))
>> +return -EINVAL;
>> +
>> +code->code = ov9640_codes[code->index];
>
> And here.
>
np
>> +/* Request bus settings on camera side */
>> +static int ov9640_g_mbus_config(struct v4l2_subdev *sd,
>> +struct v4l2_mbus_config *cfg)
>> +{
>> +struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>> +
>> +cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
>> +V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_HIGH |
>> +V4L2_MBUS_DATA_ACTIVE_HIGH;
>
> This should come from DT instead. Could you add DT binding documentation
> for the sensor, please? There's already some for ov9650; I wonder how
> similar that one is.
The platform doesn't support it yet, so I have no way to test any DT
patches.
>> +cfg->type = V4L2_MBUS_PARALLEL;
>> +cfg->flags = soc_camera_apply_board_flags(ssdd, cfg);
>> +
>> +return 0;
>> +}
>> +
>> +static const struct v4l2_subdev_video_ops ov9640_video_ops = {
>> +.s_stream = ov9640_s_stream,
>> +.g_mbus_config = ov9640_g_mbus_config,
>> +};
>> +
>> +static const struct v4l2_subdev_pad_ops ov9640_pad_ops = {
>> +.enum_mbus_code = ov9640_enum_mbus_code,
>> +.get_selection = ov9640_get_selection,
>> +.set_fmt= ov9640_set_fmt,
>
> Please add an operating to get the format as well.
OK, but it will be tested on a preliminary hacked pxa-camera :-).
>
>> +};
>> +
>> +static const struct v4l2_subdev_ops ov9640_subdev_ops = {
>> +.core = _core_ops,
>> +.video = _video_ops,
>> +.pad= _pad_ops,
>> +};
>> +
>> +/*
>> + * i2c_driver function
>> + */
>> +static int ov9640_probe(struct i2c_client *client,
>> +const struct i2c_device_id *did)
>> +{
>> +struct ov9640_priv *priv;
>> +struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>> +int ret;
>> +
>> +if (!ssdd) {
>> +dev_err(>dev, "Missing platform_data for driver\n");
>> +return -EINVAL;
>> +}
>> +
>> +priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
>> +if (!priv)
>> +return -ENOMEM;
>> +
>> +v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
>> +
>> +v4l2_ctrl_handler_init(>hdl, 2);
>> +v4l2_ctrl_new_std(>hdl, _ctrl_ops,
>> +V4L2_CID_VFLIP, 0, 1, 1, 0);
>> +v4l2_ctrl_new_std(>hdl, _ctrl_ops,
>> +V4L2_CID_HFLIP, 0, 1, 1, 0);
>> +priv->subdev.ctrl_handler = >hdl;
>> +if
Re: [PATCH 0/4] em28xx: solve several issues pointed by v4l2-compliance
Em Fri, 14 Sep 2018 15:22:30 -0300 Mauro Carvalho Chehab escreveu: > There are several non-compliance issues on em28xx. Fix those that > I can hit with a simple grabber board like Terratec AV 350. > > I also tested it with a WinTV USB2. There, I got several other compliants > all related to msp3400 driver and step size. Fixing those is more complex, > as it would require some non-trivial changes. So, for now, let's do just > the ones that aren't related to msp3400. > > Mauro Carvalho Chehab (4): > media: em28xx: fix handler for vidioc_s_input() > media: em28xx: use a default format if TRY_FMT fails > media: em28xx: fix input name for Terratec AV 350 > media: em28xx: make v4l2-compliance happier by starting sequence on > zero > > drivers/media/usb/em28xx/em28xx-cards.c | 33 - > drivers/media/usb/em28xx/em28xx-video.c | 91 ++--- > drivers/media/usb/em28xx/em28xx.h | 8 ++- > 3 files changed, 118 insertions(+), 14 deletions(-) > Btw, if one wants to see the results for v4l2-compliance, I applied those together with my tvp5150-4 test git branch: https://git.linuxtv.org/mchehab/experimental.git/log/?h=tvp5150-4 Results enclosed. Thanks, Mauro $ v4l2-compliance -s v4l2-compliance SHA: bc71e4a67c6fbc5940062843bc41e7c8679634ce, 64 bits Compliance test for device /dev/video0: Driver Info: Driver name : em28xx Card type: Terratec AV350 Bus info : usb-:00:14.0-2 Driver version : 4.19.0 Capabilities : 0x85220011 Video Capture VBI Capture Audio Read/Write Streaming Extended Pix Format Device Capabilities Device Caps : 0x05220001 Video Capture Audio Read/Write 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 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 test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK Inputs: 2 Audio Inputs: 1 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 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 (Input 0): 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: 13 Private Controls: 0 Format ioctls (Input 0): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK 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 (Input 0): 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 (Input 0): test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test VIDIOC_EXPBUF: OK (Not Supported) Control ioctls (Input 1): 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: 13 Private Controls: 0 Format ioctls (Input 1): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK 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
Re: [PATCH v2 7/7] [media] tvp5150: add s_power callback
Em Fri, 14 Sep 2018 20:20:46 +0200
Marco Felsch escreveu:
> Hi Sakari,
>
> On 18-09-14 16:23, Sakari Ailus wrote:
> > Hi Marco,
> >
> > On Mon, Aug 13, 2018 at 11:25:08AM +0200, Marco Felsch wrote:
> > > Don't en-/disable the interrupts during s_stream because someone can
> > > disable the stream but wants to get informed if the stream is locked
> > > again. So keep the interrupts enabled the whole time the pipeline is
> > > opened.
> > >
> > > Signed-off-by: Marco Felsch
> > > ---
> > > drivers/media/i2c/tvp5150.c | 23 +--
> > > 1 file changed, 17 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > > index e736f609fecd..e296f5bfae21 100644
> > > --- a/drivers/media/i2c/tvp5150.c
> > > +++ b/drivers/media/i2c/tvp5150.c
> > > @@ -1389,11 +1389,26 @@ static const struct media_entity_operations
> > > tvp5150_sd_media_ops = {
> > >
> > > /
> > > I2C Command
> > >
> > > /
> > > +static int tvp5150_s_power(struct v4l2_subdev *sd, int on)
> > > +{
> > > + struct tvp5150 *decoder = to_tvp5150(sd);
> > > + unsigned int val = 0;
> > > +
> > > + if (on)
> > > + val = TVP5150_INT_A_LOCK;
> > > +
> > > + if (decoder->irq)
> > > + /* Enable / Disable lock interrupt */
> > > + regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
> > > +TVP5150_INT_A_LOCK, val);
> >
> > Could you use runtime PM instead?
>
> I will test it next monday. What's the different between s_power and
> runtime PM?
>
> >
> > For an example, the dw9714 driver does this: drivers/media/i2c/dw9714.c .
>
> Hopefully I got you right, should I use the
> v4l2_subdev_internal_ops.open/close and call the pm_runtime_put/get
> there or did you mean the driver.pm callbacks? I'm not that familiar
> with the pm ops at the moment, sorry.
I guess the main issue here is: will this work if the bridge
driver is em28xx?
Whatever change we do, tvp5150 should still fully work with em28xx,
as several devices use this demod there.
Changing em28xx to cope with runtime PM would be *very* complex,
as there are lots of other drivers that can work with it, and
touching those will affect lots of other drivers. At the end, it
will very likely affect all PCI/PCIe V4L2 drivers, and several
USB ones.
If it can be done without affecting PM with em28xx, let's do it.
Otherwise, let's stick with s_power on this series, and let
the mass PM rework on non-platform drivers to happen on some
separate patchset.
Thanks,
Mauro
Re: [PATCH v3 0/5] Fix OV5640 exposure & gain
Interesting, I just hit this problem yesterday. Same module as Steve, with MIPI CSI-2 OV5640 (on Sabre Lite). Tested-By: Nicolas Dufresne Le mardi 11 septembre 2018 à 15:48 +0200, Hugues Fruchet a écrit : > This patch serie fixes some problems around exposure & gain in OV5640 > driver. > > The 4th patch about autocontrols requires also a fix in v4l2-ctrls.c: > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133164.html > > Here is the test procedure used for exposure & gain controls check: > 1) Preview in background > $> gst-launch-1.0 v4l2src ! "video/x-raw, width=640, Height=480" ! > queue ! waylandsink -e & > 2) Check gain & exposure values > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 > default=0 value=330 flags=inactive, volatile >gain (int): min=0 max=1023 step=1 > default=0 value=19 flags=inactive, volatile > 3) Put finger in front of camera and check that gain/exposure values > are changing: > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 > default=0 value=660 flags=inactive, volatile >gain (int): min=0 max=1023 step=1 > default=0 value=37 flags=inactive, volatile > 4) switch to manual mode, image exposition must not change > $> v4l2-ctl --set-ctrl=gain_automatic=0 > $> v4l2-ctl --set-ctrl=auto_exposure=1 > Note the "1" for manual exposure. > > 5) Check current gain/exposure values: > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 > default=0 value=330 >gain (int): min=0 max=1023 step=1 > default=0 value=20 > > 6) Put finger behind camera and check that gain/exposure values are > NOT changing: > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 > default=0 value=330 >gain (int): min=0 max=1023 step=1 > default=0 value=20 > 7) Update exposure, check that it is well changed on display and that > same value is returned: > $> v4l2-ctl --set-ctrl=exposure=100 > $> v4l2-ctl --get-ctrl=exposure > exposure: 100 > > 9) Update gain, check that it is well changed on display and that > same value is returned: > $> v4l2-ctl --set-ctrl=gain=10 > $> v4l2-ctl --get-ctrl=gain > gain: 10 > > 10) Switch back to auto gain/exposure, verify that image is correct > and values returned are correct: > $> v4l2-ctl --set-ctrl=gain_automatic=1 > $> v4l2-ctl --set-ctrl=auto_exposure=0 > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 > default=0 value=330 flags=inactive, volatile >gain (int): min=0 max=1023 step=1 > default=0 value=22 flags=inactive, volatile > Note the "0" for auto exposure. > > === > = history = > === > version 3: > - Change patch 5/5 by removing set_mode() orig_mode parameter as > per jacopo' suggestion: > https://www.spinics.net/lists/linux-media/msg139457.html > > version 2: > - Fix patch 3/5 commit comment and rename binning function as per > jacopo' suggestion: > > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133272.html > > Hugues Fruchet (5): > media: ov5640: fix exposure regression > media: ov5640: fix auto gain & exposure when changing mode > media: ov5640: fix wrong binning value in exposure calculation > media: ov5640: fix auto controls values when switching to manual > mode > media: ov5640: fix restore of last mode set > > drivers/media/i2c/ov5640.c | 128 ++- > -- > 1 file changed, 73 insertions(+), 55 deletions(-) > signature.asc Description: This is a digitally signed message part
[PATCH 2/4] media: em28xx: use a default format if TRY_FMT fails
Follow the V4L2 spec, as warned by v4l2-compliance:
warn: v4l2-test-formats.cpp(732): TRY_FMT cannot handle an invalid
pixelformat.
warn: v4l2-test-formats.cpp(733): This may or may not be a problem. For
more information see:
warn: v4l2-test-formats.cpp(734):
http://www.mail-archive.com/linux-media@vger.kernel.org/msg56550.html
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/usb/em28xx/em28xx-video.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-video.c
b/drivers/media/usb/em28xx/em28xx-video.c
index fcb5c1b25ab3..41ac47f1589c 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -1471,9 +1471,9 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void
*priv,
fmt = format_by_fourcc(f->fmt.pix.pixelformat);
if (!fmt) {
- em28xx_videodbg("Fourcc format (%08x) invalid.\n",
- f->fmt.pix.pixelformat);
- return -EINVAL;
+ fmt = [0];
+ em28xx_videodbg("Fourcc format (%08x) invalid. Using default
(%08x).\n",
+ f->fmt.pix.pixelformat, fmt->fourcc);
}
if (dev->board.is_em2800) {
--
2.17.1
[PATCH 0/4] em28xx: solve several issues pointed by v4l2-compliance
There are several non-compliance issues on em28xx. Fix those that I can hit with a simple grabber board like Terratec AV 350. I also tested it with a WinTV USB2. There, I got several other compliants all related to msp3400 driver and step size. Fixing those is more complex, as it would require some non-trivial changes. So, for now, let's do just the ones that aren't related to msp3400. Mauro Carvalho Chehab (4): media: em28xx: fix handler for vidioc_s_input() media: em28xx: use a default format if TRY_FMT fails media: em28xx: fix input name for Terratec AV 350 media: em28xx: make v4l2-compliance happier by starting sequence on zero drivers/media/usb/em28xx/em28xx-cards.c | 33 - drivers/media/usb/em28xx/em28xx-video.c | 91 ++--- drivers/media/usb/em28xx/em28xx.h | 8 ++- 3 files changed, 118 insertions(+), 14 deletions(-) -- 2.17.1
[PATCH 4/4] media: em28xx: make v4l2-compliance happier by starting sequence on zero
The v4l2-compliance tool complains if a video doesn't start
with a zero sequence number.
While this shouldn't cause any real problem for apps, let's
make it happier, in order to better check the v4l2-compliance
differences before and after patchsets.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/usb/em28xx/em28xx-video.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/media/usb/em28xx/em28xx-video.c
b/drivers/media/usb/em28xx/em28xx-video.c
index 41ac47f1589c..26859aaf5c93 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -1093,6 +1093,8 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq,
unsigned int count)
em28xx_videodbg("%s\n", __func__);
+ dev->v4l2->field_count = 0;
+
/*
* Make sure streaming is not already in progress for this type
* of filehandle (e.g. video, vbi)
--
2.17.1
[PATCH 3/4] media: em28xx: fix input name for Terratec AV 350
Instead of using a register value, use an AMUX name, as otherwise
VIDIOC_G_AUDIO would fail.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/usb/em28xx/em28xx-cards.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c
b/drivers/media/usb/em28xx/em28xx-cards.c
index e9ab1fbc8f0d..2a3f3e237d05 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -2141,13 +2141,13 @@ const struct em28xx_board em28xx_boards[] = {
.input = { {
.type = EM28XX_VMUX_COMPOSITE,
.vmux = TVP5150_COMPOSITE1,
- .amux = EM28XX_AUDIO_SRC_LINE,
+ .amux = EM28XX_AMUX_LINE_IN,
.gpio = terratec_av350_unmute_gpio,
}, {
.type = EM28XX_VMUX_SVIDEO,
.vmux = TVP5150_SVIDEO,
- .amux = EM28XX_AUDIO_SRC_LINE,
+ .amux = EM28XX_AMUX_LINE_IN,
.gpio = terratec_av350_unmute_gpio,
} },
},
--
2.17.1
[PATCH 1/4] media: em28xx: fix handler for vidioc_s_input()
The a->index is not the name of the internal amux entry,
but, instead a value from zero to the maximum number
of audio inputs.
As the actual available inputs depend on each board, build
it dynamically.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/usb/em28xx/em28xx-cards.c | 29 +
drivers/media/usb/em28xx/em28xx-video.c | 83 ++---
drivers/media/usb/em28xx/em28xx.h | 8 ++-
3 files changed, 111 insertions(+), 9 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c
b/drivers/media/usb/em28xx/em28xx-cards.c
index 71c829f31d3b..e9ab1fbc8f0d 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3039,6 +3039,9 @@ static int em28xx_hint_board(struct em28xx *dev)
static void em28xx_card_setup(struct em28xx *dev)
{
+ int i, j, idx;
+ bool duplicate_entry;
+
/*
* If the device can be a webcam, seek for a sensor.
* If sensor is not found, then it isn't a webcam.
@@ -3195,6 +3198,32 @@ static void em28xx_card_setup(struct em28xx *dev)
/* Allow override tuner type by a module parameter */
if (tuner >= 0)
dev->tuner_type = tuner;
+
+ /*
+* Dynamically generate a list of valid audio inputs for this
+* specific board, mapping them via enum em28xx_amux.
+*/
+
+ idx = 0;
+ for (i = 0; i < MAX_EM28XX_INPUT; i++) {
+ if (!INPUT(i)->type)
+ continue;
+
+ /* Skip already mapped audio inputs */
+ duplicate_entry = false;
+ for (j = 0; j < idx; j++) {
+ if (INPUT(i)->amux == dev->amux_map[j]) {
+ duplicate_entry = true;
+ break;
+ }
+ }
+ if (duplicate_entry)
+ continue;
+
+ dev->amux_map[idx++] = INPUT(i)->amux;
+ }
+ for (;idx < MAX_EM28XX_INPUT; idx++)
+ dev->amux_map[idx] = EM28XX_AMUX_UNUSED;
}
void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl)
diff --git a/drivers/media/usb/em28xx/em28xx-video.c
b/drivers/media/usb/em28xx/em28xx-video.c
index 917602954bfb..fcb5c1b25ab3 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -1666,6 +1666,7 @@ static int vidioc_enum_input(struct file *file, void
*priv,
{
struct em28xx *dev = video_drvdata(file);
unsigned int n;
+ int j;
n = i->index;
if (n >= MAX_EM28XX_INPUT)
@@ -1685,6 +1686,12 @@ static int vidioc_enum_input(struct file *file, void
*priv,
if (dev->is_webcam)
i->capabilities = 0;
+ /* Dynamically generates an audioset bitmask */
+ i->audioset = 0;
+ for (j = 0; j < MAX_EM28XX_INPUT; j++)
+ if (dev->amux_map[j] != EM28XX_AMUX_UNUSED)
+ i->audioset |= 1 << j;
+
return 0;
}
@@ -1710,11 +1717,25 @@ static int vidioc_s_input(struct file *file, void
*priv, unsigned int i)
return 0;
}
-static int vidioc_g_audio(struct file *file, void *priv, struct v4l2_audio *a)
+
+static int em28xx_fill_audio_input(struct em28xx *dev,
+ const char *s,
+ struct v4l2_audio *a,
+ unsigned index)
{
- struct em28xx *dev = video_drvdata(file);
+ unsigned int idx = dev->amux_map[index];
- switch (a->index) {
+ /*
+* With msp3400, almost all mappings use the default (amux = 0).
+* The only one may use a different value is WinTV USB2, where it
+* can also be SCART1 input.
+* As it is very doubtful that we would see new boards with msp3400,
+* let's just reuse the existing switch.
+*/
+ if (dev->has_msp34xx && idx != EM28XX_AMUX_UNUSED)
+ idx = EM28XX_AMUX_LINE_IN;
+
+ switch (idx) {
case EM28XX_AMUX_VIDEO:
strscpy(a->name, "Television", sizeof(a->name));
break;
@@ -1739,32 +1760,77 @@ static int vidioc_g_audio(struct file *file, void
*priv, struct v4l2_audio *a)
case EM28XX_AMUX_PCM_OUT:
strscpy(a->name, "PCM", sizeof(a->name));
break;
+ case EM28XX_AMUX_UNUSED:
default:
return -EINVAL;
}
-
- a->index = dev->ctl_ainput;
+ a->index = index;
a->capability = V4L2_AUDCAP_STEREO;
+ em28xx_videodbg("%s: audio input index %d is '%s'\n", s, a->index,
a->name);
+
return 0;
}
+static int vidioc_enumaudio(struct file *file, void *fh, struct v4l2_audio *a)
+{
+ struct em28xx *dev = video_drvdata(file);
+
+ if (a->index >= MAX_EM28XX_INPUT)
+ return -EINVAL;
+
+ return em28xx_fill_audio_input(dev, __func__,a, a->index);
+}
Re: [PATCH v2 7/7] [media] tvp5150: add s_power callback
Hi Sakari,
On 18-09-14 16:23, Sakari Ailus wrote:
> Hi Marco,
>
> On Mon, Aug 13, 2018 at 11:25:08AM +0200, Marco Felsch wrote:
> > Don't en-/disable the interrupts during s_stream because someone can
> > disable the stream but wants to get informed if the stream is locked
> > again. So keep the interrupts enabled the whole time the pipeline is
> > opened.
> >
> > Signed-off-by: Marco Felsch
> > ---
> > drivers/media/i2c/tvp5150.c | 23 +--
> > 1 file changed, 17 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > index e736f609fecd..e296f5bfae21 100644
> > --- a/drivers/media/i2c/tvp5150.c
> > +++ b/drivers/media/i2c/tvp5150.c
> > @@ -1389,11 +1389,26 @@ static const struct media_entity_operations
> > tvp5150_sd_media_ops = {
> >
> > /
> > I2C Command
> >
> > /
> > +static int tvp5150_s_power(struct v4l2_subdev *sd, int on)
> > +{
> > + struct tvp5150 *decoder = to_tvp5150(sd);
> > + unsigned int val = 0;
> > +
> > + if (on)
> > + val = TVP5150_INT_A_LOCK;
> > +
> > + if (decoder->irq)
> > + /* Enable / Disable lock interrupt */
> > + regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
> > + TVP5150_INT_A_LOCK, val);
>
> Could you use runtime PM instead?
I will test it next monday. What's the different between s_power and
runtime PM?
>
> For an example, the dw9714 driver does this: drivers/media/i2c/dw9714.c .
Hopefully I got you right, should I use the
v4l2_subdev_internal_ops.open/close and call the pm_runtime_put/get
there or did you mean the driver.pm callbacks? I'm not that familiar
with the pm ops at the moment, sorry.
Regards,
Marco
> > +
> > + return 0;
> > +}
> >
> > static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
> > {
> > struct tvp5150 *decoder = to_tvp5150(sd);
> > - unsigned int mask, val = 0, int_val = 0;
> > + unsigned int mask, val = 0;
> >
> > mask = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE |
> >TVP5150_MISC_CTL_CLOCK_OE;
> > @@ -1406,15 +1421,10 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd,
> > int enable)
> > val = decoder->lock ? decoder->oe : 0;
> > else
> > val = decoder->oe;
> > - int_val = TVP5150_INT_A_LOCK;
> > v4l2_subdev_notify_event(>sd, _ev_fmt);
> > }
> >
> > regmap_update_bits(decoder->regmap, TVP5150_MISC_CTL, mask, val);
> > - if (decoder->irq)
> > - /* Enable / Disable lock interrupt */
> > - regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
> > - TVP5150_INT_A_LOCK, int_val);
> >
> > return 0;
> > }
> > @@ -1616,6 +1626,7 @@ static const struct v4l2_subdev_core_ops
> > tvp5150_core_ops = {
> > .g_register = tvp5150_g_register,
> > .s_register = tvp5150_s_register,
> > #endif
> > + .s_power = tvp5150_s_power,
> > };
> >
> > static const struct v4l2_subdev_tuner_ops tvp5150_tuner_ops = {
>
> --
> Kind regards,
>
> Sakari Ailus
> sakari.ai...@linux.intel.com
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2 4/7] [media] v4l2-subdev: fix v4l2_subdev_get_try_* dependency
Hi Sakari,
On 18-09-14 16:25, Sakari Ailus wrote:
> Hi Marco,
>
> On Mon, Aug 13, 2018 at 11:25:05AM +0200, Marco Felsch wrote:
> > These helpers make us of the media-controller entity which is only
> > available if the CONFIG_MEDIA_CONTROLLER is enabled.
> >
> > Signed-off-by: Marco Felsch
> > ---
> > include/media/v4l2-subdev.h | 100 ++--
> > 1 file changed, 50 insertions(+), 50 deletions(-)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index ce48f1fcf295..79c066934ad2 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -912,6 +912,56 @@ struct v4l2_subdev_fh {
> > #define to_v4l2_subdev_fh(fh) \
> > container_of(fh, struct v4l2_subdev_fh, vfh)
> >
> > +extern const struct v4l2_file_operations v4l2_subdev_fops;
> > +
> > +/**
> > + * v4l2_set_subdevdata - Sets V4L2 dev private device data
> > + *
> > + * @sd: pointer to v4l2_subdev
> > + * @p: pointer to the private device data to be stored.
> > + */
> > +static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
> > +{
> > + sd->dev_priv = p;
> > +}
> > +
> > +/**
> > + * v4l2_get_subdevdata - Gets V4L2 dev private device data
> > + *
> > + * @sd: pointer to v4l2_subdev
> > + *
> > + * Returns the pointer to the private device data to be stored.
> > + */
> > +static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
> > +{
> > + return sd->dev_priv;
> > +}
> > +
> > +/**
> > + * v4l2_set_subdev_hostdata - Sets V4L2 dev private host data
> > + *
> > + * @sd: pointer to v4l2_subdev
> > + * @p: pointer to the private data to be stored.
> > + */
> > +static inline void v4l2_set_subdev_hostdata(struct v4l2_subdev *sd, void
> > *p)
> > +{
> > + sd->host_priv = p;
> > +}
> > +
> > +/**
> > + * v4l2_get_subdev_hostdata - Gets V4L2 dev private data
> > + *
> > + * @sd: pointer to v4l2_subdev
> > + *
> > + * Returns the pointer to the private host data to be stored.
> > + */
> > +static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
> > +{
> > + return sd->host_priv;
> > +}
>
> Could you leave the functions dealing with host_priv where they are? I'd
> like to avoid expanding their use; rather reduce it. The field is
> problematic in some cases and generally not really needed either.
Sure, I just moved the v4l2_subdev_get_try_* formats to the
CONFIG_MEDIA_CONTROLLER ifdef block to avoid a second ifdef block (see
below). Git made it that way.. and I'm with you, the patch looks not good.
Should I open a 2nd ifdef CONFIG_MEDIA_CONTROLLER block instead?
Regards,
Marco
>
> > +
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +
> > /**
> > * v4l2_subdev_get_try_format - ancillary routine to call
> > * v4l2_subdev_pad_config->try_fmt
> > @@ -978,56 +1028,6 @@ static inline struct v4l2_rect
> > #endif
> > }
> >
> > -extern const struct v4l2_file_operations v4l2_subdev_fops;
> > -
> > -/**
> > - * v4l2_set_subdevdata - Sets V4L2 dev private device data
> > - *
> > - * @sd: pointer to v4l2_subdev
> > - * @p: pointer to the private device data to be stored.
> > - */
> > -static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
> > -{
> > - sd->dev_priv = p;
> > -}
> > -
> > -/**
> > - * v4l2_get_subdevdata - Gets V4L2 dev private device data
> > - *
> > - * @sd: pointer to v4l2_subdev
> > - *
> > - * Returns the pointer to the private device data to be stored.
> > - */
> > -static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
> > -{
> > - return sd->dev_priv;
> > -}
> > -
> > -/**
> > - * v4l2_set_subdev_hostdata - Sets V4L2 dev private host data
> > - *
> > - * @sd: pointer to v4l2_subdev
> > - * @p: pointer to the private data to be stored.
> > - */
> > -static inline void v4l2_set_subdev_hostdata(struct v4l2_subdev *sd, void
> > *p)
> > -{
> > - sd->host_priv = p;
> > -}
> > -
> > -/**
> > - * v4l2_get_subdev_hostdata - Gets V4L2 dev private data
> > - *
> > - * @sd: pointer to v4l2_subdev
> > - *
> > - * Returns the pointer to the private host data to be stored.
> > - */
> > -static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
> > -{
> > - return sd->host_priv;
> > -}
> > -
> > -#ifdef CONFIG_MEDIA_CONTROLLER
> > -
> > /**
> > * v4l2_subdev_link_validate_default - validates a media link
> > *
>
> --
> Kind regards,
>
> Sakari Ailus
> sakari.ai...@linux.intel.com
>
Re: [PATCH v2 1/7] [media] tvp5150: add input source selection of_graph support
Hi Sakari,
thanks for the review.
On 18-09-14 16:31, Sakari Ailus wrote:
> Hi Marco,
>
> On Mon, Aug 13, 2018 at 11:25:02AM +0200, Marco Felsch wrote:
> ...
> > +static void tvp5150_dt_cleanup(struct tvp5150 *decoder)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < TVP5150_NUM_PADS; i++)
> > + of_node_put(decoder->endpoints[i]);
> > +}
> > +
> > static const char * const tvp5150_test_patterns[2] = {
> > "Disabled",
> > "Black screen"
> > @@ -1586,7 +1996,7 @@ static int tvp5150_probe(struct i2c_client *c,
> > res = tvp5150_parse_dt(core, np);
> > if (res) {
> > dev_err(sd->dev, "DT parsing error: %d\n", res);
> > - return res;
> > + goto err_cleanup_dt;
> > }
> > } else {
> > /* Default to BT.656 embedded sync */
> > @@ -1594,25 +2004,16 @@ static int tvp5150_probe(struct i2c_client *c,
> > }
> >
> > v4l2_i2c_subdev_init(sd, c, _ops);
> > + sd->internal_ops = _internal_ops;
> > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >
> > -#if defined(CONFIG_MEDIA_CONTROLLER)
> > - core->pads[TVP5150_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
> > - core->pads[TVP5150_PAD_IF_INPUT].sig_type = PAD_SIGNAL_ANALOG;
> > - core->pads[TVP5150_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
> > - core->pads[TVP5150_PAD_VID_OUT].sig_type = PAD_SIGNAL_DV;
> > -
> > - sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
> > -
> > - res = media_entity_pads_init(>entity, TVP5150_NUM_PADS, core->pads);
> > - if (res < 0)
> > - return res;
> > -
> > -#endif
> > + res = tvp5150_mc_init(sd);
> > + if (res)
> > + goto err_cleanup_dt;
> >
> > res = tvp5150_detect_version(core);
> > if (res < 0)
> > - return res;
> > + goto err_cleanup_dt;
> >
> > core->norm = V4L2_STD_ALL; /* Default is autodetect */
> > core->detected_norm = V4L2_STD_UNKNOWN;
> > @@ -1664,6 +2065,9 @@ static int tvp5150_probe(struct i2c_client *c,
> > err:
>
> Now that you have more error labels, you could rename this one.
Hm.. okay make sense, I will change this.
>
> > v4l2_ctrl_handler_free(>hdl);
> > return res;
>
> Is the above line intended to be kept?
Nope, sorry I will fix this.
Regards,
Marco
>
> > +err_cleanup_dt:
> > + tvp5150_dt_cleanup(core);
> > + return res;
> > }
> >
> > static int tvp5150_remove(struct i2c_client *c)
>
> --
> Sakari Ailus
> sakari.ai...@linux.intel.com
>
Re: [PATCH v3 0/5] Fix OV5640 exposure & gain
Hi Hughes, The whole series, Acked-by: Steve Longerbeam and Tested-by: Steve Longerbeam on i.MX6q SabreSD with MIPI CSI-2 OV5640 module On 09/11/2018 06:48 AM, Hugues Fruchet wrote: This patch serie fixes some problems around exposure & gain in OV5640 driver. The 4th patch about autocontrols requires also a fix in v4l2-ctrls.c: https://www.mail-archive.com/linux-media@vger.kernel.org/msg133164.html Here is the test procedure used for exposure & gain controls check: 1) Preview in background $> gst-launch-1.0 v4l2src ! "video/x-raw, width=640, Height=480" ! queue ! waylandsink -e & 2) Check gain & exposure values $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" exposure (int): min=0 max=65535 step=1 default=0 value=330 flags=inactive, volatile gain (int): min=0 max=1023 step=1 default=0 value=19 flags=inactive, volatile 3) Put finger in front of camera and check that gain/exposure values are changing: $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" exposure (int): min=0 max=65535 step=1 default=0 value=660 flags=inactive, volatile gain (int): min=0 max=1023 step=1 default=0 value=37 flags=inactive, volatile 4) switch to manual mode, image exposition must not change $> v4l2-ctl --set-ctrl=gain_automatic=0 $> v4l2-ctl --set-ctrl=auto_exposure=1 Note the "1" for manual exposure. 5) Check current gain/exposure values: $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" exposure (int): min=0 max=65535 step=1 default=0 value=330 gain (int): min=0 max=1023 step=1 default=0 value=20 6) Put finger behind camera and check that gain/exposure values are NOT changing: $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" exposure (int): min=0 max=65535 step=1 default=0 value=330 gain (int): min=0 max=1023 step=1 default=0 value=20 7) Update exposure, check that it is well changed on display and that same value is returned: $> v4l2-ctl --set-ctrl=exposure=100 $> v4l2-ctl --get-ctrl=exposure exposure: 100 9) Update gain, check that it is well changed on display and that same value is returned: $> v4l2-ctl --set-ctrl=gain=10 $> v4l2-ctl --get-ctrl=gain gain: 10 10) Switch back to auto gain/exposure, verify that image is correct and values returned are correct: $> v4l2-ctl --set-ctrl=gain_automatic=1 $> v4l2-ctl --set-ctrl=auto_exposure=0 $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" exposure (int): min=0 max=65535 step=1 default=0 value=330 flags=inactive, volatile gain (int): min=0 max=1023 step=1 default=0 value=22 flags=inactive, volatile Note the "0" for auto exposure. === = history = === version 3: - Change patch 5/5 by removing set_mode() orig_mode parameter as per jacopo' suggestion: https://www.spinics.net/lists/linux-media/msg139457.html version 2: - Fix patch 3/5 commit comment and rename binning function as per jacopo' suggestion: https://www.mail-archive.com/linux-media@vger.kernel.org/msg133272.html Hugues Fruchet (5): media: ov5640: fix exposure regression media: ov5640: fix auto gain & exposure when changing mode media: ov5640: fix wrong binning value in exposure calculation media: ov5640: fix auto controls values when switching to manual mode media: ov5640: fix restore of last mode set drivers/media/i2c/ov5640.c | 128 ++--- 1 file changed, 73 insertions(+), 55 deletions(-)
Re: [PATCH v4 2/2] media: ov5640: Fix timings setup code
On 09/14/2018 08:58 AM, Jacopo Mondi wrote:
From: Jacopo Mondi
As of:
commit 476dec012f4c ("media: ov5640: Add horizontal and vertical totals")
the timings parameters gets programmed separately from the static register
values array.
When changing capture mode, the vertical and horizontal totals gets inspected
by the set_mode_exposure_calc() functions, and only later programmed with the
new values. This means exposure, light banding filter and shutter gain are
calculated using the previous timings, and are thus not correct.
Fix this by programming timings right after the static register value table
has been sent to the sensor in the ov5640_load_regs() function.
Fixes: 476dec012f4c ("media: ov5640: Add horizontal and vertical totals")
Tested-by: Steve Longerbeam
on i.MX6q SabreSD with MIPI CSI-2 OV5640 module
Acked-by: Steve Longerbeam
Tested-by: Loic Poulain
on Dragonboard-410c with MIPI CSI-2 OV5640 module
Signed-off-by: Samuel Bobrowicz
Signed-off-by: Maxime Ripard
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/ov5640.c | 50 +++---
1 file changed, 21 insertions(+), 29 deletions(-)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 7ade416..2b9e84f 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -908,6 +908,26 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16
reg,
}
/* download ov5640 settings to sensor through i2c */
+static int ov5640_set_timings(struct ov5640_dev *sensor,
+ const struct ov5640_mode_info *mode)
+{
+ int ret;
+
+ ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPHO, mode->hact);
+ if (ret < 0)
+ return ret;
+
+ ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPVO, mode->vact);
+ if (ret < 0)
+ return ret;
+
+ ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HTS, mode->htot);
+ if (ret < 0)
+ return ret;
+
+ return ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, mode->vtot);
+}
+
static int ov5640_load_regs(struct ov5640_dev *sensor,
const struct ov5640_mode_info *mode)
{
@@ -935,7 +955,7 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
usleep_range(1000 * delay_ms, 1000 * delay_ms + 100);
}
- return ret;
+ return ov5640_set_timings(sensor, mode);
}
/* read exposure, in number of line periods */
@@ -1398,30 +1418,6 @@ static int ov5640_set_virtual_channel(struct ov5640_dev
*sensor)
return ov5640_write_reg(sensor, OV5640_REG_DEBUG_MODE, temp);
}
-static int ov5640_set_timings(struct ov5640_dev *sensor,
- const struct ov5640_mode_info *mode)
-{
- int ret;
-
- ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPHO, mode->hact);
- if (ret < 0)
- return ret;
-
- ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPVO, mode->vact);
- if (ret < 0)
- return ret;
-
- ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HTS, mode->htot);
- if (ret < 0)
- return ret;
-
- ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, mode->vtot);
- if (ret < 0)
- return ret;
-
- return 0;
-}
-
static const struct ov5640_mode_info *
ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
int width, int height, bool nearest)
@@ -1665,10 +1661,6 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
if (ret < 0)
return ret;
- ret = ov5640_set_timings(sensor, mode);
- if (ret < 0)
- return ret;
-
ret = ov5640_set_binning(sensor, dn_mode != SCALING);
if (ret < 0)
return ret;
--
2.7.4
Re: [PATCH v4 1/2] media: ov5640: Re-work MIPI startup sequence
On 09/14/2018 08:58 AM, Jacopo Mondi wrote:
From: Jacopo Mondi
Rework the MIPI interface startup sequence with the following changes:
- Remove MIPI bus initialization from the initial settings blob
- At set_power(1) time power up MIPI Tx/Rx and set data and clock lanes in
LP11 during 'sleep' and 'idle' with MIPI clock in non-continuous mode.
- At s_stream time enable/disable the MIPI interface output.
- Restore default settings at set_power(0) time.
Before this commit the sensor MIPI interface was initialized with settings
that require a start/stop sequence at power-up time in order to force lanes
into LP11 state, as they were initialized in LP00 when in 'sleep mode',
which is assumed to be the sensor manual definition for the D-PHY defined
stop mode.
The stream start/stop was performed by enabling disabling clock gating,
and had the side effect to change the lanes sleep mode configuration when
stream was stopped.
Clock gating/ungating:
- ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
-on ? 0 : BIT(5));
- if (ret)
Set lanes in LP11 when in 'sleep mode':
- ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
- on ? 0x00 : 0x70);
This commit fixes an issue reported by Jagan Teki on i.MX6 platforms that
prevents the host interface from powering up correctly:
https://lkml.org/lkml/2018/6/1/38
It also improves MIPI capture operations stability on my testing platform
where MIPI capture often failed and returned all-purple frames.
fixes: f22996db44e2 ("media: ov5640: add support of DVP parallel interface")
Tested-by: Steve Longerbeam
on i.MX6q SabreSD with MIPI CSI-2 OV5640 module
Acked-by: Steve Longerbeam
Tested-by: Loic Poulain
on Dragonboard-410c with MIPI CSI-2 OV5640 module
Reported-by: Jagan Teki
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/ov5640.c | 99 --
1 file changed, 79 insertions(+), 20 deletions(-)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 071f4bc..7ade416 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -286,10 +286,10 @@ static const struct reg_value
ov5640_init_setting_30fps_VGA[] = {
{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x3000, 0x00, 0, 0},
{0x3002, 0x1c, 0, 0}, {0x3004, 0xff, 0, 0}, {0x3006, 0xc3, 0, 0},
- {0x300e, 0x45, 0, 0}, {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
+ {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
{0x501f, 0x00, 0, 0}, {0x4713, 0x03, 0, 0}, {0x4407, 0x04, 0, 0},
{0x440e, 0x00, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
- {0x4837, 0x0a, 0, 0}, {0x4800, 0x04, 0, 0}, {0x3824, 0x02, 0, 0},
+ {0x4837, 0x0a, 0, 0}, {0x3824, 0x02, 0, 0},
{0x5000, 0xa7, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x5180, 0xff, 0, 0},
{0x5181, 0xf2, 0, 0}, {0x5182, 0x00, 0, 0}, {0x5183, 0x14, 0, 0},
{0x5184, 0x25, 0, 0}, {0x5185, 0x24, 0, 0}, {0x5186, 0x09, 0, 0},
@@ -1102,12 +1102,25 @@ static int ov5640_set_stream_mipi(struct ov5640_dev
*sensor, bool on)
{
int ret;
- ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
-on ? 0 : BIT(5));
- if (ret)
- return ret;
- ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
- on ? 0x00 : 0x70);
+ /*
+* Enable/disable the MIPI interface
+*
+* 0x300e = on ? 0x45 : 0x40
+*
+* FIXME: the sensor manual (version 2.03) reports
+* [7:5] = 000 : 1 data lane mode
+* [7:5] = 001 : 2 data lanes mode
+* But this settings do not work, while the following ones
+* have been validated for 2 data lanes mode.
+*
+* [7:5] = 010 : 2 data lanes mode
+* [4] = 0 : Power up MIPI HS Tx
+* [3] = 0 : Power up MIPI LS Rx
+* [2] = 1/0: MIPI interface enable/disable
+* [1:0] = 01/00: FIXME: 'debug'
+*/
+ ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
+ on ? 0x45 : 0x40);
if (ret)
return ret;
@@ -1786,23 +1799,69 @@ static int ov5640_set_power(struct ov5640_dev *sensor,
bool on)
if (ret)
goto power_off;
+ /* We're done here for DVP bus, while CSI-2 needs setup. */
+ if (sensor->ep.bus_type != V4L2_MBUS_CSI2)
+ return 0;
+
+ /*
+* Power up MIPI HS Tx and LS Rx; 2 data lanes mode
+*
+* 0x300e = 0x40
+* [7:5] = 010 : 2 data lanes mode (see FIXME note in
+*"ov5640_set_stream_mipi()")
+* [4] = 0 : Power up MIPI HS Tx
+* [3] = 0 : Power up
Re: [PATCH v3 0/5] Fix OV5640 exposure & gain
Hi Sakari, On Tue, Sep 11, 2018 at 03:48:16PM +0200, Hugues Fruchet wrote: > This patch serie fixes some problems around exposure & gain in OV5640 driver. As you offered to collect this series and my CSI-2 fixes I have just re-sent, you might be interested in this branch: git://jmondi.org/linux engicam-imx6q/media-master/ov5640/csi2_init_v4_exposure_v3 I have there re-based this series on top of mine, which is in turn based on latest media master, where this series do not apply as-is afaict. I have added to Hugues' patches my reviewed-by and tested-by tags. If you prefer to I can send you a pull request, or if you want to have a chance to review the whole patch list please refer to the above branch. Let me know if I can help speeding up the inclusion of these two series as they fix two real issues of MIPI CSI-2 capture operations for this sensor. Thanks j > > The 4th patch about autocontrols requires also a fix in v4l2-ctrls.c: > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133164.html > > Here is the test procedure used for exposure & gain controls check: > 1) Preview in background > $> gst-launch-1.0 v4l2src ! "video/x-raw, width=640, Height=480" ! queue ! > waylandsink -e & > 2) Check gain & exposure values > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 default=0 > value=330 flags=inactive, volatile >gain (int): min=0 max=1023 step=1 default=0 > value=19 flags=inactive, volatile > 3) Put finger in front of camera and check that gain/exposure values are > changing: > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 default=0 > value=660 flags=inactive, volatile >gain (int): min=0 max=1023 step=1 default=0 > value=37 flags=inactive, volatile > 4) switch to manual mode, image exposition must not change > $> v4l2-ctl --set-ctrl=gain_automatic=0 > $> v4l2-ctl --set-ctrl=auto_exposure=1 > Note the "1" for manual exposure. > > 5) Check current gain/exposure values: > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 default=0 > value=330 >gain (int): min=0 max=1023 step=1 default=0 > value=20 > > 6) Put finger behind camera and check that gain/exposure values are NOT > changing: > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 default=0 > value=330 >gain (int): min=0 max=1023 step=1 default=0 > value=20 > 7) Update exposure, check that it is well changed on display and that same > value is returned: > $> v4l2-ctl --set-ctrl=exposure=100 > $> v4l2-ctl --get-ctrl=exposure > exposure: 100 > > 9) Update gain, check that it is well changed on display and that same value > is returned: > $> v4l2-ctl --set-ctrl=gain=10 > $> v4l2-ctl --get-ctrl=gain > gain: 10 > > 10) Switch back to auto gain/exposure, verify that image is correct and > values returned are correct: > $> v4l2-ctl --set-ctrl=gain_automatic=1 > $> v4l2-ctl --set-ctrl=auto_exposure=0 > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 default=0 > value=330 flags=inactive, volatile >gain (int): min=0 max=1023 step=1 default=0 > value=22 flags=inactive, volatile > Note the "0" for auto exposure. > > === > = history = > === > version 3: > - Change patch 5/5 by removing set_mode() orig_mode parameter as per > jacopo' suggestion: > https://www.spinics.net/lists/linux-media/msg139457.html > > version 2: > - Fix patch 3/5 commit comment and rename binning function as per jacopo' > suggestion: > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133272.html > > Hugues Fruchet (5): > media: ov5640: fix exposure regression > media: ov5640: fix auto gain & exposure when changing mode > media: ov5640: fix wrong binning value in exposure calculation > media: ov5640: fix auto controls values when switching to manual mode > media: ov5640: fix restore of last mode set > > drivers/media/i2c/ov5640.c | 128 > ++--- > 1 file changed, 73 insertions(+), 55 deletions(-) > > -- > 2.7.4 > signature.asc Description: PGP signature
Re: [PATCH v3 0/5] Fix OV5640 exposure & gain
Hi Hugues, thanks for the patches On Tue, Sep 11, 2018 at 03:48:16PM +0200, Hugues Fruchet wrote: > This patch serie fixes some problems around exposure & gain in OV5640 driver. > > The 4th patch about autocontrols requires also a fix in v4l2-ctrls.c: > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133164.html > > Here is the test procedure used for exposure & gain controls check: > 1) Preview in background > $> gst-launch-1.0 v4l2src ! "video/x-raw, width=640, Height=480" ! queue ! > waylandsink -e & > 2) Check gain & exposure values > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 default=0 > value=330 flags=inactive, volatile >gain (int): min=0 max=1023 step=1 default=0 > value=19 flags=inactive, volatile > 3) Put finger in front of camera and check that gain/exposure values are > changing: > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 default=0 > value=660 flags=inactive, volatile >gain (int): min=0 max=1023 step=1 default=0 > value=37 flags=inactive, volatile > 4) switch to manual mode, image exposition must not change > $> v4l2-ctl --set-ctrl=gain_automatic=0 > $> v4l2-ctl --set-ctrl=auto_exposure=1 > Note the "1" for manual exposure. > > 5) Check current gain/exposure values: > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 default=0 > value=330 >gain (int): min=0 max=1023 step=1 default=0 > value=20 > > 6) Put finger behind camera and check that gain/exposure values are NOT > changing: > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 default=0 > value=330 >gain (int): min=0 max=1023 step=1 default=0 > value=20 > 7) Update exposure, check that it is well changed on display and that same > value is returned: > $> v4l2-ctl --set-ctrl=exposure=100 > $> v4l2-ctl --get-ctrl=exposure > exposure: 100 > > 9) Update gain, check that it is well changed on display and that same value > is returned: > $> v4l2-ctl --set-ctrl=gain=10 > $> v4l2-ctl --get-ctrl=gain > gain: 10 > > 10) Switch back to auto gain/exposure, verify that image is correct and > values returned are correct: > $> v4l2-ctl --set-ctrl=gain_automatic=1 > $> v4l2-ctl --set-ctrl=auto_exposure=0 > $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)" >exposure (int): min=0 max=65535 step=1 default=0 > value=330 flags=inactive, volatile >gain (int): min=0 max=1023 step=1 default=0 > value=22 flags=inactive, volatile > Note the "0" for auto exposure. > I've tested on my side and I can confirm the exposure and gain when in auto-mode changes as expected, and it is possible to switch back and forth between auto and manual modes. The patches also fixes an issue when capturing frames, as the first two/three frames where always black in my setup before this series. (While streaming to gstreamers' fakesink) # v4l2-ctl --get-ctrl "exposure" --get-ctrl "gain" -d /dev/video4 exposure: 885 gain: 50 (Point a light in front of the sensor) # v4l2-ctl --get-ctrl "exposure" --get-ctrl "gain" -d /dev/video4 exposure: 17 gain: 19 (Disable auto-gain and auto-exposure) # v4l2-ctl -d /dev/video4 --set-ctrl=auto_exposure=1 # v4l2-ctl -d /dev/video4 --set-ctrl=gain_automatic=0 # v4l2-ctl -d /dev/video4 --set-ctrl=exposure=100 # v4l2-ctl --get-ctrl "exposure" --get-ctrl "gain" -d /dev/video4 exposure: 100 gain: 46 (Re-enable auto-exp and auto-gain) # v4l2-ctl -d /dev/video4 --set-ctrl=auto_exposure=0 # v4l2-ctl -d /dev/video4 --set-ctrl=gain_automatic=1 # v4l2-ctl --get-ctrl "exposure" --get-ctrl "gain" -d /dev/video4 exposure: 885 gain: 46 (Finger on the sensor) # v4l2-ctl --get-ctrl "exposure" --get-ctrl "gain" -d /dev/video4 exposure: 885 gain: 248 (Point a light on the sensor) exposure: 16 gain: 19 So please add my Tested-by: Jacopo Mondi to this series. Thanks j > === > = history = > === > version 3: > - Change patch 5/5 by removing set_mode() orig_mode parameter as per > jacopo' suggestion: > https://www.spinics.net/lists/linux-media/msg139457.html > > version 2: > - Fix patch 3/5 commit comment and rename binning function as per jacopo' > suggestion: > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133272.html > > Hugues Fruchet (5): > media: ov5640: fix exposure regression > media: ov5640: fix auto gain & exposure when changing mode > media: ov5640: fix wrong binning value in exposure calculation > media: ov5640: fix auto controls values when switching to manual mode > media: ov5640: fix restore of last mode set > > drivers/media/i2c/ov5640.c |
[PATCH v4 0/2] media: i2c: ov5640: Re-work MIPI startup sequence
Hello ov5640 people, this driver has received a lot of attention recently, and this series aims to fix the CSI-2 interface startup on i.Mx6Q platforms. Please refer to the v2 cover letters for more background informations: https://www.mail-archive.com/linux-media@vger.kernel.org/msg133420.html This two patches alone allows the MIPI interface to startup properly, but in order to capture good images (good as in 'not completely black') exposure and gain handling should be fixed too. Hugues Fruchet has a series in review that fixes that issues: [PATCH v3 0/5] Fix OV5640 exposure & gain I have re-based Hugues' one this two patches and the latest media-tree master at git://jmondi.org/linux engicam-imx6q/media-master/ov5640/csi2_init_v4_exposure_v3 For the interested to test. Compared to previous version, the series has been tested by Loic on Dragonboard-410c and he helped finding out a discrepancy between the (working) implementation and the sensor manual I have now add a comment on. Testing so far has been done with only 2 data lanes, anyone with a 1-data lane setup willing to test would be great. Thanks j v3 -> v4: - Add Loic's tested by tag - Add comment on register 0x300e[7:5] discrepancy between implementation and sensor manual (thanks Loic) v2 -> v3: - patch [2/2] was originally sent in a different series, compared to v2 it removes entries from the blob array instead of adding more. Jacopo Mondi (2): media: ov5640: Re-work MIPI startup sequence media: ov5640: Fix timings setup code drivers/media/i2c/ov5640.c | 149 ++--- 1 file changed, 100 insertions(+), 49 deletions(-) -- 2.7.4
[PATCH v4 2/2] media: ov5640: Fix timings setup code
From: Jacopo Mondi
As of:
commit 476dec012f4c ("media: ov5640: Add horizontal and vertical totals")
the timings parameters gets programmed separately from the static register
values array.
When changing capture mode, the vertical and horizontal totals gets inspected
by the set_mode_exposure_calc() functions, and only later programmed with the
new values. This means exposure, light banding filter and shutter gain are
calculated using the previous timings, and are thus not correct.
Fix this by programming timings right after the static register value table
has been sent to the sensor in the ov5640_load_regs() function.
Fixes: 476dec012f4c ("media: ov5640: Add horizontal and vertical totals")
Tested-by: Steve Longerbeam
on i.MX6q SabreSD with MIPI CSI-2 OV5640 module
Tested-by: Loic Poulain
on Dragonboard-410c with MIPI CSI-2 OV5640 module
Signed-off-by: Samuel Bobrowicz
Signed-off-by: Maxime Ripard
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/ov5640.c | 50 +++---
1 file changed, 21 insertions(+), 29 deletions(-)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 7ade416..2b9e84f 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -908,6 +908,26 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16
reg,
}
/* download ov5640 settings to sensor through i2c */
+static int ov5640_set_timings(struct ov5640_dev *sensor,
+ const struct ov5640_mode_info *mode)
+{
+ int ret;
+
+ ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPHO, mode->hact);
+ if (ret < 0)
+ return ret;
+
+ ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPVO, mode->vact);
+ if (ret < 0)
+ return ret;
+
+ ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HTS, mode->htot);
+ if (ret < 0)
+ return ret;
+
+ return ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, mode->vtot);
+}
+
static int ov5640_load_regs(struct ov5640_dev *sensor,
const struct ov5640_mode_info *mode)
{
@@ -935,7 +955,7 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
usleep_range(1000 * delay_ms, 1000 * delay_ms + 100);
}
- return ret;
+ return ov5640_set_timings(sensor, mode);
}
/* read exposure, in number of line periods */
@@ -1398,30 +1418,6 @@ static int ov5640_set_virtual_channel(struct ov5640_dev
*sensor)
return ov5640_write_reg(sensor, OV5640_REG_DEBUG_MODE, temp);
}
-static int ov5640_set_timings(struct ov5640_dev *sensor,
- const struct ov5640_mode_info *mode)
-{
- int ret;
-
- ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPHO, mode->hact);
- if (ret < 0)
- return ret;
-
- ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPVO, mode->vact);
- if (ret < 0)
- return ret;
-
- ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HTS, mode->htot);
- if (ret < 0)
- return ret;
-
- ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, mode->vtot);
- if (ret < 0)
- return ret;
-
- return 0;
-}
-
static const struct ov5640_mode_info *
ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
int width, int height, bool nearest)
@@ -1665,10 +1661,6 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
if (ret < 0)
return ret;
- ret = ov5640_set_timings(sensor, mode);
- if (ret < 0)
- return ret;
-
ret = ov5640_set_binning(sensor, dn_mode != SCALING);
if (ret < 0)
return ret;
--
2.7.4
[PATCH v4 1/2] media: ov5640: Re-work MIPI startup sequence
From: Jacopo Mondi
Rework the MIPI interface startup sequence with the following changes:
- Remove MIPI bus initialization from the initial settings blob
- At set_power(1) time power up MIPI Tx/Rx and set data and clock lanes in
LP11 during 'sleep' and 'idle' with MIPI clock in non-continuous mode.
- At s_stream time enable/disable the MIPI interface output.
- Restore default settings at set_power(0) time.
Before this commit the sensor MIPI interface was initialized with settings
that require a start/stop sequence at power-up time in order to force lanes
into LP11 state, as they were initialized in LP00 when in 'sleep mode',
which is assumed to be the sensor manual definition for the D-PHY defined
stop mode.
The stream start/stop was performed by enabling disabling clock gating,
and had the side effect to change the lanes sleep mode configuration when
stream was stopped.
Clock gating/ungating:
- ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
-on ? 0 : BIT(5));
- if (ret)
Set lanes in LP11 when in 'sleep mode':
- ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
- on ? 0x00 : 0x70);
This commit fixes an issue reported by Jagan Teki on i.MX6 platforms that
prevents the host interface from powering up correctly:
https://lkml.org/lkml/2018/6/1/38
It also improves MIPI capture operations stability on my testing platform
where MIPI capture often failed and returned all-purple frames.
fixes: f22996db44e2 ("media: ov5640: add support of DVP parallel interface")
Tested-by: Steve Longerbeam
on i.MX6q SabreSD with MIPI CSI-2 OV5640 module
Tested-by: Loic Poulain
on Dragonboard-410c with MIPI CSI-2 OV5640 module
Reported-by: Jagan Teki
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/ov5640.c | 99 --
1 file changed, 79 insertions(+), 20 deletions(-)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 071f4bc..7ade416 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -286,10 +286,10 @@ static const struct reg_value
ov5640_init_setting_30fps_VGA[] = {
{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x3000, 0x00, 0, 0},
{0x3002, 0x1c, 0, 0}, {0x3004, 0xff, 0, 0}, {0x3006, 0xc3, 0, 0},
- {0x300e, 0x45, 0, 0}, {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
+ {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
{0x501f, 0x00, 0, 0}, {0x4713, 0x03, 0, 0}, {0x4407, 0x04, 0, 0},
{0x440e, 0x00, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
- {0x4837, 0x0a, 0, 0}, {0x4800, 0x04, 0, 0}, {0x3824, 0x02, 0, 0},
+ {0x4837, 0x0a, 0, 0}, {0x3824, 0x02, 0, 0},
{0x5000, 0xa7, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x5180, 0xff, 0, 0},
{0x5181, 0xf2, 0, 0}, {0x5182, 0x00, 0, 0}, {0x5183, 0x14, 0, 0},
{0x5184, 0x25, 0, 0}, {0x5185, 0x24, 0, 0}, {0x5186, 0x09, 0, 0},
@@ -1102,12 +1102,25 @@ static int ov5640_set_stream_mipi(struct ov5640_dev
*sensor, bool on)
{
int ret;
- ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
-on ? 0 : BIT(5));
- if (ret)
- return ret;
- ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
- on ? 0x00 : 0x70);
+ /*
+* Enable/disable the MIPI interface
+*
+* 0x300e = on ? 0x45 : 0x40
+*
+* FIXME: the sensor manual (version 2.03) reports
+* [7:5] = 000 : 1 data lane mode
+* [7:5] = 001 : 2 data lanes mode
+* But this settings do not work, while the following ones
+* have been validated for 2 data lanes mode.
+*
+* [7:5] = 010 : 2 data lanes mode
+* [4] = 0 : Power up MIPI HS Tx
+* [3] = 0 : Power up MIPI LS Rx
+* [2] = 1/0: MIPI interface enable/disable
+* [1:0] = 01/00: FIXME: 'debug'
+*/
+ ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
+ on ? 0x45 : 0x40);
if (ret)
return ret;
@@ -1786,23 +1799,69 @@ static int ov5640_set_power(struct ov5640_dev *sensor,
bool on)
if (ret)
goto power_off;
+ /* We're done here for DVP bus, while CSI-2 needs setup. */
+ if (sensor->ep.bus_type != V4L2_MBUS_CSI2)
+ return 0;
+
+ /*
+* Power up MIPI HS Tx and LS Rx; 2 data lanes mode
+*
+* 0x300e = 0x40
+* [7:5] = 010 : 2 data lanes mode (see FIXME note in
+*"ov5640_set_stream_mipi()")
+* [4] = 0 : Power up MIPI HS Tx
+* [3] = 0 : Power up MIPI LS Rx
+* [2] = 0 : MIPI interface disabled
+
Re: [PATCH v2 1/7] [media] tvp5150: add input source selection of_graph support
Hi Marco,
On Mon, Aug 13, 2018 at 11:25:02AM +0200, Marco Felsch wrote:
...
> +static void tvp5150_dt_cleanup(struct tvp5150 *decoder)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < TVP5150_NUM_PADS; i++)
> + of_node_put(decoder->endpoints[i]);
> +}
> +
> static const char * const tvp5150_test_patterns[2] = {
> "Disabled",
> "Black screen"
> @@ -1586,7 +1996,7 @@ static int tvp5150_probe(struct i2c_client *c,
> res = tvp5150_parse_dt(core, np);
> if (res) {
> dev_err(sd->dev, "DT parsing error: %d\n", res);
> - return res;
> + goto err_cleanup_dt;
> }
> } else {
> /* Default to BT.656 embedded sync */
> @@ -1594,25 +2004,16 @@ static int tvp5150_probe(struct i2c_client *c,
> }
>
> v4l2_i2c_subdev_init(sd, c, _ops);
> + sd->internal_ops = _internal_ops;
> sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> - core->pads[TVP5150_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
> - core->pads[TVP5150_PAD_IF_INPUT].sig_type = PAD_SIGNAL_ANALOG;
> - core->pads[TVP5150_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
> - core->pads[TVP5150_PAD_VID_OUT].sig_type = PAD_SIGNAL_DV;
> -
> - sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
> -
> - res = media_entity_pads_init(>entity, TVP5150_NUM_PADS, core->pads);
> - if (res < 0)
> - return res;
> -
> -#endif
> + res = tvp5150_mc_init(sd);
> + if (res)
> + goto err_cleanup_dt;
>
> res = tvp5150_detect_version(core);
> if (res < 0)
> - return res;
> + goto err_cleanup_dt;
>
> core->norm = V4L2_STD_ALL; /* Default is autodetect */
> core->detected_norm = V4L2_STD_UNKNOWN;
> @@ -1664,6 +2065,9 @@ static int tvp5150_probe(struct i2c_client *c,
> err:
Now that you have more error labels, you could rename this one.
> v4l2_ctrl_handler_free(>hdl);
> return res;
Is the above line intended to be kept?
> +err_cleanup_dt:
> + tvp5150_dt_cleanup(core);
> + return res;
> }
>
> static int tvp5150_remove(struct i2c_client *c)
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v2 4/7] [media] v4l2-subdev: fix v4l2_subdev_get_try_* dependency
Hi Marco,
On Mon, Aug 13, 2018 at 11:25:05AM +0200, Marco Felsch wrote:
> These helpers make us of the media-controller entity which is only
> available if the CONFIG_MEDIA_CONTROLLER is enabled.
>
> Signed-off-by: Marco Felsch
> ---
> include/media/v4l2-subdev.h | 100 ++--
> 1 file changed, 50 insertions(+), 50 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index ce48f1fcf295..79c066934ad2 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -912,6 +912,56 @@ struct v4l2_subdev_fh {
> #define to_v4l2_subdev_fh(fh)\
> container_of(fh, struct v4l2_subdev_fh, vfh)
>
> +extern const struct v4l2_file_operations v4l2_subdev_fops;
> +
> +/**
> + * v4l2_set_subdevdata - Sets V4L2 dev private device data
> + *
> + * @sd: pointer to v4l2_subdev
> + * @p: pointer to the private device data to be stored.
> + */
> +static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
> +{
> + sd->dev_priv = p;
> +}
> +
> +/**
> + * v4l2_get_subdevdata - Gets V4L2 dev private device data
> + *
> + * @sd: pointer to v4l2_subdev
> + *
> + * Returns the pointer to the private device data to be stored.
> + */
> +static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
> +{
> + return sd->dev_priv;
> +}
> +
> +/**
> + * v4l2_set_subdev_hostdata - Sets V4L2 dev private host data
> + *
> + * @sd: pointer to v4l2_subdev
> + * @p: pointer to the private data to be stored.
> + */
> +static inline void v4l2_set_subdev_hostdata(struct v4l2_subdev *sd, void *p)
> +{
> + sd->host_priv = p;
> +}
> +
> +/**
> + * v4l2_get_subdev_hostdata - Gets V4L2 dev private data
> + *
> + * @sd: pointer to v4l2_subdev
> + *
> + * Returns the pointer to the private host data to be stored.
> + */
> +static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
> +{
> + return sd->host_priv;
> +}
Could you leave the functions dealing with host_priv where they are? I'd
like to avoid expanding their use; rather reduce it. The field is
problematic in some cases and generally not really needed either.
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +
> /**
> * v4l2_subdev_get_try_format - ancillary routine to call
> *v4l2_subdev_pad_config->try_fmt
> @@ -978,56 +1028,6 @@ static inline struct v4l2_rect
> #endif
> }
>
> -extern const struct v4l2_file_operations v4l2_subdev_fops;
> -
> -/**
> - * v4l2_set_subdevdata - Sets V4L2 dev private device data
> - *
> - * @sd: pointer to v4l2_subdev
> - * @p: pointer to the private device data to be stored.
> - */
> -static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
> -{
> - sd->dev_priv = p;
> -}
> -
> -/**
> - * v4l2_get_subdevdata - Gets V4L2 dev private device data
> - *
> - * @sd: pointer to v4l2_subdev
> - *
> - * Returns the pointer to the private device data to be stored.
> - */
> -static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
> -{
> - return sd->dev_priv;
> -}
> -
> -/**
> - * v4l2_set_subdev_hostdata - Sets V4L2 dev private host data
> - *
> - * @sd: pointer to v4l2_subdev
> - * @p: pointer to the private data to be stored.
> - */
> -static inline void v4l2_set_subdev_hostdata(struct v4l2_subdev *sd, void *p)
> -{
> - sd->host_priv = p;
> -}
> -
> -/**
> - * v4l2_get_subdev_hostdata - Gets V4L2 dev private data
> - *
> - * @sd: pointer to v4l2_subdev
> - *
> - * Returns the pointer to the private host data to be stored.
> - */
> -static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
> -{
> - return sd->host_priv;
> -}
> -
> -#ifdef CONFIG_MEDIA_CONTROLLER
> -
> /**
> * v4l2_subdev_link_validate_default - validates a media link
> *
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v2 7/7] [media] tvp5150: add s_power callback
Hi Marco,
On Mon, Aug 13, 2018 at 11:25:08AM +0200, Marco Felsch wrote:
> Don't en-/disable the interrupts during s_stream because someone can
> disable the stream but wants to get informed if the stream is locked
> again. So keep the interrupts enabled the whole time the pipeline is
> opened.
>
> Signed-off-by: Marco Felsch
> ---
> drivers/media/i2c/tvp5150.c | 23 +--
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index e736f609fecd..e296f5bfae21 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -1389,11 +1389,26 @@ static const struct media_entity_operations
> tvp5150_sd_media_ops = {
> /
> I2C Command
>
> /
> +static int tvp5150_s_power(struct v4l2_subdev *sd, int on)
> +{
> + struct tvp5150 *decoder = to_tvp5150(sd);
> + unsigned int val = 0;
> +
> + if (on)
> + val = TVP5150_INT_A_LOCK;
> +
> + if (decoder->irq)
> + /* Enable / Disable lock interrupt */
> + regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
> +TVP5150_INT_A_LOCK, val);
Could you use runtime PM instead?
For an example, the dw9714 driver does this: drivers/media/i2c/dw9714.c .
> +
> + return 0;
> +}
>
> static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
> {
> struct tvp5150 *decoder = to_tvp5150(sd);
> - unsigned int mask, val = 0, int_val = 0;
> + unsigned int mask, val = 0;
>
> mask = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE |
> TVP5150_MISC_CTL_CLOCK_OE;
> @@ -1406,15 +1421,10 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd,
> int enable)
> val = decoder->lock ? decoder->oe : 0;
> else
> val = decoder->oe;
> - int_val = TVP5150_INT_A_LOCK;
> v4l2_subdev_notify_event(>sd, _ev_fmt);
> }
>
> regmap_update_bits(decoder->regmap, TVP5150_MISC_CTL, mask, val);
> - if (decoder->irq)
> - /* Enable / Disable lock interrupt */
> - regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
> -TVP5150_INT_A_LOCK, int_val);
>
> return 0;
> }
> @@ -1616,6 +1626,7 @@ static const struct v4l2_subdev_core_ops
> tvp5150_core_ops = {
> .g_register = tvp5150_g_register,
> .s_register = tvp5150_s_register,
> #endif
> + .s_power = tvp5150_s_power,
> };
>
> static const struct v4l2_subdev_tuner_ops tvp5150_tuner_ops = {
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v2 0/4] media: soc_camera: ov9640: switch driver to v4l2_async
Hi Petr, On Wed, Aug 15, 2018 at 03:30:23PM +0200, petrcve...@gmail.com wrote: > From: Petr Cvek > > This patch series transfer the ov9640 driver from the soc_camera subsystem > into a standalone v4l2 driver. There is no changes except the required > v4l2_async calls, GPIO allocation, deletion of now unused variables, > a change from mdelay() to msleep() and an addition of SPDX identifiers > (as suggested in the v1 version RFC). > > The config symbol has been changed from CONFIG_SOC_CAMERA_OV9640 to > CONFIG_VIDEO_OV9640. > > Also as the drivers of the soc_camera seems to be orphaned I'm volunteering > as a maintainer of the driver (I own the hardware). Thanks for the set. The patches seem good to me as such but there's some more work to do there. For one, the depedency to v4l2_clk should be removed; the common clock framework has supported clocks from random devices for many, many years now. The PXA camera driver does still depend on v4l2_clk so I guess this is better to do later on in a different patchset. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v2 1/4] media: soc_camera: ov9640: move ov9640 out of soc_camera
Hi Petr,
Thanks for the patchset, and my apologies for reviewing it so late!
I'm commenting this one but feel free to add patches to address the
comments.
On Wed, Aug 15, 2018 at 03:30:24PM +0200, petrcve...@gmail.com wrote:
> From: Petr Cvek
>
> Initial part of ov9640 transition from soc_camera subsystem to a standalone
> v4l2 subdevice. The soc_camera version seems to be used only in Palm Zire72
> and in (the future) HTC Magician. On these two devices the support is
> broken as pxa_camera driver doesn't use soc_camera anymore. The other
> mentions from git grep are "TODOs" (in board-osk.c) or chip names for
> unsupported sensors on HW which doesn't use soc_camera at all (irelevant).
>
> Copy the driver files from soc_camera and mark the original ones in the
> Kconfig description as obsoleted.
>
> Add config option VIDEO_OV9640 to the build files in drivers/media/i2c.
>
> Signed-off-by: Petr Cvek
> ---
> drivers/media/i2c/Kconfig| 7 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/ov9640.c | 739 +++
> drivers/media/i2c/ov9640.h | 208
> drivers/media/i2c/soc_camera/Kconfig | 6 +-
> 5 files changed, 959 insertions(+), 2 deletions(-)
> create mode 100644 drivers/media/i2c/ov9640.c
> create mode 100644 drivers/media/i2c/ov9640.h
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 439f6be08b95..c948b163a567 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -771,6 +771,13 @@ config VIDEO_OV7740
> This is a Video4Linux2 sensor driver for the OmniVision
> OV7740 VGA camera sensor.
>
> +config VIDEO_OV9640
> + tristate "OmniVision OV9640 sensor support"
> + depends on I2C && VIDEO_V4L2
> + help
> + This is a Video4Linux2 sensor driver for the OmniVision
> + OV9640 camera sensor.
> +
> config VIDEO_OV9650
> tristate "OmniVision OV9650/OV9652 sensor support"
> depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 837c428339df..9cc951f9c041 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
> obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
> obj-$(CONFIG_VIDEO_OV772X) += ov772x.o
> obj-$(CONFIG_VIDEO_OV7740) += ov7740.o
> +obj-$(CONFIG_VIDEO_OV9640) += ov9640.o
> obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
> obj-$(CONFIG_VIDEO_OV13858) += ov13858.o
> obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
> diff --git a/drivers/media/i2c/ov9640.c b/drivers/media/i2c/ov9640.c
> new file mode 100644
> index ..c63948989688
> --- /dev/null
> +++ b/drivers/media/i2c/ov9640.c
> @@ -0,0 +1,739 @@
> +/*
> + * OmniVision OV96xx Camera Driver
> + *
> + * Copyright (C) 2009 Marek Vasut
> + *
> + * Based on ov772x camera driver:
> + *
> + * Copyright (C) 2008 Renesas Solutions Corp.
> + * Kuninori Morimoto
> + *
> + * Based on ov7670 and soc_camera_platform driver,
> + *
> + * Copyright 2006-7 Jonathan Corbet
> + * Copyright (C) 2008 Magnus Damm
> + * Copyright (C) 2008, Guennadi Liakhovetski
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include "ov9640.h"
> +
> +#define to_ov9640_sensor(sd) container_of(sd, struct ov9640_priv, subdev)
> +
> +/* default register setup */
> +static const struct ov9640_reg ov9640_regs_dflt[] = {
> + { OV9640_COM5, OV9640_COM5_SYSCLK | OV9640_COM5_LONGEXP },
> + { OV9640_COM6, OV9640_COM6_OPT_BLC | OV9640_COM6_ADBLC_BIAS |
> + OV9640_COM6_FMT_RST | OV9640_COM6_ADBLC_OPTEN },
> + { OV9640_PSHFT, OV9640_PSHFT_VAL(0x01) },
> + { OV9640_ACOM, OV9640_ACOM_2X_ANALOG | OV9640_ACOM_RSVD },
> + { OV9640_TSLB, OV9640_TSLB_YUYV_UYVY },
> + { OV9640_COM16, OV9640_COM16_RB_AVG },
> +
> + /* Gamma curve P */
> + { 0x6c, 0x40 }, { 0x6d, 0x30 }, { 0x6e, 0x4b }, { 0x6f, 0x60 },
> + { 0x70, 0x70 }, { 0x71, 0x70 }, { 0x72, 0x70 }, { 0x73, 0x70 },
> + { 0x74, 0x60 }, { 0x75, 0x60 }, { 0x76, 0x50 }, { 0x77, 0x48 },
> + { 0x78, 0x3a }, { 0x79, 0x2e }, { 0x7a, 0x28 }, { 0x7b, 0x22 },
> +
> + /* Gamma curve T */
> + { 0x7c, 0x04 }, { 0x7d, 0x07 }, { 0x7e, 0x10 }, { 0x7f, 0x28 },
> + { 0x80, 0x36 }, { 0x81, 0x44 }, { 0x82, 0x52 }, { 0x83, 0x60 },
> + { 0x84, 0x6c }, { 0x85, 0x78 }, { 0x86, 0x8c }, { 0x87, 0x9e },
> + { 0x88, 0xbb }, { 0x89, 0xd2 }, { 0x8a, 0xe6 },
> +};
> +
> +/* Configurations
> + * NOTE: for YUV, alter the following registers:
> + * COM12 |= OV9640_COM12_YUV_AVG
> + *
> + *for RGB, alter the following registers:
> + *
[GIT PULL FOR v4.20 (request_api branch)] Add Allwinner cedrus decoder driver (v2)
Hi Mauro, This is the cedrus Allwinner decoder driver. It is for the request_api topic branch. Note that there is a COMPILE_TEST issue with sram functions, for that another patch is needed: https://lore.kernel.org/patchwork/patch/983848/ But that's going through another subsystem and is already queued up for 4.20. The first two patches fix two trivial sparse and smatch issues. Many, many thanks go to Paul for working on this, trying to keep up to date with the Request API changes at the same time. It was a pleasure working with you on this! I'm now using a signed tag, let me know if this works or not. Regards, Hans The following changes since commit d4215edbd4b170b207b0e5a1d8ae42fb49f5c470: media: media-request: update documentation (2018-09-11 09:58:43 -0400) are available in the Git repository at: git://linuxtv.org/hverkuil/media_tree.git tags/br-cedrus for you to fetch changes up to 615ba78ac81ce76edf5ae84981e404fd4eee3ee0: media: platform: Add Cedrus VPU decoder driver (2018-09-14 14:27:45 +0200) Tag cedrus branch Hans Verkuil (2): v4l2-compat-ioctl32.c: fix sparse warning v4l2-ctrls.c: fix smatch error Paul Kocialkowski (5): media: videobuf2-core: Rework and rename helper for request buffer count media: v4l: Add definitions for MPEG-2 slice format and metadata media: v4l: Add definition for the Sunxi tiled NV12 format dt-bindings: media: Document bindings for the Cedrus VPU driver media: platform: Add Cedrus VPU decoder driver Documentation/devicetree/bindings/media/cedrus.txt | 54 +++ Documentation/media/uapi/v4l/extended-controls.rst | 176 ++ Documentation/media/uapi/v4l/pixfmt-compressed.rst | 16 ++ Documentation/media/uapi/v4l/pixfmt-reserved.rst | 15 +- Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 14 +- Documentation/media/videodev2.h.rst.exceptions | 2 + MAINTAINERS| 7 + drivers/media/common/videobuf2/videobuf2-core.c| 18 +-- drivers/media/common/videobuf2/videobuf2-v4l2.c| 2 +- drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 1 + drivers/media/v4l2-core/v4l2-ctrls.c | 65 +++- drivers/media/v4l2-core/v4l2-ioctl.c | 2 + drivers/staging/media/Kconfig | 2 + drivers/staging/media/Makefile | 1 + drivers/staging/media/sunxi/Kconfig| 15 ++ drivers/staging/media/sunxi/Makefile | 1 + drivers/staging/media/sunxi/cedrus/Kconfig | 14 ++ drivers/staging/media/sunxi/cedrus/Makefile| 3 + drivers/staging/media/sunxi/cedrus/TODO| 7 + drivers/staging/media/sunxi/cedrus/cedrus.c| 431 + drivers/staging/media/sunxi/cedrus/cedrus.h| 167 + drivers/staging/media/sunxi/cedrus/cedrus_dec.c| 70 + drivers/staging/media/sunxi/cedrus/cedrus_dec.h| 27 drivers/staging/media/sunxi/cedrus/cedrus_hw.c | 327 drivers/staging/media/sunxi/cedrus/cedrus_hw.h | 30 drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 246 ++ drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 235 + drivers/staging/media/sunxi/cedrus/cedrus_video.c | 542 +++ drivers/staging/media/sunxi/cedrus/cedrus_video.h | 30 include/media/v4l2-ctrls.h | 18 ++- include/media/videobuf2-core.h | 4 +- include/uapi/linux/v4l2-controls.h | 65 include/uapi/linux/videodev2.h | 6 + 33 files changed, 2589 insertions(+), 24 deletions(-) create mode 100644 Documentation/devicetree/bindings/media/cedrus.txt create mode 100644 drivers/staging/media/sunxi/Kconfig create mode 100644 drivers/staging/media/sunxi/Makefile create mode 100644 drivers/staging/media/sunxi/cedrus/Kconfig create mode 100644 drivers/staging/media/sunxi/cedrus/Makefile create mode 100644 drivers/staging/media/sunxi/cedrus/TODO create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus.c create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus.h create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_dec.c create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_dec.h create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_hw.c create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_hw.h create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_regs.h create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_video.c create mode
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Ping-chung, My apologies for the late review. On Wed, Aug 08, 2018 at 03:16:00PM +0800, 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 > Reviewed-by: Tomasz Figa > > --- > 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. > since v4: > -- Fix the wrong index in link_freq_menu_items. > > 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..61de0c8 > --- /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_STANDBY 0x00 > +#define IMX208_MODE_STREAMING0x01 > + > +/* 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_384MHZ2248 > +#define IMX208_PPL_96MHZ 2248 Does this generally depend on the link frequency? > + > +/* Exposure control */ > +#define IMX208_REG_EXPOSURE 0x0202 > +#define IMX208_EXPOSURE_MIN 4 > +#define IMX208_EXPOSURE_STEP 1 > +#define IMX208_EXPOSURE_DEFAULT 0x190 > +#define IMX208_EXPOSURE_MAX 65535 > + > +/* Analog gain control */ > +#define IMX208_REG_ANALOG_GAIN 0x0204 > +#define IMX208_ANA_GAIN_MIN 0 > +#define IMX208_ANA_GAIN_MAX 0x00e0 > +#define IMX208_ANA_GAIN_STEP 1 > +#define IMX208_ANA_GAIN_DEFAULT 0x0 > + > +/* Digital gain control */ > +#define
[PATCH v3 1/1] v4l: event: Prevent freeing event subscriptions while accessed
The event subscriptions are added to the subscribed event list while
holding a spinlock, but that lock is subsequently released while still
accessing the subscription object. This makes it possible to unsubscribe
the event --- and freeing the subscription object's memory --- while
the subscription object is simultaneously accessed.
Prevent this by adding a mutex to serialise the event subscription and
unsubscription. This also gives a guarantee to the callback ops that the
add op has returned before the del op is called.
This change also results in making the elems field less special:
subscriptions are only added to the event list once they are fully
initialised.
Signed-off-by: Sakari Ailus
Reviewed-by: Hans Verkuil
---
since v1:
- Call the mutex field subscribe_lock instead.
- Move the field that is now subscribe_lock above the subscribed field the
write access to which it serialises.
- Improve documentation of the subscribe_lock field.
since v2:
- Acquire spinlock for the duration of list_add() in v4l2_event_subscribe().
- Remove a redundant comment in the same place.
drivers/media/v4l2-core/v4l2-event.c | 38 +++-
drivers/media/v4l2-core/v4l2-fh.c| 2 ++
include/media/v4l2-fh.h | 4
3 files changed, 26 insertions(+), 18 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index 127fe6eb91d9..a3ef1f50a4b3 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
const struct v4l2_event *e
if (sev == NULL)
return;
- /*
-* If the event has been added to the fh->subscribed list, but its
-* add op has not completed yet elems will be 0, treat this as
-* not being subscribed.
-*/
- if (!sev->elems)
- return;
-
/* Increase event sequence number on fh. */
fh->sequence++;
@@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
struct v4l2_subscribed_event *sev, *found_ev;
unsigned long flags;
unsigned i;
+ int ret = 0;
if (sub->type == V4L2_EVENT_ALL)
return -EINVAL;
@@ -225,31 +218,36 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
sev->flags = sub->flags;
sev->fh = fh;
sev->ops = ops;
+ sev->elems = elems;
+
+ mutex_lock(>subscribe_lock);
spin_lock_irqsave(>vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (!found_ev)
- list_add(>list, >subscribed);
spin_unlock_irqrestore(>vdev->fh_lock, flags);
if (found_ev) {
+ /* Already listening */
kvfree(sev);
- return 0; /* Already listening */
+ goto out_unlock;
}
if (sev->ops && sev->ops->add) {
- int ret = sev->ops->add(sev, elems);
+ ret = sev->ops->add(sev, elems);
if (ret) {
- sev->ops = NULL;
- v4l2_event_unsubscribe(fh, sub);
- return ret;
+ kvfree(sev);
+ goto out_unlock;
}
}
- /* Mark as ready for use */
- sev->elems = elems;
+ spin_lock_irqsave(>vdev->fh_lock, flags);
+ list_add(>list, >subscribed);
+ spin_unlock_irqrestore(>vdev->fh_lock, flags);
- return 0;
+out_unlock:
+ mutex_unlock(>subscribe_lock);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
@@ -288,6 +286,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
return 0;
}
+ mutex_lock(>subscribe_lock);
+
spin_lock_irqsave(>vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
@@ -305,6 +305,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
if (sev && sev->ops && sev->ops->del)
sev->ops->del(sev);
+ mutex_unlock(>subscribe_lock);
+
kvfree(sev);
return 0;
diff --git a/drivers/media/v4l2-core/v4l2-fh.c
b/drivers/media/v4l2-core/v4l2-fh.c
index 3895999bf880..c91a7bd3ecfc 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -45,6 +45,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device
*vdev)
INIT_LIST_HEAD(>available);
INIT_LIST_HEAD(>subscribed);
fh->sequence = -1;
+ mutex_init(>subscribe_lock);
}
EXPORT_SYMBOL_GPL(v4l2_fh_init);
@@ -90,6 +91,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
return;
v4l_disable_media_source(fh->vdev);
v4l2_event_unsubscribe_all(fh);
+ mutex_destroy(>subscribe_lock);
fh->vdev = NULL;
}
EXPORT_SYMBOL_GPL(v4l2_fh_exit);
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index
Re: [PATCH v2 1/1] v4l: event: Prevent freeing event subscriptions while accessed
On 09/13/18 00:24, Sakari Ailus wrote:
> The event subscriptions are added to the subscribed event list while
> holding a spinlock, but that lock is subsequently released while still
> accessing the subscription object. This makes it possible to unsubscribe
> the event --- and freeing the subscription object's memory --- while
> the subscription object is simultaneously accessed.
>
> Prevent this by adding a mutex to serialise the event subscription and
> unsubscription. This also gives a guarantee to the callback ops that the
> add op has returned before the del op is called.
>
> This change also results in making the elems field less special:
> subscriptions are only added to the event list once they are fully
> initialised.
>
> Signed-off-by: Sakari Ailus
Reviewed-by: Hans Verkuil
Nice!
Hans
> ---
> since v1:
>
> - Call the mutex field subscribe_lock instead.
>
> - Move the field that is now subscribe_lock above the subscribed field the
> write access to which it serialises.
>
> - Improve documentation of the subscribe_lock field.
>
> drivers/media/v4l2-core/v4l2-event.c | 35 ++-
> drivers/media/v4l2-core/v4l2-fh.c| 2 ++
> include/media/v4l2-fh.h | 4
> 3 files changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-event.c
> b/drivers/media/v4l2-core/v4l2-event.c
> index 127fe6eb91d9..b76fd92e24c8 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
> const struct v4l2_event *e
> if (sev == NULL)
> return;
>
> - /*
> - * If the event has been added to the fh->subscribed list, but its
> - * add op has not completed yet elems will be 0, treat this as
> - * not being subscribed.
> - */
> - if (!sev->elems)
> - return;
> -
> /* Increase event sequence number on fh. */
> fh->sequence++;
>
> @@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> struct v4l2_subscribed_event *sev, *found_ev;
> unsigned long flags;
> unsigned i;
> + int ret = 0;
>
> if (sub->type == V4L2_EVENT_ALL)
> return -EINVAL;
> @@ -225,31 +218,35 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> sev->flags = sub->flags;
> sev->fh = fh;
> sev->ops = ops;
> + sev->elems = elems;
> +
> + mutex_lock(>subscribe_lock);
>
> spin_lock_irqsave(>vdev->fh_lock, flags);
> found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
> - if (!found_ev)
> - list_add(>list, >subscribed);
> spin_unlock_irqrestore(>vdev->fh_lock, flags);
>
> if (found_ev) {
> + /* Already listening */
> kvfree(sev);
> - return 0; /* Already listening */
> + goto out_unlock;
> }
>
> if (sev->ops && sev->ops->add) {
> - int ret = sev->ops->add(sev, elems);
> + ret = sev->ops->add(sev, elems);
> if (ret) {
> - sev->ops = NULL;
> - v4l2_event_unsubscribe(fh, sub);
> - return ret;
> + kvfree(sev);
> + goto out_unlock;
> }
> }
>
> /* Mark as ready for use */
> - sev->elems = elems;
> + list_add(>list, >subscribed);
>
> - return 0;
> +out_unlock:
> + mutex_unlock(>subscribe_lock);
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
>
> @@ -288,6 +285,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> return 0;
> }
>
> + mutex_lock(>subscribe_lock);
> +
> spin_lock_irqsave(>vdev->fh_lock, flags);
>
> sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> @@ -305,6 +304,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> if (sev && sev->ops && sev->ops->del)
> sev->ops->del(sev);
>
> + mutex_unlock(>subscribe_lock);
> +
> kvfree(sev);
>
> return 0;
> diff --git a/drivers/media/v4l2-core/v4l2-fh.c
> b/drivers/media/v4l2-core/v4l2-fh.c
> index 3895999bf880..c91a7bd3ecfc 100644
> --- a/drivers/media/v4l2-core/v4l2-fh.c
> +++ b/drivers/media/v4l2-core/v4l2-fh.c
> @@ -45,6 +45,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device
> *vdev)
> INIT_LIST_HEAD(>available);
> INIT_LIST_HEAD(>subscribed);
> fh->sequence = -1;
> + mutex_init(>subscribe_lock);
> }
> EXPORT_SYMBOL_GPL(v4l2_fh_init);
>
> @@ -90,6 +91,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
> return;
> v4l_disable_media_source(fh->vdev);
> v4l2_event_unsubscribe_all(fh);
> + mutex_destroy(>subscribe_lock);
> fh->vdev = NULL;
> }
> EXPORT_SYMBOL_GPL(v4l2_fh_exit);
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> index ea73fef8bdc0..8586cfb49828 100644
> ---
[PATCH for request_api branch] v4l2-compat-ioctl32.c: fix sparse warning
Fix this sparse warning:
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:256: warning:
Function parameter or member 'capabilities' not described in
'v4l2_create_buffers32'
Signed-off-by: Hans Verkuil
---
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 0028e0be6b5b..f4325329fbd6 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -244,6 +244,7 @@ struct v4l2_format32 {
* return: number of created buffers
* @memory:buffer memory type
* @format:frame format, for which buffers are requested
+ * @capabilities: capabilities of this buffer type.
* @reserved: future extensions
*/
struct v4l2_create_buffers32 {
[PATCH for request_api branch] v4l2-ctrls.c: fix smatch error
Fix this smatch error:
drivers/media/v4l2-core/v4l2-ctrls.c:2971 v4l2_ctrl_request_clone() error:
uninitialized symbol 'err'.
Signed-off-by: Hans Verkuil
---
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 73665c7d7045..65e3cf838ac7 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2942,7 +2942,7 @@ static int v4l2_ctrl_request_clone(struct
v4l2_ctrl_handler *hdl,
const struct v4l2_ctrl_handler *from)
{
struct v4l2_ctrl_ref *ref;
- int err;
+ int err = 0;
if (WARN_ON(!hdl || hdl == from))
return -EINVAL;
Re: [PATCH v3 1/2] media: ov5640: Re-work MIPI startup sequence
Hello Loic,
On Thu, Sep 06, 2018 at 10:13:53AM +0200, Loic Poulain wrote:
> On 6 September 2018 at 09:48, jacopo mondi wrote:
> > Hello Loic,
> >thanks for looking into this
> >
> > On Tue, Sep 04, 2018 at 07:22:50PM +0200, Loic Poulain wrote:
> >> Hi Jacopo,
> >>
> >> > - ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
> >> > -on ? 0 : BIT(5));
> >> > - if (ret)
> >> > - return ret;
> >> > - ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
> >> > - on ? 0x00 : 0x70);
> >> > + /*
> >> > +* Enable/disable the MIPI interface
> >> > +*
> >> > +* 0x300e = on ? 0x45 : 0x40
> >> > +* [7:5] = 001 : 2 data lanes mode
> >>
> >> Does 2-Lanes work with this config?
> >> AFAIU, if 2-Lanes is bit 5, value should be 0x25 and 0x20.
> >>
> >
> > Yes, confusing.
> >
> > The sensor manual reports
> > 0x300e[7:5] = 000 one lane mode
> > 0x300e[7:5] = 001 two lanes mode
> >
> > Although this configuration works with 2 lanes, and the application
> > note I have, with the suggested settings for MIPI CSI-2 2 lanes
> > reports 0x40 to be the 2 lanes mode...
> >
> > I used that one, also because the removed entry from the settings blob
> > is:
> > - {0x300e, 0x45, 0, 0}, {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> > + {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> >
> > So it was using BIT(6) already.
>
> Yes, it was setting BIT(6) from static config and BIT(5) from the
> ov5640_set_stream_mipi function. In your patch you don't set
> BIT(5) anymore.
I've resumed looking into this series.
Just FYI, the snippet you refer to is:
- ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
-on ? 0 : BIT(5));
- if (ret)
- return ret;
- ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
- on ? 0x00 : 0x70);
+ /*
+* Enable/disable the MIPI interface
+*
+* 0x300e = on ? 0x45 : 0x40
+* [7:5] = 001 : 2 data lanes mode
+* [4] = 0 : Power up MIPI HS Tx
+* [3] = 0 : Power up MIPI LS Rx
+* [2] = 1/0: MIPI interface enable/disable
+* [1:0] = 01/00: FIXME: 'debug'
+*/
+ ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
+ on ? 0x45 : 0x40)
As you can see (it took me a while) the old code was indeed setting
BIT(5) as you mentioned, but on OV5640_REG_MIPI_CTRL00 (0x4800) and
not on OV5640_REG_IO_MIPI_CTRL00 (0x300e) as mine does. So the lane
configuration mode was set to 0x45 and never changed later.
>
> So it's not clear to me why it is still working, and the datasheet does
> not help a lot on this (BIT(6) is for debug modes).
> FYI I tried with BIT(5) only but it does not work (though I did not
> investigate a lot).
I'll keep BIT(6) set, point out the discrepancy with the datasheet,
and point out it has been tested with 2 lanes, until someone can
confirm it works with 1 lane too.
Thanks
j
>
> > Anyway, it works for me with 2 lanes (and I assume Steve), you have tested
> > too, with how many lanes are you working with?
> >
> > Anyway, a comment there might be nice to have... Will add in next
> > version
>
> Definitely.
>
> Regards,
> Loic
signature.asc
Description: PGP signature
Re: [PATCH v2 0/7] TVP5150 fixes and new features
Em Fri, 14 Sep 2018 10:43:03 +0200
Marco Felsch escreveu:
> Hi,
>
> since I sent this series I only got feedback from Rob.
I'm doing some tests on it. If everything gets ok, I'll likely merge
it today.
>
> Regards,
> Marco
>
> On 18-08-13 11:25, Marco Felsch wrote:
> > Hi,
> >
> > this is my v2 with the integrated reviews from my v1 [1]. Since Mauro
> > applied the most patches from my v1 to his experimental/tvp5150-3
> > branch [2], this series only contains those which aren't applied.
> >
> > Patches I changed contain a changelog, so I will omit these here.
> >
> > Patch ('[media] tvp5150: add FORMAT_TRY support for get/set selection
> > handlers') throws a compile error. Therefore I added two v4l2-subdev.h
> > patches which should fix the error in a common way.
> >
> > Patch ('[media] tvp5150: add s_power callback') is new too. I forget
> > them in my v1. This patch address the interrupt enable/disable handling.
> > Now it is possible to pause streaming and keep the interrupts on.
> >
> > The changes I made in the ('[media] tvp5150: add input source selection
> > of_graph support') patch are based on the the RFC [3] and discussion [4].
> > I dropped patch ('[media] tvp5150: Change default input source selection
> > behaviour') since the default input source selectopn is setup during the
> > .registered() callback now.
> >
> > I've tested this series on a customer dt-based board. Unfortunately I
> > haven't a device which use the em28xx driver. So other tester a welcome :)
> >
> > [1] https://www.spinics.net/lists/devicetree/msg236650.html
> > [2] https://git.linuxtv.org/mchehab/experimental.git/log/?h=tvp5150-3
> > [3] https://www.spinics.net/lists/devicetree/msg243181.html
> > [4] https://www.spinics.net/lists/devicetree/msg243840.html
> >
> > Regards,
> > Marco
> >
> > Marco Felsch (6):
> > [media] tvp5150: add input source selection of_graph support
> > [media] dt-bindings: tvp5150: Add input port connectors DT bindings
> > [media] v4l2-subdev: add stubs for v4l2_subdev_get_try_*
> > [media] v4l2-subdev: fix v4l2_subdev_get_try_* dependency
> > [media] tvp5150: add FORMAT_TRY support for get/set selection handlers
> > [media] tvp5150: add s_power callback
> >
> > Michael Tretter (1):
> > [media] tvp5150: initialize subdev before parsing device tree
> >
> > .../devicetree/bindings/media/i2c/tvp5150.txt | 191 +-
> > drivers/media/i2c/tvp5150.c | 611 +++---
> > include/media/v4l2-subdev.h | 111 ++--
> > 3 files changed, 776 insertions(+), 137 deletions(-)
> >
> > --
> > 2.18.0
> >
> >
Thanks,
Mauro
Re: [PATCH] tvp5150: avoid going past array on v4l2_querymenu()
On 18-09-13 18:36, Mauro Carvalho Chehab wrote:
> The parameters of v4l2_ctrl_new_std_menu_items() are tricky: instead of
> the number of possible values, it requires the number of the maximum
> value. In other words, the ARRAY_SIZE() value should be decremented,
> otherwise it will go past the array bounds, as warned by KASAN:
>
> [ 279.839688] BUG: KASAN: global-out-of-bounds in v4l2_querymenu+0x10d/0x180
> [videodev]
> [ 279.839709] Read of size 8 at addr c10a4cb0 by task
> v4l2-compliance/16676
>
> [ 279.839736] CPU: 1 PID: 16676 Comm: v4l2-compliance Not tainted
> 4.18.0-rc2+ #120
> [ 279.839741] Hardware name: /NUC5i7RYB, BIOS
> RYBDWi35.86A.0364.2017.0511.0949 05/11/2017
> [ 279.839743] Call Trace:
> [ 279.839758] dump_stack+0x71/0xab
> [ 279.839807] ? v4l2_querymenu+0x10d/0x180 [videodev]
> [ 279.839817] print_address_description+0x1c9/0x270
> [ 279.839863] ? v4l2_querymenu+0x10d/0x180 [videodev]
> [ 279.839871] kasan_report+0x237/0x360
> [ 279.839918] v4l2_querymenu+0x10d/0x180 [videodev]
> [ 279.839964] __video_do_ioctl+0x2c8/0x590 [videodev]
> [ 279.840011] ? copy_overflow+0x20/0x20 [videodev]
> [ 279.840020] ? avc_ss_reset+0xa0/0xa0
> [ 279.840028] ? check_stack_object+0x21/0x60
> [ 279.840036] ? __check_object_size+0xe7/0x240
> [ 279.840080] video_usercopy+0xed/0x730 [videodev]
> [ 279.840123] ? copy_overflow+0x20/0x20 [videodev]
> [ 279.840167] ? v4l_enumstd+0x40/0x40 [videodev]
> [ 279.840177] ? __handle_mm_fault+0x9f9/0x1ba0
> [ 279.840186] ? __pmd_alloc+0x2c0/0x2c0
> [ 279.840193] ? __vfs_write+0xb6/0x350
> [ 279.840200] ? kernel_read+0xa0/0xa0
> [ 279.840244] ? video_usercopy+0x730/0x730 [videodev]
> [ 279.840284] v4l2_ioctl+0xa1/0xb0 [videodev]
> [ 279.840295] do_vfs_ioctl+0x117/0x8a0
> [ 279.840303] ? selinux_file_ioctl+0x211/0x2f0
> [ 279.840313] ? ioctl_preallocate+0x120/0x120
> [ 279.840319] ? selinux_capable+0x20/0x20
> [ 279.840332] ksys_ioctl+0x70/0x80
> [ 279.840342] __x64_sys_ioctl+0x3d/0x50
> [ 279.840351] do_syscall_64+0x6d/0x1c0
> [ 279.840361] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 279.840367] RIP: 0033:0x7fdfb46275d7
> [ 279.840369] Code: b3 66 90 48 8b 05 b1 48 2d 00 64 c7 00 26 00 00 00 48 c7
> c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d
> 01 f0 ff ff 73 01 c3 48 8b 0d 81 48 2d 00 f7 d8 64 89 01 48
> [ 279.840474] RSP: 002b:7ffee1179038 EFLAGS: 0202 ORIG_RAX:
> 0010
> [ 279.840483] RAX: ffda RBX: 7ffee1179180 RCX:
> 7fdfb46275d7
> [ 279.840488] RDX: 7ffee11790c0 RSI: c02c5625 RDI:
> 0003
> [ 279.840493] RBP: 0002 R08: 0020 R09:
> 009f0902
> [ 279.840497] R10: R11: 0202 R12:
> 7ffee117a5a0
> [ 279.840501] R13: 7ffee11790c0 R14: 0002 R15:
>
>
> [ 279.840515] The buggy address belongs to the variable:
> [ 279.840535] tvp5150_test_patterns+0x10/0xe360 [tvp5150]
>
> Signed-off-by: Mauro Carvalho Chehab
Should there a Fixes: tag and also a fix keyword in the patch title?
Regards,
Marco
> ---
> drivers/media/i2c/tvp5150.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index b5d44c25d1da..133073518744 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -1633,7 +1633,7 @@ static int tvp5150_probe(struct i2c_client *c,
> 2700, 1, 2700);
> v4l2_ctrl_new_std_menu_items(>hdl, _ctrl_ops,
>V4L2_CID_TEST_PATTERN,
> - ARRAY_SIZE(tvp5150_test_patterns),
> + ARRAY_SIZE(tvp5150_test_patterns) - 1,
>0, 0, tvp5150_test_patterns);
> sd->ctrl_handler = >hdl;
> if (core->hdl.error) {
> --
> 2.17.1
>
>
Re: [PATCH v2 0/7] TVP5150 fixes and new features
Hi,
since I sent this series I only got feedback from Rob.
Regards,
Marco
On 18-08-13 11:25, Marco Felsch wrote:
> Hi,
>
> this is my v2 with the integrated reviews from my v1 [1]. Since Mauro
> applied the most patches from my v1 to his experimental/tvp5150-3
> branch [2], this series only contains those which aren't applied.
>
> Patches I changed contain a changelog, so I will omit these here.
>
> Patch ('[media] tvp5150: add FORMAT_TRY support for get/set selection
> handlers') throws a compile error. Therefore I added two v4l2-subdev.h
> patches which should fix the error in a common way.
>
> Patch ('[media] tvp5150: add s_power callback') is new too. I forget
> them in my v1. This patch address the interrupt enable/disable handling.
> Now it is possible to pause streaming and keep the interrupts on.
>
> The changes I made in the ('[media] tvp5150: add input source selection
> of_graph support') patch are based on the the RFC [3] and discussion [4].
> I dropped patch ('[media] tvp5150: Change default input source selection
> behaviour') since the default input source selectopn is setup during the
> .registered() callback now.
>
> I've tested this series on a customer dt-based board. Unfortunately I
> haven't a device which use the em28xx driver. So other tester a welcome :)
>
> [1] https://www.spinics.net/lists/devicetree/msg236650.html
> [2] https://git.linuxtv.org/mchehab/experimental.git/log/?h=tvp5150-3
> [3] https://www.spinics.net/lists/devicetree/msg243181.html
> [4] https://www.spinics.net/lists/devicetree/msg243840.html
>
> Regards,
> Marco
>
> Marco Felsch (6):
> [media] tvp5150: add input source selection of_graph support
> [media] dt-bindings: tvp5150: Add input port connectors DT bindings
> [media] v4l2-subdev: add stubs for v4l2_subdev_get_try_*
> [media] v4l2-subdev: fix v4l2_subdev_get_try_* dependency
> [media] tvp5150: add FORMAT_TRY support for get/set selection handlers
> [media] tvp5150: add s_power callback
>
> Michael Tretter (1):
> [media] tvp5150: initialize subdev before parsing device tree
>
> .../devicetree/bindings/media/i2c/tvp5150.txt | 191 +-
> drivers/media/i2c/tvp5150.c | 611 +++---
> include/media/v4l2-subdev.h | 111 ++--
> 3 files changed, 776 insertions(+), 137 deletions(-)
>
> --
> 2.18.0
>
>
[RFP] Media Summit: Save/Restore controls from MTD
Industrial/Scientific sensors usually come with very extensive calibration information such as: per column gain, list of dead/pixels, temperature sensor offset... etc We are saving that information on an flash device that is located by the sensor. I would like some minutes (15 max) to show you how we are integrating that calibration flash with v4l2-ctrl. And if this feature is useful for someone else and upstream it. Thanks! -- Ricardo Ribalda
[PATCH] media: imx-pxp: fix compilation on i386 or x86_64
Include the missing interrupt.h header to fix compilation on i386 or
x86_64:
../drivers/media/platform/imx-pxp.c:988:1: error: unknown type name
'irqreturn_t'
static irqreturn_t pxp_irq_handler(int irq, void *dev_id)
^
../drivers/media/platform/imx-pxp.c: In function 'pxp_irq_handler':
../drivers/media/platform/imx-pxp.c:1012:9: error: 'IRQ_HANDLED' undeclared
(first use in this function)
return IRQ_HANDLED;
^
../drivers/media/platform/imx-pxp.c:1012:9: note: each undeclared identifier
is reported only once for each function it appears in
../drivers/media/platform/imx-pxp.c: In function 'pxp_probe':
../drivers/media/platform/imx-pxp.c:1660:2: error: implicit declaration of
function 'devm_request_threaded_irq' [-Werror=implicit-function-declaration]
ret = devm_request_threaded_irq(>dev, irq, NULL, pxp_irq_handler,
^
../drivers/media/platform/imx-pxp.c:1661:4: error: 'IRQF_ONESHOT' undeclared
(first use in this function)
IRQF_ONESHOT, dev_name(>dev), dev);
Fixes: 51abcf7fdb70 ("media: imx-pxp: add i.MX Pixel Pipeline driver")
Reported-by: Randy Dunlap
Signed-off-by: Philipp Zabel
---
drivers/media/platform/imx-pxp.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/media/platform/imx-pxp.c b/drivers/media/platform/imx-pxp.c
index 68ecfed7098b..229c23ae4029 100644
--- a/drivers/media/platform/imx-pxp.c
+++ b/drivers/media/platform/imx-pxp.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
#include
--
2.19.0
Re: RFC: stop support for 2.6 kernel in the daily build
On 09/13/2018 11:32 PM, Jasmin J. wrote: > Hello Hans! > >> I'm inclined to drop support for 2.6 altogether. > RHEL 6 is on kernel 2.6.32, which we do not support since long time. > Most other distributions switched to 3.x also years in the past. > So lets drop 2.6.x then! OK, I'll do that in the next few days. > As you know I maintain a media-tree DKMS package and this should work for > older > Kernels. I personally have a VDR based on Ubuntu 14.04 system with the > original > 3.13 Kernel. So this is the minimum version for me. > > When you want to support RHEL 7, then the version goes down to 3.10. > >> Whether we should also drop support for 3.0-3.9 is another matter. > We can decide now to remove those versions also and wait if people are > complaining. At least this is what I would do. > > I would suggest to remove all kernels prior to 3.10 from your build system and > wait what people will say over the next months. I'll wait a few days if I get any other comments about this, and if not I will kill 3.0-3.9. Regards, Hans
