Re: ATSC Tuner in KWorld PC120-U PCI Hybrid ATSC (17de:a134)

[hermann pitton]

Hi,

Am Donnerstag, den 25.11.2010, 00:56 -0500 schrieb Hooman B.:
> Thanks,
> 
> I see the drivers for both TDA18271HDC2 and TDA8290 loaded.
> I thought TDA18271HDC2 was the digital channel decoder, isn't it? Is
> the "digital channel decoder" different from the digital tuner??
> Should be looking for a different chip?

they are detected by chip IDs.

The tda8290 is the IF demodulator for global analog TV.
In case of the saa7135 it is an extra chip on the PCB, which most often
can also control the tuner over an i2c gate.

The tda18271hdc2 is a global hybrid tuner for analog and digital
terrestrial TV. It can also provide a FM radio IF. Further processing
and stereo separation for that is done on capable bridges like
saa7133/35/31e, not on the tda 8290.

Yes, for any terrestrial digital TV you need an extra channel decoder
and all known details about it.

http://linuxtv.org/wiki/index.php/Category:ATSC_PCI_Cards

In case of terrestrial ATSC, it must be able to deal with 8VSB
modulation.

IIRC, on the saa713x we have that tuner only in combination with the
tda10048 and DVB-T for now, but other combinations seem to be already
around.

Hermann

> Based on these to chips, I added my card in  saa7134_tda8290_callback
> to call saa7134_tda8290_18271_callback and here is the output of my
> dmesg:
> [  828.879454] dvb_init() allocating 1 frontend
> [  829.180234] nxt200x: nxt200x_readbytes: i2c read error (addr 0x0a, err == 
> -5)
> [  829.180244] Unknown/Unsupported NXT chip: 00 00 00 00 00
> [  829.180542] saa7133[0]/dvb: frontend initialization failed
> 
> Hooman
> 
> On Wed, Nov 24, 2010 at 7:21 PM, hermann pitton  
> wrote:
> >
> > Hi,
> >
> > Am Dienstag, den 23.11.2010, 17:34 -0500 schrieb Hooman B.:
> >> Hello!
> >> I've been trying to get the ATSC tuner in my KWorld PC120-U PCI Hybrid
> >> ATSC (17de:a134)
> >> to work with the latest v4l drivers from source (in Ubuntu).
> >>
> >> Right now, everything [capture, analog, radio, even IR] works - except
> >> the ATSC tuner (there is no
> >> front-end device). But that's the one thing I need working :-(
> >
> > OK.
> >
> >> Here's the output of lspci -nnvv
> >> 
> >> 03:01.0 Multimedia controller [0480]: Philips Semiconductors
> >> SAA7131/SAA7133/SAA7135 Video Broadcast Decoder [1131:7133] (rev d1)
> >> Subsystem: KWorld Computer Co. Ltd. Device [17de:a134]
> >> Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
> >> Stepping- SERR- FastB2B- DisINTx-
> >> Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort-
> >> SERR-  >> Latency: 255 (63750ns min, 63750ns max)
> >> Interrupt: pin A routed to IRQ 16
> >> Region 0: Memory at fdaff000 (32-bit, non-prefetchable) [size=2K]
> >> Capabilities: 
> >> Kernel driver in use: saa7134
> >> Kernel modules: saa7134
> >> 
> >>
> >> This is the most similar card that I forced in saa7134-cards.c:
> >> 
> >> .vendor   = PCI_VENDOR_ID_PHILIPS,
> >> .device   = PCI_DEVICE_ID_PHILIPS_SAA7133,
> >> .subvendor= 0x17de,
> >> .subdevice= 0xa134,
> >> .driver_data  = SAA7134_BOARD_MSI_TVATANYWHERE_PLUS,
> >> 
> >>
> >> The chips are : SAA7135HL/203 and TDA18271HDC2
> >>
> >
> > Both variants of the MSI t...@nywhere Plus do not have any support for
> > digital TV.
> >
> > You need to find out the type of digital channel decoder on your board
> > at first.
> >
> > Then you check saa7134-dvb.c, if it is already supported on other cards.
> >
> > You have to investigate the details of how the channel decoder is
> > employed, but with some luck can try with already supported cards.
> >
> > If i2c traffic locks up and chips disappear from the bus, a cold boot
> > might be necessary to continue with testing.
> >
> > Cheers,
> > Hermann
> >


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 05/12] media: Reference count and power handling

[Mark Brown]

On Thu, Nov 25, 2010 at 07:49:05PM +0200, Sakari Ailus wrote:

> Currently when two media entities are connected they will be powered up
> for the duration of the link setup, just in case the drivers would need
> to access hardware for some reason. I don't think we have a need for
> that at the moment, it's so just because it seemed to be the right thing
> to do.

This is *really* bad for audio, powering things on and off needlessly
can cause audible effects for users.  If the individual drivers need to
do something they can go and implement that but powering things up by
default seems like it'll at best waste power most of the time.

> Essentially the idea is that the drivers do not need to be involved with
> the power state handling and the device would be powered always when
> they are run, but not be powered when it's not necessary.

This is what DAPM does in ASoC at the minute.  What it does is that
every time there is a change in the device configuration which might
have affected the power state it walks the graph of power nodes in the
system.  If there has been a change in the state the core will generate
a power transition sequence and apply it.  The devices have no knowledge
of this (though they can insert manual sequences for nodes if they need
to), for the most part they just describe the graph and the register
bits that control the power for the nodes and the routing.

> Subdev is a V4L2 specific concept and I don't know if ALSA would benefit
> from something similar. If you want to be able to access the individual
> enties from user space, then similar arrangement might be useful.

ALSA already has this feature (at least in the embedded case, HDA has
some of these features too).

> I don't see a problem in applying restrictions on power on / off
> sequence. They would probably be useful also on the V4L2 side in the future.

> Could the restrictions be described as dependencies only, i.e. entity 1
> power-up requires entity 2 to be powered first, also meaning that entity
> 2 could be powered down only after entity 1 has been powered down?

No.  Audio power sequencing tends to work better if sorted by the type
of object rather than the route (though using the route as a lower order
sort key can be useful).  It's also useful to coalesce the I/O on the
hardware for performance (both speed and user experience).

The way we're currently doing it the sequencing is actually separated
from the graph walk - the result of the graph walk is a set of things
that need doing which is then implemented in a separate step.  I think
this would be the easiest way to integrate with what you're doing at the
minute, keep the same split and then ASoC can do the postprocessing of
the results of the graph walk in the same way as it does currently.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] rc-core: add loopback driver

[David Härdeman]

This patch adds a loopback driver to rc-core which I've found useful for
running scripted tests of different parts of rc-core without having to
fiddle with real hardware.

Basically it emulates hardware with a learning and a non-learning
receiver and two transmitters (which correspond to the two
receivers). TX data that is sent is fed back as input on the
corresponding receiver, which allows for debugging of IR decoders,
keymaps, etc.

Signed-off-by: David Härdeman 
---
 drivers/media/rc/Kconfig   |   13 ++
 drivers/media/rc/Makefile  |1 
 drivers/media/rc/rc-loopback.c |  260 
 3 files changed, 274 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/rc/rc-loopback.c

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index 42b4feb..3785162 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -177,4 +177,17 @@ config IR_WINBOND_CIR
To compile this driver as a module, choose M here: the module will
   be called winbond_cir.
 
+config RC_LOOPBACK
+   tristate "Remote Control Loopback Driver"
+   depends on RC_CORE
+   ---help---
+  Say Y here if you want support for the remote control loopback
+  driver which allows TX data to be sent back as RX data.
+  This is mostly useful for debugging purposes.
+
+  If you're not sure, select N here.
+
+  To compile this driver as a module, choose M here: the module will
+  be called rc_loopback.
+
 endif #RC_CORE
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index 78ac8c5..67b4f7f 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_IR_NUVOTON) += nuvoton-cir.o
 obj-$(CONFIG_IR_ENE) += ene_ir.o
 obj-$(CONFIG_IR_STREAMZAP) += streamzap.o
 obj-$(CONFIG_IR_WINBOND_CIR) += winbond-cir.o
+obj-$(CONFIG_RC_LOOPBACK) += rc-loopback.o
diff --git a/drivers/media/rc/rc-loopback.c b/drivers/media/rc/rc-loopback.c
new file mode 100644
index 000..49cee61
--- /dev/null
+++ b/drivers/media/rc/rc-loopback.c
@@ -0,0 +1,260 @@
+/*
+ * Loopback driver for rc-core,
+ *
+ * Copyright (c) 2010 David Härdeman 
+ *
+ * This driver receives TX data and passes it back as RX data,
+ * which is useful for (scripted) debugging of rc-core without
+ * having to use actual hardware.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#define DRIVER_NAME"rc-loopback"
+#define dprintk(x...)  if (debug) printk(KERN_INFO DRIVER_NAME ": " x)
+#define RXMASK_REGULAR 0x1
+#define RXMASK_LEARNING0x2
+
+static bool debug;
+
+struct loopback_dev {
+   struct rc_dev *dev;
+   u32 txmask;
+   u32 txcarrier;
+   u32 txduty;
+   bool idle;
+   bool learning;
+   bool carrierreport;
+   u32 rxcarriermin;
+   u32 rxcarriermax;
+};
+
+static struct loopback_dev loopdev;
+
+static int loop_set_tx_mask(struct rc_dev *dev, u32 mask)
+{
+   struct loopback_dev *lodev = dev->priv;
+
+   if ((mask & (RXMASK_REGULAR | RXMASK_LEARNING)) != mask) {
+   dprintk("invalid tx mask: %u\n", mask);
+   return -EINVAL;
+   }
+
+   dprintk("setting tx mask: %u\n", mask);
+   lodev->txmask = mask;
+   return 0;
+}
+
+static int loop_set_tx_carrier(struct rc_dev *dev, u32 carrier)
+{
+   struct loopback_dev *lodev = dev->priv;
+
+   dprintk("setting tx carrier: %u\n", carrier);
+   lodev->txcarrier = carrier;
+   return 0;
+}
+
+static int loop_set_tx_duty_cycle(struct rc_dev *dev, u32 duty_cycle)
+{
+   struct loopback_dev *lodev = dev->priv;
+
+   if (duty_cycle < 1 || duty_cycle > 99) {
+   dprintk("invalid duty cycle: %u\n", duty_cycle);
+   return -EINVAL;
+   }
+
+   dprintk("setting duty cycle: %u\n", duty_cycle);
+   lodev->txduty = duty_cycle;
+   return 0;
+}
+
+static int loop_set_rx_carrier_range(struct rc_dev *dev, u32 min, u32 max)
+{
+   struct loopback_dev *lodev = dev->priv;
+
+   if (min < 1 || min > max) {
+   dprintk("invalid rx carrier range %u to %u\n", min, max);
+   return -EINVAL;
+   }
+
+   dprintk("setting rx carrier range %u to %u\n", min, max);
+

Re: [PATCH] media: fix casting issues in timberdale logiwin

[Richard Röjfors]

On 11/24/2010 10:41 AM, Richard Röjfors wrote:
> On 11/23/2010 09:39 PM, Jarod Wilson wrote:
>> I get warnings about casting to and from pointers and integers of
>> different sizes w/current code, this silences them.
>>
>> Signed-off-by: Jarod Wilson 
> 
> Looks good, and works.

I did a mistake and tested the wrong binary, this patch is NOT ok.

Mauro please to do apply this patch.

> 
> Acked-by: Richard Röjfors 
> 
>> ---
>>  drivers/media/video/timblogiw.c |6 --
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/video/timblogiw.c 
>> b/drivers/media/video/timblogiw.c
>> index 48989e5..4da3625 100644
>> --- a/drivers/media/video/timblogiw.c
>> +++ b/drivers/media/video/timblogiw.c
>> @@ -148,7 +148,9 @@ static void timblogiw_dma_cb(void *data)
>>  
>>  static bool timblogiw_dma_filter_fn(struct dma_chan *chan, void 
>> *filter_param)
>>  {
>> -return chan->chan_id == (int)filter_param;
>> +int chan_id = *(int *)filter_param;
>> +
>> +return chan->chan_id == chan_id;
>>  }

This part dereferences the pointer, which actually is a type casted int!

>>  
>>  /* IOCTL functions */
>> @@ -670,7 +672,7 @@ static int timblogiw_open(struct file *file)
>>  
>>  /* find the DMA channel */
>>  fh->chan = dma_request_channel(mask, timblogiw_dma_filter_fn,
>> -(void *)lw->pdata.dma_channel);
>> +(void *)(unsigned long)lw->pdata.dma_channel);

You see here, the channel is type casted to a pointer, not something we want to 
dereference as above.

--Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] kconfig: add an option to determine a menu's visibility

[Sam Ravnborg]

On Thu, Nov 25, 2010 at 12:06:46PM -0500, Arnaud Lacombe wrote:
> Hi folks,
> 
> On Sat, Nov 6, 2010 at 5:30 PM, Arnaud Lacombe  wrote:
> > This option is aimed to add the possibility to control a menu's visibility
> > without adding dependency to the expression to all the submenu.
> >
> > Signed-off-by: Arnaud Lacombe 
> > ---
> >  scripts/kconfig/expr.h      |    1 +
> >  scripts/kconfig/lkc.h       |    1 +
> >  scripts/kconfig/menu.c      |   11 +++
> >  scripts/kconfig/zconf.gperf |    1 +
> >  scripts/kconfig/zconf.y     |   21 ++---
> >  5 files changed, 32 insertions(+), 3 deletions(-)
> >
> As there seem to be no interested from Michal to either, ACK, NACK, or

Hi Arnaud.
I have these sitting in my TODO pile.
But I have other things that I need to do first and I expect
these other things to take a couple of weeks.
[Not Linux related - just busy at my day-job/night-job]

Sam
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[cron job] v4l-dvb daily build: WARNINGS

[Hans Verkuil]

This message is generated daily by a cron job that builds v4l-dvb for
the kernels and architectures in the list below.

Results of the daily build of v4l-dvb:

date:Thu Nov 25 19:01:43 CET 2010
git master:   59365d136d205cc20fe666ca7f89b1c5001b0d5a
git media-master: gcc version:  i686-linux-gcc (GCC) 4.5.1
host hardware:x86_64
host os:  2.6.32.5

linux-git-armv5: WARNINGS
linux-git-armv5-davinci: WARNINGS
linux-git-armv5-ixp: WARNINGS
linux-git-armv5-omap2: WARNINGS
linux-git-i686: WARNINGS
linux-git-m32r: WARNINGS
linux-git-mips: WARNINGS
linux-git-powerpc64: WARNINGS
linux-git-x86_64: WARNINGS
spec-git: OK
sparse: ERRORS

Detailed results are available here:

http://www.xs4all.nl/~hverkuil/logs/Thursday.log

Full logs are available here:

http://www.xs4all.nl/~hverkuil/logs/Thursday.tar.bz2

The V4L-DVB specification from this daily build is here:

http://www.xs4all.nl/~hverkuil/spec/media.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 02/12] media: Media device

[Alan Cox]

On Thu, 25 Nov 2010 12:20:31 -0500
Andy Walls  wrote:

> The signedness of char is ambiguous for 8 bit data, which is why an API would 
> normally use u8 (or s8, I guess).
> 
> Since this is known to be character data, I would think char would be fine.  
> I am assuming C compilers would never assume multibyte "char"s.

char is 8bit in all modern C. I have used C compilers with configurable 9
or 7 bit char and such a machine is never going to run Linux without
serious insanity. Even grep in 9bit char is not fun...

The advantage of using u8 is that any casting goes the way you expect
whereas when working with UTF-8 things like

int x = name[0];

may not produce the expected result otherwise.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-dvb] dvbstream fails to tune QAM-256

[David Liontooth]

On 11/24/2010 03:02 AM, Nico Sabbi wrote:

David Liontooth ha scritto:

On 11/19/2010 11:51 PM, David Liontooth wrote:


I'm using Debian's dvbstream 0.6+cvs20090621-1 to capture video and 
closed captioning to file.


If I tune with azap, dvbstream works fine, but I can't get it to 
tune on its own.


In the Debian source code, I activated DVB_ATSC by adding -DDVB_ATSC 
to CFLAGS in the Makefile.


dvbstream -f 64500 -qam 256 -v 49 a 52 -o > test.ts

gives me the error "Unknown FE type".

Suggestions? Is this a problem with an API that has changed since 
dvbstream's last release?


If I change tune.h to allow DVB API >=3 major instead of just ==3, I 
get this (this is a very lightly patched version to allow 8 cards):


# dvbstream -f 621000 -qam 256 -v 8192 -a 8192 -s 6875 -n 10 -o:test.ts
dvbstream v0.7.01 - (C) Dave Chapman 2001-2004
Released under the GPL.
Latest version available from http://dvbtools.sourceforge.net
Open file test.ts
Tuning to 621000 MHz
Using DVB card "Samsung S5H1411 QAM/8VSB Frontend", freq=621000
tuning ATSC to 62100, modulation=5
Getting frontend status
Event:  Frequency: 62100
Modulation: 5
Bit error rate: 0
Signal strength: 0
SNR: 0
UNC: 0
FE_STATUS: FE_HAS_SIGNAL FE_HAS_LOCK FE_HAS_CARRIER FE_HAS_VITERBI 
FE_HAS_SYNC


MAP 0, file test.ts: From -1 secs, To -1 secs, 0 PIDs -
dvbstream will stop after 10 seconds (0 minutes)
Streaming 1 stream
Caught signal 1 - closing cleanly.

So it tries to tune, and succeeds in a manner of speaking -- if I try 
to tune to an unavailable frequency, it will fail; here we can see it 
locks. However, the Signal strength is zero and the file is empty.


What happened with the ATSC QAM-256 tuning in between DVB API 3.1 and 
now? I'm running 2.6.32.5, with DVB API 5.1.


Cheers,
Dave




it's the usual debian idiocy to package only "official releases".
I refuse to release anything, so unless Dave does it you should use a 
fresh cvs checkout from sourceforge, where the limitation was removed 
years ago.

Hi Nico,

Glad to hear you're around and thanks for responding! The Debian 
maintainer is actually using dvbstream_0.6+cvs20090621, which looks to 
me like the latest version -- I used


cvs -z3 
-d:pserver:anonym...@dvbtools.cvs.sourceforge.net:/cvsroot/dvbtools co 
-P  dvbstream


cvs tune.h still contains the block that restricts the DVB_API to an 
outdated version,


#undef DVB_ATSC
#if defined(DVB_API_VERSION_MINOR)
#if DVB_API_VERSION == 3 && DVB_API_VERSION_MINOR >= 1
#define DVB_ATSC 1
#endif
#endif

and tune.c is not different from the Debian tune.c.

Where is the working code for ATSC QAM tuning? I'd be happy to work with 
you if you can provide some pointers.


Salve,
Dave



--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 09/10] MCDE: Add build files and bus

[Marcus LORENTZON]

> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: den 25 november 2010 17:48
> To: Marcus LORENTZON
> Cc: linux-arm-ker...@lists.infradead.org; Jimmy RUBIN; linux-
> me...@vger.kernel.org; Dan JOHANSSON; Linus WALLEIJ
> Subject: Re: [PATCH 09/10] MCDE: Add build files and bus
>
> On Thursday 25 November 2010, Marcus LORENTZON wrote:
> > From: Arnd Bergmann [mailto:a...@arndb.de]
> > > On Wednesday 10 November 2010, Jimmy Rubin wrote:
> > > > This patch adds support for the MCDE, Memory-to-display
> controller,
> > > > found in the ST-Ericsson ux500 products.
> > > >
> > > > This patch adds the necessary build files for MCDE and the bus
> that
> > > > all displays are connected to.
> > > >
> > >
> > > Can you explain why you need a bus for this?
> >
> > The idea of the bus is to make it easier to add new panel/display
> support
> > using the normal device/driver model. Boards add devices at init, and
> > drivers are selected in config. If we were to model the "real
> physical"
> > bus structure it would be very complex, hard to use. We use our own
> bus,
> > but it's really a virtual bus for adding display devices and drivers
> to
> > MCDE host. Is there any "generic virtual bus type" we should use
> instead
> > of our own type? If you have another idea of how to add display
> devices
> > and drivers without MCDE code modifications, please let us know. A
> virtual
> > bus just give us a nice framework to do this.
>
> All devices that you cannot probe by asking hardware or firmware are
> normally
> considered platform devices. Then again, a platform device is usally
> identified by its resources, i.e. MMIO addresses and interrupts, which
> I guess your display does not have.

Then we might be on right track to model them as devices on a platform bus. 
Since most displays/panels can't be "plug-n-play" probed, instead devices has 
to be statically declared in board-xx.c files in mach-ux500 folder. Or is 
platform bus a "singleton"? Or can we define a new platform bus device?
Displays like HDMI TV-sets are not considered for device/driver in mcde. 
Instead there will be a hdmi-chip-device/driver on the mcde bus. So all devices 
and drivers on this bus are static.

> > > With the code you currently have, there is only a single driver
> > > associated
> > > with this bus type, and also just a single device that gets
> registered
> > > here!
> >
> > And yes, currently there's only one single driver. But there's a lot
> more
> > coming in the pipe. Like some LCD, DPI, DBI, DSI display drivers. And
> once
> > all different u8500 boards are pushed, there will be multiple boards
> > registering devices with different setups. But one thing at a time.
>
> Your approach is making it hard to review the code in context. Adding a
> device driver that uses existing infrastructure is usually
> straightforward,
> but adding infrastructure is a hard thing and needs to be done with
> great
> care, especially if you add infrastructure before it actually is
> needed.
>
> Staging it in a way that adds all the display drivers later than the
> base driver is a good idea, but it would be helpful to also add the
> infrastructure at the later stage. Maybe you can try to simplify the
> code for now by hardcoding the single display and remove the dynamic
> registration. You still have the the code, so once the base code looks
> good for inclusion, we can talk about it in the context of adding
> dynamic display support back in, possibly in exactly the way you are
> proposing now, but perhaps in an entirely different way if we come up
> with a better solution.

What about starting with MCDE HW, which is the core HW driver doing all real 
work? And then continue with the infrastructure + some displays + drivers ...
Only problem is that we then have a driver that can't be used from user space, 
because I don't think I can find anyone with enough time to write a display 
driver + framebuffer on top of mcde_hw (which is what the existing code does).

> On a more abstract level, when you want to add large chunks of code
> to the kernel, you often cannot find anyone to review and understand
> all of it at once. Splitting a subsystem into ten patches by file
> level rarely helps, so instead you should ideally have a series of
> patches that each add working code that enable more features.
>
> This is of course more work for you, especially if you did not consider
> it while writing the code in the first place. Still, you should
> always try hard to make code easy to review as much as possible,
> because you need to work with reviewers both to get code in and to
> make sure you make the quality ends up as good as possible.
>
> > > >+static int __init mcde_subsystem_init(void)
> > > >+{
> > > >+   int ret;
> > > >+   pr_info("MCDE subsystem init begin\n");
> > > >+
> > > >+   /* MCDE module init sequence */
> > > >+   ret = mcde_init();
> > > >+   if (ret)
> > > >+   return ret;
> > > >

Re: [RFC/PATCH v6 05/12] media: Reference count and power handling

[Sakari Ailus]

Laurent Pinchart wrote:
> Hi Mark,

Hi Laurent, others,

> On Thursday 25 November 2010 14:49:08 Mark Brown wrote:
>> On Thu, Nov 25, 2010 at 03:28:12AM +0100, Laurent Pinchart wrote:
>>> If the entity is of node type, the power change is distributed to
>>> all connected entities. For non-nodes it only affects that very
>>> node. A mutex is used to serialise access to the entity graph.
>>
>> ASoC has its own power management stuff which doesn't *quite* map onto
>> this one as-is.  The power determination stuff is essentially identical
>> (and this is actually nicer) but we have a separate postprocessing stage
>> that actually applies the changes in a sequence which minimises audible
>> issues caused by doing things in a bad order (eg, power down a PGA
>> before you turn off inputs).  This is noddy enough to implement, though
>> - we just need a pre and post run notifications to set up and implement
>> the changes I think.
> 
> That sounds feasible. Sakari, you've implemented power management, what do 
> you 
> think about this ?

I first have to admit that I don't know ALSA almost at all.

Currently when two media entities are connected they will be powered up
for the duration of the link setup, just in case the drivers would need
to access hardware for some reason. I don't think we have a need for
that at the moment, it's so just because it seemed to be the right thing
to do.

Essentially the idea is that the drivers do not need to be involved with
the power state handling and the device would be powered always when
they are run, but not be powered when it's not necessary.

That feature put aside, then what's left is what Laurent described; the
entities will be powered up based on opening subdev and video nodes
(V4L2 case, for ALSA it'd be different kind of ALSA nodes). But I don't
think there's necessarily even need to remove this feature.

Subdev is a V4L2 specific concept and I don't know if ALSA would benefit
from something similar. If you want to be able to access the individual
enties from user space, then similar arrangement might be useful.

There are use cases to only use subdev nodes on V4L2: camera LED flash
in torch mode, for example. There is no need to power on the rest of the
system.

I don't see a problem in applying restrictions on power on / off
sequence. They would probably be useful also on the V4L2 side in the future.


Could the restrictions be described as dependencies only, i.e. entity 1
power-up requires entity 2 to be powered first, also meaning that entity
2 could be powered down only after entity 1 has been powered down?

This type of restrictions would fit quite well to the current model as
it would essentially mean just media_entity_get() for the dependent
entities, and media_entity_put() when the original entity is powered
down. This would work recursively.

In the case of audio, most if not all devices (right?) would be always
powered up in certain order. At the same time this would be good for the
flash in torch mode case.


Comments, opinions? :-)


Best regards,

-- 
Sakari Ailus
sakari.ai...@maxwell.research.nokia.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 02/12] media: Media device

[Laurent Pinchart]

Hi Andy,

On Thursday 25 November 2010 18:20:31 Andy Walls wrote:
> The signedness of char is ambiguous for 8 bit data, which is why an API
> would normally use u8 (or s8, I guess).
> 
> Since this is known to be character data, I would think char would be fine.

I think so too.

> I am assuming C compilers would never assume multibyte "char"s.

Not on sane platforms I guess. We would have problems much worse than MC API 
breakage in that case anyway :-)

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 02/12] media: Media device

[Andy Walls]

The signedness of char is ambiguous for 8 bit data, which is why an API would 
normally use u8 (or s8, I guess).

Since this is known to be character data, I would think char would be fine.  I 
am assuming C compilers would never assume multibyte "char"s.

Regards,
Andy

Laurent Pinchart  wrote:

>Hi Clemens,
>
>Thanks for the review.
>
>On Thursday 25 November 2010 10:33:02 Clemens Ladisch wrote:
>> Laurent Pinchart wrote:
>> > +struct media_device {
>> > ...
>> > +  u8 model[32];
>> > +  u8 serial[40];
>> > +  u8 bus_info[32];
>> 
>> All drivers and userspace applications have to treat this as char[], so
>> why u8[]?
>
>Good question. I've copied the V4L2 practice of using u8 (or __u8) for fixed-
>length strings in structures. I can't think of any reason for that.
>
>I will replace u8 with char unless someone comes up with a good reason to keep 
>u8.
>
>-- 
>Regards,
>
>Laurent Pinchart
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media" in
>the body of a message to majord...@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
N�r��yb�X��ǧv�^�)޺{.n�+{���bj)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥

Re: [PATCH/RFC] v4l: Add subdev sensor g_skip_frames operation

[Laurent Pinchart]

Hi Eino-Ville,

On Tuesday 23 November 2010 00:18:18 Eino-Ville Talvala wrote:
> On 11/19/2010 6:22 AM, Laurent Pinchart wrote:
> > On Friday 19 November 2010 15:15:11 Guennadi Liakhovetski wrote:
> >> On Fri, 19 Nov 2010, Laurent Pinchart wrote:
> >>> On Friday 19 November 2010 14:42:31 Hans Verkuil wrote:
>  On Friday 19 November 2010 14:26:42 Laurent Pinchart wrote:
> > Some buggy sensors generate corrupt frames when the stream is
> > started. This new operation returns the number of corrupt frames to
> > skip when starting the stream.
>  
>  Looks OK, but perhaps the two should be combined to one function?
> >>> 
> >>> I'm fine with both. Guennadi, any opinion ?
> >> 
> >> Same as before;) I think, there can be many more such "micro"
> >> parameters, that we'll want to collect from the sensor. So, if we had a
> >> good idea - what those parameters are like, we could implement just one
> >> API call to get them all, or even just pass one object with this
> >> information - if it is constant. If we don't have a good idea yet, what
> >> to expect there, it might be best to wait and first collect a more
> >> complete understanding of this kind of information.
> 
> A few days late on this, but I wanted to add my two cents.
> 
> When using V4L2 for still photography applications (like what's done on the
> N900, or on our custom Frankencamera), it becomes a lot more important to
> know what data is good or bad, when typically you capture only one or a
> few frames at a high resolution, before dropping back down to a
> viewfinding mode.  One doesn't want to throw any frames on the floor if it
> isn't absolutely neccessary ( 100 ms extra shutter lag for every frame you
> have to drop, on the N900).  So having some reasonable way to know when
> the data becomes good is very helpful.

I agree with this. To make things worse, as Guennadi noted, the world isn't 
black and white and we have lots of shades of grey between "good" and "bad". 
Depending on the use case the same image could be considered good or bad. 
That's why we will need to pass metadata to userspace at some point (providing 
the kernel can receive the metadata from the hardware of course).

This patch aims at solving a subset of the good-bad problem, namely sensors 
that always produce a fixed number of completely corrupted frames when the 
stream starts.

> Sensor data sheets are also often rather vague, and state things like
> changing regions of interest, digital zoom, etc, 'may cause a bad frame'. 
> It'd be great if once somebody figures out what 'may' translates to, not
> everyone has to figure it out again for every application.

I totally agree. We would need better cooperation from sensor vendors.

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] kconfig: add an option to determine a menu's visibility

[Arnaud Lacombe]

Hi folks,

On Sat, Nov 6, 2010 at 5:30 PM, Arnaud Lacombe  wrote:
> This option is aimed to add the possibility to control a menu's visibility
> without adding dependency to the expression to all the submenu.
>
> Signed-off-by: Arnaud Lacombe 
> ---
>  scripts/kconfig/expr.h      |    1 +
>  scripts/kconfig/lkc.h       |    1 +
>  scripts/kconfig/menu.c      |   11 +++
>  scripts/kconfig/zconf.gperf |    1 +
>  scripts/kconfig/zconf.y     |   21 ++---
>  5 files changed, 32 insertions(+), 3 deletions(-)
>
As there seem to be no interested from Michal to either, ACK, NACK, or
even comment this series, please let me withdraw these patches. If
this mail is not enough to void the patch, I hope to still be able to
withdraw my Signed-off-by from this particular series, and thus no
longer be able to certify the origin of the patches to prevent their
merge.

I guess lot of people will keep being pissed by these warnings.

I hope another solution will be found or someone will have fun wasting
time re-writting the patch.

Thanks,
 - Arnaud

ps: as we all live a world of consensus, I can still be convinced to
change my mind if interest arise.

> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
> index 184eb6a..e57826c 100644
> --- a/scripts/kconfig/expr.h
> +++ b/scripts/kconfig/expr.h
> @@ -164,6 +164,7 @@ struct menu {
>        struct menu *list;
>        struct symbol *sym;
>        struct property *prompt;
> +       struct expr *visibility;
>        struct expr *dep;
>        unsigned int flags;
>        char *help;
> diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
> index 753cdbd..3f7240d 100644
> --- a/scripts/kconfig/lkc.h
> +++ b/scripts/kconfig/lkc.h
> @@ -107,6 +107,7 @@ void menu_end_menu(void);
>  void menu_add_entry(struct symbol *sym);
>  void menu_end_entry(void);
>  void menu_add_dep(struct expr *dep);
> +void menu_add_visibility(struct expr *dep);
>  struct property *menu_add_prop(enum prop_type type, char *prompt, struct 
> expr *expr, struct expr *dep);
>  struct property *menu_add_prompt(enum prop_type type, char *prompt, struct 
> expr *dep);
>  void menu_add_expr(enum prop_type type, struct expr *expr, struct expr *dep);
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index 7e83aef..b9d9aa1 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -152,6 +152,12 @@ struct property *menu_add_prompt(enum prop_type type, 
> char *prompt, struct expr
>        return menu_add_prop(type, prompt, NULL, dep);
>  }
>
> +void menu_add_visibility(struct expr *expr)
> +{
> +       current_entry->visibility = expr_alloc_and(current_entry->visibility,
> +           expr);
> +}
> +
>  void menu_add_expr(enum prop_type type, struct expr *expr, struct expr *dep)
>  {
>        menu_add_prop(type, NULL, expr, dep);
> @@ -410,6 +416,11 @@ bool menu_is_visible(struct menu *menu)
>        if (!menu->prompt)
>                return false;
>
> +       if (menu->visibility) {
> +               if (expr_calc_value(menu->visibility) == no)
> +                       return no;
> +       }
> +
>        sym = menu->sym;
>        if (sym) {
>                sym_calc_value(sym);
> diff --git a/scripts/kconfig/zconf.gperf b/scripts/kconfig/zconf.gperf
> index d8bc742..c9e690e 100644
> --- a/scripts/kconfig/zconf.gperf
> +++ b/scripts/kconfig/zconf.gperf
> @@ -38,6 +38,7 @@ hex,          T_TYPE,         TF_COMMAND, S_HEX
>  string,                T_TYPE,         TF_COMMAND, S_STRING
>  select,                T_SELECT,       TF_COMMAND
>  range,         T_RANGE,        TF_COMMAND
> +visible,       T_VISIBLE,      TF_COMMAND
>  option,                T_OPTION,       TF_COMMAND
>  on,            T_ON,           TF_PARAM
>  modules,       T_OPT_MODULES,  TF_OPTION
> diff --git a/scripts/kconfig/zconf.y b/scripts/kconfig/zconf.y
> index 2abd3c7..49fb4ab 100644
> --- a/scripts/kconfig/zconf.y
> +++ b/scripts/kconfig/zconf.y
> @@ -36,7 +36,7 @@ static struct menu *current_menu, *current_entry;
>  #define YYERROR_VERBOSE
>  #endif
>  %}
> -%expect 28
> +%expect 30
>
>  %union
>  {
> @@ -68,6 +68,7 @@ static struct menu *current_menu, *current_entry;
>  %token T_DEFAULT
>  %token T_SELECT
>  %token T_RANGE
> +%token T_VISIBLE
>  %token T_OPTION
>  %token T_ON
>  %token  T_WORD
> @@ -123,7 +124,7 @@ stmt_list:
>  ;
>
>  option_name:
> -       T_DEPENDS | T_PROMPT | T_TYPE | T_SELECT | T_OPTIONAL | T_RANGE | 
> T_DEFAULT
> +       T_DEPENDS | T_PROMPT | T_TYPE | T_SELECT | T_OPTIONAL | T_RANGE | 
> T_DEFAULT | T_VISIBLE
>  ;
>
>  common_stmt:
> @@ -359,7 +360,7 @@ menu: T_MENU prompt T_EOL
>        printd(DEBUG_PARSE, "%s:%d:menu\n", zconf_curname(), zconf_lineno());
>  };
>
> -menu_entry: menu depends_list
> +menu_entry: menu visibility_list depends_list
>  {
>        $$ = menu_add_menu();
>  };
> @@ -430,6 +431,19 @@ depends: T_DEPENDS T_ON expr T_EOL
>        printd(DEBUG_PARSE, "%s:%d:depends on\n", zconf_curname(), 
> zconf_lineno());
>  };
>
> +/* visibil

Re: [PATCH 09/10] MCDE: Add build files and bus

[Arnd Bergmann]

On Thursday 25 November 2010, Marcus LORENTZON wrote:
> From: Arnd Bergmann [mailto:a...@arndb.de]
> > On Wednesday 10 November 2010, Jimmy Rubin wrote:
> > > This patch adds support for the MCDE, Memory-to-display controller,
> > > found in the ST-Ericsson ux500 products.
> > >
> > > This patch adds the necessary build files for MCDE and the bus that
> > > all displays are connected to.
> > >
> > 
> > Can you explain why you need a bus for this?
> 
> The idea of the bus is to make it easier to add new panel/display support
> using the normal device/driver model. Boards add devices at init, and
> drivers are selected in config. If we were to model the "real physical"
> bus structure it would be very complex, hard to use. We use our own bus,
> but it's really a virtual bus for adding display devices and drivers to 
> MCDE host. Is there any "generic virtual bus type" we should use instead
> of our own type? If you have another idea of how to add display devices
> and drivers without MCDE code modifications, please let us know. A virtual
> bus just give us a nice framework to do this.

All devices that you cannot probe by asking hardware or firmware are normally
considered platform devices. Then again, a platform device is usally
identified by its resources, i.e. MMIO addresses and interrupts, which
I guess your display does not have.

> > With the code you currently have, there is only a single driver
> > associated
> > with this bus type, and also just a single device that gets registered
> > here!
> 
> And yes, currently there's only one single driver. But there's a lot more
> coming in the pipe. Like some LCD, DPI, DBI, DSI display drivers. And once
> all different u8500 boards are pushed, there will be multiple boards
> registering devices with different setups. But one thing at a time.

Your approach is making it hard to review the code in context. Adding a
device driver that uses existing infrastructure is usually straightforward,
but adding infrastructure is a hard thing and needs to be done with great
care, especially if you add infrastructure before it actually is needed.

Staging it in a way that adds all the display drivers later than the
base driver is a good idea, but it would be helpful to also add the
infrastructure at the later stage. Maybe you can try to simplify the
code for now by hardcoding the single display and remove the dynamic
registration. You still have the the code, so once the base code looks
good for inclusion, we can talk about it in the context of adding
dynamic display support back in, possibly in exactly the way you are
proposing now, but perhaps in an entirely different way if we come up
with a better solution.

On a more abstract level, when you want to add large chunks of code
to the kernel, you often cannot find anyone to review and understand
all of it at once. Splitting a subsystem into ten patches by file
level rarely helps, so instead you should ideally have a series of
patches that each add working code that enable more features.

This is of course more work for you, especially if you did not consider
it while writing the code in the first place. Still, you should
always try hard to make code easy to review as much as possible,
because you need to work with reviewers both to get code in and to
make sure you make the quality ends up as good as possible.

> > >+static int __init mcde_subsystem_init(void)
> > >+{
> > >+   int ret;
> > >+   pr_info("MCDE subsystem init begin\n");
> > >+
> > >+   /* MCDE module init sequence */
> > >+   ret = mcde_init();
> > >+   if (ret)
> > >+   return ret;
> > >+   ret = mcde_display_init();
> > >+   if (ret)
> > >+   goto mcde_display_failed;
> > >+   ret = mcde_dss_init();
> > >+   if (ret)
> > >+   goto mcde_dss_failed;
> > >+   ret = mcde_fb_init();
> > >+   if (ret)
> > >+   goto mcde_fb_failed;
> > >+   pr_info("MCDE subsystem init done\n");
> > >+
> > >+   return 0;
> > >+mcde_fb_failed:
> > >+   mcde_dss_exit();
> > >+mcde_dss_failed:
> > >+   mcde_display_exit();
> > >+mcde_display_failed:
> > >+   mcde_exit();
> > >+   return ret;
> > >+}
> > 
> > Splitting up the module into four sub-modules and then initializing
> > everything from one place indicates that something is done wrong
> > on a global scale.
> > 
> > If you indeed need a bus, that should be a separate module that gets
> > loaded first and then has the other modules build on top of.
> 
> Yes, that's the general idea. We don't completely understand the correct
> way of making sure how one module gets loaded before another, without
> selecting init level, like the fs_initcall below you commented on. I
> guess most of our submodules should be initialized during subsys_init.
> But we have not found how to specify the module init order inside subsys
> init level. Maybe you can shed some light on this? Makefile order seems
> like a fragile way of defini

Re: Media framework backwards compatibility

[Laurent Pinchart]

Hi Lane,

On Wednesday 24 November 2010 01:14:13 Lane Brooks wrote:
> On 11/23/2010 04:45 PM, Laurent Pinchart wrote:
> > On Tuesday 23 November 2010 23:29:10 Lane Brooks wrote:Laurent,
> > 
> >> If the links are setup to the resizer, then it seems that user space
> >> applications should be able to talk the resizer output (/dev/video3)
> >> like a traditional V4L2 device and need not worry about the new media
> >> framework. It even seems possible for the resizer to allow the final
> >> link format to be adjusted so that the user space application can
> >> actually adjust the resizer subdev output format across the range of
> >> valid resizer options based on the format of the resizer input pad. If
> >> the resizer output device node worked this way, then our camera would
> >> work with all the existing V4L2 applications with the simple caveat that
> >> the user has to run a separate setup application first.
> >> 
> >> The resizer output device node does not currently behave this way, and I
> >> am not sure why. These are the reasons that I can think of as to why:
> >> 1. It has not been implemented this way yet.
> >> 2. I am doing something incorrectly with the media-ctl application.
> >> 3. It not intended to work this way (by the new media framework design
> >> principles).
> >> 4. It cannot work this way because of some reason I am not considering.
> > 
> > It's probably a combination of 1 and "it cannot work this way because of
> > reasons I can't remember at 1AM" :-)
> > 
> > The ISP video device nodes implementation doesn't initialize vfh->format
> > when the device node is opened. I think this should be fixed by querying
> > to connected subdevice for its current format. Of course there could be
> > no connected subdevice when the video device node is opened, in which
> > case the format can't be initialized. Pure V4L2 applications must not
> > try to use the video device nodes before the pipeline is initialized.
> 
> I'll look into implementing this. This is mostly what I am looking for
> and hopefully won't be too involved to implement.
> 
> > Regarding adjusting the format at the output of the connected subdevice
> > when the video device node format is set, that might be possible to
> > implement, but we will run into several issues. One of them is that
> > applications currently can open the video device nodes, set the format
> > and request buffers without influencing the ISP at all. The format set
> > on the video device node will be checked against the format on the
> > connected pad at streamon time. This allows preallocating buffers for
> > snapshot capture to lower snapshot latency. Making set_format configure
> > the connected subdev directly would break this
> 
> How does calling set_format on the subdev pad at the same as the device
> node prevent preallocating buffers? I don't really understand the ISP
> buffering, so I think at this point I will look into implementing the
> previous option and then perhaps I will have a better understanding of
> the issue you raise here. I think it is only the resizer that would need
> this capability. I am bringing it up as a nice to have, but we can
> certainly live without it if it does not fit into the design goals of
> the framework.

The preallocate buffers the driver needs to know the buffer size, and that 
information is computed using the format set on the video device node. If you 
want to preallocate two sets of buffers you need to call VIDIOC_S_FMT with 
different sizes on the file handles before calling VIDIOC_REQBUFS. That's a 
hack, and that's why VIDIOC_S_FMT on the video device nodes does not configure 
the connected pad. At some point in the future we will need to brainstorm a 
buffers management API that will solve this problem in a clean way.

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/10] MCDE: Add configuration registers

[Arnd Bergmann]

On Thursday 25 November 2010, Jimmy RUBIN wrote:
> 
> All these header files,
> Configuration, pixel processing, formatter, dsi link registers are auto 
> generated from an xml file.

This actually may or may not be a legal problem, because it means that the
distributed source code is not the preferred form for making modifications
as the GPL intends, you might want to ask a lawyer. Distributing the XML
file and the script with the kernel would solve that, but people might
consider that ugly ;-).

> This is made because there are many registers which a prone to change for new 
> hardware revisions and there is a possibility to exclude registers that are 
> not used.
> The chance of manual errors are less this way.
> 
> I also believe that these helper macros makes the source code easier to read.
> For example.
> #define MCDE_CR_DSICMD2_EN_V1_SHIFT 0
> #define MCDE_CR_DSICMD2_EN_V1_MASK 0x0001
> #define MCDE_CR_DSICMD2_EN_V1(__x) \
>   MCDE_VAL2REG(MCDE_CR, DSICMD2_EN_V1, __x)
> 
> Writing a single field in the register MCDE_CR can e.g. be done like this:
> mcde_wfld(MCDE_CR, DSICMD1_EN_V1, true);
> 
> and for a whole register (a totally other register but just to show):
>   mcde_wreg(MCDE_ROTACONF + chnl_id * MCDE_ROTACONF_GROUPOFFSET,
>   MCDE_ROTACONF_ROTBURSTSIZE_ENUM(8W) |
>   MCDE_ROTACONF_ROTBURSTSIZE_HW(1) |
>   MCDE_ROTACONF_ROTDIR(regs->rotdir) |
>   MCDE_ROTACONF_STRIP_WIDTH_ENUM(16PIX) |
>   MCDE_ROTACONF_RD_MAXOUT_ENUM(4_REQ) |
>   MCDE_ROTACONF_WR_MAXOUT_ENUM(8_REQ));

I see what you mean, though I would probably still prefer an inline
function for doing the same:

static inline void mcde_write_rotaconf(struct mcde_chnl *chnl, unsigned 
burstsize,
unsigned rotdir, unsigned strip_width,
unsigned rd_maxout, wr_maxout)
{
unsigned __iomem *reg = chnl->base + MCDE_ROTACONF +
chnl->id * MCDE_ROTACONF_GROUPOFFSET;

writel(reg, burstsize << 20 | rotdir << 12 | strip_width << 8 |
rd_maxout << 4 | wr_maxout);
}

Anyway, it's not really important how you do it, this is mostly a matter
of personal preference. If you can find a way to make it more readable,
that would be really good, but it's really a minor issue compared to
getting the overall layering inside the driver right.

> I agree that the header files looks a bit unreadable I will try indent as you
> suggested I am just worried about the file size. (Patch limit 100kbyte)

Don't worry about the patch size limit too much, that is not the real
problem here ;-). For review, you can always cut parts of these files
since nobody will notice a problem in the middle of 2000 almost identical
source lines. When you ask for a git pull, the size no longer matters.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 03/12] media: Entities, pads and links

[Mark Brown]

On Thu, Nov 25, 2010 at 04:40:41PM +0100, Laurent Pinchart wrote:
> On Thursday 25 November 2010 14:36:50 Mark Brown wrote:
> > On Thu, Nov 25, 2010 at 03:28:10AM +0100, Laurent Pinchart wrote:

> > > + MEDIA_LINK_FLAG_ACTIVE indicates that the link is active and can be
> > > + used to transfer media data. When two or more links target a sink pad,
> > > + only one of them can be active at a time.

> > Is this supposed to reflect the current state (if the link is carrying
> > data right now) or if it's possible for the link to carry data?

> It's supposed to reflect whether the link can carry data. Think of the active 
> flag as a valve on a pipe. If the valve is open, the link is active. If the 
> valve is closed, the link is inactive. This is unrelated to whether water 
> actually flows through the pipe.

This seems a confusing name, then - I'd expect an active link to be one
which is actually carrying data rather than one which is available to
carry data.  How a more neutrally worded name such as "connected" (which
is what ASoC uses currently)?

This also falls through into the power management stuff, we don't want
to be powering things up unless they're actually doing something right
now.

> Immutable links have no valve (in theory you could have an inactive immutable 
> link, but that's not very useful, unless we define immutable as no user-
> controllable valve, in which case it might be better to rename it as read-
> only, or create separate immutable and read-only flags - just brainstorming 
> here).

That was what I was expecting immutable to mean - no user control.  A
link that's permanantly wired can have the data flow controlled through
its inputs and outputs, even if it is not itself controllable.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 09/12] media: Entity locking and pipeline management

[Laurent Pinchart]

Hi Mark,

On Thursday 25 November 2010 14:53:51 Mark Brown wrote:
> On Thu, Nov 25, 2010 at 03:28:16AM +0100, Laurent Pinchart wrote:
> > Link states must not be modified while streaming is in progress on a
> > graph they belong or connect to. The entity locking API helps drivers
> > enforcing that requirement.
> 
> This is not desirable for embedded audio - for example, it is very
> common to wish to switch from headphone to speaker and back mid
> playback, or to move between headset, speakerphone and handset modes on
> a phone.  This is normally implemented by reconfiguring the output paths
> at runtime without interrupting applications.

Agreed. The locking should be made optional. I'll work on that.

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 05/12] media: Reference count and power handling

[Laurent Pinchart]

Hi Mark,

On Thursday 25 November 2010 14:49:08 Mark Brown wrote:
> On Thu, Nov 25, 2010 at 03:28:12AM +0100, Laurent Pinchart wrote:
> > If the entity is of node type, the power change is distributed to
> > all connected entities. For non-nodes it only affects that very
> > node. A mutex is used to serialise access to the entity graph.
> 
> ASoC has its own power management stuff which doesn't *quite* map onto
> this one as-is.  The power determination stuff is essentially identical
> (and this is actually nicer) but we have a separate postprocessing stage
> that actually applies the changes in a sequence which minimises audible
> issues caused by doing things in a bad order (eg, power down a PGA
> before you turn off inputs).  This is noddy enough to implement, though
> - we just need a pre and post run notifications to set up and implement
> the changes I think.

That sounds feasible. Sakari, you've implemented power management, what do you 
think about this ?


> BTW, I notice you've not CCed the ALSA list, Liam, Takashi or Jaroslav
> on any of this - might be good.

I've just subscrived to the alsa-devel list. I will make sure to CC it, as 
well as Liam, Takashi and Jaroslav, on the next version of the patches. Sorry 
about the missing CC's in these e-mails.

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 03/12] media: Entities, pads and links

[Laurent Pinchart]

Hi Mark,

On Thursday 25 November 2010 14:36:50 Mark Brown wrote:
> On Thu, Nov 25, 2010 at 03:28:10AM +0100, Laurent Pinchart wrote:
> > +Links have flags that describe the link capabilities and state.
> > 
> > +   MEDIA_LINK_FLAG_ACTIVE indicates that the link is active and can be
> > +   used to transfer media data. When two or more links target a sink pad,
> > +   only one of them can be active at a time.
> 
> Is this supposed to reflect the current state (if the link is carrying
> data right now) or if it's possible for the link to carry data?

It's supposed to reflect whether the link can carry data. Think of the active 
flag as a valve on a pipe. If the valve is open, the link is active. If the 
valve is closed, the link is inactive. This is unrelated to whether water 
actually flows through the pipe.

Immutable links have no valve (in theory you could have an inactive immutable 
link, but that's not very useful, unless we define immutable as no user-
controllable valve, in which case it might be better to rename it as read-
only, or create separate immutable and read-only flags - just brainstorming 
here).

> > +struct media_entity {
> > +   struct list_head list;
> > +   struct media_device *parent;/* Media device this entity belongs to*/
> > +   u32 id; /* Entity ID, unique in the parent media
> > +* device context */
> > +   const char *name;   /* Entity name */
> > +   u32 type;   /* Entity type (MEDIA_ENTITY_TYPE_*) */
> > +   u32 revision;   /* Entity revision, driver specific */
> > +   unsigned long flags;/* Entity flags (MEDIA_ENTITY_FLAG_*) */
> > +   u32 group_id;   /* Entity group ID */
> > +
> > +   u16 num_pads;   /* Number of input and output pads */
> > +   u16 num_links;  /* Number of existing links, both active
> > +* and inactive */
> > +   u16 num_backlinks;  /* Number of backlinks */
> > +   u16 max_links;  /* Maximum number of links */
> > +
> > +   struct media_pad *pads; /* Pads array (num_pads elements) */
> > +   struct media_link *links;   /* Links array (max_links elements)*/
> 
> Hrm.  This is getting kind of large, especially considering the volume
> of data we're holding per node and link already in ASoC.  On the other
> hand we may over time be able to refactor some of the existing stuff
> (especially the link management) to use this structure.

That's the idea :-) In the long term (hopefully not that long) I would like to 
see both sources of information merged. That's why I'm interested in your 
requirements to make sure this will be possible.

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 03/12] [alsa-devel] media: Entities, pads and links

[Mark Brown]

On Thu, Nov 25, 2010 at 04:29:53PM +0100, Laurent Pinchart wrote:

> It depends on how you define nodes. I can certainly imagine a graph with 100 
> controls, but maybe several controls can be part of the same node ? On the 
> video side we've decided to split entities depending on the possible data 
> paths configurations. As I'm not a fluent ascii-art speaker, please have a 
> look at pages 4 and 5 of http://www.ideasonboard.org/media/20101103-lpc-
> media.pdf

Please take a look at:

http://www.wolfsonmicro.com/products/WM8994

for an example of a modern smartphone CODEC.  This has in the ballpark
of 75-100 nodes (I've not counted exactly) represented in the routing
graph in the driver today.  This does not include any audio routing
available in other components such as the CPU.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 03/12] [alsa-devel] media: Entities, pads and links

[Laurent Pinchart]

Hi Mark,

On Thursday 25 November 2010 14:41:35 Mark Brown wrote:
> On Thu, Nov 25, 2010 at 10:38:05AM +0100, Clemens Ladisch wrote:
> > In USB and HD audio devices, all links are immutable, and the routing
> > is controlled by 'selector' entities that activate exactly one of their
> > input pads.  In userspace, this entity shows up as a mixer control.
> > I guess it would be possible to map the ACTIVE flag onto these controls.
> 
> Ditto for ASoC, mostly.
> 
> > Alternatively, entities can have 'mute' mixer controls associated with
> > their pads.  In this case, multiple unmuted inputs would be mixed
> > together.
> > 
> > ALSA has PCM and MIDI devices, and several types of mixer controls.
> > (It also has hardware dependent and timer devices, but I don't think
> > 
> > these would need topology information.)  So we need at least these:
> >   MEDIA_ENTITY_TYPE_NODE_ALSA_PCM
> >   MEDIA_ENTITY_TYPE_NODE_ALSA_MIDI
> >   MEDIA_ENTITY_TYPE_SUBDEV_ALSA_CONTROL
> > 
> > Furthermore, topology information is also needed for entities not
> > associated with a mixer control, such as microphones, speakers, jacks/
> > connectors, and effect units.  These entities are defined in the USB and
> > HD audio specifications, but are not yet handled by ALSA.
> 
> All this and more in the embedded case - digital audio link nodes and DSP
> I/O nodes (could possibly do those as digital audio ones) spring to
> mind.  Also bear in mind that embedded devices can get *very* large - a
> mobile phone audio system can have of the order of 100 nodes in the
> graph.

It depends on how you define nodes. I can certainly imagine a graph with 100 
controls, but maybe several controls can be part of the same node ? On the 
video side we've decided to split entities depending on the possible data 
paths configurations. As I'm not a fluent ascii-art speaker, please have a 
look at pages 4 and 5 of http://www.ideasonboard.org/media/20101103-lpc-
media.pdf

Page 4 shows the internal topology of the OMAP3 ISP. The major blocks in that 
diagram are reported as entities. Page 5 shows the internal topology of one of 
the blocks, the OMAP3 ISP preview engine. As you can see the pipeline is made 
of sub-blocks that implement a single image processing function. As the 
pipeline is linear (don't worry about the non-linear part in the beginning, 
it's just there to take into account link configurability at the higher level) 
we don't export all the sub-blocks as entities, but we expose the controls on 
the preview engine entity instead.

> > ALSA devices are not addressed by their device node but with card/device/
> > 
> > subdevice numbers; mixer controls have numeric IDs, unique per card:
> > struct {
> > 
> > int card;
> > int device;
> > int subdevice;
> > 
> > } alsa_device;
> > struct {
> > 
> > int card;
> > int numid;
> > 
> > } alsa_control;
> 
> For the embedded stuff we also have a bunch of stuff in the graph which
> may not be visible to userspace at all at present and would just have a
> string based identifier.

That could be easily added (provided the string is not too long).

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 03/12] [alsa-devel] media: Entities, pads and links

[Mark Brown]

On Thu, Nov 25, 2010 at 04:21:38PM +0100, Laurent Pinchart wrote:
> On Thursday 25 November 2010 10:38:05 Clemens Ladisch wrote:

> > ALSA has PCM and MIDI devices, and several types of mixer controls.
> > (It also has hardware dependent and timer devices, but I don't think
> > these would need topology information.)  So we need at least these:
> >   MEDIA_ENTITY_TYPE_NODE_ALSA_PCM
> >   MEDIA_ENTITY_TYPE_NODE_ALSA_MIDI
> >   MEDIA_ENTITY_TYPE_SUBDEV_ALSA_CONTROL

> I agree about PCM and MIDI, but I'm not sure about controls. If controls are 
> part of an entity, the entity will be reported through the media controller 
> API. If information about that entity can be queried through existing APIs 
> (ALSA, V4L, ...) those APIs should be used. For instance, on the V4L side, 
> V4L2 sub-devices are mapped to entities and also have a device node which can 
> be used to enumerate the controls supported by the subdev. The media 
> controller only reports the entity -> major:minor mapping to let applications 
> find the device and query it directly.

For audio we don't currently have a sensible API for associating
controls with any sort of map of how the device is laid out, userspace
has to play guessing games.

> I think we will need a new ioctl in the media controller API to report 
> advanced information about an entity. This could be used to report controls 
> implemented by an ALSA element if ALSA doesn't provide a way to do that 
> directly. Another use case, which make me think that such an ioctl would be 
> needed, is to report UVC extension units type (16-byte GUID) to userspace.

That seems reasonable.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-dvb] [bug] AF9015 message "WARNING: >>> tuning failed!!!"

[Antti Palosaari]

On 11/23/2010 10:11 PM, Paul Gover wrote:

On Wednesday 06 October 2010 22:29:35 Antti Palosaari wrote:

On 10/06/2010 11:36 PM, dave cunningham wrote:

In message<4cacd0f3.6030...@iki.fi>, Antti Palosaari wrote


It is QT1010 tuner driver issue. None is working for that currently or
in near future. Feel free to fix :]


The wiki appears to show this stick as working.
.

Is this information incorrect or is it hit and miss depending on the
host system?


It "works" but performance is poor. Usually it locks when RF signal is
weak. If you fix bug around line 381 in qt1010.c it will work much
better. But if you fix that it will break devices zl10353+qt1010 since
zl10353 driver misses AGC configuration.

Antti


Antti,

I took a look at qt1010.c, but didn't see what the bug was.  I was hoping to
see a FIXME or BUG or some comment; any comment ;-)


Look this, I think it does have that fixed.
http://linuxtv.org/hg/~anttip/qt1010/


But then I went back to my traces.  They said my AF9015 detected an MT2060
tuner, not a QT1010.  Does this mean the QT1010 comment was wrong?  The
detection code that said so looks to be part of the AF9015/9013 support, maybe
they use the same code.  Whatever, you will know, and I certainly don't.  The
tuner code certainly uses the QT1010 driver and not the MT2060 driver, if I
understood the traces correctly.


You surely misunderstand something now. Look your first post:
"Quantek QT1010 successfully identified."


FWIW, I think you are right about AGC; when Kaffeine scans for channels, it
gives a few fleeting signal levels about 50% but fails to identify anything.
My previous DVB-T stick, different older chip, produced 100% signal solidly,
and Kaffeine identified more than 80 channels.

I think this bug renders the AF9015 in this configuration virtually useless in
Linux; the signal is enough for the Windoze tuner bloatware supplied with it
to find all the channels, but not a thing in Linux.

Sorry for coming back on this so much later; I've been busy doing house
repair.  Also, I stopped following the mailing list - I was getting swamped
with stuff and it seems to make yahoo break KDE PIM.  If I should post this via
the list, please say, and I'll start obeying the rules!

Thanks in advance for any guidance.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
http://palosaari.fi/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 03/12] [alsa-devel] media: Entities, pads and links

[Laurent Pinchart]

Hi Clemens,

Thanks a lot for the review.

On Thursday 25 November 2010 10:38:05 Clemens Ladisch wrote:
> Laurent Pinchart wrote:
> > A link is a point-to-point oriented connection between two pads, either
> > on the same entity or on different entities. Data flows from a source
> > pad to a sink pad.
> > 
> > Links are stored in the source entity.
> 
> In the descriptors of USB Audio and HDAudio devices, the links are
> stored only in the sink.  But AFAICS this doesn't matter in the media
> driver API.

Same for USB Video (UVC). The reason for that is that UAC and UVC have only 1-
to-many connections (same for the media controller by the way), and it's 
easier to store that information in fixed-size structures if you store it in 
the sink entities.

On the kernel side, the media controller stores links in both source and sink 
entities for internal reasons. Links stored in the sink entity are called 
backlinks.

On the userspace side applications will only see "forward" links, stored in 
source entities. Applications will of course be free to store the links 
whereever they want in their internal data structures.

As you mentioned, I think it doesn't matter much anyway.

> > +Links have flags that describe the link capabilities and state.
> > +
> > +   MEDIA_LINK_FLAG_ACTIVE indicates that the link is active and can be
> > +   used to transfer media data. When two or more links target a sink pad,
> > +   only one of them can be active at a time.
> > +   MEDIA_LINK_FLAG_IMMUTABLE indicates that the link active state can't
> > +   be modified at runtime. If MEDIA_LINK_FLAG_IMMUTABLE is set, then
> > +   MEDIA_LINK_FLAG_ACTIVE must also be set since an immutable link is
> > +   always active.
> 
> So the intended userspace API for controlling routing is to change the
> ACTIVE flag of links?

That's correct.

> In USB and HD audio devices, all links are immutable, and the routing
> is controlled by 'selector' entities that activate exactly one of their
> input pads.  In userspace, this entity shows up as a mixer control.
> I guess it would be possible to map the ACTIVE flag onto these controls.

Same for UVC once again. It would be quite easy to map selector units to link 
activation, but it doesn't even have to be done. We could keep the selector 
units in the graph with all links immutable, and use the existing APIs to 
control the selector units.

The alternative would have been to add selector units to the media controller. 
I've thought about it, but we have embedded hardware (at least on the video 
side) that have configurable links without selector units. The media 
controller would have had to create fake selector units.

Between creating fake selector and translating link activation to selector 
unit commands, I've decided to go for the second solution, as this will make 
the graph simpler. Once again we can decide to keep explicit selector units in 
the graph for UVC, UAC and HD audio devices. That's one I've done in a test 
implementation of the media controller API in the uvcvideo driver.

> Alternatively, entities can have 'mute' mixer controls associated with
> their pads.  In this case, multiple unmuted inputs would be mixed
> together.

I don't see any problem keeping those.

> > +#define MEDIA_ENTITY_TYPE_MASK   0x00ff
> > +#define MEDIA_ENTITY_SUBTYPE_MASK0x
> > +
> > +#define MEDIA_ENTITY_TYPE_NODE   (1 <<
> > MEDIA_ENTITY_TYPE_SHIFT) ...
> > +#define MEDIA_ENTITY_TYPE_NODE_ALSA  (MEDIA_ENTITY_TYPE_NODE +
> > 3)
> 
> ALSA has PCM and MIDI devices, and several types of mixer controls.
> (It also has hardware dependent and timer devices, but I don't think
> these would need topology information.)  So we need at least these:
>   MEDIA_ENTITY_TYPE_NODE_ALSA_PCM
>   MEDIA_ENTITY_TYPE_NODE_ALSA_MIDI
>   MEDIA_ENTITY_TYPE_SUBDEV_ALSA_CONTROL

I agree about PCM and MIDI, but I'm not sure about controls. If controls are 
part of an entity, the entity will be reported through the media controller 
API. If information about that entity can be queried through existing APIs 
(ALSA, V4L, ...) those APIs should be used. For instance, on the V4L side, 
V4L2 sub-devices are mapped to entities and also have a device node which can 
be used to enumerate the controls supported by the subdev. The media 
controller only reports the entity -> major:minor mapping to let applications 
find the device and query it directly.

I think we will need a new ioctl in the media controller API to report 
advanced information about an entity. This could be used to report controls 
implemented by an ALSA element if ALSA doesn't provide a way to do that 
directly. Another use case, which make me think that such an ioctl would be 
needed, is to report UVC extension units type (16-byte GUID) to userspace.

> Furthermore, topology information is also needed for entities not
> associated with a mixer control, such as microphones, speakers, jacks/
> connectors, and effect units

[PATCH 6/6] [media] s5p-fimc: Fix output DMA handling in S5PV310 IP revisions

[Sylwester Nawrocki]

FIMC IP in S5Pv310 series has extended DMA status registers
and some bit fields are marked as reserved comparing to S5PC100/110.
Use correct registers for getting DMA write pointer in each SoC variant
supported by the driver.

Signed-off-by: Sylwester Nawrocki 
Signed-off-by: Kyungmin Park 
---
 drivers/media/video/s5p-fimc/fimc-capture.c |1 +
 drivers/media/video/s5p-fimc/fimc-core.c|2 ++
 drivers/media/video/s5p-fimc/fimc-core.h|   16 +---
 drivers/media/video/s5p-fimc/regs-fimc.h|3 +++
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c 
b/drivers/media/video/s5p-fimc/fimc-capture.c
index c020bf6..ba18acb 100644
--- a/drivers/media/video/s5p-fimc/fimc-capture.c
+++ b/drivers/media/video/s5p-fimc/fimc-capture.c
@@ -522,6 +522,7 @@ static int fimc_cap_streamon(struct file *file, void *priv,
INIT_LIST_HEAD(&fimc->vid_cap.active_buf_q);
fimc->vid_cap.active_buf_cnt = 0;
fimc->vid_cap.frame_count = 0;
+   fimc->vid_cap.buf_index = fimc_hw_get_frame_index(fimc);
 
set_bit(ST_CAPT_PEND, &fimc->state);
ret = videobuf_streamon(&fimc->vid_cap.vbq);
diff --git a/drivers/media/video/s5p-fimc/fimc-core.c 
b/drivers/media/video/s5p-fimc/fimc-core.c
index f538eb5..bb99f2d 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.c
+++ b/drivers/media/video/s5p-fimc/fimc-core.c
@@ -1746,6 +1746,7 @@ static struct samsung_fimc_variant fimc0_variant_s5pv310 
= {
.pix_hoff= 1,
.has_inp_rot = 1,
.has_out_rot = 1,
+   .has_cistatus2   = 1,
.min_inp_pixsize = 16,
.min_out_pixsize = 16,
.hor_offs_align  = 1,
@@ -1755,6 +1756,7 @@ static struct samsung_fimc_variant fimc0_variant_s5pv310 
= {
 
 static struct samsung_fimc_variant fimc2_variant_s5pv310 = {
.pix_hoff= 1,
+   .has_cistatus2   = 1,
.min_inp_pixsize = 16,
.min_out_pixsize = 16,
.hor_offs_align  = 1,
diff --git a/drivers/media/video/s5p-fimc/fimc-core.h 
b/drivers/media/video/s5p-fimc/fimc-core.h
index 6137340..4f047d3 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.h
+++ b/drivers/media/video/s5p-fimc/fimc-core.h
@@ -371,6 +371,7 @@ struct fimc_pix_limit {
  * @pix_hoff: indicate whether horizontal offset is in pixels or in bytes
  * @has_inp_rot: set if has input rotator
  * @has_out_rot: set if has output rotator
+ * @has_cistatus2: 1 if CISTATUS2 register is present in this IP revision
  * @pix_limit: pixel size constraints for the scaler
  * @min_inp_pixsize: minimum input pixel size
  * @min_out_pixsize: minimum output pixel size
@@ -381,6 +382,7 @@ struct samsung_fimc_variant {
unsigned intpix_hoff:1;
unsigned inthas_inp_rot:1;
unsigned inthas_out_rot:1;
+   unsigned inthas_cistatus2:1;
struct fimc_pix_limit *pix_limit;
u16 min_inp_pixsize;
u16 min_out_pixsize;
@@ -556,11 +558,19 @@ static inline struct fimc_frame *ctx_get_frame(struct 
fimc_ctx *ctx,
return frame;
 }
 
+/* Return an index to the buffer actually being written. */
 static inline u32 fimc_hw_get_frame_index(struct fimc_dev *dev)
 {
-   u32 reg = readl(dev->regs + S5P_CISTATUS);
-   return (reg & S5P_CISTATUS_FRAMECNT_MASK) >>
-   S5P_CISTATUS_FRAMECNT_SHIFT;
+   u32 reg;
+
+   if (dev->variant->has_cistatus2) {
+   reg = readl(dev->regs + S5P_CISTATUS2) & 0x3F;
+   return reg > 0 ? --reg : reg;
+   } else {
+   reg = readl(dev->regs + S5P_CISTATUS);
+   return (reg & S5P_CISTATUS_FRAMECNT_MASK) >>
+   S5P_CISTATUS_FRAMECNT_SHIFT;
+   }
 }
 
 /* -*/
diff --git a/drivers/media/video/s5p-fimc/regs-fimc.h 
b/drivers/media/video/s5p-fimc/regs-fimc.h
index a57daed..57e33f8 100644
--- a/drivers/media/video/s5p-fimc/regs-fimc.h
+++ b/drivers/media/video/s5p-fimc/regs-fimc.h
@@ -165,6 +165,9 @@
 #define S5P_CISTATUS_VVALID_A  (1 << 15)
 #define S5P_CISTATUS_VVALID_B  (1 << 14)
 
+/* Indexes to the last and the currently processed buffer. */
+#define S5P_CISTATUS2  0x68
+
 /* Image capture control */
 #define S5P_CIIMGCPT   0xc0
 #define S5P_CIIMGCPT_IMGCPTEN  (1 << 31)
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] [media] s5p-fimc: Use correct fourcc code for 32-bit RGB format

[Sylwester Nawrocki]

Replace V4L2_PIX_FMT_RGB24 code with V4L2_PIX_FMT_RGB32
since the hardware uses 24-bits for actual pixel data but pixels
are 4-byte aligned in memory.

Signed-off-by: Sylwester Nawrocki 
Signed-off-by: Kyungmin Park 
---
 drivers/media/video/s5p-fimc/fimc-core.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-core.c 
b/drivers/media/video/s5p-fimc/fimc-core.c
index fa82a2a..f538eb5 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.c
+++ b/drivers/media/video/s5p-fimc/fimc-core.c
@@ -50,8 +50,8 @@ static struct fimc_fmt fimc_formats[] = {
.planes_cnt = 1,
.flags = FMT_FLAGS_M2M,
}, {
-   .name = "XRGB-8-8-8-8, 24 bpp",
-   .fourcc = V4L2_PIX_FMT_RGB24,
+   .name = "XRGB-8-8-8-8, 32 bpp",
+   .fourcc = V4L2_PIX_FMT_RGB32,
.depth = 32,
.color  = S5P_FIMC_RGB888,
.buff_cnt = 1,
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] [media] s5p-fimc: Explicitly add required header file

[Sylwester Nawrocki]

Reported by: Dan Carpenter 
Signed-off-by: Sylwester Nawrocki 
Signed-off-by: Kyungmin Park 
---
 drivers/media/video/s5p-fimc/fimc-core.h |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-core.h 
b/drivers/media/video/s5p-fimc/fimc-core.h
index afafebc..6137340 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.h
+++ b/drivers/media/video/s5p-fimc/fimc-core.h
@@ -13,13 +13,15 @@
 
 /*#define DEBUG*/
 
+#include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
-#include 
+
 #include "regs-fimc.h"
 
 #define err(fmt, args...) \
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/6] [media] s5p-fimc: Convert m2m driver to unlocked_ioctl

[Sylwester Nawrocki]

Signed-off-by: Sylwester Nawrocki 
Signed-off-by: Kyungmin Park 
---
 drivers/media/video/s5p-fimc/fimc-core.c |   22 --
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-core.c 
b/drivers/media/video/s5p-fimc/fimc-core.c
index f45970d..fa82a2a 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.c
+++ b/drivers/media/video/s5p-fimc/fimc-core.c
@@ -983,6 +983,7 @@ int fimc_vidioc_queryctrl(struct file *file, void *priv,
 {
struct fimc_ctx *ctx = priv;
struct v4l2_queryctrl *c;
+   int ret = -EINVAL;
 
c = get_ctrl(qc->id);
if (c) {
@@ -990,10 +991,14 @@ int fimc_vidioc_queryctrl(struct file *file, void *priv,
return 0;
}
 
-   if (ctx->state & FIMC_CTX_CAP)
-   return v4l2_subdev_call(ctx->fimc_dev->vid_cap.sd,
+   if (ctx->state & FIMC_CTX_CAP) {
+   if (mutex_lock_interruptible(&ctx->fimc_dev->lock))
+   return -ERESTARTSYS;
+   ret = v4l2_subdev_call(ctx->fimc_dev->vid_cap.sd,
core, queryctrl, qc);
-   return -EINVAL;
+   mutex_unlock(&ctx->fimc_dev->lock);
+   }
+   return ret;
 }
 
 int fimc_vidioc_g_ctrl(struct file *file, void *priv,
@@ -1233,6 +1238,9 @@ static int fimc_m2m_s_crop(struct file *file, void *fh, 
struct v4l2_crop *cr)
f = (cr->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) ?
&ctx->s_frame : &ctx->d_frame;
 
+   if (mutex_lock_interruptible(&fimc->lock))
+   return -ERESTARTSYS;
+
spin_lock_irqsave(&ctx->slock, flags);
if (~ctx->state & (FIMC_SRC_FMT | FIMC_DST_FMT)) {
/* Check to see if scaling ratio is within supported range */
@@ -1241,9 +1249,9 @@ static int fimc_m2m_s_crop(struct file *file, void *fh, 
struct v4l2_crop *cr)
else
ret = fimc_check_scaler_ratio(&cr->c, &ctx->s_frame);
if (ret) {
-   spin_unlock_irqrestore(&ctx->slock, flags);
v4l2_err(&fimc->m2m.v4l2_dev, "Out of scaler range");
-   return -EINVAL;
+   ret = -EINVAL;
+   goto scr_unlock;
}
}
ctx->state |= FIMC_PARAMS;
@@ -1253,7 +1261,9 @@ static int fimc_m2m_s_crop(struct file *file, void *fh, 
struct v4l2_crop *cr)
f->width  = cr->c.width;
f->height = cr->c.height;
 
+scr_unlock:
spin_unlock_irqrestore(&ctx->slock, flags);
+   mutex_unlock(&fimc->lock);
return 0;
 }
 
@@ -1396,7 +1406,7 @@ static const struct v4l2_file_operations fimc_m2m_fops = {
.open   = fimc_m2m_open,
.release= fimc_m2m_release,
.poll   = fimc_m2m_poll,
-   .ioctl  = video_ioctl2,
+   .unlocked_ioctl = video_ioctl2,
.mmap   = fimc_m2m_mmap,
 };
 
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] [media] s5p-fimc: Fix vidioc_g_crop/cropcap on camera sensor

[Sylwester Nawrocki]

Create separate vidioc_g_crop/vidioc_s_crop handlers for capture
video node and so image cropping parameters are properly queried
at FIMC input (image sensor) and not at FIMC output.

Signed-off-by: Sylwester Nawrocki 
Signed-off-by: Kyungmin Park 
---
 drivers/media/video/s5p-fimc/fimc-capture.c |   48 +-
 drivers/media/video/s5p-fimc/fimc-core.c|   26 +++---
 drivers/media/video/s5p-fimc/fimc-core.h|4 --
 3 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c 
b/drivers/media/video/s5p-fimc/fimc-capture.c
index 4c70fba..c020bf6 100644
--- a/drivers/media/video/s5p-fimc/fimc-capture.c
+++ b/drivers/media/video/s5p-fimc/fimc-capture.c
@@ -652,6 +652,50 @@ static int fimc_cap_s_ctrl(struct file *file, void *priv,
return ret;
 }
 
+static int fimc_cap_cropcap(struct file *file, void *fh,
+   struct v4l2_cropcap *cr)
+{
+   struct fimc_frame *f;
+   struct fimc_ctx *ctx = fh;
+   struct fimc_dev *fimc = ctx->fimc_dev;
+
+   if (cr->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+   return -EINVAL;
+
+   if (mutex_lock_interruptible(&fimc->lock))
+   return -ERESTARTSYS;
+
+   f = &ctx->s_frame;
+   cr->bounds.left = 0;
+   cr->bounds.top  = 0;
+   cr->bounds.width= f->o_width;
+   cr->bounds.height   = f->o_height;
+   cr->defrect = cr->bounds;
+
+   mutex_unlock(&fimc->lock);
+   return 0;
+}
+
+static int fimc_cap_g_crop(struct file *file, void *fh, struct v4l2_crop *cr)
+{
+   struct fimc_frame *f;
+   struct fimc_ctx *ctx = file->private_data;
+   struct fimc_dev *fimc = ctx->fimc_dev;
+
+
+   if (mutex_lock_interruptible(&fimc->lock))
+   return -ERESTARTSYS;
+
+   f = &ctx->s_frame;
+   cr->c.left  = f->offs_h;
+   cr->c.top   = f->offs_v;
+   cr->c.width = f->width;
+   cr->c.height= f->height;
+
+   mutex_unlock(&fimc->lock);
+   return 0;
+}
+
 static int fimc_cap_s_crop(struct file *file, void *fh,
   struct v4l2_crop *cr)
 {
@@ -716,9 +760,9 @@ static const struct v4l2_ioctl_ops fimc_capture_ioctl_ops = 
{
.vidioc_g_ctrl  = fimc_vidioc_g_ctrl,
.vidioc_s_ctrl  = fimc_cap_s_ctrl,
 
-   .vidioc_g_crop  = fimc_vidioc_g_crop,
+   .vidioc_g_crop  = fimc_cap_g_crop,
.vidioc_s_crop  = fimc_cap_s_crop,
-   .vidioc_cropcap = fimc_vidioc_cropcap,
+   .vidioc_cropcap = fimc_cap_cropcap,
 
.vidioc_enum_input  = fimc_cap_enum_input,
.vidioc_s_input = fimc_cap_s_input,
diff --git a/drivers/media/video/s5p-fimc/fimc-core.c 
b/drivers/media/video/s5p-fimc/fimc-core.c
index 2e7c547..f45970d 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.c
+++ b/drivers/media/video/s5p-fimc/fimc-core.c
@@ -1115,7 +1115,7 @@ static int fimc_m2m_s_ctrl(struct file *file, void *priv,
return 0;
 }
 
-int fimc_vidioc_cropcap(struct file *file, void *fh,
+static int fimc_m2m_cropcap(struct file *file, void *fh,
struct v4l2_cropcap *cr)
 {
struct fimc_frame *frame;
@@ -1139,7 +1139,7 @@ int fimc_vidioc_cropcap(struct file *file, void *fh,
return 0;
 }
 
-int fimc_vidioc_g_crop(struct file *file, void *fh, struct v4l2_crop *cr)
+static int fimc_m2m_g_crop(struct file *file, void *fh, struct v4l2_crop *cr)
 {
struct fimc_frame *frame;
struct fimc_ctx *ctx = file->private_data;
@@ -1167,22 +1167,22 @@ int fimc_try_crop(struct fimc_ctx *ctx, struct 
v4l2_crop *cr)
struct fimc_frame *f;
u32 min_size, halign;
 
-   f = (cr->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) ?
-   &ctx->s_frame : &ctx->d_frame;
-
if (cr->c.top < 0 || cr->c.left < 0) {
v4l2_err(&fimc->m2m.v4l2_dev,
"doesn't support negative values for top & left\n");
return -EINVAL;
}
 
-   f = ctx_get_frame(ctx, cr->type);
-   if (IS_ERR(f))
-   return PTR_ERR(f);
+   if (cr->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
+   f = (ctx->state & FIMC_CTX_CAP) ? &ctx->s_frame : &ctx->d_frame;
+   else if (cr->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
+ctx->state & FIMC_CTX_M2M)
+   f = &ctx->s_frame;
+   else
+   return -EINVAL;
 
-   min_size = (cr->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
-   ? fimc->variant->min_inp_pixsize
-   : fimc->variant->min_out_pixsize;
+   min_size = (f == &ctx->s_frame) ?
+   fimc->variant->min_inp_pixsize : fimc->variant->min_out_pixsize;
 
if (ctx->state & FIMC_CTX_M2M) {
if (fimc->id == 1 && fimc->variant->pix_hoff)
@@ -1285,9 +1285,9 @@ static

[PATCH 1/6] [media] s5p-fimc: BKL lock removal - compilation fix

[Sylwester Nawrocki]

Adapt to recent videobuf_queue_dma_contig_init signature change.

Signed-off-by: Sylwester Nawrocki 
Signed-off-by: Kyungmin 
---
 drivers/media/video/s5p-fimc/fimc-capture.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c 
b/drivers/media/video/s5p-fimc/fimc-capture.c
index e8f13d3..4c70fba 100644
--- a/drivers/media/video/s5p-fimc/fimc-capture.c
+++ b/drivers/media/video/s5p-fimc/fimc-capture.c
@@ -785,7 +785,7 @@ int fimc_register_capture_device(struct fimc_dev *fimc)
videobuf_queue_dma_contig_init(&vid_cap->vbq, &fimc_qops,
vid_cap->v4l2_dev.dev, &fimc->irqlock,
V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FIELD_NONE,
-   sizeof(struct fimc_vid_buffer), (void *)ctx);
+   sizeof(struct fimc_vid_buffer), (void *)ctx, NULL);
 
ret = video_register_device(vfd, VFL_TYPE_GRABBER, -1);
if (ret) {
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/6] s5p-fimc driver fixes for 2.6.37

[Sylwester Nawrocki]

Hello,

the following are a few bugfix patches for s5p-fimc driver.
Two of them are related to recent BKL removal efforts and others 
are minor fixes for V4L2 API conformance and correct handling of
different IP revisions.

The patch series contains:

[PATCH 1/6] [media] s5p-fimc: BKL lock removal - compilation fix
[PATCH 2/6] [media] s5p-fimc: Fix vidioc_g_crop/cropcap on camera sensor
[PATCH 3/6] [media] s5p-fimc: Explicitly add required header file
[PATCH 4/6] [media] s5p-fimc: Convert m2m driver to unlocked_ioctl
[PATCH 5/6] [media] s5p-fimc: Use correct fourcc code for 32-bit RGB format
[PATCH 6/6] [media] s5p-fimc: Fix output DMA handling in S5PV310 IP revisions

It has been rebased onto linuxtv/staging-2.6.37-rc1 branch at
git://linuxtv.org/media_tree.git.

Thanks,
Sylwester

--
Sylwester Nawrocki
Linux Platform Group
Samsung Poland R&D Center




--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 02/12] media: Media device

[Laurent Pinchart]

Hi Clemens,

Thanks for the review.

On Thursday 25 November 2010 10:33:02 Clemens Ladisch wrote:
> Laurent Pinchart wrote:
> > +struct media_device {
> > ...
> > +   u8 model[32];
> > +   u8 serial[40];
> > +   u8 bus_info[32];
> 
> All drivers and userspace applications have to treat this as char[], so
> why u8[]?

Good question. I've copied the V4L2 practice of using u8 (or __u8) for fixed-
length strings in structures. I can't think of any reason for that.

I will replace u8 with char unless someone comes up with a good reason to keep 
u8.

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 11/12] v4l: Make video_device inherit from media_entity

[Laurent Pinchart]

Hi Hans,

Thanks for the review.

On Thursday 25 November 2010 12:38:15 Hans Verkuil wrote:
> On Thursday, November 25, 2010 03:28:18 Laurent Pinchart wrote:
> > V4L2 devices are media entities. As such they need to inherit from
> > (include) the media_entity structure.
> > 
> > When registering/unregistering the device, the media entity is
> > automatically registered/unregistered. The entity is acquired on device
> > open and released on device close.
> > 
> > Signed-off-by: Laurent Pinchart 
> > Signed-off-by: Sakari Ailus 
> > ---
> > 
> >  Documentation/video4linux/v4l2-framework.txt |   38
> >  +++-- drivers/media/video/v4l2-dev.c   |  
> >  47 +++--- include/media/v4l2-dev.h 
> > |7 
> >  3 files changed, 84 insertions(+), 8 deletions(-)
> 
> 
> 
> > diff --git a/drivers/media/video/v4l2-dev.c
> > b/drivers/media/video/v4l2-dev.c index 035db52..511e1ee 100644
> > --- a/drivers/media/video/v4l2-dev.c
> > +++ b/drivers/media/video/v4l2-dev.c

[snip]

> > @@ -558,12 +579,25 @@ int __video_register_device(struct video_device
> > *vdev, int type, int nr,
> > 
> > if (nr != -1 && nr != vdev->num && warn_if_nr_in_use)
> > 
> > printk(KERN_WARNING "%s: requested %s%d, got %s\n", __func__,
> > 
> > name_base, nr, video_device_node_name(vdev));
> > 
> > -
> > -   /* Part 5: Activate this minor. The char device can now be used. */
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +   /* Part 5: Register the entity. */
> > +   if (vdev->v4l2_dev && vdev->v4l2_dev->mdev) {
> > +   vdev->entity.type = MEDIA_ENTITY_TYPE_NODE_V4L;
> > +   vdev->entity.name = vdev->name;
> > +   vdev->entity.v4l.major = VIDEO_MAJOR;
> > +   vdev->entity.v4l.minor = vdev->minor;
> > +   ret = media_device_register_entity(vdev->v4l2_dev->mdev,
> > +   &vdev->entity);
> > +   if (ret < 0)
> > +   printk(KERN_ERR "error\n"); /* TODO */
> 
> Was this forgotten, or will this be fixed in the next version? It looks
> out-of-place...

OOPS. I totally forgot about that one. I'll fix it for the next version.

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 00/12] Media controller (core and V4L2)

[Mark Brown]

On Thu, Nov 25, 2010 at 03:28:07AM +0100, Laurent Pinchart wrote:

> I want to emphasize that the media controller API does *not* replace the V4L,
> DVB or ALSA APIs. It complements them.

Overall this looks relatively good and should be mappable onto the
embedded audio stack.  I'd need to sit down and actually do that to
go into too much specific detail but the only real issue I see is
working out how the control aspects of this interact with the control in
ALSA.  The simplest thing would be to have the userspace API be read
only for ALSA devices, I guess.

One potential issue 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 09/12] media: Entity locking and pipeline management

[Mark Brown]

On Thu, Nov 25, 2010 at 03:28:16AM +0100, Laurent Pinchart wrote:
> Link states must not be modified while streaming is in progress on a
> graph they belong or connect to. The entity locking API helps drivers
> enforcing that requirement.

This is not desirable for embedded audio - for example, it is very
common to wish to switch from headphone to speaker and back mid
playback, or to move between headset, speakerphone and handset modes on
a phone.  This is normally implemented by reconfiguring the output paths
at runtime without interrupting applications.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 05/12] media: Reference count and power handling

[Mark Brown]

On Thu, Nov 25, 2010 at 03:28:12AM +0100, Laurent Pinchart wrote:

>   If the entity is of node type, the power change is distributed to
>   all connected entities. For non-nodes it only affects that very
>   node. A mutex is used to serialise access to the entity graph.

ASoC has its own power management stuff which doesn't *quite* map onto
this one as-is.  The power determination stuff is essentially identical
(and this is actually nicer) but we have a separate postprocessing stage
that actually applies the changes in a sequence which minimises audible
issues caused by doing things in a bad order (eg, power down a PGA
before you turn off inputs).  This is noddy enough to implement, though
- we just need a pre and post run notifications to set up and implement
the changes I think.

BTW, I notice you've not CCed the ALSA list, Liam, Takashi or Jaroslav
on any of this - might be good.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 03/12] [alsa-devel] media: Entities, pads and links

[Mark Brown]

On Thu, Nov 25, 2010 at 10:38:05AM +0100, Clemens Ladisch wrote:

> In USB and HD audio devices, all links are immutable, and the routing
> is controlled by 'selector' entities that activate exactly one of their
> input pads.  In userspace, this entity shows up as a mixer control.
> I guess it would be possible to map the ACTIVE flag onto these controls.

Ditto for ASoC, mostly.

> Alternatively, entities can have 'mute' mixer controls associated with
> their pads.  In this case, multiple unmuted inputs would be mixed
> together.

> ALSA has PCM and MIDI devices, and several types of mixer controls.
> (It also has hardware dependent and timer devices, but I don't think
> these would need topology information.)  So we need at least these:
>   MEDIA_ENTITY_TYPE_NODE_ALSA_PCM
>   MEDIA_ENTITY_TYPE_NODE_ALSA_MIDI
>   MEDIA_ENTITY_TYPE_SUBDEV_ALSA_CONTROL

> Furthermore, topology information is also needed for entities not
> associated with a mixer control, such as microphones, speakers, jacks/
> connectors, and effect units.  These entities are defined in the USB and
> HD audio specifications, but are not yet handled by ALSA.

All this and more in the embedded case - digital audio link nodes and DSP
I/O nodes (could possibly do those as digital audio ones) spring to
mind.  Also bear in mind that embedded devices can get *very* large - a
mobile phone audio system can have of the order of 100 nodes in the
graph.

> ALSA devices are not addressed by their device node but with card/device/
> subdevice numbers; mixer controls have numeric IDs, unique per card:
> 
>   struct {
>   int card;
>   int device;
>   int subdevice;
>   } alsa_device;
>   struct {
>   int card;
>   int numid;
>   } alsa_control;

For the embedded stuff we also have a bunch of stuff in the graph which
may not be visible to userspace at all at present and would just have a
string based identifier.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 03/12] media: Entities, pads and links

[Mark Brown]

On Thu, Nov 25, 2010 at 03:28:10AM +0100, Laurent Pinchart wrote:

> +Links have flags that describe the link capabilities and state.

> + MEDIA_LINK_FLAG_ACTIVE indicates that the link is active and can be
> + used to transfer media data. When two or more links target a sink pad,
> + only one of them can be active at a time.

Is this supposed to reflect the current state (if the link is carrying
data right now) or if it's possible for the link to carry data?

> +struct media_entity {
> + struct list_head list;
> + struct media_device *parent;/* Media device this entity belongs to*/
> + u32 id; /* Entity ID, unique in the parent media
> +  * device context */
> + const char *name;   /* Entity name */
> + u32 type;   /* Entity type (MEDIA_ENTITY_TYPE_*) */
> + u32 revision;   /* Entity revision, driver specific */
> + unsigned long flags;/* Entity flags (MEDIA_ENTITY_FLAG_*) */
> + u32 group_id;   /* Entity group ID */
> +
> + u16 num_pads;   /* Number of input and output pads */
> + u16 num_links;  /* Number of existing links, both active
> +  * and inactive */
> + u16 num_backlinks;  /* Number of backlinks */
> + u16 max_links;  /* Maximum number of links */
> +
> + struct media_pad *pads; /* Pads array (num_pads elements) */
> + struct media_link *links;   /* Links array (max_links elements)*/

Hrm.  This is getting kind of large, especially considering the volume
of data we're holding per node and link already in ASoC.  On the other
hand we may over time be able to refactor some of the existing stuff
(especially the link management) to use this structure.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [media] msp3400: fix mute audio regression

[Hans Verkuil]

This patch applies 2.6.37 commit 0310871d8f71da4ad8643687fbc40f219a0dac4d to the
2.6.36 stable series.

It was just too late to be included in 2.6.36 at the time, but it should be 
added
to 2.6.36 since it fixes broken audio on this fairly popular audio receiver 
chip.

Regards,

Hans

>From 0310871d8f71da4ad8643687fbc40f219a0dac4d Mon Sep 17 00:00:00 2001
Message-Id: 
<0310871d8f71da4ad8643687fbc40f219a0dac4d.1290686265.git.hverk...@xs4all.nl>
From: Hans Verkuil 
Date: Sun, 17 Oct 2010 07:24:20 -0300
Subject: [PATCH] [media] msp3400: fix mute audio regression
To: linux-media@vger.kernel.org

The switch to the new control framework caused a regression where the audio was
no longer unmuted after the carrier scan finished.

The original code attempted to set the volume control to its current value in
order to have the set-volume control code to be called that handles the volume
and muting. However, the framework will not call that code unless the new volume
value is different from the old.

Instead we now call msp_s_ctrl directly.

It is a bit of a hack: we really need a v4l2_ctrl_refresh_ctrl function for this
(or something along those lines).

Thanks to Andy Walls for bisecting this and to Shane Shrybman for reporting it!

Reported-by: Shane Shrybman 
Thanks-to: Andy Walls 
Signed-off-by: Hans Verkuil 
Signed-off-by: Mauro Carvalho Chehab 

diff --git a/drivers/media/video/msp3400-driver.c 
b/drivers/media/video/msp3400-driver.c
index fe18a0a..b1763ac 100644
--- a/drivers/media/video/msp3400-driver.c
+++ b/drivers/media/video/msp3400-driver.c
@@ -381,7 +381,12 @@ static int msp_s_ctrl(struct v4l2_ctrl *ctrl)
 
 void msp_update_volume(struct msp_state *state)
 {
-   v4l2_ctrl_s_ctrl(state->volume, v4l2_ctrl_g_ctrl(state->volume));
+   /* Force an update of the volume/mute cluster */
+   v4l2_ctrl_lock(state->volume);
+   state->volume->val = state->volume->cur.val;
+   state->muted->val = state->muted->cur.val;
+   msp_s_ctrl(state->volume);
+   v4l2_ctrl_unlock(state->volume);
 }
 
 /* --- v4l2 ioctls --- */
-- 
1.7.0.4


-- 
Hans Verkuil - video4linux developer - sponsored by Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 09/10] MCDE: Add build files and bus

[Marcus LORENTZON]

Hi Arnd,
Comments inline.

> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: den 12 november 2010 17:23
> To: linux-arm-ker...@lists.infradead.org
> Cc: Jimmy RUBIN; linux-fb...@vger.kernel.org; linux-
> me...@vger.kernel.org; Dan JOHANSSON; Linus WALLEIJ
> Subject: Re: [PATCH 09/10] MCDE: Add build files and bus
> 
> On Wednesday 10 November 2010, Jimmy Rubin wrote:
> > This patch adds support for the MCDE, Memory-to-display controller,
> > found in the ST-Ericsson ux500 products.
> >
> > This patch adds the necessary build files for MCDE and the bus that
> > all displays are connected to.
> >
> 
> Can you explain why you need a bus for this?

The idea of the bus is to make it easier to add new panel/display support using 
the normal device/driver model. Boards add devices at init, and drivers are 
selected in config. If we were to model the "real physical" bus structure it 
would be very complex, hard to use. We use our own bus, but it's really a 
virtual bus for adding display devices and drivers to MCDE host. Is there any 
"generic virtual bus type" we should use instead of our own type? If you have 
another idea of how to add display devices and drivers without MCDE code 
modifications, please let us know. A virtual bus just give us a nice framework 
to do this.

> With the code you currently have, there is only a single driver
> associated
> with this bus type, and also just a single device that gets registered
> here!

And yes, currently there's only one single driver. But there's a lot more 
coming in the pipe. Like some LCD, DPI, DBI, DSI display drivers. And once all 
different u8500 boards are pushed, there will be multiple boards registering 
devices with different setups. But one thing at a time.

> >+static int __init mcde_subsystem_init(void)
> >+{
> >+   int ret;
> >+   pr_info("MCDE subsystem init begin\n");
> >+
> >+   /* MCDE module init sequence */
> >+   ret = mcde_init();
> >+   if (ret)
> >+   return ret;
> >+   ret = mcde_display_init();
> >+   if (ret)
> >+   goto mcde_display_failed;
> >+   ret = mcde_dss_init();
> >+   if (ret)
> >+   goto mcde_dss_failed;
> >+   ret = mcde_fb_init();
> >+   if (ret)
> >+   goto mcde_fb_failed;
> >+   pr_info("MCDE subsystem init done\n");
> >+
> >+   return 0;
> >+mcde_fb_failed:
> >+   mcde_dss_exit();
> >+mcde_dss_failed:
> >+   mcde_display_exit();
> >+mcde_display_failed:
> >+   mcde_exit();
> >+   return ret;
> >+}
> 
> Splitting up the module into four sub-modules and then initializing
> everything from one place indicates that something is done wrong
> on a global scale.
> 
> If you indeed need a bus, that should be a separate module that gets
> loaded first and then has the other modules build on top of.

Yes, that's the general idea. We don't completely understand the correct way of 
making sure how one module gets loaded before another, without selecting init 
level, like the fs_initcall below you commented on. I guess most of our 
submodules should be initialized during subsys_init. But we have not found how 
to specify the module init order inside subsys init level. Maybe you can shed 
some light on this? Makefile order seems like a fragile way of defining init 
order dependencies.
Do you think static init calls from machine subsys init are a better solution?

> I'm not sure how the other parts layer on top of one another, can you
> provide some more insight?

++
| mcde_fb/mcde_kms/mcde_v4l2 |
+---++
|mcde_dss   |
+   +---+
|   | disp drvs |
+---+---+
|mcde hw|
+---+
|  HW   |
+---+

> From what I understood so far, you have a single multi-channel display
> controller (mcde_hw.c) that drives the hardware.
> Each controller can have multiple frame buffers attached to it, which
> in turn can have multiple displays attached to each of them, but your
> current configuration only has one of each, right?

Correct, channels A/B (crtcs) can have two blended "framebuffers" plus 
background color, channels C0/C1 can have one framebuffer.

> Right now you have a single top-level bus device for the displays,
> maybe that can be integrated into the controller so the displays are
> properly rooted below the hardware that drives them.

Not sure I understand 100%. Routing is independent of bus structure. Routing 
could change dynamically during runtime reconfiguration using future KMS for 
example. Bus is only for connecting display devices with drivers. Of course we 
could have one bus per port/connector. But then the code would be more complex 
and we would end up with one bus/device/driver per connector (or in some rare 
cases 2-4 on DBI/DSI connectors).

> The frame buffer device also looks weird. Right now you only seem
> to have a single frame buffer registered to a driver in the same
> m

RE: [PATCH 08/10] MCDE: Add frame buffer device

[Jimmy RUBIN]

Hi,

> 
> On Wednesday 10 November 2010, Jimmy Rubin wrote:
> > +
> > +static struct platform_device mcde_fb_device = {
> > +   .name = "mcde_fb",
> > +   .id = -1,
> > +};
> 
> Do not introduce new static devices. We are trying to remove them and
> they will stop working. Why do you even need a device here if there is
> only one of them?

> 
> > +struct fb_info *mcde_fb_create(struct mcde_display_device *ddev,
> > +   u16 w, u16 h, u16 vw, u16 vh, enum mcde_ovly_pix_fmt
> pix_fmt,
> > +   u32 rotate)
> > +{
> 
> Here you have another device, which you could just use!

I will do that.

> 
> > +/* Overlay fbs' platform device */
> > +static int mcde_fb_probe(struct platform_device *pdev)
> > +{
> > +   return 0;
> > +}
> > +
> > +static int mcde_fb_remove(struct platform_device *pdev)
> > +{
> > +   return 0;
> > +}
> > +
> > +static struct platform_driver mcde_fb_driver = {
> > +   .probe  = mcde_fb_probe,
> > +   .remove = mcde_fb_remove,
> > +   .driver = {
> > +   .name  = "mcde_fb",
> > +   .owner = THIS_MODULE,
> > +   },
> > +};
> > +
> > +/* MCDE fb init */
> > +
> > +int __init mcde_fb_init(void)
> > +{
> > +   int ret;
> > +
> > +   ret = platform_driver_register(&mcde_fb_driver);
> > +   if (ret)
> > +   goto fb_driver_failed;
> > +   ret = platform_device_register(&mcde_fb_device);
> > +   if (ret)
> > +   goto fb_device_failed;
> > +
> > +   goto out;
> > +fb_device_failed:
> > +   platform_driver_unregister(&mcde_fb_driver);
> > +fb_driver_failed:
> > +out:
> > +   return ret;
> > +}
> > +
> > +void mcde_fb_exit(void)
> > +{
> > +   platform_device_unregister(&mcde_fb_device);
> > +   platform_driver_unregister(&mcde_fb_driver);
> > +}
> 
> This appears to be an entirely useless registration for something that
> does not exist and that you are not using anywhere ...

Will look into this and remove it.
> 
> > +
> > +#include 
> > +#include 
> > +#if !defined(__KERNEL__) && !defined(_KERNEL)
> > +#include 
> > +#else
> > +#include 
> > +#endif
> > +
> > +#ifdef __KERNEL__
> > +#include "mcde_dss.h"
> > +#endif
> > +
> > +#ifdef __KERNEL__
> > +#define to_mcde_fb(x) ((struct mcde_fb *)(x)->par)
> 
> Everything in this file is enclosed in #ifdef __KERNEL__, and the file
> is not even exported. You can remove the #ifdef and the #else path
> everywhere AFAICT.

Agree, no IOCTLs there for the moment so only kernel include. 

/Jimmy
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/7] v4l: add videobuf2 Video for Linux 2 driver framework

[Marek Szyprowski]

Hello,

On Thursday, November 25, 2010 12:23 PM Hans Verkuil wrote:

> On Thursday, November 25, 2010 10:48:39 Marek Szyprowski wrote:
> > Hello,
> >
> > On Thursday, November 25, 2010 2:17 AM Laurent Pinchart wrote:
> >
> > > On Friday 19 November 2010 16:55:38 Marek Szyprowski wrote:
> > > > From: Pawel Osciak 
> > > >
> > > > Videobuf2 is a Video for Linux 2 API-compatible driver framework for
> > > > multimedia devices. It acts as an intermediate layer between userspace
> > > > applications and device drivers. It also provides low-level, modular
> > > > memory management functions for drivers.
> > > >
> > > > Videobuf2 eases driver development, reduces drivers' code size and aids 
> > > > in
> > > > proper and consistent implementation of V4L2 API in drivers.
> > > >
> > > > Videobuf2 memory management backend is fully modular. This allows custom
> > > > memory management routines for devices and platforms with non-standard
> > > > memory management requirements to be plugged in, without changing the
> > > > high-level buffer management functions and API.
> > > >
> > > > The framework provides:
> > > > - implementations of streaming I/O V4L2 ioctls and file operations
> > > > - high-level video buffer, video queue and state management functions
> > > > - video buffer memory allocation and management
> > >
> > > Excellent job. I'm really pleased by the outstanding code quality.
> > > Nevertheless I got a bunch of comments (I wonder if I'm known as an 
> > > annoying
> > > nit-picking reviewer in the community :-)).
> > >
> > > I've reviewed the patch from bottom to top (starting at the header file), 
> > > so
> > > comments at the bottom can also apply to functions at the top when I got 
> > > tired
> > > repeating the same information. Sorry about that weird order.
> > >
> > > Feel free to disagree with my comments, I've probably been overzealous 
> > > during
> > > the review to make sure everything I had a doubt about would be properly
> > > addressed.
> > >
> > > It's now past 2AM and I don't have enough courage to review the other 
> > > patches.
> > > I'm afraid they will have to wait until tomorrow. If they're of the same
> > > quality as this one I'm sure they will be good.
> >
> > Thanks for your hard work! I can imagine how much time you had to spend on 
> > catching
> > all these things.
> >
> > > BTW, where's the patch for Documentation/video4linux ? :-)
> >
> > Well, I hope it will be ready one day ;) What kind of documentation do you 
> > expect
> > there? Vb2 structures and functions are already documented directly in the 
> > source.
> >
> > > One last comment, why was the decision to remove all locking from vb2 
> > > taken ?
> >
> > The idea came from Hans after posting version 1 and I really like it. It 
> > simplifies
> > a lot of things in the drivers. The original idea of irq_spinlock and 
> > vb_lock was
> > horrible. Having more than one queue in the driver meant that you need to 
> > make a
> > lot of nasty hacks to ensure proper locking, because in some cases you had 
> > to take
> > locks from all queues. The irq_lock usage was even worse. Videobuf used it 
> > to
> > silently mess around the buffers that have been already queued to the 
> > hardware.
> > Now, after Hans changes to v4l2 core (BKL removal in favor of simple mutex) 
> > almost
> > all this mess can be simply removed. Proper locking can be easily ensured 
> > by the
> > new mutex in the v4l2 core. There is no need to have any locks inside vb2. 
> > By
> > removing irq_lock the design of the drivers is easier to understand, 
> > because there
> > is no implicit actions on buffers that are processed by the hardware.
> 
> The BKL removal stuff has nothing to do with this. The main reason is just to 
> simplify
> locking in drivers. Things get really complex if you have multiple nested 
> locks.
> Moving locking out of vb2 and into the driver gives the control back to the 
> driver.
> 
> > > > +/**
> > > > + * vb2_has_consumers() - return true if the userspace is waiting for a
> > > > buffer + * @q:  videobuf2 queue
> > > > + *
> > > > + * This function returns true if a userspace application is waiting 
> > > > for a
> > > > buffer + * to be ready to dequeue (on which a hardware operation has 
> > > > been
> > > > finished). + */
> > > > +bool vb2_has_consumers(struct vb2_queue *q)
> > > > +{
> > > > +   return waitqueue_active(&q->done_wq);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(vb2_has_consumers);
> > >
> > > What is this for ? Do you have use cases in mind ?
> >
> > The vivi driver is using it, but frankly it should be redesigned to use some
> > other check.
> 
> It is a bit of an odd function. I'm also not sure how useful it is.

After more thoughts I've decided to remove it in the next version.

> > > > + * @plane_setup:   called before memory allocation num_planes 
> > > > times;
> > > > + * driver should return the required size of plane 
> > > > number
> > > > + *  

Re: [RFC/PATCH v6 11/12] v4l: Make video_device inherit from media_entity

[Hans Verkuil]

On Thursday, November 25, 2010 03:28:18 Laurent Pinchart wrote:
> V4L2 devices are media entities. As such they need to inherit from
> (include) the media_entity structure.
> 
> When registering/unregistering the device, the media entity is
> automatically registered/unregistered. The entity is acquired on device
> open and released on device close.
> 
> Signed-off-by: Laurent Pinchart 
> Signed-off-by: Sakari Ailus 
> ---
>  Documentation/video4linux/v4l2-framework.txt |   38 +++--
>  drivers/media/video/v4l2-dev.c   |   47 
> +++---
>  include/media/v4l2-dev.h |7 
>  3 files changed, 84 insertions(+), 8 deletions(-)
> 



> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 035db52..511e1ee 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -278,6 +278,9 @@ static int v4l2_mmap(struct file *filp, struct 
> vm_area_struct *vm)
>  static int v4l2_open(struct inode *inode, struct file *filp)
>  {
>   struct video_device *vdev;
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + struct media_entity *entity = NULL;
> +#endif
>   int ret = 0;
>  
>   /* Check if the video device is available */
> @@ -291,6 +294,16 @@ static int v4l2_open(struct inode *inode, struct file 
> *filp)
>   /* and increase the device refcount */
>   video_get(vdev);
>   mutex_unlock(&videodev_lock);
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + if (vdev->v4l2_dev && vdev->v4l2_dev->mdev) {
> + entity = media_entity_get(&vdev->entity);
> + if (!entity) {
> + ret = -EBUSY;
> + video_put(vdev);
> + return ret;
> + }
> + }
> +#endif
>   if (vdev->fops->open) {
>   if (vdev->lock)
>   mutex_lock(vdev->lock);
> @@ -303,8 +316,13 @@ static int v4l2_open(struct inode *inode, struct file 
> *filp)
>   }
>  
>   /* decrease the refcount in case of an error */
> - if (ret)
> + if (ret) {
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + if (vdev->v4l2_dev && vdev->v4l2_dev->mdev)
> + media_entity_put(entity);
> +#endif
>   video_put(vdev);
> + }
>   return ret;
>  }
>  
> @@ -321,7 +339,10 @@ static int v4l2_release(struct inode *inode, struct file 
> *filp)
>   if (vdev->lock)
>   mutex_unlock(vdev->lock);
>   }
> -
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + if (vdev->v4l2_dev && vdev->v4l2_dev->mdev)
> + media_entity_put(&vdev->entity);
> +#endif
>   /* decrease the refcount unconditionally since the release()
>  return value is ignored. */
>   video_put(vdev);
> @@ -558,12 +579,25 @@ int __video_register_device(struct video_device *vdev, 
> int type, int nr,
>   if (nr != -1 && nr != vdev->num && warn_if_nr_in_use)
>   printk(KERN_WARNING "%s: requested %s%d, got %s\n", __func__,
>   name_base, nr, video_device_node_name(vdev));
> -
> - /* Part 5: Activate this minor. The char device can now be used. */
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + /* Part 5: Register the entity. */
> + if (vdev->v4l2_dev && vdev->v4l2_dev->mdev) {
> + vdev->entity.type = MEDIA_ENTITY_TYPE_NODE_V4L;
> + vdev->entity.name = vdev->name;
> + vdev->entity.v4l.major = VIDEO_MAJOR;
> + vdev->entity.v4l.minor = vdev->minor;
> + ret = media_device_register_entity(vdev->v4l2_dev->mdev,
> + &vdev->entity);
> + if (ret < 0)
> + printk(KERN_ERR "error\n"); /* TODO */

Was this forgotten, or will this be fixed in the next version? It looks
out-of-place...

> + }
> +#endif

Regards,

Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 02/10] MCDE: Add configuration registers

[Jimmy RUBIN]

Hi,

> >
> > This patch adds the configuration registers found in MCDE.
> 
> > +
> > +#define MCDE_VAL2REG(__reg, __fld, __val) \
> > +   (((__val) << __reg##_##__fld##_SHIFT) &
> __reg##_##__fld##_MASK)
> > +#define MCDE_REG2VAL(__reg, __fld, __val) \
> > +   (((__val) & __reg##_##__fld##_MASK) >>
> __reg##_##__fld##_SHIFT)
> > +
> > +#define MCDE_CR 0x
> > +#define MCDE_CR_DSICMD2_EN_V1_SHIFT 0
> > +#define MCDE_CR_DSICMD2_EN_V1_MASK 0x0001
> > +#define MCDE_CR_DSICMD2_EN_V1(__x) \
> > +   MCDE_VAL2REG(MCDE_CR, DSICMD2_EN_V1, __x)
> > +#define MCDE_CR_DSICMD1_EN_V1_SHIFT 1
> > +#define MCDE_CR_DSICMD1_EN_V1_MASK 0x0002
> > +#define MCDE_CR_DSICMD1_EN_V1(__x) \
> > +   MCDE_VAL2REG(MCDE_CR, DSICMD1_EN_V1, __x)
> > +#define MCDE_CR_DSI0_EN_V3_SHIFT 0
> > +#define MCDE_CR_DSI0_EN_V3_MASK 0x0001
> > +#define MCDE_CR_DSI0_EN_V3(__x) \
> > +   MCDE_VAL2REG(MCDE_CR, DSI0_EN_V3, __x)
> 
> This looks all rather unreadable. The easiest way is usually to just
> define the bit mask, i.e. the second line of each register definition,
> which you can use to mask the bits. It's also useful to indent the
> lines
> so you can easily tell the register offsets apart from the contents:
> 
> #define MCDE_CR 0x
> #define   MCDE_CR_DSICMD2_EN_V1 0x0001
> #define   MCDE_CR_DSICMD1_EN_V1 0x0002
> 
> Some people prefer to express all this in C instead of macros:
> 
> struct mcde_registers {
>   enum {
>   mcde_cr_dsicmd2_en = 0x0001,
>   mcde_cr_dsicmd1_en = 0x0002,
>   ...
>   } cr;
>   enum {
>   mcde_conf0_syncmux0 = 0x0001,
>   ...
>   } conf0;
>   ...
> };
> 
> This gives you better type safety, but which one you choose is your
> decision.
> 
>   Arnd

All these header files,
Configuration, pixel processing, formatter, dsi link registers are auto 
generated from an xml file.
This is made because there are many registers which a prone to change for new 
hardware revisions and there is a possibility to exclude registers that are not 
used.
The chance of manual errors are less this way.

I also believe that these helper macros makes the source code easier to read.
For example.
#define MCDE_CR_DSICMD2_EN_V1_SHIFT 0
#define MCDE_CR_DSICMD2_EN_V1_MASK 0x0001
#define MCDE_CR_DSICMD2_EN_V1(__x) \
MCDE_VAL2REG(MCDE_CR, DSICMD2_EN_V1, __x)

Writing a single field in the register MCDE_CR can e.g. be done like this:
mcde_wfld(MCDE_CR, DSICMD1_EN_V1, true);

and for a whole register (a totally other register but just to show):
mcde_wreg(MCDE_ROTACONF + chnl_id * MCDE_ROTACONF_GROUPOFFSET,
MCDE_ROTACONF_ROTBURSTSIZE_ENUM(8W) |
MCDE_ROTACONF_ROTBURSTSIZE_HW(1) |
MCDE_ROTACONF_ROTDIR(regs->rotdir) |
MCDE_ROTACONF_STRIP_WIDTH_ENUM(16PIX) |
MCDE_ROTACONF_RD_MAXOUT_ENUM(4_REQ) |
MCDE_ROTACONF_WR_MAXOUT_ENUM(8_REQ));


I agree that the header files looks a bit unreadable I will try indent as you 
suggested I am just worried about the file size. (Patch limit 100kbyte)

/Jimmy
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] v4l: add videobuf2 Video for Linux 2 driver framework

[Hans Verkuil]

On Thursday, November 25, 2010 10:48:39 Marek Szyprowski wrote:
> Hello,
> 
> On Thursday, November 25, 2010 2:17 AM Laurent Pinchart wrote:
> 
> > On Friday 19 November 2010 16:55:38 Marek Szyprowski wrote:
> > > From: Pawel Osciak 
> > >
> > > Videobuf2 is a Video for Linux 2 API-compatible driver framework for
> > > multimedia devices. It acts as an intermediate layer between userspace
> > > applications and device drivers. It also provides low-level, modular
> > > memory management functions for drivers.
> > >
> > > Videobuf2 eases driver development, reduces drivers' code size and aids in
> > > proper and consistent implementation of V4L2 API in drivers.
> > >
> > > Videobuf2 memory management backend is fully modular. This allows custom
> > > memory management routines for devices and platforms with non-standard
> > > memory management requirements to be plugged in, without changing the
> > > high-level buffer management functions and API.
> > >
> > > The framework provides:
> > > - implementations of streaming I/O V4L2 ioctls and file operations
> > > - high-level video buffer, video queue and state management functions
> > > - video buffer memory allocation and management
> > 
> > Excellent job. I'm really pleased by the outstanding code quality.
> > Nevertheless I got a bunch of comments (I wonder if I'm known as an annoying
> > nit-picking reviewer in the community :-)).
> > 
> > I've reviewed the patch from bottom to top (starting at the header file), so
> > comments at the bottom can also apply to functions at the top when I got 
> > tired
> > repeating the same information. Sorry about that weird order.
> > 
> > Feel free to disagree with my comments, I've probably been overzealous 
> > during
> > the review to make sure everything I had a doubt about would be properly
> > addressed.
> > 
> > It's now past 2AM and I don't have enough courage to review the other 
> > patches.
> > I'm afraid they will have to wait until tomorrow. If they're of the same
> > quality as this one I'm sure they will be good.
> 
> Thanks for your hard work! I can imagine how much time you had to spend on 
> catching
> all these things.
> 
> > BTW, where's the patch for Documentation/video4linux ? :-)
> 
> Well, I hope it will be ready one day ;) What kind of documentation do you 
> expect
> there? Vb2 structures and functions are already documented directly in the 
> source.
> 
> > One last comment, why was the decision to remove all locking from vb2 taken 
> > ?
> 
> The idea came from Hans after posting version 1 and I really like it. It 
> simplifies
> a lot of things in the drivers. The original idea of irq_spinlock and vb_lock 
> was
> horrible. Having more than one queue in the driver meant that you need to 
> make a
> lot of nasty hacks to ensure proper locking, because in some cases you had to 
> take
> locks from all queues. The irq_lock usage was even worse. Videobuf used it to
> silently mess around the buffers that have been already queued to the 
> hardware.
> Now, after Hans changes to v4l2 core (BKL removal in favor of simple mutex) 
> almost
> all this mess can be simply removed. Proper locking can be easily ensured by 
> the
> new mutex in the v4l2 core. There is no need to have any locks inside vb2. By
> removing irq_lock the design of the drivers is easier to understand, because 
> there
> is no implicit actions on buffers that are processed by the hardware.

The BKL removal stuff has nothing to do with this. The main reason is just to 
simplify
locking in drivers. Things get really complex if you have multiple nested locks.
Moving locking out of vb2 and into the driver gives the control back to the 
driver.

> > > +/**
> > > + * vb2_has_consumers() - return true if the userspace is waiting for a
> > > buffer + * @q:videobuf2 queue
> > > + *
> > > + * This function returns true if a userspace application is waiting for a
> > > buffer + * to be ready to dequeue (on which a hardware operation has been
> > > finished). + */
> > > +bool vb2_has_consumers(struct vb2_queue *q)
> > > +{
> > > + return waitqueue_active(&q->done_wq);
> > > +}
> > > +EXPORT_SYMBOL_GPL(vb2_has_consumers);
> > 
> > What is this for ? Do you have use cases in mind ?
> 
> The vivi driver is using it, but frankly it should be redesigned to use some
> other check.

It is a bit of an odd function. I'm also not sure how useful it is.

> > > + * @plane_setup: called before memory allocation num_planes times;
> > > + *   driver should return the required size of plane 
> > > number
> > > + *   plane_no
> > > + * @unlock:  release any locks taken while calling vb2 
> > > functions;
> > > + *   it is called before poll_wait function in 
> > > vb2_poll
> > > + *   implementation; required to avoid deadlock when 
> > > vb2_poll
> > > + *   function waits for a buffer
> > > + * @lock:reacquire all locks

Re: [RFC/PATCH v3 6/7] omap3: Export omap3isp platform device structure

[Felipe Balbi]

Hi,

On Thu, Nov 25, 2010 at 12:17:59PM +0100, Laurent Pinchart wrote:

pass platform_data as an argument to this call ? Then remove the static
inline and export this one ?


Yes indeed, why ? :-)

I guess things like that are difficult to spot when you've had your nose on
the code for too long. Thanks for the review, I'll fix this.


no problem :-)

--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 10/10] ux500: MCDE: Add platform specific data

[Jimmy RUBIN]

Hi,


> > +
> > +   if (ddev->id == PRIMARY_DISPLAY_ID && rotate_main) {
> > +   swap(width, height);
> > +#ifdef CONFIG_DISPLAY_GENERIC_DSI_PRIMARY_ROTATE_180_DEGREES
> > +   rotate = FB_ROTATE_CCW;
> > +#else
> > +   rotate = FB_ROTATE_CW;
> > +#endif
> > +   }
> > +
> > +   virtual_width = width;
> > +   virtual_height = height * 2;
> > +#ifdef CONFIG_DISPLAY_GENERIC_DSI_PRIMARY_AUTO_SYNC
> > +   if (ddev->id == PRIMARY_DISPLAY_ID)
> > +   virtual_height = height;
> > +#endif
> > +
> 
> The contents of the hardware description should really not
> be configuration dependent, because that breaks booting the same
> kernel on machines that have different requirements.
> 
> This is something that should be passed down from the boot loader.
The defines above is more configuration of the display driver than hardware 
dependant defines.

The define CONFIG_DISPLAY_GENERIC_DSI_PRIMARY_AUTO_SYNC is not hardware 
dependant.
It is used to tell mcde that it should not rely on pan_display for refreshing 
the display, it should rather refresh the display itself, currently using 
software triggers and it will also tell mcde that only one frame buffer should 
be used.
This mode can be used if the application system is for example X.

CONFIG_DISPLAY_GENERIC_DSI_PRIMARY_ROTATE_180_DEGREES is used for some boards 
that have the display rotated 90 instead of 270 degrees, so in a sense it is 
hardware dependant, but it will not break the kernel. This defines is going to 
be removed and replaced by one define that handles all rotations, 0, 90, 180, 
270 degrees.

> 
> > +static void mcde_epod_enable(void)
> > +{
> > +   /* Power on DSS mem */
> > +   writel(PRCMU_ENABLE_DSS_MEM, PRCM_EPOD_C_SET);
> > +   mdelay(PRCMU_MCDE_DELAY);
> > +   /* Power on DSS logic */
> > +   writel(PRCMU_ENABLE_DSS_LOGIC, PRCM_EPOD_C_SET);
> > +   mdelay(PRCMU_MCDE_DELAY);
> > +}
> 
> In general, try to avoid using mdelay. Keeping the CPU busy for
> miliseconds
> or even microseconds for no reason is just wrong.
> 
> Reasonable hardware will not require this and do the right thing
> anyway.
> multiple writel calls are by design strictly ordered on the bus. If
> that is
> not the case on your hardware, you should find a proper way to ensure
> ordering and create a small wrapper for it with a comment that explains
> the breakage. Better get the hardware designers to fix their crap
> before
> releasing a product ;-)
> 
> If there is not even a way to reorder I/O by accessing other registers,
> use msleep() to let the CPU do something useful in the meantime and
> complain
> even more to the HW people.
> 
>   Arnd
These registers are normally not accessed by the cpu and therefore it is 
required to use some delays between them, however I agree that msleep is better 
to use.
We are investigating how this can be removed but for now we have to keep it.

/Jimmy
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v3 6/7] omap3: Export omap3isp platform device structure

[Laurent Pinchart]

Hi Felipe,

On Thursday 25 November 2010 08:02:41 Felipe Balbi wrote:
> On Thu, Nov 25, 2010 at 03:54:37AM +0100, Laurent Pinchart wrote:
> >From: Stanimir Varbanov 
> >
> >The omap3isp platform device requires platform data. As the data can be
> >provided by a kernel module, the device can't be registered during arch
> >initialization.
> >
> >Remove the omap3isp platform device registration from
> >omap_init_camera(), and export the platform device structure to let
> >board code register/unregister it.
> 
> instead, why don't you...
> 
> >diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
> >index d5da345..c2275d3 100644
> >--- a/arch/arm/mach-omap2/devices.c
> >+++ b/arch/arm/mach-omap2/devices.c
> >@@ -34,6 +34,8 @@
> >
> > #include "mux.h"
> > #include "control.h"
> >
> >+#include "devices.h"
> >+
> >
> > #if defined(CONFIG_VIDEO_OMAP2) || defined(CONFIG_VIDEO_OMAP2_MODULE)
> > 
> > static struct resource cam_resources[] = {
> >
> >@@ -144,16 +146,28 @@ static struct resource omap3isp_resources[] = {
> >
> > }
> > 
> > };
> >
> >-static struct platform_device omap3isp_device = {
> >+static void omap3isp_release(struct device *dev)
> >+{
> >+/* Zero the device structure to avoid re-initialization complaints from
> >+ * kobject when the device will be re-registered.
> >+ */
> >+memset(dev, 0, sizeof(*dev));
> >+dev->release = omap3isp_release;
> >+}
> >+
> >+struct platform_device omap3isp_device = {
> >
> > .name   = "omap3isp",
> > .id = -1,
> > .num_resources  = ARRAY_SIZE(omap3isp_resources),
> > .resource   = omap3isp_resources,
> >
> >+.dev = {
> >+.release= omap3isp_release,
> >+},
> >
> > };
> >
> >+EXPORT_SYMBOL_GPL(omap3isp_device);
> >
> > static inline void omap_init_camera(void)
> 
> pass platform_data as an argument to this call ? Then remove the static
> inline and export this one ?

Yes indeed, why ? :-)

I guess things like that are difficult to spot when you've had your nose on 
the code for too long. Thanks for the review, I'll fix this.

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/7] v4l: videobuf2: add vmalloc allocator

[Hans Verkuil]

On Thursday, November 25, 2010 11:26:56 Marek Szyprowski wrote:
> Hello,
> 
> On Thursday, November 25, 2010 11:07 AM Hans Verkuil wrote:
> 
> > On Friday, November 19, 2010 16:55:40 Marek Szyprowski wrote:
> > > From: Pawel Osciak 
> > >
> > > Add an implementation of contiguous virtual memory allocator and handling
> > > routines for videobuf2, implemented on top of vmalloc()/vfree() calls.
> > >
> > > Signed-off-by: Pawel Osciak 
> > > Signed-off-by: Kyungmin Park 
> > > Signed-off-by: Marek Szyprowski 
> > > CC: Pawel Osciak 
> > > ---
> > >  drivers/media/video/Kconfig |5 +
> > >  drivers/media/video/Makefile|1 +
> > >  drivers/media/video/videobuf2-vmalloc.c |  177 
> > > +++
> > >  include/media/videobuf2-vmalloc.h   |   21 
> > >  4 files changed, 204 insertions(+), 0 deletions(-)
> > >  create mode 100644 drivers/media/video/videobuf2-vmalloc.c
> > >  create mode 100644 include/media/videobuf2-vmalloc.h
> > >
> > > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> > > index 83ce858..9351423 100644
> > > --- a/drivers/media/video/Kconfig
> > > +++ b/drivers/media/video/Kconfig
> > > @@ -55,6 +55,11 @@ config VIDEOBUF2_CORE
> > >  config VIDEOBUF2_MEMOPS
> > >   tristate
> > >
> > > +config VIDEOBUF2_VMALLOC
> > > + select VIDEOBUF2_CORE
> > > + select VIDEOBUF2_MEMOPS
> > > + tristate
> > > +
> > >  #
> > >  # Multimedia Video device configuration
> > >  #
> > > diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> > > index a97a2a0..538bee9 100644
> > > --- a/drivers/media/video/Makefile
> > > +++ b/drivers/media/video/Makefile
> > > @@ -116,6 +116,7 @@ obj-$(CONFIG_VIDEO_BTCX)  += btcx-risc.o
> > >
> > >  obj-$(CONFIG_VIDEOBUF2_CORE) += videobuf2-core.o
> > >  obj-$(CONFIG_VIDEOBUF2_MEMOPS)   += videobuf2-memops.o
> > > +obj-$(CONFIG_VIDEOBUF2_VMALLOC)  += videobuf2-vmalloc.o
> > >
> > >  obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
> > >
> > > diff --git a/drivers/media/video/videobuf2-vmalloc.c 
> > > b/drivers/media/video/videobuf2-vmalloc.c
> > > new file mode 100644
> > > index 000..3310900
> > > --- /dev/null
> > > +++ b/drivers/media/video/videobuf2-vmalloc.c
> > > @@ -0,0 +1,177 @@
> > > +/*
> > > + * videobuf2-vmalloc.c - vmalloc memory allocator for videobuf2
> > > + *
> > > + * Copyright (C) 2010 Samsung Electronics
> > > + *
> > > + * Author: Pawel Osciak 
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation.
> > > + */
> > > +
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +
> > > +#include 
> > > +#include 
> > > +
> > > +struct vb2_vmalloc_conf {
> > > + struct vb2_alloc_ctxalloc_ctx;
> > > +};
> > > +
> > > +struct vb2_vmalloc_buf {
> > > + void*vaddr;
> > > + unsigned long   size;
> > > + unsigned intrefcount;
> > > +};
> > > +
> > > +static void *vb2_vmalloc_alloc(const struct vb2_alloc_ctx *alloc_ctx,
> > > + unsigned long size)
> > > +{
> > > + struct vb2_vmalloc_buf *buf;
> > > +
> > > + buf = kzalloc(sizeof *buf, GFP_KERNEL);
> > > + if (!buf)
> > > + return NULL;
> > > +
> > > + buf->size = size;
> > > + buf->vaddr = vmalloc_user(buf->size);
> > > + if (!buf->vaddr) {
> > > + printk(KERN_ERR "vmalloc of size %ld failed\n", buf->size);
> > > + kfree(buf);
> > > + return NULL;
> > > + }
> > > +
> > > + buf->refcount++;
> > > + printk(KERN_DEBUG "Allocated vmalloc buffer of size %ld at vaddr=%p\n",
> > > + buf->size, buf->vaddr);
> > > +
> > > + return buf;
> > > +}
> > > +
> > > +static void vb2_vmalloc_put(void *buf_priv)
> > > +{
> > > + struct vb2_vmalloc_buf *buf = buf_priv;
> > > +
> > > + buf->refcount--;
> > > +
> > > + if (0 == buf->refcount) {
> > 
> > I think I would feel happier if refcount was of type atomic_t and you would 
> > use
> > the atomic decrease-and-test operation here. I know that this is almost 
> > certainly
> > called with some lock, but still...
> 
> Yes, it is called with mm_sem taken in all cases. Maybe a comment in the 
> source will
> be enough to indicate that this variable does not need to be atomic_t type?
> I can change it to atomic_t if this is really required anyway.

It definitely needs to be documented. But I wonder if what you say is true 
anyway:

vb2_mmap is called without mm_sem taken as far as I can tell, this calls the 
mmap
callback, vb2_vmalloc_mmap calls vb2_vmalloc_vm_open directly which does 
buf_refcount++.

Making refcount atomic would make it easier to prove correctness IMHO.

Anyway, this leads me to a second question: the documentation (either separate 
or in the
header) should document which ops need to be called with some lock set. That 
way driver
writers have some guidelines regar

Re: [PATCH 6/7] v4l: vivi: port to videobuf2

[Hans Verkuil]

On Friday, November 19, 2010 16:55:43 Marek Szyprowski wrote:
> Make vivi use videobuf2 in place of videobuf.
> 
> Signed-off-by: Pawel Osciak 
> Signed-off-by: Marek Szyprowski 
> Signed-off-by: Kyungmin Park 
> CC: Pawel Osciak 



> -static void vivi_stop_generating(struct file *file)
> +static void vivi_stop_generating(struct vivi_dev *dev)
>  {
> - struct vivi_dev *dev = video_drvdata(file);
>   struct vivi_dmaqueue *dma_q = &dev->vidq;
>  
>   dprintk(dev, 1, "%s\n", __func__);
>  
> - if (!file->private_data)
> - return;
> - if (!test_and_clear_bit(0, &dev->generating))
> - return;
> -
>   /* shutdown control thread */
>   if (dma_q->kthread) {
>   kthread_stop(dma_q->kthread);
>   dma_q->kthread = NULL;
>   }
> - videobuf_stop(&dev->vb_vidq);
> - videobuf_mmap_free(&dev->vb_vidq);
> -}
>  
> -static int vivi_is_generating(struct vivi_dev *dev)
> -{
> - return test_bit(0, &dev->generating);
> + /*
> +  * Typical driver might need to wait here until dma engine stops.
> +  * In this case we can abort imiedetly, so it's just a noop.

typo: imiedetly -> immediately




Regards,

Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/7] v4l: videobuf2: add vmalloc allocator

[Marek Szyprowski]

Hello,

On Thursday, November 25, 2010 11:07 AM Hans Verkuil wrote:

> On Friday, November 19, 2010 16:55:40 Marek Szyprowski wrote:
> > From: Pawel Osciak 
> >
> > Add an implementation of contiguous virtual memory allocator and handling
> > routines for videobuf2, implemented on top of vmalloc()/vfree() calls.
> >
> > Signed-off-by: Pawel Osciak 
> > Signed-off-by: Kyungmin Park 
> > Signed-off-by: Marek Szyprowski 
> > CC: Pawel Osciak 
> > ---
> >  drivers/media/video/Kconfig |5 +
> >  drivers/media/video/Makefile|1 +
> >  drivers/media/video/videobuf2-vmalloc.c |  177 
> > +++
> >  include/media/videobuf2-vmalloc.h   |   21 
> >  4 files changed, 204 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/media/video/videobuf2-vmalloc.c
> >  create mode 100644 include/media/videobuf2-vmalloc.h
> >
> > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> > index 83ce858..9351423 100644
> > --- a/drivers/media/video/Kconfig
> > +++ b/drivers/media/video/Kconfig
> > @@ -55,6 +55,11 @@ config VIDEOBUF2_CORE
> >  config VIDEOBUF2_MEMOPS
> > tristate
> >
> > +config VIDEOBUF2_VMALLOC
> > +   select VIDEOBUF2_CORE
> > +   select VIDEOBUF2_MEMOPS
> > +   tristate
> > +
> >  #
> >  # Multimedia Video device configuration
> >  #
> > diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> > index a97a2a0..538bee9 100644
> > --- a/drivers/media/video/Makefile
> > +++ b/drivers/media/video/Makefile
> > @@ -116,6 +116,7 @@ obj-$(CONFIG_VIDEO_BTCX)  += btcx-risc.o
> >
> >  obj-$(CONFIG_VIDEOBUF2_CORE)   += videobuf2-core.o
> >  obj-$(CONFIG_VIDEOBUF2_MEMOPS) += videobuf2-memops.o
> > +obj-$(CONFIG_VIDEOBUF2_VMALLOC)+= videobuf2-vmalloc.o
> >
> >  obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
> >
> > diff --git a/drivers/media/video/videobuf2-vmalloc.c 
> > b/drivers/media/video/videobuf2-vmalloc.c
> > new file mode 100644
> > index 000..3310900
> > --- /dev/null
> > +++ b/drivers/media/video/videobuf2-vmalloc.c
> > @@ -0,0 +1,177 @@
> > +/*
> > + * videobuf2-vmalloc.c - vmalloc memory allocator for videobuf2
> > + *
> > + * Copyright (C) 2010 Samsung Electronics
> > + *
> > + * Author: Pawel Osciak 
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +#include 
> > +
> > +struct vb2_vmalloc_conf {
> > +   struct vb2_alloc_ctxalloc_ctx;
> > +};
> > +
> > +struct vb2_vmalloc_buf {
> > +   void*vaddr;
> > +   unsigned long   size;
> > +   unsigned intrefcount;
> > +};
> > +
> > +static void *vb2_vmalloc_alloc(const struct vb2_alloc_ctx *alloc_ctx,
> > +   unsigned long size)
> > +{
> > +   struct vb2_vmalloc_buf *buf;
> > +
> > +   buf = kzalloc(sizeof *buf, GFP_KERNEL);
> > +   if (!buf)
> > +   return NULL;
> > +
> > +   buf->size = size;
> > +   buf->vaddr = vmalloc_user(buf->size);
> > +   if (!buf->vaddr) {
> > +   printk(KERN_ERR "vmalloc of size %ld failed\n", buf->size);
> > +   kfree(buf);
> > +   return NULL;
> > +   }
> > +
> > +   buf->refcount++;
> > +   printk(KERN_DEBUG "Allocated vmalloc buffer of size %ld at vaddr=%p\n",
> > +   buf->size, buf->vaddr);
> > +
> > +   return buf;
> > +}
> > +
> > +static void vb2_vmalloc_put(void *buf_priv)
> > +{
> > +   struct vb2_vmalloc_buf *buf = buf_priv;
> > +
> > +   buf->refcount--;
> > +
> > +   if (0 == buf->refcount) {
> 
> I think I would feel happier if refcount was of type atomic_t and you would 
> use
> the atomic decrease-and-test operation here. I know that this is almost 
> certainly
> called with some lock, but still...

Yes, it is called with mm_sem taken in all cases. Maybe a comment in the source 
will
be enough to indicate that this variable does not need to be atomic_t type?
I can change it to atomic_t if this is really required anyway.



Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] v4l: videobuf2: add read() and write() emulator

[Hans Verkuil]

On Friday, November 19, 2010 16:55:42 Marek Szyprowski wrote:
> Add a generic file io (read and write) emulator for videobuf2. It uses
> MMAP memory type buffers and generic vb2 calls: req_bufs, qbuf and
> dqbuf. Video date is being copied from mmap buffers to userspace with
> standard copy_to_user() function. To add support for file io the driver
> needs to provide an additional callback - read_setup or write_setup. It
> should provide the default number of buffers used by emulator and flags.
> 
> With these flags one can detemine the style of read() or write()
> emulation. By default 'streaming' style is used. With
> VB2_FILEIO_READ_ONCE flag one can select 'one shot' mode for read()
> emulator. With VB2_FILEIO_WRITE_IMMEDIATE flag one can select immediate
> conversion of write calls to qbuf for write() emulator, so the vb2 will
> not wait until each buffer is filled completely before queueing it to
> the driver.
> 
> Signed-off-by: Marek Szyprowski 
> Signed-off-by: Kyungmin Park 
> CC: Pawel Osciak 
> ---
>  drivers/media/video/videobuf2-core.c |  396 
> +-
>  include/media/videobuf2-core.h   |   31 +++
>  2 files changed, 426 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf2-core.c 
> b/drivers/media/video/videobuf2-core.c
> index 828803f..bc497e3 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c



> +/**
> + * __vb2_init_fileio() - initialize file io emulator
> + * @q:   videobuf2 queue
> + * @read:mode selector (1 means read, 0 means write)
> + */
> +static int __vb2_init_fileio(struct vb2_queue *q, int read)
> +{
> + struct vb2_fileio_data *fileio;
> + int i, ret;
> + unsigned int count=0, flags=0;
> +
> + /*
> +  * Sanity check
> +  */
> + if ((read && !q->ops->read_setup) || (!read && !q->ops->write_setup))
> + BUG();
> +
> + /*
> +  * Check if device supports mapping buffers to kernel virtual space.
> +  */
> + if (!q->alloc_ctx[0]->mem_ops->vaddr)
> + return -EBUSY;
> +
> + /*
> +  * Check if steaming api has not been already activated.

typo: steaming -> streaming :-)

> +  */
> + if (q->streaming || q->num_buffers > 0)
> + return -EBUSY;
> +
> + /*
> +  * Basic checks done, lets try to set up file io emulator
> +  */
> + if (read)
> + ret = call_qop(q, read_setup, q, &count, &flags);
> + else
> + ret = call_qop(q, write_setup, q, &count, &flags);
> + if (ret)
> + return ret;
> +
> + dprintk(3, "setting up file io: mode %s, count %d, flags %08x\n",
> + (read) ? "read" : "write", count, flags);
> +
> + fileio = kzalloc(sizeof(struct vb2_fileio_data), GFP_KERNEL);
> + if (fileio == NULL)
> + return -ENOMEM;
> +
> + fileio->flags = flags;
> +
> + /*
> +  * Request buffers and use MMAP type to force driver
> +  * to allocate buffers by itself.
> +  */
> + fileio->req.count = count;
> + fileio->req.memory = V4L2_MEMORY_MMAP;
> + fileio->req.type = q->type;
> + ret = vb2_reqbufs(q, &fileio->req);
> + if (ret)
> + goto err_kfree;
> +
> + /*
> +  * Check if plane_count is correct
> +  * (multiplane buffers are not supported).
> +  */
> + if (q->bufs[0]->num_planes != 1) {
> + fileio->req.count = 0;
> + ret = -EBUSY;

I'm not sure about this error code. I think EINVAL is better although it's not
ideal either. A debug message would certainly help here.

> + goto err_reqbufs;
> + }
> +
> + /*
> +  * Get kernel address of each buffer.
> +  */
> + for (i = 0; i < q->num_buffers; i++) {
> + fileio->bufs[i].vaddr = vb2_plane_vaddr(q->bufs[i], 0);
> + if (fileio->bufs[i].vaddr == NULL)
> + goto err_reqbufs;
> + fileio->bufs[i].size = vb2_plane_size(q->bufs[i], 0);
> + }
> +
> + /*
> +  * Read mode requires pre queuing of all buffers.
> +  */
> + if (read) {
> + /*
> +  * Queue all buffers.
> +  */
> + for (i = 0; i < q->num_buffers; i++) {
> + struct v4l2_buffer *b = &fileio->b;
> + memset(b, 0, sizeof(*b));
> + b->type = q->type;
> + b->memory = q->memory;
> + b->index = i;
> + ret = vb2_qbuf(q, b);
> + if (ret)
> + goto err_reqbufs;
> + fileio->bufs[i].queued = 1;
> + }
> +
> + /*
> +  * Start streaming.
> +  */
> + ret = vb2_streamon(q, q->type);
> + if (ret)
> + goto err_reqbufs;
> + }
> +
> + q->fileio = fileio;
> +
> + return ret;
> +
> +err_reqbufs:
> +  

Re: [PATCH 3/7] v4l: videobuf2: add vmalloc allocator

[Hans Verkuil]

On Friday, November 19, 2010 16:55:40 Marek Szyprowski wrote:
> From: Pawel Osciak 
> 
> Add an implementation of contiguous virtual memory allocator and handling
> routines for videobuf2, implemented on top of vmalloc()/vfree() calls.
> 
> Signed-off-by: Pawel Osciak 
> Signed-off-by: Kyungmin Park 
> Signed-off-by: Marek Szyprowski 
> CC: Pawel Osciak 
> ---
>  drivers/media/video/Kconfig |5 +
>  drivers/media/video/Makefile|1 +
>  drivers/media/video/videobuf2-vmalloc.c |  177 
> +++
>  include/media/videobuf2-vmalloc.h   |   21 
>  4 files changed, 204 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/video/videobuf2-vmalloc.c
>  create mode 100644 include/media/videobuf2-vmalloc.h
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 83ce858..9351423 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -55,6 +55,11 @@ config VIDEOBUF2_CORE
>  config VIDEOBUF2_MEMOPS
>   tristate
>  
> +config VIDEOBUF2_VMALLOC
> + select VIDEOBUF2_CORE
> + select VIDEOBUF2_MEMOPS
> + tristate
> +
>  #
>  # Multimedia Video device configuration
>  #
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index a97a2a0..538bee9 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -116,6 +116,7 @@ obj-$(CONFIG_VIDEO_BTCX)  += btcx-risc.o
>  
>  obj-$(CONFIG_VIDEOBUF2_CORE) += videobuf2-core.o
>  obj-$(CONFIG_VIDEOBUF2_MEMOPS)   += videobuf2-memops.o
> +obj-$(CONFIG_VIDEOBUF2_VMALLOC)  += videobuf2-vmalloc.o
>  
>  obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
>  
> diff --git a/drivers/media/video/videobuf2-vmalloc.c 
> b/drivers/media/video/videobuf2-vmalloc.c
> new file mode 100644
> index 000..3310900
> --- /dev/null
> +++ b/drivers/media/video/videobuf2-vmalloc.c
> @@ -0,0 +1,177 @@
> +/*
> + * videobuf2-vmalloc.c - vmalloc memory allocator for videobuf2
> + *
> + * Copyright (C) 2010 Samsung Electronics
> + *
> + * Author: Pawel Osciak 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +struct vb2_vmalloc_conf {
> + struct vb2_alloc_ctxalloc_ctx;
> +};
> +
> +struct vb2_vmalloc_buf {
> + void*vaddr;
> + unsigned long   size;
> + unsigned intrefcount;
> +};
> +
> +static void *vb2_vmalloc_alloc(const struct vb2_alloc_ctx *alloc_ctx,
> + unsigned long size)
> +{
> + struct vb2_vmalloc_buf *buf;
> +
> + buf = kzalloc(sizeof *buf, GFP_KERNEL);
> + if (!buf)
> + return NULL;
> +
> + buf->size = size;
> + buf->vaddr = vmalloc_user(buf->size);
> + if (!buf->vaddr) {
> + printk(KERN_ERR "vmalloc of size %ld failed\n", buf->size);
> + kfree(buf);
> + return NULL;
> + }
> +
> + buf->refcount++;
> + printk(KERN_DEBUG "Allocated vmalloc buffer of size %ld at vaddr=%p\n",
> + buf->size, buf->vaddr);
> +
> + return buf;
> +}
> +
> +static void vb2_vmalloc_put(void *buf_priv)
> +{
> + struct vb2_vmalloc_buf *buf = buf_priv;
> +
> + buf->refcount--;
> +
> + if (0 == buf->refcount) {

I think I would feel happier if refcount was of type atomic_t and you would use
the atomic decrease-and-test operation here. I know that this is almost 
certainly
called with some lock, but still...

> + printk(KERN_DEBUG "%s: Freeing vmalloc mem at vaddr=%p\n",
> + __func__, buf->vaddr);
> + vfree(buf->vaddr);
> + kfree(buf);
> + }
> +}
> +
> +static void *vb2_vmalloc_vaddr(void *buf_priv)
> +{
> + struct vb2_vmalloc_buf *buf = buf_priv;
> +
> + BUG_ON(!buf);
> +
> + if (!buf->vaddr) {
> + printk(KERN_ERR "Address of an unallocated "
> + "plane requested\n");
> + return NULL;
> + }
> +
> + return buf->vaddr;
> +}
> +
> +static unsigned int vb2_vmalloc_num_users(void *buf_priv)
> +{
> + struct vb2_vmalloc_buf *buf = buf_priv;
> +
> + return buf->refcount;
> +}
> +
> +/* TODO generalize and extract to core as much as possible */
> +static void vb2_vmalloc_vm_open(struct vm_area_struct *vma)
> +{
> + struct vb2_vmalloc_buf *buf = vma->vm_private_data;
> +
> + printk(KERN_DEBUG "%s vmalloc_priv: %p, refcount: %d, "
> + "vma: %08lx-%08lx\n", __func__, buf, buf->refcount,
> + vma->vm_start, vma->vm_end);
> +
> + buf->refcount++;
> +}
> +
> +static void vb2_vmalloc_vm_close(struct vm_area_struct *vma)
> +{
> + struct vb2_vmalloc_buf *buf = vma->vm_private_data;
> +
> + pri

RE: [PATCH 1/7] v4l: add videobuf2 Video for Linux 2 driver framework

[Marek Szyprowski]

Hello,

On Thursday, November 25, 2010 2:17 AM Laurent Pinchart wrote:

> On Friday 19 November 2010 16:55:38 Marek Szyprowski wrote:
> > From: Pawel Osciak 
> >
> > Videobuf2 is a Video for Linux 2 API-compatible driver framework for
> > multimedia devices. It acts as an intermediate layer between userspace
> > applications and device drivers. It also provides low-level, modular
> > memory management functions for drivers.
> >
> > Videobuf2 eases driver development, reduces drivers' code size and aids in
> > proper and consistent implementation of V4L2 API in drivers.
> >
> > Videobuf2 memory management backend is fully modular. This allows custom
> > memory management routines for devices and platforms with non-standard
> > memory management requirements to be plugged in, without changing the
> > high-level buffer management functions and API.
> >
> > The framework provides:
> > - implementations of streaming I/O V4L2 ioctls and file operations
> > - high-level video buffer, video queue and state management functions
> > - video buffer memory allocation and management
> 
> Excellent job. I'm really pleased by the outstanding code quality.
> Nevertheless I got a bunch of comments (I wonder if I'm known as an annoying
> nit-picking reviewer in the community :-)).
> 
> I've reviewed the patch from bottom to top (starting at the header file), so
> comments at the bottom can also apply to functions at the top when I got tired
> repeating the same information. Sorry about that weird order.
> 
> Feel free to disagree with my comments, I've probably been overzealous during
> the review to make sure everything I had a doubt about would be properly
> addressed.
> 
> It's now past 2AM and I don't have enough courage to review the other patches.
> I'm afraid they will have to wait until tomorrow. If they're of the same
> quality as this one I'm sure they will be good.

Thanks for your hard work! I can imagine how much time you had to spend on 
catching
all these things.

> BTW, where's the patch for Documentation/video4linux ? :-)

Well, I hope it will be ready one day ;) What kind of documentation do you 
expect
there? Vb2 structures and functions are already documented directly in the 
source.

> One last comment, why was the decision to remove all locking from vb2 taken ?

The idea came from Hans after posting version 1 and I really like it. It 
simplifies
a lot of things in the drivers. The original idea of irq_spinlock and vb_lock 
was
horrible. Having more than one queue in the driver meant that you need to make a
lot of nasty hacks to ensure proper locking, because in some cases you had to 
take
locks from all queues. The irq_lock usage was even worse. Videobuf used it to
silently mess around the buffers that have been already queued to the hardware.
Now, after Hans changes to v4l2 core (BKL removal in favor of simple mutex) 
almost
all this mess can be simply removed. Proper locking can be easily ensured by the
new mutex in the v4l2 core. There is no need to have any locks inside vb2. By
removing irq_lock the design of the drivers is easier to understand, because 
there
is no implicit actions on buffers that are processed by the hardware.



> > +/**
> > + * __vb2_buf_userptr_put() - release userspace memory associated
> > associated + * with a USERPTR buffer
> > + */
> > +static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
> > +{
> > +   struct vb2_queue *q = vb->vb2_queue;
> > +   unsigned int plane;
> > +
> > +   for (plane = 0; plane < vb->num_planes; ++plane) {
> > +   call_memop(q, plane, put_userptr, vb->planes[plane].mem_priv);
> > +   vb->planes[plane].mem_priv = NULL;
> 
> Any reason to initialize mem_priv to NULL here but not in __vb2_buf_mem_free ?

Yes, __vb2_buf_userptr_put is called when user calls QBUF with different address
than in the previous call, so the buffer is reinitialized without a call to
__vb2_buf_mem_free.



> > +/**
> > + * __vb2_queue_alloc() - allocate videobuf buffer structures and (for MMAP
> > type) + * video buffer memory for all buffers/planes on the queue and
> > initializes the + * queue
> > + *
> > + * Returns the number of buffers successfully allocated.
> > + */
> > +static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
> > +unsigned int num_buffers, unsigned int num_planes)
> > +{
> > +   unsigned long plane_sizes[VIDEO_MAX_PLANES];
> > +   unsigned int buffer, plane;
> > +   struct vb2_buffer *vb;
> > +   int ret;
> > +
> > +   /* Get requested plane sizes from the driver */
> > +   for (plane = 0; plane < num_planes; ++plane) {
> > +   ret = call_qop(q, plane_setup, q, plane, &plane_sizes[plane]);
> > +   if (ret) {
> > +   dprintk(1, "Plane setup failed\n");
> > +   return ret;
> > +   }
> > +   }
> > +
> > +   for (buffer = 0; buffer < num_buffers; ++buffer) {
> > +   /* Allocate videobuf buffer structures */
> > +   vb = __v

Re: [RFC/PATCH v6 03/12] [alsa-devel] media: Entities, pads and links

[Clemens Ladisch]

Laurent Pinchart wrote:
> A link is a point-to-point oriented connection between two pads, either
> on the same entity or on different entities. Data flows from a source
> pad to a sink pad.
> 
> Links are stored in the source entity.

In the descriptors of USB Audio and HDAudio devices, the links are
stored only in the sink.  But AFAICS this doesn't matter in the media
driver API.

> +Links have flags that describe the link capabilities and state.
> +
> + MEDIA_LINK_FLAG_ACTIVE indicates that the link is active and can be
> + used to transfer media data. When two or more links target a sink pad,
> + only one of them can be active at a time.
> + MEDIA_LINK_FLAG_IMMUTABLE indicates that the link active state can't
> + be modified at runtime. If MEDIA_LINK_FLAG_IMMUTABLE is set, then
> + MEDIA_LINK_FLAG_ACTIVE must also be set since an immutable link is
> + always active.

So the intended userspace API for controlling routing is to change the
ACTIVE flag of links?

In USB and HD audio devices, all links are immutable, and the routing
is controlled by 'selector' entities that activate exactly one of their
input pads.  In userspace, this entity shows up as a mixer control.
I guess it would be possible to map the ACTIVE flag onto these controls.

Alternatively, entities can have 'mute' mixer controls associated with
their pads.  In this case, multiple unmuted inputs would be mixed
together.

> +#define MEDIA_ENTITY_TYPE_MASK   0x00ff
> +#define MEDIA_ENTITY_SUBTYPE_MASK0x
> +
> +#define MEDIA_ENTITY_TYPE_NODE   (1 << MEDIA_ENTITY_TYPE_SHIFT)
> ...
> +#define MEDIA_ENTITY_TYPE_NODE_ALSA  (MEDIA_ENTITY_TYPE_NODE + 3)

ALSA has PCM and MIDI devices, and several types of mixer controls.
(It also has hardware dependent and timer devices, but I don't think
these would need topology information.)  So we need at least these:
  MEDIA_ENTITY_TYPE_NODE_ALSA_PCM
  MEDIA_ENTITY_TYPE_NODE_ALSA_MIDI
  MEDIA_ENTITY_TYPE_SUBDEV_ALSA_CONTROL

Furthermore, topology information is also needed for entities not
associated with a mixer control, such as microphones, speakers, jacks/
connectors, and effect units.  These entities are defined in the USB and
HD audio specifications, but are not yet handled by ALSA.

> +struct media_entity {
> ...
> + union {
> + /* Node specifications */
> + struct {
> + u32 major;
> + u32 minor;
> + } v4l;
> + struct {
> + u32 major;
> + u32 minor;
> + } fb;
> + int alsa;
> + int dvb;
> +
> + /* Sub-device specifications */
> + /* Nothing needed yet */
> + };

ALSA devices are not addressed by their device node but with card/device/
subdevice numbers; mixer controls have numeric IDs, unique per card:

struct {
int card;
int device;
int subdevice;
} alsa_device;
struct {
int card;
int numid;
} alsa_control;


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v6 02/12] media: Media device

[Clemens Ladisch]

Laurent Pinchart wrote:
> +struct media_device {
> ...
> + u8 model[32];
> + u8 serial[40];
> + u8 bus_info[32];

All drivers and userspace applications have to treat this as char[], so
why u8[]?


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html