(sin asunto)
-- Good Day, I, Mavis Wanczyk donates $ 5 Million Dollars to you, from part of my Powerball Jackpot Lottery of $ 758 Million Dollars, respond with your details for claims. I await your earliest response and God Bless you. Good luck, Mavis Wanczyk.
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: Thu Sep 27 05:00:10 CEST 2018 media-tree git hash:4158757395b300b6eb308fc20b96d1d231484413 media_build git hash: 44385b9c61ecc27059a651885895c8ea09cd4179 v4l-utils git hash: 3874aa8eb1ff0c2e103d024ba5af915b1b26f098 edid-decode git hash: 5eeb151a748788666534d6ea3da07f90400d24c2 gcc version:i686-linux-gcc (GCC) 8.1.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.18.10-marune 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: WARNINGS 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.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.158-i686: OK linux-4.4.158-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.129-i686: OK linux-4.9.129-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.72-i686: OK linux-4.14.72-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.10-i686: OK linux-4.18.10-x86_64: OK linux-4.19-rc5-i686: OK linux-4.19-rc5-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi, >-Original Message- >From: Yeh, Andy >Sent: Wednesday, September 26, 2018 11:19 PM >To: Sakari Ailus ; Chen, Ping-chung > >Hi Sakari, PC, >sensors that do need >digital gain applied, too --- assuming it'd be >combined with the TRY_EXT_CTRLS rounding flags. >> >> There might be many kinds of discrete DG formats. For imx208, DG=2^n, >> but for other sensors, DG could be 2*n, 5*n, or other styles. If HAL >> needs to > >I guess the most common is multiplication and a bit shift (by e.g. 8), e.g. >multiplying the value by a 16-bit number with a 8-bit fractional part. >The >imx208 apparently lacks the multiplication and only has the bit shift. > >Usually there's some sort of technical reason for the choice of the >digital gain implementation and therefore I expect at least the vast >majority of the implementations to be either of the two. >We shall ensure the expansibility of this architecture to include other kind >of styles in the future. Is this API design architecture-wise ok? Indeed. Seems it is hard to cover all rules and HAL needs complex flow to judge the DG value. Hi Sakari, could you provide an example that how HAL uses the modified interface to set available digital gain? > >> cover all cases, kernel will have to update more information to this >> control. Another problem is should HAL take over the SMIA calculation? >> If so, kernel will also need to update SMIA parameters to user space >> (or create an addition filed for SMIA in the configuration XML file). > >The parameters for the analogue gain model should come from the driver, yes. >We do not have controls for that purpose but they can (and should) be added. > >How about still follow PC's proposal to implement in XML? It was in IQ tuning >file before which is in userspace. Even I proposed to PC to study with ICG SW >team whether this info could be retrieved from 3A algorithm. Hi Andy, because we has to use total gain instead of AG in 3A for the WA, our tuning data of imx208 will not include SMIA of AG anymore. So HAL has no way to retrieve correct SMIA parameters of AG from 3A. Thanks, PC Chen >-- >Regards, > >Sakari Ailus >sakari.ai...@linux.intel.com
[linuxtv-media:request_api 77/77] drivers/staging/media/sunxi/cedrus/cedrus.c:93 cedrus_init_ctrls() error: potential null dereference 'ctx->ctrls'. (kzalloc returns null)
tree: git://linuxtv.org/media_tree.git request_api
head: 50e761516f2b8c0cdeb31a8c6ca1b4ef98cd13f1
commit: 50e761516f2b8c0cdeb31a8c6ca1b4ef98cd13f1 [77/77] media: platform: Add
Cedrus VPU decoder driver
smatch warnings:
drivers/staging/media/sunxi/cedrus/cedrus.c:93 cedrus_init_ctrls() error:
potential null dereference 'ctx->ctrls'. (kzalloc returns null)
vim +93 drivers/staging/media/sunxi/cedrus/cedrus.c
57
58 static int cedrus_init_ctrls(struct cedrus_dev *dev, struct cedrus_ctx
*ctx)
59 {
60 struct v4l2_ctrl_handler *hdl = >hdl;
61 struct v4l2_ctrl *ctrl;
62 unsigned int ctrl_size;
63 unsigned int i;
64
65 v4l2_ctrl_handler_init(hdl, CEDRUS_CONTROLS_COUNT);
66 if (hdl->error) {
67 v4l2_err(>v4l2_dev,
68 "Failed to initialize control handler\n");
69 return hdl->error;
70 }
71
72 ctrl_size = sizeof(ctrl) * CEDRUS_CONTROLS_COUNT + 1;
73
74 ctx->ctrls = kzalloc(ctrl_size, GFP_KERNEL);
75 memset(ctx->ctrls, 0, ctrl_size);
76
77 for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
78 struct v4l2_ctrl_config cfg = { 0 };
79
80 cfg.elem_size = cedrus_controls[i].elem_size;
81 cfg.id = cedrus_controls[i].id;
82
83 ctrl = v4l2_ctrl_new_custom(hdl, , NULL);
84 if (hdl->error) {
85 v4l2_err(>v4l2_dev,
86 "Failed to create new custom
control\n");
87
88 v4l2_ctrl_handler_free(hdl);
89 kfree(ctx->ctrls);
90 return hdl->error;
91 }
92
> 93 ctx->ctrls[i] = ctrl;
94 }
95
96 ctx->fh.ctrl_handler = hdl;
97 v4l2_ctrl_handler_setup(hdl);
98
99 return 0;
100 }
101
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH v3 4/9] media: dt-bindings: tvp5150: Add input port connectors DT bindings
On Tue, 18 Sep 2018 15:14:48 +0200, Marco Felsch wrote: > The TVP5150/1 decoders support different video input sources to their > AIP1A/B pins. > > Possible configurations are as follows: > - Analog Composite signal connected to AIP1A. > - Analog Composite signal connected to AIP1B. > - Analog S-Video Y (luminance) and C (chrominance) > signals connected to AIP1A and AIP1B respectively. > > This patch extends the device tree bindings documentation to describe > how the input connectors for these devices should be defined in a DT. > > Signed-off-by: Marco Felsch > --- > Changelog: > > v3: > - remove examples for one and two inputs > - replace space by tabs > > v2: > - adapt port layout in accordance with > https://www.spinics.net/lists/linux-media/msg138546.html with the > svideo-connector deviation (use only one endpoint) > > .../devicetree/bindings/media/i2c/tvp5150.txt | 92 +-- > 1 file changed, 85 insertions(+), 7 deletions(-) > Reviewed-by: Rob Herring
Re: [PATCH 4/5] v4l: controls: QUERY_EXT_CTRL support for base, prefix and unit
Hi Sakari, I love your patch! Perhaps something to improve: [auto build test WARNING on linuxtv-media/master] [also build test WARNING on v4.19-rc5 next-20180926] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Sakari-Ailus/Add-units-to-controls/20180925-183703 base: git://linuxtv.org/media_tree.git master reproduce: make htmldocs All warnings (new ones prefixed by >>): include/net/mac80211.h:977: warning: Function parameter or member 'status.ampdu_ack_len' not described in 'ieee80211_tx_info' include/net/mac80211.h:977: warning: Function parameter or member 'status.ampdu_len' not described in 'ieee80211_tx_info' include/net/mac80211.h:977: warning: Function parameter or member 'status.antenna' not described in 'ieee80211_tx_info' include/net/mac80211.h:977: warning: Function parameter or member 'status.tx_time' not described in 'ieee80211_tx_info' include/net/mac80211.h:977: warning: Function parameter or member 'status.is_valid_ack_signal' not described in 'ieee80211_tx_info' include/net/mac80211.h:977: warning: Function parameter or member 'status.status_driver_data' not described in 'ieee80211_tx_info' include/net/mac80211.h:977: warning: Function parameter or member 'driver_rates' not described in 'ieee80211_tx_info' include/net/mac80211.h:977: warning: Function parameter or member 'pad' not described in 'ieee80211_tx_info' include/net/mac80211.h:977: warning: Function parameter or member 'rate_driver_data' not described in 'ieee80211_tx_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.chain_signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.filtered' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_failed' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_count' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.lost_packets' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_tdls_pkt_time' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_retries' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_failed' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack_signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.ack_signal_filled' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.avg_ack_signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.packets' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.bytes' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.last_rate' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.msdu' not described in 'sta_info' include/linux/mod_devicetable.h:763: warning: Function parameter or member 'driver_data' not described in 'typec_device_id' kernel/sched/fair.c:3371: warning: Function parameter or member 'flags' not described in 'attach_entity_load_avg' arch/x86/include/asm/atomic.h:84: warning: Excess function parameter 'i' description in 'arch_atomic_sub_and_test' arch/x86/include/asm/atomic.h:84: warning: Excess function parameter 'v' description in 'arch_atomic_sub_and_test' arch/x86/include/asm/atomic.h:96: warning: Excess function parameter 'v' description in 'arch_atomic_inc' arch/x86/include/asm/atomic.h:109: warning: Excess function parameter 'v' description in 'arch_atomic_dec' arch/x86/include/asm/atomic.h:124: warning: Excess function parameter 'v' description in 'arch_atomic_dec_and_test' arch/x86/include/asm/atomic.h:138: warning: Excess function parameter 'v' description in 'arch_atomic_inc_and_test' arch/x86/include/asm/atomic.h:153: warning: Excess function parameter 'i' description in 'arch_atomic_add_negative' arch/x86/include/asm/atomic.h:153: warning: Excess function parame
[PATCH] media: rtl28xxu: add support for Sony CXD2837ER slave demod
Since 2018 some new revisions of RTL2832P based devices having
Sony CXD2837ER as a slave demodulator instead of Panasonic MN88473.
CXD2837ER handled in DVB_CXD2841ER module but it's has a lack of control.
So slave demod has to be reseted by GPIO0 before detecting to woke up
CXD2837ER.
Signed-off-by: Nikita Gerasimov
---
drivers/media/usb/dvb-usb-v2/Kconfig| 1 +
drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 40 +++--
drivers/media/usb/dvb-usb-v2/rtl28xxu.h | 4 ++-
3 files changed, 42 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/dvb-usb-v2/Kconfig
b/drivers/media/usb/dvb-usb-v2/Kconfig
index df4412245a8a..511e3f270308 100644
--- a/drivers/media/usb/dvb-usb-v2/Kconfig
+++ b/drivers/media/usb/dvb-usb-v2/Kconfig
@@ -133,6 +133,7 @@ config DVB_USB_RTL28XXU
depends on DVB_USB_V2 && I2C_MUX
select DVB_MN88472 if MEDIA_SUBDRV_AUTOSELECT
select DVB_MN88473 if MEDIA_SUBDRV_AUTOSELECT
+ select DVB_CXD2841ER if MEDIA_SUBDRV_AUTOSELECT
select DVB_RTL2830
select DVB_RTL2832
select DVB_RTL2832_SDR if (MEDIA_SUBDRV_AUTOSELECT && MEDIA_SDR_SUPPORT)
diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
index a970224a94bd..917129404668 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
@@ -384,6 +384,7 @@ static int rtl2832u_read_config(struct dvb_usb_device *d)
struct rtl28xxu_req req_r828d = {0x0074, CMD_I2C_RD, 1, buf};
struct rtl28xxu_req req_mn88472 = {0xff38, CMD_I2C_RD, 1, buf};
struct rtl28xxu_req req_mn88473 = {0xff38, CMD_I2C_RD, 1, buf};
+ struct rtl28xxu_req req_cxd2837er = {0xfdd8, CMD_I2C_RD, 1, buf};
struct rtl28xxu_req req_si2157 = {0x00c0, CMD_I2C_RD, 1, buf};
struct rtl28xxu_req req_si2168 = {0x00c8, CMD_I2C_RD, 1, buf};
@@ -540,7 +541,18 @@ static int rtl2832u_read_config(struct dvb_usb_device *d)
/* probe slave demod */
if (dev->tuner == TUNER_RTL2832_R828D) {
- /* power on MN88472 demod on GPIO0 */
+ /* power off slave demod on GPIO0 to reset CXD2837ER */
+ ret = rtl28xxu_wr_reg_mask(d, SYS_GPIO_OUT_VAL, 0x00, 0x01);
+ if (ret)
+ goto err;
+
+ ret = rtl28xxu_wr_reg_mask(d, SYS_GPIO_OUT_EN, 0x00, 0x01);
+ if (ret)
+ goto err;
+
+ msleep(50);
+
+ /* power on slave demod on GPIO0 */
ret = rtl28xxu_wr_reg_mask(d, SYS_GPIO_OUT_VAL, 0x01, 0x01);
if (ret)
goto err;
@@ -553,7 +565,7 @@ static int rtl2832u_read_config(struct dvb_usb_device *d)
if (ret)
goto err;
- /* check MN88472 answers */
+ /* check slave answers */
ret = rtl28xxu_ctrl_msg(d, _mn88472);
if (ret == 0 && buf[0] == 0x02) {
dev_dbg(>intf->dev, "MN88472 found\n");
@@ -567,6 +579,13 @@ static int rtl2832u_read_config(struct dvb_usb_device *d)
dev->slave_demod = SLAVE_DEMOD_MN88473;
goto demod_found;
}
+
+ ret = rtl28xxu_ctrl_msg(d, _cxd2837er);
+ if (ret == 0 && buf[0] == 0xb1) {
+ dev_dbg(>intf->dev, "CXD2837ER found\n");
+ dev->slave_demod = SLAVE_DEMOD_CXD2837ER;
+ goto demod_found;
+ }
}
if (dev->tuner == TUNER_RTL2832_SI2157) {
/* check Si2168 ID register; reg=c8 val=80 */
@@ -989,6 +1008,23 @@ static int rtl2832u_frontend_attach(struct
dvb_usb_adapter *adap)
}
dev->i2c_client_slave_demod = client;
+ } else if (dev->slave_demod == SLAVE_DEMOD_CXD2837ER) {
+ struct cxd2841er_config cxd2837er_config = {};
+
+ cxd2837er_config.i2c_addr = 0xd8;
+ cxd2837er_config.xtal = SONY_XTAL_20500;
+ cxd2837er_config.flags = (CXD2841ER_AUTO_IFHZ |
+ CXD2841ER_NO_AGCNEG | CXD2841ER_TSBITS |
+ CXD2841ER_EARLY_TUNE | CXD2841ER_TS_SERIAL);
+ adap->fe[1] = dvb_attach(cxd2841er_attach_t_c,
+_config,
+>i2c_adap);
+ if (!adap->fe[1]) {
+ dev->slave_demod = SLAVE_DEMOD_NONE;
+ goto err_slave_demod_failed;
+ }
+ adap->fe[1]->id = 1;
+ dev->i2c_client_slave_demod = NULL;
} else {
struct si2168_config si2168_config = {};
struct i2c_adapter *adapter;
diff
[GIT PULL for 4.20] Unlocked control grab and imx319 driver
Hi Mauro, Here's a driver for Sony imx319 sensor and an unlocked version of v4l2_ctrl_grab() which is used by the driver. Please pull. The following changes since commit 985cdcb08a0488558d1005139596b64d73bee267: media: ov5640: fix restore of last mode set (2018-09-17 15:33:38 -0400) are available in the git repository at: ssh://linuxtv.org/git/sailus/media_tree.git tags/for-4.20-8-sign-2 for you to fetch changes up to 0e5924d5c0051228a59adffa1e48777f9ebd60de: media: add imx319 camera sensor driver (2018-09-27 00:19:40 +0300) v4l2_ctrl_grab and imx319 Bingbu Cao (1): media: add imx319 camera sensor driver Sakari Ailus (2): v4l: ctrl: Remove old documentation from v4l2_ctrl_grab v4l: ctrl: Provide unlocked variant of v4l2_ctrl_grab MAINTAINERS |7 + drivers/media/i2c/Kconfig| 11 + drivers/media/i2c/Makefile |1 + drivers/media/i2c/imx319.c | 2558 ++ drivers/media/v4l2-core/v4l2-ctrls.c | 14 +- include/media/v4l2-ctrls.h | 26 +- 6 files changed, 2606 insertions(+), 11 deletions(-) create mode 100644 drivers/media/i2c/imx319.c -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [ANN] Draft Agenda for the media summit on Thursday Oct 25th in Edinburgh
Em Mon, 24 Sep 2018 19:41:13 +0200 Hans Verkuil escreveu: > On 09/24/2018 07:12 PM, Mauro Carvalho Chehab wrote: > > Em Mon, 24 Sep 2018 16:42:13 +0200 > > Hans Verkuil escreveu: > > > >> Hi all, > >> > >> We are organizing a media mini-summit on Thursday October 25th in > >> Edinburgh, Edinburgh International Conference Centre. > >> > >> If you plan to attend, please let Mauro know. It is open for all, but > >> we have a limited number of seats. > > > > No need to let me explicitly know in advance, but be sure to register for > > it at the ELCE/OSS register site. I'll use their tracking system to > > know who will be there. We have a limited number of seats there, and > > I'm relying on their system to control the number of attendees for > > us. > > How many registrations do we have now? At the moment, there are 75 people subscribed to it! From them, on a quick glance, I was able to recognize ~18 names from people that are already contributors to media. Yet, running: $ IFS=$'\n'; for i in $(cat ~/Documentos/attendees_list.csv|cut -d\; -f1-2|sed 's,;, ,'); do echo -n "$i;";git log --oneline --author "$i" |wc -l; done I noticed 32 people[1] that submitted at least one Kernel patch (either to media or to other subsystems), where 30 of them submitted more than 7 patches. So, I guess we'll have full house this time. Thanks, Mauro [1] I'm seeking for names, not e-mails. So, this is just a hint, as it may have more than one people with the same name.
RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Sakari, PC,
>-Original Message-
>From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
>Sent: Wednesday, September 26, 2018 6:12 PM
>To: Chen, Ping-chung
>Cc: Ricardo Ribalda Delgado ; Laurent Pinchart
>; Sakari Ailus ;
>tf...@chromium.org; sylwester.nawro...@gmail.com; linux-media me...@vger.kernel.org>; Yeh, Andy ; Lai, Jim
>; grund...@chromium.org; Mani, Rajmohan
>
>Subject: Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
>
>Hi Ping-chung,
>
>On Wed, Sep 26, 2018 at 02:27:01AM +, Chen, Ping-chung wrote:
>> Hi Sakari,
>>
>> >-Original Message-
>> >From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
>> >Sent: Wednesday, September 26, 2018 5:55 AM
>>
>> >Hi Ping-chung,
>>
>> >On Tue, Sep 25, 2018 at 10:17:48AM +, Chen, Ping-chung wrote:
>> >...
>> > > > > 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
>> > >
>> > > How about creating an additional control to handle the case of
>> > > V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG
>> > > separately for the general sensor usage by V4L2_CID_ANALOGUE_GAIN
>> > > and V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition
>> > > of setting total_gain=AGxDG.
>> >
>> > >How do you update the two other controls if the user updates the
>V4L2_CID_GAIN control?
>> >
>> > In imx208 driver:
>> >
>> > Add two pointers *digital_gain and *analog_gain of v4l2_ctrl in the global
>structure imx208.
>> > static int imx208_init_controls(struct imx208 *imx208) {
>> > Imx208->digital_gain = v4l2_ctrl_new_std(..., V4L2_CID_DIGITAL_GAIN,
>> > Imx208->...); analog_gain =
>> > Imx208->v4l2_ctrl_new_std(...,V4L2_CID_ANALOGUE_GAIN, ...);
>> >
>> > static int imx208_set_ctrl(struct v4l2_ctrl *ctrl) { ...
>> >case V4L2_CID_ANALOGUE_GAIN:
>> >ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
>ctrl->val);
>> >break;
>> >case V4L2_CID_DIGITAL_GAIN:
>> >ret = imx208_update_digital_gain(imx208, 2, ctrl->val);
>> >break;
>> >case V4L2_CID_ GAIN:
>> >ret = imx208_update_gain(imx208, 2, ctrl->val); // total gain
>> >break;
>> > }
>> >
>> > Then the implementation of imx208_update_gain():
>> > static int imx208_update_gain(struct imx208 *imx208, u32 len, u32
>> > val) {
>> >digital_gain = (val - 1) / ag_max;
>> >digital_gain = map_to_real_DG(digital_gain);// map to 1x,
>2x, 4x, 8x, 16x
>> >digital_gain_code = digital_gain << 8 // DGx256 for
>DG_code
>> >ret = imx208_update_digital_gain(imx208, 2, digital_gain_code);
>> >imx208->digital_gain->val = digital_gain_code;
>> >analog_gain = val/digital_gain;
>> >analog_gain_code = SMIA_AG_to_Code(analog_gain); // AG =
>256/(256-ag_code)
>> >ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
>analog_gain_code);
>> >imx208->digital_gain->val = analog_gain_code;
>>
>> >How about putting this piece of code to the user space instead?
>>
>> >Some work would be needed to generalise it in order for it to work on other
>sensors that do need >digital gain applied, too --- assuming it'd be combined
>with the TRY_EXT_CTRLS rounding flags.
>>
>> There might be many kinds of discrete DG formats. For imx208, DG=2^n,
>> but for other sensors, DG could be 2*n, 5*n, or other styles. If HAL
>> needs to
>
>I guess the most common is multiplication and a bit shift (by e.g. 8), e.g.
>multiplying the value by a 16-bit number with a 8-bit fractional part. The
>imx208 apparently lacks the multiplication and only has the bit shift.
>
>Usually there's some sort of technical reason for the choice of the digital
>gain
>implementation and therefore I expect at least the vast majority of the
>implementations to be either of the two.
Re: [PATCH 03/13] v4l2-mc: switch it to use the new approach to setup pipelines
Hi Mauro,
Thank you for the patch.
On Wednesday, 1 August 2018 18:55:05 EEST Mauro Carvalho Chehab wrote:
> Instead of relying on a static map for pids, use the new sig_type
> "taint" type to setup the pipelines with the same tipe between
s/tipe/type/
> different entities.
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/media-entity.c | 26 +++
> drivers/media/v4l2-core/v4l2-mc.c | 73 ---
> include/media/media-entity.h | 19
> 3 files changed, 101 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 3498551e618e..0b1cb3559140 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -662,6 +662,32 @@ static void __media_entity_remove_link(struct
> media_entity *entity, kfree(link);
> }
>
> +int media_get_pad_index(struct media_entity *entity, bool is_sink,
> + enum media_pad_signal_type sig_type)
> +{
> + int i;
is is never negative, please use an unsigned int.
> + bool pad_is_sink;
> +
> + if (!entity)
> + return -EINVAL;
> +
> + for (i = 0; i < entity->num_pads; i++) {
> + if (entity->pads[i].flags == MEDIA_PAD_FL_SINK)
> + pad_is_sink = true;
> + else if (entity->pads[i].flags == MEDIA_PAD_FL_SOURCE)
> + pad_is_sink = false;
> + else
> + continue; /* This is an error! */
> +
> + if (pad_is_sink != is_sink)
> + continue;
> + if (entity->pads[i].sig_type == sig_type)
> + return i;
> + }
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(media_get_pad_index);
> +
> int
> media_create_pad_link(struct media_entity *source, u16 source_pad,
>struct media_entity *sink, u16 sink_pad, u32 flags)
> diff --git a/drivers/media/v4l2-core/v4l2-mc.c
> b/drivers/media/v4l2-core/v4l2-mc.c index 982bab3530f6..1925e1a3b861 100644
> --- a/drivers/media/v4l2-core/v4l2-mc.c
> +++ b/drivers/media/v4l2-core/v4l2-mc.c
> @@ -28,7 +28,7 @@ int v4l2_mc_create_media_graph(struct media_device *mdev)
> struct media_entity *io_v4l = NULL, *io_vbi = NULL, *io_swradio = NULL;
> bool is_webcam = false;
> u32 flags;
> - int ret;
> + int ret, pad_sink, pad_source;
>
> if (!mdev)
> return 0;
> @@ -97,29 +97,52 @@ int v4l2_mc_create_media_graph(struct media_device
> *mdev) /* Link the tuner and IF video output pads */
> if (tuner) {
> if (if_vid) {
> - ret = media_create_pad_link(tuner, TUNER_PAD_OUTPUT,
> - if_vid,
> - IF_VID_DEC_PAD_IF_INPUT,
> + pad_source = media_get_pad_index(tuner, false,
> + PAD_SIGNAL_ANALOG);
> + pad_sink = media_get_pad_index(if_vid, true,
> +PAD_SIGNAL_ANALOG);
> + if (pad_source < 0 || pad_sink < 0)
> + return -EINVAL;
> + ret = media_create_pad_link(tuner, pad_source,
> + if_vid, pad_sink,
> MEDIA_LNK_FL_ENABLED);
> if (ret)
> return ret;
> - ret = media_create_pad_link(if_vid, IF_VID_DEC_PAD_OUT,
> - decoder, DEMOD_PAD_IF_INPUT,
> +
> + pad_source = media_get_pad_index(if_vid, false,
> + PAD_SIGNAL_DV);
> + pad_sink = media_get_pad_index(decoder, true,
> +PAD_SIGNAL_DV);
> + if (pad_source < 0 || pad_sink < 0)
> + return -EINVAL;
> + ret = media_create_pad_link(if_vid, pad_source,
> + decoder, pad_sink,
> MEDIA_LNK_FL_ENABLED);
> if (ret)
> return ret;
> } else {
> - ret = media_create_pad_link(tuner, TUNER_PAD_OUTPUT,
> - decoder, DEMOD_PAD_IF_INPUT,
> + pad_source = media_get_pad_index(tuner, false,
> + PAD_SIGNAL_ANALOG);
> + pad_sink = media_get_pad_index(decoder, true,
> +PAD_SIGNAL_ANALOG);
> + if (pad_source < 0 || pad_sink < 0)
> + return -EINVAL;
> + ret =
Re: [ANN] Draft Agenda for the media summit on Thursday Oct 25th in Edinburgh
Hi Hans and Mauro, On 9/24/18 11:42 AM, Hans Verkuil wrote: > Hi all, > > We are organizing a media mini-summit on Thursday October 25th in > Edinburgh, Edinburgh International Conference Centre. > > If you plan to attend, please let Mauro know. It is open for all, but > we have a limited number of seats. I believe I also I selected attendance when registering for the conference. Please add my name too. > > Name of the room for the summit: TBD > > Currently known attendees (please add/remove names as needed): > > Sakari Ailus > Mauro Carvalho Chehab > Ezequiel Garcia > Michael Ira Krufky > Laurent Pinchart > Ricardo Ribalda Delgado > Hans Verkuil > Sean Young > > Agenda (First draft!) > = > > General remarks: the given start/end times for the various topics are > approximate since it is always hard to predict how long a discussion will > take. > If people are attending other summits and those conflict with specific media > topics they want to be part of, then let me know and we can rearrange the > schedule to (hopefully) accommodate that. > > 9:00-9:15: Introduction (Hans Verkuil) > > 9:15-9:30: Status of the HDMI CEC kernel support (Hans Verkuil) > Give a quick overview of the status: what has been merged, what is > still pending, what is under development. > > 9:30-9:45: Save/restore controls from MTD (Ricardo Ribalda Delgado) > Industrial/Scientific sensors usually come with very extensive > calibration information such as: per column gain, list of dead > pixels, temperature sensor offset... etc > > We are saving that information on an flash device that is located > by the sensor. > > Show how we are integrating that calibration flash with v4l2-ctrl. > And if this feature is useful for someone else and upstream it. > > 9:45-11:00: Complex Cameras (Mauro Carvalho Chehab) > I expect that we could have something to discuss there about complex > cameras. So, I'd reserve a 50 mins slot for it. > > The idea is to discuss about the undergoing work with complex camera > development is happening. > > As we're working to merge request API, another topic for discussion > is how to add support for requests on it (or on a separate but related > library). > > 11:00-11:15: Break > > 11:15-12:00: Automated Testing (Ezequiel Garcia) > There is a lot of discussion going on around testing, > so it's a good opportunity for us to talk about our > current testing infrastructure. > > We are already doing a good job with v4l2-compliance. > Can we do more? > > Lunch > > 13:30-14:30: Stateless Codec userspace (Hans Verkuil) > Support for stateless codecs and Request API should be merged for > 4.20, and the next step is to discuss how to organize the userspace > support. > > Hopefully by the time the media summit starts we'll have some better > ideas of what we want in this area. > > 14:30-15:15: Which ioctls should be replaced with better versions? (Hans > Verkuil) > 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=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. > > Discuss what possible other ioctls are candidates for a refresh. > > 15:15-15:30: Break > > 15:30-16:00: Discuss the media development process > Since we are all here, discuss any issues there may be with the media > subsystem development process. Anything to improve? > > 16:00-16:15: Wrap up > Create action items (and who will take care of them) if needed. > Summarize and conclude the day. > > End of the day: Key Signing Party > > Regards, > > Hans > Thanks Helen
Re: [PATCH 02/13] media: v4l2: taint pads with the signal types for consumer devices
Hi Mauro,
Thank you for the patch.
Could you please CC me on patches touching the media controller core ? I can
send a MAINTAINERS patch to make sure that gets handled automatically.
On Wednesday, 1 August 2018 18:55:04 EEST Mauro Carvalho Chehab wrote:
> Consumer devices are provided with a wide diferent range of types
> supported by the same driver, allowing different configutations.
>
> In order to make easier to setup media controller links, "taint"
> pads with the signal type it carries.
>
> While here, get rid of DEMOD_PAD_VBI_OUT, as the signal it carries
> is actually the same as the normal video output.
>
> The difference happens at the video/VBI interface:
> - for VBI, only the hidden lines are streamed;
> - for video, the stream is usually cropped to hide the
> vbi lines.
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/dvb-frontends/au8522_decoder.c | 3 ++
> drivers/media/i2c/msp3400-driver.c | 2 ++
> drivers/media/i2c/saa7115.c | 2 ++
> drivers/media/i2c/tvp5150.c | 2 ++
> drivers/media/pci/saa7134/saa7134-core.c | 2 ++
> drivers/media/tuners/si2157.c| 3 ++
> drivers/media/usb/dvb-usb-v2/mxl111sf.c | 2 ++
> drivers/media/v4l2-core/tuner-core.c | 5 +++
> include/media/media-entity.h | 35
> 9 files changed, 56 insertions(+)
[snip]
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 3aa3d58d1d58..8bfbe6b59fa9 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -155,6 +155,40 @@ struct media_link {
> bool is_backlink;
> };
>
> +/**
> + * struct media_pad_signal_type - type of the signal inside a media pad
I'd say "carried by a media pad" instead of "inside a media pad".
> + *
> + * @PAD_SIGNAL_DEFAULT
Shouldn't we use a MEDIA_PAD_ prefix ?
> + * Default signal. Use this when all inputs or all outputs are
> + * uniquely identified by the pad number.
How about "Use this when the pad can carry a single signal type" ? I
understand your formulation as meaning that all pads of the entity have to be
of the default type, or none can be.
> + * @PAD_SIGNAL_ANALOG
This isn't a very good name given that PAD_SIGNAL_TV_CARRIERS is also analog.
> + * The pad contains an analog signa. It can be Radio Frequency,
s/signa/signal/
> + * Intermediate Frequency or baseband signal.
> + * Tuner inputs, composite and s-video signals should use it.
> + * On tuner sources, this is used for digital TV demodulators and for
> + * IF-PLL demodulator like tda9887.
> + * @PAD_SIGNAL_TV_CARRIERS
> + * The pad contains analog signals carrying either a digital or an analog
> + * modulated (or baseband) signal.
As above, maybe "The pad carries either ...".
> This is provided by tuner source
> + * pads and used by analog TV standard decoders and by digital TV demods.
> + * @PAD_SIGNAL_DV
> + * Contains a digital video signal, with can be a bitstream of samples
> + * taken from an analog TV video source. On such case, it usually
> + * contains the VBI data on it.
> + * @PAD_SIGNAL_AUDIO
> + * Contains an Intermediate Frequency analog signal from an audio
> + * sub-carrier or an audio bitstream. IF signals are provided by tuners
> + * and consumed by audio AM/FM decoders. Bitstream audio is provided by
s/ / /
> + * an audio decoder.
Generally speaking the types you propose here seem quite ad-hoc, without much
coherency. For instance you split analog and digital video, but group all
audio under a single type. It's also not very clear from the description how
to handle analog video, as it could match both PAD_SIGNAL_ANALOG and
PAD_SIGNAL_TV_CARRIERS. Both of those types also accept analog baseband
signals. I think this should be reworked, it doesn't sound very usable except
for the specific use case that this series tries to address.
> + */
> +enum media_pad_signal_type {
> + PAD_SIGNAL_DEFAULT = 0,
> + PAD_SIGNAL_ANALOG,
> + PAD_SIGNAL_TV_CARRIERS,
> + PAD_SIGNAL_DV,
> + PAD_SIGNAL_AUDIO,
> +};
> +
> /**
> * struct media_pad - A media pad graph object.
> *
> @@ -169,6 +203,7 @@ struct media_pad {
> struct media_gobj graph_obj;/* must be first field in struct */
> struct media_entity *entity;
> u16 index;
> + enum media_pad_signal_type sig_type;
Missing kerneldoc and Documentation/ update ? It's important to document the
use cases, and in particular whether the type should be static or can vary (as
in whether a pad can carry different types over time).
> unsigned long flags;
> };
--
Regards,
Laurent Pinchart
Re: [PATCH 2/2] v4l: ctrl: Provide unlocked variant of v4l2_ctrl_grab
On 09/26/2018 10:09 AM, Sakari Ailus wrote:
> Sometimes it may be necessary to grab a control while holding the control
> handler's lock. Provide an unlocked variant of v4l2_ctrl_grab for the
> purpose --- it's called __v4l2_ctrl_grab.
>
> Signed-off-by: Sakari Ailus
Acked-by: Hans Verkuil
Regards,
Hans
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 8
> include/media/v4l2-ctrls.h | 26 +-
> 2 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c
> index ab393adf51eb..4c0ecf29d278 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -2511,14 +2511,15 @@ void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool
> active)
> }
> EXPORT_SYMBOL(v4l2_ctrl_activate);
>
> -void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
> +void __v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
> {
> bool old;
>
> if (ctrl == NULL)
> return;
>
> - v4l2_ctrl_lock(ctrl);
> + lockdep_assert_held(ctrl->handler->lock);
> +
> if (grabbed)
> /* set V4L2_CTRL_FLAG_GRABBED */
> old = test_and_set_bit(1, >flags);
> @@ -2527,9 +2528,8 @@ void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool
> grabbed)
> old = test_and_clear_bit(1, >flags);
> if (old != grabbed)
> send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_FLAGS);
> - v4l2_ctrl_unlock(ctrl);
> }
> -EXPORT_SYMBOL(v4l2_ctrl_grab);
> +EXPORT_SYMBOL(__v4l2_ctrl_grab);
>
> /* Log the control name and value */
> static void log_ctrl(const struct v4l2_ctrl *ctrl,
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index f615ba1b29dd..ff89df428f79 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -729,6 +729,22 @@ struct v4l2_ctrl *v4l2_ctrl_find(struct
> v4l2_ctrl_handler *hdl, u32 id);
> void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active);
>
> /**
> + * __v4l2_ctrl_grab() - Unlocked variant of v4l2_ctrl_grab.
> + *
> + * @ctrl:The control to (de)activate.
> + * @grabbed: True if the control should become grabbed.
> + *
> + * This sets or clears the V4L2_CTRL_FLAG_GRABBED flag atomically.
> + * Does nothing if @ctrl == NULL.
> + * The V4L2_EVENT_CTRL event will be generated afterwards.
> + * This will usually be called when starting or stopping streaming in the
> + * driver.
> + *
> + * This function assumes that the control handler is locked by the caller.
> + */
> +void __v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
> +
> +/**
> * v4l2_ctrl_grab() - Mark the control as grabbed or not grabbed.
> *
> * @ctrl:The control to (de)activate.
> @@ -743,7 +759,15 @@ void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool
> active);
> * This function assumes that the control handler is not locked and will
> * take the lock itself.
> */
> -void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
> +static inline void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
> +{
> + if (!ctrl)
> + return;
> +
> + v4l2_ctrl_lock(ctrl);
> + __v4l2_ctrl_grab(ctrl, grabbed);
> + v4l2_ctrl_unlock(ctrl);
> +}
>
> /**
> *__v4l2_ctrl_modify_range() - Unlocked variant of v4l2_ctrl_modify_range()
>
Re: [PATCH 1/2] v4l: ctrl: Remove old documentation from v4l2_ctrl_grab
On 09/26/2018 10:09 AM, Sakari Ailus wrote:
> v4l2_ctrl_grab() is documented in the header; there's no need to have a
> comment explaining what the function does in the .c file.
>
> Signed-off-by: Sakari Ailus
Acked-by: Hans Verkuil
Regards,
Hans
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c
> index ee006d34c19f..ab393adf51eb 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -2511,12 +2511,6 @@ void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool
> active)
> }
> EXPORT_SYMBOL(v4l2_ctrl_activate);
>
> -/* Grab/ungrab a control.
> - Typically used when streaming starts and you want to grab controls,
> - preventing the user from changing them.
> -
> - Just call this and the framework will block any attempts to change
> - these controls. */
> void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
> {
> bool old;
>
Re: [PATCH 0/5] Add units to controls
On Tue, Sep 25, 2018 at 02:30:31PM +0200, Sakari Ailus wrote: > On Tue, Sep 25, 2018 at 01:48:02PM +0200, Helmut Grohne wrote: > > 1. There are a number of controls whose value is not easily described as > >either linear or exponential. I'm faced with at least two controls > >that actually are floating point numbers. One with two bits for the > >exponent and one (strange) bit for the mantissa (no joke) and another > >with three bits for the exponent and four bits for the mantissa. > >Neither can suitably be represented. > The proposal does not address all potential situations, that's true. > There's no way to try to represent everything out there (without > enumerating the values) in an easily generalised way but something can be > done. Sure, but the rounding control flag would address that. The rounding control flag could also solve exponential controls. Since there is a certain overlap here. These proposals should be discussed together. We should avoid solving problems twice. > There are devices such as some flash LED controllers where the flash current > if simply a value you can pick from the list. It's currently implemented as > an integer control. AFAIR the driver is drivers/leds/leds-aat1290.c . Possibly, relaxing the "each control id has a fixed type" requirement is another option. Allowing an integer menu wherever an integer is specified could solve issues such as these in a different way. Again it is fine that your proposal, does not cover that. Still these uapi changes are interdependent and therefore need to be considered together. > The fact is that the unit is specific to hardware. The documentation > documents something, and often suggests a unit, but if the hardware does > something else, then that's what you get --- independently of what the > documentation says. > > Hence the need to convey it via the API. That is vaguely convincing. It still seems like a niche case. > Some controls could have the unit set by the framework if that makes sense. I'd use a stronger s/could/should/ here as it directly translates into maintenance cost. > Most drivers shouldn't actually need to touch this if they're fine with > defaults (whenever a control has a default). Exactly this. Therefore I think you shouldn't update smiapp, but the framework instead. > A macro or two, it's not a major change. From the user space point of view, > does it make a difference if a control has no unit or when it's not known > what the unit is? There are situations where there is a difference. If you count things (e.g. reference pictures V4L2_CID_MPEG_VIDEO_MAX_REF_PIC), there is no unit, but that's different from unknown. Having unknown separate from no unit also helps with the transition period where controls lack units. Therefore, i'm in favour of keeping the distinction between unknown and no unit. Still, I don't think that merging them is fundamentally "wrong". > Yes, I think [V4L2_CTRL_FLAG_EXPONENTIAL] can be dropped as I suggested. Ok. Looking forward to the rounding flag then. > > Thus, I think that control over the rounding is tightly related to this > > patchset and needs to be discussed together. > > It addresses some of the same problem area but the implementation is > orthogonal to this. I don't disagree here. Still I think that the question "what should be implemented" (not how) is dependent on those other problems. > Providing that would probably make the base field less useful: the valid > control values could be enumerated by the user using TRY_EXT_CTRLS without > the need to tell the valid values are powers of e.g. two. After dropping V4L2_CTRL_FLAG_EXPONENTIAL, the base field is meaningless, no? > I don't really have a strong opinion on that actually when it comes to the > API itself. The imx208 driver could proceed to use linear relation between > the control value and the digital gain. My worry is just the driver > implementation: this may not be entirely trivial. There's still no way to > address this problem in a generic way otherwise. Yeah, I'm mostly looking at this from an uapi pov (as that is the one that cannot be changed later) and have no good answer here. Allowing integer menus for integers would be easy from a driver pov though. Helmut
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
Hi Ping-chung,
On Wed, Sep 26, 2018 at 02:27:01AM +, Chen, Ping-chung wrote:
> Hi Sakari,
>
> >-Original Message-
> >From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> >Sent: Wednesday, September 26, 2018 5:55 AM
>
> >Hi Ping-chung,
>
> >On Tue, Sep 25, 2018 at 10:17:48AM +, Chen, Ping-chung wrote:
> >...
> > > > > 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
> > >
> > > How about creating an additional control to handle the case of
> > > V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG separately
> > > for the general sensor usage by V4L2_CID_ANALOGUE_GAIN and
> > > V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition of
> > > setting total_gain=AGxDG.
> >
> > >How do you update the two other controls if the user updates the
> > >V4L2_CID_GAIN control?
> >
> > In imx208 driver:
> >
> > Add two pointers *digital_gain and *analog_gain of v4l2_ctrl in the global
> > structure imx208.
> > static int imx208_init_controls(struct imx208 *imx208) {
> > Imx208->digital_gain = v4l2_ctrl_new_std(..., V4L2_CID_DIGITAL_GAIN,
> > Imx208->...); analog_gain =
> > Imx208->v4l2_ctrl_new_std(...,V4L2_CID_ANALOGUE_GAIN, ...);
> >
> > static int imx208_set_ctrl(struct v4l2_ctrl *ctrl) { ...
> > case V4L2_CID_ANALOGUE_GAIN:
> > ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
> > ctrl->val);
> > break;
> > case V4L2_CID_DIGITAL_GAIN:
> > ret = imx208_update_digital_gain(imx208, 2, ctrl->val);
> > break;
> > case V4L2_CID_ GAIN:
> > ret = imx208_update_gain(imx208, 2, ctrl->val); // total gain
> > break;
> > }
> >
> > Then the implementation of imx208_update_gain():
> > static int imx208_update_gain(struct imx208 *imx208, u32 len, u32 val)
> > {
> > digital_gain = (val - 1) / ag_max;
> > digital_gain = map_to_real_DG(digital_gain);// map to 1x,
> > 2x, 4x, 8x, 16x
> > digital_gain_code = digital_gain << 8 // DGx256 for
> > DG_code
> > ret = imx208_update_digital_gain(imx208, 2, digital_gain_code);
> > imx208->digital_gain->val = digital_gain_code;
> > analog_gain = val/digital_gain;
> > analog_gain_code = SMIA_AG_to_Code(analog_gain); // AG =
> > 256/(256-ag_code)
> > ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
> > analog_gain_code);
> > imx208->digital_gain->val = analog_gain_code;
>
> >How about putting this piece of code to the user space instead?
>
> >Some work would be needed to generalise it in order for it to work on other
> >sensors that do need >digital gain applied, too --- assuming it'd be
> >combined with the TRY_EXT_CTRLS rounding flags.
>
> There might be many kinds of discrete DG formats. For imx208, DG=2^n, but
> for other sensors, DG could be 2*n, 5*n, or other styles. If HAL needs to
I guess the most common is multiplication and a bit shift (by e.g. 8), e.g.
multiplying the value by a 16-bit number with a 8-bit fractional part. The
imx208 apparently lacks the multiplication and only has the bit shift.
Usually there's some sort of technical reason for the choice of the digital
gain implementation and therefore I expect at least the vast majority of
the implementations to be either of the two.
> cover all cases, kernel will have to update more information to this
> control. Another problem is should HAL take over the SMIA calculation? If
> so, kernel will also need to update SMIA parameters to user space (or
> create an addition filed for SMIA in the configuration XML file).
The parameters for the analogue gain model should come from the driver,
yes. We do not have controls for that purpose but they can (and should) be
added.
Capturing camera frames from an i.MX51 board.
Dear linux-media Developers, I was assigned the task to port a recent kernel (4.18.6, downloaded from kernel.org) to an old imx51-babbage style board. It works with a 2.6.36 kernel. I know, i.MX51 is "mature" in NXP lingo. Not supported anymore in my mind, but work is work. The i.MX51 board uses a monochrome Aptina MT9V024 driver for the sensor. I patched the DTS and imx-media driver in drivers/staging/media/imx only to later find out that my experience was very similar to the one described in this thread: https://www.spinics.net/lists/linux-media/msg129273.html Trying to start a capture yields EPIPE from v4l2-ctl. Following pointers I found that the node pertaining the ipu_csi0 has two NULL pointers for sink and src_pad in the V4L2 subdev and csi_link_setup() in imx-media-csi.c is never invoked. So I will not repeat the questions in the previous thread, because I suppose I would not get any answer. I have to evaluate which of these two paths costs less: 1. Debug imx-media for my architecture, knowing that it is really specialized for i.MX6. 2. Find another Linux kernel branch supporting i.MX51 and the Aptina driver. Is there any such thing as point (2)? I found: http://git.freescale.com/git/cgit.cgi/imx/linux-imx.git/?h=imx_4.9.11_1.0.0_ga but this branch directly supports OV564x sensors only. I have to minimize time/cost at this point, avoiding to write/rewrite a driver. Thank you in advance, Antonio Tringali
[PATCH 0/2] Unlocked v4l2_ctrl_grab
Hi folks, This set adds an unlocked variant of v4l2_ctrl_grab(). This is needed when there's a need not to release the control lock for grabbing a control. Used by the imx319 driver, but beneficial on a bunch of other sensor drivers, too --- to grab the *flip controls. Sakari Ailus (2): v4l: ctrl: Remove old documentation from v4l2_ctrl_grab v4l: ctrl: Provide unlocked variant of v4l2_ctrl_grab drivers/media/v4l2-core/v4l2-ctrls.c | 14 -- include/media/v4l2-ctrls.h | 26 +- 2 files changed, 29 insertions(+), 11 deletions(-) -- 2.11.0
[PATCH 1/2] v4l: ctrl: Remove old documentation from v4l2_ctrl_grab
v4l2_ctrl_grab() is documented in the header; there's no need to have a
comment explaining what the function does in the .c file.
Signed-off-by: Sakari Ailus
---
drivers/media/v4l2-core/v4l2-ctrls.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index ee006d34c19f..ab393adf51eb 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2511,12 +2511,6 @@ void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool
active)
}
EXPORT_SYMBOL(v4l2_ctrl_activate);
-/* Grab/ungrab a control.
- Typically used when streaming starts and you want to grab controls,
- preventing the user from changing them.
-
- Just call this and the framework will block any attempts to change
- these controls. */
void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
{
bool old;
--
2.11.0
[PATCH 2/2] v4l: ctrl: Provide unlocked variant of v4l2_ctrl_grab
Sometimes it may be necessary to grab a control while holding the control
handler's lock. Provide an unlocked variant of v4l2_ctrl_grab for the
purpose --- it's called __v4l2_ctrl_grab.
Signed-off-by: Sakari Ailus
---
drivers/media/v4l2-core/v4l2-ctrls.c | 8
include/media/v4l2-ctrls.h | 26 +-
2 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index ab393adf51eb..4c0ecf29d278 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2511,14 +2511,15 @@ void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool
active)
}
EXPORT_SYMBOL(v4l2_ctrl_activate);
-void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
+void __v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
{
bool old;
if (ctrl == NULL)
return;
- v4l2_ctrl_lock(ctrl);
+ lockdep_assert_held(ctrl->handler->lock);
+
if (grabbed)
/* set V4L2_CTRL_FLAG_GRABBED */
old = test_and_set_bit(1, >flags);
@@ -2527,9 +2528,8 @@ void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
old = test_and_clear_bit(1, >flags);
if (old != grabbed)
send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_FLAGS);
- v4l2_ctrl_unlock(ctrl);
}
-EXPORT_SYMBOL(v4l2_ctrl_grab);
+EXPORT_SYMBOL(__v4l2_ctrl_grab);
/* Log the control name and value */
static void log_ctrl(const struct v4l2_ctrl *ctrl,
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index f615ba1b29dd..ff89df428f79 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -729,6 +729,22 @@ struct v4l2_ctrl *v4l2_ctrl_find(struct v4l2_ctrl_handler
*hdl, u32 id);
void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active);
/**
+ * __v4l2_ctrl_grab() - Unlocked variant of v4l2_ctrl_grab.
+ *
+ * @ctrl: The control to (de)activate.
+ * @grabbed: True if the control should become grabbed.
+ *
+ * This sets or clears the V4L2_CTRL_FLAG_GRABBED flag atomically.
+ * Does nothing if @ctrl == NULL.
+ * The V4L2_EVENT_CTRL event will be generated afterwards.
+ * This will usually be called when starting or stopping streaming in the
+ * driver.
+ *
+ * This function assumes that the control handler is locked by the caller.
+ */
+void __v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
+
+/**
* v4l2_ctrl_grab() - Mark the control as grabbed or not grabbed.
*
* @ctrl: The control to (de)activate.
@@ -743,7 +759,15 @@ void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool
active);
* This function assumes that the control handler is not locked and will
* take the lock itself.
*/
-void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
+static inline void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
+{
+ if (!ctrl)
+ return;
+
+ v4l2_ctrl_lock(ctrl);
+ __v4l2_ctrl_grab(ctrl, grabbed);
+ v4l2_ctrl_unlock(ctrl);
+}
/**
*__v4l2_ctrl_modify_range() - Unlocked variant of v4l2_ctrl_modify_range()
--
2.11.0
[GIT PULL for 4.20] More fixes and cleanups for 4.20
Hi Mauro, Here are a few cleanups and fixes for 4.20. Sensor drivers as well as sub-device crop target alignment with API documentation. Please pull. The following changes since commit 985cdcb08a0488558d1005139596b64d73bee267: media: ov5640: fix restore of last mode set (2018-09-17 15:33:38 -0400) are available in the git repository at: ssh://linuxtv.org/git/sailus/media_tree.git tags/for-4.20-7-sign for you to fetch changes up to e5b39730005e5f6fd3346b6d27d55e9f57e35212: media: smiapp: Remove unused loop (2018-09-26 10:20:43 +0300) no crop bounds on subdevs, ov5640 fix... Hugues Fruchet (1): media: ov5640: use JPEG mode 3 for 720p Ricardo Ribalda Delgado (1): media: smiapp: Remove unused loop Sakari Ailus (2): v4l: i2c: Add a comment not to use static sub-device names in the future v4l: Remove support for crop default target in subdev drivers drivers/media/i2c/ak881x.c | 1 - drivers/media/i2c/m5mols/m5mols_core.c | 1 + drivers/media/i2c/mt9m111.c| 1 - drivers/media/i2c/mt9t112.c| 6 -- drivers/media/i2c/noon010pc30.c| 1 + drivers/media/i2c/ov2640.c | 1 - drivers/media/i2c/ov5640.c | 2 +- drivers/media/i2c/ov6650.c | 1 - drivers/media/i2c/ov772x.c | 1 - drivers/media/i2c/rj54n1cb0c.c | 1 - drivers/media/i2c/s5c73m3/s5c73m3-core.c | 1 + drivers/media/i2c/s5k4ecgx.c | 1 + drivers/media/i2c/s5k6aa.c | 1 + drivers/media/i2c/smiapp/smiapp-core.c | 4 +--- drivers/media/i2c/soc_camera/mt9m001.c | 1 - drivers/media/i2c/soc_camera/mt9t112.c | 6 -- drivers/media/i2c/soc_camera/mt9v022.c | 1 - drivers/media/i2c/soc_camera/ov5642.c | 1 - drivers/media/i2c/soc_camera/ov772x.c | 1 - drivers/media/i2c/soc_camera/ov9640.c | 1 - drivers/media/i2c/soc_camera/ov9740.c | 1 - drivers/media/i2c/soc_camera/rj54n1cb0c.c | 1 - drivers/media/i2c/tvp5150.c| 1 - drivers/media/platform/soc_camera/soc_scale_crop.c | 2 +- drivers/staging/media/imx074/imx074.c | 1 - drivers/staging/media/mt9t031/mt9t031.c| 1 - 26 files changed, 8 insertions(+), 33 deletions(-) -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v7] media: add imx319 camera sensor driver
On 09/26/2018 03:57 PM, Sakari Ailus wrote:
> Hi Bingbu,
>
> On Wed, Sep 26, 2018 at 10:42:18AM +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
>>
>> Cc: Tomasz Figa
>> Cc: Sakari Ailus
>> 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 v5:
>> - add some comments for gain calculation
>> - use lock to protect the format
>> - fix some style issues
> Thanks for the update!
>
> I've applied the patch with the following diff. Dividing a 64-bit number
> generally requires do_div() which was missed in the review:
Thanks!
>
> diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> index e10d60f500dd..37c31d17ecf0 100644
> --- a/drivers/media/i2c/imx319.c
> +++ b/drivers/media/i2c/imx319.c
> @@ -2038,7 +2038,7 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
> s32 vblank_def;
> s32 vblank_min;
> s64 h_blank;
> - s64 pixel_rate;
> + u64 pixel_rate;
> u32 height;
>
> mutex_lock(>mutex);
> @@ -2059,7 +2059,8 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
> *framefmt = fmt->format;
> } else {
> imx319->cur_mode = mode;
> - pixel_rate = (imx319->link_def_freq * 2 * 4) / 10;
> + pixel_rate = imx319->link_def_freq * 2 * 4;
> + do_div(pixel_rate, 10);
> __v4l2_ctrl_s_ctrl_int64(imx319->pixel_rate, pixel_rate);
> /* Update limits and set FPS to default */
> height = imx319->cur_mode->height;
> @@ -2268,7 +2269,7 @@ static int imx319_init_controls(struct imx319 *imx319)
> s64 vblank_def;
> s64 vblank_min;
> s64 hblank;
> - s64 pixel_rate;
> + u64 pixel_rate;
> const struct imx319_mode *mode;
> u32 max;
> int ret;
> @@ -2287,7 +2288,8 @@ static int imx319_init_controls(struct imx319 *imx319)
> 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;
> + pixel_rate = imx319->link_def_freq * 2 * 4;
> + do_div(pixel_rate, 10);
> /* By default, PIXEL_RATE is read only */
> imx319->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
> V4L2_CID_PIXEL_RATE, pixel_rate,
>
Re: [PATCH v7] media: add imx319 camera sensor driver
Hi Bingbu,
On Wed, Sep 26, 2018 at 10:42:18AM +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
>
> Cc: Tomasz Figa
> Cc: Sakari Ailus
> 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 v5:
> - add some comments for gain calculation
> - use lock to protect the format
> - fix some style issues
Thanks for the update!
I've applied the patch with the following diff. Dividing a 64-bit number
generally requires do_div() which was missed in the review:
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index e10d60f500dd..37c31d17ecf0 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -2038,7 +2038,7 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
s32 vblank_def;
s32 vblank_min;
s64 h_blank;
- s64 pixel_rate;
+ u64 pixel_rate;
u32 height;
mutex_lock(>mutex);
@@ -2059,7 +2059,8 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
*framefmt = fmt->format;
} else {
imx319->cur_mode = mode;
- pixel_rate = (imx319->link_def_freq * 2 * 4) / 10;
+ pixel_rate = imx319->link_def_freq * 2 * 4;
+ do_div(pixel_rate, 10);
__v4l2_ctrl_s_ctrl_int64(imx319->pixel_rate, pixel_rate);
/* Update limits and set FPS to default */
height = imx319->cur_mode->height;
@@ -2268,7 +2269,7 @@ static int imx319_init_controls(struct imx319 *imx319)
s64 vblank_def;
s64 vblank_min;
s64 hblank;
- s64 pixel_rate;
+ u64 pixel_rate;
const struct imx319_mode *mode;
u32 max;
int ret;
@@ -2287,7 +2288,8 @@ static int imx319_init_controls(struct imx319 *imx319)
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;
+ pixel_rate = imx319->link_def_freq * 2 * 4;
+ do_div(pixel_rate, 10);
/* By default, PIXEL_RATE is read only */
imx319->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
V4L2_CID_PIXEL_RATE, pixel_rate,
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH 1/1] v4l: Remove support for crop default target in subdev drivers
Hi Helmut, On Tue, Sep 25, 2018 at 08:33:29AM +0200, Helmut Grohne wrote: > On Mon, Sep 24, 2018 at 04:42:27PM +0200, Sakari Ailus wrote: > > --- a/drivers/media/i2c/mt9t112.c > > +++ b/drivers/media/i2c/mt9t112.c > > @@ -888,12 +888,6 @@ static int mt9t112_get_selection(struct v4l2_subdev > > *sd, > > sel->r.width = MAX_WIDTH; > > sel->r.height = MAX_HEIGHT; > > return 0; > > - case V4L2_SEL_TGT_CROP_DEFAULT: > > - sel->r.left = 0; > > - sel->r.top = 0; > > - sel->r.width = VGA_WIDTH; > > - sel->r.height = VGA_HEIGHT; > > - return 0; > > case V4L2_SEL_TGT_CROP: > > sel->r = priv->frame; > > return 0; > > Together with the change in soc_scale_crop.c, this constitutes an > (unintentional?) behaviour change. It was formerly reporting 640x480 and > will now be reporting 2048x1536. I cannot tell whether that is > reasonable. I'd say "yes". This is the only sub-device driver that puts a default other than the bounds there. Its source is SoC camera as you see below. > > > --- a/drivers/media/i2c/soc_camera/mt9t112.c > > +++ b/drivers/media/i2c/soc_camera/mt9t112.c > > @@ -884,12 +884,6 @@ static int mt9t112_get_selection(struct v4l2_subdev > > *sd, > > sel->r.width = MAX_WIDTH; > > sel->r.height = MAX_HEIGHT; > > return 0; > > - case V4L2_SEL_TGT_CROP_DEFAULT: > > - sel->r.left = 0; > > - sel->r.top = 0; > > - sel->r.width = VGA_WIDTH; > > - sel->r.height = VGA_HEIGHT; > > - return 0; > > case V4L2_SEL_TGT_CROP: > > sel->r = priv->frame; > > return 0; > > This one looks duplicate. Is there a good reason to have two drivers for > mt9t112? This is lilely out of scope for the patch. Cced Jacopo Mondi as > he introduced the copy. > > Other than your patch looks fine to me. > > Helmut -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH] media: smiapp: Remove unused loop
Hi Ricardo,
On Wed, Sep 26, 2018 at 08:12:42AM +0200, Ricardo Ribalda Delgado wrote:
> The loop seemed to be made to calculate max, but max is not used in that
> function.
>
> Signed-off-by: Ricardo Ribalda Delgado
The code has been left there probably when the valid link frequency
calculation was changed.
Thanks!
> ---
> drivers/media/i2c/smiapp/smiapp-core.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> b/drivers/media/i2c/smiapp/smiapp-core.c
> index 99f3b295ae3c..bccbf4c841d6 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -624,7 +624,7 @@ static int smiapp_init_late_controls(struct smiapp_sensor
> *sensor)
> {
> unsigned long *valid_link_freqs = >valid_link_freqs[
> sensor->csi_format->compressed - sensor->compressed_min_bpp];
> - unsigned int max, i;
> + unsigned int i;
>
> for (i = 0; i < ARRAY_SIZE(sensor->test_data); i++) {
> int max_value = (1 << sensor->csi_format->width) - 1;
> @@ -635,8 +635,6 @@ static int smiapp_init_late_controls(struct smiapp_sensor
> *sensor)
> 0, max_value, 1, max_value);
> }
>
> - for (max = 0; sensor->hwcfg->op_sys_clock[max + 1]; max++);
> -
> sensor->link_freq = v4l2_ctrl_new_int_menu(
> >src->ctrl_handler, _ctrl_ops,
> V4L2_CID_LINK_FREQ, __fls(*valid_link_freqs),
> --
> 2.19.0
>
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
[PATCH] media: smiapp: Remove unused loop
The loop seemed to be made to calculate max, but max is not used in that
function.
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/i2c/smiapp/smiapp-core.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
b/drivers/media/i2c/smiapp/smiapp-core.c
index 99f3b295ae3c..bccbf4c841d6 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -624,7 +624,7 @@ static int smiapp_init_late_controls(struct smiapp_sensor
*sensor)
{
unsigned long *valid_link_freqs = >valid_link_freqs[
sensor->csi_format->compressed - sensor->compressed_min_bpp];
- unsigned int max, i;
+ unsigned int i;
for (i = 0; i < ARRAY_SIZE(sensor->test_data); i++) {
int max_value = (1 << sensor->csi_format->width) - 1;
@@ -635,8 +635,6 @@ static int smiapp_init_late_controls(struct smiapp_sensor
*sensor)
0, max_value, 1, max_value);
}
- for (max = 0; sensor->hwcfg->op_sys_clock[max + 1]; max++);
-
sensor->link_freq = v4l2_ctrl_new_int_menu(
>src->ctrl_handler, _ctrl_ops,
V4L2_CID_LINK_FREQ, __fls(*valid_link_freqs),
--
2.19.0
