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: Fri Sep 21 04:00:16 CEST 2018 media-tree git hash:985cdcb08a0488558d1005139596b64d73bee267 media_build git hash: 44385b9c61ecc27059a651885895c8ea09cd4179 v4l-utils git hash: d977e20a45a03543c4bdfeb3aef5211446de7398 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.17.0-3-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK Check COMPILE_TEST: OK linux-3.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.119-i686: OK linux-3.18.119-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.152-i686: OK linux-4.4.152-x86_64: OK linux-4.5.7-i686: OK linux-4.5.7-x86_64: OK linux-4.6.7-i686: OK linux-4.6.7-x86_64: OK linux-4.7.10-i686: OK linux-4.7.10-x86_64: OK linux-4.8.17-i686: OK linux-4.8.17-x86_64: OK linux-4.9.124-i686: OK linux-4.9.124-x86_64: OK linux-4.10.17-i686: OK linux-4.10.17-x86_64: OK linux-4.11.12-i686: OK linux-4.11.12-x86_64: OK linux-4.12.14-i686: OK linux-4.12.14-x86_64: OK linux-4.13.16-i686: OK linux-4.13.16-x86_64: OK linux-4.14.67-i686: OK linux-4.14.67-x86_64: OK linux-4.15.18-i686: OK linux-4.15.18-x86_64: OK linux-4.16.18-i686: OK linux-4.16.18-x86_64: OK linux-4.17.19-i686: OK linux-4.17.19-x86_64: OK linux-4.18.5-i686: OK linux-4.18.5-x86_64: OK linux-4.19-rc1-i686: OK linux-4.19-rc1-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Friday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Friday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
Re: [PATCH v5] media: add imx319 camera sensor driver
Ack, I will add more explanation into the code.
On 09/19/2018 12:11 PM, Tomasz Figa wrote:
> Hi Bingbu,
>
> On Mon, Sep 17, 2018 at 2:53 PM wrote:
> [snip]
>> +static int imx319_update_digital_gain(struct imx319 *imx319, u32 d_gain)
>> +{
>> + int ret;
>> +
>> + ret = imx319_write_reg(imx319, IMX319_REG_DPGA_USE_GLOBAL_GAIN, 1,
>> 1);
>> + if (ret)
>> + return ret;
>> +
>> + /* Digital gain = (d_gain & 0xFF00) + (d_gain & 0xFF)/256 times */
> What's the unit here?
>
> Is the equation above really correct? The range, besides ~0, would be
> from 256.0 to 65280 + 255/256, which sounds strange.
>
>> + return imx319_write_reg(imx319, IMX319_REG_DIG_GAIN_GLOBAL, 2,
>> d_gain);
>> +}
>> +
>> +static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct imx319 *imx319 = container_of(ctrl->handler,
>> +struct imx319, ctrl_handler);
>> + struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
>> + s64 max;
>> + int ret;
>> +
>> + /* Propagate change of current control to all related controls */
>> + switch (ctrl->id) {
>> + case V4L2_CID_VBLANK:
>> + /* Update max exposure while meeting expected vblanking */
>> + max = imx319->cur_mode->height + ctrl->val - 18;
>> + __v4l2_ctrl_modify_range(imx319->exposure,
>> +imx319->exposure->minimum,
>> +max, imx319->exposure->step, max);
>> + break;
>> + }
>> +
>> + /*
>> +* Applying V4L2 control value only happens
>> +* when power is up for streaming
>> +*/
>> + if (pm_runtime_get_if_in_use(&client->dev) == 0)
> nit: if (!pm_runtime_get_if_in_use(&client->dev)
>
>> + return 0;
>> +
> [snip]
>> +/* Initialize control handlers */
>> +static int imx319_init_controls(struct imx319 *imx319)
>> +{
>> + struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
>> + struct v4l2_ctrl_handler *ctrl_hdlr;
>> + s64 exposure_max;
>> + s64 vblank_def;
>> + s64 vblank_min;
>> + s64 hblank;
>> + s64 pixel_rate;
>> + const struct imx319_mode *mode;
>> + int ret;
>> +
>> + ctrl_hdlr = &imx319->ctrl_handler;
>> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
>> + if (ret)
>> + return ret;
>> +
>> + ctrl_hdlr->lock = &imx319->mutex;
>> + imx319->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>> &imx319_ctrl_ops,
>> + V4L2_CID_LINK_FREQ, 0, 0,
>> +
>> imx319->pdata->link_freqs);
>> + if (imx319->link_freq)
>> + imx319->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +
>> + /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
>> + pixel_rate = (imx319->link_def_freq * 2 * 4) / 10;
>> + /* By default, PIXEL_RATE is read only */
>> + imx319->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops,
>> + V4L2_CID_PIXEL_RATE,
>> pixel_rate,
>> + pixel_rate, 1, pixel_rate);
>> +
>> + /* Initialze vblank/hblank/exposure parameters based on current mode
>> */
>> + mode = imx319->cur_mode;
>> + vblank_def = mode->fll_def - mode->height;
>> + vblank_min = mode->fll_min - mode->height;
>> + imx319->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops,
>> + V4L2_CID_VBLANK, vblank_min,
>> + IMX319_FLL_MAX - mode->height,
>> + 1, vblank_def);
>> +
>> + hblank = mode->llp - mode->width;
>> + imx319->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops,
>> + V4L2_CID_HBLANK, hblank, hblank,
>> + 1, hblank);
>> + if (imx319->hblank)
>> + imx319->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +
>> + exposure_max = mode->fll_def - 18;
>> + imx319->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops,
>> +V4L2_CID_EXPOSURE,
>> +IMX319_EXPOSURE_MIN,
>> exposure_max,
>> +IMX319_EXPOSURE_STEP,
>> +IMX319_EXPOSURE_DEFAULT);
> Please explain how to interpret the exposure value in a comment.
>
>> +
>> + imx319->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops,
>> + V4L2_CID_HFLIP, 0, 1, 1, 0);
>> + imx319->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops,
>> + V4L2_CID_VFLIP, 0, 1, 1, 0);
Re: [PATCH v5] media: add imx319 camera sensor driver
On 09/18/2018 05:49 PM, Tomasz Figa wrote:
> Hi Bingbu,
>
> On Mon, Sep 17, 2018 at 2:53 PM wrote:
>> From: Bingbu Cao
>>
>> Add a v4l2 sub-device driver for the Sony imx319 image sensor.
>> This is a camera sensor using the i2c bus for control and the
>> csi-2 bus for data.
> Please see my comments inline. Also, I'd appreciate being CCed on
> related work in the future.
Ack.
Sorry, will add you into the cc-list.
>
> [snip]
>> +
>> +static const char * const imx319_test_pattern_menu[] = {
>> + "Disabled",
>> + "100% color bars",
>> + "Solid color",
>> + "Fade to gray color bars",
>> + "PN9"
>> +};
>> +
>> +static const int imx319_test_pattern_val[] = {
>> + IMX319_TEST_PATTERN_DISABLED,
>> + IMX319_TEST_PATTERN_COLOR_BARS,
>> + IMX319_TEST_PATTERN_SOLID_COLOR,
>> + IMX319_TEST_PATTERN_GRAY_COLOR_BARS,
>> + IMX319_TEST_PATTERN_PN9,
>> +};
> This array is not needed. All the entries are equal to corresponding
> indices, i.e. the array is equivalent to { 0, 1, 2, 3, 4 }. We can use
> ctrl->val directly.
Ack.
> [snip]
>
>> +/* Write a list of registers */
>> +static int imx319_write_regs(struct imx319 *imx319,
>> + const struct imx319_reg *regs, u32 len)
>> +{
>> + struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
>> + int ret;
>> + u32 i;
>> +
>> + for (i = 0; i < len; i++) {
>> + ret = imx319_write_reg(imx319, regs[i].address, 1,
>> regs[i].val);
>> + if (ret) {
>> + dev_err_ratelimited(&client->dev,
>> +
> Hmm, the message is clipped here. Let me see if it's something with my
> email client...
The code here:
1827 for (i = 0; i < len; i++) {
1828 ret = imx319_write_reg(imx319, regs[i].address, 1, regs[i].val);
1829 if (ret) {
1830 dev_err_ratelimited(&client->dev,
1831 "write reg 0x%4.4x return err %d",
1832 regs[i].address, ret);
1833 return ret;
1834 }
1835 } Same as the code shown on your client?
>
> Best regards,
> Tomasz
>
Re: [PATCH v5] media: add imx319 camera sensor driver
Ack. On 09/17/2018 07:34 PM, Sakari Ailus wrote: > Hi Bingbu, > > Thanks for the update! A few more small comments, I think we're done after > these. > > On Mon, Sep 17, 2018 at 01:57:52PM +0800, bingbu@intel.com wrote: >> From: Bingbu Cao >> >> Add a v4l2 sub-device driver for the Sony imx319 image sensor. >> This is a camera sensor using the i2c bus for control and the >> csi-2 bus for data. >> >> This driver supports following features: >> - manual exposure and analog/digital gain control support >> - vblank/hblank control support >> - 4 test patterns control support >> - vflip/hflip control support (will impact the output bayer order) >> - support following resolutions: >> - 3264x2448, 3280x2464 @ 30fps >> - 1936x1096, 1920x1080 @ 60fps >> - 1640x1232, 1640x922, 1296x736, 1280x720 @ 120fps >> - support 4 bayer orders output (via change v/hflip) >> - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 >> >> Signed-off-by: Bingbu Cao >> Signed-off-by: Tianshu Qiu >> >> --- >> >> This patch is based on sakari's media-tree git: >> https://git.linuxtv.org/sailus/media_tree.git/log/?h=for-4.20-1 >> >> Changes from v4 to v5: >> - use single PLL for all internal clocks >> - change link frequency to 482.4MHz >> - adjust frame timing for 2x2 binning modes >>and enlarge frame readout time >> - get CSI-2 link frequencies and external clock >>from firmware >> - use unlocked __v4l2_ctrl_grab() with change from: >>https://git.linuxtv.org/sailus/media_tree.git/commit/?h=unlocked-ctrl-grab >> >> Changes since v1: >> - fix some coding style issues - line breaks >> - add v4l2_ctrl_grab() to prevent v/hflip change >>during streaming >> - add v4l2 ctrl event (un)subscribe support >> - add more info into commit message >> >> --- >> --- >> MAINTAINERS|7 + >> drivers/media/i2c/Kconfig | 11 + >> drivers/media/i2c/Makefile |1 + >> drivers/media/i2c/imx319.c | 2524 >> >> 4 files changed, 2543 insertions(+) >> create mode 100644 drivers/media/i2c/imx319.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index a5b256b25905..abc4abb6f83c 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -13530,6 +13530,13 @@ S: Maintained >> F: drivers/media/i2c/imx274.c >> F: Documentation/devicetree/bindings/media/i2c/imx274.txt >> >> +SONY IMX319 SENSOR DRIVER >> +M: Bingbu Cao >> +L: linux-media@vger.kernel.org >> +T: git git://linuxtv.org/media_tree.git >> +S: Maintained >> +F: drivers/media/i2c/imx319.c >> + >> SONY MEMORYSTICK CARD SUPPORT >> M: Alex Dubov >> W: http://tifmxx.berlios.de/ >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig >> index bfdb494686bf..603ac087975b 100644 >> --- a/drivers/media/i2c/Kconfig >> +++ b/drivers/media/i2c/Kconfig >> @@ -614,6 +614,17 @@ config VIDEO_IMX274 >>This is a V4L2 sensor driver for the Sony IMX274 >>CMOS image sensor. >> >> +config VIDEO_IMX319 >> +tristate "Sony IMX319 sensor support" >> +depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API >> +depends on MEDIA_CAMERA_SUPPORT >> +help >> + This is a Video4Linux2 sensor driver for the Sony >> + IMX319 camera. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called imx319. >> + >> config VIDEO_OV2640 >> tristate "OmniVision OV2640 sensor support" >> depends on VIDEO_V4L2 && I2C >> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile >> index a94eb03d10d4..d10b438577be 100644 >> --- a/drivers/media/i2c/Makefile >> +++ b/drivers/media/i2c/Makefile >> @@ -108,5 +108,6 @@ obj-$(CONFIG_VIDEO_OV2659) += ov2659.o >> obj-$(CONFIG_VIDEO_TC358743)+= tc358743.o >> obj-$(CONFIG_VIDEO_IMX258) += imx258.o >> obj-$(CONFIG_VIDEO_IMX274) += imx274.o >> +obj-$(CONFIG_VIDEO_IMX319) += imx319.o >> >> obj-$(CONFIG_SDR_MAX2175) += max2175.o >> diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c >> new file mode 100644 >> index ..43c28c701431 >> --- /dev/null >> +++ b/drivers/media/i2c/imx319.c >> @@ -0,0 +1,2524 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (C) 2018 Intel Corporation >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define IMX319_REG_MODE_SELECT 0x0100 >> +#define IMX319_MODE_STANDBY 0x00 >> +#define IMX319_MODE_STREAMING 0x01 >> + >> +/* Chip ID */ >> +#define IMX319_REG_CHIP_ID 0x0016 >> +#define IMX319_CHIP_ID 0x0319 >> + >> +/* V_TIMING internal */ >> +#define IMX319_REG_FLL 0x0340 >> +#define IMX319_FLL_MAX 0x >> + >> +/* Exposure control */ >> +#define IMX319_REG_EXPOSURE 0x0202 >> +#define IMX319_EXPOSURE_MIN 1 >> +#define IMX319_EXPOSURE_STEP1 >> +#define IMX319_EXPOSURE_DEFA
Re: [PATCH] vidioc-dqevent.rst: clarify V4L2_EVENT_SRC_CH_RESOLUTION
On Mon, Sep 17, 2018 at 01:37:18PM +0200, Hans Verkuil wrote: > Clarify that when you receive V4L2_EVENT_SOURCE_CHANGE with flag > V4L2_EVENT_SRC_CH_RESOLUTION set, and the new resolution appears > identical to the old resolution, then you still must restart the > streaming I/O. > > Signed-off-by: Hans Verkuil Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
HI
On Thu, Sep 20, 2018 at 11:13 PM Laurent Pinchart
wrote:
>
> Hi Sakari,
>
> On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> > On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > > [snip]
> > >
> > > > +
> > > > +/* Digital gain control */
> > > > +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> > > > +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> > > > +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> > > > +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> > > > +#define IMX208_DGTL_GAIN_MIN 0
> > > > +#define IMX208_DGTL_GAIN_MAX 4096
> > > > +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> > > > +#define IMX208_DGTL_GAIN_STEP 1
> > > > +
> > >
> > > [snip]
> > >
> > > > +/* Initialize control handlers */
> > > > +static int imx208_init_controls(struct imx208 *imx208)
> > > > +{
> > >
> > > [snip]
> > >
> > > > + v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops,
> > > > V4L2_CID_DIGITAL_GAIN, + IMX208_DGTL_GAIN_MIN,
> > > > IMX208_DGTL_GAIN_MAX, + IMX208_DGTL_GAIN_STEP,
> > > > + IMX208_DGTL_GAIN_DEFAULT);
> > >
> > > We have a problem here. The sensor supports only a discrete range of
> > > values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> > > fixed point). This makes it possible for the userspace to set values
> > > that are not allowed by the sensor specification and also leaves no
> > > way to enumerate the supported values.
> > >
> > > I can see two solutions here:
> > >
> > > 1) Define the control range from 0 to 4 and treat it as an exponent of
> > > 2, so that the value for the sensor becomes (1 << val) * 256.
> > > (Suggested by Sakari offline.)
> > >
> > > This approach has the problem of losing the original unit (and scale)
> > > of the value.
> >
> > I'd like to add that this is not a property of the proposed solution.
> >
> > Rather, the above needs to be accompanied by additional information
> > provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as well as
> > other information such as whether the control is linear or exponential (as
> > in this case).
> >
> > > 2) Use an integer menu control, which reports only the supported
> > > discrete values - {1, 2, 4, 8, 16}.
> > >
> > > With this approach, userspace can enumerate the real gain values, but
> > > we would either need to introduce a new control (e.g.
> > > V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> > > register V4L2_CID_DIGITAL_GAIN as an integer menu.
> >
> > New controls in V4L2 are, for the most part, created when there's something
> > new to control. The documentation of some controls (similar to e.g. gain)
> > documents a unit as well as a prefix but that's the case only because
> > there's been no way to tell the unit or prefix otherwise in the API.
> >
> > An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not entirely
> > sure how they came to be though. An accident is a possibility as far as I
> > see.
>
> If I remember correctly I introduced the absolute variant for the UVC driver
> (even though git blame points to Brandon Philips). I don't really remember why
> though.
>
> > Controls that have a documented unit use that unit --- as long as that's
> > the unit used by the hardware. If it's not, it tends to be that another
> > unit is used but the user space has currently no way of knowing this. And
> > the digital gain control is no exception to this.
> >
> > So if we want to improve the user space's ability to be informed how the
> > control values reflect to device configuration, we do need to provide more
> > information to the user space. One way to do that would be through
> > VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of reserved fields ---
> > just for purposes such as this one.
>
> I don't think we can come up with a good way to expose arbitrary mathematical
> formulas through an ioctl. In my opinion the question is how far we want to
> go, how precise we need to be.
>
> > > Any opinions or better ideas?
My 0.02 DKK. On a similar situation, where userspace was running a
close loop calibration:
We have implemented two extra control: eposure_next exposure_pre that
tell us which one is the next value. Perhaps we could embebed such
functionality in QUERY_EXT_CTRL.
Cheers
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
--
Ricardo Ribalda
Re: [RESEND PATCH 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names
Hi Sylwester, On Thu, Sep 20, 2018 at 11:01:26PM +0200, Sylwester Nawrocki wrote: > Hi Sakari, > > On 09/16/2018 12:52 AM, Sakari Ailus wrote: > > v4l2_i2c_subdev_init() sets the name of the sub-devices (as well as > > entities) to what is fairly certainly known to be unique in the system, > > even if there were more devices of the same kind. > > > > These drivers (m5mols, noon010pc30, ov9650, s5c73m3, s5k4ecgx, s5k6aa) set > > the name to the name of the driver or the module while omitting the > > device's I²C address and bus, leaving the devices with a static name and > > effectively limiting the number of such devices in a media device to 1. > > > > Address this by using the name set by the V4L2 framework. > > > > Signed-off-by: Sakari Ailus > > Reviewed-by: Akinobu Mita (ov9650) > > I'm not against this patch but please don't expect an ack from me as this > patch breaks existing user space code, scripts using media-ctl, etc. will > need to be updated after kernel upgrade. I'm mostly concerned about ov9650 > as other drivers are likely only used by Samsung or are obsoleted. That is a fair point and also why I originally sent the patch out as RFC... I checked that OV9650 is around 14 years (!) old now, so it's unlikely to appear in modern hardware in dual configurations. I think this patch thus probably causes more trouble than it has chances of fixing anything. I'll submit one that adds a note that the convention is not to be used in new drivers instead --- unless someone strongly feels this patch really should be merged. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v3 1/1] v4l: event: Prevent freeing event subscriptions while accessed
Hi Sakari,
Thank you for the patch.
On Friday, 14 September 2018 14:03:01 EEST Sakari Ailus wrote:
> The event subscriptions are added to the subscribed event list while
> holding a spinlock, but that lock is subsequently released while still
> accessing the subscription object. This makes it possible to unsubscribe
> the event --- and freeing the subscription object's memory --- while
> the subscription object is simultaneously accessed.
>
> Prevent this by adding a mutex to serialise the event subscription and
> unsubscription. This also gives a guarantee to the callback ops that the
> add op has returned before the del op is called.
>
> This change also results in making the elems field less special:
> subscriptions are only added to the event list once they are fully
> initialised.
>
> Signed-off-by: Sakari Ailus
> Reviewed-by: Hans Verkuil
It wasn't immediately clear to me that the !sev->elems check can be removed
because the subscriptions are *now* only added to the event list once they are
fully initialized, I thought the sentence documented the current
implementation. After realizing that,
Reviewed-by: Laurent Pinchart
> ---
> since v1:
>
> - Call the mutex field subscribe_lock instead.
>
> - Move the field that is now subscribe_lock above the subscribed field the
> write access to which it serialises.
>
> - Improve documentation of the subscribe_lock field.
>
> since v2:
>
> - Acquire spinlock for the duration of list_add() in v4l2_event_subscribe().
>
> - Remove a redundant comment in the same place.
>
> drivers/media/v4l2-core/v4l2-event.c | 38 +
> drivers/media/v4l2-core/v4l2-fh.c| 2 ++
> include/media/v4l2-fh.h | 4
> 3 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-event.c
> b/drivers/media/v4l2-core/v4l2-event.c index 127fe6eb91d9..a3ef1f50a4b3
> 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -115,14 +115,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
> const struct v4l2_event *e if (sev == NULL)
> return;
>
> - /*
> - * If the event has been added to the fh->subscribed list, but its
> - * add op has not completed yet elems will be 0, treat this as
> - * not being subscribed.
> - */
> - if (!sev->elems)
> - return;
> -
> /* Increase event sequence number on fh. */
> fh->sequence++;
>
> @@ -208,6 +200,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> struct v4l2_subscribed_event *sev, *found_ev;
> unsigned long flags;
> unsigned i;
> + int ret = 0;
>
> if (sub->type == V4L2_EVENT_ALL)
> return -EINVAL;
> @@ -225,31 +218,36 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> sev->flags = sub->flags;
> sev->fh = fh;
> sev->ops = ops;
> + sev->elems = elems;
> +
> + mutex_lock(&fh->subscribe_lock);
>
> spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
> - if (!found_ev)
> - list_add(&sev->list, &fh->subscribed);
> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>
> if (found_ev) {
> + /* Already listening */
> kvfree(sev);
> - return 0; /* Already listening */
> + goto out_unlock;
> }
>
> if (sev->ops && sev->ops->add) {
> - int ret = sev->ops->add(sev, elems);
> + ret = sev->ops->add(sev, elems);
> if (ret) {
> - sev->ops = NULL;
> - v4l2_event_unsubscribe(fh, sub);
> - return ret;
> + kvfree(sev);
> + goto out_unlock;
> }
> }
>
> - /* Mark as ready for use */
> - sev->elems = elems;
> + spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> + list_add(&sev->list, &fh->subscribed);
> + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>
> - return 0;
> +out_unlock:
> + mutex_unlock(&fh->subscribe_lock);
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
>
> @@ -288,6 +286,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> return 0;
> }
>
> + mutex_lock(&fh->subscribe_lock);
> +
> spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>
> sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> @@ -305,6 +305,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> if (sev && sev->ops && sev->ops->del)
> sev->ops->del(sev);
>
> + mutex_unlock(&fh->subscribe_lock);
> +
> kvfree(sev);
>
> return 0;
> diff --git a/drivers/media/v4l2-core/v4l2-fh.c
> b/drivers/media/v4l2-core/v4l2-fh.c index 3895999bf880..c91a7bd3ecfc 100644
> --- a/drivers/media/v4l2-core/v4l2-fh.c
> +++ b/drivers/media/v4l2-core/v4l2-fh.c
> @@ -45,6 +45,7 @@ void v4l2_fh_init(
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Sakari,
On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > [snip]
> >
> > > +
> > > +/* Digital gain control */
> > > +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> > > +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> > > +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> > > +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> > > +#define IMX208_DGTL_GAIN_MIN 0
> > > +#define IMX208_DGTL_GAIN_MAX 4096
> > > +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> > > +#define IMX208_DGTL_GAIN_STEP 1
> > > +
> >
> > [snip]
> >
> > > +/* Initialize control handlers */
> > > +static int imx208_init_controls(struct imx208 *imx208)
> > > +{
> >
> > [snip]
> >
> > > + v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops,
> > > V4L2_CID_DIGITAL_GAIN, + IMX208_DGTL_GAIN_MIN,
> > > IMX208_DGTL_GAIN_MAX, + IMX208_DGTL_GAIN_STEP,
> > > + IMX208_DGTL_GAIN_DEFAULT);
> >
> > We have a problem here. The sensor supports only a discrete range of
> > values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> > fixed point). This makes it possible for the userspace to set values
> > that are not allowed by the sensor specification and also leaves no
> > way to enumerate the supported values.
> >
> > I can see two solutions here:
> >
> > 1) Define the control range from 0 to 4 and treat it as an exponent of
> > 2, so that the value for the sensor becomes (1 << val) * 256.
> > (Suggested by Sakari offline.)
> >
> > This approach has the problem of losing the original unit (and scale)
> > of the value.
>
> I'd like to add that this is not a property of the proposed solution.
>
> Rather, the above needs to be accompanied by additional information
> provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as well as
> other information such as whether the control is linear or exponential (as
> in this case).
>
> > 2) Use an integer menu control, which reports only the supported
> > discrete values - {1, 2, 4, 8, 16}.
> >
> > With this approach, userspace can enumerate the real gain values, but
> > we would either need to introduce a new control (e.g.
> > V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> > register V4L2_CID_DIGITAL_GAIN as an integer menu.
>
> New controls in V4L2 are, for the most part, created when there's something
> new to control. The documentation of some controls (similar to e.g. gain)
> documents a unit as well as a prefix but that's the case only because
> there's been no way to tell the unit or prefix otherwise in the API.
>
> An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not entirely
> sure how they came to be though. An accident is a possibility as far as I
> see.
If I remember correctly I introduced the absolute variant for the UVC driver
(even though git blame points to Brandon Philips). I don't really remember why
though.
> Controls that have a documented unit use that unit --- as long as that's
> the unit used by the hardware. If it's not, it tends to be that another
> unit is used but the user space has currently no way of knowing this. And
> the digital gain control is no exception to this.
>
> So if we want to improve the user space's ability to be informed how the
> control values reflect to device configuration, we do need to provide more
> information to the user space. One way to do that would be through
> VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of reserved fields ---
> just for purposes such as this one.
I don't think we can come up with a good way to expose arbitrary mathematical
formulas through an ioctl. In my opinion the question is how far we want to
go, how precise we need to be.
> > Any opinions or better ideas?
--
Regards,
Laurent Pinchart
Re: [RESEND PATCH 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names
Hi Sakari, On 09/16/2018 12:52 AM, Sakari Ailus wrote: > v4l2_i2c_subdev_init() sets the name of the sub-devices (as well as > entities) to what is fairly certainly known to be unique in the system, > even if there were more devices of the same kind. > > These drivers (m5mols, noon010pc30, ov9650, s5c73m3, s5k4ecgx, s5k6aa) set > the name to the name of the driver or the module while omitting the > device's I²C address and bus, leaving the devices with a static name and > effectively limiting the number of such devices in a media device to 1. > > Address this by using the name set by the V4L2 framework. > > Signed-off-by: Sakari Ailus > Reviewed-by: Akinobu Mita (ov9650) I'm not against this patch but please don't expect an ack from me as this patch breaks existing user space code, scripts using media-ctl, etc. will need to be updated after kernel upgrade. I'm mostly concerned about ov9650 as other drivers are likely only used by Samsung or are obsoleted. -- Thanks, Sylwester
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hello,
(CC'ing Helmut Grohne)
On Thursday, 20 September 2018 23:16:47 EEST Sylwester Nawrocki wrote:
> On 09/20/2018 06:49 PM, Grant Grundler wrote:
> > On Thu, Sep 20, 2018 at 1:52 AM Tomasz Figa wrote:
> >> On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> >>> +/* Digital gain control */
> >>>
> >>> +#define IMX208_DGTL_GAIN_MIN 0
> >>> +#define IMX208_DGTL_GAIN_MAX 4096
> >>> +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> >>> +#define IMX208_DGTL_GAIN_STEP 1
> >>>
> >>> +/* Initialize control handlers */
> >>> +static int imx208_init_controls(struct imx208 *imx208)
> >>> +{
> >>
> >> [snip]
> >>
> >>> + v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops,
> >>> V4L2_CID_DIGITAL_GAIN, + IMX208_DGTL_GAIN_MIN,
> >>> IMX208_DGTL_GAIN_MAX, + IMX208_DGTL_GAIN_STEP,
> >>> + IMX208_DGTL_GAIN_DEFAULT);
> >>
> >> We have a problem here. The sensor supports only a discrete range of
> >> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> >> fixed point). This makes it possible for the userspace to set values
> >> that are not allowed by the sensor specification and also leaves no
> >> way to enumerate the supported values.
>
> The driver could always adjust the value in set_ctrl callback so invalid
> settings are not allowed.
>
> I'm not sure if it's best approach but I once did something similar for
> the ov9650 sensor. The gain was fixed point 10-bits value with 4 bits
> for fractional part. The driver reports values multiplied by 16. See
> ov965x_set_gain() function in drivers/media/i2c/ov9650.c and "Table 4-1.
> Total Gain to Control Bit Correlation" in the OV9650 datasheet for details.
> The integer menu control just seemed not suitable for 2^10 values.
I've had a similar discussion on IRC recently with Helmut, who posted a nice
summary of the problem on the mailing list (see https://www.mail-archive.com/
linux-media@vger.kernel.org/msg134502.html). This is a known issue, and while
I proposed the same approach, I understand that in some cases userspace may
need to know exactly what values are acceptable. In such a case, however, I
would expect userspace to have knowledge of the particular sensor model, so
the information may not need to come from the kernel.
> Now the gain control has range 16...1984 out of which only 1024 values
> are valid. It might not be best approach for a GUI but at least the driver
> exposes mapping of all valid values, which could be enumerated with
> VIDIOC_TRY_EXT_CTRLS if required, without a need for a driver-specific
> user space code.
That would be ~2000 ioctl calls, I don't think that's very practical :-S
> >> I can see two solutions here:
> >>
> >> 1) Define the control range from 0 to 4 and treat it as an exponent of
> >> 2, so that the value for the sensor becomes (1 << val) * 256.
> >> (Suggested by Sakari offline.)
> >>
> >> This approach has the problem of losing the original unit (and scale)
> >> of the value.
> >
> > Exactly - will users be confused by this? If we have to explain it,
> > probably not the best choice.
> >
> >> 2) Use an integer menu control, which reports only the supported
> >> discrete values - {1, 2, 4, 8, 16}.
> >>
> >> With this approach, userspace can enumerate the real gain values, but
> >> we would either need to introduce a new control (e.g.
> >> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> >> register V4L2_CID_DIGITAL_GAIN as an integer menu.
> >>
> >> Any opinions or better ideas?
> >
> > My $0.02: leave the user UI alone - let users specify/select anything
> > in the range the normal API or UI allows. But have sensor specific
> > code map all values in that range to values the sensor supports. Users
> > will notice how it works when they play with it. One can "adjust" the
> > mapping so it "feels right".
--
Regards,
Laurent Pinchart
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Tomasz,
On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> [+Laurent and Sylwester]
>
> On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen
> wrote:
> [snip]
> > +
> > +/* Digital gain control */
> > +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> > +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> > +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> > +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> > +#define IMX208_DGTL_GAIN_MIN 0
> > +#define IMX208_DGTL_GAIN_MAX 4096
> > +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> > +#define IMX208_DGTL_GAIN_STEP 1
> > +
> [snip]
> > +/* Initialize control handlers */
> > +static int imx208_init_controls(struct imx208 *imx208)
> > +{
> [snip]
> > + v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops,
> > V4L2_CID_DIGITAL_GAIN,
> > + IMX208_DGTL_GAIN_MIN, IMX208_DGTL_GAIN_MAX,
> > + IMX208_DGTL_GAIN_STEP,
> > + IMX208_DGTL_GAIN_DEFAULT);
>
> We have a problem here. The sensor supports only a discrete range of
> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> fixed point). This makes it possible for the userspace to set values
> that are not allowed by the sensor specification and also leaves no
> way to enumerate the supported values.
>
> I can see two solutions here:
>
> 1) Define the control range from 0 to 4 and treat it as an exponent of
> 2, so that the value for the sensor becomes (1 << val) * 256.
> (Suggested by Sakari offline.)
>
> This approach has the problem of losing the original unit (and scale)
> of the value.
I'd like to add that this is not a property of the proposed solution.
Rather, the above needs to be accompanied by additional information
provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as well as
other information such as whether the control is linear or exponential (as
in this case).
>
> 2) Use an integer menu control, which reports only the supported
> discrete values - {1, 2, 4, 8, 16}.
>
> With this approach, userspace can enumerate the real gain values, but
> we would either need to introduce a new control (e.g.
> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> register V4L2_CID_DIGITAL_GAIN as an integer menu.
New controls in V4L2 are, for the most part, created when there's something
new to control. The documentation of some controls (similar to e.g. gain)
documents a unit as well as a prefix but that's the case only because
there's been no way to tell the unit or prefix otherwise in the API.
An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not entirely
sure how they came to be though. An accident is a possibility as far as I
see.
Controls that have a documented unit use that unit --- as long as that's
the unit used by the hardware. If it's not, it tends to be that another
unit is used but the user space has currently no way of knowing this. And
the digital gain control is no exception to this.
So if we want to improve the user space's ability to be informed how the
control values reflect to device configuration, we do need to provide more
information to the user space. One way to do that would be through
VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of reserved fields ---
just for purposes such as this one.
>
> Any opinions or better ideas?
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
ad5820: Sorry for the invalid v3
HI Sorry, I did not check the produced files before sending them with git send-email. It has been a long day. Please ignore v3 and use v4. I will write 100 times: I will check my patches before sending them to the mailing list Cheers -- Ricardo Ribalda
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
On 09/20/2018 06:49 PM, Grant Grundler wrote:
> On Thu, Sep 20, 2018 at 1:52 AM Tomasz Figa wrote:
>> On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen
>> wrote:
>>> +/* Digital gain control */
>>> +#define IMX208_DGTL_GAIN_MIN 0
>>> +#define IMX208_DGTL_GAIN_MAX 4096
>>> +#define IMX208_DGTL_GAIN_DEFAULT 0x100
>>> +#define IMX208_DGTL_GAIN_STEP 1
>>> +/* Initialize control handlers */
>>> +static int imx208_init_controls(struct imx208 *imx208)
>>> +{
>> [snip]
>>> + v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops,
>>> V4L2_CID_DIGITAL_GAIN,
>>> + IMX208_DGTL_GAIN_MIN, IMX208_DGTL_GAIN_MAX,
>>> + IMX208_DGTL_GAIN_STEP,
>>> + IMX208_DGTL_GAIN_DEFAULT);
>>
>> We have a problem here. The sensor supports only a discrete range of
>> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
>> fixed point). This makes it possible for the userspace to set values
>> that are not allowed by the sensor specification and also leaves no
>> way to enumerate the supported values.
The driver could always adjust the value in set_ctrl callback so invalid
settings are not allowed.
I'm not sure if it's best approach but I once did something similar for
the ov9650 sensor. The gain was fixed point 10-bits value with 4 bits
for fractional part. The driver reports values multiplied by 16. See
ov965x_set_gain() function in drivers/media/i2c/ov9650.c and "Table 4-1.
Total Gain to Control Bit Correlation" in the OV9650 datasheet for details.
The integer menu control just seemed not suitable for 2^10 values.
Now the gain control has range 16...1984 out of which only 1024 values
are valid. It might not be best approach for a GUI but at least the driver
exposes mapping of all valid values, which could be enumerated with
VIDIOC_TRY_EXT_CTRLS if required, without a need for a driver-specific
user space code.
>> I can see two solutions here:
>>
>> 1) Define the control range from 0 to 4 and treat it as an exponent of
>> 2, so that the value for the sensor becomes (1 << val) * 256.
>> (Suggested by Sakari offline.)
>>
>> This approach has the problem of losing the original unit (and scale)
>> of the value.
>
> Exactly - will users be confused by this? If we have to explain it,
> probably not the best choice.
>
>>
>> 2) Use an integer menu control, which reports only the supported
>> discrete values - {1, 2, 4, 8, 16}.
>>
>> With this approach, userspace can enumerate the real gain values, but
>> we would either need to introduce a new control (e.g.
>> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
>> register V4L2_CID_DIGITAL_GAIN as an integer menu.
>>
>> Any opinions or better ideas?
>
> My $0.02: leave the user UI alone - let users specify/select anything
> in the range the normal API or UI allows. But have sensor specific
> code map all values in that range to values the sensor supports. Users
> will notice how it works when they play with it. One can "adjust" the
> mapping so it "feels right".
--
Regards,
Sylwester
[PATCH] libv4l: Add support for BAYER10P format conversion
Add support for 10 bit packet Bayer formats:
-V4L2_PIX_FMT_SBGGR10P
-V4L2_PIX_FMT_SGBRG10P
-V4L2_PIX_FMT_SGRBG10P
-V4L2_PIX_FMT_SRGGB10P
These formats pack the 2 LSBs for every 4 pixels in an indeppendent
byte.
Signed-off-by: Ricardo Ribalda Delgado
---
lib/libv4lconvert/bayer.c | 15 +++
lib/libv4lconvert/libv4lconvert-priv.h | 4 +++
lib/libv4lconvert/libv4lconvert.c | 35 ++
3 files changed, 54 insertions(+)
diff --git a/lib/libv4lconvert/bayer.c b/lib/libv4lconvert/bayer.c
index 4b70ddd9..d7d488f9 100644
--- a/lib/libv4lconvert/bayer.c
+++ b/lib/libv4lconvert/bayer.c
@@ -631,3 +631,18 @@ void v4lconvert_bayer_to_yuv420(const unsigned char
*bayer, unsigned char *yuv,
v4lconvert_border_bayer_line_to_y(bayer + stride, bayer, ydst, width,
!start_with_green, !blue_line);
}
+
+void v4lconvert_bayer10p_to_bayer8(unsigned char *bayer10p,
+ unsigned char *bayer8, int width, int height)
+{
+ long i, len = width * height;
+ uint32_t *src, *dst;
+
+ src = (uint32_t *)bayer10p;
+ dst = (uint32_t *)bayer8;
+ for (i = 0; i < len ; i += 4) {
+ *dst = *src;
+ dst++;
+ src = (uint32_t *)(((uint8_t *)src) + 5);
+ }
+}
diff --git a/lib/libv4lconvert/libv4lconvert-priv.h
b/lib/libv4lconvert/libv4lconvert-priv.h
index 9a467e10..3020a39e 100644
--- a/lib/libv4lconvert/libv4lconvert-priv.h
+++ b/lib/libv4lconvert/libv4lconvert-priv.h
@@ -264,6 +264,10 @@ void v4lconvert_bayer_to_bgr24(const unsigned char *bayer,
void v4lconvert_bayer_to_yuv420(const unsigned char *bayer, unsigned char *yuv,
int width, int height, const unsigned int stride, unsigned int
src_pixfmt, int yvu);
+
+void v4lconvert_bayer10p_to_bayer8(unsigned char *bayer10p,
+ unsigned char *bayer8, int width, int height);
+
void v4lconvert_hm12_to_rgb24(const unsigned char *src,
unsigned char *dst, int width, int height);
diff --git a/lib/libv4lconvert/libv4lconvert.c
b/lib/libv4lconvert/libv4lconvert.c
index d666bd97..b3dbf5a0 100644
--- a/lib/libv4lconvert/libv4lconvert.c
+++ b/lib/libv4lconvert/libv4lconvert.c
@@ -133,6 +133,10 @@ static const struct v4lconvert_pixfmt
supported_src_pixfmts[] = {
{ V4L2_PIX_FMT_SRGGB8, 8, 8, 8, 0 },
{ V4L2_PIX_FMT_STV0680, 8, 8, 8, 1 },
{ V4L2_PIX_FMT_SGRBG10, 16, 8, 8, 1 },
+ { V4L2_PIX_FMT_SBGGR10P,10, 8, 8, 1 },
+ { V4L2_PIX_FMT_SGBRG10P,10, 8, 8, 1 },
+ { V4L2_PIX_FMT_SGRBG10P,10, 8, 8, 1 },
+ { V4L2_PIX_FMT_SRGGB10P,10, 8, 8, 1 },
/* compressed bayer */
{ V4L2_PIX_FMT_SPCA561, 0, 9, 9, 1 },
{ V4L2_PIX_FMT_SN9C10X, 0, 9, 9, 1 },
@@ -687,6 +691,10 @@ static int v4lconvert_processing_needs_double_conversion(
case V4L2_PIX_FMT_SGBRG8:
case V4L2_PIX_FMT_SGRBG8:
case V4L2_PIX_FMT_SRGGB8:
+ case V4L2_PIX_FMT_SBGGR10P:
+ case V4L2_PIX_FMT_SGBRG10P:
+ case V4L2_PIX_FMT_SGRBG10P:
+ case V4L2_PIX_FMT_SRGGB10P:
case V4L2_PIX_FMT_STV0680:
return 0;
}
@@ -979,6 +987,33 @@ static int v4lconvert_convert_pixfmt(struct
v4lconvert_data *data,
}
/* Raw bayer formats */
+ case V4L2_PIX_FMT_SBGGR10P:
+ case V4L2_PIX_FMT_SGBRG10P:
+ case V4L2_PIX_FMT_SGRBG10P:
+ case V4L2_PIX_FMT_SRGGB10P:
+ if (src_size < ((width * height * 10)/8)) {
+ V4LCONVERT_ERR("short raw bayer10 data frame\n");
+ errno = EPIPE;
+ result = -1;
+ }
+ switch (src_pix_fmt) {
+ case V4L2_PIX_FMT_SBGGR10P:
+ src_pix_fmt = V4L2_PIX_FMT_SBGGR8;
+ break;
+ case V4L2_PIX_FMT_SGBRG10P:
+ src_pix_fmt = V4L2_PIX_FMT_SGBRG8;
+ break;
+ case V4L2_PIX_FMT_SGRBG10P:
+ src_pix_fmt = V4L2_PIX_FMT_SGRBG8;
+ break;
+ case V4L2_PIX_FMT_SRGGB10P:
+ src_pix_fmt = V4L2_PIX_FMT_SRGGB8;
+ break;
+ }
+ v4lconvert_bayer10p_to_bayer8(src, src, width, height);
+ bytesperline = width;
+
+ /* Fall-through*/
case V4L2_PIX_FMT_SBGGR8:
case V4L2_PIX_FMT_SGBRG8:
case V4L2_PIX_FMT_SGRBG8:
--
2.18.0
Re: [PATCH v2 2/4] [media] ad5820: Add support for enable pin
Hi Ricardo,
On Thursday, 20 September 2018 22:12:44 EEST Ricardo Ribalda Delgado wrote:
> On Thu, Sep 20, 2018 at 9:08 PM Pavel Machek wrote:
> > On Thu 2018-09-20 21:06:16, Ricardo Ribalda Delgado wrote:
> >> On Thu, Sep 20, 2018 at 8:54 PM Pavel Machek wrote:
> >>> On Thu 2018-09-20 20:45:52, Ricardo Ribalda Delgado wrote:
> This patch adds support for a programmable enable pin. It can be
> used in situations where the ANA-vcc is not configurable (dummy-
> regulator), or just to have a more fine control of the power saving.
>
> The use of the enable pin is optional.
>
> Signed-off-by: Ricardo Ribalda Delgado
> >>>
> >>> Do we really want to do that?
> >>>
> >>> Would it make more sense to add gpio-regulator and connect ad5820 to
> >>> it in such case?
> >>
> >> My board (based on db820c) has both:
> >>
> >> ad5820: dac@0c {
> >>compatible = "adi,ad5820";
> >>reg = <0x0c>;
> >>
> >>VANA-supply = <&pm8994_l23>;
> >>enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
> >> };
> >
> > Well, I'm sure you could have gpio-based regulator powered from
> > pm8994_l23, and outputting to ad5820.
> >
> > Does ad5820 chip have a gpio input for enable?
>
> xshutdown pin:
> http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.pd
> f
>
> (AD5820,AD5821, and AD5823 are compatibles, or at least that is waht
> the module manufacturer says :)
Is that the pin that msmgpio 26 is connected to on your board ?
I'd argue that the GPIO should be called xshutdown in that case, as DT usually
uses the pin name, but there's precedent of using well-known names for pins
with the same functionality. In any case you should update the DT bindings to
document the new property, and clearly explain that it describes the GPIO
connected to the xshutdown pin. Please CC the devicet...@vger.kernel.org
mailing list on the bindings change (they usually like bindings changes to be
split to a separate patch).
--
Regards,
Laurent Pinchart
Re: [PATCH v2 2/4] [media] ad5820: Add support for enable pin
On Thursday, 20 September 2018 23:04:21 EEST Laurent Pinchart wrote:
> Hi Ricardo,
>
> On Thursday, 20 September 2018 22:12:44 EEST Ricardo Ribalda Delgado wrote:
> > On Thu, Sep 20, 2018 at 9:08 PM Pavel Machek wrote:
> > > On Thu 2018-09-20 21:06:16, Ricardo Ribalda Delgado wrote:
> > >> On Thu, Sep 20, 2018 at 8:54 PM Pavel Machek wrote:
> > >>> On Thu 2018-09-20 20:45:52, Ricardo Ribalda Delgado wrote:
> > This patch adds support for a programmable enable pin. It can be
> > used in situations where the ANA-vcc is not configurable (dummy-
> > regulator), or just to have a more fine control of the power saving.
> >
> > The use of the enable pin is optional.
> >
> > Signed-off-by: Ricardo Ribalda Delgado
> > >>>
> > >>> Do we really want to do that?
> > >>>
> > >>> Would it make more sense to add gpio-regulator and connect ad5820 to
> > >>> it in such case?
> > >>
> > >> My board (based on db820c) has both:
> > >>
> > >> ad5820: dac@0c {
> > >>
> > >>compatible = "adi,ad5820";
> > >>reg = <0x0c>;
> > >>
> > >>VANA-supply = <&pm8994_l23>;
> > >>enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
> > >>
> > >> };
> > >
> > > Well, I'm sure you could have gpio-based regulator powered from
> > > pm8994_l23, and outputting to ad5820.
> > >
> > > Does ad5820 chip have a gpio input for enable?
> >
> > xshutdown pin:
> > http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.
> > pd f
> >
> > (AD5820,AD5821, and AD5823 are compatibles, or at least that is waht
> > the module manufacturer says :)
>
> Is that the pin that msmgpio 26 is connected to on your board ?
>
> I'd argue that the GPIO should be called xshutdown in that case, as DT
> usually uses the pin name, but there's precedent of using well-known names
> for pins with the same functionality. In any case you should update the DT
> bindings to document the new property, and clearly explain that it
> describes the GPIO connected to the xshutdown pin. Please CC the
> devicet...@vger.kernel.org mailing list on the bindings change (they
> usually like bindings changes to be split to a separate patch).
And now I've noticed patch 3/4 :-/ Please scratch this.
--
Regards,
Laurent Pinchart
Re: [RFP] Which V4L2 ioctls could be replaced by better versions?
Le jeudi 20 septembre 2018 à 16:42 +0200, Hans Verkuil a écrit : > Some parts of the V4L2 API are awkward to use and I think it would be > a good idea to look at possible candidates for that. > > Examples are the ioctls that use struct v4l2_buffer: the multiplanar support > is > really horrible, and writing code to support both single and multiplanar is > hard. > We are also running out of fields and the timeval isn't y2038 compliant. > > A proof-of-concept is here: > > https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=v4l2-buffer&id=a95549df06d9900f3559afdbb9da06bd4b22d1f3 > > It's a bit old, but it gives a good impression of what I have in mind. > > Another candidate is > VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL/VIDIOC_ENUM_FRAMEINTERVALS: > expressing frame intervals as a fraction is really awkward and so is the fact > that the subdev and 'normal' ioctls are not the same. > > Would using nanoseconds or something along those lines for intervals be > better? This one is not a good idea, because you cannot represent well known rates used a lot in the field. Like 6/1001 also known as 59.94 Hz. You could endup with drift issues. For me, what is the most difficult with this API is the fact that it uses frame internal (duration) instead of frame rate. > > I have similar concerns with VIDIOC_SUBDEV_ENUM_FRAME_SIZE where there is no > stepwise option, making it different from VIDIOC_ENUM_FRAMESIZES. But it > should > be possible to extend VIDIOC_SUBDEV_ENUM_FRAME_SIZE with stepwise support, I > think. One of the thing to fix, maybe it's doable now, is the differentiation between allocation size and display size. Pretty much all video capture code assumes this is display size and ignores the selection API. This should be documented explicitly. In fact, the display/allocation dimension isn't very nice, as both information overlaps in structures. As an example, you call S_FMT with a display size you want, and it will return you an allocation size (which yes, can be smaller, because we always round to the middle). > > Do we have more ioctls that could use a refresh? S/G/TRY_FMT perhaps, again in > order to improve single vs multiplanar handling. Yes, but that would fall in a complete redesign I guess. The buffer allocation scheme is very inflexible. You can't have buffers of two dimensions allocated at the same time for the same queue. Worst, you cannot leave even 1 buffer as your scannout buffer while reallocating new buffers, this is not permitted by the framework (in software). As a side effect, there is no way to optimize the resolution changes, you even have to copy your scannout buffer on the CPU, to free it in order to proceed. Resolution changes are thus painfully slow, by design. You also cannot switch from internal buffers to importing buffers easily (in some case you, like encoder, you cannot do that without flushing the encoder state). > > It is not the intention to come to a full design, it's more to test the waters > so to speak. > > Regards, > > Hans signature.asc Description: This is a digitally signed message part
Re: [PATCH 5/6] media: isp: fix a warning about a wrong struct initializer
Hi Mauro,
On Thursday, 20 September 2018 17:44:53 EEST Mauro Carvalho Chehab wrote:
> Em Fri, 07 Sep 2018 14:46:34 +0300 Laurent Pinchart escreveu:
> > Hi Mauro,
> >
> > As maintainers should be held to the same level of obligations as
> > developers, and to avoid demotivating reviewers, could you handle
> > comments you receive before pushing your own patches to your tree ? There
> > should be no maintainer privilege here.
>
> Sorry, yeah, this was improperly handled. Not sure what happened.
>
> From whatever reason, I ended by mishandling this and lost the
> related emails. I had to do several e-mail changes at the beginning
> of August, as I'm not using s-opensource.com anymore, and had to
> reconfigure my inbox handling logic.
Thank you for the explanation.
> Do you want me to revert the patch?
I appreciate the proposal, but there's no need to :-) It's not a big issue, I
could easily submit a follow-up patch, but even that isn't really needed. My
main concern was the lack of reaction to the review, and now that this has be
cleared, the issue is closed for me.
> > On Wednesday, 8 August 2018 18:45:49 EEST Laurent Pinchart wrote:
> > > Hi Mauro,
> > >
> > > Thank you for the patch.
> > >
> > > The subject line should be "media: omap3isp: ...".
> > >
> > > On Wednesday, 8 August 2018 17:52:55 EEST Mauro Carvalho Chehab wrote:
> > > > As sparse complains:
> > > > drivers/media/platform/omap3isp/isp.c:303:39: warning: Using
> > > > plain
> > > > integer
> > > >
> > > > as NULL pointer
> > > >
> > > > when a struct is initialized with { 0 }, actually the first
> > > > element of the struct is initialized with zeros, initializing the
> > > > other elements recursively. That can even generate gcc warnings
> > > > on nested structs.
> > > >
> > > > So, instead, use the gcc-specific syntax for that (with is used
> > > > broadly inside the Kernel), initializing it with {};
> > > >
> > > > Signed-off-by: Mauro Carvalho Chehab
> > > > ---
> > > >
> > > > drivers/media/platform/omap3isp/isp.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/platform/omap3isp/isp.c
> > > > b/drivers/media/platform/omap3isp/isp.c index
> > > > 03354d513311..842e2235047d
> > > > 100644
> > > > --- a/drivers/media/platform/omap3isp/isp.c
> > > > +++ b/drivers/media/platform/omap3isp/isp.c
> > > > @@ -300,7 +300,7 @@ static struct clk *isp_xclk_src_get(struct
> > > > of_phandle_args *clkspec, void *data) static int isp_xclk_init(struct
> > > > isp_device *isp)
> > > >
> > > > {
> > > >
> > > > struct device_node *np = isp->dev->of_node;
> > > >
> > > > - struct clk_init_data init = { 0 };
> > > > + struct clk_init_data init = {};
> > >
> > > How about = { NULL }; to avoid a gcc-specific syntax ?
> > >
> > > > unsigned int i;
> > > >
> > > > for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i)
>
> Thanks,
> Mauro
--
Regards,
Laurent Pinchart
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
[resend in plain text - sorry!]
On Thu, Sep 20, 2018 at 1:52 AM Tomasz Figa wrote:
>
> [+Laurent and Sylwester]
>
> On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen
> wrote:
> [snip]
> > +
> > +/* Digital gain control */
> > +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> > +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> > +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> > +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> > +#define IMX208_DGTL_GAIN_MIN 0
> > +#define IMX208_DGTL_GAIN_MAX 4096
> > +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> > +#define IMX208_DGTL_GAIN_STEP 1
> > +
> [snip]
> > +/* Initialize control handlers */
> > +static int imx208_init_controls(struct imx208 *imx208)
> > +{
> [snip]
> > + v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops,
> > V4L2_CID_DIGITAL_GAIN,
> > + IMX208_DGTL_GAIN_MIN, IMX208_DGTL_GAIN_MAX,
> > + IMX208_DGTL_GAIN_STEP,
> > + IMX208_DGTL_GAIN_DEFAULT);
>
> We have a problem here. The sensor supports only a discrete range of
> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> fixed point). This makes it possible for the userspace to set values
> that are not allowed by the sensor specification and also leaves no
> way to enumerate the supported values.
>
> I can see two solutions here:
>
> 1) Define the control range from 0 to 4 and treat it as an exponent of
> 2, so that the value for the sensor becomes (1 << val) * 256.
> (Suggested by Sakari offline.)
>
> This approach has the problem of losing the original unit (and scale)
> of the value.
Exactly - will users be confused by this? If we have to explain it,
probably not the best choice.
>
> 2) Use an integer menu control, which reports only the supported
> discrete values - {1, 2, 4, 8, 16}.
>
> With this approach, userspace can enumerate the real gain values, but
> we would either need to introduce a new control (e.g.
> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> register V4L2_CID_DIGITAL_GAIN as an integer menu.
>
> Any opinions or better ideas?
My $0.02: leave the user UI alone - let users specify/select anything
in the range the normal API or UI allows. But have sensor specific
code map all values in that range to values the sensor supports. Users
will notice how it works when they play with it. One can "adjust" the
mapping so it "feels right".
cheers,
grant
>
> Best regards,
> Tomasz
Re: [PATCH v3 1/2] media: v4l2-subdev.h: allow V4L2_FRMIVAL_TYPE_CONTINUOUS & _STEPWISE
On Thu, Sep 20, 2018 at 05:11:50PM +0300, Laurent Pinchart wrote:
> Hi Philippe,
>
> Thank you for the patch.
>
> On Tuesday, 11 September 2018 20:06:32 EEST Philippe De Muyter wrote:
> > add V4L2_FRMIVAL_TYPE_CONTINUOUS and V4L2_FRMIVAL_TYPE_STEPWISE for
> > subdev's frame intervals in addition to implicit existing
> > V4L2_FRMIVAL_TYPE_DISCRETE type. This needs three new fields in the
> > v4l2_subdev_frame_interval_enum struct :
> > - type
> > - max_interval
> > - step_interval
> >
> > A helper function 'v4l2_fill_frmivalenum_from_subdev' is also added.
> >
> > Subdevs must fill the 'type' field. If they do not, the default
> > value (0) is used which is equal to V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE.
> >
> > if type is set to V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE, or left untouched,
> > only the 'interval' field must be filled, just as before.
> >
> > If type is set to V4L2_FRMIVAL_TYPE_CONTINUOUS, 'interval' must be set
> > to the minimum frame interval (highest framerate), and 'max_interval'
> > to the maximum frame interval.
> >
> > If type is set to V4L2_FRMIVAL_TYPE_STEPWISE, 'step_interval' must be
> > set to the step between available intervals, in addition to 'interval'
> > and 'max_interval' which must be set as for V4L2_FRMIVAL_TYPE_CONTINUOUS
>
> Continuous is a special case of stepwise with the step set to 1. Should we
> merge the two types ?
I always wondered what that '1' meant; surely not 1/1. I rather see
STEPWISE as a special case of CONTINUOUS, where the granularity of the
step is too big, making it more DISCRETE-like than CONTINUOUS-like.
I do not use STEPWISE. It is only here for completeness compared to
the not-subdev case.
>
> I'm curious, as there's nothing in this series, using the new types, what are
> your use cases ?
I have drivers for industrial and even monochrome sensors, but that must
run in vendor-specific and old linux versions, and I must add this patch
each time, but I am not able to upstream the sensor drivers as they are
too tighted to the vendor-specific linux.
Adding that patch now increases the probability that the next vendor-specific
linux I'll get will contain it :)
>
> > Old users which do not check the 'type' field will get the minimum frame
> > interval (highest framrate) just like before.
> >
> > Callers who intend to check the 'type' field should zero it themselves,
> > in case an old subdev driver does not do zero it.
> >
> > When filled correctly by the sensor driver, the new fields must be
> > used as follows by the caller :
> >
> > struct v4l2_frmivalenum * fival;
> > struct v4l2_subdev_frame_interval_enum fie;
> >
> > if (fie.type == V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE) {
> > fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> > fival->discrete = fie.interval;
> > } else if (fie.type == V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS) {
> > fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> > fival->stepwise.min = fie.interval;
> > fival->stepwise.max = fie.max_interval;
> > } else {
> > fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
> > fival->stepwise.min = fie.interval;
> > fival->stepwise.max = fie.max_interval;
> > fival->stepwise.step = fie.step_interval;
> > }
> >
> > Kernel users should use the new 'v4l2_fill_frmivalenum_from_subdev'
> > helper function.
> >
> > Signed-off-by: Philippe De Muyter
> > ---
> > .../uapi/v4l/vidioc-subdev-enum-frame-interval.rst | 69 ++-
> > drivers/media/v4l2-core/v4l2-common.c | 33 +++
> > include/media/v4l2-common.h| 12
> > include/uapi/linux/v4l2-subdev.h | 22 ++-
> > 4 files changed, 133 insertions(+), 3 deletions(-)
> >
> > diff --git
> > a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst
> > b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst index
> > 1bfe386..e14fa14 100644
> > --- a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst
> > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst
> > @@ -51,6 +51,41 @@ EINVAL error code if one of the input fields is invalid.
> > All frame intervals are enumerable by beginning at index zero and
> > incrementing by one until ``EINVAL`` is returned.
> >
> > +If the sub-device can work only with a fixed set of frame intervals, then
> > +the driver must enumerate them with increasing indexes, by setting the
> > +``type`` field to ``V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE`` and only filling
> > +the ``interval`` field . If the sub-device can work with a continuous
> > +range of frame intervals, then the driver must only return success for
> > +index 0, set the ``type`` field to ``V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS``,
> > +fill ``interval`` with the minimum interval and ``max_interval`` with
> > +the maximum interval. If it is worth mentioning the step in the
> > +continuous i
Re: [PATCH 5/6] media: isp: fix a warning about a wrong struct initializer
Em Fri, 07 Sep 2018 14:46:34 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> As maintainers should be held to the same level of obligations as developers,
> and to avoid demotivating reviewers, could you handle comments you receive
> before pushing your own patches to your tree ? There should be no maintainer
> privilege here.
Sorry, yeah, this was improperly handled. Not sure what happened.
From whatever reason, I ended by mishandling this and lost the
related emails. I had to do several e-mail changes at the beginning
of August, as I'm not using s-opensource.com anymore, and had to
reconfigure my inbox handling logic.
Do you want me to revert the patch?
>
> On Wednesday, 8 August 2018 18:45:49 EEST Laurent Pinchart wrote:
> > Hi Mauro,
> >
> > Thank you for the patch.
> >
> > The subject line should be "media: omap3isp: ...".
> >
> > On Wednesday, 8 August 2018 17:52:55 EEST Mauro Carvalho Chehab wrote:
> > > As sparse complains:
> > > drivers/media/platform/omap3isp/isp.c:303:39: warning: Using plain
> > > integer
> > >
> > > as NULL pointer
> > >
> > > when a struct is initialized with { 0 }, actually the first
> > > element of the struct is initialized with zeros, initializing the
> > > other elements recursively. That can even generate gcc warnings
> > > on nested structs.
> > >
> > > So, instead, use the gcc-specific syntax for that (with is used
> > > broadly inside the Kernel), initializing it with {};
> > >
> > > Signed-off-by: Mauro Carvalho Chehab
> > > ---
> > >
> > > drivers/media/platform/omap3isp/isp.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/platform/omap3isp/isp.c
> > > b/drivers/media/platform/omap3isp/isp.c index 03354d513311..842e2235047d
> > > 100644
> > > --- a/drivers/media/platform/omap3isp/isp.c
> > > +++ b/drivers/media/platform/omap3isp/isp.c
> > > @@ -300,7 +300,7 @@ static struct clk *isp_xclk_src_get(struct
> > > of_phandle_args *clkspec, void *data) static int isp_xclk_init(struct
> > > isp_device *isp)
> > >
> > > {
> > >
> > > struct device_node *np = isp->dev->of_node;
> > >
> > > - struct clk_init_data init = { 0 };
> > > + struct clk_init_data init = {};
> >
> > How about = { NULL }; to avoid a gcc-specific syntax ?
> >
> > > unsigned int i;
> > >
> > > for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i)
>
>
Thanks,
Mauro
[RFP] Which V4L2 ioctls could be replaced by better versions?
Some parts of the V4L2 API are awkward to use and I think it would be a good idea to look at possible candidates for that. Examples are the ioctls that use struct v4l2_buffer: the multiplanar support is really horrible, and writing code to support both single and multiplanar is hard. We are also running out of fields and the timeval isn't y2038 compliant. A proof-of-concept is here: https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=v4l2-buffer&id=a95549df06d9900f3559afdbb9da06bd4b22d1f3 It's a bit old, but it gives a good impression of what I have in mind. Another candidate is VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL/VIDIOC_ENUM_FRAMEINTERVALS: expressing frame intervals as a fraction is really awkward and so is the fact that the subdev and 'normal' ioctls are not the same. Would using nanoseconds or something along those lines for intervals be better? I have similar concerns with VIDIOC_SUBDEV_ENUM_FRAME_SIZE where there is no stepwise option, making it different from VIDIOC_ENUM_FRAMESIZES. But it should be possible to extend VIDIOC_SUBDEV_ENUM_FRAME_SIZE with stepwise support, I think. Do we have more ioctls that could use a refresh? S/G/TRY_FMT perhaps, again in order to improve single vs multiplanar handling. It is not the intention to come to a full design, it's more to test the waters so to speak. Regards, Hans
Re: [PATCH v3 1/2] media: v4l2-subdev.h: allow V4L2_FRMIVAL_TYPE_CONTINUOUS & _STEPWISE
Hi Philippe,
Thank you for the patch.
On Tuesday, 11 September 2018 20:06:32 EEST Philippe De Muyter wrote:
> add V4L2_FRMIVAL_TYPE_CONTINUOUS and V4L2_FRMIVAL_TYPE_STEPWISE for
> subdev's frame intervals in addition to implicit existing
> V4L2_FRMIVAL_TYPE_DISCRETE type. This needs three new fields in the
> v4l2_subdev_frame_interval_enum struct :
> - type
> - max_interval
> - step_interval
>
> A helper function 'v4l2_fill_frmivalenum_from_subdev' is also added.
>
> Subdevs must fill the 'type' field. If they do not, the default
> value (0) is used which is equal to V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE.
>
> if type is set to V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE, or left untouched,
> only the 'interval' field must be filled, just as before.
>
> If type is set to V4L2_FRMIVAL_TYPE_CONTINUOUS, 'interval' must be set
> to the minimum frame interval (highest framerate), and 'max_interval'
> to the maximum frame interval.
>
> If type is set to V4L2_FRMIVAL_TYPE_STEPWISE, 'step_interval' must be
> set to the step between available intervals, in addition to 'interval'
> and 'max_interval' which must be set as for V4L2_FRMIVAL_TYPE_CONTINUOUS
Continuous is a special case of stepwise with the step set to 1. Should we
merge the two types ?
I'm curious, as there's nothing in this series, using the new types, what are
your use cases ?
> Old users which do not check the 'type' field will get the minimum frame
> interval (highest framrate) just like before.
>
> Callers who intend to check the 'type' field should zero it themselves,
> in case an old subdev driver does not do zero it.
>
> When filled correctly by the sensor driver, the new fields must be
> used as follows by the caller :
>
>struct v4l2_frmivalenum * fival;
>struct v4l2_subdev_frame_interval_enum fie;
>
>if (fie.type == V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE) {
>fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
>fival->discrete = fie.interval;
>} else if (fie.type == V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS) {
>fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
>fival->stepwise.min = fie.interval;
>fival->stepwise.max = fie.max_interval;
>} else {
>fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
>fival->stepwise.min = fie.interval;
>fival->stepwise.max = fie.max_interval;
>fival->stepwise.step = fie.step_interval;
>}
>
> Kernel users should use the new 'v4l2_fill_frmivalenum_from_subdev'
> helper function.
>
> Signed-off-by: Philippe De Muyter
> ---
> .../uapi/v4l/vidioc-subdev-enum-frame-interval.rst | 69 ++-
> drivers/media/v4l2-core/v4l2-common.c | 33 +++
> include/media/v4l2-common.h| 12
> include/uapi/linux/v4l2-subdev.h | 22 ++-
> 4 files changed, 133 insertions(+), 3 deletions(-)
>
> diff --git
> a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst
> b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst index
> 1bfe386..e14fa14 100644
> --- a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-interval.rst
> @@ -51,6 +51,41 @@ EINVAL error code if one of the input fields is invalid.
> All frame intervals are enumerable by beginning at index zero and
> incrementing by one until ``EINVAL`` is returned.
>
> +If the sub-device can work only with a fixed set of frame intervals, then
> +the driver must enumerate them with increasing indexes, by setting the
> +``type`` field to ``V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE`` and only filling
> +the ``interval`` field . If the sub-device can work with a continuous
> +range of frame intervals, then the driver must only return success for
> +index 0, set the ``type`` field to ``V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS``,
> +fill ``interval`` with the minimum interval and ``max_interval`` with
> +the maximum interval. If it is worth mentioning the step in the
> +continuous interval, the driver must set the ``type`` field to
> +``V4L2_SUBDEV_FRMIVAL_TYPE_STEPWISE`` and fill also the ``step_interval``
> +field with the step between the possible intervals.
> +
> +Callers are expected to use the returned information as follows:
> +
> +.. code-block:: c
> +
> +struct v4l2_frmivalenum *fival;
> +struct v4l2_subdev_frame_interval_enum fie;
> +
> +if (fie.type == V4L2_SUBDEV_FRMIVAL_TYPE_DISCRETE) {
> +fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> +fival->discrete = fie.interval;
> +} else if (fie.type == V4L2_SUBDEV_FRMIVAL_TYPE_CONTINUOUS) {
> +fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> +fival->stepwise.min = fie.interval;
> +fival->stepwise.max = fie.max_interval;
> +} else {
> +fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
>
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
[+Laurent and Sylwester]
On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen
wrote:
[snip]
> +
> +/* Digital gain control */
> +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e
> +#define IMX208_REG_R_DIGITAL_GAIN 0x0210
> +#define IMX208_REG_B_DIGITAL_GAIN 0x0212
> +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214
> +#define IMX208_DGTL_GAIN_MIN 0
> +#define IMX208_DGTL_GAIN_MAX 4096
> +#define IMX208_DGTL_GAIN_DEFAULT 0x100
> +#define IMX208_DGTL_GAIN_STEP 1
> +
[snip]
> +/* Initialize control handlers */
> +static int imx208_init_controls(struct imx208 *imx208)
> +{
[snip]
> + v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> + IMX208_DGTL_GAIN_MIN, IMX208_DGTL_GAIN_MAX,
> + IMX208_DGTL_GAIN_STEP,
> + IMX208_DGTL_GAIN_DEFAULT);
We have a problem here. The sensor supports only a discrete range of
values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
fixed point). This makes it possible for the userspace to set values
that are not allowed by the sensor specification and also leaves no
way to enumerate the supported values.
I can see two solutions here:
1) Define the control range from 0 to 4 and treat it as an exponent of
2, so that the value for the sensor becomes (1 << val) * 256.
(Suggested by Sakari offline.)
This approach has the problem of losing the original unit (and scale)
of the value.
2) Use an integer menu control, which reports only the supported
discrete values - {1, 2, 4, 8, 16}.
With this approach, userspace can enumerate the real gain values, but
we would either need to introduce a new control (e.g.
V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
register V4L2_CID_DIGITAL_GAIN as an integer menu.
Any opinions or better ideas?
Best regards,
Tomasz
