| Message ID | 1457493972-4063-1-git-send-email-shuahkh@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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);
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(-)