[ragnatech:media-tree] BUILD INCOMPLETE 72c27a68a2a3f650f0dc7891ee98f02283fc11af
tree/branch: git://git.ragnatech.se/linux media-tree branch HEAD: 72c27a68a2a3f650f0dc7891ee98f02283fc11af media: pvrusb2: properly check endpoint types TIMEOUT after 1192m Sorry we cannot finish the testset for your branch within a reasonable time. It's our fault -- either some build server is down or some build worker is busy doing bisects for _other_ trees. The branch will get more complete coverage and possible error reports when our build infrastructure is restored or catches up. There will be no more build success notification for this branch head, but you can expect reasonably good test coverage after waiting for 1 day. configs timed out: 164 alphaallmodconfig alphaallyesconfig alpha defconfig arm allnoconfig arm at91_dt_defconfig armcustomconfig-0 armcustomconfig-1 armcustomconfig-2 arm efm32_defconfig arm exynos_defconfig armmulti_v5_defconfig armmulti_v7_defconfig armshmobile_defconfig arm sunxi_defconfig arm64 allnoconfig arm64 customconfig-0 arm64 customconfig-1 arm64 customconfig-2 arm64 defconfig blackfinBF526-EZBRD_defconfig blackfinBF533-EZKIT_defconfig blackfinBF561-EZKIT-SMP_defconfig blackfin TCM-BF537_defconfig blackfin allmodconfig blackfin allyesconfig c6xevmc6678_defconfig cris allmodconfig cris allyesconfig cris etrax-100lx_v2_defconfig frv defconfig h8300h8300h-sim_defconfig i386 alldefconfig i386 allmodconfig i386 allnoconfig i386 customconfig-0 i386 customconfig-1 i386 customconfig-2 i386defconfig i386randconfig-a0 i386randconfig-a1 i386randconfig-i0 i386randconfig-i1 i386randconfig-n0 i386randconfig-s0 i386randconfig-s1 i386 randconfig-x000 i386 randconfig-x001 i386 randconfig-x002 i386 randconfig-x003 i386 randconfig-x004 i386 randconfig-x005 i386 randconfig-x006 i386 randconfig-x007 i386 randconfig-x008 i386 randconfig-x009 i386 randconfig-x010 i386 randconfig-x011 i386 randconfig-x012 i386 randconfig-x013 i386 randconfig-x014 i386 randconfig-x015 i386 randconfig-x016 i386 randconfig-x017 i386 randconfig-x018 i386 randconfig-x019 i386 tinyconfig ia64 alldefconfig ia64 allmodconfig ia64 allnoconfig ia64 allyesconfig ia64defconfig m32r m32104ut_defconfig m32r mappi3.smp_defconfig m32r opsput_defconfig m32r usrv_defconfig m68k allmodconfig m68k allyesconfig m68k m5475evb_defconfig m68k multi_defconfig m68k sun3_defconfig microblaze mmu_defconfig microblazenommu_defconfig mips 32r2_defconfig mips 64r6el_defconfig mips allmodconfig mips allnoconfig mips allyesconfig mips fuloong2e_defconfig mips jz4740 mips malta_kvm_defconfig mips txx9 mn10300 asb2364_defconfig nios2 10m50_defconfig openriscor1ksim_defconfig parisc
cron job: media_tree daily build: ERRORS
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: Wed Dec 13 05:00:18 CET 2017 media-tree git hash:330dada5957e3ca0c8811b14c45e3ac42c694651 media_build git hash: f5a5e5e470d834f9843fee7a7c2ce3e4be610ca7 v4l-utils git hash: 655a8e3e412fd30b5ec201b73ec2b923c76e34bb gcc version:i686-linux-gcc (GCC) 7.1.0 sparse version: v0.5.0-3911-g6f737e1f smatch version: v0.5.0-3911-g6f737e1f host hardware: x86_64 host os:4.13.0-164 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-blackfin-bf561: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK linux-2.6.36.4-i686: ERRORS linux-2.6.37.6-i686: ERRORS linux-2.6.38.8-i686: ERRORS linux-2.6.39.4-i686: ERRORS linux-3.0.60-i686: ERRORS linux-3.1.10-i686: ERRORS linux-3.2.37-i686: ERRORS linux-3.3.8-i686: ERRORS linux-3.4.27-i686: ERRORS linux-3.5.7-i686: ERRORS linux-3.6.11-i686: ERRORS linux-3.7.4-i686: ERRORS linux-3.8-i686: ERRORS linux-3.9.2-i686: ERRORS linux-3.10.1-i686: ERRORS linux-3.11.1-i686: ERRORS linux-3.12.67-i686: ERRORS linux-3.13.11-i686: ERRORS linux-3.14.9-i686: ERRORS linux-3.15.2-i686: ERRORS linux-3.16.7-i686: ERRORS linux-3.17.8-i686: ERRORS linux-3.18.7-i686: ERRORS linux-3.19-i686: ERRORS linux-4.0.9-i686: ERRORS linux-4.1.33-i686: ERRORS linux-4.2.8-i686: ERRORS linux-4.3.6-i686: ERRORS linux-4.4.22-i686: ERRORS linux-4.5.7-i686: ERRORS linux-4.6.7-i686: ERRORS linux-4.7.5-i686: ERRORS linux-4.8-i686: ERRORS linux-4.9.26-i686: ERRORS linux-4.10.14-i686: ERRORS linux-4.11-i686: ERRORS linux-4.12.1-i686: ERRORS linux-4.13-i686: ERRORS linux-4.14-i686: ERRORS linux-2.6.36.4-x86_64: ERRORS linux-2.6.37.6-x86_64: ERRORS linux-2.6.38.8-x86_64: ERRORS linux-2.6.39.4-x86_64: ERRORS linux-3.0.60-x86_64: ERRORS linux-3.1.10-x86_64: ERRORS linux-3.2.37-x86_64: ERRORS linux-3.3.8-x86_64: ERRORS linux-3.4.27-x86_64: ERRORS linux-3.5.7-x86_64: ERRORS linux-3.6.11-x86_64: ERRORS linux-3.7.4-x86_64: ERRORS linux-3.8-x86_64: ERRORS linux-3.9.2-x86_64: ERRORS linux-3.10.1-x86_64: ERRORS linux-3.11.1-x86_64: ERRORS linux-3.12.67-x86_64: ERRORS linux-3.13.11-x86_64: ERRORS linux-3.14.9-x86_64: ERRORS linux-3.15.2-x86_64: ERRORS linux-3.16.7-x86_64: ERRORS linux-3.17.8-x86_64: ERRORS linux-3.18.7-x86_64: ERRORS linux-3.19-x86_64: ERRORS linux-4.0.9-x86_64: ERRORS linux-4.1.33-x86_64: ERRORS linux-4.2.8-x86_64: ERRORS linux-4.3.6-x86_64: ERRORS linux-4.4.22-x86_64: ERRORS linux-4.5.7-x86_64: ERRORS linux-4.6.7-x86_64: ERRORS linux-4.7.5-x86_64: ERRORS linux-4.8-x86_64: ERRORS linux-4.9.26-x86_64: ERRORS linux-4.10.14-x86_64: ERRORS linux-4.11-x86_64: ERRORS linux-4.12.1-x86_64: ERRORS linux-4.13-x86_64: ERRORS linux-4.14-x86_64: ERRORS apps: OK spec-git: OK smatch: OK Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Wednesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Wednesday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
Re: [PATCH] au0828: fix use-after-free at USB probing
Hey Andrey, Quoting Andrey Konovalov : On Thu, Nov 23, 2017 at 2:31 AM, Gustavo A. R. Silva wrote: Hi Andrey, I have successfully installed and tested syzkaller with QEMU. Can you please tell me how to reproduce this bug or share with me the full crash report? Also, can you point me out to the PoC file? Hi Gustavo, Sorry for the delay. No worries. I've now published the USB fuzzing prototype, so here's how you can reproduce this: 1. Get Linux 4.15-rc3 upstream kernel (50c4c4e268a2d7a3e58ebb698ac74da0de40ae36). 2. Apply this patch (it adds a new interface to emulate USB devices): https://github.com/google/syzkaller/blob/usb-fuzzer/tools/usb/0002-usb-fuzzer-main-usb-gadget-fuzzer-driver.patch 3. Build the kernel with the attached .config (you need relatively new GCC to make KASAN work). 4. Run the attached reproducer. Also attaching the full kernel log. Awesome. :D I'll try this. Thank you! -- Gustavo A. R. Silva
[PATCH 1/3] [media] staging/cxd2099: fix remaining checkpatch-strict issues
From: Daniel Scheller
Fix up all remaining cosmetic issues as reported by checkpatch.pl.
Signed-off-by: Daniel Scheller
Signed-off-by: Jasmin Jessich
---
drivers/staging/media/cxd2099/cxd2099.c | 19 +--
drivers/staging/media/cxd2099/cxd2099.h | 14 +++---
2 files changed, 8 insertions(+), 25 deletions(-)
diff --git a/drivers/staging/media/cxd2099/cxd2099.c
b/drivers/staging/media/cxd2099/cxd2099.c
index 3e30f4864e2b..21b1c6fcf9bf 100644
--- a/drivers/staging/media/cxd2099/cxd2099.c
+++ b/drivers/staging/media/cxd2099/cxd2099.c
@@ -3,23 +3,14 @@
*
* Copyright (C) 2010-2013 Digital Devices GmbH
*
- *
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* version 2 only, as published by the Free Software Foundation.
*
- *
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
- *
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
- * 02110-1301, USA
- * Or, point your browser to http://www.gnu.org/copyleft/gpl.html
*/
#include
@@ -59,7 +50,7 @@ struct cxd {
intamem_read;
intcammode;
- struct mutex lock;
+ struct mutex lock; /* device access lock */
u8 rbuf[1028];
u8 wbuf[1028];
@@ -101,7 +92,7 @@ static int i2c_read_reg(struct i2c_adapter *adapter, u8 adr,
.buf = val, .len = 1} };
if (i2c_transfer(adapter, msgs, 2) != 2) {
- dev_err(&adapter->dev, "error in i2c_read_reg\n");
+ dev_err(&adapter->dev, "error in %s()\n", __func__);
return -1;
}
return 0;
@@ -116,7 +107,7 @@ static int i2c_read(struct i2c_adapter *adapter, u8 adr,
.buf = data, .len = n} };
if (i2c_transfer(adapter, msgs, 2) != 2) {
- dev_err(&adapter->dev, "error in i2c_read\n");
+ dev_err(&adapter->dev, "error in %s()\n", __func__);
return -1;
}
return 0;
@@ -134,7 +125,7 @@ static int read_block(struct cxd *ci, u8 adr, u8 *data, u16
n)
while (n) {
int len = n;
- if (ci->cfg.max_i2c && (len > ci->cfg.max_i2c))
+ if (ci->cfg.max_i2c && len > ci->cfg.max_i2c)
len = ci->cfg.max_i2c;
status = i2c_read(ci->i2c, ci->cfg.adr, 1, data, len);
if (status)
@@ -591,7 +582,7 @@ static int campoll(struct cxd *ci)
}
}
if ((istat & 8) &&
- (ci->slot_stat == DVB_CA_EN50221_POLL_CAM_PRESENT)) {
+ ci->slot_stat == DVB_CA_EN50221_POLL_CAM_PRESENT) {
ci->ready = 1;
ci->slot_stat |= DVB_CA_EN50221_POLL_CAM_READY;
}
diff --git a/drivers/staging/media/cxd2099/cxd2099.h
b/drivers/staging/media/cxd2099/cxd2099.h
index f4b29b1d6eb8..aba803268e94 100644
--- a/drivers/staging/media/cxd2099/cxd2099.h
+++ b/drivers/staging/media/cxd2099/cxd2099.h
@@ -3,23 +3,14 @@
*
* Copyright (C) 2010-2011 Digital Devices GmbH
*
- *
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* version 2 only, as published by the Free Software Foundation.
*
- *
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
- *
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
- * 02110-1301, USA
- * Or, point your browser to http://www.gnu.org/copyleft/gpl.html
*/
#ifndef _CXD2099_H_
@@ -42,8 +33,9 @@ struct dvb_ca_en50221 *cxd2099_attach(struct cxd2099_cfg *cfg,
void *priv, struct i2c_adapter *i2c);
#else
-static inline struct dvb_ca_en50221 *cxd2099_attach(struct cxd2099_cfg *cfg,
- void *priv, struct i2c_adapter *i2c)
+static inline struct
+dvb_ca_en50221 *cxd2099_attach(struct cxd2099_cfg *cfg, void *priv,
+ struct i2c_adapter *i2c)
{
dev_warn(&i2c->dev, "%s: driver disabled by Kconfig\n", __func__);
return NULL;
--
2.13.6
[PATCH 2/3] [media] staging/cxd2099: fix debug message severity
From: Daniel Scheller
Debug messages should go to KERN_DEBUG, thus change the slot_shutdown()
notice from dev_info() to dev_dbg().
Signed-off-by: Daniel Scheller
Signed-off-by: Jasmin Jessich
---
drivers/staging/media/cxd2099/cxd2099.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/cxd2099/cxd2099.c
b/drivers/staging/media/cxd2099/cxd2099.c
index 21b1c6fcf9bf..38d43647d4bf 100644
--- a/drivers/staging/media/cxd2099/cxd2099.c
+++ b/drivers/staging/media/cxd2099/cxd2099.c
@@ -518,7 +518,7 @@ static int slot_shutdown(struct dvb_ca_en50221 *ca, int
slot)
{
struct cxd *ci = ca->data;
- dev_info(&ci->i2c->dev, "%s\n", __func__);
+ dev_dbg(&ci->i2c->dev, "%s\n", __func__);
if (ci->cammode)
read_data(ca, slot, ci->rbuf, 0);
mutex_lock(&ci->lock);
--
2.13.6
[PATCH 0/3] staging/cxd2099: cosmetics, checkpatch fixup
From: Daniel Scheller These three small patches make the driver checkpatch-strict clean and improves a few strings. No functional changes. Essentially drivers/staging/media/cxd2099/ is now clean, esp.: $ scripts/checkpatch.pl --strict --file drivers/staging/media/cxd2099/cxd2099.c total: 0 errors, 0 warnings, 0 checks, 705 lines checked $ scripts/checkpatch.pl --strict --file drivers/staging/media/cxd2099/cxd2099.h total: 0 errors, 0 warnings, 0 checks, 45 lines checked The three patches are the outcome of some bigger refactoring WIP. Daniel Scheller (3): [media] staging/cxd2099: fix remaining checkpatch-strict issues [media] staging/cxd2099: fix debug message severity [media] staging/cxd2099: cosmetics: improve strings drivers/staging/media/cxd2099/cxd2099.c | 30 +++--- drivers/staging/media/cxd2099/cxd2099.h | 14 +++--- 2 files changed, 14 insertions(+), 30 deletions(-) -- 2.13.6
[PATCH 3/3] [media] staging/cxd2099: cosmetics: improve strings
From: Daniel Scheller
Prefix dev_*() I2C address prints with 0x, change CXD2099 to CXD2099AR,
change the MODULE_DESCRIPTION to a proper one and have a better (and
shorter) description for the buffermode modparam.
Signed-off-by: Daniel Scheller
Signed-off-by: Jasmin Jessich
---
drivers/staging/media/cxd2099/cxd2099.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/cxd2099/cxd2099.c
b/drivers/staging/media/cxd2099/cxd2099.c
index 38d43647d4bf..227a329f0c6e 100644
--- a/drivers/staging/media/cxd2099/cxd2099.c
+++ b/drivers/staging/media/cxd2099/cxd2099.c
@@ -26,7 +26,7 @@
static int buffermode;
module_param(buffermode, int, 0444);
-MODULE_PARM_DESC(buffermode, "Enable use of the CXD2099AR buffer mode
(default: disabled)");
+MODULE_PARM_DESC(buffermode, "Enable CXD2099AR buffer mode (default:
disabled)");
static int read_data(struct dvb_ca_en50221 *ca, int slot, u8 *ebuf, int
ecount);
@@ -668,7 +668,8 @@ struct dvb_ca_en50221 *cxd2099_attach(struct cxd2099_cfg
*cfg,
u8 val;
if (i2c_read_reg(i2c, cfg->adr, 0, &val) < 0) {
- dev_info(&i2c->dev, "No CXD2099 detected at %02x\n", cfg->adr);
+ dev_info(&i2c->dev, "No CXD2099AR detected at 0x%02x\n",
+cfg->adr);
return NULL;
}
@@ -686,7 +687,7 @@ struct dvb_ca_en50221 *cxd2099_attach(struct cxd2099_cfg
*cfg,
ci->en = en_templ;
ci->en.data = ci;
init(ci);
- dev_info(&i2c->dev, "Attached CXD2099AR at %02x\n", ci->cfg.adr);
+ dev_info(&i2c->dev, "Attached CXD2099AR at 0x%02x\n", ci->cfg.adr);
if (!buffermode) {
ci->en.read_data = NULL;
@@ -699,6 +700,6 @@ struct dvb_ca_en50221 *cxd2099_attach(struct cxd2099_cfg
*cfg,
}
EXPORT_SYMBOL(cxd2099_attach);
-MODULE_DESCRIPTION("cxd2099");
+MODULE_DESCRIPTION("CXD2099AR Common Interface controller driver");
MODULE_AUTHOR("Ralph Metzler");
MODULE_LICENSE("GPL");
--
2.13.6
[PATCH] [build] fixup v3.13_ddbridge_pcimsi.patch
From: Jasmin Jessich
Required after the ddbridge 0.9.32 bump in media_tree.
Signed-off-by: Jasmin Jessich
Signed-off-by: Daniel Scheller
---
Fixes at least the patch issue with kernel <=3.13. Jasmin originally
prepared the updated patch when the ddbridge-0.9.32 bump was done, so
sending it in behalf of her (with CONFIG_VIDEO_PVRUSB2 disabled this
makes the patch phase work again with older kernels).
Hans, this is probably for you. I don't have a fix for the PVRUSB2
usb_urb_ep_type_check() issue at hands though.
backports/v3.13_ddbridge_pcimsi.patch | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/backports/v3.13_ddbridge_pcimsi.patch
b/backports/v3.13_ddbridge_pcimsi.patch
index 5f602a7..f410251 100644
--- a/backports/v3.13_ddbridge_pcimsi.patch
+++ b/backports/v3.13_ddbridge_pcimsi.patch
@@ -2,7 +2,7 @@ diff --git a/drivers/media/pci/ddbridge/ddbridge-main.c
b/drivers/media/pci/ddbr
index 9ab4736..50c3b4f 100644
--- a/drivers/media/pci/ddbridge/ddbridge-main.c
+++ b/drivers/media/pci/ddbridge/ddbridge-main.c
-@@ -129,13 +129,18 @@ static void ddb_irq_msi(struct ddb *dev, int nr)
+@@ -129,14 +129,18 @@ static void ddb_irq_msi(struct ddb *dev, int nr)
int stat;
if (msi && pci_msi_enabled()) {
@@ -10,17 +10,18 @@ index 9ab4736..50c3b4f 100644
- if (stat >= 1) {
- dev->msi = stat;
- dev_info(dev->dev, "using %d MSI interrupt(s)\n",
-- dev->msi);
-- } else
+- dev->msi);
+- } else {
+- dev_info(dev->dev, "MSI not available.\n");
+ stat = pci_enable_msi_block(dev->pdev, nr);
+ if (stat == 0) {
+ dev->msi = nr;
+ } else if (stat == 1) {
+ stat = pci_enable_msi(dev->pdev);
+ dev->msi = 1;
-+ }
+ }
+ if (stat < 0)
- dev_info(dev->dev, "MSI not available.\n");
++ dev_info(dev->dev, "MSI not available.\n");
+ else
+ dev_info(dev->dev, "using %d MSI interrupts\n",
+ dev->msi);
--
2.13.6
Re: [PATCH] tuners: tda8290: reduce stack usage with kasan
On Tue, 2017-12-12 at 15:21 +0100, Arnd Bergmann wrote:
> On Tue, Dec 12, 2017 at 1:45 PM, Mauro Carvalho Chehab
> wrote:
> > Em Tue, 12 Dec 2017 03:42:32 -0800
> > Joe Perches escreveu:
> >
> > > > I actually thought about marking them 'const' here before sending
> > > > (without noticing the changelog text) and then ran into what must
> > > > have led me to drop the 'const' originally: tuner_i2c_xfer_send()
> > > > takes a non-const pointer. This can be fixed but it requires
> > > > an ugly cast:
> > >
> > > Casting away const is always a horrible hack.
> > >
> > > Until it could be changed, my preference would
> > > be to update the changelog and perhaps add to
> > > the changelog the reason why it can not be const
> > > as detailed below.
> > >
> > > ie: xfer_send and xfer_xend_recv both take a
> > > non-const unsigned char *
>
> Ok.
>
> > Perhaps, on a separate changeset, we could change I2C routines to
> > accept const unsigned char pointers. This is unrelated to tda8290
> > KASAN fixes. So, it should go via I2C tree, and, once accepted
> > there, we can change V4L2 drivers (and other drivers) accordingly.
>
> I don't see how that would work unfortunately. i2c_msg contains
> a pointer to the data, and that is used for both input and output,
> including arrays like
>
> struct i2c_msg msgs[] = {
> {
> .addr = dvo->slave_addr,
> .flags = 0,
> .len = 1,
> .buf = &addr,
> },
> {
> .addr = dvo->slave_addr,
> .flags = I2C_M_RD,
> .len = 1,
> .buf = val,
> }
> };
>
> that have one constant output pointer and one non-constant
> input pointer. We could add an anonymous union for 'buf'
> to make that two separate pointers, but that's barely any
> better than the cast, and it would break the named initializers
> in the example above, at least on older compilers. Adding
> a second pointer to i2c_msg would add a bit of bloat and
> also require tree-wide changes or ugly hacks.
Perhaps add something like
struct i2c_msg_set {
__u16 addr; /* slave address*/
__u16 flags;
__u16 len; /* msg length */
const __u8 *buf;/* pointer to read-only msg data*/
};
struct i2c_msg_get {
__u16 addr; /* slave address*/
__u16 flags;
__u16 len; /* msg length */
__u8 *buf; /* pointer to writeable msg data*/
};
to the uapi include and use that where appropriate
but where a write then read is done via a single
i2c_msg array, it's not really feasible either.
Probably better to avoid any churn and just mark
all these as static rather than static const.
Re: [PATCH v10 4/4] [media] platform: Add Synopsys DesignWare HDMI RX Controller Driver
Hi Hans, On 12-12-2017 15:47, Hans Verkuil wrote: > Hi Jose, > > Some more comments: Thanks for the review! > > On 11/12/17 18:41, Jose Abreu wrote: >> This is an initial submission for the Synopsys DesignWare HDMI RX >> Controller Driver. This driver interacts with a phy driver so that >> a communication between them is created and a video pipeline is >> configured. >> >> The controller + phy pipeline can then be integrated into a fully >> featured system that can be able to receive video up to 4k@60Hz >> with deep color 48bit RGB, depending on the platform. Although, >> this initial version does not yet handle deep color modes. >> >> This driver was implemented as a standard V4L2 subdevice and its >> main features are: >> - Internal state machine that reconfigures phy until the >> video is not stable >> - JTAG communication with phy >> - Inter-module communication with phy driver >> - Debug write/read ioctls >> >> Some notes: >> - RX sense controller (cable connection/disconnection) must >> be handled by the platform wrapper as this is not integrated >> into the controller RTL >> - The same goes for EDID ROM's >> - ZCAL calibration is needed only in FPGA platforms, in ASIC >> this is not needed >> - The state machine is not an ideal solution as it creates a >> kthread but it is needed because some sources might not be >> very stable at sending the video (i.e. we must react >> accordingly). >> >> Signed-off-by: Jose Abreu >> Cc: Joao Pinto >> Cc: Mauro Carvalho Chehab >> Cc: Hans Verkuil >> Cc: Sylwester Nawrocki >> Cc: Sakari Ailus >> Cc: Philippe Ombredanne >> --- >> Changes from v9: >> - Use SPDX License ID (Philippe) >> - Use LOW_DRIVE CEC error (Hans) >> - Fill bt->il_* fields (Hans) >> - Fix format->field to NONE (Hans) >> - Drop a left-over comment (Hans) >> - Use CEC_CAP_DEFAULTS (Hans) >> Changes from v8: >> - Incorporate Sakari's work on ASYNC subdevs >> Changes from v6: >> - edid-phandle now also looks for parent node (Sylwester) >> - Fix kbuild build warnings >> Changes from v5: >> - Removed HDCP 1.4 support (Hans) >> - Removed some CEC debug messages (Hans) >> - Add s_dv_timings callback (Hans) >> - Add V4L2_CID_DV_RX_POWER_PRESENT ctrl (Hans) >> Changes from v4: >> - Add flag V4L2_SUBDEV_FL_HAS_DEVNODE (Sylwester) >> - Remove some comments and change some messages to dev_dbg (Sylwester) >> - Use v4l2_async_subnotifier_register() (Sylwester) >> Changes from v3: >> - Use v4l2 async API (Sylwester) >> - Do not block waiting for phy >> - Do not use busy waiting delays (Sylwester) >> - Simplify dw_hdmi_power_on (Sylwester) >> - Use clock API (Sylwester) >> - Use compatible string (Sylwester) >> - Minor fixes (Sylwester) >> Changes from v2: >> - Address review comments from Hans regarding CEC >> - Use CEC notifier >> - Enable SCDC >> Changes from v1: >> - Add support for CEC >> - Correct typo errors >> - Correctly detect interlaced video modes >> - Correct VIC parsing >> Changes from RFC: >> - Add support for HDCP 1.4 >> - Fixup HDMI_VIC not being parsed (Hans) >> - Send source change signal when powering off (Hans) >> - Add a "wait stable delay" >> - Detect interlaced video modes (Hans) >> - Restrain g/s_register from reading/writing to HDCP regs (Hans) >> --- >> drivers/media/platform/dwc/Kconfig | 15 + >> drivers/media/platform/dwc/Makefile |1 + >> drivers/media/platform/dwc/dw-hdmi-rx.c | 1840 >> +++ >> drivers/media/platform/dwc/dw-hdmi-rx.h | 419 +++ >> include/media/dwc/dw-hdmi-rx-pdata.h| 48 + >> 5 files changed, 2323 insertions(+) >> create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.c >> create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.h >> create mode 100644 include/media/dwc/dw-hdmi-rx-pdata.h >> >> diff --git a/drivers/media/platform/dwc/Kconfig >> b/drivers/media/platform/dwc/Kconfig >> index 361d38d..3ddccde 100644 >> --- a/drivers/media/platform/dwc/Kconfig >> +++ b/drivers/media/platform/dwc/Kconfig >> @@ -6,3 +6,18 @@ config VIDEO_DWC_HDMI_PHY_E405 >> >>To compile this driver as a module, choose M here. The module >>will be called dw-hdmi-phy-e405. >> + >> +config VIDEO_DWC_HDMI_RX >> +tristate "Synopsys Designware HDMI Receiver driver" >> +depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API >> +help >> + Support for Synopsys Designware HDMI RX controller. >> + >> + To compile this driver as a module, choose M here. The module >> + will be called dw-hdmi-rx. >> + >> +config VIDEO_DWC_HDMI_RX_CEC >> +bool >> +depends on VIDEO_DWC_HDMI_RX >> +select CEC_CORE >> +select CEC_NOTIFIER >> diff --git a/drivers/media/platform/dwc/Makefile >> b/drivers/media/platform/dwc/Makefile >> index fc3b62
Re: [PATCH 1/3] atomisp: Fix up the open v load race
On Tue, 12 Dec 2017 09:03:50 -0200 Mauro Carvalho Chehab wrote: > Em Mon, 06 Nov 2017 23:36:36 + > Alan escreveu: > > > This isn't the ideal final solution but it stops the main problem for now > > where an open (often from udev) races the device initialization and we try > > and load the firmware twice at the same time. This needless to say doesn't > > usually end well. > > What we do on most drivers is that video_register_device() is called > only after all hardware init. > > That's usually enough to avoid race conditions with udev, although > a mutex is also common in order to avoid some other race conditions > between open/close - with can happen with multiple opens. There are a whole bunch of other ordering issues to deal with in the atomisp case beyond this - another one is that the camera probe can race the ISP probe. Quite a lot of the registration code needs fixing, but I'm prioritizing stabilizing the code first, and trying to get the Cherrytrail version going. Alan
Re: [PATCH v10 4/4] [media] platform: Add Synopsys DesignWare HDMI RX Controller Driver
Hi Jose, Some more comments: On 11/12/17 18:41, Jose Abreu wrote: > This is an initial submission for the Synopsys DesignWare HDMI RX > Controller Driver. This driver interacts with a phy driver so that > a communication between them is created and a video pipeline is > configured. > > The controller + phy pipeline can then be integrated into a fully > featured system that can be able to receive video up to 4k@60Hz > with deep color 48bit RGB, depending on the platform. Although, > this initial version does not yet handle deep color modes. > > This driver was implemented as a standard V4L2 subdevice and its > main features are: > - Internal state machine that reconfigures phy until the > video is not stable > - JTAG communication with phy > - Inter-module communication with phy driver > - Debug write/read ioctls > > Some notes: > - RX sense controller (cable connection/disconnection) must > be handled by the platform wrapper as this is not integrated > into the controller RTL > - The same goes for EDID ROM's > - ZCAL calibration is needed only in FPGA platforms, in ASIC > this is not needed > - The state machine is not an ideal solution as it creates a > kthread but it is needed because some sources might not be > very stable at sending the video (i.e. we must react > accordingly). > > Signed-off-by: Jose Abreu > Cc: Joao Pinto > Cc: Mauro Carvalho Chehab > Cc: Hans Verkuil > Cc: Sylwester Nawrocki > Cc: Sakari Ailus > Cc: Philippe Ombredanne > --- > Changes from v9: > - Use SPDX License ID (Philippe) > - Use LOW_DRIVE CEC error (Hans) > - Fill bt->il_* fields (Hans) > - Fix format->field to NONE (Hans) > - Drop a left-over comment (Hans) > - Use CEC_CAP_DEFAULTS (Hans) > Changes from v8: > - Incorporate Sakari's work on ASYNC subdevs > Changes from v6: > - edid-phandle now also looks for parent node (Sylwester) > - Fix kbuild build warnings > Changes from v5: > - Removed HDCP 1.4 support (Hans) > - Removed some CEC debug messages (Hans) > - Add s_dv_timings callback (Hans) > - Add V4L2_CID_DV_RX_POWER_PRESENT ctrl (Hans) > Changes from v4: > - Add flag V4L2_SUBDEV_FL_HAS_DEVNODE (Sylwester) > - Remove some comments and change some messages to dev_dbg (Sylwester) > - Use v4l2_async_subnotifier_register() (Sylwester) > Changes from v3: > - Use v4l2 async API (Sylwester) > - Do not block waiting for phy > - Do not use busy waiting delays (Sylwester) > - Simplify dw_hdmi_power_on (Sylwester) > - Use clock API (Sylwester) > - Use compatible string (Sylwester) > - Minor fixes (Sylwester) > Changes from v2: > - Address review comments from Hans regarding CEC > - Use CEC notifier > - Enable SCDC > Changes from v1: > - Add support for CEC > - Correct typo errors > - Correctly detect interlaced video modes > - Correct VIC parsing > Changes from RFC: > - Add support for HDCP 1.4 > - Fixup HDMI_VIC not being parsed (Hans) > - Send source change signal when powering off (Hans) > - Add a "wait stable delay" > - Detect interlaced video modes (Hans) > - Restrain g/s_register from reading/writing to HDCP regs (Hans) > --- > drivers/media/platform/dwc/Kconfig | 15 + > drivers/media/platform/dwc/Makefile |1 + > drivers/media/platform/dwc/dw-hdmi-rx.c | 1840 > +++ > drivers/media/platform/dwc/dw-hdmi-rx.h | 419 +++ > include/media/dwc/dw-hdmi-rx-pdata.h| 48 + > 5 files changed, 2323 insertions(+) > create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.c > create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.h > create mode 100644 include/media/dwc/dw-hdmi-rx-pdata.h > > diff --git a/drivers/media/platform/dwc/Kconfig > b/drivers/media/platform/dwc/Kconfig > index 361d38d..3ddccde 100644 > --- a/drivers/media/platform/dwc/Kconfig > +++ b/drivers/media/platform/dwc/Kconfig > @@ -6,3 +6,18 @@ config VIDEO_DWC_HDMI_PHY_E405 > > To compile this driver as a module, choose M here. The module > will be called dw-hdmi-phy-e405. > + > +config VIDEO_DWC_HDMI_RX > + tristate "Synopsys Designware HDMI Receiver driver" > + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API > + help > + Support for Synopsys Designware HDMI RX controller. > + > + To compile this driver as a module, choose M here. The module > + will be called dw-hdmi-rx. > + > +config VIDEO_DWC_HDMI_RX_CEC > + bool > + depends on VIDEO_DWC_HDMI_RX > + select CEC_CORE > + select CEC_NOTIFIER > diff --git a/drivers/media/platform/dwc/Makefile > b/drivers/media/platform/dwc/Makefile > index fc3b62c..cd04ca9 100644 > --- a/drivers/media/platform/dwc/Makefile > +++ b/drivers/media/platform/dwc/Makefile > @@ -1 +1,2 @@ > obj-$(CONFIG_
Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race
Hi Mauro, On Tuesday, 12 December 2017 14:39:32 EET Mauro Carvalho Chehab wrote: > Em Thu, 23 Nov 2017 15:21:01 +0100 Greg Kroah-Hartman escreveu: > > On Thu, Nov 23, 2017 at 11:07:51AM -0200, Mauro Carvalho Chehab wrote: > >> Em Thu, 16 Nov 2017 02:33:48 +0200 Laurent Pinchart escreveu: > >>> Device unplug being asynchronous, it naturally races with operations > >>> performed by userspace through ioctls or other file operations on > >>> video device nodes. > >>> > >>> This leads to potential access to freed memory or to other resources > >>> during device access if unplug occurs during device access. To solve > >>> this, we need to wait until all device access completes when > >>> unplugging the device, and block all further access when the device is > >>> being unplugged. > >>> > >>> Three new functions are introduced. The video_device_enter() and > >>> video_device_exit() functions must be used to mark entry and exit from > >>> all code sections where the device can be accessed. The > >>> video_device_unplug() function is then used in the unplug handler to > >>> mark the device as being unplugged and wait for all access to > >>> complete. > >>> > >>> As an example mark the ioctl handler as a device access section. Other > >>> file operations need to be protected too, and blocking ioctls (such as > >>> VIDIOC_DQBUF) need to be handled as well. > >>> > >>> Signed-off-by: Laurent Pinchart > >>> > >>> --- > >>> > >>> drivers/media/v4l2-core/v4l2-dev.c | 57 ++ > >>> include/media/v4l2-dev.h | 47 ++ > >>> 2 files changed, 104 insertions(+) [snip] > >> I'm c/c Greg here, as I don't think, that, the way it is, it > >> belongs at V4L2 core. > >> > >> I mean: if this is a problem that affects all drivers, it would should, > >> instead, be sitting at the driver's core. > > > > What "problem" is trying to be solved here? One where your specific > > device type races with your specific user api? Doesn't sound very > > driver-core specific to me :) > > > > As an example, what other bus/device type needs this? If you can see > > others that do, then sure, move it into the core. But for just one, I > > don't know if that's really needed here, do you? > > The problem that this patch is trying to solve is related to > hot-unplugging a platform device[1]. Quoting Laurent's comments about > it on IRC: > > "it applies to all platform devices at least" Note how I said "at least" :-) I2C, SPI and PCI devices are affected too, and after a closer look at USB today I believe USB devices are affected as well. > "I'm actually considering moving that code to the device core as >it applies to all drivers that have device nodes, but I'm not >sure that will be feasible it won't hurt other devices >it applies to I2C and SPI as well at least and PCI too" > > [1] https://linuxtv.org/irc/irclogger_log/media-maint?date=2017-11-23,Thu > > For USB drivers, hot-unplug seems to work fine for media drivers, > although keeping it working require tests from time to time, as > it is not hard to break hotplug support. so, currently, I don't see > the need of anything like that for non-platform drivers. I2C, SPI and PCI are non-platform drivers, and USB seems to be affected too. The race window is small, making it difficult to reproduce the problem, but with carefully placed delays it gets much easier to hit the race. > My point here is that adding a new lock inside the media core that > would be used for all media drivers, including the ones that don't need > doesn't sound a good idea. Why not, if it doesn't affect performances (or anything else) negatively ? > So, if this is something that applies to all platform drivers (including > non-media ones), or if are there anything that can be done at driver's core > that would improve hotplug support for all buses, making it more stable or > easier to implement, then it would make sense to improve the driver's core. > If not, this sounds a driver-specific issue whose fix doesn't belong to the > media core. It's clearly not a driver-specific issue as most, if not all, drivers are affected. I've replied to Greg's e-mail in this thread with more details, let's try to keep the discussion there to avoid splitting it in multiple sub-threads. -- Regards, Laurent Pinchart
[GIT PULL FOR v4.16] staging/media: add NVIDIA Tegra video decoder driver
This adds a new NVIDIA Tegra video decoder driver. It is depending on the request API work since it is a stateless codec, so for now park this in staging. The dts patches should go through nvidia's tree. Regards, Hans The following changes since commit 330dada5957e3ca0c8811b14c45e3ac42c694651: media: dvb_frontend: fix return error code (2017-12-12 07:50:14 -0500) are available in the Git repository at: git://linuxtv.org/hverkuil/media_tree.git tegradec for you to fetch changes up to c3c530f45e48b33a2cc49cdeec246d255a5ca7db: staging: media: Introduce NVIDIA Tegra video decoder driver (2017-12-12 16:06:06 +0100) Dmitry Osipenko (2): media: dt: bindings: Add binding for NVIDIA Tegra Video Decoder Engine staging: media: Introduce NVIDIA Tegra video decoder driver Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt | 55 +++ MAINTAINERS |9 + drivers/staging/media/Kconfig|2 + drivers/staging/media/Makefile |1 + drivers/staging/media/tegra-vde/Kconfig |7 + drivers/staging/media/tegra-vde/Makefile |1 + drivers/staging/media/tegra-vde/TODO |4 + drivers/staging/media/tegra-vde/tegra-vde.c | 1213 ++ drivers/staging/media/tegra-vde/uapi.h | 78 +++ 9 files changed, 1370 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt create mode 100644 drivers/staging/media/tegra-vde/Kconfig create mode 100644 drivers/staging/media/tegra-vde/Makefile create mode 100644 drivers/staging/media/tegra-vde/TODO create mode 100644 drivers/staging/media/tegra-vde/tegra-vde.c create mode 100644 drivers/staging/media/tegra-vde/uapi.h
Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race
Hi Greg and Mauro,
On Thursday, 23 November 2017 16:21:01 EET Greg Kroah-Hartman wrote:
> On Thu, Nov 23, 2017 at 11:07:51AM -0200, Mauro Carvalho Chehab wrote:
> > Em Thu, 16 Nov 2017 02:33:48 +0200 Laurent Pinchart escreveu:
> >> Device unplug being asynchronous, it naturally races with operations
> >> performed by userspace through ioctls or other file operations on video
> >> device nodes.
> >>
> >> This leads to potential access to freed memory or to other resources
> >> during device access if unplug occurs during device access. To solve
> >> this, we need to wait until all device access completes when unplugging
> >> the device, and block all further access when the device is being
> >> unplugged.
> >>
> >> Three new functions are introduced. The video_device_enter() and
> >> video_device_exit() functions must be used to mark entry and exit from
> >> all code sections where the device can be accessed. The
> >> video_device_unplug() function is then used in the unplug handler to
> >> mark the device as being unplugged and wait for all access to complete.
> >>
> >> As an example mark the ioctl handler as a device access section. Other
> >> file operations need to be protected too, and blocking ioctls (such as
> >> VIDIOC_DQBUF) need to be handled as well.
> >>
> >> Signed-off-by: Laurent Pinchart
> >>
> >> ---
> >>
> >> drivers/media/v4l2-core/v4l2-dev.c | 57
> >> include/media/v4l2-dev.h | 47 +++
> >> 2 files changed, 104 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> >> b/drivers/media/v4l2-core/v4l2-dev.c index c647ba648805..c73c6d49e7cf
> >> 100644
> >> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> >> @@ -156,6 +156,52 @@ void video_device_release_empty(struct video_device
> >> *vdev)
> >> }
> >> EXPORT_SYMBOL(video_device_release_empty);
> >>
> >> +int video_device_enter(struct video_device *vdev)
> >> +{
> >> + bool unplugged;
> >> +
> >> + spin_lock(&vdev->unplug_lock);
> >> + unplugged = vdev->unplugged;
> >> + if (!unplugged)
> >> + vdev->access_refcount++;
> >> + spin_unlock(&vdev->unplug_lock);
> >> +
> >> + return unplugged ? -ENODEV : 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(video_device_enter);
> >> +
> >> +void video_device_exit(struct video_device *vdev)
> >> +{
> >> + bool wake_up;
> >> +
> >> + spin_lock(&vdev->unplug_lock);
> >> + WARN_ON(--vdev->access_refcount < 0);
> >> + wake_up = vdev->access_refcount == 0;
> >> + spin_unlock(&vdev->unplug_lock);
> >> +
> >> + if (wake_up)
> >> + wake_up(&vdev->unplug_wait);
> >> +}
> >> +EXPORT_SYMBOL_GPL(video_device_exit);
> >> +
> >> +void video_device_unplug(struct video_device *vdev)
> >> +{
> >> + bool unplug_blocked;
> >> +
> >> + spin_lock(&vdev->unplug_lock);
> >> + unplug_blocked = vdev->access_refcount > 0;
> >> + vdev->unplugged = true;
> >> + spin_unlock(&vdev->unplug_lock);
> >> +
> >> + if (!unplug_blocked)
> >> + return;
> >> +
> >> + if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
> >> + msecs_to_jiffies(15)))
> >> + WARN(1, "Timeout waiting for device access to complete\n");
> >> +}
> >> +EXPORT_SYMBOL_GPL(video_device_unplug);
> >> +
> >> static inline void video_get(struct video_device *vdev)
> >> {
> >>get_device(&vdev->dev);
> >> @@ -351,6 +397,10 @@ static long v4l2_ioctl(struct file *filp, unsigned
> >> int cmd, unsigned long arg)
> >>struct video_device *vdev = video_devdata(filp);
> >>int ret = -ENODEV;
> >>
> >> + ret = video_device_enter(vdev);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >>if (vdev->fops->unlocked_ioctl) {
> >>struct mutex *lock = v4l2_ioctl_get_lock(vdev, cmd);
> >>
> >> @@ -358,11 +408,14 @@ static long v4l2_ioctl(struct file *filp, unsigned
> >> int cmd, unsigned long arg)
> >>return -ERESTARTSYS;
> >>if (video_is_registered(vdev))
> >>ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
> >> + else
> >> + ret = -ENODEV;
> >>if (lock)
> >>mutex_unlock(lock);
> >>} else
> >>ret = -ENOTTY;
> >>
> >> + video_device_exit(vdev);
> >>return ret;
> >> }
> >>
> >> @@ -841,6 +894,10 @@ int __video_register_device(struct video_device
> >> *vdev, int type, int nr,
> >>if (WARN_ON(!vdev->v4l2_dev))
> >>return -EINVAL;
> >>
> >> + /* unplug support */
> >> + spin_lock_init(&vdev->unplug_lock);
> >> + init_waitqueue_head(&vdev->unplug_wait);
> >> +
> >
> > I'm c/c Greg here, as I don't think, that, the way it is, it
> > belongs at V4L2 core.
> >
> > I mean: if this is a problem that affects all drivers, it would should,
> > instead, be sitting at the driver's core.
>
> What "problem" is trying to be solved here? One where your specific
> device type races with your
Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race
Hi Mauro, On Thursday, 23 November 2017 15:07:51 EET Mauro Carvalho Chehab wrote: > Em Thu, 16 Nov 2017 02:33:48 +0200 Laurent Pinchart escreveu: > > Device unplug being asynchronous, it naturally races with operations > > performed by userspace through ioctls or other file operations on video > > device nodes. > > > > This leads to potential access to freed memory or to other resources > > during device access if unplug occurs during device access. To solve > > this, we need to wait until all device access completes when unplugging > > the device, and block all further access when the device is being > > unplugged. > > > > Three new functions are introduced. The video_device_enter() and > > video_device_exit() functions must be used to mark entry and exit from > > all code sections where the device can be accessed. The > > video_device_unplug() function is then used in the unplug handler to > > mark the device as being unplugged and wait for all access to complete. > > > > As an example mark the ioctl handler as a device access section. Other > > file operations need to be protected too, and blocking ioctls (such as > > VIDIOC_DQBUF) need to be handled as well. > > > > Signed-off-by: Laurent Pinchart > > > > --- > > > > drivers/media/v4l2-core/v4l2-dev.c | 57 + > > include/media/v4l2-dev.h | 47 +++ > > 2 files changed, 104 insertions(+) [snip] > I'm c/c Greg here, as I don't think, that, the way it is, it > belongs at V4L2 core. > > I mean: if this is a problem that affects all drivers, it would should, > instead, be sitting at the driver's core. > > If, otherwise, this is specific to rcar-vin (and other platform drivers), > that's likely should be inside the drivers that require it. > > That's said, I remember we had to add some things in the past for > USB drivers hot unplug to happen softly. I don't remember the specifics > anymore, but it was solved by both a V4L2 core and changes at USB > drivers. > > One of the things that it was added, on that time, was this patch: > > commit ae6cfaace120f4330715b56265ce0e4a710e1276 > Author: Hans Verkuil > Date: Sat Mar 14 08:28:45 2009 -0300 > > V4L/DVB (11044): v4l2-device: add v4l2_device_disconnect > > So, I would expect that a change at V4L2 core (or at driver core) that > would be applied would also be affecting USB drivers disconnect logic > and v4l2_device_disconnect() function. I wasn't aware of v4l2_device_disconnect(), thank you for pointing it out. I don't know what the full history behind that function is, but I don't see why it's needed. struct device instances are refcounted, the struct device corresponding to a USB device or USB interface doesn't get freed when a device is disconnected as long as a reference is present. We take such a reference in v4l2_device_register(), so there should be no problem. [snip] -- Regards, Laurent Pinchart
Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race
Hi Hans, On Friday, 17 November 2017 13:09:20 EET Hans Verkuil wrote: > On 16/11/17 01:33, Laurent Pinchart wrote: > > Device unplug being asynchronous, it naturally races with operations > > performed by userspace through ioctls or other file operations on video > > device nodes. > > > > This leads to potential access to freed memory or to other resources > > during device access if unplug occurs during device access. To solve > > this, we need to wait until all device access completes when unplugging > > the device, and block all further access when the device is being > > unplugged. > > > > Three new functions are introduced. The video_device_enter() and > > video_device_exit() functions must be used to mark entry and exit from > > all code sections where the device can be accessed. The > > video_device_unplug() function is then used in the unplug handler to > > mark the device as being unplugged and wait for all access to complete. > > > > As an example mark the ioctl handler as a device access section. Other > > file operations need to be protected too, and blocking ioctls (such as > > VIDIOC_DQBUF) need to be handled as well. > > As long as the queue field in struct video_device is filled in properly > this shouldn't be a problem. > > This looks pretty good, simple and straightforward. Thank you. > Do we need something similar for media_device? Other devices? I believe so, which is why I'm wondering whether this shouldn't somehow go to the device core (and in the cdev core). Not all devices will need such an infrastructure as some subsystems already protect against device access after unbind (USB is one of them if I'm not mistaken), but it certainly shouldn't hurt. DRM is also considering a similar implementation, but based on srcu to lower the performance penalty. I feel that's going a bit overboard but I have no numbers yet to confirm or infirm the suspicion. > > Signed-off-by: Laurent Pinchart > > > > --- > > > > drivers/media/v4l2-core/v4l2-dev.c | 57 + > > include/media/v4l2-dev.h | 47 +++ > > 2 files changed, 104 insertions(+) [snip] -- Regards, Laurent Pinchart
Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race
Hi Kieran,
On Thursday, 16 November 2017 16:47:11 EET Kieran Bingham wrote:
> On 16/11/17 12:32, Sakari Ailus wrote:
> > Hi Laurent,
> >
> > Thank you for the initiative to bring up and address the matter!
>
> I concur - this looks like a good start towards managing the issue.
>
> One potential thing spotted on top of Sakari's review inline below, of
> course I suspect this was more of a prototype/consideration patch.
>
> > On Thu, Nov 16, 2017 at 02:33:48AM +0200, Laurent Pinchart wrote:
> >> Device unplug being asynchronous, it naturally races with operations
> >> performed by userspace through ioctls or other file operations on video
> >> device nodes.
> >>
> >> This leads to potential access to freed memory or to other resources
> >> during device access if unplug occurs during device access. To solve
> >> this, we need to wait until all device access completes when unplugging
> >> the device, and block all further access when the device is being
> >> unplugged.
> >>
> >> Three new functions are introduced. The video_device_enter() and
> >> video_device_exit() functions must be used to mark entry and exit from
> >> all code sections where the device can be accessed. The
> >
> > I wonder if it'd help splitting this patch into two: one that introduces
> > the mechanism and the other that uses it. Up to you.
> >
> > Nevertheless, it'd be better to have other system calls covered as well.
> >
> >> video_device_unplug() function is then used in the unplug handler to
> >> mark the device as being unplugged and wait for all access to complete.
> >>
> >> As an example mark the ioctl handler as a device access section. Other
> >> file operations need to be protected too, and blocking ioctls (such as
> >> VIDIOC_DQBUF) need to be handled as well.
> >>
> >> Signed-off-by: Laurent Pinchart
> >>
> >> ---
> >>
> >> drivers/media/v4l2-core/v4l2-dev.c | 57
> >> include/media/v4l2-dev.h | 47 +++
> >> 2 files changed, 104 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> >> b/drivers/media/v4l2-core/v4l2-dev.c index c647ba648805..c73c6d49e7cf
> >> 100644
> >> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-dev.c
[snip]
> >> @@ -351,6 +397,10 @@ static long v4l2_ioctl(struct file *filp, unsigned
> >> int cmd, unsigned long arg)
> >>struct video_device *vdev = video_devdata(filp);
> >>int ret = -ENODEV;
> >
> > You could leave ret unassigned here.
> >
> >> + ret = video_device_enter(vdev);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >>if (vdev->fops->unlocked_ioctl) {
> >>struct mutex *lock = v4l2_ioctl_get_lock(vdev, cmd);
> >>
> >> @@ -358,11 +408,14 @@ static long v4l2_ioctl(struct file *filp, unsigned
> >> int cmd, unsigned long arg)
> >>return -ERESTARTSYS;
>
> It looks like that return -ERESTARTSYS might need a video_device_exit() too?
Oops. Of course. I'll fix that. Thanks for catching the issue.
> >>if (video_is_registered(vdev))
> >>ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
> >> + else
> >> + ret = -ENODEV;
> >>if (lock)
> >>mutex_unlock(lock);
> >>} else
> >>ret = -ENOTTY;
> >>
> >> + video_device_exit(vdev);
> >>return ret;
> >> }
--
Regards,
Laurent Pinchart
Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race
Hi Sakari,
On Thursday, 16 November 2017 14:32:37 EET Sakari Ailus wrote:
> On Thu, Nov 16, 2017 at 02:33:48AM +0200, Laurent Pinchart wrote:
> > Device unplug being asynchronous, it naturally races with operations
> > performed by userspace through ioctls or other file operations on video
> > device nodes.
> >
> > This leads to potential access to freed memory or to other resources
> > during device access if unplug occurs during device access. To solve
> > this, we need to wait until all device access completes when unplugging
> > the device, and block all further access when the device is being
> > unplugged.
> >
> > Three new functions are introduced. The video_device_enter() and
> > video_device_exit() functions must be used to mark entry and exit from
> > all code sections where the device can be accessed. The
>
> I wonder if it'd help splitting this patch into two: one that introduces
> the mechanism and the other that uses it. Up to you.
Sure, I'm not opposed to that. The second patch would be a bit small, but that
will change when I'll add support for more system calls.
> Nevertheless, it'd be better to have other system calls covered as well.
>
> > video_device_unplug() function is then used in the unplug handler to
> > mark the device as being unplugged and wait for all access to complete.
> >
> > As an example mark the ioctl handler as a device access section. Other
> > file operations need to be protected too, and blocking ioctls (such as
> > VIDIOC_DQBUF) need to be handled as well.
> >
> > Signed-off-by: Laurent Pinchart
> >
> > ---
> >
> > drivers/media/v4l2-core/v4l2-dev.c | 57 +
> > include/media/v4l2-dev.h | 47 +++
> > 2 files changed, 104 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> > b/drivers/media/v4l2-core/v4l2-dev.c index c647ba648805..c73c6d49e7cf
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -156,6 +156,52 @@ void video_device_release_empty(struct video_device
> > *vdev)>
> > }
> > EXPORT_SYMBOL(video_device_release_empty);
> >
> > +int video_device_enter(struct video_device *vdev)
> > +{
> > + bool unplugged;
> > +
> > + spin_lock(&vdev->unplug_lock);
> > + unplugged = vdev->unplugged;
> > + if (!unplugged)
> > + vdev->access_refcount++;
> > + spin_unlock(&vdev->unplug_lock);
> > +
> > + return unplugged ? -ENODEV : 0;
> > +}
> > +EXPORT_SYMBOL_GPL(video_device_enter);
> > +
> > +void video_device_exit(struct video_device *vdev)
> > +{
> > + bool wake_up;
> > +
> > + spin_lock(&vdev->unplug_lock);
> > + WARN_ON(--vdev->access_refcount < 0);
> > + wake_up = vdev->access_refcount == 0;
> > + spin_unlock(&vdev->unplug_lock);
> > +
> > + if (wake_up)
> > + wake_up(&vdev->unplug_wait);
> > +}
> > +EXPORT_SYMBOL_GPL(video_device_exit);
>
> Is there a need to export the two, i.e. wouldn't you only call them from
> the framework, or the same module?
There could be a need to call these functions from entry points that are not
controlled by the V4L2 core, such as sysfs or debugfs. We could keep the
functions internal for now and only export them when the need arises, but if
we want to document how drivers need to handle race conditions between device
access and device unbind, we need to have them exported.
> > +
> > +void video_device_unplug(struct video_device *vdev)
> > +{
> > + bool unplug_blocked;
> > +
> > + spin_lock(&vdev->unplug_lock);
> > + unplug_blocked = vdev->access_refcount > 0;
> > + vdev->unplugged = true;
>
> Shouldn't this be set to false in video_register_device()?
Yes it should. I currently rely on the fact that the memory is zeroed when
allocated, but I shouldn't. I'll fix that.
> > + spin_unlock(&vdev->unplug_lock);
> > +
> > + if (!unplug_blocked)
> > + return;
>
> Not necessary, wait_event_timeout() handles this already.
I'll fix this as well.
> > +
> > + if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
> > + msecs_to_jiffies(15)))
> > + WARN(1, "Timeout waiting for device access to complete\n");
>
> Why a timeout? Does this get somehow less problematic over time? :-)
Not quite :-) This should never happen, but driver and/or core bugs could
cause a timeout. In that case I think proceeding after a timeout is a better
option than deadlocking forever.
> > +}
> > +EXPORT_SYMBOL_GPL(video_device_unplug);
> > +
> >
> > static inline void video_get(struct video_device *vdev)
> > {
> >
> > get_device(&vdev->dev);
> >
> > @@ -351,6 +397,10 @@ static long v4l2_ioctl(struct file *filp, unsigned
> > int cmd, unsigned long arg)>
> > struct video_device *vdev = video_devdata(filp);
> > int ret = -ENODEV;
>
> You could leave ret unassigned here.
I'll fix that.
> > + ret = video_device_enter(vdev);
> > + if (ret < 0)
> > + retu
Re: [PATCH] tuners: tda8290: reduce stack usage with kasan
On Tue, Dec 12, 2017 at 1:45 PM, Mauro Carvalho Chehab
wrote:
> Em Tue, 12 Dec 2017 03:42:32 -0800
> Joe Perches escreveu:
>
>> > I actually thought about marking them 'const' here before sending
>> > (without noticing the changelog text) and then ran into what must
>> > have led me to drop the 'const' originally: tuner_i2c_xfer_send()
>> > takes a non-const pointer. This can be fixed but it requires
>> > an ugly cast:
>>
>> Casting away const is always a horrible hack.
>>
>> Until it could be changed, my preference would
>> be to update the changelog and perhaps add to
>> the changelog the reason why it can not be const
>> as detailed below.
>>
>> ie: xfer_send and xfer_xend_recv both take a
>> non-const unsigned char *
Ok.
> Perhaps, on a separate changeset, we could change I2C routines to
> accept const unsigned char pointers. This is unrelated to tda8290
> KASAN fixes. So, it should go via I2C tree, and, once accepted
> there, we can change V4L2 drivers (and other drivers) accordingly.
I don't see how that would work unfortunately. i2c_msg contains
a pointer to the data, and that is used for both input and output,
including arrays like
struct i2c_msg msgs[] = {
{
.addr = dvo->slave_addr,
.flags = 0,
.len = 1,
.buf = &addr,
},
{
.addr = dvo->slave_addr,
.flags = I2C_M_RD,
.len = 1,
.buf = val,
}
};
that have one constant output pointer and one non-constant
input pointer. We could add an anonymous union for 'buf'
to make that two separate pointers, but that's barely any
better than the cast, and it would break the named initializers
in the example above, at least on older compilers. Adding
a second pointer to i2c_msg would add a bit of bloat and
also require tree-wide changes or ugly hacks.
Arnd
[PATCH 2/2] bdisp: Fix a possible sleep-in-atomic bug in bdisp_hw_save_request
The driver may sleep under a spinlock.
The function call path is:
bdisp_device_run (acquire the spinlock)
bdisp_hw_update
bdisp_hw_save_request
devm_kzalloc(GFP_KERNEL) --> may sleep
To fix it, GFP_KERNEL is replaced with GFP_ATOMIC.
This bug is found by my static analysis tool(DSAC) and checked by my code
review.
Signed-off-by: Jia-Ju Bai
---
drivers/media/platform/sti/bdisp/bdisp-hw.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/sti/bdisp/bdisp-hw.c
b/drivers/media/platform/sti/bdisp/bdisp-hw.c
index 4b62ceb..7b45b43 100644
--- a/drivers/media/platform/sti/bdisp/bdisp-hw.c
+++ b/drivers/media/platform/sti/bdisp/bdisp-hw.c
@@ -1064,7 +1064,7 @@ static void bdisp_hw_save_request(struct bdisp_ctx *ctx)
if (!copy_node[i]) {
copy_node[i] = devm_kzalloc(ctx->bdisp_dev->dev,
sizeof(*copy_node[i]),
- GFP_KERNEL);
+ GFP_ATOMIC);
if (!copy_node[i])
return;
}
--
1.7.9.5
[PATCH 1/2] bdisp: Fix a possible sleep-in-atomic bug in bdisp_hw_reset
The driver may sleep under a spinlock.
The function call path is:
bdisp_device_run (acquire the spinlock)
bdisp_hw_reset
msleep --> may sleep
To fix it, msleep is replaced with mdelay.
This bug is found by my static analysis tool(DSAC) and checked by my code
review.
Signed-off-by: Jia-Ju Bai
---
drivers/media/platform/sti/bdisp/bdisp-hw.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/sti/bdisp/bdisp-hw.c
b/drivers/media/platform/sti/bdisp/bdisp-hw.c
index b7892f3..4b62ceb 100644
--- a/drivers/media/platform/sti/bdisp/bdisp-hw.c
+++ b/drivers/media/platform/sti/bdisp/bdisp-hw.c
@@ -382,7 +382,7 @@ int bdisp_hw_reset(struct bdisp_dev *bdisp)
for (i = 0; i < POLL_RST_MAX; i++) {
if (readl(bdisp->regs + BLT_STA1) & BLT_STA1_IDLE)
break;
- msleep(POLL_RST_DELAY_MS);
+ mdelay(POLL_RST_DELAY_MS);
}
if (i == POLL_RST_MAX)
dev_err(bdisp->dev, "Reset timeout\n");
--
1.7.9.5
[PATCH] media: dvb_frontend: fix return error code
The correct error code when a function is not defined is
-ENOTSUPP. It was typoed wrong as -EOPNOTSUPP, with,
unfortunately, exists, but it is not used by the DVB core.
Thanks-to: Geert Uytterhoeven
Thanks-to: Arnd Bergmann
To make me revisit this code.
Fixes: a9cb97c3e628 ("media: dvb_frontend: be sure to init
dvb_frontend_handle_ioctl() return code")
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/dvb-core/dvb_frontend.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/dvb-core/dvb_frontend.c
b/drivers/media/dvb-core/dvb_frontend.c
index 46f977177faf..4eedaa5922eb 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2110,7 +2110,7 @@ static int dvb_frontend_handle_ioctl(struct file *file,
struct dvb_frontend *fe = dvbdev->priv;
struct dvb_frontend_private *fepriv = fe->frontend_priv;
struct dtv_frontend_properties *c = &fe->dtv_property_cache;
- int i, err = -EOPNOTSUPP;
+ int i, err = -ENOTSUPP;
dev_dbg(fe->dvb->device, "%s:\n", __func__);
--
2.14.3
Re: [PATCH v4 3/5] staging: Introduce NVIDIA Tegra video decoder driver
On 10/12/17 19:56, Dmitry Osipenko wrote: > On 05.12.2017 16:03, Hans Verkuil wrote: >> On 12/05/17 13:17, Dmitry Osipenko wrote: >>> Hi Hans, >>> >>> On 04.12.2017 17:04, Hans Verkuil wrote: Hi Dmitry, As you already mention in the TODO, this should become a v4l2 codec driver. Good existing examples are the coda, qcom/venus and mtk-vcodec drivers. One thing that is not clear from this code is if the tegra hardware is a stateful or stateless codec, i.e. does it keep track of the decoder state in the hardware, or does the application have to keep track of the state and provide the state information together with the video data? I ask because at the moment only stateful codecs are supported. Work is ongoing to support stateless codecs, but we don't support that for now. >>> >>> It is stateless. Is there anything ready to try out? If yes, could you >>> please >>> give a reference to that work? >> >> I rebased my two year old 'requests2' branch to the latest mainline version >> and >> gave it the imaginative name 'requests3': >> >> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=requests3 >> >> (Note: only compile tested!) > > Thank you very much. > >> This is what ChromeOS has been using (actually they use a slightly older >> version) >> and the new version that is currently being developed will be similar, so >> any work >> you do on top of this will carry over to the final version without too much >> effort. >> >> At least, that's the intention :-) >> >> I've CC-ed Maxime and Giulio as well: they are looking into adding support >> for >> the stateless allwinner codec based on this code as well. There may well be >> opportunities for you to work together, esp. on the userspace side. Note that >> Rockchip has the same issue, they too have a stateless HW codec. > > IIUC, we will have to define video decoder parameters in V4L API and then > make a > V4L driver / userspace prototype (ffmpeg for example) that will use the > requests > API for video decoding in order to upstream the requests API. Does it sound > good? Correct. Hugues Fruchet made an example bit parser for mpeg: https://www.spinics.net/lists/linux-media/msg115017.html So something similar would work. My recommendation would be to make a separate library that can be shared among different implementations (i.e. in gstreamer, in vdpau, using a libv4l2 plugin, etc., etc.). That can easily be hosted as part of v4l-utils to keep it in sync with the kernel drivers. If it's independent of the various drivers, then it can be hosted anywhere of course. BTW, we as V4L2 core developers have no plans on working on such a library :-) Regards, Hans > >>> Anyway, I'm OK with merging this in staging. Although I think it should go to staging/media since we want to keep track of it. >>> >>> Awesome, I'll move driver to staging/media in V5. Thanks! >> >> Nice, thanks!
Re: [PATCH] tuners: tda8290: reduce stack usage with kasan
Em Tue, 12 Dec 2017 03:42:32 -0800 Joe Perches escreveu: > > I actually thought about marking them 'const' here before sending > > (without noticing the changelog text) and then ran into what must > > have led me to drop the 'const' originally: tuner_i2c_xfer_send() > > takes a non-const pointer. This can be fixed but it requires > > an ugly cast: > > Casting away const is always a horrible hack. > > Until it could be changed, my preference would > be to update the changelog and perhaps add to > the changelog the reason why it can not be const > as detailed below. > > ie: xfer_send and xfer_xend_recv both take a > non-const unsigned char * Perhaps, on a separate changeset, we could change I2C routines to accept const unsigned char pointers. This is unrelated to tda8290 KASAN fixes. So, it should go via I2C tree, and, once accepted there, we can change V4L2 drivers (and other drivers) accordingly. Thanks, Mauro
Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race
Em Thu, 23 Nov 2017 15:21:01 +0100
Greg Kroah-Hartman escreveu:
> On Thu, Nov 23, 2017 at 11:07:51AM -0200, Mauro Carvalho Chehab wrote:
> > Hi Laurent,
> >
> > Em Thu, 16 Nov 2017 02:33:48 +0200
> > Laurent Pinchart escreveu:
> >
> > > Device unplug being asynchronous, it naturally races with operations
> > > performed by userspace through ioctls or other file operations on video
> > > device nodes.
> > >
> > > This leads to potential access to freed memory or to other resources
> > > during device access if unplug occurs during device access. To solve
> > > this, we need to wait until all device access completes when unplugging
> > > the device, and block all further access when the device is being
> > > unplugged.
> > >
> > > Three new functions are introduced. The video_device_enter() and
> > > video_device_exit() functions must be used to mark entry and exit from
> > > all code sections where the device can be accessed. The
> > > video_device_unplug() function is then used in the unplug handler to
> > > mark the device as being unplugged and wait for all access to complete.
> > >
> > > As an example mark the ioctl handler as a device access section. Other
> > > file operations need to be protected too, and blocking ioctls (such as
> > > VIDIOC_DQBUF) need to be handled as well.
> > >
> > > Signed-off-by: Laurent Pinchart
> > >
> > > ---
> > > drivers/media/v4l2-core/v4l2-dev.c | 57
> > > ++
> > > include/media/v4l2-dev.h | 47 +++
> > > 2 files changed, 104 insertions(+)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> > > b/drivers/media/v4l2-core/v4l2-dev.c
> > > index c647ba648805..c73c6d49e7cf 100644
> > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > @@ -156,6 +156,52 @@ void video_device_release_empty(struct video_device
> > > *vdev)
> > > }
> > > EXPORT_SYMBOL(video_device_release_empty);
> > >
> > > +int video_device_enter(struct video_device *vdev)
> > > +{
> > > + bool unplugged;
> > > +
> > > + spin_lock(&vdev->unplug_lock);
> > > + unplugged = vdev->unplugged;
> > > + if (!unplugged)
> > > + vdev->access_refcount++;
> > > + spin_unlock(&vdev->unplug_lock);
> > > +
> > > + return unplugged ? -ENODEV : 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(video_device_enter);
> > > +
> > > +void video_device_exit(struct video_device *vdev)
> > > +{
> > > + bool wake_up;
> > > +
> > > + spin_lock(&vdev->unplug_lock);
> > > + WARN_ON(--vdev->access_refcount < 0);
> > > + wake_up = vdev->access_refcount == 0;
> > > + spin_unlock(&vdev->unplug_lock);
> > > +
> > > + if (wake_up)
> > > + wake_up(&vdev->unplug_wait);
> > > +}
> > > +EXPORT_SYMBOL_GPL(video_device_exit);
> > > +
> > > +void video_device_unplug(struct video_device *vdev)
> > > +{
> > > + bool unplug_blocked;
> > > +
> > > + spin_lock(&vdev->unplug_lock);
> > > + unplug_blocked = vdev->access_refcount > 0;
> > > + vdev->unplugged = true;
> > > + spin_unlock(&vdev->unplug_lock);
> > > +
> > > + if (!unplug_blocked)
> > > + return;
> > > +
> > > + if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
> > > + msecs_to_jiffies(15)))
> > > + WARN(1, "Timeout waiting for device access to complete\n");
> > > +}
> > > +EXPORT_SYMBOL_GPL(video_device_unplug);
> > > +
> > > static inline void video_get(struct video_device *vdev)
> > > {
> > > get_device(&vdev->dev);
> > > @@ -351,6 +397,10 @@ static long v4l2_ioctl(struct file *filp, unsigned
> > > int cmd, unsigned long arg)
> > > struct video_device *vdev = video_devdata(filp);
> > > int ret = -ENODEV;
> > >
> > > + ret = video_device_enter(vdev);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > if (vdev->fops->unlocked_ioctl) {
> > > struct mutex *lock = v4l2_ioctl_get_lock(vdev, cmd);
> > >
> > > @@ -358,11 +408,14 @@ static long v4l2_ioctl(struct file *filp, unsigned
> > > int cmd, unsigned long arg)
> > > return -ERESTARTSYS;
> > > if (video_is_registered(vdev))
> > > ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
> > > + else
> > > + ret = -ENODEV;
> > > if (lock)
> > > mutex_unlock(lock);
> > > } else
> > > ret = -ENOTTY;
> > >
> > > + video_device_exit(vdev);
> > > return ret;
> > > }
> > >
> > > @@ -841,6 +894,10 @@ int __video_register_device(struct video_device
> > > *vdev, int type, int nr,
> > > if (WARN_ON(!vdev->v4l2_dev))
> > > return -EINVAL;
> > >
> > > + /* unplug support */
> > > + spin_lock_init(&vdev->unplug_lock);
> > > + init_waitqueue_head(&vdev->unplug_wait);
> > > +
> >
> > I'm c/c Greg here, as I don't think, that, the way it is, it
> > belongs at V4L2 core.
> >
> > I mean: if this is a problem that affects all drivers, it would should,
> > instead, be sitting at the driver's co
Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver
Hi Tim, Sorry for the delay, I needed to find some time to think about this. On 11/16/17 05:30, Rob Herring wrote: > On Wed, Nov 15, 2017 at 10:31:14AM -0800, Tim Harvey wrote: >> On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring wrote: >>> On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote: Add support for the TDA1997x HDMI receivers. Cc: Hans Verkuil Signed-off-by: Tim Harvey --- v3: - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros - fixed missing break - use only hdmi_infoframe_log for infoframe logging - simplify tda1997x_s_stream error handling - add delayed work proc to handle hotplug enable/disable - fix set_edid (disable HPD before writing, enable after) - remove enabling edid by default - initialize timings - take quant range into account in colorspace conversion - remove vendor/product tracking (we provide this in log_status via infoframes) - add v4l_controls - add more detail to log_status - calculate vhref generator timings - timing detection fixes (rounding errors, hswidth errors) - rename configure_input/configure_conv functions v2: - implement dv timings enum/cap - remove deprecated g_mbus_config op - fix dv_query_timings - add EDID get/set handling - remove max-pixel-rate support - add audio codec DAI support - change audio bindings --- drivers/media/i2c/Kconfig|9 + drivers/media/i2c/Makefile |1 + drivers/media/i2c/tda1997x.c | 3485 ++ include/dt-bindings/media/tda1997x.h | 78 + >>> >>> This belongs with the binding documentation patch. >>> >> >> Rob, >> >> Thanks - missed that. I will move it for v4. >> >> Regarding your previous comment to the v2 series: >>> The rest of the binding looks fine, but I have some reservations about >>> this. I think this should be common probably. There's been a few >>> bindings for display recently that deal with the interface format. Maybe >>> some vendor property is needed here to map a standard interface format >>> back to pin configuration. >> >> I take it this is not an 'Ack' for the bindings? >> >> Which did you feel should be made common? I admit I was surprised >> there wasn't a common binding for audio bus format (i2s|spdif) but if >> you were referring to the video data that would probably be much more >> complicated. > > The video data. Either you have to try to come up with some way to map > color components to signals/pins (and even cycles) or you just enumerate > the formats and keep adding to them when new ones appear. There's h/w > that allows the former, but in the end you have to interoperate, so > enumerating the formats is probably enough. > >> I was hoping one of the media/driver maintainers would respond to your >> comment with thoughts as I'm not familiar with a very wide variety of >> receivers. > > I am hoping, too. I don't think it is right to store this in the DT. How you map the output pins is a driver thing. So when you are requested to enumerate the mediabus formats (include/uapi/linux/media-bus-format.h) you support, you do so based on the capabilities of the hardware, and when a format is requested you program the hardware accordingly. The device tree should describe the physical characteristics like the number of pins that are hooked up (i.e. are there 24, 30 or 36 pins connected). These vidout-portcfg mappings do not appear to describe physical properties but really register settings. Apologies if I misunderstood any of this. Regards, Hans
Re: [PATCH] tuners: tda8290: reduce stack usage with kasan
On Tue, 2017-12-12 at 11:24 +0100, Arnd Bergmann wrote:
> On Mon, Dec 11, 2017 at 10:17 PM, Michael Ira Krufky
> wrote:
> > On Mon, Dec 11, 2017 at 2:34 PM, Joe Perches wrote:
> > > On Mon, 2017-12-11 at 13:06 +0100, Arnd Bergmann wrote:
> > > > With CONFIG_KASAN enabled, we get a relatively large stack frame in one
> > > > function
> > > >
> > > > drivers/media/tuners/tda8290.c: In function 'tda8290_set_params':
> > > > drivers/media/tuners/tda8290.c:310:1: warning: the frame size of 1520
> > > > bytes is larger than 1024 bytes [-Wframe-larger-than=]
> > > >
> > > > With CONFIG_KASAN_EXTRA this goes up to
> > > >
> > > > drivers/media/tuners/tda8290.c: In function 'tda8290_set_params':
> > > > drivers/media/tuners/tda8290.c:310:1: error: the frame size of 3200
> > > > bytes is larger than 3072 bytes [-Werror=frame-larger-than=]
> > > >
> > > > We can significantly reduce this by marking local arrays as 'static
> > > > const', and
> > > > this should result in better compiled code for everyone.
> > >
> > > []
> > > > diff --git a/drivers/media/tuners/tda8290.c
> > > > b/drivers/media/tuners/tda8290.c
> > >
> > > []
> > > > @@ -63,8 +63,8 @@ static int tda8290_i2c_bridge(struct dvb_frontend
> > > > *fe, int close)
> > > > {
> > > > struct tda8290_priv *priv = fe->analog_demod_priv;
> > > >
> > > > - unsigned char enable[2] = { 0x21, 0xC0 };
> > > > - unsigned char disable[2] = { 0x21, 0x00 };
> > > > + static unsigned char enable[2] = { 0x21, 0xC0 };
> > > > + static unsigned char disable[2] = { 0x21, 0x00 };
> > >
> > > Doesn't match commit message.
> > >
> > > static const or just static?
> > >
> > > > @@ -84,9 +84,9 @@ static int tda8295_i2c_bridge(struct dvb_frontend
> > > > *fe, int close)
> > > > {
> > > > struct tda8290_priv *priv = fe->analog_demod_priv;
> > > >
> > > > - unsigned char enable[2] = { 0x45, 0xc1 };
> > > > - unsigned char disable[2] = { 0x46, 0x00 };
> > > > - unsigned char buf[3] = { 0x45, 0x01, 0x00 };
> > > > + static unsigned char enable[2] = { 0x45, 0xc1 };
> > > > + static unsigned char disable[2] = { 0x46, 0x00 };
> > >
> > > etc.
> > >
> > >
> >
> >
> > Joe is correct - they can be CONSTified. My bad -- a lot of the code I
> > wrote many years ago has this problem -- I wasn't so stack-conscious
> > back then.
> >
> > The bytes in `enable` / `disable` don't get changed, but they may be
> > copied to another byte array that does get changed. If would be best
> > to make these `static const`
>
> Right. This was an older patch of mine that I picked up again
> after running into a warning that I had been ignoring for a while,
> and I didn't double-check the message.
>
> I actually thought about marking them 'const' here before sending
> (without noticing the changelog text) and then ran into what must
> have led me to drop the 'const' originally: tuner_i2c_xfer_send()
> takes a non-const pointer. This can be fixed but it requires
> an ugly cast:
Casting away const is always a horrible hack.
Until it could be changed, my preference would
be to update the changelog and perhaps add to
the changelog the reason why it can not be const
as detailed below.
ie: xfer_send and xfer_xend_recv both take a
non-const unsigned char *
> diff --git a/drivers/media/tuners/tuner-i2c.h
> b/drivers/media/tuners/tuner-i2c.h
> index bda67a5a76f2..809466eec780 100644
> --- a/drivers/media/tuners/tuner-i2c.h
> +++ b/drivers/media/tuners/tuner-i2c.h
> @@ -34,10 +34,10 @@ struct tuner_i2c_props {
> };
>
> static inline int tuner_i2c_xfer_send(struct tuner_i2c_props *props,
> - unsigned char *buf, int len)
> + const unsigned char *buf, int len)
> {
> struct i2c_msg msg = { .addr = props->addr, .flags = 0,
> - .buf = buf, .len = len };
> + .buf = (unsigned char *)buf, .len = len };
> int ret = i2c_transfer(props->adap, &msg, 1);
>
> return (ret == 1) ? len : ret;
> @@ -54,11 +54,11 @@ static inline int tuner_i2c_xfer_recv(struct
> tuner_i2c_props *props,
> }
>
> static inline int tuner_i2c_xfer_send_recv(struct tuner_i2c_props *props,
> - unsigned char *obuf, int olen,
> + const unsigned char *obuf, int
> olen,
>unsigned char *ibuf, int ilen)
> {
> struct i2c_msg msg[2] = { { .addr = props->addr, .flags = 0,
> - .buf = obuf, .len = olen },
> + .buf = (unsigned char *)obuf, .len = olen
> },
> { .addr = props->addr, .flags = I2C_M_RD,
> .buf = ibuf, .len = ilen } };
> int ret = i2c_transfer(props->adap, msg, 2);
>
> Should I submit it as a two-patch series with that added in, or update
Re: [PATCH v4 0/9] vsp1: TLB optimisation and DL caching
Hi Kieran,
On Fri, Nov 17, 2017 at 4:47 PM, Kieran Bingham
wrote:
> Each display list currently allocates an area of DMA memory to store register
> settings for the VSP1 to process. Each of these allocations adds pressure to
> the IPMMU TLB entries.
>
> We can reduce the pressure by pre-allocating larger areas and dividing the
> area
> across multiple bodies represented as a pool.
>
> With this reconfiguration of bodies, we can adapt the configuration code to
> separate out constant hardware configuration and cache it for re-use.
>
> --
>
> The patches provided in this series can be found at:
> git://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git
> tags/vsp1/tlb-optimise/v4
This started to conflict with commit dd286a531461748f ("v4l: vsp1:
Start and stop DRM pipeline independently of planes"), causing build
failures as it changes the signature of vsp1_entity_route_setup(), and
removed several VSP1_ENTITY_PARAMS_* definitions.
After fixing those, it hangs after:
[drm] No driver support for vblank timestamp query.
So I dropped the above for today's release.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 1/3] atomisp: Fix up the open v load race
Em Mon, 06 Nov 2017 23:36:36 +
Alan escreveu:
> This isn't the ideal final solution but it stops the main problem for now
> where an open (often from udev) races the device initialization and we try
> and load the firmware twice at the same time. This needless to say doesn't
> usually end well.
What we do on most drivers is that video_register_device() is called
only after all hardware init.
That's usually enough to avoid race conditions with udev, although
a mutex is also common in order to avoid some other race conditions
between open/close - with can happen with multiple opens.
>
> Signed-off-by: Alan Cox
> ---
> .../media/atomisp/pci/atomisp2/atomisp_fops.c | 12
> .../media/atomisp/pci/atomisp2/atomisp_internal.h |5 +
> .../media/atomisp/pci/atomisp2/atomisp_v4l2.c |6 ++
> 3 files changed, 23 insertions(+)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
> index dd7596d8763d..b82c53cee32c 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
> @@ -771,6 +771,18 @@ static int atomisp_open(struct file *file)
>
> dev_dbg(isp->dev, "open device %s\n", vdev->name);
>
> + /* Ensure that if we are still loading we block. Once the loading
> +is over we can proceed. We can't blindly hold the lock until
> +that occurs as if the load fails we'll deadlock the unload */
> + rt_mutex_lock(&isp->loading);
> + /* Revisit this with a better check once the code structure is
> +cleaned up a bit more FIXME */
> + if (!isp->ready) {
> + rt_mutex_unlock(&isp->loading);
> + return -ENXIO;
> + }
> + rt_mutex_unlock(&isp->loading);
> +
> rt_mutex_lock(&isp->mutex);
>
> acc_node = !strncmp(vdev->name, "ATOMISP ISP ACC",
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
> index 52a6f8002048..808d79c840d4 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
> @@ -252,6 +252,11 @@ struct atomisp_device {
> /* Purpose of mutex is to protect and serialize use of isp data
>* structures and css API calls. */
> struct rt_mutex mutex;
> + /* This mutex ensures that we don't allow an open to succeed while
> + * the initialization process is incomplete */
> + struct rt_mutex loading;
> + /* Set once the ISP is ready to allow opens */
> + bool ready;
> /*
>* Serialise streamoff: mutex is dropped during streamoff to
>* cancel the watchdog queue. MUST be acquired BEFORE
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
> index 3c260f8b52e2..350e298bc3a6 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
> @@ -1220,6 +1220,7 @@ static int atomisp_pci_probe(struct pci_dev *dev,
> isp->saved_regs.ispmmadr = start;
>
> rt_mutex_init(&isp->mutex);
> + rt_mutex_init(&isp->loading);
> mutex_init(&isp->streamoff_mutex);
> spin_lock_init(&isp->lock);
>
> @@ -1393,6 +1394,8 @@ static int atomisp_pci_probe(struct pci_dev *dev,
> csi_afe_trim);
> }
>
> + rt_mutex_lock(&isp->loading);
> +
> err = atomisp_initialize_modules(isp);
> if (err < 0) {
> dev_err(&dev->dev, "atomisp_initialize_modules (%d)\n", err);
> @@ -1450,6 +1453,8 @@ static int atomisp_pci_probe(struct pci_dev *dev,
> release_firmware(isp->firmware);
> isp->firmware = NULL;
> isp->css_env.isp_css_fw.data = NULL;
> + isp->ready = true;
> + rt_mutex_unlock(&isp->loading);
>
> atomisp_drvfs_init(&atomisp_pci_driver, isp);
>
> @@ -1468,6 +1473,7 @@ static int atomisp_pci_probe(struct pci_dev *dev,
> register_entities_fail:
> atomisp_uninitialize_modules(isp);
> initialize_modules_fail:
> + rt_mutex_unlock(&isp->loading);
> pm_qos_remove_request(&isp->pm_qos);
> atomisp_msi_irq_uninit(isp, dev);
> enable_msi_fail:
>
Thanks,
Mauro
[PATCH] s5p-mfc: Fix encoder menu controls initialization
This patch fixes the menu_skip_mask field initialization
and addresses a following issue found by the SVACE static
analysis:
* NO_EFFECT.SELF: assignment to self in expression 'cfg.menu_skip_mask =
cfg.menu_skip_mask'
No effect at drivers/media/platform/s5p-mfc/s5p_mfc_enc.c:2083
Signed-off-by: Sylwester Nawrocki
---
drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 2a5fd7c42cd5..0d5d465561be 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -2080,7 +2080,7 @@ int s5p_mfc_enc_ctrls_setup(struct s5p_mfc_ctx *ctx)
if (cfg.type == V4L2_CTRL_TYPE_MENU) {
cfg.step = 0;
- cfg.menu_skip_mask = cfg.menu_skip_mask;
+ cfg.menu_skip_mask = controls[i].menu_skip_mask;
cfg.qmenu = mfc51_get_menu(cfg.id);
} else {
cfg.step = controls[i].step;
--
2.14.2
Re: [PATCH 2/2] dt-bindings: Document the Rockchip RK1608 bindings
Hi Leo,
Am Dienstag, 12. Dezember 2017, 14:28:15 CET schrieb Leo Wen:
> +Optional properties:
> +
> +- spi-max-frequency : Maximum SPI clocking speed of the device;
> + (for RK1608)
> +- spi-min-frequency : Minimum SPI clocking speed of the device;
> + (for RK1608)
There is no general spi-min-frequency property specified and I also guess
systems would try to use a frequency close to maximum anyway, so I don't
really see the use of specifying a minimum frequency.
> +&pinctrl {
> + rk1608_irq_gpios {
> + rk1608_irq_gpios: rk1608_irq_gpios {
> + rockchip,pins = <6 2 RK_FUNC_GPIO &pcfg_pull_none>;
> + rockchip,pull = <1>;
> + };
> + };
There is no need to specify the soc-specific pinctrl settings in a general
devicetree example and you're using properties from your vendor-tree
like the rockchip,pull one ... that are not used in the mainline kernel.
So I'd suggest dropping the whole &pinctrl from the example.
Heiko
Re: [PATCH] tuners: tda8290: reduce stack usage with kasan
On Mon, Dec 11, 2017 at 10:17 PM, Michael Ira Krufky
wrote:
> On Mon, Dec 11, 2017 at 2:34 PM, Joe Perches wrote:
>> On Mon, 2017-12-11 at 13:06 +0100, Arnd Bergmann wrote:
>>> With CONFIG_KASAN enabled, we get a relatively large stack frame in one
>>> function
>>>
>>> drivers/media/tuners/tda8290.c: In function 'tda8290_set_params':
>>> drivers/media/tuners/tda8290.c:310:1: warning: the frame size of 1520 bytes
>>> is larger than 1024 bytes [-Wframe-larger-than=]
>>>
>>> With CONFIG_KASAN_EXTRA this goes up to
>>>
>>> drivers/media/tuners/tda8290.c: In function 'tda8290_set_params':
>>> drivers/media/tuners/tda8290.c:310:1: error: the frame size of 3200 bytes
>>> is larger than 3072 bytes [-Werror=frame-larger-than=]
>>>
>>> We can significantly reduce this by marking local arrays as 'static const',
>>> and
>>> this should result in better compiled code for everyone.
>> []
>>> diff --git a/drivers/media/tuners/tda8290.c b/drivers/media/tuners/tda8290.c
>> []
>>> @@ -63,8 +63,8 @@ static int tda8290_i2c_bridge(struct dvb_frontend *fe,
>>> int close)
>>> {
>>> struct tda8290_priv *priv = fe->analog_demod_priv;
>>>
>>> - unsigned char enable[2] = { 0x21, 0xC0 };
>>> - unsigned char disable[2] = { 0x21, 0x00 };
>>> + static unsigned char enable[2] = { 0x21, 0xC0 };
>>> + static unsigned char disable[2] = { 0x21, 0x00 };
>>
>> Doesn't match commit message.
>>
>> static const or just static?
>>
>>> @@ -84,9 +84,9 @@ static int tda8295_i2c_bridge(struct dvb_frontend *fe,
>>> int close)
>>> {
>>> struct tda8290_priv *priv = fe->analog_demod_priv;
>>>
>>> - unsigned char enable[2] = { 0x45, 0xc1 };
>>> - unsigned char disable[2] = { 0x46, 0x00 };
>>> - unsigned char buf[3] = { 0x45, 0x01, 0x00 };
>>> + static unsigned char enable[2] = { 0x45, 0xc1 };
>>> + static unsigned char disable[2] = { 0x46, 0x00 };
>>
>> etc.
>>
>>
>
>
> Joe is correct - they can be CONSTified. My bad -- a lot of the code I
> wrote many years ago has this problem -- I wasn't so stack-conscious
> back then.
>
> The bytes in `enable` / `disable` don't get changed, but they may be
> copied to another byte array that does get changed. If would be best
> to make these `static const`
Right. This was an older patch of mine that I picked up again
after running into a warning that I had been ignoring for a while,
and I didn't double-check the message.
I actually thought about marking them 'const' here before sending
(without noticing the changelog text) and then ran into what must
have led me to drop the 'const' originally: tuner_i2c_xfer_send()
takes a non-const pointer. This can be fixed but it requires
an ugly cast:
diff --git a/drivers/media/tuners/tuner-i2c.h b/drivers/media/tuners/tuner-i2c.h
index bda67a5a76f2..809466eec780 100644
--- a/drivers/media/tuners/tuner-i2c.h
+++ b/drivers/media/tuners/tuner-i2c.h
@@ -34,10 +34,10 @@ struct tuner_i2c_props {
};
static inline int tuner_i2c_xfer_send(struct tuner_i2c_props *props,
- unsigned char *buf, int len)
+ const unsigned char *buf, int len)
{
struct i2c_msg msg = { .addr = props->addr, .flags = 0,
- .buf = buf, .len = len };
+ .buf = (unsigned char *)buf, .len = len };
int ret = i2c_transfer(props->adap, &msg, 1);
return (ret == 1) ? len : ret;
@@ -54,11 +54,11 @@ static inline int tuner_i2c_xfer_recv(struct
tuner_i2c_props *props,
}
static inline int tuner_i2c_xfer_send_recv(struct tuner_i2c_props *props,
- unsigned char *obuf, int olen,
+ const unsigned char *obuf, int olen,
unsigned char *ibuf, int ilen)
{
struct i2c_msg msg[2] = { { .addr = props->addr, .flags = 0,
- .buf = obuf, .len = olen },
+ .buf = (unsigned char *)obuf, .len = olen },
{ .addr = props->addr, .flags = I2C_M_RD,
.buf = ibuf, .len = ilen } };
int ret = i2c_transfer(props->adap, msg, 2);
Should I submit it as a two-patch series with that added in, or update
the changelog to not mention 'const' instead?
Arnd
Re: [PATCH v1 05/10] arch: sh: migor: Use new renesas-ceu camera driver
Hi Jacopo,
Thank you for the patch.
On Wednesday, 15 November 2017 12:55:58 EET Jacopo Mondi wrote:
> Migo-R platform uses sh_mobile_ceu camera driver, which is now being
> replaced by a proper V4L2 camera driver named 'renesas-ceu'.
>
> Move Migo-R platform to use the v4l2 renesas-ceu camera driver
> interface and get rid of soc_camera defined components used to register
> sensor drivers.
>
> Also, memory for CEU video buffers is now reserved with membocks APIs,
> and need to be declared as dma_coherent during machine initialization to
> remove that architecture specific part from CEU driver.
>
> Signed-off-by: Jacopo Mondi
> ---
> arch/sh/boards/mach-migor/setup.c | 164 --
> 1 file changed, 76 insertions(+), 88 deletions(-)
>
> diff --git a/arch/sh/boards/mach-migor/setup.c
> b/arch/sh/boards/mach-migor/setup.c index 98aa094..10a9b3c 100644
> --- a/arch/sh/boards/mach-migor/setup.c
> +++ b/arch/sh/boards/mach-migor/setup.c
> @@ -27,7 +27,7 @@
> #include
> #include
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -308,62 +308,80 @@ static struct platform_device migor_lcdc_device = {
> static struct clk *camera_clk;
> static DEFINE_MUTEX(camera_lock);
>
> -static void camera_power_on(int is_tw)
> +static void camera_vio_clk_on(void)
> {
> - mutex_lock(&camera_lock);
> -
> /* Use 10 MHz VIO_CKO instead of 24 MHz to work
>* around signal quality issues on Panel Board V2.1.
>*/
> camera_clk = clk_get(NULL, "video_clk");
> clk_set_rate(camera_clk, 1000);
> clk_enable(camera_clk); /* start VIO_CKO */
> -
> - /* use VIO_RST to take camera out of reset */
> - mdelay(10);
> - if (is_tw) {
> - gpio_set_value(GPIO_PTT2, 0);
> - gpio_set_value(GPIO_PTT0, 0);
> - } else {
> - gpio_set_value(GPIO_PTT0, 1);
> - }
> - gpio_set_value(GPIO_PTT3, 0);
> - mdelay(10);
> - gpio_set_value(GPIO_PTT3, 1);
> - mdelay(10); /* wait to let chip come out of reset */
> }
>
> -static void camera_power_off(void)
> +static void camera_disable(void)
> {
> - clk_disable(camera_clk); /* stop VIO_CKO */
> + /* stop VIO_CKO */
> + clk_disable(camera_clk);
> clk_put(camera_clk);
>
> + gpio_set_value(GPIO_PTT0, 0);
> + gpio_set_value(GPIO_PTT2, 1);
> gpio_set_value(GPIO_PTT3, 0);
> +
> mutex_unlock(&camera_lock);
> }
>
> -static int ov7725_power(struct device *dev, int mode)
> +static void camera_reset(void)
> {
> - if (mode)
> - camera_power_on(0);
> - else
> - camera_power_off();
> + /* use VIO_RST to take camera out of reset */
> + gpio_set_value(GPIO_PTT3, 0);
> + mdelay(10);
> + gpio_set_value(GPIO_PTT3, 1);
> + mdelay(10);
> +}
> +
> +static int ov7725_enable(void)
> +{
> + mutex_lock(&camera_lock);
> + camera_vio_clk_on();
> + mdelay(10);
> + gpio_set_value(GPIO_PTT0, 1);
> +
> + camera_reset();
>
> return 0;
> }
>
> -static int tw9910_power(struct device *dev, int mode)
> +static int tw9910_enable(void)
> {
> - if (mode)
> - camera_power_on(1);
> - else
> - camera_power_off();
> + mutex_lock(&camera_lock);
> + camera_vio_clk_on();
> + mdelay(10);
> + gpio_set_value(GPIO_PTT2, 0);
> +
> + camera_reset();
>
> return 0;
> }
After studying the schematics, and if you can confirm that R26 is not mounted
on the panel board, I think all this could be handled by the OV7720 and TW9910
drivers.
The clock is easy, it's used by the OV7720 only, just expose it as the input
clock for that device. On a side note, your ov772x driver in this series tries
to get a clock named "mclk", but it should be named "xclk" as that's how the
chip's input signal is named. The TW9910 has its own crystal oscillator so it
won't be affected. As a bonus point the clock will remain off when capturing
from the TW9910.
The TV_IN_EN and CAM_EN signals (PTT2 and PTT0 GPIOs respectively) shouldn't
be difficult either. You can expose them as the PDN and PWDN GPIOs for the
OV7720 and TW9910 and handle them in the respective drivers. CAM_EN also
controls the digital video bus multiplexing, and unless I'm mistaken it will
just work as a side effect of power down signal control as long as you make
sure that the OV7720 remains in power down when not selected as the CEU input.
The VIO_RST signal (PTT3 GPIO) is a bit more annoying, as it is shared by both
the OV7720 and TW9910 as their reset signal, and as far as I know GPIOs can't
be easily shared between drivers.
Using a reset controller might be an option but I can't see any reset
controller driver for GPIOs. https://patchwork.kernel.org/patch/3978091/ seems
not to have been merged. This being said, having to instantiate a reset
controller to share a GPIO is annoying, I wonder if it would make sense to add
support for shared GPI
Re: [Patch v6 10/12] [media] v4l2: Add v4l2 control IDs for HEVC encoder
On 12/12/2017 03:34 AM, Smitha T Murthy wrote: >> s/Lay/Layer here and below >> > Ok I will change it. While it's fine to make such change for controls up to V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_QP... >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L1_QP:return "HEVC >>> Hierarchical Lay 1 QP"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L2_QP:return "HEVC >>> Hierarchical Lay 2 QP"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L3_QP:return "HEVC >>> Hierarchical Lay 3 QP"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L4_QP:return "HEVC >>> Hierarchical Lay 4 QP"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L5_QP:return "HEVC >>> Hierarchical Lay 5 QP"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_QP:return "HEVC >>> Hierarchical Lay 6 QP"; ...for the controls below we may need to replace "Lay" with "L." to make sure the length of the string don't exceed 31 characters (32 with terminating NULL). The names below seem to be 1 character too long and will be truncated when running VIDIOC_QUERY_CTRL ioctl. >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L0_BR:return "HEVC >>> Hierarchical Lay 0 Bit Rate"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L1_BR:return "HEVC >>> Hierarchical Lay 1 Bit Rate"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L2_BR:return "HEVC >>> Hierarchical Lay 2 Bit Rate"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L3_BR:return "HEVC >>> Hierarchical Lay 3 Bit Rate"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L4_BR:return "HEVC >>> Hierarchical Lay 4 Bit Rate"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L5_BR:return "HEVC >>> Hierarchical Lay 5 Bit Rate"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR:return "HEVC >>> Hierarchical Lay 6 Bit Rate"; -- Regards, Sylwester
Re: [PATCH 0/2] Add timers to en50221 protocol driver
Yes, acked-by: Ralph Metzler Jasmin J. writes: > Hi! > > > I don't know if you are also waiting for a comment from me. > All positive ones are welcome ;) > > > I am fine with those changes. They are even desperately needed > > to get some "problem CAMs" working. > THX, so we can interpret this as an "Acked-by"? > > BR, >Jasmin -- --
Re: [PATCH v8] uvcvideo: Add a metadata device node
Hi Laurent,
On Mon, 11 Dec 2017, Laurent Pinchart wrote:
> Hi Guennadi,
>
> On Monday, 11 December 2017 23:44:09 EET Guennadi Liakhovetski wrote:
> > On Mon, 11 Dec 2017, Laurent Pinchart wrote:
> > > On Monday, 11 December 2017 22:16:23 EET Laurent Pinchart wrote:
> > >> On Wednesday, 6 December 2017 17:15:40 EET Guennadi Liakhovetski wrote:
> > >>> From: Guennadi Liakhovetski
> > >>>
> > >>> Some UVC video cameras contain metadata in their payload headers. This
> > >>> patch extracts that data, adding more clock synchronisation
> > >>> information, on both bulk and isochronous endpoints and makes it
> > >>> available to the user space on a separate video node, using the
> > >>> V4L2_CAP_META_CAPTURE capability and the V4L2_BUF_TYPE_META_CAPTURE
> > >>> buffer queue type. By default, only the V4L2_META_FMT_UVC pixel format
> > >>> is available from those nodes. However, cameras can be added to the
> > >>> device ID table to additionally specify their own metadata format, in
> > >>> which case that format will also become available from the metadata
> > >>> node.
> > >>>
> > >>> Signed-off-by: Guennadi Liakhovetski
> > >>> ---
> > >>>
> > >>> v8: addressed comments and integrated changes from Laurent, thanks
> > >>> again, e.g.:
> > >>>
> > >>> - multiple stylistic changes
> > >>> - remove the UVC_DEV_FLAG_METADATA_NODE flag / quirk: nodes are now
> > >>> created unconditionally
> > >>> - reuse uvc_ioctl_querycap()
> > >>> - reuse code in uvc_register_video()
> > >>> - set an error flag when the metadata buffer overflows
> > >>>
> > >>> drivers/media/usb/uvc/Makefile | 2 +-
> > >>> drivers/media/usb/uvc/uvc_driver.c | 15 ++-
> > >>> drivers/media/usb/uvc/uvc_isight.c | 2 +-
> > >>> drivers/media/usb/uvc/uvc_metadata.c | 179
> > >>> drivers/media/usb/uvc/uvc_queue.c| 44 +++--
> > >>> drivers/media/usb/uvc/uvc_video.c| 132 --
> > >>> drivers/media/usb/uvc/uvcvideo.h | 16 +++-
> > >>> include/uapi/linux/uvcvideo.h| 26 +
> > >>> 8 files changed, 394 insertions(+), 22 deletions(-)
> > >>> create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
> > >>
> > >> [snip]
> > >>
> > >> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > >> > b/drivers/media/usb/uvc/uvc_video.c index 13f459e..2fc0bf2 100644
> > >> > --- a/drivers/media/usb/uvc/uvc_video.c
> > >> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > >>
> > >> [snip]
> > >>
> > >>> +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> > >>> + struct uvc_buffer *meta_buf,
> > >>> + const u8 *mem, unsigned int length)
> > >>> +{
> > >>> + struct uvc_meta_buf *meta;
> > >>> + size_t len_std = 2;
> > >>> + bool has_pts, has_scr;
> > >>> + unsigned long flags;
> > >>> + ktime_t time;
> > >>> + const u8 *scr;
> > >>> +
> > >>> + if (!meta_buf || length == 2)
> > >>> + return;
> > >>> +
> > >>> + if (meta_buf->length - meta_buf->bytesused <
> > >>> + length + sizeof(meta->ns) + sizeof(meta->sof)) {
> > >>> + meta_buf->error = 1;
> > >>> + return;
> > >>> + }
> > >>> +
> > >>> + has_pts = mem[1] & UVC_STREAM_PTS;
> > >>> + has_scr = mem[1] & UVC_STREAM_SCR;
> > >>> +
> > >>> + if (has_pts) {
> > >>> + len_std += 4;
> > >>> + scr = mem + 6;
> > >>> + } else {
> > >>> + scr = mem + 2;
> > >>> + }
> > >>> +
> > >>> + if (has_scr)
> > >>> + len_std += 6;
> > >>> +
> > >>> + if (stream->meta.format == V4L2_META_FMT_UVC)
> > >>> + length = len_std;
> > >>> +
> > >>> + if (length == len_std && (!has_scr ||
> > >>> + !memcmp(scr, stream->clock.last_scr,
> > >>> 6)))
> > >>> + return;
> > >>> +
> > >>> + meta = (struct uvc_meta_buf *)((u8 *)meta_buf->mem +
> > >>> meta_buf->bytesused); + local_irq_save(flags);
> > >>> + time = uvc_video_get_time();
> > >>> + meta->sof = usb_get_current_frame_number(stream->dev->udev);
> > >>
> > >> You need a put_unaligned here too. If you're fine with the patch below
> > >> there's no need to resubmit, and
> > >
> > > One more thing, __put_unaligned_cpu16() and __put_unaligned_cpu64() don't
> > > compile on x86_64 with v4.12 (using media_build.git). I propose replacing
> > > them with put_unaligned() which compiles and should do the right thing.
> > Sure, thanks for catching! Shall I fix and resubmit?
>
> If you're fine with
>
> git://linuxtv.org/pinchartl/media.git uvc/next
I was a bit concerned about just using "int" for unaligned writing of a
16-bit value, but looking at definitions, after a couple of macros
put_unaligned() currently resolves to one inline functions, which should
make that safe. However, at least theoretically, an arch could deci
