* [PATCH 1/2] em28xx: Get rid of some unused modprobe parameters at vbi code @ 2014-09-19 16:02 Mauro Carvalho Chehab 2014-09-19 16:02 ` [PATCH 2/2] em28xx: fix VBI handling logic Mauro Carvalho Chehab 0 siblings, 1 reply; 7+ messages in thread From: Mauro Carvalho Chehab @ 2014-09-19 16:02 UTC (permalink / raw) Cc: Mauro Carvalho Chehab, Linux Media Mailing List, Mauro Carvalho Chehab There are two modprobe parameters for VBI that aren't used anywhere (one for debug, the other one related to the buffer size). Get rid of them! Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> diff --git a/drivers/media/usb/em28xx/em28xx-vbi.c b/drivers/media/usb/em28xx/em28xx-vbi.c index 6d7f657f6f55..34ee1e03a732 100644 --- a/drivers/media/usb/em28xx/em28xx-vbi.c +++ b/drivers/media/usb/em28xx/em28xx-vbi.c @@ -29,17 +29,6 @@ #include "em28xx.h" #include "em28xx-v4l.h" -static unsigned int vbibufs = 5; -module_param(vbibufs, int, 0644); -MODULE_PARM_DESC(vbibufs, "number of vbi buffers, range 2-32"); - -static unsigned int vbi_debug; -module_param(vbi_debug, int, 0644); -MODULE_PARM_DESC(vbi_debug, "enable debug messages [vbi]"); - -#define dprintk(level, fmt, arg...) if (vbi_debug >= level) \ - printk(KERN_DEBUG "%s: " fmt, dev->core->name , ## arg) - /* ------------------------------------------------------------------ */ static int vbi_queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt, -- 1.9.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] em28xx: fix VBI handling logic 2014-09-19 16:02 [PATCH 1/2] em28xx: Get rid of some unused modprobe parameters at vbi code Mauro Carvalho Chehab @ 2014-09-19 16:02 ` Mauro Carvalho Chehab 2014-09-23 19:32 ` Frank Schäfer 0 siblings, 1 reply; 7+ messages in thread From: Mauro Carvalho Chehab @ 2014-09-19 16:02 UTC (permalink / raw) Cc: Mauro Carvalho Chehab, Linux Media Mailing List, Mauro Carvalho Chehab When both VBI and video are streaming, and video stream is stopped, a subsequent trial to restart it will fail, because S_FMT will return -EBUSY. That prevents applications like zvbi to work properly. Please notice that, while this fix it fully for zvbi, the best is to get rid of streaming_users and res_get logic as a hole. However, this single-line patch is better to be merged at -stable. Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c index 08569cbccd95..d75e7f82dfb9 100644 --- a/drivers/media/usb/em28xx/em28xx-video.c +++ b/drivers/media/usb/em28xx/em28xx-video.c @@ -1351,7 +1351,7 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *priv, struct em28xx *dev = video_drvdata(file); struct em28xx_v4l2 *v4l2 = dev->v4l2; - if (v4l2->streaming_users > 0) + if (vb2_is_busy(&v4l2->vb_vidq)) return -EBUSY; vidioc_try_fmt_vid_cap(file, priv, f); -- 1.9.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] em28xx: fix VBI handling logic 2014-09-19 16:02 ` [PATCH 2/2] em28xx: fix VBI handling logic Mauro Carvalho Chehab @ 2014-09-23 19:32 ` Frank Schäfer 2014-09-23 23:18 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 7+ messages in thread From: Frank Schäfer @ 2014-09-23 19:32 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Linux Media Mailing List, Mauro Carvalho Chehab Am 19.09.2014 um 18:02 schrieb Mauro Carvalho Chehab: > When both VBI and video are streaming, and video stream is stopped, > a subsequent trial to restart it will fail, because S_FMT will > return -EBUSY. > > That prevents applications like zvbi to work properly. > > Please notice that, while this fix it fully for zvbi, the > best is to get rid of streaming_users and res_get logic as a hole. > > However, this single-line patch is better to be merged at -stable. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c > index 08569cbccd95..d75e7f82dfb9 100644 > --- a/drivers/media/usb/em28xx/em28xx-video.c > +++ b/drivers/media/usb/em28xx/em28xx-video.c > @@ -1351,7 +1351,7 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *priv, > struct em28xx *dev = video_drvdata(file); > struct em28xx_v4l2 *v4l2 = dev->v4l2; > > - if (v4l2->streaming_users > 0) > + if (vb2_is_busy(&v4l2->vb_vidq)) Looks dangerous. Are you 100% sure that VIDIOC_S_FMT can have no effect on VBI capturing ? It seems to trigger writes to multiple registers... Regards, Frank > return -EBUSY; > > vidioc_try_fmt_vid_cap(file, priv, f); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] em28xx: fix VBI handling logic 2014-09-23 19:32 ` Frank Schäfer @ 2014-09-23 23:18 ` Mauro Carvalho Chehab 2014-09-25 13:59 ` Frank Schäfer 0 siblings, 1 reply; 7+ messages in thread From: Mauro Carvalho Chehab @ 2014-09-23 23:18 UTC (permalink / raw) To: Frank Schäfer; +Cc: Linux Media Mailing List, Mauro Carvalho Chehab Em Tue, 23 Sep 2014 21:32:02 +0200 Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > > Am 19.09.2014 um 18:02 schrieb Mauro Carvalho Chehab: > > When both VBI and video are streaming, and video stream is stopped, > > a subsequent trial to restart it will fail, because S_FMT will > > return -EBUSY. > > > > That prevents applications like zvbi to work properly. > > > > Please notice that, while this fix it fully for zvbi, the > > best is to get rid of streaming_users and res_get logic as a hole. > > > > However, this single-line patch is better to be merged at -stable. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > > > > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c > > index 08569cbccd95..d75e7f82dfb9 100644 > > --- a/drivers/media/usb/em28xx/em28xx-video.c > > +++ b/drivers/media/usb/em28xx/em28xx-video.c > > @@ -1351,7 +1351,7 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *priv, > > struct em28xx *dev = video_drvdata(file); > > struct em28xx_v4l2 *v4l2 = dev->v4l2; > > > > - if (v4l2->streaming_users > 0) > > + if (vb2_is_busy(&v4l2->vb_vidq)) > Looks dangerous. Why Dangerous? Did you identify any problem? With what application? Regards, Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] em28xx: fix VBI handling logic 2014-09-23 23:18 ` Mauro Carvalho Chehab @ 2014-09-25 13:59 ` Frank Schäfer 2014-09-25 14:07 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 7+ messages in thread From: Frank Schäfer @ 2014-09-25 13:59 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Linux Media Mailing List, Mauro Carvalho Chehab Am 24.09.2014 um 01:18 schrieb Mauro Carvalho Chehab: > Em Tue, 23 Sep 2014 21:32:02 +0200 > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > >> Am 19.09.2014 um 18:02 schrieb Mauro Carvalho Chehab: >>> When both VBI and video are streaming, and video stream is stopped, >>> a subsequent trial to restart it will fail, because S_FMT will >>> return -EBUSY. >>> >>> That prevents applications like zvbi to work properly. >>> >>> Please notice that, while this fix it fully for zvbi, the >>> best is to get rid of streaming_users and res_get logic as a hole. >>> >>> However, this single-line patch is better to be merged at -stable. >>> >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> >>> >>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c >>> index 08569cbccd95..d75e7f82dfb9 100644 >>> --- a/drivers/media/usb/em28xx/em28xx-video.c >>> +++ b/drivers/media/usb/em28xx/em28xx-video.c >>> @@ -1351,7 +1351,7 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *priv, >>> struct em28xx *dev = video_drvdata(file); >>> struct em28xx_v4l2 *v4l2 = dev->v4l2; >>> >>> - if (v4l2->streaming_users > 0) >>> + if (vb2_is_busy(&v4l2->vb_vidq)) >> Looks dangerous. > Why Dangerous? You are an experienced kernel developer. If you still fail to see that after so many years, sorry, I can't help you. > Did you identify any problem? Yes I've identified a potential problem. Read it again, it's in the line you skipped in this reply. > With what application? I would be willing to spend more time on this and test this critical patch (like I already did in the past). But I don't have access to analog TV here at the moment, sorry. It would have to wait ~2 weeks. My fault. I shouldn't have wasted any time on reviewing your patch. Regards, Frank ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] em28xx: fix VBI handling logic 2014-09-25 13:59 ` Frank Schäfer @ 2014-09-25 14:07 ` Mauro Carvalho Chehab 2014-09-25 14:26 ` Frank Schäfer 0 siblings, 1 reply; 7+ messages in thread From: Mauro Carvalho Chehab @ 2014-09-25 14:07 UTC (permalink / raw) To: Frank Schäfer; +Cc: Linux Media Mailing List, Mauro Carvalho Chehab Em Thu, 25 Sep 2014 15:59:12 +0200 Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > > Am 24.09.2014 um 01:18 schrieb Mauro Carvalho Chehab: > > Em Tue, 23 Sep 2014 21:32:02 +0200 > > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > > > >> Am 19.09.2014 um 18:02 schrieb Mauro Carvalho Chehab: > >>> When both VBI and video are streaming, and video stream is stopped, > >>> a subsequent trial to restart it will fail, because S_FMT will > >>> return -EBUSY. > >>> > >>> That prevents applications like zvbi to work properly. > >>> > >>> Please notice that, while this fix it fully for zvbi, the > >>> best is to get rid of streaming_users and res_get logic as a hole. > >>> > >>> However, this single-line patch is better to be merged at -stable. > >>> > >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > >>> > >>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c > >>> index 08569cbccd95..d75e7f82dfb9 100644 > >>> --- a/drivers/media/usb/em28xx/em28xx-video.c > >>> +++ b/drivers/media/usb/em28xx/em28xx-video.c > >>> @@ -1351,7 +1351,7 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *priv, > >>> struct em28xx *dev = video_drvdata(file); > >>> struct em28xx_v4l2 *v4l2 = dev->v4l2; > >>> > >>> - if (v4l2->streaming_users > 0) > >>> + if (vb2_is_busy(&v4l2->vb_vidq)) > >> Looks dangerous. > > Why Dangerous? > You are an experienced kernel developer. If you still fail to see that > after so many years, sorry, I can't help you. > > > Did you identify any problem? > Yes I've identified a potential problem. > Read it again, it's in the line you skipped in this reply. Did you read the video_ioctl_ops struct? See: .vidioc_g_fmt_vid_cap = vidioc_g_fmt_vid_cap, .vidioc_try_fmt_vid_cap = vidioc_try_fmt_vid_cap, .vidioc_s_fmt_vid_cap = vidioc_s_fmt_vid_cap, .vidioc_g_fmt_vbi_cap = vidioc_g_fmt_vbi_cap, .vidioc_try_fmt_vbi_cap = vidioc_g_fmt_vbi_cap, .vidioc_s_fmt_vbi_cap = vidioc_g_fmt_vbi_cap, .vidioc_enum_framesizes = vidioc_enum_framesizes, .vidioc_g_audio = vidioc_g_audio, .vidioc_s_audio = vidioc_s_audio, Only the video node calls s_fmt_vid_cap. There's nothing to be set for VBI. In other words, a call to VIDIOC_S_FMT will actually call vidioc_g_fmt_vbi_cap() if called from VBI. So, I was unable to see any potencial issues. > > With what application? > I would be willing to spend more time on this and test this critical > patch (like I already did in the past). > But I don't have access to analog TV here at the moment, sorry. It would > have to wait ~2 weeks. On my tests with the 3 existing VBI apps, they all worked fine after the patch, no matter if the VBI or the video application is started first. That's why I asked. Regards, Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] em28xx: fix VBI handling logic 2014-09-25 14:07 ` Mauro Carvalho Chehab @ 2014-09-25 14:26 ` Frank Schäfer 0 siblings, 0 replies; 7+ messages in thread From: Frank Schäfer @ 2014-09-25 14:26 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Linux Media Mailing List, Mauro Carvalho Chehab (Resending because my crappy modem crashed while sending) Am 25.09.2014 um 16:07 schrieb Mauro Carvalho Chehab: > Em Thu, 25 Sep 2014 15:59:12 +0200 > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > >> Am 24.09.2014 um 01:18 schrieb Mauro Carvalho Chehab: >>> Em Tue, 23 Sep 2014 21:32:02 +0200 >>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: >>> >>>> Am 19.09.2014 um 18:02 schrieb Mauro Carvalho Chehab: >>>>> When both VBI and video are streaming, and video stream is stopped, >>>>> a subsequent trial to restart it will fail, because S_FMT will >>>>> return -EBUSY. >>>>> >>>>> That prevents applications like zvbi to work properly. >>>>> >>>>> Please notice that, while this fix it fully for zvbi, the >>>>> best is to get rid of streaming_users and res_get logic as a hole. >>>>> >>>>> However, this single-line patch is better to be merged at -stable. >>>>> >>>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> >>>>> >>>>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c >>>>> index 08569cbccd95..d75e7f82dfb9 100644 >>>>> --- a/drivers/media/usb/em28xx/em28xx-video.c >>>>> +++ b/drivers/media/usb/em28xx/em28xx-video.c >>>>> @@ -1351,7 +1351,7 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *priv, >>>>> struct em28xx *dev = video_drvdata(file); >>>>> struct em28xx_v4l2 *v4l2 = dev->v4l2; >>>>> >>>>> - if (v4l2->streaming_users > 0) >>>>> + if (vb2_is_busy(&v4l2->vb_vidq)) >>>> Looks dangerous. >>> Why Dangerous? >> You are an experienced kernel developer. If you still fail to see that >> after so many years, sorry, I can't help you. >> >>> Did you identify any problem? >> Yes I've identified a potential problem. >> Read it again, it's in the line you skipped in this reply. > Did you read the video_ioctl_ops struct? > > See: > > .vidioc_g_fmt_vid_cap = vidioc_g_fmt_vid_cap, > .vidioc_try_fmt_vid_cap = vidioc_try_fmt_vid_cap, > .vidioc_s_fmt_vid_cap = vidioc_s_fmt_vid_cap, > .vidioc_g_fmt_vbi_cap = vidioc_g_fmt_vbi_cap, > .vidioc_try_fmt_vbi_cap = vidioc_g_fmt_vbi_cap, > .vidioc_s_fmt_vbi_cap = vidioc_g_fmt_vbi_cap, > .vidioc_enum_framesizes = vidioc_enum_framesizes, > .vidioc_g_audio = vidioc_g_audio, > .vidioc_s_audio = vidioc_s_audio, > > Only the video node calls s_fmt_vid_cap. There's nothing to be > set for VBI. In other words, a call to VIDIOC_S_FMT will > actually call vidioc_g_fmt_vbi_cap() if called from VBI. > > So, I was unable to see any potencial issues. Yes, I checked that, too. No problems here. I was concerned about the||consequences of the em28xx_set_video_format||() call. It calls em28xx_resolution_set() which in turn calls em28xx_set_outfmt() em28xx_accumulator_set() em28xx_capture_area_set()|| em28xx_scaler_set() So lot's of device registers are reconfigured. That might cause problems even if the register values don't change. If that has been verified and it causes no problems, than of course everything is fine. Regards, Frank > >>> With what application? >> I would be willing to spend more time on this and test this critical >> patch (like I already did in the past). >> But I don't have access to analog TV here at the moment, sorry. It would >> have to wait ~2 weeks. > On my tests with the 3 existing VBI apps, they all worked fine > after the patch, no matter if the VBI or the video application > is started first. > > That's why I asked. > > Regards, > Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-09-25 14:25 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-19 16:02 [PATCH 1/2] em28xx: Get rid of some unused modprobe parameters at vbi code Mauro Carvalho Chehab 2014-09-19 16:02 ` [PATCH 2/2] em28xx: fix VBI handling logic Mauro Carvalho Chehab 2014-09-23 19:32 ` Frank Schäfer 2014-09-23 23:18 ` Mauro Carvalho Chehab 2014-09-25 13:59 ` Frank Schäfer 2014-09-25 14:07 ` Mauro Carvalho Chehab 2014-09-25 14:26 ` Frank Schäfer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).