[PATCH] Revert "[media] au0828: use v4l2_mc_create_media_graph()"

[PATCH] Revert "[media] au0828: use v4l2_mc_create_media_graph()"

[Shuah Khan]

This reverts commit 9822f4173f84cb7c592edb5e1478b7903f69d018.
This commit breaks au0828_enable_handler() logic to find the tuner.
Audio, Video, and Digital applications are broken and fail to start
streaming with tuner busy error even when tuner is free.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/usb/au0828/au0828-video.c | 103 ++++++++++++++++++++++++++++++--
 drivers/media/v4l2-core/v4l2-mc.c       |  21 +------
 2 files changed, 99 insertions(+), 25 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c
index 13f6dab..5f7c8be 100644
--- a/drivers/media/usb/au0828/au0828-video.c
+++ b/drivers/media/usb/au0828/au0828-video.c
@@ -35,7 +35,6 @@
 #include <linux/init.h>
 #include <linux/device.h>
 #include <media/v4l2-common.h>
-#include <media/v4l2-mc.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-event.h>
 #include <media/tuner.h>
@@ -653,6 +652,102 @@ void au0828_usb_v4l2_media_release(struct au0828_dev *dev)
 #endif
 }
 
+static int au0828_create_media_graph(struct au0828_dev *dev)
+{
+#ifdef CONFIG_MEDIA_CONTROLLER
+	struct media_device *mdev = dev->media_dev;
+	struct media_entity *entity;
+	struct media_entity *tuner = NULL, *decoder = NULL, *demod = NULL;
+	int i, ret;
+
+	if (!mdev)
+		return 0;
+
+	media_device_for_each_entity(entity, mdev) {
+		switch (entity->function) {
+		case MEDIA_ENT_F_TUNER:
+			tuner = entity;
+			break;
+		case MEDIA_ENT_F_ATV_DECODER:
+			decoder = entity;
+			break;
+		case MEDIA_ENT_F_DTV_DEMOD:
+			demod = entity;
+			break;
+		}
+	}
+
+	/* Analog setup, using tuner as a link */
+
+	/* Something bad happened! */
+	if (!decoder)
+		return -EINVAL;
+
+	if (tuner) {
+		dev->tuner = tuner;
+		ret = media_create_pad_link(tuner, TUNER_PAD_OUTPUT,
+					    decoder, DEMOD_PAD_IF_INPUT, 0);
+		if (ret)
+			return ret;
+	}
+	ret = media_create_pad_link(decoder, DEMOD_PAD_VID_OUT,
+				    &dev->vdev.entity, 0,
+				    MEDIA_LNK_FL_ENABLED);
+	if (ret)
+		return ret;
+	ret = media_create_pad_link(decoder, DEMOD_PAD_VBI_OUT,
+				    &dev->vbi_dev.entity, 0,
+				    MEDIA_LNK_FL_ENABLED);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < AU0828_MAX_INPUT; i++) {
+		struct media_entity *ent = &dev->input_ent[i];
+
+		switch (AUVI_INPUT(i).type) {
+		case AU0828_VMUX_UNDEFINED:
+			break;
+		case AU0828_VMUX_CABLE:
+		case AU0828_VMUX_TELEVISION:
+		case AU0828_VMUX_DVB:
+			if (!tuner)
+				break;
+
+			ret = media_create_pad_link(ent, 0, tuner,
+						    TUNER_PAD_RF_INPUT,
+						    MEDIA_LNK_FL_ENABLED);
+			if (ret)
+				return ret;
+			break;
+		case AU0828_VMUX_COMPOSITE:
+		case AU0828_VMUX_SVIDEO:
+			/* FIXME: fix the decoder PAD */
+			ret = media_create_pad_link(ent, 0, decoder,
+						    DEMOD_PAD_IF_INPUT, 0);
+			if (ret)
+				return ret;
+			break;
+		}
+	}
+
+	/*
+	 * Disable tuner to demod link to avoid disable step
+	 * when tuner is requested by video or audio
+	*/
+	if (tuner && demod) {
+		struct media_link *link;
+
+		list_for_each_entry(link, &demod->links, list) {
+			if (link->sink->entity == demod &&
+			    link->source->entity == tuner) {
+				media_entity_setup_link(link, 0);
+			}
+		}
+	}
+#endif
+	return 0;
+}
+
 static void au0828_usb_v4l2_release(struct v4l2_device *v4l2_dev)
 {
 	struct au0828_dev *dev =
@@ -2039,16 +2134,14 @@ int au0828_analog_register(struct au0828_dev *dev,
 		ret = -ENODEV;
 		goto err_reg_vbi_dev;
 	}
-
-#ifdef CONFIG_MEDIA_CONTROLLER
-	retval = v4l2_mc_create_media_graph(dev->media_dev);
+	retval = au0828_create_media_graph(dev);
 	if (retval) {
 		pr_err("%s() au0282_dev_register failed to create graph\n",
 			__func__);
 		ret = -ENODEV;
 		goto err_reg_vbi_dev;
 	}
-#endif
+
 
 	dprintk(1, "%s completed!\n", __func__);
 
diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
index f8b6e3b..ae661ac 100644
--- a/drivers/media/v4l2-core/v4l2-mc.c
+++ b/drivers/media/v4l2-core/v4l2-mc.c
@@ -34,7 +34,7 @@ int v4l2_mc_create_media_graph(struct media_device *mdev)
 {
 	struct media_entity *entity;
 	struct media_entity *if_vid = NULL, *if_aud = NULL;
-	struct media_entity *tuner = NULL, *decoder = NULL, *dtv_demod = NULL;
+	struct media_entity *tuner = NULL, *decoder = NULL;
 	struct media_entity *io_v4l = NULL, *io_vbi = NULL, *io_swradio = NULL;
 	bool is_webcam = false;
 	u32 flags;
@@ -57,9 +57,6 @@ int v4l2_mc_create_media_graph(struct media_device *mdev)
 		case MEDIA_ENT_F_ATV_DECODER:
 			decoder = entity;
 			break;
-		case MEDIA_ENT_F_DTV_DEMOD:
-			dtv_demod = entity;
-			break;
 		case MEDIA_ENT_F_IO_V4L:
 			io_v4l = entity;
 			break;
@@ -193,22 +190,6 @@ int v4l2_mc_create_media_graph(struct media_device *mdev)
 
 		flags = 0;
 	}
-
-	/*
-	 * Disable tuner to demod link to avoid disable step
-	 * when tuner is requested by video or audio
-	 */
-	if (tuner && dtv_demod) {
-		struct media_link *link;
-
-		list_for_each_entry(link, &dtv_demod->links, list) {
-			if (link->sink->entity == dtv_demod &&
-			    link->source->entity == tuner) {
-				media_entity_setup_link(link, 0);
-			}
-		}
-	}
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_mc_create_media_graph);
-- 
2.5.0

Re: [PATCH] Revert "[media] au0828: use v4l2_mc_create_media_graph()"

[Shuah Khan]

On 03/08/2016 08:26 PM, Shuah Khan wrote:
> This reverts commit 9822f4173f84cb7c592edb5e1478b7903f69d018.
> This commit breaks au0828_enable_handler() logic to find the tuner.
> Audio, Video, and Digital applications are broken and fail to start
> streaming with tuner busy error even when tuner is free.
> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/usb/au0828/au0828-video.c | 103 ++++++++++++++++++++++++++++++--
>  drivers/media/v4l2-core/v4l2-mc.c       |  21 +------
>  2 files changed, 99 insertions(+), 25 deletions(-)
> 

Hi Mauro,

Please pull this revert in as soon as possible. Without
the revert, auido, video, and digital applications won't
start even. There is a bug in the common routine introduced
in the commit 9822f4173f84cb7c592edb5e1478b7903f69d018 which
causes the link between source and sink to be not found.
I am testing on WIn-TV HVR 950Q

Here is my test sequence I use to verify the mutual exclusion
works between audio, video, and dvb applications.

Generate media graph graph - verify graph looks good.

Basic testing:
Test case 1:
Step 1: Start arecord and generate graph to verify the right
source is enabled
Step 2: Exit arecord and generate graph and verify resource is released.

Test case 2:
Step 1: Start kaffeine and generate graph to verify the right
source is enabled
Step 2: Exit kaffeine - generate graph and verify resource is released.

Test case 3:
Step 1: Start xawtv and generate graph to verify the right
source is enabled
Step 2: Exit xawtv - generate graph and verify resource is released.

Mutual exclusion testing:
Test case 1:
Step 1: Start arecord and generate graph to verify the right
source is enabled
Step 2: Start kaffeine - should see it fail to EBUSY
Step 3: Start xawtv - should see it fail with EBUSY
Step 4: Exit arecord - generate graph and verify resource is released.

Test Case 2:
Step 1: Start kaffeine and generate graph to verify the right
source is enabled
Step 2: Start arecord - it should fail - unable to set hwparams - device busy error
Step 3: Start xawtv - should see it fail with EBUSY
Step 4: Exit kaffeine - generate graph and verify resource is released.

Test Case 3:
Step 1: Start xawtv and generate graph to verify the right
source is enabled
Step 2: Start arecord - it should fail - device busy error
Step 3: Start kaffeine should see it fail with EBUSY
Step 4: Exit xawtv - generate graph and verify resource is released.

At each step make dmesg looks good. I build my kernel with kasan
enabled - to detect out of bounds access etc. good stuff.

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH] Revert "[media] au0828: use v4l2_mc_create_media_graph()"

[Mauro Carvalho Chehab]

Em Thu, 10 Mar 2016 09:16:30 -0700
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> On 03/08/2016 08:26 PM, Shuah Khan wrote:
> > This reverts commit 9822f4173f84cb7c592edb5e1478b7903f69d018.
> > This commit breaks au0828_enable_handler() logic to find the tuner.
> > Audio, Video, and Digital applications are broken and fail to start
> > streaming with tuner busy error even when tuner is free.
> > 
> > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> > ---
> >  drivers/media/usb/au0828/au0828-video.c | 103 ++++++++++++++++++++++++++++++--
> >  drivers/media/v4l2-core/v4l2-mc.c       |  21 +------
> >  2 files changed, 99 insertions(+), 25 deletions(-)
> >   
> 
> Hi Mauro,
> 
> Please pull this revert in as soon as possible. Without
> the revert, auido, video, and digital applications won't
> start even. There is a bug in the common routine introduced
> in the commit 9822f4173f84cb7c592edb5e1478b7903f69d018 which
> causes the link between source and sink to be not found.
> I am testing on WIn-TV HVR 950Q

No, this patch didn't seem to have broken anything. The problems
you're reporting seem to be related, instead, to this patch:

	https://patchwork.linuxtv.org/patch/33422/

I rebased it on the top of the master tree (without reverting this
patch).

Please check if it solved for you.

Yet, I'm seeing several troubles with au0828 after your patch series:

1) when both snd-usb-audio and au0828 are compiled as module and not
blacklisted, I'm getting some errors:
	http://pastebin.com/nMzr3ueM

2) removing/reprobing au0828 driver ~3 times, the Kernel becomes
unstable. Probably, some kobj ref were decremented every time a
module insert/removal pair is called from userspace, causing the
kref to reach zero, thus causing the trouble;

3) the media entities that should have been created by
media_snd_stream_init() are never created. Maybe this is related
with (1).

Please check.

Regards,
Mauro

Re: [PATCH] Revert "[media] au0828: use v4l2_mc_create_media_graph()"

[Shuah Khan]

On 03/10/2016 10:53 AM, Mauro Carvalho Chehab wrote:
> Em Thu, 10 Mar 2016 09:16:30 -0700
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> On 03/08/2016 08:26 PM, Shuah Khan wrote:
>>> This reverts commit 9822f4173f84cb7c592edb5e1478b7903f69d018.
>>> This commit breaks au0828_enable_handler() logic to find the tuner.
>>> Audio, Video, and Digital applications are broken and fail to start
>>> streaming with tuner busy error even when tuner is free.
>>>
>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>>> ---
>>>  drivers/media/usb/au0828/au0828-video.c | 103 ++++++++++++++++++++++++++++++--
>>>  drivers/media/v4l2-core/v4l2-mc.c       |  21 +------
>>>  2 files changed, 99 insertions(+), 25 deletions(-)
>>>   
>>
>> Hi Mauro,
>>
>> Please pull this revert in as soon as possible. Without
>> the revert, auido, video, and digital applications won't
>> start even. There is a bug in the common routine introduced
>> in the commit 9822f4173f84cb7c592edb5e1478b7903f69d018 which
>> causes the link between source and sink to be not found.
>> I am testing on WIn-TV HVR 950Q
> 
> No, this patch didn't seem to have broken anything. The problems
> you're reporting seem to be related, instead, to this patch:
> 
> 	https://patchwork.linuxtv.org/patch/33422/
> 
> I rebased it on the top of the master tree (without reverting this
> patch).

I don't think so. I sent https://patchwork.linuxtv.org/patch/33422/
after I did the revert. I tested with linux_media branch with this
top commit:

commit de08b5a8be0df1eb7c796b0fe6b30cf1d03d14a6
Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Date:   Tue Mar 1 06:31:53 2016 -0300

when I found the problem and reverting the commit 
9822f4173f84cb7c592edb5e1478b7903f69d018 - solved the proble,

Could you please test with and without the revert.

> 
> Please check if it solved for you.
> 
> Yet, I'm seeing several troubles with au0828 after your patch series:
> 
> 1) when both snd-usb-audio and au0828 are compiled as module and not
> blacklisted, I'm getting some errors:
> 	http://pastebin.com/nMzr3ueM

Also without the good graph in place, you won't see the problem
I am talking about with 9822f4173f84cb7c592edb5e1478b7903f69d018

> 
> 2) removing/reprobing au0828 driver ~3 times, the Kernel becomes
> unstable. Probably, some kobj ref were decremented every time a
> module insert/removal pair is called from userspace, causing the
> kref to reach zero, thus causing the trouble;

I answered this in detail on the other thread. Cutting and pasting
from that thread:

>>I'm also getting some other weird behavior when removing/reinserting
>>the modules a few times. OK, officially we don't support it, but,
>>as devices can be bind/unbind all the times, removing modules is
>>a way to simulate such things. Also, I use it a lot while testing
>>USB drivers ;)

>>This one is after removing both the media drivers and snd-usb-audio, 
>>and then modprobing snd-usb-audio:

I did see some issues when I did the following sequence:

- blacklisted au0828 and snd-usb-audio was probed first
  graph is good just with audio entities as expected
- modprobed au0828 - graph looks good.
- rmmod au0828 - no problems seen in dmesg
- modprobe au0828 - problems kasan reports bad access etc.
  http://pastebin.com/FFqNzx9G

Here is what's going on after each step:

blacklisted au0828 and snd-usb-audio was probed first
1. snd-usb-audio creates media device and registers it
   Creates its graph etc.
2. modprobed au0828
   au0828 finds the media device created and registered.
   Adds its graph
3. rmmod au0828
   Even though there are no problems reported, at this
   time media device is unregistered, and /dev/mediaX is
   removed. We still have snd_usb-audio and media device.
   As media device is a device resource on usb device parent,
   it will still be there, but no way to access the device
   itself, because it is no longer registered.
4. modprobe au0828
   At this point, au0828 finds the media device as it still
   there, registers it and adds its graph. No audio graph
   present at this time.

Please note that the media device will not be deleted until
the last put on the parent usb struct device. So even when
both snd-usb-audio, and au0828 modules are removed, media
device is still there without its graph and associated devnode
(/dev/mediax is removed).

This isn't bad, however, media_device could still have
stale information.

e.g: enable/disable handlers - when au0828 is removed,
these are no longer valid. Could be cleaned up in
media device unregister just like entity_notify() handler
gets deleted from media device unregister. At the moment,
either driver can call unregister and same cleanup happens.

I will send a patch to do enable/disable handler cleanup
in unregister path.

However, the root of the problem is media device is
still there without its graph and associated devnode
(/dev/mediax is removed) when any one of the drivers
is removed. This leaves the remaining drivers in a
degenerate state.

The problem can be solved with some handshaking at
unregister time. We could add a callback for each
if the drivers to handle media device unregister.
However, that would add delays in device removal path
when all the drivers exit. I think it will be hard to
handle all the corner cases without adding run-time overhead.

Any thoughts on whether we want to unofficially support
being able to remove individual drivers?

> 
> 3) the media entities that should have been created by
> media_snd_stream_init() are never created. Maybe this is related
> with (1).
> 

thanks,
-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH] Revert "[media] au0828: use v4l2_mc_create_media_graph()"

[Shuah Khan]

On 03/10/2016 11:05 AM, Shuah Khan wrote:
> On 03/10/2016 10:53 AM, Mauro Carvalho Chehab wrote:
>> Em Thu, 10 Mar 2016 09:16:30 -0700
>> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
>>
>>> On 03/08/2016 08:26 PM, Shuah Khan wrote:
>>>> This reverts commit 9822f4173f84cb7c592edb5e1478b7903f69d018.
>>>> This commit breaks au0828_enable_handler() logic to find the tuner.
>>>> Audio, Video, and Digital applications are broken and fail to start
>>>> streaming with tuner busy error even when tuner is free.
>>>>
>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>>>> ---
>>>>  drivers/media/usb/au0828/au0828-video.c | 103 ++++++++++++++++++++++++++++++--
>>>>  drivers/media/v4l2-core/v4l2-mc.c       |  21 +------
>>>>  2 files changed, 99 insertions(+), 25 deletions(-)
>>>>   
>>>
>>> Hi Mauro,
>>>
>>> Please pull this revert in as soon as possible. Without
>>> the revert, auido, video, and digital applications won't
>>> start even. There is a bug in the common routine introduced
>>> in the commit 9822f4173f84cb7c592edb5e1478b7903f69d018 which
>>> causes the link between source and sink to be not found.
>>> I am testing on WIn-TV HVR 950Q
>>
>> No, this patch didn't seem to have broken anything. The problems
>> you're reporting seem to be related, instead, to this patch:
>>
>> 	https://patchwork.linuxtv.org/patch/33422/
>>
>> I rebased it on the top of the master tree (without reverting this
>> patch).
> 
> I don't think so. I sent https://patchwork.linuxtv.org/patch/33422/
> after I did the revert. I tested with linux_media branch with this
> top commit:
> 
> commit de08b5a8be0df1eb7c796b0fe6b30cf1d03d14a6
> Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Date:   Tue Mar 1 06:31:53 2016 -0300
> 
> when I found the problem and reverting the commit 
> 9822f4173f84cb7c592edb5e1478b7903f69d018 - solved the proble,
> 
> Could you please test with and without the revert.

ok now that we determined in our offline discussions that
the revert does solve the problem and that there is an issue
with the core routine, we can try to debug the core common
routine with the revert in place.

> 
>>
>> Please check if it solved for you.
>>
>> Yet, I'm seeing several troubles with au0828 after your patch series:
>>
>> 1) when both snd-usb-audio and au0828 are compiled as module and not
>> blacklisted, I'm getting some errors:
>> 	http://pastebin.com/nMzr3ueM

I am looking into this problem. I can't reproduce this
on my system. It would be good know the sequence that
led to this. 

I see in the above pastebin:

BUG: sleeping function called from invalid context at mm/slub.c:1289

Resulting from the kzalloc(sizeof(*link), GFP_KERNEL);

In this case media_add_link() is trying to allocate memory
within  spinlock context. media_device_register_entity()
holds mdev->lock.

I think we added this spinlock for a reason. How do we 
handle this case? Can we use GFP_ATOMIC here conditionally?

thanks,
-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978