Message-ID: <20141027105237.5f5ec7fd@recife.lan>
Date:	Mon, 27 Oct 2014 10:52:37 -0200
From:	Mauro Carvalho Chehab <mchehab@...radead.org>
To:	Takashi Iwai <tiwai@...e.de>
Cc:	Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
	Devin Heitmueller <dheitmueller@...nellabs.com>,
	alsa-devel@...a-project.org, Lars-Peter Clausen <lars@...afoo.de>,
	Linux Media Mailing List <linux-media@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"ramakrmu@...co.com" <ramakrmu@...co.com>,
	Shuah Khan <shuahkh@....samsung.com>,
	Hans Verkuil <hverkuil@...all.nl>,
	Sander Eikelenboom <linux@...elenboom.it>,
	prabhakar.csengg@...il.com, Antti Palosaari <crope@....fi>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	"sakari.ailus@...ux.intel.com" <sakari.ailus@...ux.intel.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Tim Gardner <tim.gardner@...onical.com>,
	"olebowle@....com" <olebowle@....com>,
	Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media
 token api

Em Sun, 26 Oct 2014 09:27:40 +0100
Takashi Iwai <tiwai@...e.de> escreveu:

> At Sat, 25 Oct 2014 11:41:15 -0200,
> Mauro Carvalho Chehab wrote:

> >      +---------------+
> >      |     start     |
> >      +---------------+
> >        |
> >        |
> >        v
> >      +--------------------------------+
> >      |              idle              | <+
> >      +--------------------------------+  |
> >        |                ^           |    |
> >        |                |           |    |
> >        v                |           |    |
> >      +---------------+  |           |    |
> >      | configuration |  |           |    |
> >      +---------------+  |           |    |
> >        |                |           |    |
> >        |                |           |    |
> >        v                |           |    |
> >      +---------------+  |           |    |
> >   +> |   streaming   | -+           |    |
> >   |  +---------------+              |    |
> >   |    |                            |    |
> >   |    |                            |    |
> >   |    v                            v    |
> >   |  +---------------+-----------+----+  |
> >   +- |       1       | suspended |  2 | -+
> >      +---------------+-----------+----+


The above diagram is actually simplified. There's an extra state
there that should be mentioned:

idle <-> DVB stream

(actually, DVB stream is a copy of the above, except that ALSA
won't have any business to do while the device is on DVB mode)

> So, judging from your description, the problem isn't about the
> exclusive lock, but rather implementation a kind of master/slave
> devices?  Then the proposed patch doesn't look like a correct
> implementation to me.

You might see it as a master/slave, except that it is not that simple.
It is really a hardware lock.

There are actually 3 (sub)drivers that may need to set the hardware:
	- ALSA driver;
	- V4L2 driver;
	- DVB driver.

The goal of the lock is to prevent that more than one driver would try 
to use a common piece of the hardware that it is already in usage
by another driver.

So, for example, the ALSA driver should not reprogram the hardware while 
the V4L2 driver is doing that.

I agree that, for V4L2/ALSA driver's interaction PoV, it could resemble
a sort of master/slave control, in the sense that ALSA capture start only
makes sense while V4L2 is at streaming state. Yet, just opening the device
or (even start capture on ALSA, for the hardware with multiple DMA engines,
like the ones that use snd-usb-audio) won't cause any harm if the V4L2 driver
is not reconfiguring the audio registers at the same time.

Also, ALSA open/close should be supported any time, as otherwise it will 
break existing applications. 

However, when the device is streaming on DVB mode, it is not possible
to stream on V4L2 mode, as there's just one DMA for both and just one
tuner. Also, ALSA capture doesn't make much sense on such case. Still,
locking on open/close may eventually break existing applications. Also,
it doesn't really make any sense to block the device to move from analog
to digital mode just because the ALSA devnodes are opened.

> What I (and supposedly Pierre) opposed is the implementation of
> exclusive lock control in spontaneous callbacks.  Especially the
> trigger callback is a bad place since it's a callback that is supposed
> to just trigger atomically.  In general, the only good place for
> allowing user-space to *control* the exclusive lock is open/close,
> unless the finer lock control is exposed.

I see your point. Then perhaps we should expose callbacks from other
parts of the ALSA core, perhaps at the logic that calls the trigger
callback at read and poll syscalls, plus the corresponding logic that is
used when the device is using the mmap syscall.

> But, reading through the argument from you guys, the intention of the
> patch seems like just to raise the conflict from the hardware level to
> the software level. 

Yes.

> If so, I doubt whether such an exclusive lock is
> the best way.   For example, audio stream can simply receive an error
> at any time if something is wrong and reacts accordingly instead of
> keeping the lock while streaming. Then the master side (video) can
> set the error flag, let the audio stream stop in the driver (if
> running), and sync with it.

Hmm... this is actually more complex than that. V4L2 driver doesn't
know if ALSA is streaming or not, or even if ALSA device node is opened
while he is touching at the hardware configuration or changing the
state. I mean: it is not an error to set the hardware. The error only
happens if ALSA and V4L2 tries to do it at the same time on an incompatible
way.

Also, this won't work for DVB, as on DVB this is really an exclusive
lock that would prevent both ALSA and V4L2 drivers to stream while in
DVB mode.

Implementing it with a lock seems to be the best approach, at least on
my eyes.

> That said, we should go back and start discussing the design goal at
> first.

Surely.

> 
> 
> thanks,
> 
> Takashi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists