lkml.org 
[lkml]   [2008]   [May]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] video4linux: Push down the BKL
From
Date
On Tue, 2008-05-27 at 18:00 -0300, Mauro Carvalho Chehab wrote:
> Hi Devin,
>
> On Tue, 27 May 2008 15:26:27 -0400
> "Devin Heitmueller" <devin.heitmueller@gmail.com> wrote:
>
> > Hello Mauro,
> >
> > On Tue, May 27, 2008 at 2:59 PM, Mauro Carvalho Chehab
> > <mchehab@infradead.org> wrote:
> > > For example, em28xx has already a lock at the operations that change values at
> > > "dev" struct, including open() method. However, since the lock is not called at
> > > get operations, it needs to be fixed. I would also change it from mutex to a
> > > read/write semaphore, since two (or more) get operations can safely happen in
> > > parallel.
> >
> > Please bear in mind that we have not worked out the locking semantics
> > for hybrid tuner devices, and it's entirely possible that the get()
> > routines will need to switch the tuner mode, which would eliminate any
> > benefits of converting to a read/write semaphore.
>
> Arjan pointed some good reasons about why we shouldn't use r/w semaphores. So,
> it seems better to keep using mutexes.
>
> > I'm not sure yet exactly how that's going to work, but it's something
> > that might prompt you to defer converting it from a mutex until we
> > have that worked out.
>
>
> The hybrid device mode lock is somewhat complex. The simplest solution would be
> to block an open() call, if the device is already used by a different mode.
>
> This will minimize things like firmware reload, on xc3028 devices, where you
> have different firmwares for analog and digital modes.
>
> Also, some USB devices (like HVR-900/HVR-950) switches off analog audio and tv
> demod chips when in digital mode (the reverse is also true - e.g. digital demod
> is switched off at analog mode).
>
> So, if you are in digital mode, on HVR-900, and changes to analog, you'll need
> to re-initialize tvp5150/msp3400. The current code for those devices handles
> this at open(). If we let this to happen later, we'll need to re-send the video
> and audio parameters to all I2C connected devices, when switching mode.
>
> On the other hand, some userspace apps, like mythtv, opens both analog and
> digital API's. I'm not sure if it does this at the same time, but, if so, a
> lock at open() will cause a regression there (someone told me that this is the
> case - I didn't test it here yet).

MythTV's mythbackend can open both sides of the card at the same time
and the cx18 driver supports it. On my HVR-1600, MythTV may have the
digital side of the card open pulling EPG data off of the ATSC
broadcasts, when I open up the MythTV frontend and start watching live
TV on the analog side of the card. MythTV also supports
Picture-in-Picture using both the analog and digital parts of the
HVR-1600.



> One possible solution of providing a proper code to change mode, and not
> blocking open() would be to write something like this:
>
> static int check_and_switch_mode(void *priv, int digital)
> {
> struct dev_foo *dev = priv;
>
> mutex_lock(dev->lock);
>
> if (digital)
> return change_to_digital(dev);
> else
> return change_to_analog(dev);
>
> mutex_unlock(dev->lock);
> }
>
> Since this should be called for every valid V4L2 and DVB ioctl, the better
> place for it would be to add this as a new function callback, at video_ioctl2.
> Something like [1]:
>
> --- a/linux/drivers/media/video/videodev.c Tue May 27 16:02:56 2008 -0300
> +++ b/linux/drivers/media/video/videodev.c Tue May 27 17:34:04 2008 -0300
> @@ -821,6 +821,10 @@
> v4l_print_ioctl(vfd->name, cmd);
> printk("\n");
> }
> +
> + if (_IOC_TYPE(cmd)=='v') || _IOC_TYPE(cmd)=='V') &&
> + vfd->vidioc_switch_mode)
> + ret=vfd->vidioc_switch_mode(fh, 0);
>
> #ifdef CONFIG_VIDEO_V4L1_COMPAT
> /***********************************************************
>
>
>
> And something like this, at dvb core [2]:
>
> diff -r b94d587ee596 linux/drivers/media/dvb/dvb-core/dvb_frontend.c
> --- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c Tue May 27 16:02:56 2008 -0300
> +++ b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c Tue May 27 17:35:57 2008 -0300
> @@ -784,6 +784,9 @@
> cmd == FE_DISEQC_RECV_SLAVE_REPLY))
> return -EPERM;
>
> + if (fe->ops.switch_mode)
> + err = fe->ops.switch_mode(fe, 1);
> +
> if (down_interruptible (&fepriv->sem))
> return -ERESTARTSYS;
>
> [1] The code can be more conservative, changing mode only if S_STD or a video
> stream ioctl is called.
>
> [2] We need to think more about the proper places for the DVB changing mode. I
> suspect that we'll need to add the mode change callback there and/or at other
> different places.
>
> PS.: I suspect that the real code will be much more complex than the above skeletons.
>
> Cheers,
> Mauro
>
> --
> video4linux-list mailing list
> Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list
>



\
 
 \ /
  Last update: 2008-05-28 01:53    [W:0.198 / U:0.164 seconds]
©2003-2011 Jasper Spaans. Advertise on this site