cron job: media_tree daily build: OK
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: Thu Jul 19 05:00:11 CEST 2018 media-tree git hash:39fbb88165b2bbbc77ea7acab5f10632a31526e6 media_build git hash: f3b64e45d2f2ef45cd4ae5b90a8f2a4fb284e43c v4l-utils git hash: e4df0e3cd3a84570714defe279d13eae894cb1fa edid-decode git hash: ab18befbcacd6cd4dff63faa82e32700369d6f25 gcc version:i686-linux-gcc (GCC) 8.1.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.16.0-1-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK Check COMPILE_TEST: OK linux-2.6.36.4-i686: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-i686: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-i686: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-i686: OK linux-2.6.39.4-x86_64: OK linux-3.0.101-i686: OK linux-3.0.101-x86_64: OK linux-3.1.10-i686: OK linux-3.1.10-x86_64: OK linux-3.2.102-i686: OK linux-3.2.102-x86_64: OK linux-3.3.8-i686: OK linux-3.3.8-x86_64: OK linux-3.4.113-i686: OK linux-3.4.113-x86_64: OK linux-3.5.7-i686: OK linux-3.5.7-x86_64: OK linux-3.6.11-i686: OK linux-3.6.11-x86_64: OK linux-3.7.10-i686: OK linux-3.7.10-x86_64: OK linux-3.8.13-i686: OK linux-3.8.13-x86_64: OK linux-3.9.11-i686: OK linux-3.9.11-x86_64: OK linux-3.10.108-i686: OK linux-3.10.108-x86_64: OK linux-3.11.10-i686: OK linux-3.11.10-x86_64: OK linux-3.12.74-i686: OK linux-3.12.74-x86_64: OK linux-3.13.11-i686: OK linux-3.13.11-x86_64: OK linux-3.14.79-i686: OK linux-3.14.79-x86_64: OK linux-3.15.10-i686: OK linux-3.15.10-x86_64: OK linux-3.16.57-i686: OK linux-3.16.57-x86_64: OK linux-3.17.8-i686: OK linux-3.17.8-x86_64: OK linux-3.18.115-i686: OK linux-3.18.115-x86_64: OK linux-3.19.8-i686: OK linux-3.19.8-x86_64: OK linux-4.0.9-i686: OK linux-4.0.9-x86_64: OK linux-4.1.52-i686: OK linux-4.1.52-x86_64: OK linux-4.2.8-i686: OK linux-4.2.8-x86_64: OK linux-4.3.6-i686: OK linux-4.3.6-x86_64: OK linux-4.4.140-i686: OK linux-4.4.140-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.112-i686: OK linux-4.9.112-x86_64: OK linux-4.10.17-i686: OK linux-4.10.17-x86_64: OK linux-4.11.12-i686: OK linux-4.11.12-x86_64: OK linux-4.12.14-i686: OK linux-4.12.14-x86_64: OK linux-4.13.16-i686: OK linux-4.13.16-x86_64: OK linux-4.14.55-i686: OK linux-4.14.55-x86_64: OK linux-4.15.18-i686: OK linux-4.15.18-x86_64: OK linux-4.16.18-i686: OK linux-4.16.18-x86_64: OK linux-4.17.6-i686: OK linux-4.17.6-x86_64: OK linux-4.18-rc4-i686: OK linux-4.18-rc4-x86_64: OK apps: OK spec-git: OK Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
the editing for your photos
Do you have need for image editing? We have 20 image editors and on daily basis 2000 images can be processed. If you want to check our quality of work please send us a photo with instruction and we will work on it. Photo cut out, masking, clipping path Color, brightness and contrast correction Beauty, Model retouching, skin retouching Image cropping and resizing We do unlimited revisions until you are satisfied with the work. Thanks, Ruby
Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI
Hi Mauro, Hans, On Fri, Jun 15, 2018 at 12:30 PM Yong Zhi wrote: > > These meta formats are used on Intel IPU3 ImgU video queues > to carry 3A statistics and ISP pipeline parameters. > > V4L2_META_FMT_IPU3_3A > V4L2_META_FMT_IPU3_PARAMS > > Signed-off-by: Yong Zhi > Signed-off-by: Chao C Li > Signed-off-by: Rajmohan Mani > --- > Documentation/media/uapi/v4l/meta-formats.rst |1 + > .../media/uapi/v4l/pixfmt-meta-intel-ipu3.rst | 174 ++ > include/uapi/linux/intel-ipu3.h| 2816 > The documentation seems to be quite extensive in current version. Do you think it's more acceptable now? Would you be able to take a look? We obviously need to keep working on the user space framework (and we're in process of figuring out how we can proceed further), but having the driver bit-rotting downstream might not be a very encouraging factor. ;) Best regards, Tomasz
Re: [PATCH v1 1/2] doc-rst: Add Intel IPU3 documentation
Hi Yong,
On Thu, Jun 14, 2018 at 10:29:32PM -0500, Yong Zhi wrote:
> From: Rajmohan Mani
>
> This patch adds the details about the IPU3 Imaging Unit driver.
>
> Signed-off-by: Rajmohan Mani
> Signed-off-by: Tian Shu Qiu
> ---
> Documentation/media/v4l-drivers/index.rst | 1 +
> Documentation/media/v4l-drivers/ipu3.rst | 304
> ++
> 2 files changed, 305 insertions(+)
> create mode 100644 Documentation/media/v4l-drivers/ipu3.rst
>
> diff --git a/Documentation/media/v4l-drivers/index.rst
> b/Documentation/media/v4l-drivers/index.rst
> index 679238e..179a393 100644
> --- a/Documentation/media/v4l-drivers/index.rst
> +++ b/Documentation/media/v4l-drivers/index.rst
> @@ -44,6 +44,7 @@ For more details see the file COPYING in the source
> distribution of Linux.
> davinci-vpbe
> fimc
> imx
> + ipu3
> ivtv
> max2175
> meye
> diff --git a/Documentation/media/v4l-drivers/ipu3.rst
> b/Documentation/media/v4l-drivers/ipu3.rst
> new file mode 100644
> index 000..a4550d8
> --- /dev/null
> +++ b/Documentation/media/v4l-drivers/ipu3.rst
> @@ -0,0 +1,304 @@
> +.. include::
> +
> +===
> +Intel Image Processing Unit 3 (IPU3) Imaging Unit (ImgU) driver
> +===
> +
> +Copyright |copy| 2018 Intel Corporation
> +
> +Introduction
> +
> +
> +This file documents Intel IPU3 (3rd generation Image Processing Unit) Imaging
> +Unit driver located under drivers/media/pci/intel/ipu3.
> +
> +The Intel IPU3 found in certain Kaby Lake (as well as certain Sky Lake)
> +platforms (U/Y processor lines) is made up of two parts namely Imaging Unit
> +(ImgU) and CIO2 device (MIPI CSI2 receiver).
> +
> +The CIO2 device receives the raw bayer data from the sensors and outputs the
> +frames in a format that is specific to IPU3 (for consumption by IPU3 ImgU).
> +CIO2 driver is available as drivers/media/pci/intel/ipu3/ipu3-cio2* and is
> +enabled through the CONFIG_VIDEO_IPU3_CIO2 config option.
> +
> +The Imaging Unit (ImgU) is responsible for processing images captured
> +through IPU3 CIO2 device. The ImgU driver sources can be found under
> +drivers/media/pci/intel/ipu3 directory. The driver is enabled through the
> +CONFIG_VIDEO_IPU3_IMGU config option.
> +
> +The two driver modules are named ipu3-csi2 and ipu3-imgu, respectively.
> +
> +The driver has been tested on Kaby Lake platforms (U/Y processor lines).
> +
> +The driver implements V4L2, Media controller and V4L2 sub-device interfaces.
> +Camera sensors that have CSI-2 bus, which are connected to the IPU3 CIO2
> +device are supported. Support for lens and flash drivers depends on the
> +above sensors.
> +
> +ImgU device nodes
> +=
> +
> +The ImgU is represented as a single V4L2 subdev, which provides a V4L2 subdev
> +interface to the user space.
> +
> +CIO2 device
> +===
> +
> +The CIO2 is represented as a single V4L2 subdev, which provides a V4L2 subdev
> +interface to the user space. There is a video node for each CSI-2 receiver,
> +with a single media controller interface for the entire device.
> +
> +Media controller
> +
> +
> +The media device interface allows to configure the ImgU links, which defines
> +the behavior of the IPU3 firmware. The link configuration tells the firmware
> +whether viewfinder or postview ISP pipeline should be enabled.
> +
> +Device operation
> +
> +
> +With IPU3, once the input video node ("ipu3-imgu":0, in :
> +format) is queued with buffer (in packed raw bayer format), IPU3 ISP starts
> +processing the buffer and produces the video output in YUV format and
> +statistics output on respective output nodes. The driver is expected to have
> +buffers ready for all of parameter, output and statistics nodes, when input
> +video node is queued with buffer.
> +
> +At a minimum, all of input, main output, 3A statistics, and either of
> +viewfinder or postview video nodes should be enabled for IPU3 to start image
> +processing. viewfinder and postview video nodes are mutually exclusive.
> +
> +input, output, viewfinder and postview video nodes
> +--
> +
> +The frames (in packed raw bayer format specific to IPU3) received by the
> +input video node is processed by the IPU3 Imaging Unit and is output to 2
> +video nodes, with each targeting different purpose (main output and
> viewfinder
> +or postview output).
> +
> +Details on raw bayer format specific to IPU3 can be found as below.
> +Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> +
> +The driver supports V4L2 Video Capture Interface as defined at
> :ref:`devices`.
> +
> +Only the multi-planar API is supported. More details can be found at
> +:ref:`planar-apis`.
> +
> +
> +parameters video node
> +-
> +
> +The parameter video node receives the ISP algorithm parameters
Compliment of the day to you Dear Friend.
Compliment of the day to you Dear Friend. Dear Friend. I am Mrs. Amina Kadi. am sending this brief letter to solicit your partnership to transfer $5.5 million US Dollars. I shall send you more information and procedures when I receive positive response from you. Mrs. Amina Kadi
Re: [PATCH 2/2] media: ov5640: Fix auto-exposure disabling
Hi again,
On Wed, Jul 18, 2018 at 01:19:03PM +0200, Jacopo Mondi wrote:
> As of:
> commit bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at
> start time") auto-exposure got disabled before programming new capture modes
> to
> the sensor. Unfortunately the function used to do that (ov5640_set_exposure())
> does not enable/disable auto-exposure engine through register 0x3503[0] bit,
> but
> programs registers [0x3500 - 0x3502] which represent the desired exposure time
> when running with manual exposure. As a result, auto-exposure was not actually
> disabled at all.
>
> To actually disable auto-exposure, go through the control framework instead of
> calling ov5640_set_exposure() function directly.
>
> Also, as auto-gain and auto-exposure are disabled un-conditionally but only
> restored to their previous values in ov5640_set_mode_direct() function, move
> controls restoring so that their value is re-programmed opportunely after
> either ov5640_set_mode_direct() or ov5640_set_mode_exposure_calc() have been
> executed.
>
> Fixes: bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at
> start time")
> Signed-off-by: Jacopo Mondi
>
> ---
> Is it worth doing with auto-gain what we're doing with auto-exposure? Cache
> the
> value and then re-program it instead of unconditionally disable/enable it?
I have missed this patch from Hugues that address almost the same
issue
https://www.mail-archive.com/linux-media@vger.kernel.org/msg133264.html
I feel this new one is simpler, and unless we want to avoid going
through the control framework, it is not worth adding new functions to
handle auto-exposure as Hugues' patch is doing.
Hugues, do you have comments? Feel free to add your sob or rb tags if
you like to.
Thanks
j
>
> Thanks
> j
> ---
> ---
> drivers/media/i2c/ov5640.c | 29 +
> 1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 12b3496..bc75cb7 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1588,25 +1588,13 @@ static int ov5640_set_mode_exposure_calc(struct
> ov5640_dev *sensor,
> * change mode directly
> */
> static int ov5640_set_mode_direct(struct ov5640_dev *sensor,
> - const struct ov5640_mode_info *mode,
> - s32 exposure)
> + const struct ov5640_mode_info *mode)
> {
> - int ret;
> -
> if (!mode->reg_data)
> return -EINVAL;
>
> /* Write capture setting */
> - ret = ov5640_load_regs(sensor, mode);
> - if (ret < 0)
> - return ret;
> -
> - /* turn auto gain/exposure back on for direct mode */
> - ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
> - if (ret)
> - return ret;
> -
> - return __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure);
> + return ov5640_load_regs(sensor, mode);
> }
>
> static int ov5640_set_mode(struct ov5640_dev *sensor,
> @@ -1626,7 +1614,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
> return ret;
>
> exposure = sensor->ctrls.auto_exp->val;
> - ret = ov5640_set_exposure(sensor, V4L2_EXPOSURE_MANUAL);
> + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, V4L2_EXPOSURE_MANUAL);
> if (ret)
> return ret;
>
> @@ -1642,12 +1630,21 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
>* change inside subsampling or scaling
>* download firmware directly
>*/
> - ret = ov5640_set_mode_direct(sensor, mode, exposure);
> + ret = ov5640_set_mode_direct(sensor, mode);
> }
>
> if (ret < 0)
> return ret;
>
> + /* Restore auto-gain and auto-exposure after mode has changed. */
> + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
> + if (ret)
> + return ret;
> +
> + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure)
> + if (ret)
> + return ret;
> +
> ret = ov5640_set_binning(sensor, dn_mode != SCALING);
> if (ret < 0)
> return ret;
> --
> 2.7.4
>
signature.asc
Description: PGP signature
Re: Devices with a front and back webcam represented as a single UVC device
Hi,
On 18-07-18 13:53, Carlos Garnacho wrote:
Hey,
On Wed, Jul 11, 2018 at 9:51 PM, Hans de Goede wrote:
Hi,
On 11-07-18 20:26, Carlos Garnacho wrote:
Hi!,
On Wed, Jul 11, 2018 at 7:41 PM, Hans de Goede
wrote:
Hi,
On 11-07-18 18:07, Carlos Garnacho wrote:
Hi!,
On Wed, Jul 11, 2018 at 2:41 PM, Hans de Goede
wrote:
HI,
On 11-07-18 14:08, Laurent Pinchart wrote:
Hi Carlos,
On Wednesday, 11 July 2018 14:36:48 EEST Carlos Garnacho wrote:
On Wed, Jul 11, 2018 at 1:00 PM, Laurent Pinchart wrote:
On Wednesday, 11 July 2018 11:37:14 EEST Hans de Goede wrote:
Hi Laurent,
At Guadec Carlos (in the Cc) told me that on his Acer 2-in-1 only
the frontcam is working and it seems both are represented by a
single UVC USB device. I've told him to check for some v4l control
to flip between front and back.
Carlos, as I mentioned you can try gtk-v4l
("sudo dnf install gtk-v4l") or qv4l2
("sudo dnf install qv4l2") these will both show
you various controls for the camera. One of those might do the
trick.
But I recently bought a 2nd second hand Cherry Trail based HP
X2 2-in-1 and much to my surprise that is actually using an UVC
cam, rather then the usual ATOMISP crap and it has the same issue.
This device does not seem to have a control to flip between the
2 cams, instead it registers 2 /dev/video? nodes but the second
node does not work
The second node is there to expose metadata to userspace, not image
data.
That's a recent addition to the uvcvideo driver.
and dmesg contains:
[ 26.079868] uvcvideo: Found UVC 1.00 device HP TrueVision HD
(05c8:03a3)
[ 26.095485] uvcvideo 1-4.2:1.0: Entity type for entity Extension
4
was
not initialized!
[ 26.095492] uvcvideo 1-4.2:1.0: Entity type for entity
Processing
2
was
not initialized!
[ 26.095496] uvcvideo 1-4.2:1.0: Entity type for entity Camera 1
was
not
initialized!
You can safely ignore those messages. I need to submit a patch to
get
rid
of them.
Laurent, I've attached lsusb -v output so that you can check the
descriptors.
Thank you.
It's funny how UVC specifies a standard way to describe a device
with
two
camera sensors with dynamic selection of one of them at runtime, and
vendors instead implement vendor-specific crap :-(
The interesting part in the descriptors is
VideoControl Interface Descriptor:
bLength27
bDescriptorType36
bDescriptorSubtype 6 (EXTENSION_UNIT)
bUnitID 4
guidExtensionCode
{1229a78c-47b4-4094-b0ce-db07386fb938}
bNumControl 2
bNrPins 1
baSourceID( 0) 2
bControlSize2
bmControls( 0) 0x00
bmControls( 1) 0x06
iExtension 0
The extension unit exposes two controls (bmControls is a bitmask).
They
can be accessed from userspace through the UVCIOC_CTRL_QUERY ioctl,
or
mapped to V4L2 controls through the UVCIOC_CTRL_MAP ioctl, in which
case
they will be exposed to standard V4L2 applications.
If you want to experiment with this, I would advise querying both
controls
with UVCIOC_CTRL_QUERY. You can use the UVC_GET_CUR, UVC_GET_MIN,
UVC_GET_MAX, UVC_GET_DEF and UVC_GET_RES requests to get the control
current, minimum, maximum, default and resolution values, and
UVC_GET_LEN
and UVC_GET_INFO to get the control size (in bytes) and flags. Based
on
that you can start experimenting with UVC_SET_CUR to set semi-random
values.
I'm however worried that those two controls would be a register
address
and a register value, for indirect access to all hardware registers
in
the device. In that case, you would likely need information from the
device vendor, or possibly a USB traffic dump from a Windows machine
when
switching between the front and back cameras.
Carlos, it might be good to get Laurent your descriptors too, to do
this do "lsusb", note what is the : for your camera and
then
run:
sudo lsusb -v -d : > lsusb.log
And send Laurent a mail with the generated lsusb
That would be appreciated, but I expect the same issue :-(
Please find it attached. IIUC your last email, it might not be the
exact same issue, but you can definitely judge better.
Your device is similar in the sense that it doesn't use the standard
UVC
support for multiple camera sensors. It instead exposes two extension
units:
VideoControl Interface Descriptor:
bLength27
bDescriptorType36
bDescriptorSubtype 6 (EXTENSION_UNIT)
bUnitID 4
guidExtensionCode
{1229a78c-47b4-4094-b0ce-db07386fb938}
bNumControl 2
bNrPins 1
baSourceID( 0) 2
bControlSize2
bmControls( 0) 0x00
bmControls( 1) 0x06
iExtension
Re: [PATCH 1/2] media: ov5640: Fix timings setup code
On Wed, Jul 18, 2018 at 01:19:02PM +0200, Jacopo Mondi wrote:
> 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")
> Signed-off-by: Samuel Bobrowicz
> Signed-off-by: Maxime Ripard
> Signed-off-by: Jacopo Mondi
>
> ---
> This fix has been circulating around for quite some time now, in Maxime clock
> tree patches, in Sam dropbox patches and in my latest MIPI fixes patches.
> While the rest of the series have not yet been accepted, there is general
> consensus this is an actual fix that has to be collected.
>
> I've slightly modified Sam's and Maxime's version I previously sent,
> programming timings directly in ov5640_load_regs() function.
> You can find Sam's previous version here:
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg131654.html
> and mine here, with an additional change that aimed to fix MIPI mode, which
> I've left out in this version:
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg133422.html
>
> Sam, Maxime, I took the liberty of taking your Signed-off-by from the previous
> patch, as this was spotted by you first. Is this ok with you?
Yep, thanks!
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
Re: Devices with a front and back webcam represented as a single UVC device
Hey,
On Wed, Jul 11, 2018 at 9:51 PM, Hans de Goede wrote:
> Hi,
>
>
> On 11-07-18 20:26, Carlos Garnacho wrote:
>>
>> Hi!,
>>
>> On Wed, Jul 11, 2018 at 7:41 PM, Hans de Goede
>> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 11-07-18 18:07, Carlos Garnacho wrote:
Hi!,
On Wed, Jul 11, 2018 at 2:41 PM, Hans de Goede
wrote:
>
>
> HI,
>
>
> On 11-07-18 14:08, Laurent Pinchart wrote:
>>
>>
>>
>> Hi Carlos,
>>
>> On Wednesday, 11 July 2018 14:36:48 EEST Carlos Garnacho wrote:
>>>
>>>
>>>
>>> On Wed, Jul 11, 2018 at 1:00 PM, Laurent Pinchart wrote:
On Wednesday, 11 July 2018 11:37:14 EEST Hans de Goede wrote:
>
>
>
> Hi Laurent,
>
> At Guadec Carlos (in the Cc) told me that on his Acer 2-in-1 only
> the frontcam is working and it seems both are represented by a
> single UVC USB device. I've told him to check for some v4l control
> to flip between front and back.
>
> Carlos, as I mentioned you can try gtk-v4l
> ("sudo dnf install gtk-v4l") or qv4l2
> ("sudo dnf install qv4l2") these will both show
> you various controls for the camera. One of those might do the
> trick.
>
> But I recently bought a 2nd second hand Cherry Trail based HP
> X2 2-in-1 and much to my surprise that is actually using an UVC
> cam, rather then the usual ATOMISP crap and it has the same issue.
>
> This device does not seem to have a control to flip between the
> 2 cams, instead it registers 2 /dev/video? nodes but the second
> node does not work
The second node is there to expose metadata to userspace, not image
data.
That's a recent addition to the uvcvideo driver.
> and dmesg contains:
>
> [ 26.079868] uvcvideo: Found UVC 1.00 device HP TrueVision HD
> (05c8:03a3)
> [ 26.095485] uvcvideo 1-4.2:1.0: Entity type for entity Extension
> 4
> was
> not initialized!
> [ 26.095492] uvcvideo 1-4.2:1.0: Entity type for entity
> Processing
> 2
> was
> not initialized!
> [ 26.095496] uvcvideo 1-4.2:1.0: Entity type for entity Camera 1
> was
> not
> initialized!
You can safely ignore those messages. I need to submit a patch to
get
rid
of them.
> Laurent, I've attached lsusb -v output so that you can check the
> descriptors.
Thank you.
It's funny how UVC specifies a standard way to describe a device
with
two
camera sensors with dynamic selection of one of them at runtime, and
vendors instead implement vendor-specific crap :-(
The interesting part in the descriptors is
VideoControl Interface Descriptor:
bLength27
bDescriptorType36
bDescriptorSubtype 6 (EXTENSION_UNIT)
bUnitID 4
guidExtensionCode
{1229a78c-47b4-4094-b0ce-db07386fb938}
bNumControl 2
bNrPins 1
baSourceID( 0) 2
bControlSize2
bmControls( 0) 0x00
bmControls( 1) 0x06
iExtension 0
The extension unit exposes two controls (bmControls is a bitmask).
They
can be accessed from userspace through the UVCIOC_CTRL_QUERY ioctl,
or
mapped to V4L2 controls through the UVCIOC_CTRL_MAP ioctl, in which
case
they will be exposed to standard V4L2 applications.
If you want to experiment with this, I would advise querying both
controls
with UVCIOC_CTRL_QUERY. You can use the UVC_GET_CUR, UVC_GET_MIN,
UVC_GET_MAX, UVC_GET_DEF and UVC_GET_RES requests to get the control
current, minimum, maximum, default and resolution values, and
UVC_GET_LEN
and UVC_GET_INFO to get the control size (in bytes) and flags. Based
on
that you can start experimenting with UVC_SET_CUR to set semi-random
values.
I'm however worried that those two controls would be a register
address
and a register value, for indirect access to all hardware registers
in
the device. In that case, you would likely need information from the
device vendor, or possibly a USB traffic dump from a Windows machine
[PATCH 0/2] media: ov5640: Fix set_timings and auto-exposure
Hello, the ov5640 driver has received a lot of attentions recently. Maxime and Sam tackled the clock tree handling and I tried to fix the MIPI capture on i.MX6 platforms, but none of those patches ever made it to inclusion. Although a few fixes have been circulating around those series, and it's my opinion it is worth including them before any other developments takes place. I'm sending here a re-work of a patch from Sam and Maxime to fix timings setup, and a small fix for auto-exposure enabling/disabling operations. Each patch has a comment, on which I would like to have some feedback on. [1/1] has already received several acks on the mailing list, but never a formal Reviewed-by or Tested-by, while [2/2] is new. Thanks j Jacopo Mondi (2): media: ov5640: Fix timings setup code media: ov5640: Fix auto-exposure disabling drivers/media/i2c/ov5640.c | 75 -- 1 file changed, 32 insertions(+), 43 deletions(-) -- 2.7.4
[PATCH 2/2] media: ov5640: Fix auto-exposure disabling
As of:
commit bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at
start time") auto-exposure got disabled before programming new capture modes to
the sensor. Unfortunately the function used to do that (ov5640_set_exposure())
does not enable/disable auto-exposure engine through register 0x3503[0] bit, but
programs registers [0x3500 - 0x3502] which represent the desired exposure time
when running with manual exposure. As a result, auto-exposure was not actually
disabled at all.
To actually disable auto-exposure, go through the control framework instead of
calling ov5640_set_exposure() function directly.
Also, as auto-gain and auto-exposure are disabled un-conditionally but only
restored to their previous values in ov5640_set_mode_direct() function, move
controls restoring so that their value is re-programmed opportunely after
either ov5640_set_mode_direct() or ov5640_set_mode_exposure_calc() have been
executed.
Fixes: bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at
start time")
Signed-off-by: Jacopo Mondi
---
Is it worth doing with auto-gain what we're doing with auto-exposure? Cache the
value and then re-program it instead of unconditionally disable/enable it?
Thanks
j
---
---
drivers/media/i2c/ov5640.c | 29 +
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 12b3496..bc75cb7 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1588,25 +1588,13 @@ static int ov5640_set_mode_exposure_calc(struct
ov5640_dev *sensor,
* change mode directly
*/
static int ov5640_set_mode_direct(struct ov5640_dev *sensor,
- const struct ov5640_mode_info *mode,
- s32 exposure)
+ const struct ov5640_mode_info *mode)
{
- int ret;
-
if (!mode->reg_data)
return -EINVAL;
/* Write capture setting */
- ret = ov5640_load_regs(sensor, mode);
- if (ret < 0)
- return ret;
-
- /* turn auto gain/exposure back on for direct mode */
- ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
- if (ret)
- return ret;
-
- return __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure);
+ return ov5640_load_regs(sensor, mode);
}
static int ov5640_set_mode(struct ov5640_dev *sensor,
@@ -1626,7 +1614,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
return ret;
exposure = sensor->ctrls.auto_exp->val;
- ret = ov5640_set_exposure(sensor, V4L2_EXPOSURE_MANUAL);
+ ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, V4L2_EXPOSURE_MANUAL);
if (ret)
return ret;
@@ -1642,12 +1630,21 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
* change inside subsampling or scaling
* download firmware directly
*/
- ret = ov5640_set_mode_direct(sensor, mode, exposure);
+ ret = ov5640_set_mode_direct(sensor, mode);
}
if (ret < 0)
return ret;
+ /* Restore auto-gain and auto-exposure after mode has changed. */
+ ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
+ if (ret)
+ return ret;
+
+ ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure)
+ if (ret)
+ return ret;
+
ret = ov5640_set_binning(sensor, dn_mode != SCALING);
if (ret < 0)
return ret;
--
2.7.4
[PATCH 1/2] media: ov5640: Fix timings setup code
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")
Signed-off-by: Samuel Bobrowicz
Signed-off-by: Maxime Ripard
Signed-off-by: Jacopo Mondi
---
This fix has been circulating around for quite some time now, in Maxime clock
tree patches, in Sam dropbox patches and in my latest MIPI fixes patches.
While the rest of the series have not yet been accepted, there is general
consensus this is an actual fix that has to be collected.
I've slightly modified Sam's and Maxime's version I previously sent,
programming timings directly in ov5640_load_regs() function.
You can find Sam's previous version here:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg131654.html
and mine here, with an additional change that aimed to fix MIPI mode, which
I've left out in this version:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg133422.html
Sam, Maxime, I took the liberty of taking your Signed-off-by from the previous
patch, as this was spotted by you first. Is this ok with you?
Thanks
j
---
---
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 1ecbb7a..12b3496 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 */
@@ -1385,30 +1405,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)
@@ -1652,10 +1648,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
[GIT PULL FOR v4.19] Various fixes
Hi Mauro, Various fixes. Please note that I re-added the 'Add support for STD ioctls on subdev nodes' patch. It really is needed. Regards, Hans The following changes since commit 39fbb88165b2bbbc77ea7acab5f10632a31526e6: media: bpf: ensure bpf program is freed on detach (2018-07-13 11:07:29 -0400) are available in the Git repository at: git://linuxtv.org/hverkuil/media_tree.git for-v4.19m for you to fetch changes up to 38630cde092e3fd66e18fc8752d7dd6b32947ca7: media: staging: tegra-vde: Replace debug messages with trace points (2018-07-18 12:15:18 +0200) Dmitry Osipenko (1): media: staging: tegra-vde: Replace debug messages with trace points Ezequiel Garcia (3): rcar_jpu: Remove unrequired wait in .job_abort s5p-g2d: Remove unrequired wait in .job_abort mem2mem: Make .job_abort optional Hans Verkuil (1): videobuf2-core: check for q->error in vb2_core_qbuf() Jacopo Mondi (9): sh: defconfig: migor: Update defconfig sh: defconfig: migor: Enable CEU and sensor drivers sh: defconfig: ecovec: Update defconfig sh: defconfig: ecovec: Enable CEU and video drivers sh: defconfig: se7724: Update defconfig sh: defconfig: se7724: Enable CEU and sensor driver sh: defconfig: ap325rxa: Update defconfig sh: defconfig: ap325rxa: Enable CEU and sensor driver sh: migor: Remove stale soc_camera include Laurent Pinchart (1): v4l: rcar_fdp1: Enable compilation on Gen2 platforms Neil Armstrong (1): media: platform: meson-ao-cec: make busy TX warning silent Niklas Söderlund (1): v4l: Add support for STD ioctls on subdev nodes Philipp Zabel (1): media: video-mux: fix compliance failures Documentation/media/uapi/v4l/vidioc-enumstd.rst | 11 ++- Documentation/media/uapi/v4l/vidioc-g-std.rst| 14 +++- Documentation/media/uapi/v4l/vidioc-querystd.rst | 11 ++- arch/sh/boards/mach-migor/setup.c| 1 - arch/sh/configs/ap325rxa_defconfig | 29 ++- arch/sh/configs/ecovec24_defconfig | 35 ++--- arch/sh/configs/migor_defconfig | 31 ++-- arch/sh/configs/se7724_defconfig | 30 ++-- drivers/media/common/videobuf2/videobuf2-core.c | 5 ++ drivers/media/platform/Kconfig | 2 +- drivers/media/platform/meson/ao-cec.c| 2 +- drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 5 -- drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 5 -- drivers/media/platform/rcar_jpu.c| 16 drivers/media/platform/rockchip/rga/rga.c| 6 -- drivers/media/platform/s5p-g2d/g2d.c | 16 drivers/media/platform/s5p-g2d/g2d.h | 1 - drivers/media/platform/s5p-jpeg/jpeg-core.c | 7 -- drivers/media/platform/video-mux.c | 119 - drivers/media/v4l2-core/v4l2-mem2mem.c | 6 +- drivers/media/v4l2-core/v4l2-subdev.c| 22 ++ drivers/staging/media/tegra-vde/tegra-vde.c | 221 +++--- drivers/staging/media/tegra-vde/trace.h | 98 include/media/v4l2-mem2mem.h | 2 +- include/uapi/linux/v4l2-subdev.h | 4 + 25 files changed, 429 insertions(+), 270 deletions(-) create mode 100644 drivers/staging/media/tegra-vde/trace.h
Re: [PATCH 1/2] v4l2-core: Simplify v4l2_m2m_try_{schedule,run}
On 12/07/18 17:43, Ezequiel Garcia wrote:
> v4l2_m2m_try_run() has only one caller and so it's possible
> to move its contents.
>
> Although this de-modularization change could reduce clarity,
> in this case it allows to remove a spinlock lock/unlock pair
> and an unneeded sanity check.
>
> Signed-off-by: Ezequiel Garcia
This patch no longer applies, can you respin?
Regards,
Hans
> ---
> drivers/media/v4l2-core/v4l2-mem2mem.c | 44 ++
> 1 file changed, 10 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
> b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 6bce8dcd182a..c2e9c2b7dcd1 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -181,37 +181,6 @@ void *v4l2_m2m_get_curr_priv(struct v4l2_m2m_dev
> *m2m_dev)
> }
> EXPORT_SYMBOL(v4l2_m2m_get_curr_priv);
>
> -/**
> - * v4l2_m2m_try_run() - select next job to perform and run it if possible
> - * @m2m_dev: per-device context
> - *
> - * Get next transaction (if present) from the waiting jobs list and run it.
> - */
> -static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
> -{
> - unsigned long flags;
> -
> - spin_lock_irqsave(_dev->job_spinlock, flags);
> - if (NULL != m2m_dev->curr_ctx) {
> - spin_unlock_irqrestore(_dev->job_spinlock, flags);
> - dprintk("Another instance is running, won't run now\n");
> - return;
> - }
> -
> - if (list_empty(_dev->job_queue)) {
> - spin_unlock_irqrestore(_dev->job_spinlock, flags);
> - dprintk("No job pending\n");
> - return;
> - }
> -
> - m2m_dev->curr_ctx = list_first_entry(_dev->job_queue,
> -struct v4l2_m2m_ctx, queue);
> - m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
> - spin_unlock_irqrestore(_dev->job_spinlock, flags);
> -
> - m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
> -}
> -
> void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> {
> struct v4l2_m2m_dev *m2m_dev;
> @@ -269,15 +238,22 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> list_add_tail(_ctx->queue, _dev->job_queue);
> m2m_ctx->job_flags |= TRANS_QUEUED;
>
> - spin_unlock_irqrestore(_dev->job_spinlock, flags_job);
> + if (NULL != m2m_dev->curr_ctx) {
> + spin_unlock_irqrestore(_dev->job_spinlock, flags_job);
> + dprintk("Another instance is running, won't run now\n");
> + return;
> + }
>
> - v4l2_m2m_try_run(m2m_dev);
> + m2m_dev->curr_ctx = list_first_entry(_dev->job_queue,
> +struct v4l2_m2m_ctx, queue);
> + m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
> + spin_unlock_irqrestore(_dev->job_spinlock, flags_job);
>
> + m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
> return;
>
> out_unlock:
> spin_unlock_irqrestore(_dev->job_spinlock, flags_job);
> -
> return;
> }
> EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
>
Re: [PATCH 2/2] v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
On 12/07/18 17:43, Ezequiel Garcia wrote:
> v4l2_m2m_job_finish() is typically called in interrupt context.
>
> Some implementation of .device_run might sleep, and so it's
> desirable to avoid calling it directly from
> v4l2_m2m_job_finish(), thus avoiding .device_run from running
> in interrupt context.
>
> Implement a deferred context that gets scheduled by
> v4l2_m2m_job_finish().
>
> The worker calls v4l2_m2m_try_schedule(), which makes sure
> a single job is running at the same time, so it's safe to
> call it from different executions context.
I am not sure about this. I think that the only thing that needs to
run in the work queue is the call to device_run. Everything else
up to that point runs fine in interrupt context.
While we're on it: I see that v4l2_m2m_prepare_buf() also calls
v4l2_m2m_try_schedule(): I don't think it should do that since
prepare_buf does not actually queue a new buffer to the driver.
Regards,
Hans
>
> Signed-off-by: Ezequiel Garcia
> ---
> drivers/media/v4l2-core/v4l2-mem2mem.c | 14 +-
> include/media/v4l2-mem2mem.h | 2 ++
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
> b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index c2e9c2b7dcd1..1d0e20809ffe 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -258,6 +258,17 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> }
> EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
>
> +/**
> + * v4l2_m2m_try_schedule_work() - run pending jobs for the context
> + * @work: Work structure used for scheduling the execution of this function.
> + */
> +static void v4l2_m2m_try_schedule_work(struct work_struct *work)
> +{
> + struct v4l2_m2m_ctx *m2m_ctx =
> + container_of(work, struct v4l2_m2m_ctx, job_work);
> + v4l2_m2m_try_schedule(m2m_ctx);
> +}
> +
> /**
> * v4l2_m2m_cancel_job() - cancel pending jobs for the context
> * @m2m_ctx: m2m context with jobs to be canceled
> @@ -316,7 +327,7 @@ void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
> /* This instance might have more buffers ready, but since we do not
>* allow more than one job on the job_queue per instance, each has
>* to be scheduled separately after the previous one finishes. */
> - v4l2_m2m_try_schedule(m2m_ctx);
> + schedule_work(_ctx->job_work);
> }
> EXPORT_SYMBOL(v4l2_m2m_job_finish);
>
> @@ -614,6 +625,7 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct
> v4l2_m2m_dev *m2m_dev,
> m2m_ctx->priv = drv_priv;
> m2m_ctx->m2m_dev = m2m_dev;
> init_waitqueue_head(_ctx->finished);
> + INIT_WORK(_ctx->job_work, v4l2_m2m_try_schedule_work);
>
> out_q_ctx = _ctx->out_q_ctx;
> cap_q_ctx = _ctx->cap_q_ctx;
> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> index 3d07ba3a8262..2543fbfdd2b1 100644
> --- a/include/media/v4l2-mem2mem.h
> +++ b/include/media/v4l2-mem2mem.h
> @@ -89,6 +89,7 @@ struct v4l2_m2m_queue_ctx {
> * @job_flags: Job queue flags, used internally by v4l2-mem2mem.c:
> * %TRANS_QUEUED, %TRANS_RUNNING and %TRANS_ABORT.
> * @finished: Wait queue used to signalize when a job queue finished.
> + * @job_work: Worker to run queued jobs.
> * @priv: Instance private data
> *
> * The memory to memory context is specific to a file handle, NOT to e.g.
> @@ -109,6 +110,7 @@ struct v4l2_m2m_ctx {
> struct list_headqueue;
> unsigned long job_flags;
> wait_queue_head_t finished;
> + struct work_struct job_work;
>
> void*priv;
> };
>
Re: [PATCH] videobuf2-core: check for q->error in vb2_core_qbuf()
On 18/07/18 12:06, Sakari Ailus wrote:
> On Wed, Jul 18, 2018 at 11:29:01AM +0200, Hans Verkuil wrote:
>> On 16/07/18 14:49, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Thu, Jul 05, 2018 at 10:25:19AM +0200, Hans Verkuil wrote:
The vb2_core_qbuf() function didn't check if q->error was set. It is
checked in
__buf_prepare(), but that function isn't called if the buffer was already
prepared before with VIDIOC_PREPARE_BUF.
So check it at the start of vb2_core_qbuf() as well.
Signed-off-by: Hans Verkuil
---
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
b/drivers/media/common/videobuf2/videobuf2-core.c
index d3501cd604cb..5d7946ec80d8 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1484,6 +1484,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
index, void *pb,
struct vb2_buffer *vb;
int ret;
+ if (q->error) {
+ dprintk(1, "fatal error occurred on queue\n");
+ return -EIO;
+ }
+
vb = q->bufs[index];
if ((req && q->uses_qbuf) ||
>>>
>>> How long has this problem existed? It looks like something that should go
>>> to the stable branches, too...
>>
>> It's always been there, but I don't think it is worth backporting. The use of
>> VIDIOC_PREPARE_BUF is very rare, let alone the combination with
>> vb2_queue_error().
>>
>> I came across it while reviewing code.
>
> What's the effect of the missing check? That the user may queue a buffer
> when the driver thinks the hardware won't be able to complete it? At least
> that doesn't seem like a security issue.
Right. But e.g. dqbuf will still return EIO in this case, so normally apps
will discover this error condition when dequeueing and not when enqueueing
buffers.
>
> Anyway,
>
> Acked-by: Sakari Ailus
>
Thanks,
Hans
Re: [PATCH] videobuf2-core: check for q->error in vb2_core_qbuf()
On Wed, Jul 18, 2018 at 11:29:01AM +0200, Hans Verkuil wrote:
> On 16/07/18 14:49, Sakari Ailus wrote:
> > Hi Hans,
> >
> > On Thu, Jul 05, 2018 at 10:25:19AM +0200, Hans Verkuil wrote:
> >> The vb2_core_qbuf() function didn't check if q->error was set. It is
> >> checked in
> >> __buf_prepare(), but that function isn't called if the buffer was already
> >> prepared before with VIDIOC_PREPARE_BUF.
> >>
> >> So check it at the start of vb2_core_qbuf() as well.
> >>
> >> Signed-off-by: Hans Verkuil
> >> ---
> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> >> b/drivers/media/common/videobuf2/videobuf2-core.c
> >> index d3501cd604cb..5d7946ec80d8 100644
> >> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> >> @@ -1484,6 +1484,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
> >> index, void *pb,
> >>struct vb2_buffer *vb;
> >>int ret;
> >>
> >> + if (q->error) {
> >> + dprintk(1, "fatal error occurred on queue\n");
> >> + return -EIO;
> >> + }
> >> +
> >>vb = q->bufs[index];
> >>
> >>if ((req && q->uses_qbuf) ||
> >
> > How long has this problem existed? It looks like something that should go
> > to the stable branches, too...
>
> It's always been there, but I don't think it is worth backporting. The use of
> VIDIOC_PREPARE_BUF is very rare, let alone the combination with
> vb2_queue_error().
>
> I came across it while reviewing code.
What's the effect of the missing check? That the user may queue a buffer
when the driver thinks the hardware won't be able to complete it? At least
that doesn't seem like a security issue.
Anyway,
Acked-by: Sakari Ailus
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH 3/3] media: add Rockchip VPU driver
On 05/07/18 19:28, Ezequiel Garcia wrote:
> Add a mem2mem driver for the VPU available on Rockchip SoCs.
> Currently only JPEG encoding is supported, for RK3399 and RK3288
> platforms.
>
> Signed-off-by: Ezequiel Garcia
> ---
> drivers/media/platform/Kconfig| 12 +
> drivers/media/platform/Makefile | 1 +
> drivers/media/platform/rockchip/vpu/Makefile | 8 +
> .../platform/rockchip/vpu/rk3288_vpu_hw.c | 127 +++
> .../rockchip/vpu/rk3288_vpu_hw_jpege.c| 156
> .../platform/rockchip/vpu/rk3288_vpu_regs.h | 442 ++
> .../platform/rockchip/vpu/rk3399_vpu_hw.c | 127 +++
> .../rockchip/vpu/rk3399_vpu_hw_jpege.c| 165
> .../platform/rockchip/vpu/rk3399_vpu_regs.h | 601 ++
> .../platform/rockchip/vpu/rockchip_vpu.h | 270 +++
> .../platform/rockchip/vpu/rockchip_vpu_drv.c | 416 ++
> .../platform/rockchip/vpu/rockchip_vpu_enc.c | 763 ++
> .../platform/rockchip/vpu/rockchip_vpu_enc.h | 25 +
> .../platform/rockchip/vpu/rockchip_vpu_hw.h | 67 ++
> 14 files changed, 3180 insertions(+)
> create mode 100644 drivers/media/platform/rockchip/vpu/Makefile
> create mode 100644 drivers/media/platform/rockchip/vpu/rk3288_vpu_hw.c
> create mode 100644 drivers/media/platform/rockchip/vpu/rk3288_vpu_hw_jpege.c
> create mode 100644 drivers/media/platform/rockchip/vpu/rk3288_vpu_regs.h
> create mode 100644 drivers/media/platform/rockchip/vpu/rk3399_vpu_hw.c
> create mode 100644 drivers/media/platform/rockchip/vpu/rk3399_vpu_hw_jpege.c
> create mode 100644 drivers/media/platform/rockchip/vpu/rk3399_vpu_regs.h
> create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu.h
> create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu_drv.c
> create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu_enc.c
> create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu_enc.h
> create mode 100644 drivers/media/platform/rockchip/vpu/rockchip_vpu_hw.h
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 2728376b04b5..7244a2360732 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -448,6 +448,18 @@ config VIDEO_ROCKCHIP_RGA
>
> To compile this driver as a module choose m here.
>
> +config VIDEO_ROCKCHIP_VPU
> + tristate "Rockchip VPU driver"
> + depends on VIDEO_DEV && VIDEO_V4L2
> + depends on ARCH_ROCKCHIP || COMPILE_TEST
> + select VIDEOBUF2_DMA_CONTIG
> + select V4L2_MEM2MEM_DEV
> + default n
> + ---help---
> + Support for the VPU video codec found on Rockchip SoC.
> + To compile this driver as a module, choose M here: the module
> + will be called rockchip-vpu.
> +
> config VIDEO_TI_VPE
> tristate "TI VPE (Video Processing Engine) driver"
> depends on VIDEO_DEV && VIDEO_V4L2
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 04bc1502a30e..83378180d8ac 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_VIDEO_RENESAS_JPU) += rcar_jpu.o
> obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1/
>
> obj-$(CONFIG_VIDEO_ROCKCHIP_RGA) += rockchip/rga/
> +obj-$(CONFIG_VIDEO_ROCKCHIP_VPU)+= rockchip/vpu/
>
> obj-y+= omap/
>
> diff --git a/drivers/media/platform/rockchip/vpu/Makefile
> b/drivers/media/platform/rockchip/vpu/Makefile
> new file mode 100644
> index ..cab0123c49d4
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/vpu/Makefile
> @@ -0,0 +1,8 @@
> +obj-$(CONFIG_VIDEO_ROCKCHIP_VPU) += rockchip-vpu.o
> +
> +rockchip-vpu-y += rockchip_vpu_drv.o \
> + rockchip_vpu_enc.o \
> + rk3288_vpu_hw.o \
> + rk3288_vpu_hw_jpege.o \
> + rk3399_vpu_hw.o \
> + rk3399_vpu_hw_jpege.o
> diff --git a/drivers/media/platform/rockchip/vpu/rk3288_vpu_hw.c
> b/drivers/media/platform/rockchip/vpu/rk3288_vpu_hw.c
> new file mode 100644
> index ..0caff8ecf343
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/vpu/rk3288_vpu_hw.c
> @@ -0,0 +1,127 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip VPU codec driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + * Jeffy Chen
> + */
> +
> +#include
> +
> +#include "rockchip_vpu.h"
> +#include "rk3288_vpu_regs.h"
> +
> +#define RK3288_ACLK_MAX_FREQ (400 * 1000 * 1000)
> +
> +/*
> + * Supported formats.
> + */
> +
> +static const struct rockchip_vpu_fmt rk3288_vpu_enc_fmts[] = {
> + /* Source formats. */
> + {
> + .name = "4:2:0 3 planes Y/Cb/Cr",
Drop the name field, it's not needed.
> + .fourcc = V4L2_PIX_FMT_YUV420M,
> + .codec_mode = RK_VPU_CODEC_NONE,
> + .num_planes = 3,
> + .depth = { 8, 4, 4 },
> + .enc_fmt = RK3288_VPU_ENC_FMT_YUV420P,
>
Re: [PATCH 1/3] media: Add JPEG_RAW format
On 05/07/18 19:28, Ezequiel Garcia wrote:
> From: Shunqian Zheng
>
> Add V4L2_PIX_FMT_JPEG_RAW format that does not contain
> JPEG header in the output frame.
>
> Signed-off-by: Shunqian Zheng
> ---
> Documentation/media/uapi/v4l/pixfmt-compressed.rst | 5 +
> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> include/uapi/linux/videodev2.h | 1 +
> 3 files changed, 7 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst
> b/Documentation/media/uapi/v4l/pixfmt-compressed.rst
> index abec03937bb3..ebfc3cb7399c 100644
> --- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst
> +++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst
> @@ -23,6 +23,11 @@ Compressed Formats
>- 'JPEG'
>- TBD. See also :ref:`VIDIOC_G_JPEGCOMP `,
> :ref:`VIDIOC_S_JPEGCOMP `.
> +* .. _V4L2-PIX-FMT-JPEG-RAW:
> +
> + - ``V4L2_PIX_FMT_JPEG_RAW``
> + - 'Raw JPEG'
> + - JPEG without any headers.
> * .. _V4L2-PIX-FMT-MPEG:
>
>- ``V4L2_PIX_FMT_MPEG``
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index dd210067151f..9f0c76ec7c2c 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1259,6 +1259,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> /* Max description length mask: descr =
> "0123456789012345678901234567890" */
> case V4L2_PIX_FMT_MJPEG:descr = "Motion-JPEG"; break;
> case V4L2_PIX_FMT_JPEG: descr = "JFIF JPEG"; break;
> + case V4L2_PIX_FMT_JPEG_RAW: descr = "Raw JPEG"; break;
> case V4L2_PIX_FMT_DV: descr = "1394"; break;
> case V4L2_PIX_FMT_MPEG: descr = "MPEG-1/2/4"; break;
> case V4L2_PIX_FMT_H264: descr = "H.264"; break;
You missed one more case: JPEG_RAW should also set the COMPRESSED flag.
Regards,
Hans
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 600877be5c22..934e91af1b40 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -621,6 +621,7 @@ struct v4l2_pix_format {
> /* compressed formats */
> #define V4L2_PIX_FMT_MJPEGv4l2_fourcc('M', 'J', 'P', 'G') /* Motion-JPEG
> */
> #define V4L2_PIX_FMT_JPEG v4l2_fourcc('J', 'P', 'E', 'G') /* JFIF JPEG
> */
> +#define V4L2_PIX_FMT_JPEG_RAW v4l2_fourcc('J', 'P', 'G', 'R') /* JFIF JPEG
> RAW without headers */
> #define V4L2_PIX_FMT_DV v4l2_fourcc('d', 'v', 's', 'd') /* 1394
> */
> #define V4L2_PIX_FMT_MPEG v4l2_fourcc('M', 'P', 'E', 'G') /* MPEG-1/2/4
> Multiplexed */
> #define V4L2_PIX_FMT_H264 v4l2_fourcc('H', '2', '6', '4') /* H264 with
> start codes */
>
Re: [PATCH 2/3] media: Add controls for jpeg quantization tables
On 05/07/18 19:28, Ezequiel Garcia wrote:
> From: Shunqian Zheng
>
> Add V4L2_CID_JPEG_LUMA/CHROMA_QUANTIZATION controls to allow userspace
> configure the JPEG quantization tables.
>
> Signed-off-by: Shunqian Zheng
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 4
> include/uapi/linux/v4l2-controls.h | 3 +++
> 2 files changed, 7 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c
> index d29e45516eb7..b06adba2c486 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -980,6 +980,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_JPEG_RESTART_INTERVAL:return "Restart Interval";
> case V4L2_CID_JPEG_COMPRESSION_QUALITY: return "Compression Quality";
> case V4L2_CID_JPEG_ACTIVE_MARKER: return "Active Markers";
> + case V4L2_CID_JPEG_LUMA_QUANTIZATION: return "Luminance Quantization
> Matrix";
> + case V4L2_CID_JPEG_CHROMA_QUANTIZATION: return "Chrominance
> Quantization Matrix";
>
> /* Image source controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1263,6 +1265,8 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> v4l2_ctrl_type *type,
> *flags |= V4L2_CTRL_FLAG_READ_ONLY;
> break;
> case V4L2_CID_DETECT_MD_REGION_GRID:
> + case V4L2_CID_JPEG_LUMA_QUANTIZATION:
> + case V4L2_CID_JPEG_CHROMA_QUANTIZATION:
> *type = V4L2_CTRL_TYPE_U8;
> break;
> case V4L2_CID_DETECT_MD_THRESHOLD_GRID:
> diff --git a/include/uapi/linux/v4l2-controls.h
> b/include/uapi/linux/v4l2-controls.h
> index 8d473c979b61..b731fd7bc899 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -971,6 +971,9 @@ enum v4l2_jpeg_chroma_subsampling {
> #define V4L2_JPEG_ACTIVE_MARKER_DQT (1 << 17)
> #define V4L2_JPEG_ACTIVE_MARKER_DHT (1 << 18)
>
> +#define V4L2_CID_JPEG_LUMA_QUANTIZATION
> (V4L2_CID_JPEG_CLASS_BASE + 5)
> +#define V4L2_CID_JPEG_CHROMA_QUANTIZATION(V4L2_CID_JPEG_CLASS_BASE + 6)
> +
>
> /* Image source controls */
> #define V4L2_CID_IMAGE_SOURCE_CLASS_BASE (V4L2_CTRL_CLASS_IMAGE_SOURCE |
> 0x900)
>
Missing documentation of these new controls.
I suggest that the documentation refers to the JPEG_RAW pixelformat, so it is
clear how it should be used.
Regards,
Hans
Re: [PATCH 1/3] media: Add JPEG_RAW format
On 05/07/18 19:28, Ezequiel Garcia wrote:
> From: Shunqian Zheng
>
> Add V4L2_PIX_FMT_JPEG_RAW format that does not contain
> JPEG header in the output frame.
>
> Signed-off-by: Shunqian Zheng
> ---
> Documentation/media/uapi/v4l/pixfmt-compressed.rst | 5 +
> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> include/uapi/linux/videodev2.h | 1 +
> 3 files changed, 7 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst
> b/Documentation/media/uapi/v4l/pixfmt-compressed.rst
> index abec03937bb3..ebfc3cb7399c 100644
> --- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst
> +++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst
> @@ -23,6 +23,11 @@ Compressed Formats
>- 'JPEG'
>- TBD. See also :ref:`VIDIOC_G_JPEGCOMP `,
> :ref:`VIDIOC_S_JPEGCOMP `.
> +* .. _V4L2-PIX-FMT-JPEG-RAW:
> +
> + - ``V4L2_PIX_FMT_JPEG_RAW``
> + - 'Raw JPEG'
> + - JPEG without any headers.
This really needs to be more specific and explain which headers you are talking
about.
with some reference to the spec perhaps. And perhaps point to the luma/chroma
quantization
controls on how the header information should be passed in this case.
Regards,
Hans
> * .. _V4L2-PIX-FMT-MPEG:
>
>- ``V4L2_PIX_FMT_MPEG``
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index dd210067151f..9f0c76ec7c2c 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1259,6 +1259,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> /* Max description length mask: descr =
> "0123456789012345678901234567890" */
> case V4L2_PIX_FMT_MJPEG:descr = "Motion-JPEG"; break;
> case V4L2_PIX_FMT_JPEG: descr = "JFIF JPEG"; break;
> + case V4L2_PIX_FMT_JPEG_RAW: descr = "Raw JPEG"; break;
> case V4L2_PIX_FMT_DV: descr = "1394"; break;
> case V4L2_PIX_FMT_MPEG: descr = "MPEG-1/2/4"; break;
> case V4L2_PIX_FMT_H264: descr = "H.264"; break;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 600877be5c22..934e91af1b40 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -621,6 +621,7 @@ struct v4l2_pix_format {
> /* compressed formats */
> #define V4L2_PIX_FMT_MJPEGv4l2_fourcc('M', 'J', 'P', 'G') /* Motion-JPEG
> */
> #define V4L2_PIX_FMT_JPEG v4l2_fourcc('J', 'P', 'E', 'G') /* JFIF JPEG
> */
> +#define V4L2_PIX_FMT_JPEG_RAW v4l2_fourcc('J', 'P', 'G', 'R') /* JFIF JPEG
> RAW without headers */
> #define V4L2_PIX_FMT_DV v4l2_fourcc('d', 'v', 's', 'd') /* 1394
> */
> #define V4L2_PIX_FMT_MPEG v4l2_fourcc('M', 'P', 'E', 'G') /* MPEG-1/2/4
> Multiplexed */
> #define V4L2_PIX_FMT_H264 v4l2_fourcc('H', '2', '6', '4') /* H264 with
> start codes */
>
Re: [PATCH] videobuf2-core: check for q->error in vb2_core_qbuf()
On 16/07/18 14:49, Sakari Ailus wrote:
> Hi Hans,
>
> On Thu, Jul 05, 2018 at 10:25:19AM +0200, Hans Verkuil wrote:
>> The vb2_core_qbuf() function didn't check if q->error was set. It is checked
>> in
>> __buf_prepare(), but that function isn't called if the buffer was already
>> prepared before with VIDIOC_PREPARE_BUF.
>>
>> So check it at the start of vb2_core_qbuf() as well.
>>
>> Signed-off-by: Hans Verkuil
>> ---
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
>> b/drivers/media/common/videobuf2/videobuf2-core.c
>> index d3501cd604cb..5d7946ec80d8 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1484,6 +1484,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
>> index, void *pb,
>> struct vb2_buffer *vb;
>> int ret;
>>
>> +if (q->error) {
>> +dprintk(1, "fatal error occurred on queue\n");
>> +return -EIO;
>> +}
>> +
>> vb = q->bufs[index];
>>
>> if ((req && q->uses_qbuf) ||
>
> How long has this problem existed? It looks like something that should go
> to the stable branches, too...
It's always been there, but I don't think it is worth backporting. The use of
VIDIOC_PREPARE_BUF is very rare, let alone the combination with
vb2_queue_error().
I came across it while reviewing code.
Regards,
Hans
Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives
Hi Laurent,
On Wed, 18 Jul 2018, Laurent Pinchart wrote:
> Hi Guennadi,
>
> On Wednesday, 18 July 2018 00:30:45 EEST Guennadi Liakhovetski wrote:
> > On Tue, 17 Jul 2018, Laurent Pinchart wrote:
> > > On Thursday, 12 July 2018 10:30:46 EEST Guennadi Liakhovetski wrote:
> > >> On Thu, 12 Jul 2018, Laurent Pinchart wrote:
> > >>> On Tuesday, 8 May 2018 18:07:43 EEST Guennadi Liakhovetski wrote:
> > UVC defines a method of handling asynchronous controls, which sends a
> > USB packet over the interrupt pipe. This patch implements support for
> > such packets by sending a control event to the user. Since this can
> > involve USB traffic and, therefore, scheduling, this has to be done
> > in a work queue.
> >
> > Signed-off-by: Guennadi Liakhovetski
> >
> > ---
> >
> > v8:
> >
> > * avoid losing events by delaying the status URB resubmission until
> > after completion of the current event
> > * extract control value calculation into __uvc_ctrl_get_value()
> > * do not proactively return EBUSY if the previous control hasn't
> > completed yet, let the camera handle such cases
> > * multiple cosmetic changes
> >
> > drivers/media/usb/uvc/uvc_ctrl.c | 166 +--
> > drivers/media/usb/uvc/uvc_status.c | 112 ++---
> > drivers/media/usb/uvc/uvc_v4l2.c | 4 +-
> > drivers/media/usb/uvc/uvcvideo.h | 15 +++-
> > include/uapi/linux/uvcvideo.h | 2 +
> > 5 files changed, 255 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..796f86a 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > >>
> > >> [snip]
> > >>
> > +static void uvc_ctrl_status_event_work(struct work_struct *work)
> > +{
> > + struct uvc_device *dev = container_of(work, struct uvc_device,
> > +async_ctrl.work);
> > + struct uvc_ctrl_work *w = >async_ctrl;
> > + struct uvc_control_mapping *mapping;
> > + struct uvc_control *ctrl = w->ctrl;
> > + unsigned int i;
> > + int ret;
> > +
> > + mutex_lock(>chain->ctrl_mutex);
> > +
> > + list_for_each_entry(mapping, >info.mappings, list) {
> > + s32 value = __uvc_ctrl_get_value(mapping, w->data);
> > +
> > + /*
> > + * So far none of the auto-update controls in the
> > uvc_ctrls[]
> > + * table is mapped to a V4L control with slaves in the
> > + * uvc_ctrl_mappings[] list, so slave controls so far
> > never have
> > + * handle == NULL, but this can change in the future
> > + */
> > + for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
> > + if (!mapping->slave_ids[i])
> > + break;
> > +
> > + __uvc_ctrl_send_slave_event(ctrl->handle,
> > w->chain,
> > + ctrl,
> > mapping->slave_ids[i]);
> > + }
> > +
> > + uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
> > + V4L2_EVENT_CTRL_CH_VALUE);
> > + }
> > +
> > + mutex_unlock(>chain->ctrl_mutex);
> > +
> > + ctrl->handle = NULL;
> > >>>
> > >>> Can't this race with a uvc_ctrl_set() call, resulting in ctrl->handle
> > >>> being NULL after the control gets set ?
> > >>
> > >> Right, it's better to set .handle to NULL before sending events.
> > >> Something like
> > >>
> > >> mutex_lock();
> > >>
> > >> handle = ctrl->handle;
> > >> ctrl->handle = NULL;
> > >>
> > >> list_for_each_entry() {
> > >>
> > >> ...
> > >> uvc_ctrl_send_event(handle,...);
> > >>
> > >> }
> > >>
> > >> mutex_unlock();
> > >>
> > >> ?
> > >
> > > I think you also have to take the same lock in the uvc_ctrl_set() function
> > > to fix the problem, otherwise the ctrl->handle = NULL line could still be
> > > executed after the ctrl->handle assignment in uvc_ctrl_set(), resulting
> > > in ctrl->handle being NULL while the control is being set.
> >
> > Doesn't this mean, that you're attempting to send a new instance of the
> > same control before the previous has completed? In which case also taking
> > the lock in uvc_ctrl_set() wouldn't help either, because you can anyway do
> > that immediately after the first instance, before the completion even has
> > fired.
>
> You're right that it won't solve the race completely, but wouldn't it at
> least
> prevent ctrl->handle from being NULL ? We can't guarantee which of the old
> and
> new handle will be used
