cron job: media_tree daily build: ERRORS

2016-11-29 Thread Hans Verkuil 
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 Nov 30 05:00:18 CET 2016
media-tree git hash:a000f0d3995f622410d433a01e94fbfb45969e27
media_build git hash:   1606032398b1d79149c1507be2029e1a00d8dff0
v4l-utils git hash: dab9bf5687eddea2f4cb8cdb38b3bbc5b079a037
gcc version:i686-linux-gcc (GCC) 6.2.0
sparse version: v0.5.0-3553-g78b2ea6
smatch version: v0.5.0-3553-g78b2ea6
host hardware:  x86_64
host os:4.8.0-164

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: OK
linux-git-arm-pxa: 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: WARNINGS
linux-2.6.37.6-i686: WARNINGS
linux-2.6.38.8-i686: WARNINGS
linux-2.6.39.4-i686: WARNINGS
linux-3.0.60-i686: WARNINGS
linux-3.1.10-i686: WARNINGS
linux-3.2.37-i686: WARNINGS
linux-3.3.8-i686: WARNINGS
linux-3.4.27-i686: WARNINGS
linux-3.5.7-i686: WARNINGS
linux-3.6.11-i686: WARNINGS
linux-3.7.4-i686: WARNINGS
linux-3.8-i686: WARNINGS
linux-3.9.2-i686: WARNINGS
linux-3.10.1-i686: WARNINGS
linux-3.11.1-i686: OK
linux-3.12.67-i686: OK
linux-3.13.11-i686: WARNINGS
linux-3.14.9-i686: WARNINGS
linux-3.15.2-i686: WARNINGS
linux-3.16.7-i686: WARNINGS
linux-3.17.8-i686: WARNINGS
linux-3.18.7-i686: WARNINGS
linux-3.19-i686: WARNINGS
linux-4.0.9-i686: WARNINGS
linux-4.1.33-i686: WARNINGS
linux-4.2.8-i686: WARNINGS
linux-4.3.6-i686: WARNINGS
linux-4.4.22-i686: WARNINGS
linux-4.5.7-i686: WARNINGS
linux-4.6.7-i686: WARNINGS
linux-4.7.5-i686: WARNINGS
linux-4.8-i686: OK
linux-4.9-rc5-i686: OK
linux-2.6.36.4-x86_64: WARNINGS
linux-2.6.37.6-x86_64: WARNINGS
linux-2.6.38.8-x86_64: WARNINGS
linux-2.6.39.4-x86_64: WARNINGS
linux-3.0.60-x86_64: WARNINGS
linux-3.1.10-x86_64: WARNINGS
linux-3.2.37-x86_64: WARNINGS
linux-3.3.8-x86_64: WARNINGS
linux-3.4.27-x86_64: WARNINGS
linux-3.5.7-x86_64: WARNINGS
linux-3.6.11-x86_64: WARNINGS
linux-3.7.4-x86_64: WARNINGS
linux-3.8-x86_64: WARNINGS
linux-3.9.2-x86_64: WARNINGS
linux-3.10.1-x86_64: WARNINGS
linux-3.11.1-x86_64: OK
linux-3.12.67-x86_64: OK
linux-3.13.11-x86_64: WARNINGS
linux-3.14.9-x86_64: WARNINGS
linux-3.15.2-x86_64: WARNINGS
linux-3.16.7-x86_64: WARNINGS
linux-3.17.8-x86_64: WARNINGS
linux-3.18.7-x86_64: WARNINGS
linux-3.19-x86_64: WARNINGS
linux-4.0.9-x86_64: WARNINGS
linux-4.1.33-x86_64: WARNINGS
linux-4.2.8-x86_64: WARNINGS
linux-4.3.6-x86_64: WARNINGS
linux-4.4.22-x86_64: WARNINGS
linux-4.5.7-x86_64: WARNINGS
linux-4.6.7-x86_64: WARNINGS
linux-4.7.5-x86_64: WARNINGS
linux-4.8-x86_64: OK
linux-4.9-rc5-x86_64: OK
apps: WARNINGS
spec-git: OK
smatch: ERRORS
sparse: WARNINGS

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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 4/4] vcodec: mediatek: Add Maintainers entry for Mediatek JPEG driver

2016-11-29 Thread Rick Chang 
Signed-off-by: Rick Chang 
Signed-off-by: Minghsiu Tsai 
Signed-off-by: Bin Liu 
---
 MAINTAINERS | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 93e9f42..6f68fb6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7818,6 +7818,13 @@ L:   net...@vger.kernel.org
 S: Maintained
 F: drivers/net/ethernet/mediatek/
 
+MEDIATEK JPEG DRIVER
+M: Rick Chang 
+M: Bin Liu 
+S: Supported
+F: drivers/media/platform/mtk-jpeg/
+F: Documentation/devicetree/bindings/media/mediatek-jpeg-decoder.txt
+
 MEDIATEK MEDIA DRIVER
 M: Tiffany Lin 
 M: Andrew-CT Chen 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 3/4] arm: dts: mt2701: Add node for Mediatek JPEG Decoder

2016-11-29 Thread Rick Chang 
Signed-off-by: Rick Chang 
Signed-off-by: Minghsiu Tsai 
---
This patch depends on: 
  CCF "Add clock support for Mediatek MT2701"[1]
  iommu and smi "Add the dtsi node of iommu and smi for mt2701"[2]

[1] http://lists.infradead.org/pipermail/linux-mediatek/2016-October/007271.html
[2] https://patchwork.kernel.org/patch/9164013/
---
 arch/arm/boot/dts/mt2701.dtsi | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
index 8f13c70..4dd5048 100644
--- a/arch/arm/boot/dts/mt2701.dtsi
+++ b/arch/arm/boot/dts/mt2701.dtsi
@@ -298,6 +298,20 @@
power-domains = < MT2701_POWER_DOMAIN_ISP>;
};
 
+   jpegdec: jpegdec@15004000 {
+   compatible = "mediatek,mt2701-jpgdec";
+   reg = <0 0x15004000 0 0x1000>;
+   interrupts = ;
+   clocks =  < CLK_IMG_JPGDEC_SMI>,
+ < CLK_IMG_JPGDEC>;
+   clock-names = "jpgdec-smi",
+ "jpgdec";
+   power-domains = < MT2701_POWER_DOMAIN_ISP>;
+   mediatek,larb = <>;
+   iommus = < MT2701_M4U_PORT_JPGDEC_WDMA>,
+< MT2701_M4U_PORT_JPGDEC_BSDMA>;
+   };
+
vdecsys: syscon@1600 {
compatible = "mediatek,mt2701-vdecsys", "syscon";
reg = <0 0x1600 0 0x1000>;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver

2016-11-29 Thread Rick Chang 
Add v4l2 driver for Mediatek JPEG Decoder

Signed-off-by: Rick Chang 
Signed-off-by: Minghsiu Tsai 
---
 drivers/media/platform/Kconfig   |   15 +
 drivers/media/platform/Makefile  |2 +
 drivers/media/platform/mtk-jpeg/Makefile |2 +
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c  | 1302 ++
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h  |  139 +++
 drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.c|  417 +++
 drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.h|   91 ++
 drivers/media/platform/mtk-jpeg/mtk_jpeg_parse.c |  160 +++
 drivers/media/platform/mtk-jpeg/mtk_jpeg_parse.h |   25 +
 drivers/media/platform/mtk-jpeg/mtk_jpeg_reg.h   |   58 +
 10 files changed, 2211 insertions(+)
 create mode 100644 drivers/media/platform/mtk-jpeg/Makefile
 create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
 create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
 create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.c
 create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.h
 create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_parse.c
 create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_parse.h
 create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_reg.h

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 754edbf1..96c9887 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -162,6 +162,21 @@ config VIDEO_CODA
   Coda is a range of video codec IPs that supports
   H.264, MPEG-4, and other video formats.
 
+config VIDEO_MEDIATEK_JPEG
+   tristate "Mediatek JPEG Codec driver"
+   depends on MTK_IOMMU_V1 || COMPILE_TEST
+   depends on VIDEO_DEV && VIDEO_V4L2
+   depends on ARCH_MEDIATEK || COMPILE_TEST
+   depends on HAS_DMA
+   select VIDEOBUF2_DMA_CONTIG
+   select V4L2_MEM2MEM_DEV
+   ---help---
+ Mediatek jpeg codec driver provides HW capability to decode
+ JPEG format
+
+ To compile this driver as a module, choose M here: the
+ module will be called mtk-jpeg
+
 config VIDEO_MEDIATEK_VPU
tristate "Mediatek Video Processor Unit"
depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index f842933..cf701e3 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -68,3 +68,5 @@ obj-$(CONFIG_VIDEO_MEDIATEK_VPU)  += mtk-vpu/
 obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC)+= mtk-vcodec/
 
 obj-$(CONFIG_VIDEO_MEDIATEK_MDP)   += mtk-mdp/
+
+obj-$(CONFIG_VIDEO_MEDIATEK_JPEG)  += mtk-jpeg/
diff --git a/drivers/media/platform/mtk-jpeg/Makefile 
b/drivers/media/platform/mtk-jpeg/Makefile
new file mode 100644
index 000..b2e6069
--- /dev/null
+++ b/drivers/media/platform/mtk-jpeg/Makefile
@@ -0,0 +1,2 @@
+mtk_jpeg-objs := mtk_jpeg_core.o mtk_jpeg_hw.o mtk_jpeg_parse.o
+obj-$(CONFIG_VIDEO_MEDIATEK_JPEG) += mtk_jpeg.o
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c 
b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
new file mode 100644
index 000..63a8994
--- /dev/null
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -0,0 +1,1302 @@
+/*
+ * Copyright (c) 2016 MediaTek Inc.
+ * Author: Ming Hsiu Tsai 
+ * Rick Chang 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 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.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "mtk_jpeg_hw.h"
+#include "mtk_jpeg_core.h"
+#include "mtk_jpeg_parse.h"
+
+static struct mtk_jpeg_fmt mtk_jpeg_formats[] = {
+   {
+   .fourcc = V4L2_PIX_FMT_JPEG,
+   .colplanes  = 1,
+   .flags  = MTK_JPEG_FMT_FLAG_DEC_OUTPUT,
+   },
+   {
+   .fourcc = V4L2_PIX_FMT_YUV420M,
+   .h_sample   = {4, 2, 2},
+   .v_sample   = {4, 2, 2},
+   .colplanes  = 3,
+   .h_align= 5,
+   .v_align= 4,
+   .flags  = MTK_JPEG_FMT_FLAG_DEC_CAPTURE,
+   },
+   {
+   .fourcc = V4L2_PIX_FMT_YUV422M,
+   .h_sample   = {4, 2, 2},
+   .v_sample   = {4, 4, 4},
+   .colplanes  = 3,
+   .h_align=

[PATCH v8 1/4] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder

2016-11-29 Thread Rick Chang 
Add a DT binding documentation for Mediatek JPEG Decoder of
MT2701 SoC.

Signed-off-by: Rick Chang 
Signed-off-by: Minghsiu Tsai 
Acked-by: Rob Herring 
---
 .../bindings/media/mediatek-jpeg-decoder.txt   | 37 ++
 1 file changed, 37 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/media/mediatek-jpeg-decoder.txt

diff --git a/Documentation/devicetree/bindings/media/mediatek-jpeg-decoder.txt 
b/Documentation/devicetree/bindings/media/mediatek-jpeg-decoder.txt
new file mode 100644
index 000..3813947
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-decoder.txt
@@ -0,0 +1,37 @@
+* Mediatek JPEG Decoder
+
+Mediatek JPEG Decoder is the JPEG decode hardware present in Mediatek SoCs
+
+Required properties:
+- compatible : must be one of the following string:
+   "mediatek,mt8173-jpgdec"
+   "mediatek,mt2701-jpgdec"
+- reg : physical base address of the jpeg decoder registers and length of
+  memory mapped region.
+- interrupts : interrupt number to the interrupt controller.
+- clocks: device clocks, see
+  Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
+- clock-names: must contain "jpgdec-smi" and "jpgdec".
+- power-domains: a phandle to the power domain, see
+  Documentation/devicetree/bindings/power/power_domain.txt for details.
+- mediatek,larb: must contain the local arbiters in the current Socs, see
+  Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt
+  for details.
+- iommus: should point to the respective IOMMU block with master port as
+  argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
+  for details.
+
+Example:
+   jpegdec: jpegdec@15004000 {
+   compatible = "mediatek,mt2701-jpgdec";
+   reg = <0 0x15004000 0 0x1000>;
+   interrupts = ;
+   clocks =  < CLK_IMG_JPGDEC_SMI>,
+ < CLK_IMG_JPGDEC>;
+   clock-names = "jpgdec-smi",
+ "jpgdec";
+   power-domains = < MT2701_POWER_DOMAIN_ISP>;
+   mediatek,larb = <>;
+   iommus = < MT2701_M4U_PORT_JPGDEC_WDMA>,
+< MT2701_M4U_PORT_JPGDEC_BSDMA>;
+   };
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 0/4] Add Mediatek JPEG Decoder

2016-11-29 Thread Rick Chang 
This series of patches provide a v4l2 driver to control Mediatek JPEG decoder
for decoding JPEG image and Motion JPEG bitstream.

changes since v7:
- Update MAINTAINERS

changes since v6:
- fix kbuild test fail
- Add patch for MAINTAINERS

changes since v5:
- remove redundant name from struct mtk_jpeg_fmt
- Set state of all buffers to VB2_BUF_STATE_QUEUED if fail in start streaming
- Remove VB2_USERPTR
- Add check for buffer index

changes since v4:
- Change file name of binding documentation
- Revise DT binding documentation
- Revise compatible string

changes since v3:
- Revise DT binding documentation
- Revise compatible string

changes since v2:
- Revise DT binding documentation 

changes since v1:
- Rebase for v4.9-rc1.
- Update Compliance test version and result
- Remove redundant path in Makefile
- Fix potential build error without CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP
- Fix warnings from patch check and smatch check

* Dependency
The patch "arm: dts: mt2701: Add node for JPEG decoder" depends on: 
  CCF "Add clock support for Mediatek MT2701"[1]
  iommu and smi "Add the dtsi node of iommu and smi for mt2701"[2]

[1] http://lists.infradead.org/pipermail/linux-mediatek/2016-October/007271.html
[2] https://patchwork.kernel.org/patch/9164013/

* Compliance test
v4l2-compliance SHA   : 4ad7174b908a36c4f315e3fe2efa7e2f8a6f375a

Driver Info:
Driver name   : mtk-jpeg decode
Card type : mtk-jpeg decoder
Bus info  : platform:15004000.jpegdec
Driver version: 4.9.0
Capabilities  : 0x84204000
Video Memory-to-Memory Multiplanar
Streaming
Extended Pix Format
Device Capabilities
Device Caps   : 0x04204000
Video Memory-to-Memory Multiplanar
Streaming
Extended Pix Format

Compliance test for device /dev/video3 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
test VIDIOC_QUERYCTRL: OK (Not Supported)
test VIDIOC_G/S_CTRL: OK (Not Supported)
test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 0 Private Controls: 0

Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK (Not Supported)
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK
test Scaling: OK

Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK

Test input 0:


Total: 43, Succeeded: 43, Failed: 0, Warnings: 0

Rick Chang (4):
  dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder
  vcodec: mediatek: Add Mediatek JPEG Decoder Driver
  arm: dts: mt2701: Add node for Mediatek JPEG Decoder
  vcodec: mediatek: Add Maintainers entry for Mediatek JPEG driver

 .../bindings/media/mediatek-jpeg-decoder.txt   |   37 +
 MAINTAINERS|

[PATCH v2] media: Protect enable_source and disable_source handler code paths

2016-11-29 Thread Shuah Khan 
Drivers might try to access and run enable_source and disable_source
handlers when the driver that implements these handlers is clearing
the handlers during its unregister.

Fix the following race condition:

process 1   process 2

request video streaming unbind au0828
v4l2 checks if tuner is free
... ...

au0828_unregister_media_device()
... ...
(doesn't hold graph_mutex)
mdev->enable_source = NULL;
if (mdev && mdev->enable_source)mdev->disable_source = NULL;
mdev->enable_source()
(enable_source holds graph_mutex)

As shown above enable_source check is done without holding the graph_mutex.
If unbind happens to be in progress, au0828 could clear enable_source and
disable_source handlers leading to null pointer de-reference.

Fix it by protecting enable_source and disable_source set and clear and
protecting enable_source and disable_source handler access and the call
itself.

process 1   process 2

request video streaming unbind au0828
v4l2 checks if tuner is free
... ...

au0828_unregister_media_device()
... ...
(hold graph_mutex while clearing)
mdev->enable_source = NULL;
if (mdev)   mdev->disable_source = NULL;
(hold graph_mutex to check and
 call enable_source)
if (mdev->enable_source)
mdev->enable_source()

If graph_mutex is held to just heck for handler being null and needs to be
released before calling the handler, there will be another window for the
handlers to be cleared. Hence, enable_source and disable_source handlers
no longer hold the graph_mutex and expect callers to hold it to avoid
forcing them release the graph_mutex before calling the handlers.

Signed-off-by: Shuah Khan 
---

Changes since v1:
- Collapsed two patches into one.
- Updated changelog for clarity
- Updated Documentation

 drivers/media/dvb-core/dvb_frontend.c  | 24 ++--
 drivers/media/usb/au0828/au0828-core.c | 21 +
 drivers/media/v4l2-core/v4l2-mc.c  | 26 ++
 include/media/media-device.h   |  2 ++
 4 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c 
b/drivers/media/dvb-core/dvb_frontend.c
index 01511e5..2f09c7e 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, struct 
file *file)
fepriv->voltage = -1;
 
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-   if (fe->dvb->mdev && fe->dvb->mdev->enable_source) {
-   ret = fe->dvb->mdev->enable_source(dvbdev->entity,
+   if (fe->dvb->mdev) {
+   mutex_lock(>dvb->mdev->graph_mutex);
+   if (fe->dvb->mdev->enable_source)
+   ret = fe->dvb->mdev->enable_source(
+  dvbdev->entity,
   >pipe);
+   mutex_unlock(>dvb->mdev->graph_mutex);
if (ret) {
dev_err(fe->dvb->device,
"Tuner is busy. Error %d\n", ret);
@@ -2553,8 +2557,12 @@ static int dvb_frontend_open(struct inode *inode, struct 
file *file)
 
 err3:
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-   if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
-   fe->dvb->mdev->disable_source(dvbdev->entity);
+   if (fe->dvb->mdev) {
+   mutex_lock(>dvb->mdev->graph_mutex);
+   if (fe->dvb->mdev->disable_source)
+   fe->dvb->mdev->disable_source(dvbdev->entity);
+   mutex_unlock(>dvb->mdev->graph_mutex);
+   }
 err2:
 #endif
dvb_generic_release(inode, file);
@@ -2586,8 +2594,12 @@ static int dvb_frontend_release(struct inode *inode, 
struct file *file)
if (dvbdev->users == -1) {
wake_up(>wait_queue);
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-   if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
-   fe->dvb->mdev->disable_source(dvbdev->entity);
+   if (fe->dvb->mdev) {
+   mutex_lock(>dvb->mdev->graph_mutex);
+   if (fe->dvb->mdev->disable_source)
+   fe->dvb->mdev->disable_source(dvbdev->entity);
+   mutex_unlock(>dvb->mdev->graph_mutex);
+   }
 #endif
if (fe->exit

[PATCH v4 1/4] [media] davinci: vpif_capture: don't lock over s_stream

2016-11-29 Thread Kevin Hilman 
Video capture subdevs may be over I2C and may sleep during xfer, so we
cannot do IRQ-disabled locking when calling the subdev.

Signed-off-by: Kevin Hilman 
---
 drivers/media/platform/davinci/vpif_capture.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/davinci/vpif_capture.c 
b/drivers/media/platform/davinci/vpif_capture.c
index 5104cc0ee40e..9f8f41c0f251 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -193,7 +193,10 @@ static int vpif_start_streaming(struct vb2_queue *vq, 
unsigned int count)
}
}
 
+   spin_unlock_irqrestore(>irqlock, flags);
ret = v4l2_subdev_call(ch->sd, video, s_stream, 1);
+   spin_lock_irqsave(>irqlock, flags);
+
if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV) {
vpif_dbg(1, debug, "stream on failed in subdev\n");
goto err;
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 3/4] [media] davinci: vpif_capture: get subdevs from DT

2016-11-29 Thread Kevin Hilman 
Allow getting of subdevs from DT ports and endpoints.

The _get_pdata() function was larely inspired by (i.e. stolen from)
am437x-vpfe.c

Signed-off-by: Kevin Hilman 
---
 drivers/media/platform/davinci/vpif_capture.c | 138 +-
 include/media/davinci/vpif_types.h|   9 +-
 2 files changed, 141 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif_capture.c 
b/drivers/media/platform/davinci/vpif_capture.c
index a83df07e4051..4e363da2c21f 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -26,6 +26,8 @@
 #include 
 
 #include 
+#include 
+#include 
 
 #include "vpif.h"
 #include "vpif_capture.h"
@@ -651,6 +653,10 @@ static int vpif_input_to_subdev(
 
vpif_dbg(2, debug, "vpif_input_to_subdev\n");
 
+   if (!chan_cfg)
+   return -1;
+   if (input_index >= chan_cfg->input_count)
+   return -1;
subdev_name = chan_cfg->inputs[input_index].subdev_name;
if (subdev_name == NULL)
return -1;
@@ -658,7 +664,7 @@ static int vpif_input_to_subdev(
/* loop through the sub device list to get the sub device info */
for (i = 0; i < vpif_cfg->subdev_count; i++) {
subdev_info = _cfg->subdev_info[i];
-   if (!strcmp(subdev_info->name, subdev_name))
+   if (subdev_info && !strcmp(subdev_info->name, subdev_name))
return i;
}
return -1;
@@ -1328,6 +1334,21 @@ static int vpif_async_bound(struct v4l2_async_notifier 
*notifier,
 {
int i;
 
+   for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
+   struct v4l2_async_subdev *_asd = vpif_obj.config->asd[i];
+   const struct device_node *node = _asd->match.of.node;
+
+   if (node == subdev->of_node) {
+   vpif_obj.sd[i] = subdev;
+   vpif_obj.config->chan_config->inputs[i].subdev_name =
+   (char *)subdev->of_node->full_name;
+   vpif_dbg(2, debug,
+"%s: setting input %d subdev_name = %s\n",
+__func__, i, subdev->of_node->full_name);
+   return 0;
+   }
+   }
+
for (i = 0; i < vpif_obj.config->subdev_count; i++)
if (!strcmp(vpif_obj.config->subdev_info[i].name,
subdev->name)) {
@@ -1423,6 +1444,118 @@ static int vpif_async_complete(struct 
v4l2_async_notifier *notifier)
return vpif_probe_complete();
 }
 
+static struct vpif_capture_config *
+vpif_capture_get_pdata(struct platform_device *pdev)
+{
+   struct device_node *endpoint = NULL;
+   struct v4l2_of_endpoint bus_cfg;
+   struct vpif_capture_config *pdata;
+   struct vpif_subdev_info *sdinfo;
+   struct vpif_capture_chan_config *chan;
+   unsigned int i;
+
+   dev_dbg(>dev, "vpif_get_pdata\n");
+
+   /*
+* DT boot: OF node from parent device contains
+* video ports & endpoints data.
+*/
+   if (pdev->dev.parent && pdev->dev.parent->of_node)
+   pdev->dev.of_node = pdev->dev.parent->of_node;
+   if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+   return pdev->dev.platform_data;
+
+   pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
+   if (!pdata)
+   return NULL;
+   pdata->subdev_info =
+   devm_kzalloc(>dev, sizeof(*pdata->subdev_info) *
+VPIF_CAPTURE_NUM_CHANNELS, GFP_KERNEL);
+
+   if (!pdata->subdev_info)
+   return NULL;
+
+   for (i = 0; i < VPIF_CAPTURE_NUM_CHANNELS; i++) {
+   struct device_node *rem;
+   unsigned int flags;
+   int err;
+
+   endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
+ endpoint);
+   if (!endpoint)
+   break;
+
+   sdinfo = >subdev_info[i];
+   chan = >chan_config[i];
+   chan->inputs = devm_kzalloc(>dev,
+   sizeof(*chan->inputs) *
+   VPIF_CAPTURE_NUM_CHANNELS,
+   GFP_KERNEL);
+
+   chan->input_count++;
+   chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;
+   chan->inputs[i].input.std = V4L2_STD_ALL;
+   chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
+
+   /*
+* FIXME: need a new property for input/output routing?
+* All known boards are using ch0:composite ch1: s-video.
+*/
+   if (i == 0)
+   chan->inputs[i].input_route = INPUT_CVBS_VI2B;
+   else
+

[PATCH v4 4/4] [media] dt-bindings: add TI VPIF documentation

2016-11-29 Thread Kevin Hilman 
Signed-off-by: Kevin Hilman 
---
 .../devicetree/bindings/media/ti,da850-vpif.txt| 67 ++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt

diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif.txt 
b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
new file mode 100644
index ..fa06dfdb6898
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
@@ -0,0 +1,67 @@
+Texas Instruments VPIF
+--
+
+The TI Video Port InterFace (VPIF) is the primary component for video
+capture and display on the DA850/AM18x family of TI DaVinci/Sitara
+SoCs.
+
+TI Document reference: SPRUH82C, Chapter 35
+http://www.ti.com/lit/pdf/spruh82
+
+Required properties:
+- compatible: must be "ti,da850-vpif"
+- reg: physical base address and length of the registers set for the device;
+- interrupts: should contain IRQ line for the VPIF
+
+Video Capture:
+
+VPIF has a 16-bit parallel bus input, supporting 2 8-bit channels or a
+single 16-bit channel.  It should contain at least one port child node
+with child 'endpoint' node. Please refer to the bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example using 2 8-bit input channels, one of which is connected to an
+I2C-connected TVP5147 decoder:
+
+   vpif: vpif@217000 {
+   compatible = "ti,da850-vpif";
+   reg = <0x217000 0x1000>;
+   interrupts = <92>;
+
+   port {
+   vpif_ch0: endpoint@0 {
+ reg = <0>;
+ bus-width = <8>;
+ remote-endpoint = <>;
+   };
+
+   vpif_ch1: endpoint@1 {
+ reg = <1>;
+ bus-width = <8>;
+ data-shift = <8>;
+   };
+   };
+   };
+
+[ ... ]
+
+ {
+
+   tvp5147@5d {
+   compatible = "ti,tvp5147";
+   reg = <0x5d>;
+   status = "okay";
+
+   port {
+   composite: endpoint {
+   hsync-active = <1>;
+   vsync-active = <1>;
+   pclk-sample = <0>;
+
+   /* VPIF channel 0 (lower 8-bits) */
+   remote-endpoint = <_ch0>;
+   bus-width = <8>;
+   };
+   };
+   };
+};
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 0/4] davinci: VPIF: add DT support

2016-11-29 Thread Kevin Hilman 
Add DT support, including getting subdevs from DT ports/endpoints.

Tested video capture to memory on da850-lcdk board using composite
input.

Changes since v3:
- move to a single VPIF node, DT binding updated accordingly
- misc fixes/updates based on reviews from Sakari

Changes since v2:
- DT binding doc: fix example to use correct compatible

Changes since v1:
- more specific compatible strings, based on SoC: ti,da850-vpif*
- fix locking bug when unlocking over subdev s_stream


Kevin Hilman (4):
  [media] davinci: vpif_capture: don't lock over s_stream
  [media] davinci: VPIF: add basic support for DT init
  [media] davinci: vpif_capture: get subdevs from DT
  [media] dt-bindings: add TI VPIF documentation

 .../devicetree/bindings/media/ti,da850-vpif.txt|  67 ++
 drivers/media/platform/davinci/vpif.c  |  48 ++-
 drivers/media/platform/davinci/vpif_capture.c  | 147 -
 drivers/media/platform/davinci/vpif_display.c  |   6 +
 include/media/davinci/vpif_types.h |   9 +-
 5 files changed, 270 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt

-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/4] [media] davinci: VPIF: add basic support for DT init

2016-11-29 Thread Kevin Hilman 
Add basic support for initialization via DT.

Because existing capture and display devices are implemented as separate
platform_devices, and in order to preserve that legacy support, during
DT boot, manually create capture and display platform devices to
initialize capture and display support.

Signed-off-by: Kevin Hilman 
---
 drivers/media/platform/davinci/vpif.c | 48 ++-
 drivers/media/platform/davinci/vpif_capture.c |  6 
 drivers/media/platform/davinci/vpif_display.c |  6 
 3 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/davinci/vpif.c 
b/drivers/media/platform/davinci/vpif.c
index 0380cf2e5775..528b30d52208 100644
--- a/drivers/media/platform/davinci/vpif.c
+++ b/drivers/media/platform/davinci/vpif.c
@@ -420,7 +420,8 @@ EXPORT_SYMBOL(vpif_channel_getfid);
 
 static int vpif_probe(struct platform_device *pdev)
 {
-   static struct resource  *res;
+   static struct resource  *res, *res_irq;
+   struct platform_device *pdev_capture, *pdev_display;
 
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
vpif_base = devm_ioremap_resource(>dev, res);
@@ -432,6 +433,42 @@ static int vpif_probe(struct platform_device *pdev)
 
spin_lock_init(_lock);
dev_info(>dev, "vpif probe success\n");
+
+   if (!pdev->dev.of_node)
+   return 0;
+
+   /*
+* For DT platforms, manually create platform_devices for
+* capture/display drivers.
+*/
+   res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+   if (!res_irq) {
+   dev_warn(>dev, "Missing IRQ resource.\n");
+   return -EINVAL;
+   }
+
+   pdev_capture = devm_kzalloc(>dev, sizeof(*pdev_capture),
+   GFP_KERNEL);
+   pdev_capture->name = "vpif_capture";
+   pdev_capture->id = -1;
+   pdev_capture->resource = res_irq;
+   pdev_capture->num_resources = 1;
+   pdev_capture->dev.dma_mask = pdev->dev.dma_mask;
+   pdev_capture->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask;
+   pdev_capture->dev.parent = >dev;
+   platform_device_register(pdev_capture);
+
+   pdev_display = devm_kzalloc(>dev, sizeof(*pdev_display),
+   GFP_KERNEL);
+   pdev_display->name = "vpif_display";
+   pdev_display->id = -1;
+   pdev_display->resource = res_irq;
+   pdev_display->num_resources = 1;
+   pdev_display->dev.dma_mask = pdev->dev.dma_mask;
+   pdev_display->dev.coherent_dma_mask = pdev->dev.coherent_dma_mask;
+   pdev_display->dev.parent = >dev;
+   platform_device_register(pdev_display);
+
return 0;
 }
 
@@ -464,8 +501,17 @@ static const struct dev_pm_ops vpif_pm = {
 #define vpif_pm_ops NULL
 #endif
 
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id vpif_of_match[] = {
+   { .compatible = "ti,da850-vpif", },
+   { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, vpif_of_match);
+#endif
+
 static struct platform_driver vpif_driver = {
.driver = {
+   .of_match_table = of_match_ptr(vpif_of_match),
.name   = "vpif",
.pm = vpif_pm_ops,
},
diff --git a/drivers/media/platform/davinci/vpif_capture.c 
b/drivers/media/platform/davinci/vpif_capture.c
index 9f8f41c0f251..a83df07e4051 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -45,6 +45,7 @@ module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "Debug level 0-1");
 
 #define VPIF_DRIVER_NAME   "vpif_capture"
+MODULE_ALIAS("platform:" VPIF_DRIVER_NAME);
 
 /* global variables */
 static struct vpif_device vpif_obj = { {NULL} };
@@ -1438,6 +1439,11 @@ static __init int vpif_probe(struct platform_device 
*pdev)
int res_idx = 0;
int i, err;
 
+   if (!pdev->dev.platform_data) {
+   dev_warn(>dev, "Missing platform data.  Giving up.\n");
+   return -EINVAL;
+   }
+
vpif_dev = >dev;
 
err = initialize_vpif();
diff --git a/drivers/media/platform/davinci/vpif_display.c 
b/drivers/media/platform/davinci/vpif_display.c
index 75b27233ec2f..7f632b757d32 100644
--- a/drivers/media/platform/davinci/vpif_display.c
+++ b/drivers/media/platform/davinci/vpif_display.c
@@ -42,6 +42,7 @@ module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "Debug level 0-1");
 
 #define VPIF_DRIVER_NAME   "vpif_display"
+MODULE_ALIAS("platform:" VPIF_DRIVER_NAME);
 
 /* Is set to 1 in case of SDTV formats, 2 in case of HDTV formats. */
 static int ycmux_mode;
@@ -1249,6 +1250,11 @@ static __init int vpif_probe(struct platform_device 
*pdev)
int res_idx = 0;
int i, err;
 
+   if (!pdev->dev.platform_data) {
+   dev_warn(>dev, "Missing platform data.  Giving up.\n");
+   return -EINVAL;
+   }
+
vpif_dev = >dev;
err =

Donation

2016-11-29 Thread Lopez Omar 
Hello, My name is Gloria C. Mackenzie, i have a Monetary Donation to make for 
less privilege and yourself and your organization, am writing you with a 
friend's email, please contact me on g_macken...@rogers.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[GIT PULL FOR v4.10] lirc cdev fixes for 4.10

2016-11-29 Thread Sean Young 
Hi Mauro,

Just one fix to prevent double free or NULL deref in error path.

Thanks,
Sean

The following changes since commit a000f0d3995f622410d433a01e94fbfb45969e27:

  [media] vivid: Set color_enc on HSV formats (2016-11-29 12:12:32 -0200)

are available in the git repository at:

  git://linuxtv.org/syoung/media_tree.git for-v4.10

for you to fetch changes up to 90a0560777a72ea91b0fb1147264614dd236853c:

  [media] lirc: fix error paths in lirc_cdev_add() (2016-11-29 20:05:25 +)


Sean Young (1):
  [media] lirc: fix error paths in lirc_cdev_add()

 drivers/media/rc/lirc_dev.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 3A / auto-exposure Region of Interest setting

2016-11-29 Thread Laurent Pinchart 
Hi Guennadi,

(CC'ing Sakari)

On Monday 28 Nov 2016 15:18:03 Guennadi Liakhovetski wrote:
> Hi,
> 
> Has anyone already considered supporting 3A (e.g. auto-exposure) Region of
> Interest selection? In UVC this is the "Digital Region of Interest (ROI)
> Control." Android defines ANDROID_CONTROL_AE_REGIONS,
> ANDROID_CONTROL_AWB_REGIONS, ANDROID_CONTROL_AF_REGIONS. The UVC control
> defines just a single rectangle for all (supported) 3A functions. That
> could be implemented, defining a new selection target. However, Android
> allows arbitrary numbers of ROI rectangles with associated weights. Any
> ideas?

Selections could be used, possibly with an update to the API to allow indexing 
selections for a given target. We'd be missing weights though. Another option 
would be to use compound controls.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] media: protect enable and disable source handler checks and calls

2016-11-29 Thread Shuah Khan 
On 11/29/2016 02:22 AM, Sakari Ailus wrote:
> Hi Shuah,
> 
> On Mon, Nov 28, 2016 at 07:15:14PM -0700, Shuah Khan wrote:
>> Protect enable and disable source handler checks and calls from dvb-core
>> and v4l2-core. Hold graph_mutex to check if enable and disable source
>> handlers are present and invoke them while holding the mutex. This change
>> ensures these handlers will not be removed while they are being checked
>> and invoked.
>>
>> au08282 enable and disable source handlers are changed to not hold the
>> graph_mutex.
>>
>> Signed-off-by: Shuah Khan 
>> ---
>>  drivers/media/dvb-core/dvb_frontend.c  | 24 ++--
>>  drivers/media/usb/au0828/au0828-core.c | 17 +
>>  drivers/media/v4l2-core/v4l2-mc.c  | 26 ++
>>  3 files changed, 41 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/media/dvb-core/dvb_frontend.c 
>> b/drivers/media/dvb-core/dvb_frontend.c
>> index 01511e5..2f09c7e 100644
>> --- a/drivers/media/dvb-core/dvb_frontend.c
>> +++ b/drivers/media/dvb-core/dvb_frontend.c
>> @@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, 
>> struct file *file)
>>  fepriv->voltage = -1;
>>  
>>  #ifdef CONFIG_MEDIA_CONTROLLER_DVB
>> -if (fe->dvb->mdev && fe->dvb->mdev->enable_source) {
>> -ret = fe->dvb->mdev->enable_source(dvbdev->entity,
>> +if (fe->dvb->mdev) {
>> +mutex_lock(>dvb->mdev->graph_mutex);
>> +if (fe->dvb->mdev->enable_source)
>> +ret = fe->dvb->mdev->enable_source(
>> +   dvbdev->entity,
>> >pipe);
>> +mutex_unlock(>dvb->mdev->graph_mutex);
> 
> You have to make sure the media device actually will stay aronud while it is
> being accessed. In this case, when dvb_frontend_open() runs, it will proceed
> to access the media device without knowing whether it's going to stay around
> or not. Without doing so, it may well be in the process of being removed by
> au0828_unregister_media_device() at the same time.

Right. What this is trying to protect is just the check for enable_source
and disable handlers before calling them.

> 
> The approach I took in my patchset was that the device that requires the
> media device will acquire a reference to it, this way the media device will
> stick around as long as other data structures have references to it. The
> current set did not yet implement this to dvb devices but I can add that.
> Then there's no even a need for the frontend driver to acquire the graph
> lock just to call the enable_source() callback.

Taking reference to media_device alone will not solve this problem of enable
and disable handlers going away. au0828_unregister_media_device() will clear
the handlers and then call media_device_unregister() and it also does
media_device_cleanup(). Your patch set and media dev allocator api I did solve
the problem of media_device not going away, however, it doesn't fix this race
where callers of enable and disable source handlers checking for them and 
calling
them while the driver might be clearing them.

So here is the scenario these patches fix. Say user app starts
and during start of video streaming v4l2 checks to see if enable
source handler is defined. This check is done without holding the
graph_mutex. If unbind happens to be in progress, au0828 could
clear enable and disable source handlers. So these could race.
I am not how large this window is, but could happen.

If graph_mutex protects the check for enable source handler not
being null, then it has to be released before calling enable source
handler as shown below:

if (mdev) {
mutex_lock(>graph_mutex);
if (mdev->disable_source) {
mutex_unlock(>graph_mutex);
mdev->disable_source(>entity);
} else
mutex_unlock(>graph_mutex);
}

The above will leave another window for handlers to be cleared.
That is why it would make sense for the caller to hold the lock
and the call enable and disable source handlers.

We do need a way to protect enable and disable handler access and the
call itself. I am using the same graph_mutex for both, hence I decided
to have the caller hold the lock.

Hope this helps.

thanks,
-- Shuah

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] adv7604: Initialize drive strength to default when using DT

2016-11-29 Thread Laurent Pinchart 
Hi Lars,

Thank you for the patch.

On Tuesday 29 Nov 2016 12:23:48 Lars-Peter Clausen wrote:
> The adv7604 driver platform data contains fields for configuring the drive
> strength of the output pins. When probing the driver through DT these
> fields are not explicitly initialized, which means they are left at 0. This
> is a reserved setting for the drive strength configuration though and can
> cause signal integrity issues.
> 
> Whether these signal integrity issues are visible depends on the PCB
> specifics (e.g. the higher the load capacitance for the output the more
> visible the issue). But it has been observed on existing solutions at high
> pixel clock rates.
> 
> Initialize the drive strength settings to the power-on-reset value of the
> device when probing through devicetree to avoid this issue.
> 
> Fixes: 0e158be0162b ("adv7604: Add DT support")
> Signed-off-by: Lars-Peter Clausen 

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/i2c/adv7604.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 5630eb2..a4dc64a 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -3132,6 +3132,9 @@ static int adv76xx_parse_dt(struct adv76xx_state
> *state) state->pdata.blank_data = 1;
>   state->pdata.op_format_mode_sel = ADV7604_OP_FORMAT_MODE0;
>   state->pdata.bus_order = ADV7604_BUS_ORDER_RGB;
> + state->pdata.dr_str_data = ADV76XX_DR_STR_MEDIUM_HIGH;
> + state->pdata.dr_str_clk = ADV76XX_DR_STR_MEDIUM_HIGH;
> + state->pdata.dr_str_sync = ADV76XX_DR_STR_MEDIUM_HIGH;
> 
>   return 0;
>  }

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2.1 1/2] smiapp: Implement power-on and power-off sequences without runtime PM

2016-11-29 Thread Laurent Pinchart 
Hi Sakari,

Thank you for the patch.

On Tuesday 29 Nov 2016 19:28:53 Sakari Ailus wrote:
> Power on the sensor when the module is loaded and power it off when it is
> removed.
> 
> Signed-off-by: Sakari Ailus 
> ---
> Hi Alan and Laurent,
> 
> I hope this should be good then. I'm only enabling runtime PM at the end
> of probe() when all is well, which reduces need for error handling.
> 
> Regards,
> Sakari
> 
>  drivers/media/i2c/smiapp/smiapp-core.c | 28 +---
>  1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> b/drivers/media/i2c/smiapp/smiapp-core.c index 59872b3..683a3e0 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -2741,8 +2741,6 @@ static const struct v4l2_subdev_internal_ops
> smiapp_internal_ops = { * I2C Driver
>   */
> 
> -#ifdef CONFIG_PM
> -
>  static int smiapp_suspend(struct device *dev)
>  {
>   struct i2c_client *client = to_i2c_client(dev);
> @@ -2783,13 +2781,6 @@ static int smiapp_resume(struct device *dev)
>   return rval;
>  }
> 
> -#else
> -
> -#define smiapp_suspend   NULL
> -#define smiapp_resumeNULL
> -
> -#endif /* CONFIG_PM */
> -
>  static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
>  {
>   struct smiapp_hwconfig *hwcfg;
> @@ -2913,13 +2904,9 @@ static int smiapp_probe(struct i2c_client *client,
>   if (IS_ERR(sensor->xshutdown))
>   return PTR_ERR(sensor->xshutdown);
> 
> - pm_runtime_enable(>dev);
> -
> - rval = pm_runtime_get_sync(>dev);
> - if (rval < 0) {
> - rval = -ENODEV;
> - goto out_power_off;
> - }
> + rval = smiapp_power_on(>dev);
> + if (rval < 0)
> + return rval;
> 
>   rval = smiapp_identify_module(sensor);
>   if (rval) {
> @@ -3100,6 +3087,9 @@ static int smiapp_probe(struct i2c_client *client,
>   if (rval < 0)
>   goto out_media_entity_cleanup;
> 
> + pm_runtime_set_active(>dev);
> + pm_runtime_get_noresume(>dev);
> + pm_runtime_enable(>dev);
>   pm_runtime_set_autosuspend_delay(>dev, 1000);
>   pm_runtime_use_autosuspend(>dev);
>   pm_runtime_put_autosuspend(>dev);

This looks better to me, although these 6 lines really call for a new helper 
function.

However, I still believe a helper that calls the runtime PM handlers directly 
when CONFIG_PM=n and rely on runtime PM when CONFIG_PM=y would be the cleanest 
solution from a driver point of view.

> @@ -3113,8 +3103,7 @@ static int smiapp_probe(struct i2c_client *client,
>   smiapp_cleanup(sensor);
> 
>  out_power_off:
> - pm_runtime_put(>dev);
> - pm_runtime_disable(>dev);
> + smiapp_power_off(>dev);
> 
>   return rval;
>  }
> @@ -3127,8 +3116,9 @@ static int smiapp_remove(struct i2c_client *client)
> 
>   v4l2_async_unregister_subdev(subdev);
> 
> - pm_runtime_suspend(>dev);
>   pm_runtime_disable(>dev);
> + pm_runtime_set_suspended(>dev);
> + smiapp_power_off(>dev);

The device could be powered off already.

> 
>   for (i = 0; i < sensor->ssds_used; i++) {
>   v4l2_device_unregister_subdev(>ssds[i].sd);

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2.1 1/2] smiapp: Implement power-on and power-off sequences without runtime PM

2016-11-29 Thread Sakari Ailus 
Power on the sensor when the module is loaded and power it off when it is
removed.

Signed-off-by: Sakari Ailus 
---
Hi Alan and Laurent,

I hope this should be good then. I'm only enabling runtime PM at the end
of probe() when all is well, which reduces need for error handling.

Regards,
Sakari

 drivers/media/i2c/smiapp/smiapp-core.c | 28 +---
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
b/drivers/media/i2c/smiapp/smiapp-core.c
index 59872b3..683a3e0 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2741,8 +2741,6 @@ static const struct v4l2_subdev_internal_ops 
smiapp_internal_ops = {
  * I2C Driver
  */
 
-#ifdef CONFIG_PM
-
 static int smiapp_suspend(struct device *dev)
 {
struct i2c_client *client = to_i2c_client(dev);
@@ -2783,13 +2781,6 @@ static int smiapp_resume(struct device *dev)
return rval;
 }
 
-#else
-
-#define smiapp_suspend NULL
-#define smiapp_resume  NULL
-
-#endif /* CONFIG_PM */
-
 static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
 {
struct smiapp_hwconfig *hwcfg;
@@ -2913,13 +2904,9 @@ static int smiapp_probe(struct i2c_client *client,
if (IS_ERR(sensor->xshutdown))
return PTR_ERR(sensor->xshutdown);
 
-   pm_runtime_enable(>dev);
-
-   rval = pm_runtime_get_sync(>dev);
-   if (rval < 0) {
-   rval = -ENODEV;
-   goto out_power_off;
-   }
+   rval = smiapp_power_on(>dev);
+   if (rval < 0)
+   return rval;
 
rval = smiapp_identify_module(sensor);
if (rval) {
@@ -3100,6 +3087,9 @@ static int smiapp_probe(struct i2c_client *client,
if (rval < 0)
goto out_media_entity_cleanup;
 
+   pm_runtime_set_active(>dev);
+   pm_runtime_get_noresume(>dev);
+   pm_runtime_enable(>dev);
pm_runtime_set_autosuspend_delay(>dev, 1000);
pm_runtime_use_autosuspend(>dev);
pm_runtime_put_autosuspend(>dev);
@@ -3113,8 +3103,7 @@ static int smiapp_probe(struct i2c_client *client,
smiapp_cleanup(sensor);
 
 out_power_off:
-   pm_runtime_put(>dev);
-   pm_runtime_disable(>dev);
+   smiapp_power_off(>dev);
 
return rval;
 }
@@ -3127,8 +3116,9 @@ static int smiapp_remove(struct i2c_client *client)
 
v4l2_async_unregister_subdev(subdev);
 
-   pm_runtime_suspend(>dev);
pm_runtime_disable(>dev);
+   pm_runtime_set_suspended(>dev);
+   smiapp_power_off(>dev);
 
for (i = 0; i < sensor->ssds_used; i++) {
v4l2_device_unregister_subdev(>ssds[i].sd);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] media protect enable and disable source handler paths

2016-11-29 Thread Mauro Carvalho Chehab 
Em Tue, 29 Nov 2016 10:07:21 -0700
Shuah Khan  escreveu:

> On 11/29/2016 02:15 AM, Mauro Carvalho Chehab wrote:
> > Em Mon, 28 Nov 2016 19:15:12 -0700
> > Shuah Khan  escreveu:
> >   
> >> These two patches fix enable and disable source handler paths. These
> >> aren't dependent patches, grouped because they fix similar problems.  
> > 
> > Those two patches should be fold, as applying just the first patch
> > would cause au0828 to try to double lock.
> >   
> 
> No it doesn't. The first patch holds the lock to just clear and set
> enable disable source handlers and doesn't change any other paths.
> The second patch removes lock hold from enable and disable source
> handlers and the callers to hold the lock.
> 
> However, I can easily fold them together and not a problem.
> 
> >>
> >> This work is triggered by a review comment from Mauro Chehab on a
> >> snd_usb_audio patch about protecting the enable and disabel handler
> >> path in it.
> >>
> >> Ran tests to make sure enable and disable handler paths work. When
> >> digital stream is active, analog app finds the tuner busy and vice
> >> versa. Also ran the Sakari's unbind while video stream is active test.  
> > 
> > Sorry, but your patches descriptions don't make things clear:  
> 
> Right. I should have explained it better.
> 
> > 
> > - It doesn't present any OOPS or logs that would help to
> >   understand what you're trying to fix;
> > 
> > - From what I understood, you're moving the lock out of
> >   enable/disable handlers, and letting their callers to do
> >   the locks themselves. Why? Are there any condition where it
> >   won't need to be locked?  
> 
> So here is the scenario these patches fix. Say user app starts
> and during start of video streaming v4l2 checks to see if enable
> source handler is defined. This check is done without holding the
> graph_mutex.

Why? You need to hold the graph_mutex before navigating at the
media graph, or to convert MC to use a lockless protection (like 
RCU or restartable sequence).

> If unbind happens to be in progress, au0828 could
> clear enable and disable source handlers. So these could race.
> I am not how large this window is, but could happen.
> 
> If graph_mutex protects the check for enable source handler not
> being null, then it has to be released before calling enable source
> handler as shown below:
> 
> if (mdev) {
>   mutex_lock(>graph_mutex);
>   if (mdev->disable_source) {
>   mutex_unlock(>graph_mutex);
>   mdev->disable_source(>entity);
>   } else
>   mutex_unlock(>graph_mutex);
> }
> 
> The above will leave another window for handlers to be cleared.
> That is why it would make sense for the caller to hold the lock
> and the call enable and disable source handlers.

I see. Please add such explanation at the patch description,
showing how it should happen, instead.
> 
> > 
> > - It is not touching documentation. If now the callbacks should
> >   not implement locks, this should be explicitly described.  
> 
> Yes documentation needs to be updated and I can do that in v2 if
> we are okay with this approach.
> 
> > 
> > Btw, I think it is a bad idea to let the callers to handle
> > the locks. The best would be, instead, to change the code in
> > some other way to avoid it, if possible. If not possible at all,
> > clearly describe why it is not possible and insert some comments
> > inside the code, to avoid some cleanup patch to mess up with this.
> >   
> 
> Hope the above explanation helps answer the question. We do need a
> way to protect enable and disable handler access and the call itself.
> I am using the same graph_mutex for both, hence I decided to have the
> caller hold the lock. Any other ideas welcome.
> 
> thanks,
> -- Shuah
> 



Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] media protect enable and disable source handler paths

2016-11-29 Thread Shuah Khan 
On 11/29/2016 02:15 AM, Mauro Carvalho Chehab wrote:
> Em Mon, 28 Nov 2016 19:15:12 -0700
> Shuah Khan  escreveu:
> 
>> These two patches fix enable and disable source handler paths. These
>> aren't dependent patches, grouped because they fix similar problems.
> 
> Those two patches should be fold, as applying just the first patch
> would cause au0828 to try to double lock.
> 

No it doesn't. The first patch holds the lock to just clear and set
enable disable source handlers and doesn't change any other paths.
The second patch removes lock hold from enable and disable source
handlers and the callers to hold the lock.

However, I can easily fold them together and not a problem.

>>
>> This work is triggered by a review comment from Mauro Chehab on a
>> snd_usb_audio patch about protecting the enable and disabel handler
>> path in it.
>>
>> Ran tests to make sure enable and disable handler paths work. When
>> digital stream is active, analog app finds the tuner busy and vice
>> versa. Also ran the Sakari's unbind while video stream is active test.
> 
> Sorry, but your patches descriptions don't make things clear:

Right. I should have explained it better.

> 
> - It doesn't present any OOPS or logs that would help to
>   understand what you're trying to fix;
> 
> - From what I understood, you're moving the lock out of
>   enable/disable handlers, and letting their callers to do
>   the locks themselves. Why? Are there any condition where it
>   won't need to be locked?

So here is the scenario these patches fix. Say user app starts
and during start of video streaming v4l2 checks to see if enable
source handler is defined. This check is done without holding the
graph_mutex. If unbind happens to be in progress, au0828 could
clear enable and disable source handlers. So these could race.
I am not how large this window is, but could happen.

If graph_mutex protects the check for enable source handler not
being null, then it has to be released before calling enable source
handler as shown below:

if (mdev) {
mutex_lock(>graph_mutex);
if (mdev->disable_source) {
mutex_unlock(>graph_mutex);
mdev->disable_source(>entity);
} else
mutex_unlock(>graph_mutex);
}

The above will leave another window for handlers to be cleared.
That is why it would make sense for the caller to hold the lock
and the call enable and disable source handlers.

> 
> - It is not touching documentation. If now the callbacks should
>   not implement locks, this should be explicitly described.

Yes documentation needs to be updated and I can do that in v2 if
we are okay with this approach.

> 
> Btw, I think it is a bad idea to let the callers to handle
> the locks. The best would be, instead, to change the code in
> some other way to avoid it, if possible. If not possible at all,
> clearly describe why it is not possible and insert some comments
> inside the code, to avoid some cleanup patch to mess up with this.
> 

Hope the above explanation helps answer the question. We do need a
way to protect enable and disable handler access and the call itself.
I am using the same graph_mutex for both, hence I decided to have the
caller hold the lock. Any other ideas welcome.

thanks,
-- Shuah

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] [media] dt-bindings: add TI VPIF documentation

2016-11-29 Thread Rob Herring 
On Mon, Nov 28, 2016 at 4:30 PM, Kevin Hilman  wrote:
> Hi Rob,
>
> Rob Herring  writes:
>
>> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
>>> Signed-off-by: Kevin Hilman 
>>> ---
>>>  .../bindings/media/ti,da850-vpif-capture.txt   | 65 
>>> ++
>>>  .../devicetree/bindings/media/ti,da850-vpif.txt|  8 +++
>>>  2 files changed, 73 insertions(+)
>>>  create mode 100644 
>>> Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>>>  create mode 100644 
>>> Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt 
>>> b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>>> new file mode 100644
>>> index ..c447ac482c1d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>>> @@ -0,0 +1,65 @@
>>> +Texas Instruments VPIF Capture
>>> +--
>>> +
>>> +The TI Video Port InterFace (VPIF) capture component is the primary
>>> +component for video capture on the DA850 family of TI DaVinci SoCs.
>>> +
>>> +TI Document number reference: SPRUH82C
>>> +
>>> +Required properties:
>>> +- compatible: must be "ti,da850-vpif-capture"
>>> +- reg: physical base address and length of the registers set for the 
>>> device;
>>> +- interrupts: should contain IRQ line for the VPIF
>>> +
>>> +VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
>>> +channels or a single 16-bit channel.  It should contain at least one
>>> +port child node with child 'endpoint' node. Please refer to the
>>> +bindings defined in
>>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>>> +
>>> +Example using 2 8-bit input channels, one of which is connected to an
>>> +I2C-connected TVP5147 decoder:
>>> +
>>> +vpif_capture: video-capture@0x00217000 {
>>> +reg = <0x00217000 0x1000>;
>>> +interrupts = <92>;
>>> +
>>> +port {
>>> +vpif_ch0: endpoint@0 {
>>> +  reg = <0>;
>>> +  bus-width = <8>;
>>> +  remote-endpoint = <>;
>>> +};
>>> +
>>> +vpif_ch1: endpoint@1 {
>>
>> I think probably channels here should be ports rather than endpoints.
>> AIUI, having multiple endpoints is for cases like a mux or 1 to many
>> connections. There's only one data flow, but multiple sources or sinks.
>
> Looking at this closer... , I used an endpoint because it's bascially a
> 16-bit parallel bus, that can be configured as (up to) 2 8-bit
> "channels.  So, based on the video-interfaces.txt doc, I configured this
> as a single port, with (up to) 2 endpoints.  That also allows me to
> connect output of the decoder directly, using the remote-endpoint
> property.
>
> So I guess I'm not fully understanding your suggestion.

NM, looks like video-interfaces.txt actually spells out this case and
defines doing it as you did.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [media] adv7604: Initialize drive strength to default when using DT

2016-11-29 Thread Lars-Peter Clausen 
The adv7604 driver platform data contains fields for configuring the drive
strength of the output pins. When probing the driver through DT these
fields are not explicitly initialized, which means they are left at 0. This
is a reserved setting for the drive strength configuration though and can
cause signal integrity issues.

Whether these signal integrity issues are visible depends on the PCB
specifics (e.g. the higher the load capacitance for the output the more
visible the issue). But it has been observed on existing solutions at high
pixel clock rates.

Initialize the drive strength settings to the power-on-reset value of the
device when probing through devicetree to avoid this issue.

Fixes: 0e158be0162b ("adv7604: Add DT support")
Signed-off-by: Lars-Peter Clausen 
---
 drivers/media/i2c/adv7604.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 5630eb2..a4dc64a 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -3132,6 +3132,9 @@ static int adv76xx_parse_dt(struct adv76xx_state *state)
state->pdata.blank_data = 1;
state->pdata.op_format_mode_sel = ADV7604_OP_FORMAT_MODE0;
state->pdata.bus_order = ADV7604_BUS_ORDER_RGB;
+   state->pdata.dr_str_data = ADV76XX_DR_STR_MEDIUM_HIGH;
+   state->pdata.dr_str_clk = ADV76XX_DR_STR_MEDIUM_HIGH;
+   state->pdata.dr_str_sync = ADV76XX_DR_STR_MEDIUM_HIGH;
 
return 0;
 }
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v3 00/21] Make use of kref in media device, grab references as needed

2016-11-29 Thread Mauro Carvalho Chehab 
Hi Sakari,

I answered you point to point below, but I suspect that you missed how the 
current approach works. So, I decided to write a quick summary here.

The character devices /dev/media? are created via cdev, with relies on a 
kobject per device, with has an embedded struct kref inside.

Also, each kobj at /dev/media0, /dev/media1, ... is associated with a
struct device, when the code does:
devnode->cdev.kobj.parent = >dev.kobj;

before calling cdev_add().

The current lifetime management is actually based on cdev's kobject's
refcount, provided by its embedded kref.

The kref warrants that any data associated with /dev/media0 won't be 
freed if there are any pending system call. In other words, when 
cdev_del() is called, it will remove /dev/media0 from the filesystem, and
will call kobject_put(). 

If the refcount is zero, it will call devnode->dev.release(). If the 
kobject refcount is not zero, the data won't be freed.

So, in the best case scenario, there's no opened file descriptors
by the time media device node is unregistered. So, it will free
everything.

In the worse case scenario, e. g. when the driver is removed or 
unbind while /dev/media0 has some opened file descriptor(s),
the cdev logic will do the proper lifetime management.

On such case, /dev/media0 disappears from the file system, so another open
is not possible anymore. The data structures will remain allocated until
all associated file descriptors are not closed.

When all file descriptors are closed, the data will be freed.

On that time, it will call an optional dev.release() callback,
responsible to free any other data struct that the driver allocated.

Em Mon, 28 Nov 2016 12:45:56 +0200
Sakari Ailus  escreveu:

> Hi Mauro,
> 
> On Tue, Nov 22, 2016 at 03:44:29PM -0200, Mauro Carvalho Chehab wrote:
> > Em Mon, 14 Nov 2016 15:27:22 +0200
> > Sakari Ailus  escreveu:
> >   
> > > Hi Mauro,
> > > 
> > > I'm replying below but let me first summarise the remaining problem area
> > > that this patchset addresses.  
> > 
> > Sorry for answering too late. Somehow, I missed this email in the cloud.
> >   
> > > The problems you and Shuah have seen and partially addressed are related 
> > > to
> > > a larger picture which is the lifetime of (mostly) memory resources 
> > > related
> > > to various objects used by as well both the Media controller and V4L2
> > > frameworks (including videobuf2) as the drivers which make use of these
> > > frameworks.
> > > 
> > > The Media controller and V4L2 interfaces exposed by drivers consist of
> > > multiple devices nodes, data structures with interdependencies within the
> > > frameworks themselves and dependencies from the driver's own data 
> > > structures
> > > towards the framework data structures. The Media device and the media 
> > > graph
> > > objects are central to the problem area as well.
> > > 
> > > So what are the issues then? Until now, we've attempted to regulate the
> > > users' ability to access the devices at the time they're being 
> > > unregistered
> > > (and the associated memory released), but that approach does not really
> > > scale: you have to make sure that the unregistering also will not take 
> > > place
> > > _during_ the system call --- not just in the beginning of it.
> > >
> > > The media graph contains media graph objects, some of which are media
> > > entities (contained in struct video_device or struct v4l2_subdev, for
> > > instance). Media entities as graph nodes have links to other entities. In
> > > order to implement the system calls, the drivers do parse this graph in
> > > order to obtain information they need to obtain from it. For instance, 
> > > it's
> > > not uncommon for an implementation for video node format enumeration to
> > > figure out which sub-device the link from that video nodes leads to. 
> > > Drivers
> > > may also have similar paths they follow.
> > > 
> > > Interrupt handling may also be taking place during the device removal 
> > > during
> > > which a number of data structures are now freed. This really does call 
> > > for a
> > > solution based on reference counting.
> > > 
> > > This leads to the conclusion that all the memory resources that could be
> > > accessed by the drivers or the kernel frameworks must stay intact until 
> > > the
> > > last file handle to the said devices is closed. Otherwise, there is a
> > > possibility of accessing released memory.  
> > 
> > So far, we're aligned.
> >   
> > > Right now in a lot of the cases, such as for video device and sub-device
> > > nodes, we do release the memory when a device (as in struct device) is 
> > > being
> > > unregistered. There simply is in the current mainline kernel a way to do
> > > this in a safe way.  
> >   
> > > Drivers do use devm_() family of functions to allocate
> > > the memory of the media graph object and their internal data structures.  
> > 
> > Removing devm_() from those drivers seem to be the

Re: [PATCH] v4l: async: make v4l2 coexists with devicetree nodes in a dt overlay

2016-11-29 Thread Javi Merino 
On Fri, Nov 25, 2016 at 10:21:21AM +0200, Sakari Ailus wrote:
Hi Sakari,

> On Wed, Nov 23, 2016 at 04:15:11PM +, Javi Merino wrote:
> > On Wed, Nov 23, 2016 at 05:10:42PM +0200, Sakari Ailus wrote:
> > > Hi Javi,
> > 
> > Hi Sakari,
> > 
> > > On Wed, Nov 23, 2016 at 10:09:57AM +, Javi Merino wrote:
> > > > In asd's configured with V4L2_ASYNC_MATCH_OF, if the v4l2 subdev is in
> > > > a devicetree overlay, its of_node pointer will be different each time
> > > > the overlay is applied.  We are not interested in matching the
> > > > pointer, what we want to match is that the path is the one we are
> > > > expecting.  Change to use of_node_cmp() so that we continue matching
> > > > after the overlay has been removed and reapplied.
> > > > 
> > > > Cc: Mauro Carvalho Chehab 
> > > > Cc: Javier Martinez Canillas 
> > > > Cc: Sakari Ailus 
> > > > Signed-off-by: Javi Merino 
> > > > ---
> > > > Hi,
> > > > 
> > > > I feel it is a bit of a hack, but I couldn't think of anything better.
> > > > I'm ccing devicetree@ and Pantelis because there may be a simpler
> > > > solution.
> > > 
> > > First I have to admit that I'm not an expert when it comes to DT overlays.
> > > 
> > > That said, my understanding is that the sub-device and the async 
> > > sub-device
> > > are supposed to point to the exactly same DT node. I wonder if there's
> > > actually anything wrong in the current code.
> > > 
> > > If the overlay has changed between probing the driver for the async 
> > > notifier
> > > and the async sub-device, there should be no match here, should there? The
> > > two nodes actually point to a node in a different overlay in that case.
> > 
> > Overlays are parts of the devicetree that can be added and removed.
> > When the overlay is applied, the camera driver is probed and does
> > v4l2_async_register_subdev().  However, v4l2_async_belongs() fails.
> > The problem is with comparing pointers.  I haven't looked at the
> > implementation of overlays in detail, but what I see is that the
> > of_node pointer changes when you remove and reapply an overlay (I
> > guess it's dynamically allocated and when you remove the overlay, it's
> > freed).
> 
> The concern here which we were discussing was whether the overlay should be
> relied on having compliant configuration compared to the part which was not
> part of the overlay.
> 
> As external components are involved, quite possibly also the ISP DT node
> will require changes, not just the image source (TV tuner, camera sensor
> etc.). This could be because of number of CSI-2 lanes or parallel bus width,
> for instance.
> 
> I'd also be interested in having an actual driver implement support for
> removing and adding a DT overlay first so we'd see how this would actually
> work. We need both in order to be able to actually remove and add DT
> overlays _without_ unbinding the ISP driver. Otherwise it should already
> work in the current codebase.

Unfortunately, the driver I'm working on is not upstream and I can't
submit it to mainline.  This patch fixes the issue for me, so I
thought it could be useful fix for the kernel.

Cheers,
Javi

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] media: protect enable and disable source handler checks and calls

2016-11-29 Thread Sakari Ailus 
Hi Shuah,

On Mon, Nov 28, 2016 at 07:15:14PM -0700, Shuah Khan wrote:
> Protect enable and disable source handler checks and calls from dvb-core
> and v4l2-core. Hold graph_mutex to check if enable and disable source
> handlers are present and invoke them while holding the mutex. This change
> ensures these handlers will not be removed while they are being checked
> and invoked.
> 
> au08282 enable and disable source handlers are changed to not hold the
> graph_mutex.
> 
> Signed-off-by: Shuah Khan 
> ---
>  drivers/media/dvb-core/dvb_frontend.c  | 24 ++--
>  drivers/media/usb/au0828/au0828-core.c | 17 +
>  drivers/media/v4l2-core/v4l2-mc.c  | 26 ++
>  3 files changed, 41 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c 
> b/drivers/media/dvb-core/dvb_frontend.c
> index 01511e5..2f09c7e 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, 
> struct file *file)
>   fepriv->voltage = -1;
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER_DVB
> - if (fe->dvb->mdev && fe->dvb->mdev->enable_source) {
> - ret = fe->dvb->mdev->enable_source(dvbdev->entity,
> + if (fe->dvb->mdev) {
> + mutex_lock(>dvb->mdev->graph_mutex);
> + if (fe->dvb->mdev->enable_source)
> + ret = fe->dvb->mdev->enable_source(
> +dvbdev->entity,
>  >pipe);
> + mutex_unlock(>dvb->mdev->graph_mutex);

You have to make sure the media device actually will stay aronud while it is
being accessed. In this case, when dvb_frontend_open() runs, it will proceed
to access the media device without knowing whether it's going to stay around
or not. Without doing so, it may well be in the process of being removed by
au0828_unregister_media_device() at the same time.

The approach I took in my patchset was that the device that requires the
media device will acquire a reference to it, this way the media device will
stick around as long as other data structures have references to it. The
current set did not yet implement this to dvb devices but I can add that.
Then there's no even a need for the frontend driver to acquire the graph
lock just to call the enable_source() callback.

>   if (ret) {
>   dev_err(fe->dvb->device,
>   "Tuner is busy. Error %d\n", ret);
> @@ -2553,8 +2557,12 @@ static int dvb_frontend_open(struct inode *inode, 
> struct file *file)
>  
>  err3:
>  #ifdef CONFIG_MEDIA_CONTROLLER_DVB
> - if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
> - fe->dvb->mdev->disable_source(dvbdev->entity);
> + if (fe->dvb->mdev) {
> + mutex_lock(>dvb->mdev->graph_mutex);
> + if (fe->dvb->mdev->disable_source)
> + fe->dvb->mdev->disable_source(dvbdev->entity);
> + mutex_unlock(>dvb->mdev->graph_mutex);
> + }
>  err2:
>  #endif
>   dvb_generic_release(inode, file);
> @@ -2586,8 +2594,12 @@ static int dvb_frontend_release(struct inode *inode, 
> struct file *file)
>   if (dvbdev->users == -1) {
>   wake_up(>wait_queue);
>  #ifdef CONFIG_MEDIA_CONTROLLER_DVB
> - if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
> - fe->dvb->mdev->disable_source(dvbdev->entity);
> + if (fe->dvb->mdev) {
> + mutex_lock(>dvb->mdev->graph_mutex);
> + if (fe->dvb->mdev->disable_source)
> + fe->dvb->mdev->disable_source(dvbdev->entity);
> + mutex_unlock(>dvb->mdev->graph_mutex);
> + }
>  #endif
>   if (fe->exit != DVB_FE_NO_EXIT)
>   wake_up(>wait_queue);
> diff --git a/drivers/media/usb/au0828/au0828-core.c 
> b/drivers/media/usb/au0828/au0828-core.c
> index a1f696a..bfd6482 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -280,6 +280,7 @@ static void au0828_media_graph_notify(struct media_entity 
> *new,
>   }
>  }
>  
> +/* Callers should hold graph_mutex */
>  static int au0828_enable_source(struct media_entity *entity,
>   struct media_pipeline *pipe)
>  {
> @@ -293,8 +294,6 @@ static int au0828_enable_source(struct media_entity 
> *entity,
>   if (!mdev)
>   return -ENODEV;
>  
> - mutex_lock(>graph_mutex);
> -
>   dev = mdev->source_priv;
>  
>   /*
> @@ -421,12 +420,12 @@ static int au0828_enable_source(struct media_entity 
> *entity,
>dev->active_source->name, dev->active_sink->name,
>

Re: [PATCH 0/2] media protect enable and disable source handler paths

2016-11-29 Thread Mauro Carvalho Chehab 
Em Mon, 28 Nov 2016 19:15:12 -0700
Shuah Khan  escreveu:

> These two patches fix enable and disable source handler paths. These
> aren't dependent patches, grouped because they fix similar problems.

Those two patches should be fold, as applying just the first patch
would cause au0828 to try to double lock.

> 
> This work is triggered by a review comment from Mauro Chehab on a
> snd_usb_audio patch about protecting the enable and disabel handler
> path in it.
> 
> Ran tests to make sure enable and disable handler paths work. When
> digital stream is active, analog app finds the tuner busy and vice
> versa. Also ran the Sakari's unbind while video stream is active test.

Sorry, but your patches descriptions don't make things clear:

- It doesn't present any OOPS or logs that would help to
  understand what you're trying to fix;

- From what I understood, you're moving the lock out of
  enable/disable handlers, and letting their callers to do
  the locks themselves. Why? Are there any condition where it
  won't need to be locked?

- It is not touching documentation. If now the callbacks should
  not implement locks, this should be explicitly described.

Btw, I think it is a bad idea to let the callers to handle
the locks. The best would be, instead, to change the code in
some other way to avoid it, if possible. If not possible at all,
clearly describe why it is not possible and insert some comments
inside the code, to avoid some cleanup patch to mess up with this.

Regards

Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html