cron job: media_tree daily build: ERRORS

[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:   Thu Dec  8 05:00:16 CET 2016
media-tree git hash:365fe4e0ce218dc5ad10df17b150a366b6015499
media_build git hash:   1606032398b1d79149c1507be2029e1a00d8dff0
v4l-utils git hash: 188e604d57bec065078ff772c802b93ddb6def4b
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/Thursday.log

Full logs are available here:

http://www.xs4all.nl/~hverkuil/logs/Thursday.tar.bz2

The Media Infrastructure API from this daily build is here:

http://www.xs4all.nl/~hverkuil/spec/index.html
--
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 v5 2/2] Add support for OV5647 sensor

[Sakari Ailus]

Hi Ramiro,

On Mon, Dec 05, 2016 at 05:36:34PM +, Ramiro Oliveira wrote:
> Add support for OV5647 sensor.
> 
> Modes supported:
>  - 640x480 RAW 8
> 
> Signed-off-by: Ramiro Oliveira 
> ---
>  MAINTAINERS|   7 +
>  drivers/media/i2c/Kconfig  |  12 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/ov5647.c | 866 
> +
>  4 files changed, 886 insertions(+)
>  create mode 100644 drivers/media/i2c/ov5647.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 52cc077..72e828a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8923,6 +8923,13 @@ M: Harald Welte 
>  S:   Maintained
>  F:   drivers/char/pcmcia/cm4040_cs.*
>  
> +OMNIVISION OV5647 SENSOR DRIVER
> +M:   Ramiro Oliveira 
> +L:   linux-media@vger.kernel.org
> +T:   git git://linuxtv.org/media_tree.git
> +S:   Maintained
> +F:   drivers/media/i2c/ov5647.c
> +
>  OMNIVISION OV7670 SENSOR DRIVER
>  M:   Jonathan Corbet 
>  L:   linux-media@vger.kernel.org
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index b31fa6f..c1b78e5 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -531,6 +531,18 @@ config VIDEO_OV2659
> To compile this driver as a module, choose M here: the
> module will be called ov2659.
>  
> +config VIDEO_OV5647
> + tristate "OmniVision OV5647 sensor support"
> + depends on OF

How does this driver depend on OF, other than matching the compatible
string?

> + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> + depends on MEDIA_CAMERA_SUPPORT
> + ---help---
> +   This is a Video4Linux2 sensor-level driver for the OmniVision
> +   OV5647 camera.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called ov5647.
> +
>  config VIDEO_OV7640
>   tristate "OmniVision OV7640 sensor support"
>   depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 92773b2..0d9014c 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -82,3 +82,4 @@ obj-$(CONFIG_VIDEO_IR_I2C)  += ir-kbd-i2c.o
>  obj-$(CONFIG_VIDEO_ML86V7667)+= ml86v7667.o
>  obj-$(CONFIG_VIDEO_OV2659)   += ov2659.o
>  obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
> +obj-$(CONFIG_VIDEO_OV5647)   += ov5647.o
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> new file mode 100644
> index 000..2aae806
> --- /dev/null
> +++ b/drivers/media/i2c/ov5647.c
> @@ -0,0 +1,866 @@
> +/*
> + * A V4L2 driver for OmniVision OV5647 cameras.
> + *
> + * Based on Samsung S5K6AAFX SXGA 1/6" 1.3M CMOS Image Sensor driver
> + * Copyright (C) 2011 Sylwester Nawrocki 
> + *
> + * Based on Omnivision OV7670 Camera Driver
> + * Copyright (C) 2006-7 Jonathan Corbet 
> + *
> + * Copyright (C) 2016, Synopsys, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed .as is. WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; 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 

Alphabetical order, please.

> +
> +#define SENSOR_NAME "ov5647"
> +
> +#define OV5647_SW_RESET  0x1003
> +#define OV5647_REG_CHIPID_H  0x300A
> +#define OV5647_REG_CHIPID_L  0x300B
> +
> +#define REG_TERM 0xfffe
> +#define VAL_TERM 0xfe
> +#define REG_DLY  0x
> +
> +#define OV5647_ROW_START 0x01
> +#define OV5647_ROW_START_MIN 0
> +#define OV5647_ROW_START_MAX 2004
> +#define OV5647_ROW_START_DEF 54
> +
> +#define OV5647_COLUMN_START  0x02
> +#define OV5647_COLUMN_START_MIN  0
> +#define OV5647_COLUMN_START_MAX  2750
> +#define OV5647_COLUMN_START_DEF  16
> +
> +#define OV5647_WINDOW_HEIGHT 0x03
> +#define OV5647_WINDOW_HEIGHT_MIN 2
> +#define OV5647_WINDOW_HEIGHT_MAX 2006
> +#define OV5647_WINDOW_HEIGHT_DEF 1944
> +
> +#define OV5647_WINDOW_WIDTH  0x04
> +#define OV5647_WINDOW_WIDTH_MIN  2
> +#define OV5647_WINDOW_WIDTH_MAX  2752
> +#define OV5647_WINDOW_WIDTH_DEF  2592
> +
> +struct regval_list {
> + u16 addr;
> + u8 data;
> +};
> +
> +struct cfg_array {
> + struct regval_list *regs;
> + int size;
> +};
> +
> +struct sensor_win_size {
> + int width;
> + int height;
> + unsigned int hoffset;
> + unsigned int voffset;
> + unsigned int hts;

Re: Regression: tvp5150 refactoring breaks all em28xx devices

[Laurent Pinchart]

Hi Devin,

On Wednesday 07 Dec 2016 12:47:01 Devin Heitmueller wrote:
> Hello Javier, Mauro, Laurent,
> 
> I hope all is well with you.  Mauro, Laurent:  you guys going to
> ELC/Portland in February?

I haven't decided for sure yet, but I will likely go.

> Looks like the refactoring done to tvp5150 in January 2016 for
> s_stream() to support some embedded platform caused breakage in the
> 30+ em28xx products that also use the chip.

I assume you're talking about

commit 460b6c0831cb52ef349156cfa27e889606b4cb75
Author: Laurent Pinchart 
Date:   Thu Jan 7 10:46:45 2016 -0200

[media] tvp5150: Add s_stream subdev operation support

followed by

commit 47de9bf8931e6bf9c92fdba9867925d1ce482ab1
Author: Mauro Carvalho Chehab 
Date:   Mon Jan 25 14:39:34 2016 -0200

[media] tvp5150: Fix breakage for serial usage

both introduced in v4.6. I further assume that "serial" means BT.656 here, 
which is still parallel.

> Problem confirmed on both the Startech SVIDUSB2 board Steve Preston
> was nice enough to ship me (after adding a board profile), as well as
> on my original HVR-950 which has worked fine since 2008.
> 
> The implementation tramples the TVP5150_MISC_CTL register, blowing
> into it a hard-coded value based on one of two scenarios, neither of
> which matches what is expected by em28xx devices.  At least in the
> case of NTSC, this results in chroma cycling.  This was also reported
> by Alexandre-Xavier Labonté-Lamoureux back in August, although in the
> video below he's also having some other issue related to progressive
> video because he's using an old gaming console as the source (i.e. pay
> attention to the chroma effects in the top half of the video rather
> than the fact that only the first field is being rendered).
> 
> https://youtu.be/WLlqJ7T3y4g
> 
> The s_stream implementation writes 0x09 or 0x0d into TVP5150_MISC_CTL
> (overriding whatever was written by tvp5150_init_default and
> tvp5150_selmux().  In fact, just as a test I was able to start up
> video, see the corruption, and write the correct value back into the
> register via v4l2-dbg in order to get it working again:
> 
> sudo v4l2-dbg --chip=subdev0 --set-register=0x03 0x6f
> 
> There's no easy fix for this without extending the driver to support
> proper configuration of the output pin muxing, which it isn't clear to
> me what the right approach is and I don't have the embedded hardware
> platform that prompted the refactoring in order to do regression
> testing anyway.
> 
> Feel free to take it upon yourselves to fix the regression you introduced.

I've had a quick look at the code from the point of view opposite from yours, 
with my knowledge of the embedded side but without any experience with em28xx. 
I don't think that adding proper configuration of pinmuxing would be that 
hard, if it wasn't for the tvp5150_reset() function. The function is called 
directly in the get and set format handlers, and through subdev core 
operations.

The way we expose and use the reset operation is a very surprising (to stay 
politically correct) idea, but in the context of em28xx shouldn't be too much 
of a problem as the operation is only invoked at stream on time, before 
s_stream(1). However, calling it from the get and set format handlers can only 
lead me to conclude that the kernel is missing an ENOBRAIN error code. I'll 
blame it on history.

As a prerequisite to implement proper output mixing configuration, the 
tvp5150_reset() call needs to be removed from tvp5150_fill_fmt(). I can't test 
that with the em28xx driver as I don't have access to any such device. Devin, 
would you be able to assist with testing on em28xx by removing the function 
call from a working kernel (v4.5 would be ideal) and check if the device still 
operates correctly ? I believe it would, given that the reset operation is 
called at stream on time as well as explained above, and that call would still 
be there.

The tvp5150_reset() call in tvp5150_fill_fmt() was added by

commit ec2c4f3f93cb9ae2b09b8e942dd75ad3bdf23c9d
Author: Javier Martin 
Date:   Thu Jan 5 10:57:39 2012 -0300

[media] media: tvp5150: Add mbus_fmt callbacks

These callbacks allow a host video driver
to poll video formats supported by tvp5150.

Signed-off-by: Mauro Carvalho Chehab 

I assume the call was originally intended to put the device in a known state 
for a following call to tvp5150_read_std() in the same function. Given that 
that code got removed in the meantime, I don't see any need to reset the chip 
there. 

I'm not sure who added the code, as the commit is authored by Javier by only 
signed by Mauro. Could any (or both) of you shed some light on that ?

Mauro, as you've already attempted (unfortunately unsuccessfully) to fix this 
problem in 47de9bf8931e6bf9c92fdba9867925d1ce482ab1, do you plan to give it 
another try ? Now that I've performed 

Re: [PATCH v5 1/2] Add OV5647 device tree documentation

[Sakari Ailus]

Hi Ramiro,

Thank you for the patch.

On Mon, Dec 05, 2016 at 05:36:33PM +, Ramiro Oliveira wrote:
> Add device tree documentation.
> 
> Signed-off-by: Ramiro Oliveira 
> ---
>  .../devicetree/bindings/media/i2c/ov5647.txt  | 19 
> +++
>  1 file changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt 
> b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
> new file mode 100644
> index 000..4c91b3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
> @@ -0,0 +1,19 @@
> +Omnivision OV5647 raw image sensor
> +-
> +
> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces
> +and CCI (I2C compatible) control bus.
> +
> +Required properties:
> +
> +- compatible : "ovti,ov5647";
> +- reg: I2C slave address of the sensor;
> +
> +The common video interfaces bindings (see video-interfaces.txt) should be
> +used to specify link to the image data receiver. The OV5647 device
> +node should contain one 'port' child node with an 'endpoint' subnode.
> +
> +Following properties are valid for the endpoint node:
> +
> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
> +  video-interfaces.txt.  The sensor supports only two data lanes.

Doesn't this sensor require a external clock, a reset GPIO and / or a
regulator or a few? Do you need data-lanes, unless you can change the order
or the number?

An example DT snippet wouldn't hurt.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
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 v6 3/3] sound/usb: Use Media Controller API to share media resources

[Sakari Ailus]

Hi Shuah,

On Wed, Dec 07, 2016 at 01:03:59PM -0700, Shuah Khan wrote:
> Hi Sakari,
> 
> On 12/07/2016 03:52 AM, Sakari Ailus wrote:
> > Hi Shuah,
> > 
> > On Mon, Dec 05, 2016 at 05:38:23PM -0700, Shuah Khan wrote:
> >> On 12/05/2016 04:21 PM, Laurent Pinchart wrote:
> >>> Hi Shuah,
> >>>
> >>> On Monday 05 Dec 2016 15:44:30 Shuah Khan wrote:
>  On 11/30/2016 03:01 PM, Shuah Khan wrote:
> > Change ALSA driver to use Media Controller API to share media resources
> > with DVB, and V4L2 drivers on a AU0828 media device.
> >
> > Media Controller specific initialization is done after sound card is
> > registered. ALSA creates Media interface and entity function graph
> > nodes for Control, Mixer, PCM Playback, and PCM Capture devices.
> >
> > snd_usb_hw_params() will call Media Controller enable source handler
> > interface to request the media resource. If resource request is granted,
> > it will release it from snd_usb_hw_free(). If resource is busy, -EBUSY 
> > is
> > returned.
> >
> > Media specific cleanup is done in usb_audio_disconnect().
> >
> > Signed-off-by: Shuah Khan 
> 
>  Hi Takashi,
> 
>  If you are good with this patch, could you please Ack it, so Mauro
>  can pull it into media tree with the other two patches in this series,
>  when he is ready to do so.
> >>>
> >>> I *really* want to address the concerns raised by Sakari before pulling 
> >>> more 
> >>> code that makes fixing the race conditions more difficult. Please, let's 
> >>> all 
> >>> work on fixing the core code to build a stable base on which we can build 
> >>> additional features. V4L2 and MC need teamwork, it's time to give the 
> >>> subsystem the love it deserves.
> >>>
> >>
> >> Hi Laurent,
> >>
> >> The issue Sakari brought up is specific to using devm for video_device in
> >> omap3 and vsp1. I tried reproducing the problem on two different drivers
> >> and couldn't on Linux 4.9-rc7.
> >>
> >> After sharing that with Sakari, I suggested to Sakari to pull up his patch
> >> that removes the devm usage and see if he still needs all the patches in 
> >> his
> >> patch series. He didn't back to me on that. I also requested him to rebase 
> >> on
> > 
> > Just to see what remains, I made a small hack to test this with omap3isp by
> > just replacing the devm_() functions by their plain counterparts. The memory
> > is thus never released, for there is no really a proper moment to release it
> > --- something which the patchset resolves. The result is here:
> > 
> > 
> 
> Did you test this on 4.9-rc7 without any of your other patches? If you
> haven't could you please run this test with just the removing devm usage
> from omap3isp?
> 
> It would be good to get a baseline on the current with just the not using
> devm first and then see what needs fixing.
> 
> Also, could you please send me the complete dmesg.

Updated from v4.9-rc6 to rc7 and with increased CONFIG_LOG_BUF_SHIFT. The
diff and dmesg are here:




-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
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 v6 3/3] sound/usb: Use Media Controller API to share media resources

[Shuah Khan]

Hi Sakari,

On 12/07/2016 03:52 AM, Sakari Ailus wrote:
> Hi Shuah,
> 
> On Mon, Dec 05, 2016 at 05:38:23PM -0700, Shuah Khan wrote:
>> On 12/05/2016 04:21 PM, Laurent Pinchart wrote:
>>> Hi Shuah,
>>>
>>> On Monday 05 Dec 2016 15:44:30 Shuah Khan wrote:
 On 11/30/2016 03:01 PM, Shuah Khan wrote:
> Change ALSA driver to use Media Controller API to share media resources
> with DVB, and V4L2 drivers on a AU0828 media device.
>
> Media Controller specific initialization is done after sound card is
> registered. ALSA creates Media interface and entity function graph
> nodes for Control, Mixer, PCM Playback, and PCM Capture devices.
>
> snd_usb_hw_params() will call Media Controller enable source handler
> interface to request the media resource. If resource request is granted,
> it will release it from snd_usb_hw_free(). If resource is busy, -EBUSY is
> returned.
>
> Media specific cleanup is done in usb_audio_disconnect().
>
> Signed-off-by: Shuah Khan 

 Hi Takashi,

 If you are good with this patch, could you please Ack it, so Mauro
 can pull it into media tree with the other two patches in this series,
 when he is ready to do so.
>>>
>>> I *really* want to address the concerns raised by Sakari before pulling 
>>> more 
>>> code that makes fixing the race conditions more difficult. Please, let's 
>>> all 
>>> work on fixing the core code to build a stable base on which we can build 
>>> additional features. V4L2 and MC need teamwork, it's time to give the 
>>> subsystem the love it deserves.
>>>
>>
>> Hi Laurent,
>>
>> The issue Sakari brought up is specific to using devm for video_device in
>> omap3 and vsp1. I tried reproducing the problem on two different drivers
>> and couldn't on Linux 4.9-rc7.
>>
>> After sharing that with Sakari, I suggested to Sakari to pull up his patch
>> that removes the devm usage and see if he still needs all the patches in his
>> patch series. He didn't back to me on that. I also requested him to rebase on
> 
> Just to see what remains, I made a small hack to test this with omap3isp by
> just replacing the devm_() functions by their plain counterparts. The memory
> is thus never released, for there is no really a proper moment to release it
> --- something which the patchset resolves. The result is here:
> 
> 

Did you test this on 4.9-rc7 without any of your other patches? If you
haven't could you please run this test with just the removing devm usage
from omap3isp?

It would be good to get a baseline on the current with just the not using
devm first and then see what needs fixing.

Also, could you please send me the complete dmesg.

thanks,
-- Shuah

> 
> What happens there is that as part of the device unbinding, the graph
> objects are unregistered (by media_device_unregister()) while streaming is
> ongoing. Their parent media device pointers are set to NULL.
> 
> Then, when the user process exists, the streaming media pipeline is stopped.
> This requires parsing of the media graph, a job which will obviously fail
> miserably and immediately: the media device is obtained from the entity
> where graph traversal is expected to begin.
> 
> Two mutexes are also acquired but they've been already destroyed at that
> point (and the memory of which would also be released now without the hack
> to test this "without devm"). There's just a single warning on this though.
> 
>> top of media dev allocator because the allocator routines he has don't 
>> address
>> the shared media device need.
> 
> I do strongly prefer fixing the existing object lifetime issues in the
> framework before extending it and thus making the problem domain more
> complex than it is already.
> 
> What I mentioned as an example of this are the other callbacks to the media
> device: presumably they do suffer from the exactly same problems as the
> enable_source() and disable_source() ones.
> 
> As drivers do refer to other entities and controls they do expose also to
> the user space, we can't really even remove entities safely at the moment.
> We should have a solution to this as well, my patchset at the moment does
> not address this.
> 
> What we do need a sound basis for the framework rather than hastily written
> improvements to support a particular device. Also, testing device removal
> with a particular device in a particular use case does not guarantee that no
> further object lifetime issues related to device removal exist, even with
> that same driver, let alone other devices.
> 
> Instead, we must consider how the frameworks and drivers manage the memory
> allocated for the various objects, what are relations of those objects and
> how they're exposed to the user space either directly or indirectly. As long
> as objects (such as the media graph objects) with different lifetimes are
> referred to from other objects without taking a 

Re: [PATCH v6 0/5] davinci: VPIF: add DT support

[Javier Martinez Canillas]

Hello Kevin,

On Wed, Dec 7, 2016 at 3:30 PM, Kevin Hilman  wrote:
> Prepare the groundwork for adding DT support for davinci VPIF drivers.
> This series does some fixups/cleanups and then adds the DT binding and
> DT compatible string matching for DT probing.
>
> The controversial part from previous versions around async subdev
> parsing, and specifically hard-coding the input/output routing of
> subdevs, has been left out of this series.  That part can be done as a
> follow-on step after agreement has been reached on the path forward.

I had a similar need for another board (OMAP3 IGEPv2), that has a
TVP5151 video decoder (that also supports 2 composite or 1 s-video
signal) attached to the OMAP3 ISP.

I posted some RFC patches [0] to define the input signals in the DT,
and AFAICT Laurent and Hans were not against the approach but just had
some comments on the DT binding.

Basically they wanted the ports to be directly in the tvp5150 node
instead of under a connectors sub-node [1] and to just be called just
a (input / output) port instead of a connector [2].

Unfortunately I was busy with other tasks so I couldn't res-pin the
patches, but I think you could have something similar in the DT
binding for your case and it shouldn't be hard to parse the ports /
endpoints in the driver to get that information from DT and setup the
input and output pins.

> With this version, platforms can still use the VPIF capture/display
> drivers, but must provide platform_data for the subdevs and subdev
> routing.
>

I guess DT backward compatibility isn't a big issue on this platform,
since support for the platform is quite recently and after all someone
who wants to use the vpif with current DT will need platform data and
pdata-quirks anyways. So I agree with you that the input / output
signals lookup from DT could be done as a follow-up.

[0]: https://lkml.org/lkml/2016/4/12/983
[1]: https://lkml.org/lkml/2016/4/27/678
[2]: https://lkml.org/lkml/2016/11/11/346

Best regards,
Javier
--
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 v6 2/5] [media] davinci: vpif_capture: remove hard-coded I2C adapter id

[Kevin Hilman]

Remove hard-coded I2C adapter in favor of getting the
ID from platform_data.

Signed-off-by: Kevin Hilman 
---
 drivers/media/platform/davinci/vpif_capture.c | 5 -
 include/media/davinci/vpif_types.h| 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/davinci/vpif_capture.c 
b/drivers/media/platform/davinci/vpif_capture.c
index 20c4344ed118..c24049acd40a 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1486,7 +1486,10 @@ static __init int vpif_probe(struct platform_device 
*pdev)
}
 
if (!vpif_obj.config->asd_sizes) {
-   i2c_adap = i2c_get_adapter(1);
+   int i2c_id = vpif_obj.config->i2c_adapter_id;
+
+   i2c_adap = i2c_get_adapter(i2c_id);
+   WARN_ON(!i2c_adap);
for (i = 0; i < subdev_count; i++) {
subdevdata = _obj.config->subdev_info[i];
vpif_obj.sd[i] =
diff --git a/include/media/davinci/vpif_types.h 
b/include/media/davinci/vpif_types.h
index 3cb1704a0650..4282a7db99d4 100644
--- a/include/media/davinci/vpif_types.h
+++ b/include/media/davinci/vpif_types.h
@@ -82,6 +82,7 @@ struct vpif_capture_config {
struct vpif_capture_chan_config chan_config[VPIF_CAPTURE_MAX_CHANNELS];
struct vpif_subdev_info *subdev_info;
int subdev_count;
+   int i2c_adapter_id;
const char *card_name;
struct v4l2_async_subdev **asd; /* Flat array, arranged in groups */
int *asd_sizes; /* 0-terminated array of asd group sizes */
-- 
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 v6 3/5] [media] davinci: vpif_capture: fix start/stop streaming locking

[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.

The IRQ-disabled locking is meant to protect the DMA queue list
throughout the rest of the driver, so update the locking in
[start|stop]_streaming to protect just this list, and update the irqlock
comment to reflect what it actually protects.

Suggested-by: Laurent Pinchart 
Reviewed-by: Laurent Pinchart 
Signed-off-by: Kevin Hilman 
---
 drivers/media/platform/davinci/vpif_capture.c | 6 +++---
 drivers/media/platform/davinci/vpif_capture.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif_capture.c 
b/drivers/media/platform/davinci/vpif_capture.c
index c24049acd40a..f72a719efb3d 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -179,8 +179,6 @@ static int vpif_start_streaming(struct vb2_queue *vq, 
unsigned int count)
unsigned long addr, flags;
int ret;
 
-   spin_lock_irqsave(>irqlock, flags);
-
/* Initialize field_id */
ch->field_id = 0;
 
@@ -211,6 +209,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, 
unsigned int count)
vpif_config_addr(ch, ret);
 
/* Get the next frame from the buffer queue */
+   spin_lock_irqsave(>irqlock, flags);
common->cur_frm = common->next_frm = list_entry(common->dma_queue.next,
struct vpif_cap_buffer, list);
/* Remove buffer from the buffer queue */
@@ -244,6 +243,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, 
unsigned int count)
return 0;
 
 err:
+   spin_lock_irqsave(>irqlock, flags);
list_for_each_entry_safe(buf, tmp, >dma_queue, list) {
list_del(>list);
vb2_buffer_done(>vb.vb2_buf, VB2_BUF_STATE_QUEUED);
@@ -287,7 +287,6 @@ static void vpif_stop_streaming(struct vb2_queue *vq)
vpif_dbg(1, debug, "stream off failed in subdev\n");
 
/* release all active buffers */
-   spin_lock_irqsave(>irqlock, flags);
if (common->cur_frm == common->next_frm) {
vb2_buffer_done(>cur_frm->vb.vb2_buf,
VB2_BUF_STATE_ERROR);
@@ -300,6 +299,7 @@ static void vpif_stop_streaming(struct vb2_queue *vq)
VB2_BUF_STATE_ERROR);
}
 
+   spin_lock_irqsave(>irqlock, flags);
while (!list_empty(>dma_queue)) {
common->next_frm = list_entry(common->dma_queue.next,
struct vpif_cap_buffer, list);
diff --git a/drivers/media/platform/davinci/vpif_capture.h 
b/drivers/media/platform/davinci/vpif_capture.h
index 9e35b6771d22..1d2c052deedf 100644
--- a/drivers/media/platform/davinci/vpif_capture.h
+++ b/drivers/media/platform/davinci/vpif_capture.h
@@ -67,7 +67,7 @@ struct common_obj {
struct vb2_queue buffer_queue;
/* Queue of filled frames */
struct list_head dma_queue;
-   /* Used in video-buf */
+   /* Protects the dma_queue field */
spinlock_t irqlock;
/* lock used to access this structure */
struct mutex lock;
-- 
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 v6 4/5] [media] dt-bindings: add TI VPIF documentation

[Kevin Hilman]

Acked-by: Rob Herring 
Reviewed-by: Laurent Pinchart 
Signed-off-by: Kevin Hilman 
---
 .../devicetree/bindings/media/ti,da850-vpif.txt| 83 ++
 1 file changed, 83 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 ..6d25d7f23d26
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
@@ -0,0 +1,83 @@
+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>;
+   };
+   };
+   };
+};
+
+
+Alternatively, an example when the bus is configured as a single
+16-bit input (e.g. for raw-capture mode):
+
+   vpif: vpif@217000 {
+   compatible = "ti,da850-vpif";
+   reg = <0x217000 0x1000>;
+   interrupts = <92>;
+
+   port {
+   vpif_ch0: endpoint {
+ bus-width = <16>;
+   };
+   };
+   };
-- 
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 v6 1/5] [media] davinci: VPIF: fix module loading, init errors

[Kevin Hilman]

Fix problems with automatic module loading by adding MODULE_ALIAS.  Also
fix various load-time errors cause by incorrect or not present
platform_data.

Signed-off-by: Kevin Hilman 
---
 drivers/media/platform/davinci/vpif.c |  5 -
 drivers/media/platform/davinci/vpif_capture.c | 15 ++-
 drivers/media/platform/davinci/vpif_display.c |  6 ++
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif.c 
b/drivers/media/platform/davinci/vpif.c
index 0380cf2e5775..f50148dcba64 100644
--- a/drivers/media/platform/davinci/vpif.c
+++ b/drivers/media/platform/davinci/vpif.c
@@ -32,6 +32,9 @@
 MODULE_DESCRIPTION("TI DaVinci Video Port Interface driver");
 MODULE_LICENSE("GPL");
 
+#define VPIF_DRIVER_NAME   "vpif"
+MODULE_ALIAS("platform:" VPIF_DRIVER_NAME);
+
 #define VPIF_CH0_MAX_MODES 22
 #define VPIF_CH1_MAX_MODES 2
 #define VPIF_CH2_MAX_MODES 15
@@ -466,7 +469,7 @@ static const struct dev_pm_ops vpif_pm = {
 
 static struct platform_driver vpif_driver = {
.driver = {
-   .name   = "vpif",
+   .name   = VPIF_DRIVER_NAME,
.pm = vpif_pm_ops,
},
.remove = vpif_remove,
diff --git a/drivers/media/platform/davinci/vpif_capture.c 
b/drivers/media/platform/davinci/vpif_capture.c
index 5104cc0ee40e..20c4344ed118 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} };
@@ -647,6 +648,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;
@@ -654,7 +659,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;
@@ -685,6 +690,9 @@ static int vpif_set_input(
if (sd_index >= 0) {
sd = vpif_obj.sd[sd_index];
subdev_info = _cfg->subdev_info[sd_index];
+   } else {
+   /* no subdevice, no input to setup */
+   return 0;
}
 
/* first setup input path from sub device to vpif */
@@ -1435,6 +1443,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 = initialize_vpif();
 
-- 
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 v6 0/5] davinci: VPIF: add DT support

[Kevin Hilman]

Prepare the groundwork for adding DT support for davinci VPIF drivers.
This series does some fixups/cleanups and then adds the DT binding and
DT compatible string matching for DT probing.

The controversial part from previous versions around async subdev
parsing, and specifically hard-coding the input/output routing of
subdevs, has been left out of this series.  That part can be done as a
follow-on step after agreement has been reached on the path forward.
With this version, platforms can still use the VPIF capture/display
drivers, but must provide platform_data for the subdevs and subdev
routing.

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

Changes since v5:
- locking fix: updated comment around lock variable
- binding doc: added example for 
- added reviewed-by tags from Laurent (thanks!)

Changes since v4:
- dropped controversial async subdev parsing support.  That can be
  done as a follow-up step after the discussions have finalized on the
right approach.
- DT binding Acked by DT maintainer (Rob H.)
- reworked locking fix (suggested by Laurent)

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 (5):
  [media] davinci: VPIF: fix module loading, init errors
  [media] davinci: vpif_capture: remove hard-coded I2C adapter id
  [media] davinci: vpif_capture: fix start/stop streaming locking
  [media] dt-bindings: add TI VPIF documentation
  [media] davinci: VPIF: add basic support for DT init

 .../devicetree/bindings/media/ti,da850-vpif.txt| 83 ++
 drivers/media/platform/davinci/vpif.c  | 14 +++-
 drivers/media/platform/davinci/vpif_capture.c  | 26 +--
 drivers/media/platform/davinci/vpif_capture.h  |  2 +-
 drivers/media/platform/davinci/vpif_display.c  |  6 ++
 include/media/davinci/vpif_types.h |  1 +
 6 files changed, 125 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 v6 5/5] [media] davinci: VPIF: add basic support for DT init

[Kevin Hilman]

Add basic support for initialization via DT

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

diff --git a/drivers/media/platform/davinci/vpif.c 
b/drivers/media/platform/davinci/vpif.c
index f50148dcba64..1b02a6363f77 100644
--- a/drivers/media/platform/davinci/vpif.c
+++ b/drivers/media/platform/davinci/vpif.c
@@ -467,8 +467,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_DRIVER_NAME,
.pm = vpif_pm_ops,
},
-- 
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

Regression: tvp5150 refactoring breaks all em28xx devices

[Devin Heitmueller]

Hello Javier, Mauro, Laurent,

I hope all is well with you.  Mauro, Laurent:  you guys going to
ELC/Portland in February?

Looks like the refactoring done to tvp5150 in January 2016 for
s_stream() to support some embedded platform caused breakage in the
30+ em28xx products that also use the chip.

Problem confirmed on both the Startech SVIDUSB2 board Steve Preston
was nice enough to ship me (after adding a board profile), as well as
on my original HVR-950 which has worked fine since 2008.

The implementation tramples the TVP5150_MISC_CTL register, blowing
into it a hard-coded value based on one of two scenarios, neither of
which matches what is expected by em28xx devices.  At least in the
case of NTSC, this results in chroma cycling.  This was also reported
by Alexandre-Xavier Labonté-Lamoureux back in August, although in the
video below he's also having some other issue related to progressive
video because he's using an old gaming console as the source (i.e. pay
attention to the chroma effects in the top half of the video rather
than the fact that only the first field is being rendered).

https://youtu.be/WLlqJ7T3y4g

The s_stream implementation writes 0x09 or 0x0d into TVP5150_MISC_CTL
(overriding whatever was written by tvp5150_init_default and
tvp5150_selmux().  In fact, just as a test I was able to start up
video, see the corruption, and write the correct value back into the
register via v4l2-dbg in order to get it working again:

sudo v4l2-dbg --chip=subdev0 --set-register=0x03 0x6f

There's no easy fix for this without extending the driver to support
proper configuration of the output pin muxing, which it isn't clear to
me what the right approach is and I don't have the embedded hardware
platform that prompted the refactoring in order to do regression
testing anyway.

Feel free to take it upon yourselves to fix the regression you introduced.

Thanks,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.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

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

[Kevin Hilman]

Laurent Pinchart  writes:

> Hi Kevin,
>
> Thank you for the patch.
>
> On Tuesday 06 Dec 2016 21:08:25 Kevin Hilman wrote:
>> Acked-by: Rob Herring 
>> 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.
>
> You might want to clarify how endpoints are use in the two cases. Apart from 
> that,

OK, I'll add another example for the 16-bit case.  Something like...

>> +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>;
>> +};
>> +};
>> +};
>> +};

...this, at the end of the binding doc:

Alternatively, an example when the bus is configured as a single
16-bit input (e.g. for raw-capture mode):

vpif: vpif@217000 {
compatible = "ti,da850-vpif";
reg = <0x217000 0x1000>;
interrupts = <92>;

port {
vpif_ch0: endpoint {
  bus-width = <16>;
};
};
};


Thanks for the review,

Kevin
--
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] vim2m: Clean up file handle in open() error path.

[Santosh Kumar Singh]

Fix to avoid possible memory leak and exit file handle
in error paths.

Signed-off-by: Santosh Kumar Singh 
---
 drivers/media/platform/vim2m.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index a98f679..9fd24b8 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -907,6 +907,7 @@ static int vim2m_open(struct file *file)
if (hdl->error) {
rc = hdl->error;
v4l2_ctrl_handler_free(hdl);
+   kfree(ctx);
goto open_unlock;
}
ctx->fh.ctrl_handler = hdl;
@@ -929,6 +930,7 @@ static int vim2m_open(struct file *file)
 
v4l2_ctrl_handler_free(hdl);
kfree(ctx);
+   v4l2_fh_exit(>fh);
goto open_unlock;
}
 
-- 
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

Re: [PATCH v5 3/5] [media] davinci: vpif_capture: fix start/stop streaming locking

[Kevin Hilman]

Laurent Pinchart  writes:

> Hi Kevin,
>
> Thank you for the patch.
>
> On Tuesday 06 Dec 2016 21:08:24 Kevin Hilman wrote:
>> Video capture subdevs may be over I2C and may sleep during xfer, so we
>> cannot do IRQ-disabled locking when calling the subdev.
>> 
>> The IRQ-disabled locking is meant to protect the DMA queue list
>> throughout the rest of the driver, so update the locking in
>> [start|stop]_streaming to protect just this list.
>> 
>> Suggested-by: Laurent Pinchart 
>> Signed-off-by: Kevin Hilman 
>
> I would also add a comment to document the irqlock field as protecting the 
> dma_queue list only. Something like
>
> - /* Used in video-buf */
> + /* Protects the dma_queue field */
>   spinlock_t irqlock;
>
> With that,
>
> Reviewed-by: Laurent Pinchart 

OK, will update the comment.  Thanks for the review,

Kevin
--
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 v4 1/4] [media] davinci: vpif_capture: don't lock over s_stream

[Kevin Hilman]

Laurent Pinchart  writes:

 > Hi Kevin,
>
> On Tuesday 06 Dec 2016 08:49:38 Kevin Hilman wrote:
>> Laurent Pinchart writes:
>> > On Tuesday 29 Nov 2016 15:57:09 Kevin Hilman wrote:
>> >> 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);
>> > 
>> > I always get anxious when I see a spinlock being released randomly with an
>> > operation in the middle of a protected section. Looking at the code it
>> > looks like the spinlock is abused here. irqlock should only protect the
>> > dma_queue and should thus only be taken around the following code:
>> > 
>> > spin_lock_irqsave(>irqlock, flags);
>> > /* Get the next frame from the buffer queue */
>> > common->cur_frm = common->next_frm = list_entry(common->dma_queue.next,
>> > struct vpif_cap_buffer, list);
>> > 
>> > /* Remove buffer from the buffer queue */
>> > list_del(>cur_frm->list);
>> > spin_unlock_irqrestore(>irqlock, flags);
>> 
>> Yes, that looks correct.  Will update.
>> 
>> > The code that is currently protected by the lock in the start and stop
>> > streaming functions should be protected by a mutex instead.
>> 
>> I tried taking the mutex here, but lockdep pointed out a deadlock.  I
>> may not be fully understanding the V4L2 internals here, but it seems
>> that the ioctl is already taking a mutex, so taking it again in
>> start/stop streaming is a deadlock.  Unless you think the locking should
>> be nested here, it seems to me that the mutex isn't needed.
>
> The V4L2 core can lock all ioctls using struct video_device::lock. For buffer-
> related ioctls, it can optionally use a separate lock from struct 
> vb2_queue::lock. See v4l2_ioctl_get_lock() in drivers/media/v4l2-core/v4l2-
> ioctl.c.
>
> The vpif-capture driver sets both the video_device and vb2_queue locks to the 
> same lock (which would have the same effect as leaving the vb2_queue lock 
> NULL). All ioctls are thus serialized. You would only need to handle locking 
> in start_streaming and stop_streaming manually if you didn't rely on the core 
> serializing the ioctls.

OK, thanks for clarifying how that works.

Kevin
--
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 v4 1/4] [media] davinci: vpif_capture: don't lock over s_stream

[Laurent Pinchart]

Hi Kevin,

On Tuesday 06 Dec 2016 08:49:38 Kevin Hilman wrote:
> Laurent Pinchart writes:
> > On Tuesday 29 Nov 2016 15:57:09 Kevin Hilman wrote:
> >> 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);
> > 
> > I always get anxious when I see a spinlock being released randomly with an
> > operation in the middle of a protected section. Looking at the code it
> > looks like the spinlock is abused here. irqlock should only protect the
> > dma_queue and should thus only be taken around the following code:
> > 
> > spin_lock_irqsave(>irqlock, flags);
> > /* Get the next frame from the buffer queue */
> > common->cur_frm = common->next_frm = list_entry(common->dma_queue.next,
> > struct vpif_cap_buffer, list);
> > 
> > /* Remove buffer from the buffer queue */
> > list_del(>cur_frm->list);
> > spin_unlock_irqrestore(>irqlock, flags);
> 
> Yes, that looks correct.  Will update.
> 
> > The code that is currently protected by the lock in the start and stop
> > streaming functions should be protected by a mutex instead.
> 
> I tried taking the mutex here, but lockdep pointed out a deadlock.  I
> may not be fully understanding the V4L2 internals here, but it seems
> that the ioctl is already taking a mutex, so taking it again in
> start/stop streaming is a deadlock.  Unless you think the locking should
> be nested here, it seems to me that the mutex isn't needed.

The V4L2 core can lock all ioctls using struct video_device::lock. For buffer-
related ioctls, it can optionally use a separate lock from struct 
vb2_queue::lock. See v4l2_ioctl_get_lock() in drivers/media/v4l2-core/v4l2-
ioctl.c.

The vpif-capture driver sets both the video_device and vb2_queue locks to the 
same lock (which would have the same effect as leaving the vb2_queue lock 
NULL). All ioctls are thus serialized. You would only need to handle locking 
in start_streaming and stop_streaming manually if you didn't rely on the core 
serializing the ioctls.

-- 
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: em28xx broken 4.9.0-rc6+

[Mauro Carvalho Chehab]

Em Wed, 7 Dec 2016 17:04:07 +0200
Antti Palosaari  escreveu:

> On 12/07/2016 04:55 PM, Mauro Carvalho Chehab wrote:
> > Em Wed, 7 Dec 2016 12:22:01 -0200
> > Mauro Carvalho Chehab  escreveu:
> >  
> >> Em Tue, 6 Dec 2016 13:41:38 -0200
> >> Mauro Carvalho Chehab  escreveu:
> >>  
> >>> Em Tue, 6 Dec 2016 01:06:17 +0200
> >>> Antti Palosaari  escreveu:
> >>>  
>  Hello Mauro
>  I just noticed current em28xx driver seem to be broken. When I plug
>  device first time it loads correctly, but when I re-plug it, it does not
>  work anymore but yells a lot of noise to message log. Tested with PCTV
>  290e and 292e both same. Other USB DVB devices are working so it is very
>  likely em28xx driver bug.
> 
>  Easy to reproduce:
>  plug device
>  unplug device
>  plug device  
> >>>
> >>>
> >>> Are you referring to those:
> >>>
> >>> [ 1010.310320] WARNING: CPU: 3 PID: 119 at fs/sysfs/dir.c:31 
> >>> sysfs_warn_dup+0x7b/0x90
> >>> [ 1010.310323] sysfs: cannot create duplicate filename 
> >>> '/bus/usb/devices/1-3.3'
> >>> [ 1010.310325] Modules linked in: lgdt330x em28xx_dvb dvb_core 
> >>> em28xx_alsa tuner_xc2028 tuner tvp5150 em28xx_v4l videobuf2_vmalloc 
> >>> videobuf2_memops videobuf2_v4l2 videobuf2_core em28xx tveeprom 
> >>> v4l2_common videodev media xt_CHECKSUM iptable_mangle ipt_MASQUERADE 
> >>> nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 
> >>> nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 
> >>> xt_tcpudp tun bridge stp llc ebtable_filter ebtables ip6table_filter 
> >>> ip6_tables iptable_filter ip_tables x_tables cmac bnep cpufreq_powersave 
> >>> cpufreq_conservative cpufreq_userspace binfmt_misc parport_pc ppdev lp 
> >>> parport snd_hda_codec_hdmi iTCO_wdt snd_hda_codec_realtek 
> >>> iTCO_vendor_support snd_hda_codec_generic arc4 intel_rapl 
> >>> x86_pkg_temp_thermal iwlmvm intel_powerclamp coretemp kvm_intel mac80211 
> >>> kvm i915
> >>> [ 1010.310383]  irqbypass crct10dif_pclmul crc32_pclmul 
> >>> ghash_clmulni_intel iwlwifi pl2303 aesni_intel btusb aes_x86_64 usbserial 
> >>> lrw btrtl gf128mul glue_helper btbcm ablk_helper cryptd btintel bluetooth 
> >>> drm_kms_helper cfg80211 drm psmouse pcspkr i2c_i801 e1000e serio_raw 
> >>> snd_hda_intel snd_soc_rt5640 snd_hda_codec snd_soc_rl6231 snd_soc_ssm4567 
> >>> mei_me i2c_smbus rfkill snd_hda_core ptp mei snd_soc_core ehci_pci sg 
> >>> lpc_ich shpchp mfd_core ehci_hcd pps_core snd_hwdep i2c_algo_bit 
> >>> snd_compress snd_pcm sdhci_acpi snd_timer battery snd sdhci elan_i2c 
> >>> snd_soc_sst_acpi mmc_core fjes dw_dmac i2c_hid soundcore 
> >>> snd_soc_sst_match i2c_designware_platform video i2c_designware_core 
> >>> acpi_pad acpi_als kfifo_buf tpm_tis button industrialio tpm_tis_core tpm 
> >>> ext4 crc16 jbd2 fscrypto mbcache dm_mod joydev evdev hid_logitech_hidpp
> >>> [ 1010.310449]  sd_mod hid_logitech_dj usbhid hid ahci libahci 
> >>> crc32c_intel libata xhci_pci xhci_hcd scsi_mod usbcore fan thermal
> >>> [ 1010.310464] CPU: 3 PID: 119 Comm: kworker/3:2 Not tainted 4.9.0-rc8+ 
> >>> #14
> >>> [ 1010.310466] Hardware name:  /NUC5i7RYB, BIOS 
> >>> RYBDWi35.86A.0350.2015.0812.1722 08/12/2015
> >>> [ 1010.310487] Workqueue: usb_hub_wq hub_event [usbcore]
> >>> [ 1010.310490]   848f56c5 8803b1f7f858 
> >>> 
> >>> [ 1010.310496]  8414f8f8 8803001f ed00763eff07 
> >>> 8803b1f7f8f0
> >>> [ 1010.310501]  8803b3ea1e60 0001 ffef 
> >>> 8803b45c6840
> >>> [ 1010.310505] Call Trace:
> >>> [ 1010.310517]  [] ? dump_stack+0x5c/0x77
> >>> [ 1010.310522]  [] ? __warn+0x168/0x1a0
> >>> [ 1010.310526]  [] ? warn_slowpath_fmt+0xb4/0xf0
> >>> [ 1010.310529]  [] ? __warn+0x1a0/0x1a0
> >>> [ 1010.310534]  [] ? kasan_kmalloc+0xa6/0xd0
> >>> [ 1010.310539]  [] ? kernfs_path_from_node+0x4a/0x60
> >>> [ 1010.310543]  [] ? sysfs_warn_dup+0x7b/0x90
> >>> [ 1010.310547]  [] ? 
> >>> sysfs_do_create_link_sd.isra.2+0xb6/0xd0
> >>> [ 1010.310553]  [] ? bus_add_device+0x318/0x6b0
> >>> [ 1010.310557]  [] ? sysfs_create_groups+0x83/0x110
> >>> [ 1010.310562]  [] ? device_add+0x777/0x1350
> >>> [ 1010.310567]  [] ? device_private_init+0x180/0x180
> >>> [ 1010.310583]  [] ? usb_new_device+0x707/0x1030 
> >>> [usbcore]
> >>> [ 1010.310598]  [] ? hub_event+0x1d65/0x3280 [usbcore]
> >>> [ 1010.310604]  [] ? account_entity_dequeue+0x30b/0x4a0
> >>> [ 1010.310618]  [] ? hub_port_debounce+0x280/0x280 
> >>> [usbcore]
> >>> [ 1010.310624]  [] ? compat_start_thread+0x80/0x80
> >>> [ 1010.310629]  [] ? __schedule+0x704/0x1770
> >>> [ 1010.310633]  [] ? io_schedule_timeout+0x390/0x390
> >>> [ 1010.310638]  [] ? cache_reap+0x173/0x200
> >>> [ 1010.310642]  [] ? process_one_work+0x4ed/0xe60
> >>> [ 1010.310646]  [] ? worker_thread+0xe2/0xfd0
> >>> [ 1010.310650]  [] ? __wake_up_common+0xbc/0x160
> 

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

[Laurent Pinchart]

Hi Kevin,

Thank you for the patch.

On Tuesday 06 Dec 2016 21:08:25 Kevin Hilman wrote:
> Acked-by: Rob Herring 
> 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.

You might want to clarify how endpoints are use in the two cases. Apart from 
that,

Reviewed-by: Laurent Pinchart 

> +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>;
> + };
> + };
> + };
> +};

-- 
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: em28xx broken 4.9.0-rc6+

[Antti Palosaari]

On 12/07/2016 04:55 PM, Mauro Carvalho Chehab wrote:

Em Wed, 7 Dec 2016 12:22:01 -0200
Mauro Carvalho Chehab  escreveu:


Em Tue, 6 Dec 2016 13:41:38 -0200
Mauro Carvalho Chehab  escreveu:


Em Tue, 6 Dec 2016 01:06:17 +0200
Antti Palosaari  escreveu:


Hello Mauro
I just noticed current em28xx driver seem to be broken. When I plug
device first time it loads correctly, but when I re-plug it, it does not
work anymore but yells a lot of noise to message log. Tested with PCTV
290e and 292e both same. Other USB DVB devices are working so it is very
likely em28xx driver bug.

Easy to reproduce:
plug device
unplug device
plug device



Are you referring to those:

[ 1010.310320] WARNING: CPU: 3 PID: 119 at fs/sysfs/dir.c:31 
sysfs_warn_dup+0x7b/0x90
[ 1010.310323] sysfs: cannot create duplicate filename '/bus/usb/devices/1-3.3'
[ 1010.310325] Modules linked in: lgdt330x em28xx_dvb dvb_core em28xx_alsa 
tuner_xc2028 tuner tvp5150 em28xx_v4l videobuf2_vmalloc videobuf2_memops 
videobuf2_v4l2 videobuf2_core em28xx tveeprom v4l2_common videodev media 
xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat 
nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack 
ipt_REJECT nf_reject_ipv4 xt_tcpudp tun bridge stp llc ebtable_filter ebtables 
ip6table_filter ip6_tables iptable_filter ip_tables x_tables cmac bnep 
cpufreq_powersave cpufreq_conservative cpufreq_userspace binfmt_misc parport_pc 
ppdev lp parport snd_hda_codec_hdmi iTCO_wdt snd_hda_codec_realtek 
iTCO_vendor_support snd_hda_codec_generic arc4 intel_rapl x86_pkg_temp_thermal 
iwlmvm intel_powerclamp coretemp kvm_intel mac80211 kvm i915
[ 1010.310383]  irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel 
iwlwifi pl2303 aesni_intel btusb aes_x86_64 usbserial lrw btrtl gf128mul 
glue_helper btbcm ablk_helper cryptd btintel bluetooth drm_kms_helper cfg80211 
drm psmouse pcspkr i2c_i801 e1000e serio_raw snd_hda_intel snd_soc_rt5640 
snd_hda_codec snd_soc_rl6231 snd_soc_ssm4567 mei_me i2c_smbus rfkill 
snd_hda_core ptp mei snd_soc_core ehci_pci sg lpc_ich shpchp mfd_core ehci_hcd 
pps_core snd_hwdep i2c_algo_bit snd_compress snd_pcm sdhci_acpi snd_timer 
battery snd sdhci elan_i2c snd_soc_sst_acpi mmc_core fjes dw_dmac i2c_hid 
soundcore snd_soc_sst_match i2c_designware_platform video i2c_designware_core 
acpi_pad acpi_als kfifo_buf tpm_tis button industrialio tpm_tis_core tpm ext4 
crc16 jbd2 fscrypto mbcache dm_mod joydev evdev hid_logitech_hidpp
[ 1010.310449]  sd_mod hid_logitech_dj usbhid hid ahci libahci crc32c_intel 
libata xhci_pci xhci_hcd scsi_mod usbcore fan thermal
[ 1010.310464] CPU: 3 PID: 119 Comm: kworker/3:2 Not tainted 4.9.0-rc8+ #14
[ 1010.310466] Hardware name:  /NUC5i7RYB, BIOS 
RYBDWi35.86A.0350.2015.0812.1722 08/12/2015
[ 1010.310487] Workqueue: usb_hub_wq hub_event [usbcore]
[ 1010.310490]   848f56c5 8803b1f7f858 

[ 1010.310496]  8414f8f8 8803001f ed00763eff07 
8803b1f7f8f0
[ 1010.310501]  8803b3ea1e60 0001 ffef 
8803b45c6840
[ 1010.310505] Call Trace:
[ 1010.310517]  [] ? dump_stack+0x5c/0x77
[ 1010.310522]  [] ? __warn+0x168/0x1a0
[ 1010.310526]  [] ? warn_slowpath_fmt+0xb4/0xf0
[ 1010.310529]  [] ? __warn+0x1a0/0x1a0
[ 1010.310534]  [] ? kasan_kmalloc+0xa6/0xd0
[ 1010.310539]  [] ? kernfs_path_from_node+0x4a/0x60
[ 1010.310543]  [] ? sysfs_warn_dup+0x7b/0x90
[ 1010.310547]  [] ? sysfs_do_create_link_sd.isra.2+0xb6/0xd0
[ 1010.310553]  [] ? bus_add_device+0x318/0x6b0
[ 1010.310557]  [] ? sysfs_create_groups+0x83/0x110
[ 1010.310562]  [] ? device_add+0x777/0x1350
[ 1010.310567]  [] ? device_private_init+0x180/0x180
[ 1010.310583]  [] ? usb_new_device+0x707/0x1030 [usbcore]
[ 1010.310598]  [] ? hub_event+0x1d65/0x3280 [usbcore]
[ 1010.310604]  [] ? account_entity_dequeue+0x30b/0x4a0
[ 1010.310618]  [] ? hub_port_debounce+0x280/0x280 [usbcore]
[ 1010.310624]  [] ? compat_start_thread+0x80/0x80
[ 1010.310629]  [] ? __schedule+0x704/0x1770
[ 1010.310633]  [] ? io_schedule_timeout+0x390/0x390
[ 1010.310638]  [] ? cache_reap+0x173/0x200
[ 1010.310642]  [] ? process_one_work+0x4ed/0xe60
[ 1010.310646]  [] ? worker_thread+0xe2/0xfd0
[ 1010.310650]  [] ? __wake_up_common+0xbc/0x160
[ 1010.310654]  [] ? process_one_work+0xe60/0xe60
[ 1010.310658]  [] ? kthread+0x1cc/0x220
[ 1010.310663]  [] ? kthread_park+0x80/0x80
[ 1010.310667]  [] ? kthread_park+0x80/0x80
[ 1010.310671]  [] ? kthread_park+0x80/0x80
[ 1010.310675]  [] ? ret_from_fork+0x25/0x30
[ 1010.310698] ---[ end trace 49b46eb633ff1197 ]---
[ 1010.311298] usb 1-3.3: can't device_add, error -17
[ 1010.390703] usb 1-3.3: new high-speed USB device number 9 using xhci_hcd
[ 1010.496337] usb 1-3.3: New USB device found, idVendor=2040, idProduct=6513
[ 1010.496343] usb 1-3.3: New USB device strings: Mfr=0, Product=1, 
SerialNumber=2
[ 1010.496345] usb 1-3.3: 

Re: em28xx broken 4.9.0-rc6+

[Mauro Carvalho Chehab]

Em Wed, 7 Dec 2016 12:22:01 -0200
Mauro Carvalho Chehab  escreveu:

> Em Tue, 6 Dec 2016 13:41:38 -0200
> Mauro Carvalho Chehab  escreveu:
> 
> > Em Tue, 6 Dec 2016 01:06:17 +0200
> > Antti Palosaari  escreveu:
> >   
> > > Hello Mauro
> > > I just noticed current em28xx driver seem to be broken. When I plug 
> > > device first time it loads correctly, but when I re-plug it, it does not 
> > > work anymore but yells a lot of noise to message log. Tested with PCTV 
> > > 290e and 292e both same. Other USB DVB devices are working so it is very 
> > > likely em28xx driver bug.
> > > 
> > > Easy to reproduce:
> > > plug device
> > > unplug device
> > > plug device  
> > 
> > 
> > Are you referring to those:
> > 
> > [ 1010.310320] WARNING: CPU: 3 PID: 119 at fs/sysfs/dir.c:31 
> > sysfs_warn_dup+0x7b/0x90
> > [ 1010.310323] sysfs: cannot create duplicate filename 
> > '/bus/usb/devices/1-3.3'
> > [ 1010.310325] Modules linked in: lgdt330x em28xx_dvb dvb_core em28xx_alsa 
> > tuner_xc2028 tuner tvp5150 em28xx_v4l videobuf2_vmalloc videobuf2_memops 
> > videobuf2_v4l2 videobuf2_core em28xx tveeprom v4l2_common videodev media 
> > xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 
> > iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 
> > xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp tun bridge 
> > stp llc ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter 
> > ip_tables x_tables cmac bnep cpufreq_powersave cpufreq_conservative 
> > cpufreq_userspace binfmt_misc parport_pc ppdev lp parport 
> > snd_hda_codec_hdmi iTCO_wdt snd_hda_codec_realtek iTCO_vendor_support 
> > snd_hda_codec_generic arc4 intel_rapl x86_pkg_temp_thermal iwlmvm 
> > intel_powerclamp coretemp kvm_intel mac80211 kvm i915
> > [ 1010.310383]  irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel 
> > iwlwifi pl2303 aesni_intel btusb aes_x86_64 usbserial lrw btrtl gf128mul 
> > glue_helper btbcm ablk_helper cryptd btintel bluetooth drm_kms_helper 
> > cfg80211 drm psmouse pcspkr i2c_i801 e1000e serio_raw snd_hda_intel 
> > snd_soc_rt5640 snd_hda_codec snd_soc_rl6231 snd_soc_ssm4567 mei_me 
> > i2c_smbus rfkill snd_hda_core ptp mei snd_soc_core ehci_pci sg lpc_ich 
> > shpchp mfd_core ehci_hcd pps_core snd_hwdep i2c_algo_bit snd_compress 
> > snd_pcm sdhci_acpi snd_timer battery snd sdhci elan_i2c snd_soc_sst_acpi 
> > mmc_core fjes dw_dmac i2c_hid soundcore snd_soc_sst_match 
> > i2c_designware_platform video i2c_designware_core acpi_pad acpi_als 
> > kfifo_buf tpm_tis button industrialio tpm_tis_core tpm ext4 crc16 jbd2 
> > fscrypto mbcache dm_mod joydev evdev hid_logitech_hidpp
> > [ 1010.310449]  sd_mod hid_logitech_dj usbhid hid ahci libahci crc32c_intel 
> > libata xhci_pci xhci_hcd scsi_mod usbcore fan thermal
> > [ 1010.310464] CPU: 3 PID: 119 Comm: kworker/3:2 Not tainted 4.9.0-rc8+ #14
> > [ 1010.310466] Hardware name:  /NUC5i7RYB, BIOS 
> > RYBDWi35.86A.0350.2015.0812.1722 08/12/2015
> > [ 1010.310487] Workqueue: usb_hub_wq hub_event [usbcore]
> > [ 1010.310490]   848f56c5 8803b1f7f858 
> > 
> > [ 1010.310496]  8414f8f8 8803001f ed00763eff07 
> > 8803b1f7f8f0
> > [ 1010.310501]  8803b3ea1e60 0001 ffef 
> > 8803b45c6840
> > [ 1010.310505] Call Trace:
> > [ 1010.310517]  [] ? dump_stack+0x5c/0x77
> > [ 1010.310522]  [] ? __warn+0x168/0x1a0
> > [ 1010.310526]  [] ? warn_slowpath_fmt+0xb4/0xf0
> > [ 1010.310529]  [] ? __warn+0x1a0/0x1a0
> > [ 1010.310534]  [] ? kasan_kmalloc+0xa6/0xd0
> > [ 1010.310539]  [] ? kernfs_path_from_node+0x4a/0x60
> > [ 1010.310543]  [] ? sysfs_warn_dup+0x7b/0x90
> > [ 1010.310547]  [] ? 
> > sysfs_do_create_link_sd.isra.2+0xb6/0xd0
> > [ 1010.310553]  [] ? bus_add_device+0x318/0x6b0
> > [ 1010.310557]  [] ? sysfs_create_groups+0x83/0x110
> > [ 1010.310562]  [] ? device_add+0x777/0x1350
> > [ 1010.310567]  [] ? device_private_init+0x180/0x180
> > [ 1010.310583]  [] ? usb_new_device+0x707/0x1030 [usbcore]
> > [ 1010.310598]  [] ? hub_event+0x1d65/0x3280 [usbcore]
> > [ 1010.310604]  [] ? account_entity_dequeue+0x30b/0x4a0
> > [ 1010.310618]  [] ? hub_port_debounce+0x280/0x280 
> > [usbcore]
> > [ 1010.310624]  [] ? compat_start_thread+0x80/0x80
> > [ 1010.310629]  [] ? __schedule+0x704/0x1770
> > [ 1010.310633]  [] ? io_schedule_timeout+0x390/0x390
> > [ 1010.310638]  [] ? cache_reap+0x173/0x200
> > [ 1010.310642]  [] ? process_one_work+0x4ed/0xe60
> > [ 1010.310646]  [] ? worker_thread+0xe2/0xfd0
> > [ 1010.310650]  [] ? __wake_up_common+0xbc/0x160
> > [ 1010.310654]  [] ? process_one_work+0xe60/0xe60
> > [ 1010.310658]  [] ? kthread+0x1cc/0x220
> > [ 1010.310663]  [] ? kthread_park+0x80/0x80
> > [ 1010.310667]  [] ? kthread_park+0x80/0x80
> > [ 1010.310671]  [] ? kthread_park+0x80/0x80
> > [ 1010.310675]  [] ? ret_from_fork+0x25/0x30
> > [ 1010.310698] ---[ end trace 

Re: [PATCH v5 3/5] [media] davinci: vpif_capture: fix start/stop streaming locking

[Laurent Pinchart]

Hi Kevin,

Thank you for the patch.

On Tuesday 06 Dec 2016 21:08:24 Kevin Hilman wrote:
> Video capture subdevs may be over I2C and may sleep during xfer, so we
> cannot do IRQ-disabled locking when calling the subdev.
> 
> The IRQ-disabled locking is meant to protect the DMA queue list
> throughout the rest of the driver, so update the locking in
> [start|stop]_streaming to protect just this list.
> 
> Suggested-by: Laurent Pinchart 
> Signed-off-by: Kevin Hilman 

I would also add a comment to document the irqlock field as protecting the 
dma_queue list only. Something like

-   /* Used in video-buf */
+   /* Protects the dma_queue field */
spinlock_t irqlock;

With that,

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/platform/davinci/vpif_capture.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/davinci/vpif_capture.c
> b/drivers/media/platform/davinci/vpif_capture.c index
> c24049acd40a..f72a719efb3d 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -179,8 +179,6 @@ static int vpif_start_streaming(struct vb2_queue *vq,
> unsigned int count) unsigned long addr, flags;
>   int ret;
> 
> - spin_lock_irqsave(>irqlock, flags);
> -
>   /* Initialize field_id */
>   ch->field_id = 0;
> 
> @@ -211,6 +209,7 @@ static int vpif_start_streaming(struct vb2_queue *vq,
> unsigned int count) vpif_config_addr(ch, ret);
> 
>   /* Get the next frame from the buffer queue */
> + spin_lock_irqsave(>irqlock, flags);
>   common->cur_frm = common->next_frm = list_entry(common-
>dma_queue.next,
>   struct vpif_cap_buffer, list);
>   /* Remove buffer from the buffer queue */
> @@ -244,6 +243,7 @@ static int vpif_start_streaming(struct vb2_queue *vq,
> unsigned int count) return 0;
> 
>  err:
> + spin_lock_irqsave(>irqlock, flags);
>   list_for_each_entry_safe(buf, tmp, >dma_queue, list) {
>   list_del(>list);
>   vb2_buffer_done(>vb.vb2_buf, VB2_BUF_STATE_QUEUED);
> @@ -287,7 +287,6 @@ static void vpif_stop_streaming(struct vb2_queue *vq)
>   vpif_dbg(1, debug, "stream off failed in subdev\n");
> 
>   /* release all active buffers */
> - spin_lock_irqsave(>irqlock, flags);
>   if (common->cur_frm == common->next_frm) {
>   vb2_buffer_done(>cur_frm->vb.vb2_buf,
>   VB2_BUF_STATE_ERROR);
> @@ -300,6 +299,7 @@ static void vpif_stop_streaming(struct vb2_queue *vq)
>   VB2_BUF_STATE_ERROR);
>   }
> 
> + spin_lock_irqsave(>irqlock, flags);
>   while (!list_empty(>dma_queue)) {
>   common->next_frm = list_entry(common->dma_queue.next,
>   struct vpif_cap_buffer, list);

-- 
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: em28xx broken 4.9.0-rc6+

[Mauro Carvalho Chehab]

Em Tue, 6 Dec 2016 13:41:38 -0200
Mauro Carvalho Chehab  escreveu:

> Em Tue, 6 Dec 2016 01:06:17 +0200
> Antti Palosaari  escreveu:
> 
> > Hello Mauro
> > I just noticed current em28xx driver seem to be broken. When I plug 
> > device first time it loads correctly, but when I re-plug it, it does not 
> > work anymore but yells a lot of noise to message log. Tested with PCTV 
> > 290e and 292e both same. Other USB DVB devices are working so it is very 
> > likely em28xx driver bug.
> > 
> > Easy to reproduce:
> > plug device
> > unplug device
> > plug device
> 
> 
> Are you referring to those:
> 
> [ 1010.310320] WARNING: CPU: 3 PID: 119 at fs/sysfs/dir.c:31 
> sysfs_warn_dup+0x7b/0x90
> [ 1010.310323] sysfs: cannot create duplicate filename 
> '/bus/usb/devices/1-3.3'
> [ 1010.310325] Modules linked in: lgdt330x em28xx_dvb dvb_core em28xx_alsa 
> tuner_xc2028 tuner tvp5150 em28xx_v4l videobuf2_vmalloc videobuf2_memops 
> videobuf2_v4l2 videobuf2_core em28xx tveeprom v4l2_common videodev media 
> xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat 
> nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack 
> ipt_REJECT nf_reject_ipv4 xt_tcpudp tun bridge stp llc ebtable_filter 
> ebtables ip6table_filter ip6_tables iptable_filter ip_tables x_tables cmac 
> bnep cpufreq_powersave cpufreq_conservative cpufreq_userspace binfmt_misc 
> parport_pc ppdev lp parport snd_hda_codec_hdmi iTCO_wdt snd_hda_codec_realtek 
> iTCO_vendor_support snd_hda_codec_generic arc4 intel_rapl 
> x86_pkg_temp_thermal iwlmvm intel_powerclamp coretemp kvm_intel mac80211 kvm 
> i915
> [ 1010.310383]  irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel 
> iwlwifi pl2303 aesni_intel btusb aes_x86_64 usbserial lrw btrtl gf128mul 
> glue_helper btbcm ablk_helper cryptd btintel bluetooth drm_kms_helper 
> cfg80211 drm psmouse pcspkr i2c_i801 e1000e serio_raw snd_hda_intel 
> snd_soc_rt5640 snd_hda_codec snd_soc_rl6231 snd_soc_ssm4567 mei_me i2c_smbus 
> rfkill snd_hda_core ptp mei snd_soc_core ehci_pci sg lpc_ich shpchp mfd_core 
> ehci_hcd pps_core snd_hwdep i2c_algo_bit snd_compress snd_pcm sdhci_acpi 
> snd_timer battery snd sdhci elan_i2c snd_soc_sst_acpi mmc_core fjes dw_dmac 
> i2c_hid soundcore snd_soc_sst_match i2c_designware_platform video 
> i2c_designware_core acpi_pad acpi_als kfifo_buf tpm_tis button industrialio 
> tpm_tis_core tpm ext4 crc16 jbd2 fscrypto mbcache dm_mod joydev evdev 
> hid_logitech_hidpp
> [ 1010.310449]  sd_mod hid_logitech_dj usbhid hid ahci libahci crc32c_intel 
> libata xhci_pci xhci_hcd scsi_mod usbcore fan thermal
> [ 1010.310464] CPU: 3 PID: 119 Comm: kworker/3:2 Not tainted 4.9.0-rc8+ #14
> [ 1010.310466] Hardware name:  /NUC5i7RYB, BIOS 
> RYBDWi35.86A.0350.2015.0812.1722 08/12/2015
> [ 1010.310487] Workqueue: usb_hub_wq hub_event [usbcore]
> [ 1010.310490]   848f56c5 8803b1f7f858 
> 
> [ 1010.310496]  8414f8f8 8803001f ed00763eff07 
> 8803b1f7f8f0
> [ 1010.310501]  8803b3ea1e60 0001 ffef 
> 8803b45c6840
> [ 1010.310505] Call Trace:
> [ 1010.310517]  [] ? dump_stack+0x5c/0x77
> [ 1010.310522]  [] ? __warn+0x168/0x1a0
> [ 1010.310526]  [] ? warn_slowpath_fmt+0xb4/0xf0
> [ 1010.310529]  [] ? __warn+0x1a0/0x1a0
> [ 1010.310534]  [] ? kasan_kmalloc+0xa6/0xd0
> [ 1010.310539]  [] ? kernfs_path_from_node+0x4a/0x60
> [ 1010.310543]  [] ? sysfs_warn_dup+0x7b/0x90
> [ 1010.310547]  [] ? 
> sysfs_do_create_link_sd.isra.2+0xb6/0xd0
> [ 1010.310553]  [] ? bus_add_device+0x318/0x6b0
> [ 1010.310557]  [] ? sysfs_create_groups+0x83/0x110
> [ 1010.310562]  [] ? device_add+0x777/0x1350
> [ 1010.310567]  [] ? device_private_init+0x180/0x180
> [ 1010.310583]  [] ? usb_new_device+0x707/0x1030 [usbcore]
> [ 1010.310598]  [] ? hub_event+0x1d65/0x3280 [usbcore]
> [ 1010.310604]  [] ? account_entity_dequeue+0x30b/0x4a0
> [ 1010.310618]  [] ? hub_port_debounce+0x280/0x280 [usbcore]
> [ 1010.310624]  [] ? compat_start_thread+0x80/0x80
> [ 1010.310629]  [] ? __schedule+0x704/0x1770
> [ 1010.310633]  [] ? io_schedule_timeout+0x390/0x390
> [ 1010.310638]  [] ? cache_reap+0x173/0x200
> [ 1010.310642]  [] ? process_one_work+0x4ed/0xe60
> [ 1010.310646]  [] ? worker_thread+0xe2/0xfd0
> [ 1010.310650]  [] ? __wake_up_common+0xbc/0x160
> [ 1010.310654]  [] ? process_one_work+0xe60/0xe60
> [ 1010.310658]  [] ? kthread+0x1cc/0x220
> [ 1010.310663]  [] ? kthread_park+0x80/0x80
> [ 1010.310667]  [] ? kthread_park+0x80/0x80
> [ 1010.310671]  [] ? kthread_park+0x80/0x80
> [ 1010.310675]  [] ? ret_from_fork+0x25/0x30
> [ 1010.310698] ---[ end trace 49b46eb633ff1197 ]---
> [ 1010.311298] usb 1-3.3: can't device_add, error -17
> [ 1010.390703] usb 1-3.3: new high-speed USB device number 9 using xhci_hcd
> [ 1010.496337] usb 1-3.3: New USB device found, idVendor=2040, idProduct=6513
> [ 1010.496343] usb 1-3.3: New USB device strings: 

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

[Sakari Ailus]

Hi Kevin,

On Tue, Dec 06, 2016 at 11:50:58AM -0800, Kevin Hilman wrote:
> On Tue, Dec 6, 2016 at 9:40 AM, Kevin Hilman  wrote:
> > Hans Verkuil  writes:
> >
> >> On 12/01/2016 10:16 AM, Laurent Pinchart wrote:
> >>> Hello,
> >>>
> >>> On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:
>  On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
> > Sakari Ailus  writes:
> >> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> >>> Sakari Ailus  writes:
>  On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
> > 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 | 130 
> > +++-
> >  include/media/davinci/vpif_types.h
> >|   9 +-
> >  2 files changed, 133 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/platform/davinci/vpif_capture.c
> > b/drivers/media/platform/davinci/vpif_capture.c index
> > 94ee6cf03f02..47a4699157e7 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 
> 
>  Do you need this header?
> >>>
> >>> Yes, based on discussion with Hans, since there is no DT binding for
> >>> selecting the input pins of the TVP514x, I have to select it in the
> >>> driver, so I need the defines from this header.  More on this below...
> >>>
> >>> That's really ugly :-( The problem should be fixed properly instead of 
> >>> adding
> >>> one more offender.
> >>
> >> Do you have time for that, Laurent? I don't. Until that time we just need 
> >> to
> >> make do with this workaround.
> >>
> >>>
> >  #include "vpif.h"
> >  #include "vpif_capture.h"
> > @@ -650,6 +652,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;
> > @@ -657,7 +663,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;
> > @@ -1327,6 +1333,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;
> >>>
> >>> Can subdev_name be made const instead of blindly casting the full_name 
> >>> pointer
> >>> ? If not this is probably unsafe, and if yes it should be done :-)
> >>>
> > +  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)) {
> > @@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct
> > v4l2_async_notifier *notifier)
> >return vpif_probe_complete();
> >  }
> >
> 

Re: [PATCH v6 3/3] sound/usb: Use Media Controller API to share media resources

[Sakari Ailus]

Hi Shuah,

On Mon, Dec 05, 2016 at 05:38:23PM -0700, Shuah Khan wrote:
> On 12/05/2016 04:21 PM, Laurent Pinchart wrote:
> > Hi Shuah,
> > 
> > On Monday 05 Dec 2016 15:44:30 Shuah Khan wrote:
> >> On 11/30/2016 03:01 PM, Shuah Khan wrote:
> >>> Change ALSA driver to use Media Controller API to share media resources
> >>> with DVB, and V4L2 drivers on a AU0828 media device.
> >>>
> >>> Media Controller specific initialization is done after sound card is
> >>> registered. ALSA creates Media interface and entity function graph
> >>> nodes for Control, Mixer, PCM Playback, and PCM Capture devices.
> >>>
> >>> snd_usb_hw_params() will call Media Controller enable source handler
> >>> interface to request the media resource. If resource request is granted,
> >>> it will release it from snd_usb_hw_free(). If resource is busy, -EBUSY is
> >>> returned.
> >>>
> >>> Media specific cleanup is done in usb_audio_disconnect().
> >>>
> >>> Signed-off-by: Shuah Khan 
> >>
> >> Hi Takashi,
> >>
> >> If you are good with this patch, could you please Ack it, so Mauro
> >> can pull it into media tree with the other two patches in this series,
> >> when he is ready to do so.
> > 
> > I *really* want to address the concerns raised by Sakari before pulling 
> > more 
> > code that makes fixing the race conditions more difficult. Please, let's 
> > all 
> > work on fixing the core code to build a stable base on which we can build 
> > additional features. V4L2 and MC need teamwork, it's time to give the 
> > subsystem the love it deserves.
> > 
> 
> Hi Laurent,
> 
> The issue Sakari brought up is specific to using devm for video_device in
> omap3 and vsp1. I tried reproducing the problem on two different drivers
> and couldn't on Linux 4.9-rc7.
> 
> After sharing that with Sakari, I suggested to Sakari to pull up his patch
> that removes the devm usage and see if he still needs all the patches in his
> patch series. He didn't back to me on that. I also requested him to rebase on

Just to see what remains, I made a small hack to test this with omap3isp by
just replacing the devm_() functions by their plain counterparts. The memory
is thus never released, for there is no really a proper moment to release it
--- something which the patchset resolves. The result is here:



What happens there is that as part of the device unbinding, the graph
objects are unregistered (by media_device_unregister()) while streaming is
ongoing. Their parent media device pointers are set to NULL.

Then, when the user process exists, the streaming media pipeline is stopped.
This requires parsing of the media graph, a job which will obviously fail
miserably and immediately: the media device is obtained from the entity
where graph traversal is expected to begin.

Two mutexes are also acquired but they've been already destroyed at that
point (and the memory of which would also be released now without the hack
to test this "without devm"). There's just a single warning on this though.

> top of media dev allocator because the allocator routines he has don't address
> the shared media device need.

I do strongly prefer fixing the existing object lifetime issues in the
framework before extending it and thus making the problem domain more
complex than it is already.

What I mentioned as an example of this are the other callbacks to the media
device: presumably they do suffer from the exactly same problems as the
enable_source() and disable_source() ones.

As drivers do refer to other entities and controls they do expose also to
the user space, we can't really even remove entities safely at the moment.
We should have a solution to this as well, my patchset at the moment does
not address this.

What we do need a sound basis for the framework rather than hastily written
improvements to support a particular device. Also, testing device removal
with a particular device in a particular use case does not guarantee that no
further object lifetime issues related to device removal exist, even with
that same driver, let alone other devices.

Instead, we must consider how the frameworks and drivers manage the memory
allocated for the various objects, what are relations of those objects and
how they're exposed to the user space either directly or indirectly. As long
as objects (such as the media graph objects) with different lifetimes are
referred to from other objects without taking a reference or alternatively
serialising code paths that may access those objects and those that remove
them, we do have a problem.

The media graph with all the subsystem specific device nodes that are
exported to the user space is a rather complex data structure. I don't think
that acquiring the media graph lock in order to fix all the serialisation
problems is the right approach here.

> 
> He also didn't respond to my response regarding the reasons for choosing
> graph_mutex to protect enable_source and 

Re: [PATCH v4 00/13] Use sysfs filter for winbond & nuvoton wakeup

[Sean Young]

On Tue, Dec 06, 2016 at 10:19:08AM +, Sean Young wrote:
> This patch series resurrects an earlier series with a new approach.

I've discovered some bugs in this series. Protocol modules are not
autoloaded and rc-loopback and is missing the wakeup_protocols sysfs file.

Please treat this series as RFC while I fix the issues and do more testing.


Sean
--
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