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 Nov 17 05:00:11 CET 2018 media-tree git hash:fbe57dde7126d1b2712ab5ea93fb9d15f89de708 media_build git hash: a8aef9cea0a4a2f3ea86c0b37bd6a1378018c0c1 v4l-utils git hash: 2ee0d9434630281dbcbe47719a268044775ec7e6 edid-decode git hash: 5eeb151a748788666534d6ea3da07f90400d24c2 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.18.0-2-amd64 linux-git-arm-at91: WARNINGS linux-git-arm-davinci: WARNINGS linux-git-arm-multi: WARNINGS linux-git-arm-pxa: WARNINGS linux-git-arm-stm32: WARNINGS linux-git-arm64: OK linux-git-i686: WARNINGS linux-git-mips: OK linux-git-powerpc64: WARNINGS linux-git-sh: OK linux-git-x86_64: WARNINGS Check COMPILE_TEST: 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.123-i686: OK linux-3.18.123-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.159-i686: OK linux-4.4.159-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.131-i686: OK linux-4.9.131-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.74-i686: OK linux-4.14.74-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.12-i686: OK linux-4.18.12-x86_64: OK linux-4.19.1-i686: OK linux-4.19.1-x86_64: OK linux-4.20-rc1-i686: OK linux-4.20-rc1-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
RE: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
Hi, Sakari,
Thanks for the thorough review.
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Friday, November 2, 2018 8:03 AM
> To: Zhi, Yong
> Cc: linux-media@vger.kernel.org; tf...@chromium.org;
> mche...@kernel.org; hans.verk...@cisco.com;
> laurent.pinch...@ideasonboard.com; Mani, Rajmohan
> ; Zheng, Jian Xu ; Hu,
> Jerry W ; Toivonen, Tuukka
> ; Qiu, Tian Shu ; Cao,
> Bingbu ; Li, Chao C
> Subject: Re: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
>
> Hi Yong,
>
> Thanks for the update! I went through this again... a few comments below
> but I'd say they're mostly pretty minor issues.
>
> On Mon, Oct 29, 2018 at 03:22:57PM -0700, 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 | 181 ++
> > include/uapi/linux/intel-ipu3.h| 2819
> >
> > 3 files changed, 3001 insertions(+)
> > create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-intel-
> ipu3.rst
> > create mode 100644 include/uapi/linux/intel-ipu3.h
> >
> > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst
> b/Documentation/media/uapi/v4l/meta-formats.rst
> > index cf971d5..eafc534 100644
> > --- a/Documentation/media/uapi/v4l/meta-formats.rst
> > +++ b/Documentation/media/uapi/v4l/meta-formats.rst
> > @@ -12,6 +12,7 @@ These formats are used for the :ref:`metadata`
> interface only.
> > .. toctree::
> > :maxdepth: 1
> >
> > +pixfmt-meta-intel-ipu3
> > pixfmt-meta-d4xx
> > pixfmt-meta-uvc
> > pixfmt-meta-vsp1-hgo
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > new file mode 100644
> > index 000..23b945b
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > @@ -0,0 +1,181 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _intel-ipu3:
>
> Instead, to avoid a warning from Sphinx, replace the line with these:
>
> .. _v4l2-meta-fmt-ipu3-params:
> .. _v4l2-meta-fmt-ipu3-stat-3a:
>
Ack.
> > +
> >
> +***
> ***
> > +V4L2_META_FMT_IPU3_PARAMS ('ip3p'), V4L2_META_FMT_IPU3_3A
> ('ip3s')
> >
> +***
> ***
> > +
> > +.. c:type:: ipu3_uapi_stats_3a
> > +
> > +3A statistics
> > +=
> > +
> > +For IPU3 ImgU, the 3A statistics accelerators collect different statistics
> > over
> > +an input bayer frame. Those statistics, defined in data struct
> > +:c:type:`ipu3_uapi_stats_3a`, are meta output obtained from "ipu3-imgu
> 3a stat"
> > +video node, which are then passed to user space for statistics analysis
> > +using :c:type:`v4l2_meta_format` interface.
> > +
> > +The statistics collected are AWB (Auto-white balance) RGBS (Red, Green,
> Blue and
>
> Extra whitespace at the end of the line.
>
Ack.
> > +Saturation measure) cells, AWB filter response, AF (Auto-focus) filter
> response,
> > +and AE (Auto-exposure) histogram.
> > +
> > +struct :c:type:`ipu3_uapi_4a_config` saves configurable parameters for all
> above.
> > +
> > +
> > +.. code-block:: c
> > +
> > +
> > + struct ipu3_uapi_stats_3a {
> > + struct ipu3_uapi_awb_raw_buffer awb_raw_buffer
> > +__attribute__((aligned(32)));
> > + struct ipu3_uapi_ae_raw_buffer_aligned
> > + ae_raw_buffer[IPU3_UAPI_MAX_STRIPES];
> > + struct ipu3_uapi_af_raw_buffer af_raw_buffer;
> > + struct ipu3_uapi_awb_fr_raw_buffer awb_fr_raw_buffer;
> > + struct ipu3_uapi_4a_config stats_4a_config;
> > + __u32 ae_join_buffers;
> > + __u8 padding[28];
> > + struct ipu3_uapi_stats_3a_bubble_info_per_stripe
> > + stats_3a_bubble_per_stripe;
>
> I think you could just unwrap these, even if it causes them to be over 80
> characters per line. They display better in a web browser that way. Or
> alternatively align the wrapped lines to the same column.
>
Ack.
> > + struct ipu3_uapi_ff_status stats_3a_status;
> > + } __packed;
> > +
> > +
> > +.. c:type:: ipu3_uapi_params
> > +
> > +Pipeline parameters
> > +===
> > +
> > +IPU3 pipeline has a number of image processing stages, each of which
> takes a
> > +set of parameters as input. The major stages of pipelines are shown here:
> > +
> > +Raw pixels -> Bayer Downscaling -> Optical Black Correction ->
> > +
> > +Linearization -> Lens Shading Correction -> White Balance / Exposure /
> > +
> > +Focus Apply -> Bayer Noise Reduction -> ANR -> Demosaicing -> Color
> > +
> > +Correction Matrix -> Gamma correction -> Color Space
Re: TechnoTrend CT2-4500 remote not working
Sean, Am 16.11.2018 um 12:26 schrieb Sean Young: Ok, thank you. Now, we don't know how the IR is wired up. Please could you try enabling the enable_885_ir module parameter for cx23885. If this goes badly, then we might end up in an infinite loop of unending interrupts, so it would be prudent to not change your startup scripts to set this. As root, please run: rmmod cx23885 modprobe cx23885 enable_885_ir=1 Thank you, loading the module with enable_885_ir=1 works: cx23885: cx23885_dev_checkrevision() Hardware revision = 0xa5 cx23885: cx23885[0]/0: found at :17:00.0, rev: 4, irq: 31, latency: 0, mmio: 0xfe00 Registered IR keymap rc-tt-1500 IR RC5(x/sz) protocol handler initialized rc rc1: cx23885 IR (Technotrend TT-budget CT2-4500 CI) as /devices/pci:00/:00:01.2/:15:00.2/:16:00.0/:17:00.0/rc/rc1 input: cx23885 IR (Technotrend TT-budget CT2-4500 CI) as /devices/pci:00/:00:01.2/:15:00.2/:16:00.0/:17:00.0/rc/rc1/input21 ir-keytable output: Found /sys/class/rc/rc1/ (/dev/input/event17) with: Name: cx23885 IR (Technotrend TT-budget CT2-4500 CI) Driver: cx23885, table: rc-tt-1500 lirc device: /dev/lirc1 Supported protocols: other lirc rc-5 rc-5-sz jvc sony nec sanyo mce_kbd rc-6 sharp xmp Enabled protocols: lirc rc-5 bus: 1, vendor/product: 13c2:3013, version: 0x0001 Repeat delay = 500 ms, repeat period = 125 ms Using ir-keytable -t I see all the buttons pressed. Maybe you can amend the module options description to include my device: parm: enable_885_ir:Enable integrated IR controller for supported CX2388[57] boards that are wired for it: HVR-1250 (reported safe) TerraTec Cinergy T PCIe Dual (not well tested, appears to be safe) TeVii S470 (reported unsafe) This can cause an interrupt storm with some cards. Default: 0 [Disabled] (int) You should get another rc device, which might just work. Thanks again Sean for the help! Martin
I AWAIT YOUR SWIFT RESPONSE !!!
I am Vice Chairman of Hang Seng Bank, I have Important Matter to Discuss with you concerning my late client. Died without a NEXT OF KIN. Send me your private email for full details information. email me at E-Mail: draymondchie...@gmail.com Regards Mr.Fung
I AWAIT YOUR SWIFT RESPONSE !!!
I am Vice Chairman of Hang Seng Bank, I have Important Matter to Discuss with you concerning my late client. Died without a NEXT OF KIN. Send me your private email for full details information. email me at E-Mail: draymondchie...@gmail.com Regards Mr.Fung
Re: [PATCH v2 3/6] media: mt9m111: add support to select formats and fps for {Q,SXGA}
Hi Hans,
On Fri, Nov 16, 2018 at 02:31:01PM +0100, Hans Verkuil wrote:
> On 11/16/2018 02:26 PM, Sakari Ailus wrote:
> > Hi Marco, Michael,
> >
> > On Mon, Oct 29, 2018 at 07:24:07PM +0100, Marco Felsch wrote:
> >> From: Michael Grzeschik
> >>
> >> This patch implements the framerate selection using the skipping and
> >> readout power-modi features. The power-modi cut the framerate by half
> >> and each context has an independent selection bit. The same applies to
> >> the 2x skipping feature.
> >>
> >> Signed-off-by: Michael Grzeschik
> >> Signed-off-by: Marco Felsch
> >>
> >> ---
> >> Changelog
> >>
> >> v2:
> >> - fix updating read mode register, use mt9m111_reg_mask() to update the
> >> relevant bits only. For this purpose add reg_mask field to
> >> struct mt9m111_mode_info.
> >>
> >> drivers/media/i2c/mt9m111.c | 163
> >> 1 file changed, 163 insertions(+)
> >>
> >> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
>
>
>
> >> +static const struct mt9m111_mode_info *
> >> +mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
> >> +unsigned int width, unsigned int height)
> >> +{
> >> + const struct mt9m111_mode_info *mode;
> >> + struct v4l2_rect *sensor_rect = >rect;
> >> + unsigned int gap, gap_best = (unsigned int) -1;
> >> + int i, best_gap_idx = 1;
> >> +
> >> + /* find best matched fps */
> >> + for (i = 0; i < MT9M111_NUM_MODES; i++) {
> >> + unsigned int fps = mt9m111_mode_data[i].max_fps;
> >> +
> >> + gap = abs(fps - req_fps);
> >> + if (gap < gap_best) {
> >> + best_gap_idx = i;
> >> + gap_best = gap;
> >> + }
> >
> > Could you use v4l2_find_nearest_size() instead?
> >
> > Also see below...
> >
> >> + }
> >> +
> >> + /*
> >> + * Use context a/b default timing values instead of calculate blanking
> >> + * timing values.
> >> + */
> >> + mode = _mode_data[best_gap_idx];
> >> + mt9m111->ctx = (best_gap_idx == MT9M111_MODE_QSXGA_30FPS) ? _a :
> >> + _b;
> >> +
> >> + /*
> >> + * Check if current settings support the fps because fps selection is
> >> + * based on the row/col skipping mechanism which has some restriction.
> >> + */
> >> + if (sensor_rect->width != mode->sensor_w ||
> >> + sensor_rect->height != mode->sensor_h ||
> >> + width > mode->max_image_w ||
> >> + height > mode->max_image_h) {
> >> + /* reset sensor window size */
> >> + mt9m111->rect.left = MT9M111_MIN_DARK_COLS;
> >> + mt9m111->rect.top = MT9M111_MIN_DARK_ROWS;
> >> + mt9m111->rect.width = mode->sensor_w;
> >> + mt9m111->rect.height = mode->sensor_h;
> >> +
> >> + /* reset image size */
> >> + mt9m111->width = mode->max_image_w;
> >> + mt9m111->height = mode->max_image_h;
> >> +
> >> + dev_warn(mt9m111->subdev.dev,
> >> + "Warning: update image size %dx%d[%dx%d] ->
> >> %dx%d[%dx%d]\n",
> >> + sensor_rect->width, sensor_rect->height, width, height,
> >> + mode->sensor_w, mode->sensor_h, mode->max_image_w,
> >> + mode->max_image_h);
> >
> > I wouldn't expect requesting a particular frame rate to change the sensor
> > format. The other way around is definitely fine though.
> >
> > Cc Hans.
>
> I agree with Sakari. Changing the framerate should never change the format.
> When you enumerate framerates those framerates are the allowed framerates
> for the mediabus format and the resolution. So changing the framerate should
> never modify the format or resolution. Instead, the framerate should be
> mapped to a framerate that is valid for the format/resolution combo.
I don't think this is actually documented, at least not for the sub-device
API. I can send a patch.
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v2 4/6] media: mt9m111: allow to setup pixclk polarity
Hi Marco, Enrico,
On Mon, Oct 29, 2018 at 07:24:08PM +0100, Marco Felsch wrote:
> From: Enrico Scholz
>
> The chip can be configured to output data transitions on the
> rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> falling edge.
>
> Parsing the fw-node is made in a subfunction to bundle all (future)
> dt-parsing / fw-parsing stuff.
>
> Signed-off-by: Enrico Scholz
> (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> per default. Set bit to 0 (enable mask bit without value) to enable
> falling edge sampling.)
> Signed-off-by: Michael Grzeschik
> (m.fel...@pengutronix.de: use fwnode helpers)
> (m.fel...@pengutronix.de: mv fw parsing into own function)
> (m.fel...@pengutronix.de: adapt commit msg)
> Signed-off-by: Marco Felsch
>
> ---
> Changelog:
>
> v2:
> - make use of fwnode_*() to drop OF dependency and ifdef's
> - mt9m111_g_mbus_config: fix pclk_sample logic which I made due the
> conversion from Enrico's patch.
>
> drivers/media/i2c/mt9m111.c | 46 -
> 1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index e9879e111f58..112eaa5ba917 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -15,12 +15,14 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> #include
> #include
> #include
> +#include
>
> /*
> * MT9M111, MT9M112 and MT9M131:
> @@ -239,6 +241,8 @@ struct mt9m111 {
> const struct mt9m111_datafmt *fmt;
> int lastpage; /* PageMap cache value */
> bool is_streaming;
> + /* user point of view - 0: falling 1: rising edge */
> + unsigned int pclk_sample:1;
You could use a bool. Up to you.
> #ifdef CONFIG_MEDIA_CONTROLLER
> struct media_pad pad;
> #endif
> @@ -591,6 +595,10 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
> return -EINVAL;
> }
>
> + /* receiver samples on falling edge, chip-hw default is rising */
> + if (mt9m111->pclk_sample == 0)
> + mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
> +
> ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> data_outfmt2, mask_outfmt2);
> if (!ret)
> @@ -1051,9 +1059,15 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd,
> int enable)
> static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
> struct v4l2_mbus_config *cfg)
> {
> - cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
> + struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> +
> + cfg->flags = V4L2_MBUS_MASTER |
> V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
> V4L2_MBUS_DATA_ACTIVE_HIGH;
> +
> + cfg->flags |= mt9m111->pclk_sample ? V4L2_MBUS_PCLK_SAMPLE_RISING :
> + V4L2_MBUS_PCLK_SAMPLE_FALLING;
> +
> cfg->type = V4L2_MBUS_PARALLEL;
>
> return 0;
> @@ -1123,6 +1137,32 @@ static int mt9m111_video_probe(struct i2c_client
> *client)
> return ret;
> }
>
> +static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111
> *mt9m111)
> +{
> + struct v4l2_fwnode_endpoint *bus_cfg;
> + struct fwnode_handle *np;
> + int ret = 0;
> +
> + np = fwnode_graph_get_next_endpoint(dev_fwnode(>dev), NULL);
> + if (!np)
> + return -EINVAL;
> +
> + bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> + if (IS_ERR(bus_cfg)) {
> + ret = PTR_ERR(bus_cfg);
> + goto out_put_fw;
> + }
> +
> + mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> + V4L2_MBUS_PCLK_SAMPLE_RISING);
> +
> + v4l2_fwnode_endpoint_free(bus_cfg);
> +
> +out_put_fw:
> + fwnode_handle_put(np);
> + return ret;
> +}
> +
> static int mt9m111_probe(struct i2c_client *client,
>const struct i2c_device_id *did)
> {
> @@ -1147,6 +1187,10 @@ static int mt9m111_probe(struct i2c_client *client,
> /* Default HIGHPOWER context */
> mt9m111->ctx = _b;
>
> + ret = mt9m111_probe_fw(client, mt9m111);
> + if (ret)
> + return ret;
Can you do this before v4l2_clk_get()? That'll go anyway, but for now,
you'd need extra error handling for it.
> +
> v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
> mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>
Please also put this patch after the DT binding changes.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v2 3/6] media: mt9m111: add support to select formats and fps for {Q,SXGA}
On 11/16/2018 02:26 PM, Sakari Ailus wrote:
> Hi Marco, Michael,
>
> On Mon, Oct 29, 2018 at 07:24:07PM +0100, Marco Felsch wrote:
>> From: Michael Grzeschik
>>
>> This patch implements the framerate selection using the skipping and
>> readout power-modi features. The power-modi cut the framerate by half
>> and each context has an independent selection bit. The same applies to
>> the 2x skipping feature.
>>
>> Signed-off-by: Michael Grzeschik
>> Signed-off-by: Marco Felsch
>>
>> ---
>> Changelog
>>
>> v2:
>> - fix updating read mode register, use mt9m111_reg_mask() to update the
>> relevant bits only. For this purpose add reg_mask field to
>> struct mt9m111_mode_info.
>>
>> drivers/media/i2c/mt9m111.c | 163
>> 1 file changed, 163 insertions(+)
>>
>> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
>> +static const struct mt9m111_mode_info *
>> +mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
>> + unsigned int width, unsigned int height)
>> +{
>> +const struct mt9m111_mode_info *mode;
>> +struct v4l2_rect *sensor_rect = >rect;
>> +unsigned int gap, gap_best = (unsigned int) -1;
>> +int i, best_gap_idx = 1;
>> +
>> +/* find best matched fps */
>> +for (i = 0; i < MT9M111_NUM_MODES; i++) {
>> +unsigned int fps = mt9m111_mode_data[i].max_fps;
>> +
>> +gap = abs(fps - req_fps);
>> +if (gap < gap_best) {
>> +best_gap_idx = i;
>> +gap_best = gap;
>> +}
>
> Could you use v4l2_find_nearest_size() instead?
>
> Also see below...
>
>> +}
>> +
>> +/*
>> + * Use context a/b default timing values instead of calculate blanking
>> + * timing values.
>> + */
>> +mode = _mode_data[best_gap_idx];
>> +mt9m111->ctx = (best_gap_idx == MT9M111_MODE_QSXGA_30FPS) ? _a :
>> +_b;
>> +
>> +/*
>> + * Check if current settings support the fps because fps selection is
>> + * based on the row/col skipping mechanism which has some restriction.
>> + */
>> +if (sensor_rect->width != mode->sensor_w ||
>> +sensor_rect->height != mode->sensor_h ||
>> +width > mode->max_image_w ||
>> +height > mode->max_image_h) {
>> +/* reset sensor window size */
>> +mt9m111->rect.left = MT9M111_MIN_DARK_COLS;
>> +mt9m111->rect.top = MT9M111_MIN_DARK_ROWS;
>> +mt9m111->rect.width = mode->sensor_w;
>> +mt9m111->rect.height = mode->sensor_h;
>> +
>> +/* reset image size */
>> +mt9m111->width = mode->max_image_w;
>> +mt9m111->height = mode->max_image_h;
>> +
>> +dev_warn(mt9m111->subdev.dev,
>> + "Warning: update image size %dx%d[%dx%d] ->
>> %dx%d[%dx%d]\n",
>> + sensor_rect->width, sensor_rect->height, width, height,
>> + mode->sensor_w, mode->sensor_h, mode->max_image_w,
>> + mode->max_image_h);
>
> I wouldn't expect requesting a particular frame rate to change the sensor
> format. The other way around is definitely fine though.
>
> Cc Hans.
I agree with Sakari. Changing the framerate should never change the format.
When you enumerate framerates those framerates are the allowed framerates
for the mediabus format and the resolution. So changing the framerate should
never modify the format or resolution. Instead, the framerate should be
mapped to a framerate that is valid for the format/resolution combo.
Regards,
Hans
Re: [PATCH v2 3/6] media: mt9m111: add support to select formats and fps for {Q,SXGA}
Hi Marco, Michael,
On Mon, Oct 29, 2018 at 07:24:07PM +0100, Marco Felsch wrote:
> From: Michael Grzeschik
>
> This patch implements the framerate selection using the skipping and
> readout power-modi features. The power-modi cut the framerate by half
> and each context has an independent selection bit. The same applies to
> the 2x skipping feature.
>
> Signed-off-by: Michael Grzeschik
> Signed-off-by: Marco Felsch
>
> ---
> Changelog
>
> v2:
> - fix updating read mode register, use mt9m111_reg_mask() to update the
> relevant bits only. For this purpose add reg_mask field to
> struct mt9m111_mode_info.
>
> drivers/media/i2c/mt9m111.c | 163
> 1 file changed, 163 insertions(+)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 1f8789fe28af..e9879e111f58 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -126,6 +126,8 @@
> #define MT9M111_OUTFMT_SWAP_YCbCr_C_Y_RGB_EVEN (1 << 1)
> #define MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr_RGB_R_B (1 << 0)
> #define MT9M111_TPG_SEL_MASK GENMASK(2, 0)
> +#define MT9M111_RM_PWR_MASK BIT(10)
> +#define MT9M111_RM_SKIP2_MASKGENMASK(3, 2)
>
> /*
> * Camera control register addresses (0x200..0x2ff not implemented)
> @@ -204,6 +206,23 @@ static const struct mt9m111_datafmt
> mt9m111_colour_fmts[] = {
> {MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, V4L2_COLORSPACE_SRGB},
> };
>
> +enum mt9m111_mode_id {
> + MT9M111_MODE_SXGA_8FPS,
> + MT9M111_MODE_SXGA_15FPS,
> + MT9M111_MODE_QSXGA_30FPS,
> + MT9M111_NUM_MODES,
> +};
> +
> +struct mt9m111_mode_info {
> + unsigned int sensor_w;
> + unsigned int sensor_h;
> + unsigned int max_image_w;
> + unsigned int max_image_h;
> + unsigned int max_fps;
> + unsigned int reg_val;
> + unsigned int reg_mask;
> +};
> +
> struct mt9m111 {
> struct v4l2_subdev subdev;
> struct v4l2_ctrl_handler hdl;
> @@ -213,6 +232,8 @@ struct mt9m111 {
> struct v4l2_clk *clk;
> unsigned int width; /* output */
> unsigned int height;/* sizes */
> + struct v4l2_fract frame_interval;
> + const struct mt9m111_mode_info *current_mode;
> struct mutex power_lock; /* lock to protect power_count */
> int power_count;
> const struct mt9m111_datafmt *fmt;
> @@ -223,6 +244,37 @@ struct mt9m111 {
> #endif
> };
>
> +static const struct mt9m111_mode_info mt9m111_mode_data[MT9M111_NUM_MODES] =
> {
> + [MT9M111_MODE_SXGA_8FPS] = {
> + .sensor_w = 1280,
> + .sensor_h = 1024,
> + .max_image_w = 1280,
> + .max_image_h = 1024,
> + .max_fps = 8,
> + .reg_val = MT9M111_RM_LOW_POWER_RD,
> + .reg_mask = MT9M111_RM_PWR_MASK | MT9M111_RM_SKIP2_MASK,
> + },
> + [MT9M111_MODE_SXGA_15FPS] = {
> + .sensor_w = 1280,
> + .sensor_h = 1024,
> + .max_image_w = 1280,
> + .max_image_h = 1024,
> + .max_fps = 15,
> + .reg_val = MT9M111_RM_FULL_POWER_RD,
> + .reg_mask = MT9M111_RM_PWR_MASK | MT9M111_RM_SKIP2_MASK,
> + },
> + [MT9M111_MODE_QSXGA_30FPS] = {
> + .sensor_w = 1280,
> + .sensor_h = 1024,
> + .max_image_w = 640,
> + .max_image_h = 512,
> + .max_fps = 30,
> + .reg_val = MT9M111_RM_LOW_POWER_RD | MT9M111_RM_COL_SKIP_2X |
> +MT9M111_RM_ROW_SKIP_2X,
> + .reg_mask = MT9M111_RM_PWR_MASK | MT9M111_RM_SKIP2_MASK,
> + },
> +};
> +
> /* Find a data format by a pixel code */
> static const struct mt9m111_datafmt *mt9m111_find_datafmt(struct mt9m111
> *mt9m111,
> u32 code)
> @@ -615,6 +667,62 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
> return ret;
> }
>
> +static const struct mt9m111_mode_info *
> +mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
> + unsigned int width, unsigned int height)
> +{
> + const struct mt9m111_mode_info *mode;
> + struct v4l2_rect *sensor_rect = >rect;
> + unsigned int gap, gap_best = (unsigned int) -1;
> + int i, best_gap_idx = 1;
> +
> + /* find best matched fps */
> + for (i = 0; i < MT9M111_NUM_MODES; i++) {
> + unsigned int fps = mt9m111_mode_data[i].max_fps;
> +
> + gap = abs(fps - req_fps);
> + if (gap < gap_best) {
> + best_gap_idx = i;
> + gap_best = gap;
> + }
Could you use v4l2_find_nearest_size() instead?
Also see below...
> + }
> +
> + /*
> + * Use context a/b default timing values instead of calculate blanking
> + * timing values.
> + */
> + mode = _mode_data[best_gap_idx];
> + mt9m111->ctx = (best_gap_idx == MT9M111_MODE_QSXGA_30FPS) ? _a :
> +
Re: [PATCH 4/4] media: ov5640: Add additional media bus formats
Hi Sam,
On Mon, Oct 08, 2018 at 11:48:02PM -0700, Sam Bobrowicz wrote:
> Add support for 1X16 yuv media bus formats (v4l2_mbus_framefmt).
> These formats are equivalent to the 2X8 formats that are already
> supported, both of which accurately describe the data present on
> the CSI2 interface. This change will increase compatibility with
> CSI2 RX drivers that only advertise support for the 1X16 formats.
>
> Signed-off-by: Sam Bobrowicz
> ---
> drivers/media/i2c/ov5640.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index a50d884..ca9de56 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -125,6 +125,8 @@ static const struct ov5640_pixfmt ov5640_formats[] = {
> { MEDIA_BUS_FMT_JPEG_1X8, V4L2_COLORSPACE_JPEG, },
> { MEDIA_BUS_FMT_UYVY8_2X8, V4L2_COLORSPACE_SRGB, },
> { MEDIA_BUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_SRGB, },
> + { MEDIA_BUS_FMT_UYVY8_1X16, V4L2_COLORSPACE_SRGB, },
> + { MEDIA_BUS_FMT_YUYV8_1X16, V4L2_COLORSPACE_SRGB, },
> { MEDIA_BUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB, },
> { MEDIA_BUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB, },
> };
Can you make these selectable only for the CSI-2 bus? I understand the
driver also supports the parallel interface.
> @@ -2069,10 +2071,12 @@ static int ov5640_set_framefmt(struct ov5640_dev
> *sensor,
>
> switch (format->code) {
> case MEDIA_BUS_FMT_UYVY8_2X8:
> + case MEDIA_BUS_FMT_UYVY8_1X16:
> /* YUV422, UYVY */
> val = 0x3f;
> break;
> case MEDIA_BUS_FMT_YUYV8_2X8:
> + case MEDIA_BUS_FMT_YUYV8_1X16:
> /* YUV422, YUYV */
> val = 0x30;
> break;
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCHv2 0/9] vb2/cedrus: add tag support
On 11/16/2018 10:00 AM, Tomasz Figa wrote: > Hi Hans, > > On Wed, Nov 14, 2018 at 10:47 PM Hans Verkuil wrote: >> >> From: Hans Verkuil >> >> As was discussed here (among other places): >> >> https://lkml.org/lkml/2018/10/19/440 >> >> using capture queue buffer indices to refer to reference frames is >> not a good idea. A better idea is to use a 'tag' where the >> application can assign a u64 tag to an output buffer, which is then >> copied to the capture buffer(s) derived from the output buffer. >> > > Thanks for the patches. Please see my comments below. > >> A u64 is chosen since this allows userspace to also use pointers to >> internal structures as 'tag'. >> > > I think this is not true anymore in this version. Ah, forgot to update the commit message. > >> The first three patches add core tag support, the next patch document >> the tag support, then a new helper function is added to v4l2-mem2mem.c >> to easily copy data from a source to a destination buffer that drivers >> can use. >> >> Next a new supports_tags vb2_queue flag is added to indicate that >> the driver supports tags. Ideally this should not be necessary, but >> that would require that all m2m drivers are converted to using the >> new helper function introduced in the previous patch. That takes more >> time then I have now since we want to get this in for 4.20. >> >> Finally the vim2m, vicodec and cedrus drivers are converted to support >> tags. >> >> I also removed the 'pad' fields from the mpeg2 control structs (it >> should never been added in the first place) and aligned the structs >> to a u32 boundary (u64 for the tag values). > > u32 in this version > >> >> Note that this might change further (Paul suggested using bitfields). >> >> Also note that the cedrus code doesn't set the sequence counter, that's >> something that should still be added before this driver can be moved >> out of staging. >> >> Note: if no buffer is found for a certain tag, then the dma address >> is just set to 0. That happened before as well with invalid buffer >> indices. This should be checked in the driver! > > Note that DMA address 0 may as well be a valid address. Should we have > another way of signaling that? See this patch: https://patchwork.linuxtv.org/patch/52975/ The memory of the reference buffer should be checked and the refcount incremented in the request validate function. Once that's done the dma address will be guaranteed to be OK. This is missing functionality that is important to add. > >> >> The previous RFC series was tested successfully with the cedrus driver. >> >> Regards, >> >> Hans >> >> Changes since v1: >> >> - changed to a u32 tag. Using a 64 bit tag was overly complicated due >> to the bad layout of the v4l2_buffer struct, and there is no real >> need for it by applications. > > I hope these won't become famous last words. :) For Chromium it should > be okay indeed. The only 'feature' that is now missing is the ability to store pointers as the tag. So worst case you need to do an extra lookup in the application. > > Since we've been thinking about a redesign of the buffer struct, > perhaps we can live with u32 for now and we can design the new struct > to handle u64 nicely? It could be done, but I'm not sure if it is a good idea to have different behavior between v4l2_buffer and v4l2_ext_buffer. Something to discuss at that time. Regards, Hans > > Best regards, > Tomasz >
Re: [PATCH 2/2] vb2: don't allow queueing buffers when canceling queue
On 11/16/2018 09:45 AM, Tomasz Figa wrote:
> On Fri, Nov 16, 2018 at 5:42 PM Hans Verkuil wrote:
>>
>> On 11/16/2018 09:34 AM, Tomasz Figa wrote:
>>> Hi Hans,
>>>
>>> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil wrote:
Calling the stop_streaming op can release the core serialization lock
pointed to by vb2_queue->lock if it has to wait for buffers to finish.
An example of that behavior is the vivid driver.
>>>
>>> Why would vb2_queue->lock have to be released to wait for buffer to
>>> finish? The drivers I worked with never had to do anything like that.
>>
>> Actually, they all do. It's done through the wait_prepare/finish callbacks
>> and by setting those to vb2_ops_wait_prepare/finish.
>>
>> If you don't, then while one thread is waiting for a buffer to arrive,
>> another thread cannot queue a new buffer since it will be serialized by
>> queue->lock.
>>
>> v4l2-compliance even tests for this.
>
> Why would you need the userspace to queue more buffers when you're
> stopping the queue?
Ah, I misunderstood your question. Your question was: why should stop_streaming
have to release the lock when it waits for buffers to finish.
In this case (vivid) the thread generating the image takes the main lock, which
is the same as queue->lock. So stop_streaming (which is called with the same
lock taken) has to unlock it, stop the thread, then retake it.
I thought this would be more common, but after analyzing other usages of kthread
it appears to be vivid specific. So I agree that it is better to fix vivid
instead of messing about with vb2.
Regards,
Hans
>
>>
>>>
However, if userspace dup()ped the video device filehandle, then it is
possible to stop streaming on one filehandle and call read/write or
VIDIOC_QBUF from the other.
>>>
>>> How about other ioctls? I can imagine at least STREAMON could be
>>> called at the same time too, but not sure if it would have any side
>>> effects.
>>
>> STREAMON would return an error since q->streaming is still set while
>> in the stop_streaming callback.
>>
>> So that combination is safe.
>>
>
> Okay, thanks. I'm still slightly worried that this approach with a
> flag makes it possible to miss some non-trivial cases, though...
>
>> Regards,
>>
>> Hans
>>
>>>
>>> Best regards,
>>> Tomasz
>>>
This is fixed by setting a flag whenever stop_streaming is called and
checking the flag where needed so we can return -EBUSY.
Signed-off-by: Hans Verkuil
Reported-by: syzbot+736c3aae4af7b50d9...@syzkaller.appspotmail.com
---
drivers/media/common/videobuf2/videobuf2-core.c | 14 +-
include/media/videobuf2-core.h | 1 +
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
b/drivers/media/common/videobuf2/videobuf2-core.c
index 138223af701f..560577321fe7 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1503,6 +1503,10 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
index, void *pb,
dprintk(1, "fatal error occurred on queue\n");
return -EIO;
}
+ if (q->in_stop_streaming) {
+ dprintk(1, "stop_streaming is called\n");
+ return -EBUSY;
+ }
vb = q->bufs[index];
@@ -1834,8 +1838,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
* Tell driver to stop all transactions and release all queued
* buffers.
*/
- if (q->start_streaming_called)
+ if (q->start_streaming_called) {
+ q->in_stop_streaming = 1;
call_void_qop(q, stop_streaming, q);
+ q->in_stop_streaming = 0;
+ }
/*
* If you see this warning, then the driver isn't cleaning up
properly
@@ -2565,6 +2572,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue
*q, char __user *data, size_
return -EBUSY;
}
+ if (q->in_stop_streaming) {
+ dprintk(3, "stop_streaming is called\n");
+ return -EBUSY;
+ }
+
/*
* Initialize emulator on first call.
*/
diff --git a/include/media/videobuf2-core.h
b/include/media/videobuf2-core.h
index 613f22910174..5a3d3ada5940 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -585,6 +585,7 @@ struct vb2_queue {
unsigned interror:1;
unsigned intwaiting_for_buffers:1;
unsigned intwaiting_in_dqbuf:1;
+ unsigned intin_stop_streaming:1;
[PATCH v4l-utils] keytable: match every entry in rc_maps.cfg, not just the first
Signed-off-by: Sean Young
---
utils/keytable/keytable.c | 71 ---
1 file changed, 36 insertions(+), 35 deletions(-)
diff --git a/utils/keytable/keytable.c b/utils/keytable/keytable.c
index e15440de..df9cfc49 100644
--- a/utils/keytable/keytable.c
+++ b/utils/keytable/keytable.c
@@ -634,14 +634,13 @@ static error_t parse_keyfile(char *fname, char **table)
return parse_plain_keyfile(fname, table);
}
-struct cfgfile *nextcfg =
-
static error_t parse_cfgfile(char *fname)
{
FILE *fin;
int line = 0;
char s[2048];
char *driver, *table, *filename;
+ struct cfgfile *nextcfg =
if (debug)
fprintf(stderr, _("Parsing %s config file\n"), fname);
@@ -2142,55 +2141,57 @@ int main(int argc, char *argv[])
struct cfgfile *cur;
char *fname, *name;
int rc;
+ int matches = 0;
for (cur = cur->next; cur = cur->next) {
if ((!rc_dev.drv_name || strcasecmp(cur->driver,
rc_dev.drv_name)) && strcasecmp(cur->driver, "*"))
continue;
if ((!rc_dev.keytable_name || strcasecmp(cur->table,
rc_dev.keytable_name)) && strcasecmp(cur->table, "*"))
continue;
- break;
- }
- if (!cur->next) {
if (debug)
- fprintf(stderr, _("Table for %s, %s not found.
Keep as-is\n"),
- rc_dev.drv_name, rc_dev.keytable_name);
- return 0;
- }
- if (debug)
- fprintf(stderr, _("Table for %s, %s is on %s file.\n"),
- rc_dev.drv_name, rc_dev.keytable_name,
- cur->fname);
- if (cur->fname[0] == '/' || ((cur->fname[0] == '.') &&
strchr(cur->fname, '/'))) {
- fname = cur->fname;
- rc = parse_keyfile(fname, );
- if (rc < 0) {
- fprintf(stderr, _("Can't load %s table\n"),
fname);
- return -1;
- }
- } else {
- fname = malloc(strlen(cur->fname) +
strlen(IR_KEYTABLE_USER_DIR) + 2);
- strcpy(fname, IR_KEYTABLE_USER_DIR);
- strcat(fname, "/");
- strcat(fname, cur->fname);
- rc = parse_keyfile(fname, );
- if (rc != 0) {
- fname = malloc(strlen(cur->fname) +
strlen(IR_KEYTABLE_SYSTEM_DIR) + 2);
- strcpy(fname, IR_KEYTABLE_SYSTEM_DIR);
+ fprintf(stderr, _("Table for %s, %s is on %s
file.\n"),
+ rc_dev.drv_name, rc_dev.keytable_name,
+ cur->fname);
+ if (cur->fname[0] == '/' || ((cur->fname[0] == '.') &&
strchr(cur->fname, '/'))) {
+ fname = cur->fname;
+ rc = parse_keyfile(fname, );
+ if (rc < 0) {
+ fprintf(stderr, _("Can't load %s
table\n"), fname);
+ return -1;
+ }
+ } else {
+ fname = malloc(strlen(cur->fname) +
strlen(IR_KEYTABLE_USER_DIR) + 2);
+ strcpy(fname, IR_KEYTABLE_USER_DIR);
strcat(fname, "/");
strcat(fname, cur->fname);
rc = parse_keyfile(fname, );
+ if (rc != 0) {
+ fname = malloc(strlen(cur->fname) +
strlen(IR_KEYTABLE_SYSTEM_DIR) + 2);
+ strcpy(fname, IR_KEYTABLE_SYSTEM_DIR);
+ strcat(fname, "/");
+ strcat(fname, cur->fname);
+ rc = parse_keyfile(fname, );
+ }
+ if (rc != 0) {
+ fprintf(stderr, _("Can't load %s table
from %s or %s\n"), cur->fname, IR_KEYTABLE_USER_DIR, IR_KEYTABLE_SYSTEM_DIR);
+ return -1;
+ }
}
- if (rc != 0) {
- fprintf(stderr, _("Can't load %s table from %s
or %s\n"), cur->fname, IR_KEYTABLE_USER_DIR, IR_KEYTABLE_SYSTEM_DIR);
+ if (!keytable) {
+ fprintf(stderr, _("Empty table %s\n"), fname);
Re: [PATCH 1/2] vb2: add waiting_in_dqbuf flag
On 11/16/2018 09:43 AM, Tomasz Figa wrote:
> Hi Hans,
>
> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil wrote:
>>
>> Calling VIDIOC_DQBUF can release the core serialization lock pointed to
>> by vb2_queue->lock if it has to wait for a new buffer to arrive.
>>
>> However, if userspace dup()ped the video device filehandle, then it is
>> possible to read or call DQBUF from two filehandles at the same time.
>>
>
> What side effects would reading have?
>
> As for another DQBUF in parallel, perhaps that's actually a valid
> operation that should be handled? I can imagine that one could want to
> have multiple threads dequeuing buffers as they become available, so
> that no dispatch thread is needed.
I think parallel DQBUFs can be done, but it has never been tested, nor
has vb2 been designed with that in mind. I also don't see the use-case
since if you have, say, two DQBUFs in parallel, then it will be random
which DQBUF gets which frame.
If we ever see a need for this, then that needs to be designed and tested
properly.
>
>> It is also possible to call REQBUFS from one filehandle while the other
>> is waiting for a buffer. This will remove all the buffers and reallocate
>> new ones. Removing all the buffers isn't the problem here (that's already
>> handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
>> aware that the buffers have changed.
>>
>> This is fixed by setting a flag whenever the lock is released while waiting
>> for a buffer to arrive. And checking the flag where needed so we can return
>> -EBUSY.
>
> Maybe it would make more sense to actually handle those side effects?
> Such waiting DQBUF would then just fail in the same way as if it
> couldn't get a buffer (or if it's blocking, just retry until a correct
> buffer becomes available?).
That sounds like a good idea, but it isn't.
With this patch you can't call REQBUFS to reallocate buffers while a thread
is waiting for a buffer.
If I allow this, then the problem moves to when the thread that called REQBUFS
calls DQBUF next. Since we don't allow multiple DQBUFs this second DQBUF will
mysteriously fail. If we DO allow multiple DQBUFs, then how does REQBUFS ensure
that only the DQBUF that relied on the old buffers is stopped?
It sounds nice, but the more I think about it, the more problems I see with it.
I think it is perfectly reasonable to expect REQBUFS to return EBUSY if some
thread is still waiting for a buffer.
That said, I think one test is missing in vb2_core_create_bufs: there too it
should check waiting_in_dqbuf if q->num_buffers == 0: it is possible to do
REQBUFS(0) followed by CREATE_BUFS() while another thread is waiting for a
buffer. CREATE_BUFS acts like REQBUFS(count >= 1) in that case.
Admittedly, that would require some extremely unfortunate scheduling, but
it is easy enough to check this.
Regards,
Hans
>
> Best regards,
> Tomasz
>
>>
>> Signed-off-by: Hans Verkuil
>> ---
>> .../media/common/videobuf2/videobuf2-core.c| 18 ++
>> include/media/videobuf2-core.h | 1 +
>> 2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
>> b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 03954c13024c..138223af701f 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -672,6 +672,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
>> vb2_memory memory,
>> return -EBUSY;
>> }
>>
>> + if (q->waiting_in_dqbuf && *count) {
>> + dprintk(1, "another dup()ped fd is waiting for a buffer\n");
>> + return -EBUSY;
>> + }
>> +
>> if (*count == 0 || q->num_buffers != 0 ||
>> (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
>> /*
>> @@ -1624,6 +1629,11 @@ static int __vb2_wait_for_done_vb(struct vb2_queue
>> *q, int nonblocking)
>> for (;;) {
>> int ret;
>>
>> + if (q->waiting_in_dqbuf) {
>> + dprintk(1, "another dup()ped fd is waiting for a
>> buffer\n");
>> + return -EBUSY;
>> + }
>> +
>> if (!q->streaming) {
>> dprintk(1, "streaming off, will not wait for
>> buffers\n");
>> return -EINVAL;
>> @@ -1651,6 +1661,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q,
>> int nonblocking)
>> return -EAGAIN;
>> }
>>
>> + q->waiting_in_dqbuf = 1;
>> /*
>> * We are streaming and blocking, wait for another buffer to
>> * become ready or for streamoff. Driver's lock is released
>> to
>> @@ -1671,6 +1682,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q,
>> int nonblocking)
>> * the locks or return an error if one occurred.
>> */
>>
Re: [PATCHv2 0/9] vb2/cedrus: add tag support
Hi Hans, On Wed, Nov 14, 2018 at 10:47 PM Hans Verkuil wrote: > > From: Hans Verkuil > > As was discussed here (among other places): > > https://lkml.org/lkml/2018/10/19/440 > > using capture queue buffer indices to refer to reference frames is > not a good idea. A better idea is to use a 'tag' where the > application can assign a u64 tag to an output buffer, which is then > copied to the capture buffer(s) derived from the output buffer. > Thanks for the patches. Please see my comments below. > A u64 is chosen since this allows userspace to also use pointers to > internal structures as 'tag'. > I think this is not true anymore in this version. > The first three patches add core tag support, the next patch document > the tag support, then a new helper function is added to v4l2-mem2mem.c > to easily copy data from a source to a destination buffer that drivers > can use. > > Next a new supports_tags vb2_queue flag is added to indicate that > the driver supports tags. Ideally this should not be necessary, but > that would require that all m2m drivers are converted to using the > new helper function introduced in the previous patch. That takes more > time then I have now since we want to get this in for 4.20. > > Finally the vim2m, vicodec and cedrus drivers are converted to support > tags. > > I also removed the 'pad' fields from the mpeg2 control structs (it > should never been added in the first place) and aligned the structs > to a u32 boundary (u64 for the tag values). u32 in this version > > Note that this might change further (Paul suggested using bitfields). > > Also note that the cedrus code doesn't set the sequence counter, that's > something that should still be added before this driver can be moved > out of staging. > > Note: if no buffer is found for a certain tag, then the dma address > is just set to 0. That happened before as well with invalid buffer > indices. This should be checked in the driver! Note that DMA address 0 may as well be a valid address. Should we have another way of signaling that? > > The previous RFC series was tested successfully with the cedrus driver. > > Regards, > > Hans > > Changes since v1: > > - changed to a u32 tag. Using a 64 bit tag was overly complicated due > to the bad layout of the v4l2_buffer struct, and there is no real > need for it by applications. I hope these won't become famous last words. :) For Chromium it should be okay indeed. Since we've been thinking about a redesign of the buffer struct, perhaps we can live with u32 for now and we can design the new struct to handle u64 nicely? Best regards, Tomasz
Re: [PATCH 2/2] vb2: don't allow queueing buffers when canceling queue
On Fri, Nov 16, 2018 at 5:42 PM Hans Verkuil wrote:
>
> On 11/16/2018 09:34 AM, Tomasz Figa wrote:
> > Hi Hans,
> >
> > On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil wrote:
> >>
> >> Calling the stop_streaming op can release the core serialization lock
> >> pointed to by vb2_queue->lock if it has to wait for buffers to finish.
> >> An example of that behavior is the vivid driver.
> >
> > Why would vb2_queue->lock have to be released to wait for buffer to
> > finish? The drivers I worked with never had to do anything like that.
>
> Actually, they all do. It's done through the wait_prepare/finish callbacks
> and by setting those to vb2_ops_wait_prepare/finish.
>
> If you don't, then while one thread is waiting for a buffer to arrive,
> another thread cannot queue a new buffer since it will be serialized by
> queue->lock.
>
> v4l2-compliance even tests for this.
Why would you need the userspace to queue more buffers when you're
stopping the queue?
>
> >
> >>
> >> However, if userspace dup()ped the video device filehandle, then it is
> >> possible to stop streaming on one filehandle and call read/write or
> >> VIDIOC_QBUF from the other.
> >
> > How about other ioctls? I can imagine at least STREAMON could be
> > called at the same time too, but not sure if it would have any side
> > effects.
>
> STREAMON would return an error since q->streaming is still set while
> in the stop_streaming callback.
>
> So that combination is safe.
>
Okay, thanks. I'm still slightly worried that this approach with a
flag makes it possible to miss some non-trivial cases, though...
> Regards,
>
> Hans
>
> >
> > Best regards,
> > Tomasz
> >
> >>
> >> This is fixed by setting a flag whenever stop_streaming is called and
> >> checking the flag where needed so we can return -EBUSY.
> >>
> >> Signed-off-by: Hans Verkuil
> >> Reported-by: syzbot+736c3aae4af7b50d9...@syzkaller.appspotmail.com
> >> ---
> >> drivers/media/common/videobuf2/videobuf2-core.c | 14 +-
> >> include/media/videobuf2-core.h | 1 +
> >> 2 files changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> >> b/drivers/media/common/videobuf2/videobuf2-core.c
> >> index 138223af701f..560577321fe7 100644
> >> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> >> @@ -1503,6 +1503,10 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
> >> index, void *pb,
> >> dprintk(1, "fatal error occurred on queue\n");
> >> return -EIO;
> >> }
> >> + if (q->in_stop_streaming) {
> >> + dprintk(1, "stop_streaming is called\n");
> >> + return -EBUSY;
> >> + }
> >>
> >> vb = q->bufs[index];
> >>
> >> @@ -1834,8 +1838,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> >> * Tell driver to stop all transactions and release all queued
> >> * buffers.
> >> */
> >> - if (q->start_streaming_called)
> >> + if (q->start_streaming_called) {
> >> + q->in_stop_streaming = 1;
> >> call_void_qop(q, stop_streaming, q);
> >> + q->in_stop_streaming = 0;
> >> + }
> >>
> >> /*
> >> * If you see this warning, then the driver isn't cleaning up
> >> properly
> >> @@ -2565,6 +2572,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue
> >> *q, char __user *data, size_
> >> return -EBUSY;
> >> }
> >>
> >> + if (q->in_stop_streaming) {
> >> + dprintk(3, "stop_streaming is called\n");
> >> + return -EBUSY;
> >> + }
> >> +
> >> /*
> >> * Initialize emulator on first call.
> >> */
> >> diff --git a/include/media/videobuf2-core.h
> >> b/include/media/videobuf2-core.h
> >> index 613f22910174..5a3d3ada5940 100644
> >> --- a/include/media/videobuf2-core.h
> >> +++ b/include/media/videobuf2-core.h
> >> @@ -585,6 +585,7 @@ struct vb2_queue {
> >> unsigned interror:1;
> >> unsigned intwaiting_for_buffers:1;
> >> unsigned intwaiting_in_dqbuf:1;
> >> + unsigned intin_stop_streaming:1;
> >> unsigned intis_multiplanar:1;
> >> unsigned intis_output:1;
> >> unsigned intcopy_timestamp:1;
> >> --
> >> 2.19.1
> >>
>
Re: [PATCH 1/2] vb2: add waiting_in_dqbuf flag
Hi Hans,
On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil wrote:
>
> Calling VIDIOC_DQBUF can release the core serialization lock pointed to
> by vb2_queue->lock if it has to wait for a new buffer to arrive.
>
> However, if userspace dup()ped the video device filehandle, then it is
> possible to read or call DQBUF from two filehandles at the same time.
>
What side effects would reading have?
As for another DQBUF in parallel, perhaps that's actually a valid
operation that should be handled? I can imagine that one could want to
have multiple threads dequeuing buffers as they become available, so
that no dispatch thread is needed.
> It is also possible to call REQBUFS from one filehandle while the other
> is waiting for a buffer. This will remove all the buffers and reallocate
> new ones. Removing all the buffers isn't the problem here (that's already
> handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
> aware that the buffers have changed.
>
> This is fixed by setting a flag whenever the lock is released while waiting
> for a buffer to arrive. And checking the flag where needed so we can return
> -EBUSY.
Maybe it would make more sense to actually handle those side effects?
Such waiting DQBUF would then just fail in the same way as if it
couldn't get a buffer (or if it's blocking, just retry until a correct
buffer becomes available?).
Best regards,
Tomasz
>
> Signed-off-by: Hans Verkuil
> ---
> .../media/common/videobuf2/videobuf2-core.c| 18 ++
> include/media/videobuf2-core.h | 1 +
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index 03954c13024c..138223af701f 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -672,6 +672,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
> vb2_memory memory,
> return -EBUSY;
> }
>
> + if (q->waiting_in_dqbuf && *count) {
> + dprintk(1, "another dup()ped fd is waiting for a buffer\n");
> + return -EBUSY;
> + }
> +
> if (*count == 0 || q->num_buffers != 0 ||
> (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
> /*
> @@ -1624,6 +1629,11 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q,
> int nonblocking)
> for (;;) {
> int ret;
>
> + if (q->waiting_in_dqbuf) {
> + dprintk(1, "another dup()ped fd is waiting for a
> buffer\n");
> + return -EBUSY;
> + }
> +
> if (!q->streaming) {
> dprintk(1, "streaming off, will not wait for
> buffers\n");
> return -EINVAL;
> @@ -1651,6 +1661,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q,
> int nonblocking)
> return -EAGAIN;
> }
>
> + q->waiting_in_dqbuf = 1;
> /*
> * We are streaming and blocking, wait for another buffer to
> * become ready or for streamoff. Driver's lock is released to
> @@ -1671,6 +1682,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q,
> int nonblocking)
> * the locks or return an error if one occurred.
> */
> call_void_qop(q, wait_finish, q);
> + q->waiting_in_dqbuf = 0;
> if (ret) {
> dprintk(1, "sleep was interrupted\n");
> return ret;
> @@ -2547,6 +2559,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue
> *q, char __user *data, size_
> if (!data)
> return -EINVAL;
>
> + if (q->waiting_in_dqbuf) {
> + dprintk(3, "another dup()ped fd is %s\n",
> + read ? "reading" : "writing");
> + return -EBUSY;
> + }
> +
> /*
> * Initialize emulator on first call.
> */
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index e86981d615ae..613f22910174 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -584,6 +584,7 @@ struct vb2_queue {
> unsigned intstart_streaming_called:1;
> unsigned interror:1;
> unsigned intwaiting_for_buffers:1;
> + unsigned intwaiting_in_dqbuf:1;
> unsigned intis_multiplanar:1;
> unsigned intis_output:1;
> unsigned intcopy_timestamp:1;
> --
> 2.19.1
>
Re: [PATCH 2/2] vb2: don't allow queueing buffers when canceling queue
On 11/16/2018 09:34 AM, Tomasz Figa wrote:
> Hi Hans,
>
> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil wrote:
>>
>> Calling the stop_streaming op can release the core serialization lock
>> pointed to by vb2_queue->lock if it has to wait for buffers to finish.
>> An example of that behavior is the vivid driver.
>
> Why would vb2_queue->lock have to be released to wait for buffer to
> finish? The drivers I worked with never had to do anything like that.
Actually, they all do. It's done through the wait_prepare/finish callbacks
and by setting those to vb2_ops_wait_prepare/finish.
If you don't, then while one thread is waiting for a buffer to arrive,
another thread cannot queue a new buffer since it will be serialized by
queue->lock.
v4l2-compliance even tests for this.
>
>>
>> However, if userspace dup()ped the video device filehandle, then it is
>> possible to stop streaming on one filehandle and call read/write or
>> VIDIOC_QBUF from the other.
>
> How about other ioctls? I can imagine at least STREAMON could be
> called at the same time too, but not sure if it would have any side
> effects.
STREAMON would return an error since q->streaming is still set while
in the stop_streaming callback.
So that combination is safe.
Regards,
Hans
>
> Best regards,
> Tomasz
>
>>
>> This is fixed by setting a flag whenever stop_streaming is called and
>> checking the flag where needed so we can return -EBUSY.
>>
>> Signed-off-by: Hans Verkuil
>> Reported-by: syzbot+736c3aae4af7b50d9...@syzkaller.appspotmail.com
>> ---
>> drivers/media/common/videobuf2/videobuf2-core.c | 14 +-
>> include/media/videobuf2-core.h | 1 +
>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
>> b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 138223af701f..560577321fe7 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1503,6 +1503,10 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
>> index, void *pb,
>> dprintk(1, "fatal error occurred on queue\n");
>> return -EIO;
>> }
>> + if (q->in_stop_streaming) {
>> + dprintk(1, "stop_streaming is called\n");
>> + return -EBUSY;
>> + }
>>
>> vb = q->bufs[index];
>>
>> @@ -1834,8 +1838,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>> * Tell driver to stop all transactions and release all queued
>> * buffers.
>> */
>> - if (q->start_streaming_called)
>> + if (q->start_streaming_called) {
>> + q->in_stop_streaming = 1;
>> call_void_qop(q, stop_streaming, q);
>> + q->in_stop_streaming = 0;
>> + }
>>
>> /*
>> * If you see this warning, then the driver isn't cleaning up
>> properly
>> @@ -2565,6 +2572,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue
>> *q, char __user *data, size_
>> return -EBUSY;
>> }
>>
>> + if (q->in_stop_streaming) {
>> + dprintk(3, "stop_streaming is called\n");
>> + return -EBUSY;
>> + }
>> +
>> /*
>> * Initialize emulator on first call.
>> */
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index 613f22910174..5a3d3ada5940 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -585,6 +585,7 @@ struct vb2_queue {
>> unsigned interror:1;
>> unsigned intwaiting_for_buffers:1;
>> unsigned intwaiting_in_dqbuf:1;
>> + unsigned intin_stop_streaming:1;
>> unsigned intis_multiplanar:1;
>> unsigned intis_output:1;
>> unsigned intcopy_timestamp:1;
>> --
>> 2.19.1
>>
Re: [PATCH 2/2] vb2: don't allow queueing buffers when canceling queue
Hi Hans,
On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil wrote:
>
> Calling the stop_streaming op can release the core serialization lock
> pointed to by vb2_queue->lock if it has to wait for buffers to finish.
> An example of that behavior is the vivid driver.
Why would vb2_queue->lock have to be released to wait for buffer to
finish? The drivers I worked with never had to do anything like that.
>
> However, if userspace dup()ped the video device filehandle, then it is
> possible to stop streaming on one filehandle and call read/write or
> VIDIOC_QBUF from the other.
How about other ioctls? I can imagine at least STREAMON could be
called at the same time too, but not sure if it would have any side
effects.
Best regards,
Tomasz
>
> This is fixed by setting a flag whenever stop_streaming is called and
> checking the flag where needed so we can return -EBUSY.
>
> Signed-off-by: Hans Verkuil
> Reported-by: syzbot+736c3aae4af7b50d9...@syzkaller.appspotmail.com
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 14 +-
> include/media/videobuf2-core.h | 1 +
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index 138223af701f..560577321fe7 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1503,6 +1503,10 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
> index, void *pb,
> dprintk(1, "fatal error occurred on queue\n");
> return -EIO;
> }
> + if (q->in_stop_streaming) {
> + dprintk(1, "stop_streaming is called\n");
> + return -EBUSY;
> + }
>
> vb = q->bufs[index];
>
> @@ -1834,8 +1838,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> * Tell driver to stop all transactions and release all queued
> * buffers.
> */
> - if (q->start_streaming_called)
> + if (q->start_streaming_called) {
> + q->in_stop_streaming = 1;
> call_void_qop(q, stop_streaming, q);
> + q->in_stop_streaming = 0;
> + }
>
> /*
> * If you see this warning, then the driver isn't cleaning up properly
> @@ -2565,6 +2572,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue
> *q, char __user *data, size_
> return -EBUSY;
> }
>
> + if (q->in_stop_streaming) {
> + dprintk(3, "stop_streaming is called\n");
> + return -EBUSY;
> + }
> +
> /*
> * Initialize emulator on first call.
> */
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 613f22910174..5a3d3ada5940 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -585,6 +585,7 @@ struct vb2_queue {
> unsigned interror:1;
> unsigned intwaiting_for_buffers:1;
> unsigned intwaiting_in_dqbuf:1;
> + unsigned intin_stop_streaming:1;
> unsigned intis_multiplanar:1;
> unsigned intis_output:1;
> unsigned intcopy_timestamp:1;
> --
> 2.19.1
>
Re: media: ov8856: Add support for OV8856 sensor
Hi Ben, On Thu, Nov 08, 2018 at 11:41:46AM +0800, Ben Kao wrote: > This patch adds driver for Omnivision's ov8856 sensor, > the driver supports following features: > > - manual exposure/gain(analog and digital) control support > - two link frequencies > - VBLANK/HBLANK support > - test pattern support > - media controller support > - runtime pm support > - supported resolutions > + 3280x2464 at 30FPS > + 1640x1232 at 30FPS > > Signed-off-by: Ben Kao I just realised the driver is missing the MAINTAINERS entry. Could you provide one? Just the diff is fine, I'll then squash it to the patch. Thanks. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
