* Re: Next round: revised futex(2) man page for review
From: Thomas Gleixner @ 2015-08-19 22:40 UTC (permalink / raw)
To: Darren Hart
Cc: Michael Kerrisk (man-pages), Torvald Riegel, Carlos O'Donell,
Ingo Molnar, Jakub Jelinek, linux-man, lkml, Davidlohr Bueso,
Arnd Bergmann, Steven Rostedt, Peter Zijlstra, Linux API,
Roland McGrath, Anton Blanchard, Eric Dumazet, bill o gallmeister,
Jan Kiszka, Daniel Wagner, Rich Felker, Andy Lutomirski,
bert hubert, Rusty Russell, Heinrich Schuchardt
In-Reply-To: <20150805222140.GA74817@vmdeb7>
On Wed, 5 Aug 2015, Darren Hart wrote:
> On Mon, Jul 27, 2015 at 02:07:15PM +0200, Michael Kerrisk (man-pages) wrote:
> > .\" FIXME XXX ===== Start of adapted Hart/Guniguntala text =====
> > .\" The following text is drawn from the Hart/Guniguntala paper
> > .\" (listed in SEE ALSO), but I have reworded some pieces
> > .\" significantly. Please check it.
> >
> > The PI futex operations described below differ from the other
> > futex operations in that they impose policy on the use of the
> > value of the futex word:
> >
> > * If the lock is not acquired, the futex word's value shall be
> > 0.
> >
> > * If the lock is acquired, the futex word's value shall be the
> > thread ID (TID; see gettid(2)) of the owning thread.
> >
> > * If the lock is owned and there are threads contending for the
> > lock, then the FUTEX_WAITERS bit shall be set in the futex
> > word's value; in other words, this value is:
> >
> > FUTEX_WAITERS | TID
> >
> >
> > Note that a PI futex word never just has the value FUTEX_WAITERS,
> > which is a permissible state for non-PI futexes.
>
> The second clause is inappropriate. I don't know if that was yours or
> mine, but non-PI futexes do not have a kernel defined value policy, so
> ==FUTEX_WAITERS cannot be a "permissible state" as any value is
> permissible for non-PI futexes, and none have a kernel defined state.
Depends. If the regular futex is configured as robust, then we have a
kernel defined value policy as well.
> > .\" FIXME I'm not quite clear on the meaning of the following sentence.
> > .\" Is this trying to say that while blocked in a
> > .\" FUTEX_WAIT_REQUEUE_PI, it could happen that another
> > .\" task does a FUTEX_WAKE on uaddr that simply causes
> > .\" a normal wake, with the result that the FUTEX_WAIT_REQUEUE_PI
> > .\" does not complete? What happens then to the FUTEX_WAIT_REQUEUE_PI
> > .\" opertion? Does it remain blocked, or does it unblock
> > .\" In which case, what does user space see?
> >
> > The
> > waiter can be removed from the wait on uaddr via
> > FUTEX_WAKE without requeueing on uaddr2.
>
> Userspace should see the task wake and continue executing. This would
> effectively be a cancelation operation - which I didn't think was
> supported. Thomas?
We probably never intended to support it, but looking at the code it
works (did not try it though). It returns to user space with
-EWOULDBLOCK. So it basically behaves like any other spurious wakeup.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Eric B Munson @ 2015-08-19 21:33 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Vlastimil Babka, Jonathan Corbet,
Kirill A. Shutemov, linux-kernel, dri-devel, linux-mm, linux-api
In-Reply-To: <20150812115909.GA5182@dhcp22.suse.cz>
[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]
On Wed, 12 Aug 2015, Michal Hocko wrote:
> On Sun 09-08-15 01:22:53, Eric B Munson wrote:
> > The cost of faulting in all memory to be locked can be very high when
> > working with large mappings. If only portions of the mapping will be
> > used this can incur a high penalty for locking.
> >
> > For the example of a large file, this is the usage pattern for a large
> > statical language model (probably applies to other statical or graphical
> > models as well). For the security example, any application transacting
> > in data that cannot be swapped out (credit card data, medical records,
> > etc).
> >
> > This patch introduces the ability to request that pages are not
> > pre-faulted, but are placed on the unevictable LRU when they are finally
> > faulted in. The VM_LOCKONFAULT flag will be used together with
> > VM_LOCKED and has no effect when set without VM_LOCKED.
>
> I do not like this very much to be honest. We have only few bits
> left there and it seems this is not really necessary. I thought that
> LOCKONFAULT acts as a modifier to the mlock call to tell whether to
> poppulate or not. The only place we have to persist it is
> mlockall(MCL_FUTURE) AFAICS. And this can be handled by an additional
> field in the mm_struct. This could be handled at __mm_populate level.
> So unless I am missing something this would be much more easier
> in the end we no new bit in VM flags would be necessary.
>
> This would obviously mean that the LOCKONFAULT couldn't be exported to
> the userspace but is this really necessary?
Sorry for the latency here, I was on vacation and am now at plumbers.
I am not sure that growing the mm_struct by another flags field instead
of using available bits in the vm_flags is the right choice. After this
patch, we still have 3 free bits on 32 bit architectures (2 after the
userfaultfd set IIRC). The group which asked for this feature here
wants the ability to distinguish between LOCKED and LOCKONFAULT regions
and without the VMA flag there isn't a way to do that.
Do we know that these last two open flags are needed right now or is
this speculation that they will be and that none of the other VMA flags
can be reclaimed?
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 0/3] SysFS driver for QEMU fw_cfg device
From: Gabriel L. Somlo @ 2015-08-19 20:49 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Richard W.M. Jones, Jordan Justen,
x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, QEMU Developers,
gleb-RmZWMc9puTNJc61us3aD9laTQe2KTcn/, Matt Fleming,
kernelnewbies-7JyXY6prKcjpASu1u0TL5ti2O/JbrIOy, Gerd Hoffmann,
Paolo Bonzini, Laszlo Ersek,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org,
ralf-6z/3iImG2C8G8FEW9MqTrA, zajec5-Re5JQEeQqe8AvxtiuMwx3w,
paul-DWxLp4Yu+b8AvxtiuMwx3w, galak-sgV2jX0FEOL9JmXXK+q4OQ,
linux-api-u79uwXL29TY76Z2rM5mHXA, Leif Lindholm
In-Reply-To: <CAKv+Gu-BOKTHb24LnemKmvCwZn0W-NOT=avY94wkkRTx2ViW=Q@mail.gmail.com>
Hi Ard,
On Wed, Aug 19, 2015 at 11:42:02AM +0200, Ard Biesheuvel wrote:
> (missed some cc's)
>
> On 19 August 2015 at 11:38, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > From: "Gabriel L. Somlo" <somlo-D+Gtc/HYRWM@public.gmane.org>
> >> Several different architectures supported by QEMU are set up with a
> >> "firmware configuration" (fw_cfg) device, used to pass configuration
> >> "blobs" into the guest by the host running QEMU.
> >>
> >> Historically, these config blobs were mostly of interest to the guest
> >> BIOS, but since QEMU v2.4 it is possible to insert arbitrary blobs via
> >> the command line, which makes them potentially interesting to userspace
> >> (e.g. for passing early boot environment variables, etc.).
> >>
> >
> > Does 'potentially interesting' mean you have a use case? Could you elaborate?
My personal one would be something like:
cat > guestinfo.txt << EOT
KEY1="val1"
KEY2="val2"
...
EOT
qemu-system-x86_64 ... -fw-cfg name="opt/guestinfo",file=./guestinfo.txt ...
Then, from inside the guest:
. /sys/firmware/qemu_fw_cfg/by_name/opt/guestinfo/raw
do_something_with $KEY1 $KEY2
...
But I'm thinking this is only one of the many positive things one
could do with the ability to access random host-supplied blobs from
guest userspace :)
> >> 1/3 - probes for the qemu fw_cfg device in locations known to work on
> >> the supported architectures, in decreasing order of "likelihood".
> >>
> >> While it *may* be possible to detect the presence of fw_cfg via
> >> acpi or dtb (on x86 and arm, respectively), there's no way I know
> >> of attempting that on sun4 and ppc/mac, so I've stuck with simply
> >> probing (the fw_cfg_modes[] structure and fw_cfg_io_probe() function)
> >> in fw_cfg.c. I could use some advice on how else that could be
> >> done more elegantly, if needed.
> >>
> >
> > Sorry, but this is really out of the question, at least on ARM, but surely on
> > other architectures as well. You can't just go around and probe random memory
> > addresses. Perhaps QEMU tolerates it, but on anything that resembles a real
> > system, this will immediately blow up. Also, what happens if the QEMU memory
> > map changes? Add more probes addresses?
> >
> > It is not /that/ difficult to simply wire it up to the DT and ACPI
> > infrastructures, there are plenty of examples in the kernel tree how to
> > accomplish that. As a bonus, it removes all the arch specific knowledge
> > from your code, which means that if QEMU grows support for another DT or
> > ACPI based architecture, it will just work.
I was *hoping* a successful call to request_[mem_]region() will be
enough in the way of asking for permission before probing for the
fw_cfg registers, but I realize that might still not be polite enough :)
DT on ARM is fine, and I'm certainly happy to learn how to do it (even
though my main focus is, for now, x86). The unfortunate thing though
is that on x86, fw_cfg is *not* AFAICT in ACPI, so I'd have to detour into
first adding it in on the host side, before I can rewrite the guest side
driver to look it up in there :)
> > I am not sure how relevant sun4 and ppc/mac are for what you are trying to
> > accomplish, but perhaps it would be best to focus on x86 and ARM for now
> > and do it correctly. If the probing is actually needed, you can always add
> > it later.
I guess that's the direction things seem to be headed, although it would
make me a bit sad to leave out sun and ppc right from the very beginning :)
Thanks,
--Gabriel
PS. If you have one .c file in the kernel which does any of the DT-on-arm
boilerplate I'm supposed to immitate, I'd appreciate the shortcut :)
PS2. Do you happen to be in Seattle right now ? :)
^ permalink raw reply
* Re: [PATCH v2 0/3] SysFS driver for QEMU fw_cfg device
From: Ard Biesheuvel @ 2015-08-19 9:42 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Gabriel Somlo, Richard W.M. Jones, Jordan Justen,
x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, QEMU Developers,
gleb-RmZWMc9puTNJc61us3aD9laTQe2KTcn/, Matt Fleming,
kernelnewbies-7JyXY6prKcjpASu1u0TL5ti2O/JbrIOy, Gerd Hoffmann,
Paolo Bonzini, Laszlo Ersek,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org,
ralf-6z/3iImG2C8G8FEW9MqTrA, zajec5-Re5JQEeQqe8AvxtiuMwx3w,
paul-DWxLp4Yu+b8AvxtiuMwx3w, galak-sgV2jX0FEOL9JmXXK+q4OQ,
linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Leif Lindholm, Ard Biesheuvel
In-Reply-To: <1439977109-20314-1-git-send-email-ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
(missed some cc's)
On 19 August 2015 at 11:38, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> From: "Gabriel L. Somlo" <somlo-D+Gtc/HYRWM@public.gmane.org>
>
> Hi Gabriel,
>
>> Several different architectures supported by QEMU are set up with a
>> "firmware configuration" (fw_cfg) device, used to pass configuration
>> "blobs" into the guest by the host running QEMU.
>>
>> Historically, these config blobs were mostly of interest to the guest
>> BIOS, but since QEMU v2.4 it is possible to insert arbitrary blobs via
>> the command line, which makes them potentially interesting to userspace
>> (e.g. for passing early boot environment variables, etc.).
>>
>
> Does 'potentially interesting' mean you have a use case? Could you elaborate?
>
>> In addition to cc-ing the people and lists indicated by get-maintainer.pl,
>> I've added a few extra lists suggested by Matt Fleming on the qemu-devel
>> list, as well as the qemu-devel list itself.
>>
>> Also cc-ing kernelnewbies, as this is my very first kenel contribution,
>> so please go easy on me for whatever silly n00b mistakes I might have still
>> missed, in spite of trying hard to do all my homework properly... :)
>>
>> The series consists of three patches:
>>
>> 1/3 - probes for the qemu fw_cfg device in locations known to work on
>> the supported architectures, in decreasing order of "likelihood".
>>
>> While it *may* be possible to detect the presence of fw_cfg via
>> acpi or dtb (on x86 and arm, respectively), there's no way I know
>> of attempting that on sun4 and ppc/mac, so I've stuck with simply
>> probing (the fw_cfg_modes[] structure and fw_cfg_io_probe() function)
>> in fw_cfg.c. I could use some advice on how else that could be
>> done more elegantly, if needed.
>>
>
> Sorry, but this is really out of the question, at least on ARM, but surely on
> other architectures as well. You can't just go around and probe random memory
> addresses. Perhaps QEMU tolerates it, but on anything that resembles a real
> system, this will immediately blow up. Also, what happens if the QEMU memory
> map changes? Add more probes addresses?
>
> It is not /that/ difficult to simply wire it up to the DT and ACPI
> infrastructures, there are plenty of examples in the kernel tree how to
> accomplish that. As a bonus, it removes all the arch specific knowledge
> from your code, which means that if QEMU grows support for another DT or
> ACPI based architecture, it will just work.
>
> I am not sure how relevant sun4 and ppc/mac are for what you are trying to
> accomplish, but perhaps it would be best to focus on x86 and ARM for now
> and do it correctly. If the probing is actually needed, you can always add
> it later.
>
> --
> Ard.
>
^ permalink raw reply
* Re: Persistent Reservation API V2
From: Jens Axboe @ 2015-08-18 17:58 UTC (permalink / raw)
To: Christoph Hellwig, Christoph Hellwig
Cc: linux-api, dm-devel, linux-nvme, linux-scsi, linux-kernel
In-Reply-To: <20150817191328.GB9383@infradead.org>
On 08/17/2015 01:13 PM, Christoph Hellwig wrote:
> Does this look fine to you Jens?
>
> I'd love to get this API into 4.3 so I can submit the NFS SCSI layout
> patches that depend on it for 4.4.
>
I'll take a look at this today.
--
Jens Axboe
^ permalink raw reply
* Re: Next round: revised futex(2) man page for review
From: Darren Hart @ 2015-08-18 12:45 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Michael Kerrisk (man-pages), Torvald Riegel, Carlos O'Donell,
Ingo Molnar, Jakub Jelinek, linux-man, lkml, Davidlohr Bueso,
Arnd Bergmann, Steven Rostedt, Peter Zijlstra, Linux API,
Roland McGrath, Anton Blanchard, Eric Dumazet, bill o gallmeister,
Jan Kiszka, Daniel Wagner, Rich Felker, Andy Lutomirski,
bert hubert, Rusty Russell, Heinrich Schuchardt
In-Reply-To: <alpine.DEB.2.11.1508200031220.3873@nanos>
On Thu, Aug 20, 2015 at 12:40:46AM +0200, Thomas Gleixner wrote:
> On Wed, 5 Aug 2015, Darren Hart wrote:
> > On Mon, Jul 27, 2015 at 02:07:15PM +0200, Michael Kerrisk (man-pages) wrote:
> > > .\" FIXME XXX ===== Start of adapted Hart/Guniguntala text =====
> > > .\" The following text is drawn from the Hart/Guniguntala paper
> > > .\" (listed in SEE ALSO), but I have reworded some pieces
> > > .\" significantly. Please check it.
> > >
> > > The PI futex operations described below differ from the other
> > > futex operations in that they impose policy on the use of the
> > > value of the futex word:
> > >
> > > * If the lock is not acquired, the futex word's value shall be
> > > 0.
> > >
> > > * If the lock is acquired, the futex word's value shall be the
> > > thread ID (TID; see gettid(2)) of the owning thread.
> > >
> > > * If the lock is owned and there are threads contending for the
> > > lock, then the FUTEX_WAITERS bit shall be set in the futex
> > > word's value; in other words, this value is:
> > >
> > > FUTEX_WAITERS | TID
> > >
> > >
> > > Note that a PI futex word never just has the value FUTEX_WAITERS,
> > > which is a permissible state for non-PI futexes.
> >
> > The second clause is inappropriate. I don't know if that was yours or
> > mine, but non-PI futexes do not have a kernel defined value policy, so
> > ==FUTEX_WAITERS cannot be a "permissible state" as any value is
> > permissible for non-PI futexes, and none have a kernel defined state.
>
> Depends. If the regular futex is configured as robust, then we have a
> kernel defined value policy as well.
Indeed, thanks for catching that.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Persistent Reservation API V2
From: Christoph Hellwig @ 2015-08-17 19:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, linux-api, dm-devel, linux-nvme, linux-scsi,
linux-kernel
In-Reply-To: <1439360604-16192-1-git-send-email-hch@lst.de>
Does this look fine to you Jens?
I'd love to get this API into 4.3 so I can submit the NFS SCSI layout
patches that depend on it for 4.4.
^ permalink raw reply
* Re: [RFC] Media Controller, the next generation
From: Mauro Carvalho Chehab @ 2015-08-17 15:22 UTC (permalink / raw)
To: Hans Verkuil
Cc: Laurent Pinchart, Linux Media Mailing List,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <55D1DFD2.8050807-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org>
Em Mon, 17 Aug 2015 15:21:22 +0200
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> escreveu:
> On 08/17/2015 02:39 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 17 Aug 2015 12:33:30 +0200
> > Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> escreveu:
> >> That's not what I meant. I meant using the mc_ prefix instead of the media_
> >> prefix for internal structs. So media_entity in the public API would map to
> >> mc_entity in the internal API. Ditto for media_interface/mc_interface,
> >> media_pad/mc_pad, etc.
> >
> > Ah! that could work. Yet, I guess we decided to not use mc_ internally
> > in the first place several years ago because there was already some
> > namespace conflict.
> >
> > Ah, yes:
> >
> > $ git grep -e \\bmc_
> > ...
> > arch/powerpc/kernel/mce.c: struct machine_check_event *mc_evt;
> > arch/powerpc/kernel/mce.c: mc_evt = this_cpu_ptr(&mce_event[index]);
> > arch/powerpc/kernel/mce.c: *mce = *mc_evt;
> > ...
> > arch/um/drivers/line.h: struct mc_device mc;
> > ...
> > arch/um/drivers/net_kern.c:static struct mc_device net_mc = {
> > ...
> >
> > A total of 302 references... Among them, some arch-dependent stuff,
> > including x86 and powerPC error hanlding stuff (MCE, APEI).
> >
> > I would avoid it, as the risk of namespace collisions are high.
>
> OK.
>
> >
> >> Using graph_ instead of mc_ is something I would be OK with as well.
>
> What about graph_ as the internal prefix? Or mctl_?
>
> >> I am a bit worried about this. To my knowledge applications that use the MC
> >> today are expected to know which pad of an entity does what, and it identifies
> >> the pad by index.
> >>
> >> The new public API should still provide applications with this information in
> >> one way or another. The pad ID won't work, certainly not in the dynamic case,
> >> the PAD_TYPE as suggested here will only work as long as there is only one
> >> pad per type. Suppose there is an entity with two output pads, both for video?
> >> One might be SDTV, one HDTV. How to tell the difference?
> >
> > The first one will have "SDTV" as type, the other one "HDTV". So, no problem
> > with that.
>
> So now we have SDTV and HDTV, the next entity will have YUV_420 and YUV_422 pads,
> or whatever. The number of types will quickly escalate. It's similar to the
> 'entity type' problem (e.g. sensor, flash, video encoder, scaler, etc.).
well, as new pads will require new Kernel patches, nothing prevents to add
a new #define. A string would also work.
> >
> > There are, however, some usages where an index is enough. For example, the
> > hardware demux filter outputs can fully be identified by just an index, and
> > each index could (ideally) be mapped into a different DMA.
> >
> > So, I guess we need to offer the flexibility to keep using indexes where
> > it makes sense.
> >
> > That's why I proposed a find_pad function that would allow to use an
> > index:
> > media_pad *find_pad(struct media_entity entity, enum pad_usage usage, int idx);
> > or
> > media_pad *find_pad(struct media_entity entity, char *pad_name);
> >
> >> One option might be to have a pad_type or data_type for basic type information
> >> to have generic code that just wants to find VBI/VIDEO/AUDIO streaming pads,
> >> and a name[32] field that is uniquely identifying the pad and that can be used
> >> in userspace applications (or drivers for that matter by adding a find_pad_name()).
> >
> > just a name could be enough:
> > vbi_pad = find_pad(entity, "vbi");
> >
> > However, that would require to have a strict namespace if we want the core
> > to rely on it.
>
> I think names would work: we can make a number of 'core' names that should be
> used where appropriate, and leave it up to the driver otherwise.
OK.
> So if there is only one video source pad, then call the pad "video". If there
> are multiple video source pads, then zero or one pads can be called "video",
> the others should get different names "video-sdtv").
>
> To be honest, I'm not terribly keen on this. I think a data/pad type + name
> would work better.
Me too.
> >>> };
> >>>
> >>> And have the structs with (some) properties embed on it, like:
> >>>
> >>> struct media_graph_entity {
> >>> struct media_graph_object obj;
> >>
> >> This doesn't make sense: struct media_graph_topology would fill in an
> >> array of struct media_graph_object would point to media objects that
> >> in turn contain a struct media_graph_object.
> >
> > Sorry, I didn't get your idea. Could you please give a C code example on
> > what you're thinking?
> >
> >>
> >> I wouldn't embed a struct media_graph_object in these structs, there
> >> is no point to that. Thinking about this some more you don't need the
> >> length field either, only an id (this gives you the type, so you know
> >> whether the object_data pointer is for a media_entity or a media_pad
> >> or whatever) and the pointer. Strictly speaking you would only need
> >> the 'type' part of the ID, but I see no reason not to fill in the full
> >> ID.
> >>
> >>> u32 flags;
> >>> /* ... */
> >>> char name[64];
> >>> };
> >>>
> >>> Please notice that we don't need to add reserved fields at the structs,
> >>> as we're now putting the struct length at the media_graph_object.
> >>>
> >>> So, if we need to, for example, add a new "foo" inside the
> >>> media_graph_entity:
> >>>
> >>> struct media_graph_entity {
> >>> struct media_graph_object obj;
> >>> u32 flags;
> >>> /* ... */
> >>> char name[64];
> >>> u32 foo;
> >>> };
> >>
> >> No, you can't. Say you've compiled the application with a header that includes
> >> the foo field, and then you run the same application with an older kernel that
> >> doesn't have the foo field. Any access to foo would give garbage back (or fail).
> >>
> >> You really need those reserved fields. The alternative would be to mess around
> >> with different struct versions, and that's painful.
> >
> > No. The length will do that. All we need to do is to document that the data
> > should be zeroed on userspace before filling the data from G_TOPOLOGY.
> >
> > For example:
> >
> > let's say that, on Kernel 4.3: this is declared as:
> > struct media_graph_entity {
> > struct media_graph_object obj;
> > u32 flags;
> > char name[64];
> > };
> >
> > And, on Kernel 4.4, the new "foo" field got introduced:
> >
> > struct media_graph_entity {
> > struct media_graph_object obj;
> > u32 flags;
> > char name[64];
> > u32 foo;
> > };
> >
> > Of course, just like we do with reserved fields, we should ensure that
> > old apps can live without the "foo" field, and that new apps will
> > keep working with older Kernels.
> >
> > Now, we have:
> >
> > The length of the data on 4.3 is: sizeof(obj) + 68
> > The length of the data on 4.4 is: sizeof(obj) + 72
> >
> > As per my proposal, we need to pass the length for the public API
> > object.
> >
> > struct media_graph_object {
> > u32 length;
> > u32 type;
> > u32 id;
> > void *data;
> > } __attribute__ ((packed));
> >
> > An application for the G_TOPOLOGY would do something like:
> >
> > /* p is the pointer for the data returned from Kernel */
> > struct media_graph_entity
> > get_an_entity_graph_obj(struct media_graph_obj *obj)
> > {
> > struct media_graph_entity *ent;
> > int len;
> >
> > len = sizeof(*ent);
> > if (len > obj->length)
> > len = obj->length;
> >
> > ent = calloc(len, 1);
> > memcpy (ent, obj->data, len);
> >
> > return ent;
> > }
>
> OK, so everything has to be copied in userspace, unpacking the objects.
Well, that's one way, to just quickly express what I meant to say ;)
It is possible to avoid that using other data representations on
userspace, like having a get_field() macro that would return zero
if trying to access past the length.
> >
> > There are three cases:
> >
> > 1) both app and Kernel are compiled against the same headers
> >
> > No problem.
> >
> > 2) app is compiled against v4.4 headers
> > kernel compiled against v4.3 headers
> >
> > As the struct were zeroed before memcpy, all entities will have foo = 0.
> >
> > No problem.
> >
> > 3) app is compiled against v4.3 headers
> > kernel compiled against v4.4 headers
> >
> > As the struct won't have the "foo" field, and it will be ignored.
> >
> > No problem.
> >
> > So, I don't see any usage to add reserved fields.
> >
> >>
> >>>
> >>> There are some advantages of this approach:
> >>> - If the size of the entity will change, and obj.length will be bigger.
> >>> Userspace will allocate more space to store the object, but will be
> >>> backward compatible;
> >>> - We can add new object types anytime. If userspace doesn't know the new
> >>> type, it should simply discard the object and go to the next one. Again,
> >>> backward compatible.
> >>>
> >>> We may eventually add a way for userspace to request only a subset of
> >>> the graph elements or to add an ioctl or some other sort of event that
> >>> will report topology changes.
> >>
> >> It's an option, but the first 'advantage' doesn't actually work, and I am
> >> not sure about the second. Yes, it is an advantage but it comes at a price:
> >> the void pointer. I very much prefer strict typing.
> >
> > See above.
> >
> > We can avoid the void pointer with something like:
> >
> > struct media_graph_object {
> > u32 length;
> > u32 type;
>
> Type can be dropped.
I guess you meant to say that it can be merged with ID. Yes, agreed.
> > u32 id;
> > } __attribute__ ((packed));
> >
> > struct media_graph_entity {
> > struct media_graph_object obj;
> > char name[64];
> > } __attribute__ ((packed));
>
> This makes more sense without the pointer. But this also means that the
> objects pointer in media_graph_topology isn't an array. Instead the
> next graph object starts at ((void *)objects + objects->length).
Either that or we'll have an array of pointers, or, as suggested during
the MC, offsets that will be converted into pointers with something like:
obj[4] += obj_base;
>
> >
> > Of course, length is needed, in order to avoid filling it with reserved
> > stuff.
> >
> >>
> >> BTW, my initial proposal had a __u32 reserved[64] field, I'd redo that as
> >> follows:
> >>
> >> struct
> >> __u32 num_reserved;
> >> void *ptr_reserved;
> >> } reserved[32];
> >>
> >> This will make 32/64 bit pointer size differences much easier to handle.
> >>
> >> And in my opinion, if we end up with so many different object types, then
> >> we have a much bigger problem and another redesign would be required.
> >
> > Redesign the API because we add more things is crappy! We should do
> > something that, ideally, will never require us to redesign it again.
>
> That's not what I meant. If we end up with so many different object types,
> then the MC API has gone crazy and becomes unusable.
Well, we currently have 80+ ioctls at V4L2, and it is still usable.
> The MC is supposed
> to have just a few types: originally entity, pad, link and now we are adding
> interface and property.
And, from your own suggestions: connectors
> Perhaps there will be one or two more, but if we
> end up with 30 types, then there is something seriously wrong.
I agree that 30 sounds a good number, but the thing is: it is hard to
tell that in advance. As we add support for other subsystems like IIO,
different needs could popup.
For example, if I remember well, on IIO, PADs aren't needed. So we
may end by having some pad-less type of entity to fulfill its
needs.
The thing is: APIs generally start simple, but tend to grow as new
needs happen. See the crop/scaler thing... every time we discuss that,
new proposals to extend the API by creating a new ioctl or a new
type of crop that we didn't foresee in the past discussions.
> >> In general, I prefer strict typing over void pointers, and having to cast
> >> all the time is something I'd really like to avoid. This API also gives
> >> you filtering for free (just leave the relevant pointers NULL).
> >
> > Adding a filter field is not hard, and, as pointed above, we don't need
> > void pointers.
>
> Doing it this way means that you return a blob that needs to be unpacked
> first. It's useless without that unpacking step. Is that what I want? I
> would like to hear what others think about this. It makes this a very
> complicated and hard-to-use ioctl.
Using a single ioctl to pack different things will always require some
unpacking/parsing effort on the other side, whatever approach we take.
That's why I think we should have a way to report topology changes as
well.
>
> Regards,
>
> Hans
>
^ permalink raw reply
* Re: [RFC] Media Controller, the next generation
From: Hans Verkuil @ 2015-08-17 13:21 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Laurent Pinchart, Linux Media Mailing List, linux-api
In-Reply-To: <20150817093923.301ac60b@recife.lan>
On 08/17/2015 02:39 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 17 Aug 2015 12:33:30 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>> That's not what I meant. I meant using the mc_ prefix instead of the media_
>> prefix for internal structs. So media_entity in the public API would map to
>> mc_entity in the internal API. Ditto for media_interface/mc_interface,
>> media_pad/mc_pad, etc.
>
> Ah! that could work. Yet, I guess we decided to not use mc_ internally
> in the first place several years ago because there was already some
> namespace conflict.
>
> Ah, yes:
>
> $ git grep -e \\bmc_
> ...
> arch/powerpc/kernel/mce.c: struct machine_check_event *mc_evt;
> arch/powerpc/kernel/mce.c: mc_evt = this_cpu_ptr(&mce_event[index]);
> arch/powerpc/kernel/mce.c: *mce = *mc_evt;
> ...
> arch/um/drivers/line.h: struct mc_device mc;
> ...
> arch/um/drivers/net_kern.c:static struct mc_device net_mc = {
> ...
>
> A total of 302 references... Among them, some arch-dependent stuff,
> including x86 and powerPC error hanlding stuff (MCE, APEI).
>
> I would avoid it, as the risk of namespace collisions are high.
OK.
>
>> Using graph_ instead of mc_ is something I would be OK with as well.
What about graph_ as the internal prefix? Or mctl_?
>> I am a bit worried about this. To my knowledge applications that use the MC
>> today are expected to know which pad of an entity does what, and it identifies
>> the pad by index.
>>
>> The new public API should still provide applications with this information in
>> one way or another. The pad ID won't work, certainly not in the dynamic case,
>> the PAD_TYPE as suggested here will only work as long as there is only one
>> pad per type. Suppose there is an entity with two output pads, both for video?
>> One might be SDTV, one HDTV. How to tell the difference?
>
> The first one will have "SDTV" as type, the other one "HDTV". So, no problem
> with that.
So now we have SDTV and HDTV, the next entity will have YUV_420 and YUV_422 pads,
or whatever. The number of types will quickly escalate. It's similar to the
'entity type' problem (e.g. sensor, flash, video encoder, scaler, etc.).
>
> There are, however, some usages where an index is enough. For example, the
> hardware demux filter outputs can fully be identified by just an index, and
> each index could (ideally) be mapped into a different DMA.
>
> So, I guess we need to offer the flexibility to keep using indexes where
> it makes sense.
>
> That's why I proposed a find_pad function that would allow to use an
> index:
> media_pad *find_pad(struct media_entity entity, enum pad_usage usage, int idx);
> or
> media_pad *find_pad(struct media_entity entity, char *pad_name);
>
>> One option might be to have a pad_type or data_type for basic type information
>> to have generic code that just wants to find VBI/VIDEO/AUDIO streaming pads,
>> and a name[32] field that is uniquely identifying the pad and that can be used
>> in userspace applications (or drivers for that matter by adding a find_pad_name()).
>
> just a name could be enough:
> vbi_pad = find_pad(entity, "vbi");
>
> However, that would require to have a strict namespace if we want the core
> to rely on it.
I think names would work: we can make a number of 'core' names that should be
used where appropriate, and leave it up to the driver otherwise.
So if there is only one video source pad, then call the pad "video". If there
are multiple video source pads, then zero or one pads can be called "video",
the others should get different names "video-sdtv").
To be honest, I'm not terribly keen on this. I think a data/pad type + name
would work better.
>>> };
>>>
>>> And have the structs with (some) properties embed on it, like:
>>>
>>> struct media_graph_entity {
>>> struct media_graph_object obj;
>>
>> This doesn't make sense: struct media_graph_topology would fill in an
>> array of struct media_graph_object would point to media objects that
>> in turn contain a struct media_graph_object.
>
> Sorry, I didn't get your idea. Could you please give a C code example on
> what you're thinking?
>
>>
>> I wouldn't embed a struct media_graph_object in these structs, there
>> is no point to that. Thinking about this some more you don't need the
>> length field either, only an id (this gives you the type, so you know
>> whether the object_data pointer is for a media_entity or a media_pad
>> or whatever) and the pointer. Strictly speaking you would only need
>> the 'type' part of the ID, but I see no reason not to fill in the full
>> ID.
>>
>>> u32 flags;
>>> /* ... */
>>> char name[64];
>>> };
>>>
>>> Please notice that we don't need to add reserved fields at the structs,
>>> as we're now putting the struct length at the media_graph_object.
>>>
>>> So, if we need to, for example, add a new "foo" inside the
>>> media_graph_entity:
>>>
>>> struct media_graph_entity {
>>> struct media_graph_object obj;
>>> u32 flags;
>>> /* ... */
>>> char name[64];
>>> u32 foo;
>>> };
>>
>> No, you can't. Say you've compiled the application with a header that includes
>> the foo field, and then you run the same application with an older kernel that
>> doesn't have the foo field. Any access to foo would give garbage back (or fail).
>>
>> You really need those reserved fields. The alternative would be to mess around
>> with different struct versions, and that's painful.
>
> No. The length will do that. All we need to do is to document that the data
> should be zeroed on userspace before filling the data from G_TOPOLOGY.
>
> For example:
>
> let's say that, on Kernel 4.3: this is declared as:
> struct media_graph_entity {
> struct media_graph_object obj;
> u32 flags;
> char name[64];
> };
>
> And, on Kernel 4.4, the new "foo" field got introduced:
>
> struct media_graph_entity {
> struct media_graph_object obj;
> u32 flags;
> char name[64];
> u32 foo;
> };
>
> Of course, just like we do with reserved fields, we should ensure that
> old apps can live without the "foo" field, and that new apps will
> keep working with older Kernels.
>
> Now, we have:
>
> The length of the data on 4.3 is: sizeof(obj) + 68
> The length of the data on 4.4 is: sizeof(obj) + 72
>
> As per my proposal, we need to pass the length for the public API
> object.
>
> struct media_graph_object {
> u32 length;
> u32 type;
> u32 id;
> void *data;
> } __attribute__ ((packed));
>
> An application for the G_TOPOLOGY would do something like:
>
> /* p is the pointer for the data returned from Kernel */
> struct media_graph_entity
> get_an_entity_graph_obj(struct media_graph_obj *obj)
> {
> struct media_graph_entity *ent;
> int len;
>
> len = sizeof(*ent);
> if (len > obj->length)
> len = obj->length;
>
> ent = calloc(len, 1);
> memcpy (ent, obj->data, len);
>
> return ent;
> }
OK, so everything has to be copied in userspace, unpacking the objects.
>
> There are three cases:
>
> 1) both app and Kernel are compiled against the same headers
>
> No problem.
>
> 2) app is compiled against v4.4 headers
> kernel compiled against v4.3 headers
>
> As the struct were zeroed before memcpy, all entities will have foo = 0.
>
> No problem.
>
> 3) app is compiled against v4.3 headers
> kernel compiled against v4.4 headers
>
> As the struct won't have the "foo" field, and it will be ignored.
>
> No problem.
>
> So, I don't see any usage to add reserved fields.
>
>>
>>>
>>> There are some advantages of this approach:
>>> - If the size of the entity will change, and obj.length will be bigger.
>>> Userspace will allocate more space to store the object, but will be
>>> backward compatible;
>>> - We can add new object types anytime. If userspace doesn't know the new
>>> type, it should simply discard the object and go to the next one. Again,
>>> backward compatible.
>>>
>>> We may eventually add a way for userspace to request only a subset of
>>> the graph elements or to add an ioctl or some other sort of event that
>>> will report topology changes.
>>
>> It's an option, but the first 'advantage' doesn't actually work, and I am
>> not sure about the second. Yes, it is an advantage but it comes at a price:
>> the void pointer. I very much prefer strict typing.
>
> See above.
>
> We can avoid the void pointer with something like:
>
> struct media_graph_object {
> u32 length;
> u32 type;
Type can be dropped.
> u32 id;
> } __attribute__ ((packed));
>
> struct media_graph_entity {
> struct media_graph_object obj;
> char name[64];
> } __attribute__ ((packed));
This makes more sense without the pointer. But this also means that the
objects pointer in media_graph_topology isn't an array. Instead the
next graph object starts at ((void *)objects + objects->length).
>
> Of course, length is needed, in order to avoid filling it with reserved
> stuff.
>
>>
>> BTW, my initial proposal had a __u32 reserved[64] field, I'd redo that as
>> follows:
>>
>> struct
>> __u32 num_reserved;
>> void *ptr_reserved;
>> } reserved[32];
>>
>> This will make 32/64 bit pointer size differences much easier to handle.
>>
>> And in my opinion, if we end up with so many different object types, then
>> we have a much bigger problem and another redesign would be required.
>
> Redesign the API because we add more things is crappy! We should do
> something that, ideally, will never require us to redesign it again.
That's not what I meant. If we end up with so many different object types,
then the MC API has gone crazy and becomes unusable. The MC is supposed
to have just a few types: originally entity, pad, link and now we are adding
interface and property. Perhaps there will be one or two more, but if we
end up with 30 types, then there is something seriously wrong.
>> In general, I prefer strict typing over void pointers, and having to cast
>> all the time is something I'd really like to avoid. This API also gives
>> you filtering for free (just leave the relevant pointers NULL).
>
> Adding a filter field is not hard, and, as pointed above, we don't need
> void pointers.
Doing it this way means that you return a blob that needs to be unpacked
first. It's useless without that unpacking step. Is that what I want? I
would like to hear what others think about this. It makes this a very
complicated and hard-to-use ioctl.
Regards,
Hans
^ permalink raw reply
* Re: [RFC] Media Controller, the next generation
From: Mauro Carvalho Chehab @ 2015-08-17 12:39 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Laurent Pinchart, Linux Media Mailing List, linux-api
In-Reply-To: <55D1B87A.7010107@xs4all.nl>
Em Mon, 17 Aug 2015 12:33:30 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> On 08/16/2015 03:37 PM, Mauro Carvalho Chehab wrote:
> > Em Sat, 15 Aug 2015 21:25:40 +0300
> > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> >
> >> Hi Hans,
> >>
> >> Thank you for the RFC. I believe it's quite good (at least to my eyes).
> >> Everything is of course not perfect yet so please see below for some comments.
> >
> > (added c/c linux-api@vger.kernel.org)
> >
> >>
> >> On Monday 03 August 2015 16:07:48 Hans Verkuil wrote:
> >>> Hi all,
> >>>
> >>> During last week's brainstorm meeting in Espoo, Finland, we discussed
> >>> how to proceed with the MC API. Trying to apply the existing API to DVB
> >>> devices caused a lot of controversy and this meeting was an attempt to
> >>> resolve these issues.
> >>>
> >>> This RFC is the proposal for the public API (NOT the internal API!) we
> >>> came up with.
> >>>
> >>> The main change is that interfaces (such as devices in /dev) get their own
> >>> object: struct media_interface. The struct media_entity as is used today
> >>> will not refer to interfaces anymore.
> >>>
> >>> Since interfaces control entities we also want to tell userspace which
> >>> entities are controlled by which interfaces. We want to keep things simple
> >>> and avoid having to create a new link structure just for this,
> >
> > I agree with the Hans proposal. Having a single object to represent graph
> > links makes easier for both Kernelspace and Userspace to handle them.
>
> Right.
>
> >> During the meeting we discussed the possibility of creating a
> >> media_interface_link structure (as drafted in
> >> http://linuxtv.org/downloads/presentations/mc_ws_2015/media.h). What would be
> >> the advantage of reusing media_link ?
> >
> > I would reverse the question: what's the disadvantage?
> >
> > By using the same type, it should be easier to change userspace programs
> > to support both interface and data links.
> >
> > With a single struct, if/when apps need to do graph traversal, they can
> > use a single algorithm. It also means a single code to allocate and
> > fetch the structs that come from the Kernel.
>
> Also, since all graph objects (interfaces, entities, pads) all use the same
> base struct (media_graph_obj) it makes sense that we use the same media_link
> struct for linking two graph objects.
>
> >>> so instead a struct media_link just provides two object IDs and a flags
> >>> field. The object IDs refer to pads (for data links) or entity/interface IDs
> >>> (for associating interfaces with entities).
> >>>
> >>> To make this work we need unique pad IDs.
> >>>
> >>> We will need to represent connectors as well. The media_entity struct is
> >>> used for that, but it will be marked as a connector since connectors work
> >>> slightly different from an entity: if an entity has both input and output
> >>> pads, then that means that the entity processes the input data in some way
> >>> before passing it on to the output pads. But for a connector the input and
> >>> output pads are just hooked up to input and output pins or buses and not
> >>> to one another.
> >>>
> >>> Finally we would like to get all this information atomically in userspace.
> >>> Atomicity is desired since some of this information well be dynamic.
> >>> Currently the only dynamic thing is enabling/disabling links, but that is
> >>> too limiting for devices like FPGAs where the entities can change on the
> >>> fly as well.
> >>>
> >>> Being able to get all the information with a single ioctl will simplify
> >>> implementing this atomic behavior, and it is also more efficient than
> >>> calling lots of ENUM ioctls.
> >>>
> >>> We decided to keep the existing structs and ioctls and introduce new
> >>> versions of these structs redesigned to cope with the new insights. The
> >>> sizes of the reserved fields are suggestions only.
> >>>
> >>> We had a discussion about the object IDs. Currently we have only entity IDs,
> >>> but in the redesign we'll have interface, connector and pad IDs as well.
> >>
> >> As connectors are entities I don't think they deserve a special mention here.
> >
> > Hans proposal is to use a different type for connectors. So, I guess it
> > makes sense to keep the mention.
>
> There is a difference in how connector entities behave (e.g. input pads are
> not connected to output pads, instead they represent pins on the connector) and
> how connector entities are used (V4L2 drivers might want to enumerate connector
> entities to implement VIDIOC_ENUMINPUT).
>
> So giving them their own distinct type makes sense to me.
>
> >> Regarding pad IDs, is there a reason to give pads IDs in the global ID space
> >> other than reusing media_link ?
> >
> > I actually have another usage for that ;)
> >
> > I'll comment that below.
> >
> >>
> >>> The idea from the meeting was to use the least significant X bits of the ID
> >>> to tell the difference of entity, interface and pad types and to use a flag
> >>> for marking connector entities. After thinking some more I would suggest
> >>> this scheme instead:
> >>>
> >>> #define MEDIA_ID_T_ENTITY 0
> >>> #define MEDIA_ID_T_CONNECTOR 1
> >>> #define MEDIA_ID_T_INTERFACE 2
> >>> #define MEDIA_ID_T_PAD 3
> >
> > My suggestion is actually that we should have one type for every possible
> > graph object. So, I would also add a MEDIA_ID_T_LINK, in order to represent
> > the links.
> >
> > Btw, I think that we'll end by needing a MEDIA_ID_T_GROUP too, in order
> > to properly replace the group ID, of course assuming that we keep needing
> > to have groups of objects.
> >
> > My idea, if we ever need a MEDIA_ID_T_GROUP is to represent a group as an
> > object that contains a list of other objects: entities, interfaces, links,
> > pads, etc.
> >
> > At the userspace interface, the group will pass just the object IDs for
> > the objects that belong to such group, plus group name (and maybe flags).
> >
> > This proposal still need discussions. The most important one is: do we need
> > groups on userspace? So, I would not cover groups on this RFC yet, but it
> > would be great if the API we're writing could latter be extended to support
> > groups.
> >
> > Internally, at the Kernel, groups make sense, specially when different
> > drivers are handling the same MC graph.
> >
> > For example, let's imagine a pipeline with ALSA, V4L, DVB and DRM entities
> > inside. It makes sense for the ALSA snd-audio-usb driver to group
> > all ALSA-related objects. The same is true for the DVB core and for
> > the V4L2 and DRM drivers/core.
> >
> > So, that's another reason why I think we should have an unique object
> > ID for the links too: it makes easier to group them.
>
> Note that this usage of 'group' is quite different from what the group ID
> was intended for: that was to group entities relating to the same HW device
> (e.g. group the sensor and flash together so it is clear which flash entity
> belongs to which sensor).
I know, but using a media group object, it is still possible to group sensor
and flash together too.
> My understanding is that today no in-kernel driver uses the group ID, and I
> am very skeptical about the whole concept. I think this can be much better
> described using properties.
Yeah, group ID should be removed. linking sensor/flash together could be
done using properties.
Yet, grouping analog part, digital part and alsa part of the graph
are better handled via a new graph entity to represent the groups.
I'm pretty sure we'll need it inside Kernelspace. On userspace,
I'm not so sure. Well, it could be useful, for example to generate
a dot subgraph. It could also be useful if different userspace programs
are controlling different parts of the graph (like one app for the ALSA
pipelines, another for the DRM ones, etc).
> I have no objection against a MEDIA_ID_T_LINK, but this whole group concept
> should be ignored for now and internally we should remove the group ID
> completely.
>
> The big problem I see in general with a 'group' concept is that it is
> awfully vague. Let's just table this for now and revisit when there is a
> clear need for it.
I'm afraid that this will soon be needed in Kernelspace, in order to do
the right whing when, for example, the ALSA driver gets unbind and would
need to remove just the ALSA-related objects.
It seems easier to just store all alsa-related objects into a list and
then remove them at unbind. The same is true, of course, for other
drivers that could be unbind while leaving other objects of the media
controller enabled.
> >
> >>>
> >>> #define MEDIA_ID_TYPE(id) ((id) >> 24)
> >>> #define MEDIA_ID_VAL(id) ((id) & 0xffffff)
> >>> #define MEDIA_ID_CREATE(type, id) (((type) << 24) | id)
> >>>
> >>> One objection was that the IDs would be unwieldy for users when using e.g.
> >>> media-ctl since you'll get IDs >= 0x1000000. However, I think that media-ctl
> >>> can easily either deduce the type (i.e. setting up the data path would
> >>> always assume type 'pad' or require that the user specifies the type and ID
> >>> value (i.e. only the lowest 24 bits).
> >>
> >> What would be the advantage of that scheme compared to having a type field and
> >> an id field ?
> >>
> >> The proposal makes MEDIA_ID_VAL(id) non-unique, Is there anything really wrong
> >> with having a single ID space ?
> >
> > My personal preference when I read Hans proposal were to keep the type separate
> > from the ID, but, after seeing the usage of the mdev.entity_id, I guess that
> > this proposal makes things simpler.
>
> I agree with that.
>
> > However, as Sakari doesn't want big numbers for IDs, in order to improve
> > human readability, userspace apps may eventually accept two ways to
> > represent a graph obj that is being controlled:
> >
> > as a global ID, e. g.something like: "0x010000000000001"
> > as something like "entity#1", "link#3", ...
>
> I think this makes sense and shouldn't be a big deal at all.
>
> > The drawback is that we limit the maximum number of types to 8 bits, and
> > the maximum ID range to 24 bits, but I don't think this is an issue.
> >
> >>> Since MEDIA_ID_T_ENTITY == 0 this will be backwards compatible with the
> >>> existing entity IDs.
> >>
> >> There has never been any guarantee that entity IDs would be consecutive, so I
> >> don't think that backward compatibility would be an issue with a single ID
> >> space.
> >
> > Well, if userspace implements the same algorithm for graph traversal as
> > the Kernelspace, e. g. using 1 << entity->id to identify loops, changing
> > this will break userspace.
> >
> > So, I guess we're are bound to keep a consecutive range starting from 1
> > for entities.
>
> We could reserve the range 1-63 for entities, and if we get more entities,
> then it will just use the global ID counter.
>
> Not ideal, but it will make it safe for existing apps.
>
> >
> >>> Note that currently there are no unique pad IDs: adding this requires a
> >>> single change to struct media_pad_desc where one of the reserved fields is
> >>> used to export the unique pad ID. Existing apps can ignore this.
> >>>
> >>> The new media_entity_desc struct looks like this:
> >>>
> >>> struct mc_entity {
> >>> __u32 id;
> >>> __u16 num_pads;
> >>> __u16 reserved[13];
> >>> };
> >
> > I would add flags here too. Not needing to read properties for some trivial
> > things that might be represented via flags sound a good thing to me.
>
> No problem with that. I wasn't sure whether or not a flags field should be added
> since we don't need any flags for entities at the moment.
>
> > Also, I know that we'll keep num_pads at MC, but I would remove this
> > from the API,
>
> Sorry, I'm confused: are you referring to the internal API or the public API?
> I assume the internal API.
I'm talking about both. Of course, we'll need a function to count them
for the legacy API, but let's not add one for the new one.
> > as, the way I'm coding it, the Kernel won't have the
> > numbe anymore, as my idea is to implement as a list at Kernelspace,
> > in order to support dynamic changes.
> >
> > If we ever do such change, that would mean that the kernelspace
> > would need to count the number of pads just due to the a crap API.
> >
> > Also, if userspace implements PAD as a list, it won't need it.
> >
> > So, the "num_pads" on this new API is just an assumption that
> > either Kernel or userspace (or both) would use a certain data type.
> >
> >>>
> >>> I'm going with mc_entity for now. An alternative name might be:
> >>> mediav2_entity or media_entity_v2 (I never liked the _desc suffix and
> >>> media_entity can't be used due to clashed with the internal media_entity
> >>> struct).
> >
> > I think we should use different namespaces for the internal and external
> > representation. One alternative would be to rename the internal namespace
> > (for example, adding "graph_") and keep "media_" for the userspace API.
> >
> > Anyway, I'm comfortable with whatever consistent namespace proposal.
> >
> >>
> >> I'm not too fond of _desc either, mc_ is an interesting alternative, albeit
> >> maybe slightly too generic.
> >
> > Agreed.
> >
> >>
> >>> A third alternative might be to use mc_entity for the internal data structs
> >>> and media_entity for the external.
> >>
> >> That's interesting too.
> >
> > No, mc_entity is a bad prefix. Calling something like:
> > mc_entity_interface
> > is also confusing.
>
> That's not what I meant. I meant using the mc_ prefix instead of the media_
> prefix for internal structs. So media_entity in the public API would map to
> mc_entity in the internal API. Ditto for media_interface/mc_interface,
> media_pad/mc_pad, etc.
Ah! that could work. Yet, I guess we decided to not use mc_ internally
in the first place several years ago because there was already some
namespace conflict.
Ah, yes:
$ git grep -e \\bmc_
...
arch/powerpc/kernel/mce.c: struct machine_check_event *mc_evt;
arch/powerpc/kernel/mce.c: mc_evt = this_cpu_ptr(&mce_event[index]);
arch/powerpc/kernel/mce.c: *mce = *mc_evt;
...
arch/um/drivers/line.h: struct mc_device mc;
...
arch/um/drivers/net_kern.c:static struct mc_device net_mc = {
...
A total of 302 references... Among them, some arch-dependent stuff,
including x86 and powerPC error hanlding stuff (MCE, APEI).
I would avoid it, as the risk of namespace collisions are high.
> Using graph_ instead of mc_ is something I would be OK with as well.
>
> >
> > I would use something like:
> > media_controller_(entity|intf|pad|link|...)
> > or
> > media_graph_(entity|intf|pad|link|...)
> > or
> > media_obj_(entity|intf|pad|link|...)
> >
> >>> As you can see, we didn't have time to discuss the naming for these new
> >>> structs. But for the purposes of this RFC I'm going with mc_ for now.
> >>>
> >>> This struct is much smaller than the original: both name and type will be
> >>> stored as properties of the entity (properties will be the topic of a
> >>> separate upcoming RFC). Pretty much everything else has been removed except
> >>> for num_pads: while even this is not strictly necessary, it was considered
> >>> to be useful to know for applications that use this API.
> >
> > I would keep out those num_foo from the defines. The way I see is that
> > we'll end by removing the need of those counters internally, as, for dynamic
> > allocation/removal, we'll need to convert the internal data struct to lists.
> > I guess userspace apps relying on the new API will also do that too.
> >
> > If we preserve those num_foo and convert internal data struct to lists,
> > the Kernel will need to artificially count the number of stuff just to satisfy
> > the userspace interface, on an assumption that userspace would keep using
> > arrays there instead of lists.
>
> That's not a problem since the kernel will need to flatten the list of pads
> anyway, so adding a counter and filling that in is trivial.
>
> > It is a bad design to write an userspace API that assumes that a particular
> > data struct will be used.
>
> Hmm, OK. Let's try without num_foo and see if that causes any problems. It
> can always be added later.
Ok.
> >> To clarify the design principle here (and please correct me if my
> >> understanding doesn't match yours), even though all fields could be exposed as
> >> properties, the idea was to include core fields deemed of particular interest
> >> for userspace in mc_entity as a kind of "shortcut".
>
> Not really. The reason the entity name is a property is that the name can
> be quite long, which is hard to represent in a fixed struct (that's a problem
> in the current implementation), whereas it works fine as a property. The
> reason the type (i.e. 'sensor', 'flash', 'dma-engine') is a property is
> because there can be multiple types (bad name, type, but that's another story)
> for a single entity. Again, hard to do in a fixed struct, but it works well
> if these are represented as properties.
Makes sense.
> >
> > That doesn't sound the best way to me.
> >
> >>
> >>> The new mc_interface struct is defined as follows:
> >>>
> >>> #define MEDIA_INTF_T_DVB_FE (MEDIA_ENT_T_DEVNODE + 4)
> >>> #define MEDIA_INTF_T_DVB_DEMUX (MEDIA_ENT_T_DEVNODE + 5)
> >>> #define MEDIA_INTF_T_DVB_DVR (MEDIA_ENT_T_DEVNODE + 6)
> >>> #define MEDIA_INTF_T_DVB_CA (MEDIA_ENT_T_DEVNODE + 7)
> >>> #define MEDIA_INTF_T_DVB_NET (MEDIA_ENT_T_DEVNODE + 8)
> >>>
> >>> // TBC: #define MEDIA_INTF_T_NETIF_DVB (MEDIA_ENT_T_DEVNODE + 9)
> >>>
> >>> #define MEDIA_INTF_T_V4L_VIDEO (MEDIA_ENT_T_DEVNODE + 10)
> >>> #define MEDIA_INTF_T_V4L_VBI (MEDIA_ENT_T_DEVNODE + 11)
> >>> #define MEDIA_INTF_T_V4L_RADIO (MEDIA_ENT_T_DEVNODE + 12)
> >>> #define MEDIA_INTF_T_V4L_SUBDEV (MEDIA_ENT_T_DEVNODE + 13)
> >>> #define MEDIA_INTF_T_V4L_SWRADIO (MEDIA_ENT_T_DEVNODE + 14)
> >>>
> >>> #define MEDIA_INTF_T_ALSA_PCM_CAPTURE (MEDIA_ENT_T_DEVNODE + 15)
> >>> #define MEDIA_INTF_T_ALSA_PCM_PLAYBACK (MEDIA_ENT_T_DEVNODE + 16)
> >>> #define MEDIA_INTF_T_ALSA_CONTROL (MEDIA_ENT_T_DEVNODE + 17)
> >>> #define MEDIA_INTF_T_ALSA_COMPRESS (MEDIA_ENT_T_DEVNODE + 18)
> >>> #define MEDIA_INTF_T_ALSA_RAWMIDI (MEDIA_ENT_T_DEVNODE + 19)
> >>> #define MEDIA_INTF_T_ALSA_HWDEP (MEDIA_ENT_T_DEVNODE + 20)
> >>
> >> I wouldn't reuse MEDIA_ENT_T_DEVNODE in the definition of the new values as
> >> that macro becomes deprecated. It needs to be kept for backward-compatibility
> >> of course, but I'd rather define it in terms of the new macros than the other
> >> way around.
> >
> > Agreed.
>
> No problem.
>
> >
> >> That's an implementation detail, it doesn't invalidate the above
> >> list.
> >>
> >>> struct mc_interface {
> >>> __u32 id;
> >>> __u32 intf_type;
> >>
> >> Is there a particular reason to name this field intf_type instead of just type
> >> ?
>
> Hmm, mostly because 'type' is rather overloaded. But in this context calling it
> type would be OK, I guess.
>
> >>
> >>> __u32 flags;
> >>> __u32 num_entity_links;
> >>> __u32 reserved[8];
> >>>
> >>> union {
> >>> /* Node specifications */
> >>> struct {
> >>> __u32 major;
> >>> __u32 minor;
> >>> } devnode;
> >>>
> >>> // TBC: struct {
> >>> // __s32 if_index;
> >>> // } netif;
> >>>
> >>> __u32 raw[16];
> >>> };
> >>> };
> >
> > I'm actually in doubt keeping the union here or representing them via
> > properties. See more below.
> >
> >>>
> >>> The type determines which interface it is, and additional details on how to
> >>> find the interface are in the union. This example also shows how to identify
> >>> network interfaces if we want to expose the network interfaces such as are
> >>> created by DVB.
> >>>
> >>> The num_entity_links field reports how many entities are controlled by this
> >>> interface. While not strictly necessary it is useful for applications to
> >>> know.
> >>>
> >>> There is currently only one flag: MEDIA_INTF_FL_DEFAULT (formerly
> >>> MEDIA_ENT_FL_DEFAULT). This flag tells applications whether this interface
> >>> is a default interface, i.e. the one apps should use by default. Useful
> >>> when there are e.g. multiple video interfaces.
> >>
> >> Still brainstorming here, would it make sense to have a "usage" field instead
> >> ? I'm thinking about cases where different device nodes of the same type would
> >> be considered as default for different use cases. For instance if you have a
> >> TV capture card that can output two video streams, one for preview purpose in
> >> YUV and the other one for capture purpose in MPEG, which one would be the
> >> single default ? The answer will likely vary depending on whether users are
> >> more interested in watching TV on their screen or recording it.
> >
> > An "usage" property makes sense to me.
>
> Definitely a property, not a field.
>
> >
> >>> The new pad struct looks like this:
> >>>
> >>> struct mc_pad {
> >>> __u32 id;
> >>
> >> Is this an ID as in the global pad ID discussed above, or an index local to
> >> the entity ? I believe an index is still very useful at least inside the
> >> kernel, and possibly in userspace as well, as pad IDs won't be stable and
> >> couldn't be used to identify a pad in the context of its entity.
> >
> > This should be a global ID. We'll need dynamic PADs for some usages on
> > DVB, so the ID may not be indexed.
> >
> > Also, identifying a PAD by an index is problematic. One of the problems
> > I had when creating the V4L2 MC representation for TV devices is that
> > some entities have multiple PADs but each PAD is used to carry a different
> > type of signal.
> >
> > For example, almost all tuners have:
> > - one output PAD with the TV IF signal;
> > - one output PAD with the audio IF signal.
> >
> > Also, all analog TV demodulators have:
> > - one output PAD for video;
> > - one output PAD for raw Vertical Blank Interface (VBI) signal.
> >
> > And a few demods also have a separate PAD for decoded VBI data.
> >
> > On both cases, the output PADs are not interchangeable, and they're
> > bound to be linked to different entities or different entity pads.
> >
> > Right now, as the PAD index is driver-specific, I had to create a
> > graph traversal function to connect the links inside the drivers,
> > as the core doesn't know what PADs index has video, audio or VBI.
> >
> > That's problematic, specially when an independent driver like
> > snd-audio-usb needs to connect the audio PAD into the entity
> > that handles audio input samples inside it.
> >
> > I would very much prefer to have an "usage" field at the PADs,
>
> Please don't call it usage. Calling it 'pad_type' or possibly data_type
> will work.
No, "usage" is just due to the lack of a better name (and to avoid long
discussions about what name it should be ;)
pad_type works.
>
> I am a bit worried about this. To my knowledge applications that use the MC
> today are expected to know which pad of an entity does what, and it identifies
> the pad by index.
>
> The new public API should still provide applications with this information in
> one way or another. The pad ID won't work, certainly not in the dynamic case,
> the PAD_TYPE as suggested here will only work as long as there is only one
> pad per type. Suppose there is an entity with two output pads, both for video?
> One might be SDTV, one HDTV. How to tell the difference?
The first one will have "SDTV" as type, the other one "HDTV". So, no problem
with that.
There are, however, some usages where an index is enough. For example, the
hardware demux filter outputs can fully be identified by just an index, and
each index could (ideally) be mapped into a different DMA.
So, I guess we need to offer the flexibility to keep using indexes where
it makes sense.
That's why I proposed a find_pad function that would allow to use an
index:
media_pad *find_pad(struct media_entity entity, enum pad_usage usage, int idx);
or
media_pad *find_pad(struct media_entity entity, char *pad_name);
> One option might be to have a pad_type or data_type for basic type information
> to have generic code that just wants to find VBI/VIDEO/AUDIO streaming pads,
> and a name[32] field that is uniquely identifying the pad and that can be used
> in userspace applications (or drivers for that matter by adding a find_pad_name()).
just a name could be enough:
vbi_pad = find_pad(entity, "vbi");
However, that would require to have a strict namespace if we want the core
to rely on it.
>
> > as this would allow to have core functions to create the links
> > between the entities and to enable/disable such links at streamon
> > and streamoff for most devices, avoiding code duplication.
> >
> > In other words, I would replace the PAD index by something like:
> >
> > media_pad *find_pad(struct media_entity entity, enum pad_usage usage, int idx);
> >
> > vbi_pad = find_pad(entity, PAD_TYPE_VBI, 0);
> > video_pad = find_pad(entity, PAD_TYPE_VIDEO, 0);
> > audio_pad = find_pad(entity, PAD_TYPE_AUDIO, 0);
> > or
> > media_pad *find_pad(struct media_entity entity, char *pad_name);
> >
> > vbi_pad = find_pad(entity, "vbi");
> > video_pad = find_pad(entity, "video");
> > audio_pad = find_pad(entity, "audio");
> >
> > Such calls are driver-independent and can be used inside ALSA, DVB and
> > V4L2 core functions, solving the issue on common use cases.
> >
> >>
> >>> __u32 entity_id;
> >>> __u32 flags;
> >>> __u16 num_links;
> >>> __u16 reserved[9];
> >>> };
> >>>
> >>> And links look like this:
> >>>
> >>> struct mc_link {
> >>> __u32 source_id;
> >>> __u32 sink_id;
> >>> __u32 flags;
> >>> __u32 reserved[5];
> >>> };
> >>>
> >>> The source/sink_id fields refer to pads (for data links) or interface
> >>> (source_id) and entity (sink_id) for the interface/entity relationships.
> >>>
> >>> While not implemented initially this will also make it possible to model
> >>> cases where one entity controls another entity. Whether we actually will
> >>> need this is uncertain, but at least it can be supported should we require
> >>> it.
> >>>
> >>> Finally we will need a way to retrieve all this information. To do this a
> >>> single struct is created:
> >>>
> >>> struct mc_topology {
> >>> __u32 topology_version;
> >>> __u32 num_entities;
> >>> struct mc_entity *entities;
> >>> __u32 num_interfaces;
> >>> struct mc_interface *interfaces;
> >>> __u32 num_pads;
> >>> struct mc_pad *pads;
> >>> __u32 num_links;
> >>> struct mc_link *links;
> >>> __u32 reserved[64];
> >>> };
> >
> > This would work, and I can accept this, but my concern is that we'll have
> > a hard limit on extending the types of objects, given by the "reserved"
> > field.
> >
> > So, an alternative proposal would be to replace it with something like:
> >
> > struct media_graph_object {
> > u32 length;
> >
> > /* Actually, we can merge those two, as proposed by Hans */
> > u32 type;
> > u32 id;
> >
> > void *object_data;
> > } __attribute__ ((packed));
> >
> > struct media_graph_topology {
> > __u32 topology_version;
> > __u32 num_objects;
>
> You will need a __u32 total_length here so userspace knows how much memory
> it should allocate for the objects pointer.
True.
> You don't want userspace to call
> this ioctl three times: once to get the num_objects, then again to get the
> lengths of each objects, and a third time to get everything after allocating
> memory for each object.
>
> Instead return the total_length and let the kernel fill in the object_data
> pointers. These object_data pointers will be awkward for the compat32 code,
> but doable.
>
> So objects is sizeof(struct media_graph_object) * num_objects +
> sum_of(length of each object).
>
> > struct media_graph_object *objects;
>
> You also need a filter field: if I am only interested in interfaces, then
> I don't want to get the other object types.
Yes.
> Note that this will also require that properties are graph_objects (I'm
> not opposed to that) since it should be possible to return the properties
> with the same G_TOPOLOGY call (to keep everything atomic).
Sounds a good idea to me.
>
> > };
> >
> > And have the structs with (some) properties embed on it, like:
> >
> > struct media_graph_entity {
> > struct media_graph_object obj;
>
> This doesn't make sense: struct media_graph_topology would fill in an
> array of struct media_graph_object would point to media objects that
> in turn contain a struct media_graph_object.
Sorry, I didn't get your idea. Could you please give a C code example on
what you're thinking?
>
> I wouldn't embed a struct media_graph_object in these structs, there
> is no point to that. Thinking about this some more you don't need the
> length field either, only an id (this gives you the type, so you know
> whether the object_data pointer is for a media_entity or a media_pad
> or whatever) and the pointer. Strictly speaking you would only need
> the 'type' part of the ID, but I see no reason not to fill in the full
> ID.
>
> > u32 flags;
> > /* ... */
> > char name[64];
> > };
> >
> > Please notice that we don't need to add reserved fields at the structs,
> > as we're now putting the struct length at the media_graph_object.
> >
> > So, if we need to, for example, add a new "foo" inside the
> > media_graph_entity:
> >
> > struct media_graph_entity {
> > struct media_graph_object obj;
> > u32 flags;
> > /* ... */
> > char name[64];
> > u32 foo;
> > };
>
> No, you can't. Say you've compiled the application with a header that includes
> the foo field, and then you run the same application with an older kernel that
> doesn't have the foo field. Any access to foo would give garbage back (or fail).
>
> You really need those reserved fields. The alternative would be to mess around
> with different struct versions, and that's painful.
No. The length will do that. All we need to do is to document that the data
should be zeroed on userspace before filling the data from G_TOPOLOGY.
For example:
let's say that, on Kernel 4.3: this is declared as:
struct media_graph_entity {
struct media_graph_object obj;
u32 flags;
char name[64];
};
And, on Kernel 4.4, the new "foo" field got introduced:
struct media_graph_entity {
struct media_graph_object obj;
u32 flags;
char name[64];
u32 foo;
};
Of course, just like we do with reserved fields, we should ensure that
old apps can live without the "foo" field, and that new apps will
keep working with older Kernels.
Now, we have:
The length of the data on 4.3 is: sizeof(obj) + 68
The length of the data on 4.4 is: sizeof(obj) + 72
As per my proposal, we need to pass the length for the public API
object.
struct media_graph_object {
u32 length;
u32 type;
u32 id;
void *data;
} __attribute__ ((packed));
An application for the G_TOPOLOGY would do something like:
/* p is the pointer for the data returned from Kernel */
struct media_graph_entity
get_an_entity_graph_obj(struct media_graph_obj *obj)
{
struct media_graph_entity *ent;
int len;
len = sizeof(*ent);
if (len > obj->length)
len = obj->length;
ent = calloc(len, 1);
memcpy (ent, obj->data, len);
return ent;
}
There are three cases:
1) both app and Kernel are compiled against the same headers
No problem.
2) app is compiled against v4.4 headers
kernel compiled against v4.3 headers
As the struct were zeroed before memcpy, all entities will have foo = 0.
No problem.
3) app is compiled against v4.3 headers
kernel compiled against v4.4 headers
As the struct won't have the "foo" field, and it will be ignored.
No problem.
So, I don't see any usage to add reserved fields.
>
> >
> > There are some advantages of this approach:
> > - If the size of the entity will change, and obj.length will be bigger.
> > Userspace will allocate more space to store the object, but will be
> > backward compatible;
> > - We can add new object types anytime. If userspace doesn't know the new
> > type, it should simply discard the object and go to the next one. Again,
> > backward compatible.
> >
> > We may eventually add a way for userspace to request only a subset of
> > the graph elements or to add an ioctl or some other sort of event that
> > will report topology changes.
>
> It's an option, but the first 'advantage' doesn't actually work, and I am
> not sure about the second. Yes, it is an advantage but it comes at a price:
> the void pointer. I very much prefer strict typing.
See above.
We can avoid the void pointer with something like:
struct media_graph_object {
u32 length;
u32 type;
u32 id;
} __attribute__ ((packed));
struct media_graph_entity {
struct media_graph_object obj;
char name[64];
} __attribute__ ((packed));
Of course, length is needed, in order to avoid filling it with reserved
stuff.
>
> BTW, my initial proposal had a __u32 reserved[64] field, I'd redo that as
> follows:
>
> struct
> __u32 num_reserved;
> void *ptr_reserved;
> } reserved[32];
>
> This will make 32/64 bit pointer size differences much easier to handle.
>
> And in my opinion, if we end up with so many different object types, then
> we have a much bigger problem and another redesign would be required.
Redesign the API because we add more things is crappy! We should do
something that, ideally, will never require us to redesign it again.
> In general, I prefer strict typing over void pointers, and having to cast
> all the time is something I'd really like to avoid. This API also gives
> you filtering for free (just leave the relevant pointers NULL).
Adding a filter field is not hard, and, as pointed above, we don't need
void pointers.
Regards,
Mauro
>
> Regards,
>
> Hans
>
> >
> >>>
> >>> #define MEDIA_IOC_G_TOPOLOGY _IOWR('|', 0x04, struct mc_topology)
> >>>
> >>> Applications can allocate arrays for each type that they want to retrieve
> >>> (or NULL if they are not interested) and fill in the num_ fields with the
> >>> size of these arrays. Existing DVB/V4L applications that are typically only
> >>> interested in the interfaces created by a device will only need the
> >>> interfaces and can set the others to NULL.
> >>>
> >>> The driver will copy the data to the arrays and set the num_ fields to the
> >>> total number of elements.
> >>>
> >>> So by zeroing the struct and calling the ioctl once the num_ fields will be
> >>> filled in with the number of elements each array requires. The application
> >>> can allocate the memory and call the ioctl again to get the full topology.
> >>> The topology_version will be increased whenever the topology changes.
> >>> Static topologies that cannot change will always return 0 here. Topologies
> >>> that can change will start with version 1 and never return 0 here.
> >>>
> >>> As mentioned before, the property description is absent in this discussion,
> >>> but the idea is to allow properties for each object type except links (at
> >>> least for now) and to report all properties as part of the mc_topology
> >>> ioctl.
> >>>
> >>> A separate RFC will be written by Sakari for this.
> >>>
> >>> Backwards compatibility notes:
> >>>
> >>> - currently only entities are enumerated and that will be unchanged. So
> >>> neither interfaces nor connectors will be seen by the existing API.
> >>> - the v4l-subdev interface will still be reported in the union of the old
> >>> media_entity_desc struct.
> >>> - currently entities representing DMA engines are reported as type
> >>> MEDIA_ENT_T_DEVNODE_V4L. This will change: such entities should be reported
> >>> as having type DMA engine. I think we can make an alias for
> >>> MEDIA_ENT_T_DEVNODE_V4L called MEDIA_ENT_T_DMA_ENGINE. The major/minor in
> >>> the dev struct in the union will point to the video device. - These are the
> >>> only two cases where the dev struct is used, for all other cases it will be
> >>> 0.
> >>> - struct media_pad_desc needs to be extended with a unique pad ID field.
> >>>
> >>> Regards,
> >>>
> >>> Hans
> >>>
> >>> PS: I'm on vacation this week, so don't expect quick replies from me :-)
> >>
>
^ permalink raw reply
* Re: [PATCH RFC 3/3] nvmem: add permission flags in nvmem_config
From: Srinivas Kandagatla @ 2015-08-17 12:31 UTC (permalink / raw)
To: Stefan Wahren
Cc: wxt, linux-api, Rob Herring, p.zabel, Andrew Lunn, sboyd, arnd,
linux-arm-kernel, khilman, maitysanchayan, s.hauer, linux-kernel,
linux-arm-msm, mporter, pantelis.antoniou, devicetree, Mark Brown,
Greg Kroah-Hartman
In-Reply-To: <1634428787.18822.1439623316281.JavaMail.open-xchange@oxbsltgw04.schlund.de>
On 15/08/15 08:21, Stefan Wahren wrote:
> Hi Srinivas,
>
>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> hat am 11. August 2015 um
>> 13:03 geschrieben:
>>
>>
>> This patch adds perm variable to nvmem_config structure which will allow
>> providers to specify the permissions required for the sysfs binary file.
>> This permission is applied on top of root-only access permissions set by
>> the core.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> drivers/nvmem/core.c | 1 +
>> include/linux/nvmem-provider.h | 1 +
>> 2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index 414ed23..7481387 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -312,6 +312,7 @@ struct nvmem_device *nvmem_register(const struct
>> nvmem_config *config)
>> config->read_only;
>> nvmem->bin = bin_attr_template;
>> nvmem->bin.attr.mode = nvmem->read_only ? : (S_IRUSR | S_IWUSR);
>> + nvmem->bin.attr.mode |= config->perm;
>> nvmem->bin.size = nvmem->size;
>> device_initialize(&nvmem->dev);
>>
>> diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
>> index 0b68caf..de9a61f 100644
>> --- a/include/linux/nvmem-provider.h
>> +++ b/include/linux/nvmem-provider.h
>> @@ -23,6 +23,7 @@ struct nvmem_config {
>> const struct nvmem_cell_info *cells;
>> int ncells;
>> bool read_only;
>> + u16 perm; /* visibility in sysfs */
>
> how about "umode_t mode" or "umode_t sysfs_mode"?
>
Yep, "umode_t mode" sounds much better.
I will use it that.
--srini
> Beside that the whole series looks good to me.
>
> Thanks Stefan
>
^ permalink raw reply
* Re: [RFC] Media Controller, the next generation
From: Hans Verkuil @ 2015-08-17 10:33 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Laurent Pinchart
Cc: Linux Media Mailing List, linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150816103709.444d59c1-+RedX5hVuTR+urZeOPWqwQ@public.gmane.org>
On 08/16/2015 03:37 PM, Mauro Carvalho Chehab wrote:
> Em Sat, 15 Aug 2015 21:25:40 +0300
> Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> escreveu:
>
>> Hi Hans,
>>
>> Thank you for the RFC. I believe it's quite good (at least to my eyes).
>> Everything is of course not perfect yet so please see below for some comments.
>
> (added c/c linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org)
>
>>
>> On Monday 03 August 2015 16:07:48 Hans Verkuil wrote:
>>> Hi all,
>>>
>>> During last week's brainstorm meeting in Espoo, Finland, we discussed
>>> how to proceed with the MC API. Trying to apply the existing API to DVB
>>> devices caused a lot of controversy and this meeting was an attempt to
>>> resolve these issues.
>>>
>>> This RFC is the proposal for the public API (NOT the internal API!) we
>>> came up with.
>>>
>>> The main change is that interfaces (such as devices in /dev) get their own
>>> object: struct media_interface. The struct media_entity as is used today
>>> will not refer to interfaces anymore.
>>>
>>> Since interfaces control entities we also want to tell userspace which
>>> entities are controlled by which interfaces. We want to keep things simple
>>> and avoid having to create a new link structure just for this,
>
> I agree with the Hans proposal. Having a single object to represent graph
> links makes easier for both Kernelspace and Userspace to handle them.
Right.
>> During the meeting we discussed the possibility of creating a
>> media_interface_link structure (as drafted in
>> http://linuxtv.org/downloads/presentations/mc_ws_2015/media.h). What would be
>> the advantage of reusing media_link ?
>
> I would reverse the question: what's the disadvantage?
>
> By using the same type, it should be easier to change userspace programs
> to support both interface and data links.
>
> With a single struct, if/when apps need to do graph traversal, they can
> use a single algorithm. It also means a single code to allocate and
> fetch the structs that come from the Kernel.
Also, since all graph objects (interfaces, entities, pads) all use the same
base struct (media_graph_obj) it makes sense that we use the same media_link
struct for linking two graph objects.
>>> so instead a struct media_link just provides two object IDs and a flags
>>> field. The object IDs refer to pads (for data links) or entity/interface IDs
>>> (for associating interfaces with entities).
>>>
>>> To make this work we need unique pad IDs.
>>>
>>> We will need to represent connectors as well. The media_entity struct is
>>> used for that, but it will be marked as a connector since connectors work
>>> slightly different from an entity: if an entity has both input and output
>>> pads, then that means that the entity processes the input data in some way
>>> before passing it on to the output pads. But for a connector the input and
>>> output pads are just hooked up to input and output pins or buses and not
>>> to one another.
>>>
>>> Finally we would like to get all this information atomically in userspace.
>>> Atomicity is desired since some of this information well be dynamic.
>>> Currently the only dynamic thing is enabling/disabling links, but that is
>>> too limiting for devices like FPGAs where the entities can change on the
>>> fly as well.
>>>
>>> Being able to get all the information with a single ioctl will simplify
>>> implementing this atomic behavior, and it is also more efficient than
>>> calling lots of ENUM ioctls.
>>>
>>> We decided to keep the existing structs and ioctls and introduce new
>>> versions of these structs redesigned to cope with the new insights. The
>>> sizes of the reserved fields are suggestions only.
>>>
>>> We had a discussion about the object IDs. Currently we have only entity IDs,
>>> but in the redesign we'll have interface, connector and pad IDs as well.
>>
>> As connectors are entities I don't think they deserve a special mention here.
>
> Hans proposal is to use a different type for connectors. So, I guess it
> makes sense to keep the mention.
There is a difference in how connector entities behave (e.g. input pads are
not connected to output pads, instead they represent pins on the connector) and
how connector entities are used (V4L2 drivers might want to enumerate connector
entities to implement VIDIOC_ENUMINPUT).
So giving them their own distinct type makes sense to me.
>> Regarding pad IDs, is there a reason to give pads IDs in the global ID space
>> other than reusing media_link ?
>
> I actually have another usage for that ;)
>
> I'll comment that below.
>
>>
>>> The idea from the meeting was to use the least significant X bits of the ID
>>> to tell the difference of entity, interface and pad types and to use a flag
>>> for marking connector entities. After thinking some more I would suggest
>>> this scheme instead:
>>>
>>> #define MEDIA_ID_T_ENTITY 0
>>> #define MEDIA_ID_T_CONNECTOR 1
>>> #define MEDIA_ID_T_INTERFACE 2
>>> #define MEDIA_ID_T_PAD 3
>
> My suggestion is actually that we should have one type for every possible
> graph object. So, I would also add a MEDIA_ID_T_LINK, in order to represent
> the links.
>
> Btw, I think that we'll end by needing a MEDIA_ID_T_GROUP too, in order
> to properly replace the group ID, of course assuming that we keep needing
> to have groups of objects.
>
> My idea, if we ever need a MEDIA_ID_T_GROUP is to represent a group as an
> object that contains a list of other objects: entities, interfaces, links,
> pads, etc.
>
> At the userspace interface, the group will pass just the object IDs for
> the objects that belong to such group, plus group name (and maybe flags).
>
> This proposal still need discussions. The most important one is: do we need
> groups on userspace? So, I would not cover groups on this RFC yet, but it
> would be great if the API we're writing could latter be extended to support
> groups.
>
> Internally, at the Kernel, groups make sense, specially when different
> drivers are handling the same MC graph.
>
> For example, let's imagine a pipeline with ALSA, V4L, DVB and DRM entities
> inside. It makes sense for the ALSA snd-audio-usb driver to group
> all ALSA-related objects. The same is true for the DVB core and for
> the V4L2 and DRM drivers/core.
>
> So, that's another reason why I think we should have an unique object
> ID for the links too: it makes easier to group them.
Note that this usage of 'group' is quite different from what the group ID
was intended for: that was to group entities relating to the same HW device
(e.g. group the sensor and flash together so it is clear which flash entity
belongs to which sensor).
My understanding is that today no in-kernel driver uses the group ID, and I
am very skeptical about the whole concept. I think this can be much better
described using properties.
I have no objection against a MEDIA_ID_T_LINK, but this whole group concept
should be ignored for now and internally we should remove the group ID
completely.
The big problem I see in general with a 'group' concept is that it is
awfully vague. Let's just table this for now and revisit when there is a
clear need for it.
>
>>>
>>> #define MEDIA_ID_TYPE(id) ((id) >> 24)
>>> #define MEDIA_ID_VAL(id) ((id) & 0xffffff)
>>> #define MEDIA_ID_CREATE(type, id) (((type) << 24) | id)
>>>
>>> One objection was that the IDs would be unwieldy for users when using e.g.
>>> media-ctl since you'll get IDs >= 0x1000000. However, I think that media-ctl
>>> can easily either deduce the type (i.e. setting up the data path would
>>> always assume type 'pad' or require that the user specifies the type and ID
>>> value (i.e. only the lowest 24 bits).
>>
>> What would be the advantage of that scheme compared to having a type field and
>> an id field ?
>>
>> The proposal makes MEDIA_ID_VAL(id) non-unique, Is there anything really wrong
>> with having a single ID space ?
>
> My personal preference when I read Hans proposal were to keep the type separate
> from the ID, but, after seeing the usage of the mdev.entity_id, I guess that
> this proposal makes things simpler.
I agree with that.
> However, as Sakari doesn't want big numbers for IDs, in order to improve
> human readability, userspace apps may eventually accept two ways to
> represent a graph obj that is being controlled:
>
> as a global ID, e. g.something like: "0x010000000000001"
> as something like "entity#1", "link#3", ...
I think this makes sense and shouldn't be a big deal at all.
> The drawback is that we limit the maximum number of types to 8 bits, and
> the maximum ID range to 24 bits, but I don't think this is an issue.
>
>>> Since MEDIA_ID_T_ENTITY == 0 this will be backwards compatible with the
>>> existing entity IDs.
>>
>> There has never been any guarantee that entity IDs would be consecutive, so I
>> don't think that backward compatibility would be an issue with a single ID
>> space.
>
> Well, if userspace implements the same algorithm for graph traversal as
> the Kernelspace, e. g. using 1 << entity->id to identify loops, changing
> this will break userspace.
>
> So, I guess we're are bound to keep a consecutive range starting from 1
> for entities.
We could reserve the range 1-63 for entities, and if we get more entities,
then it will just use the global ID counter.
Not ideal, but it will make it safe for existing apps.
>
>>> Note that currently there are no unique pad IDs: adding this requires a
>>> single change to struct media_pad_desc where one of the reserved fields is
>>> used to export the unique pad ID. Existing apps can ignore this.
>>>
>>> The new media_entity_desc struct looks like this:
>>>
>>> struct mc_entity {
>>> __u32 id;
>>> __u16 num_pads;
>>> __u16 reserved[13];
>>> };
>
> I would add flags here too. Not needing to read properties for some trivial
> things that might be represented via flags sound a good thing to me.
No problem with that. I wasn't sure whether or not a flags field should be added
since we don't need any flags for entities at the moment.
> Also, I know that we'll keep num_pads at MC, but I would remove this
> from the API,
Sorry, I'm confused: are you referring to the internal API or the public API?
I assume the internal API.
> as, the way I'm coding it, the Kernel won't have the
> numbe anymore, as my idea is to implement as a list at Kernelspace,
> in order to support dynamic changes.
>
> If we ever do such change, that would mean that the kernelspace
> would need to count the number of pads just due to the a crap API.
>
> Also, if userspace implements PAD as a list, it won't need it.
>
> So, the "num_pads" on this new API is just an assumption that
> either Kernel or userspace (or both) would use a certain data type.
>
>>>
>>> I'm going with mc_entity for now. An alternative name might be:
>>> mediav2_entity or media_entity_v2 (I never liked the _desc suffix and
>>> media_entity can't be used due to clashed with the internal media_entity
>>> struct).
>
> I think we should use different namespaces for the internal and external
> representation. One alternative would be to rename the internal namespace
> (for example, adding "graph_") and keep "media_" for the userspace API.
>
> Anyway, I'm comfortable with whatever consistent namespace proposal.
>
>>
>> I'm not too fond of _desc either, mc_ is an interesting alternative, albeit
>> maybe slightly too generic.
>
> Agreed.
>
>>
>>> A third alternative might be to use mc_entity for the internal data structs
>>> and media_entity for the external.
>>
>> That's interesting too.
>
> No, mc_entity is a bad prefix. Calling something like:
> mc_entity_interface
> is also confusing.
That's not what I meant. I meant using the mc_ prefix instead of the media_
prefix for internal structs. So media_entity in the public API would map to
mc_entity in the internal API. Ditto for media_interface/mc_interface,
media_pad/mc_pad, etc.
Using graph_ instead of mc_ is something I would be OK with as well.
>
> I would use something like:
> media_controller_(entity|intf|pad|link|...)
> or
> media_graph_(entity|intf|pad|link|...)
> or
> media_obj_(entity|intf|pad|link|...)
>
>>> As you can see, we didn't have time to discuss the naming for these new
>>> structs. But for the purposes of this RFC I'm going with mc_ for now.
>>>
>>> This struct is much smaller than the original: both name and type will be
>>> stored as properties of the entity (properties will be the topic of a
>>> separate upcoming RFC). Pretty much everything else has been removed except
>>> for num_pads: while even this is not strictly necessary, it was considered
>>> to be useful to know for applications that use this API.
>
> I would keep out those num_foo from the defines. The way I see is that
> we'll end by removing the need of those counters internally, as, for dynamic
> allocation/removal, we'll need to convert the internal data struct to lists.
> I guess userspace apps relying on the new API will also do that too.
>
> If we preserve those num_foo and convert internal data struct to lists,
> the Kernel will need to artificially count the number of stuff just to satisfy
> the userspace interface, on an assumption that userspace would keep using
> arrays there instead of lists.
That's not a problem since the kernel will need to flatten the list of pads
anyway, so adding a counter and filling that in is trivial.
> It is a bad design to write an userspace API that assumes that a particular
> data struct will be used.
Hmm, OK. Let's try without num_foo and see if that causes any problems. It
can always be added later.
>> To clarify the design principle here (and please correct me if my
>> understanding doesn't match yours), even though all fields could be exposed as
>> properties, the idea was to include core fields deemed of particular interest
>> for userspace in mc_entity as a kind of "shortcut".
Not really. The reason the entity name is a property is that the name can
be quite long, which is hard to represent in a fixed struct (that's a problem
in the current implementation), whereas it works fine as a property. The
reason the type (i.e. 'sensor', 'flash', 'dma-engine') is a property is
because there can be multiple types (bad name, type, but that's another story)
for a single entity. Again, hard to do in a fixed struct, but it works well
if these are represented as properties.
>
> That doesn't sound the best way to me.
>
>>
>>> The new mc_interface struct is defined as follows:
>>>
>>> #define MEDIA_INTF_T_DVB_FE (MEDIA_ENT_T_DEVNODE + 4)
>>> #define MEDIA_INTF_T_DVB_DEMUX (MEDIA_ENT_T_DEVNODE + 5)
>>> #define MEDIA_INTF_T_DVB_DVR (MEDIA_ENT_T_DEVNODE + 6)
>>> #define MEDIA_INTF_T_DVB_CA (MEDIA_ENT_T_DEVNODE + 7)
>>> #define MEDIA_INTF_T_DVB_NET (MEDIA_ENT_T_DEVNODE + 8)
>>>
>>> // TBC: #define MEDIA_INTF_T_NETIF_DVB (MEDIA_ENT_T_DEVNODE + 9)
>>>
>>> #define MEDIA_INTF_T_V4L_VIDEO (MEDIA_ENT_T_DEVNODE + 10)
>>> #define MEDIA_INTF_T_V4L_VBI (MEDIA_ENT_T_DEVNODE + 11)
>>> #define MEDIA_INTF_T_V4L_RADIO (MEDIA_ENT_T_DEVNODE + 12)
>>> #define MEDIA_INTF_T_V4L_SUBDEV (MEDIA_ENT_T_DEVNODE + 13)
>>> #define MEDIA_INTF_T_V4L_SWRADIO (MEDIA_ENT_T_DEVNODE + 14)
>>>
>>> #define MEDIA_INTF_T_ALSA_PCM_CAPTURE (MEDIA_ENT_T_DEVNODE + 15)
>>> #define MEDIA_INTF_T_ALSA_PCM_PLAYBACK (MEDIA_ENT_T_DEVNODE + 16)
>>> #define MEDIA_INTF_T_ALSA_CONTROL (MEDIA_ENT_T_DEVNODE + 17)
>>> #define MEDIA_INTF_T_ALSA_COMPRESS (MEDIA_ENT_T_DEVNODE + 18)
>>> #define MEDIA_INTF_T_ALSA_RAWMIDI (MEDIA_ENT_T_DEVNODE + 19)
>>> #define MEDIA_INTF_T_ALSA_HWDEP (MEDIA_ENT_T_DEVNODE + 20)
>>
>> I wouldn't reuse MEDIA_ENT_T_DEVNODE in the definition of the new values as
>> that macro becomes deprecated. It needs to be kept for backward-compatibility
>> of course, but I'd rather define it in terms of the new macros than the other
>> way around.
>
> Agreed.
No problem.
>
>> That's an implementation detail, it doesn't invalidate the above
>> list.
>>
>>> struct mc_interface {
>>> __u32 id;
>>> __u32 intf_type;
>>
>> Is there a particular reason to name this field intf_type instead of just type
>> ?
Hmm, mostly because 'type' is rather overloaded. But in this context calling it
type would be OK, I guess.
>>
>>> __u32 flags;
>>> __u32 num_entity_links;
>>> __u32 reserved[8];
>>>
>>> union {
>>> /* Node specifications */
>>> struct {
>>> __u32 major;
>>> __u32 minor;
>>> } devnode;
>>>
>>> // TBC: struct {
>>> // __s32 if_index;
>>> // } netif;
>>>
>>> __u32 raw[16];
>>> };
>>> };
>
> I'm actually in doubt keeping the union here or representing them via
> properties. See more below.
>
>>>
>>> The type determines which interface it is, and additional details on how to
>>> find the interface are in the union. This example also shows how to identify
>>> network interfaces if we want to expose the network interfaces such as are
>>> created by DVB.
>>>
>>> The num_entity_links field reports how many entities are controlled by this
>>> interface. While not strictly necessary it is useful for applications to
>>> know.
>>>
>>> There is currently only one flag: MEDIA_INTF_FL_DEFAULT (formerly
>>> MEDIA_ENT_FL_DEFAULT). This flag tells applications whether this interface
>>> is a default interface, i.e. the one apps should use by default. Useful
>>> when there are e.g. multiple video interfaces.
>>
>> Still brainstorming here, would it make sense to have a "usage" field instead
>> ? I'm thinking about cases where different device nodes of the same type would
>> be considered as default for different use cases. For instance if you have a
>> TV capture card that can output two video streams, one for preview purpose in
>> YUV and the other one for capture purpose in MPEG, which one would be the
>> single default ? The answer will likely vary depending on whether users are
>> more interested in watching TV on their screen or recording it.
>
> An "usage" property makes sense to me.
Definitely a property, not a field.
>
>>> The new pad struct looks like this:
>>>
>>> struct mc_pad {
>>> __u32 id;
>>
>> Is this an ID as in the global pad ID discussed above, or an index local to
>> the entity ? I believe an index is still very useful at least inside the
>> kernel, and possibly in userspace as well, as pad IDs won't be stable and
>> couldn't be used to identify a pad in the context of its entity.
>
> This should be a global ID. We'll need dynamic PADs for some usages on
> DVB, so the ID may not be indexed.
>
> Also, identifying a PAD by an index is problematic. One of the problems
> I had when creating the V4L2 MC representation for TV devices is that
> some entities have multiple PADs but each PAD is used to carry a different
> type of signal.
>
> For example, almost all tuners have:
> - one output PAD with the TV IF signal;
> - one output PAD with the audio IF signal.
>
> Also, all analog TV demodulators have:
> - one output PAD for video;
> - one output PAD for raw Vertical Blank Interface (VBI) signal.
>
> And a few demods also have a separate PAD for decoded VBI data.
>
> On both cases, the output PADs are not interchangeable, and they're
> bound to be linked to different entities or different entity pads.
>
> Right now, as the PAD index is driver-specific, I had to create a
> graph traversal function to connect the links inside the drivers,
> as the core doesn't know what PADs index has video, audio or VBI.
>
> That's problematic, specially when an independent driver like
> snd-audio-usb needs to connect the audio PAD into the entity
> that handles audio input samples inside it.
>
> I would very much prefer to have an "usage" field at the PADs,
Please don't call it usage. Calling it 'pad_type' or possibly data_type
will work.
I am a bit worried about this. To my knowledge applications that use the MC
today are expected to know which pad of an entity does what, and it identifies
the pad by index.
The new public API should still provide applications with this information in
one way or another. The pad ID won't work, certainly not in the dynamic case,
the PAD_TYPE as suggested here will only work as long as there is only one
pad per type. Suppose there is an entity with two output pads, both for video?
One might be SDTV, one HDTV. How to tell the difference?
One option might be to have a pad_type or data_type for basic type information
to have generic code that just wants to find VBI/VIDEO/AUDIO streaming pads,
and a name[32] field that is uniquely identifying the pad and that can be used
in userspace applications (or drivers for that matter by adding a find_pad_name()).
> as this would allow to have core functions to create the links
> between the entities and to enable/disable such links at streamon
> and streamoff for most devices, avoiding code duplication.
>
> In other words, I would replace the PAD index by something like:
>
> media_pad *find_pad(struct media_entity entity, enum pad_usage usage, int idx);
>
> vbi_pad = find_pad(entity, PAD_TYPE_VBI, 0);
> video_pad = find_pad(entity, PAD_TYPE_VIDEO, 0);
> audio_pad = find_pad(entity, PAD_TYPE_AUDIO, 0);
> or
> media_pad *find_pad(struct media_entity entity, char *pad_name);
>
> vbi_pad = find_pad(entity, "vbi");
> video_pad = find_pad(entity, "video");
> audio_pad = find_pad(entity, "audio");
>
> Such calls are driver-independent and can be used inside ALSA, DVB and
> V4L2 core functions, solving the issue on common use cases.
>
>>
>>> __u32 entity_id;
>>> __u32 flags;
>>> __u16 num_links;
>>> __u16 reserved[9];
>>> };
>>>
>>> And links look like this:
>>>
>>> struct mc_link {
>>> __u32 source_id;
>>> __u32 sink_id;
>>> __u32 flags;
>>> __u32 reserved[5];
>>> };
>>>
>>> The source/sink_id fields refer to pads (for data links) or interface
>>> (source_id) and entity (sink_id) for the interface/entity relationships.
>>>
>>> While not implemented initially this will also make it possible to model
>>> cases where one entity controls another entity. Whether we actually will
>>> need this is uncertain, but at least it can be supported should we require
>>> it.
>>>
>>> Finally we will need a way to retrieve all this information. To do this a
>>> single struct is created:
>>>
>>> struct mc_topology {
>>> __u32 topology_version;
>>> __u32 num_entities;
>>> struct mc_entity *entities;
>>> __u32 num_interfaces;
>>> struct mc_interface *interfaces;
>>> __u32 num_pads;
>>> struct mc_pad *pads;
>>> __u32 num_links;
>>> struct mc_link *links;
>>> __u32 reserved[64];
>>> };
>
> This would work, and I can accept this, but my concern is that we'll have
> a hard limit on extending the types of objects, given by the "reserved"
> field.
>
> So, an alternative proposal would be to replace it with something like:
>
> struct media_graph_object {
> u32 length;
>
> /* Actually, we can merge those two, as proposed by Hans */
> u32 type;
> u32 id;
>
> void *object_data;
> } __attribute__ ((packed));
>
> struct media_graph_topology {
> __u32 topology_version;
> __u32 num_objects;
You will need a __u32 total_length here so userspace knows how much memory
it should allocate for the objects pointer. You don't want userspace to call
this ioctl three times: once to get the num_objects, then again to get the
lengths of each objects, and a third time to get everything after allocating
memory for each object.
Instead return the total_length and let the kernel fill in the object_data
pointers. These object_data pointers will be awkward for the compat32 code,
but doable.
So objects is sizeof(struct media_graph_object) * num_objects +
sum_of(length of each object).
> struct media_graph_object *objects;
You also need a filter field: if I am only interested in interfaces, then
I don't want to get the other object types.
Note that this will also require that properties are graph_objects (I'm
not opposed to that) since it should be possible to return the properties
with the same G_TOPOLOGY call (to keep everything atomic).
> };
>
> And have the structs with (some) properties embed on it, like:
>
> struct media_graph_entity {
> struct media_graph_object obj;
This doesn't make sense: struct media_graph_topology would fill in an
array of struct media_graph_object would point to media objects that
in turn contain a struct media_graph_object.
I wouldn't embed a struct media_graph_object in these structs, there
is no point to that. Thinking about this some more you don't need the
length field either, only an id (this gives you the type, so you know
whether the object_data pointer is for a media_entity or a media_pad
or whatever) and the pointer. Strictly speaking you would only need
the 'type' part of the ID, but I see no reason not to fill in the full
ID.
> u32 flags;
> /* ... */
> char name[64];
> };
>
> Please notice that we don't need to add reserved fields at the structs,
> as we're now putting the struct length at the media_graph_object.
>
> So, if we need to, for example, add a new "foo" inside the
> media_graph_entity:
>
> struct media_graph_entity {
> struct media_graph_object obj;
> u32 flags;
> /* ... */
> char name[64];
> u32 foo;
> };
No, you can't. Say you've compiled the application with a header that includes
the foo field, and then you run the same application with an older kernel that
doesn't have the foo field. Any access to foo would give garbage back (or fail).
You really need those reserved fields. The alternative would be to mess around
with different struct versions, and that's painful.
>
> There are some advantages of this approach:
> - If the size of the entity will change, and obj.length will be bigger.
> Userspace will allocate more space to store the object, but will be
> backward compatible;
> - We can add new object types anytime. If userspace doesn't know the new
> type, it should simply discard the object and go to the next one. Again,
> backward compatible.
>
> We may eventually add a way for userspace to request only a subset of
> the graph elements or to add an ioctl or some other sort of event that
> will report topology changes.
It's an option, but the first 'advantage' doesn't actually work, and I am
not sure about the second. Yes, it is an advantage but it comes at a price:
the void pointer. I very much prefer strict typing.
BTW, my initial proposal had a __u32 reserved[64] field, I'd redo that as
follows:
struct
__u32 num_reserved;
void *ptr_reserved;
} reserved[32];
This will make 32/64 bit pointer size differences much easier to handle.
And in my opinion, if we end up with so many different object types, then
we have a much bigger problem and another redesign would be required.
In general, I prefer strict typing over void pointers, and having to cast
all the time is something I'd really like to avoid. This API also gives
you filtering for free (just leave the relevant pointers NULL).
Regards,
Hans
>
>>>
>>> #define MEDIA_IOC_G_TOPOLOGY _IOWR('|', 0x04, struct mc_topology)
>>>
>>> Applications can allocate arrays for each type that they want to retrieve
>>> (or NULL if they are not interested) and fill in the num_ fields with the
>>> size of these arrays. Existing DVB/V4L applications that are typically only
>>> interested in the interfaces created by a device will only need the
>>> interfaces and can set the others to NULL.
>>>
>>> The driver will copy the data to the arrays and set the num_ fields to the
>>> total number of elements.
>>>
>>> So by zeroing the struct and calling the ioctl once the num_ fields will be
>>> filled in with the number of elements each array requires. The application
>>> can allocate the memory and call the ioctl again to get the full topology.
>>> The topology_version will be increased whenever the topology changes.
>>> Static topologies that cannot change will always return 0 here. Topologies
>>> that can change will start with version 1 and never return 0 here.
>>>
>>> As mentioned before, the property description is absent in this discussion,
>>> but the idea is to allow properties for each object type except links (at
>>> least for now) and to report all properties as part of the mc_topology
>>> ioctl.
>>>
>>> A separate RFC will be written by Sakari for this.
>>>
>>> Backwards compatibility notes:
>>>
>>> - currently only entities are enumerated and that will be unchanged. So
>>> neither interfaces nor connectors will be seen by the existing API.
>>> - the v4l-subdev interface will still be reported in the union of the old
>>> media_entity_desc struct.
>>> - currently entities representing DMA engines are reported as type
>>> MEDIA_ENT_T_DEVNODE_V4L. This will change: such entities should be reported
>>> as having type DMA engine. I think we can make an alias for
>>> MEDIA_ENT_T_DEVNODE_V4L called MEDIA_ENT_T_DMA_ENGINE. The major/minor in
>>> the dev struct in the union will point to the video device. - These are the
>>> only two cases where the dev struct is used, for all other cases it will be
>>> 0.
>>> - struct media_pad_desc needs to be extended with a unique pad ID field.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>> PS: I'm on vacation this week, so don't expect quick replies from me :-)
>>
^ permalink raw reply
* [PATCH v10 5/6] block: loop: introduce ioctl command of LOOP_SET_DIRECT_IO
From: Ming Lei @ 2015-08-17 2:31 UTC (permalink / raw)
To: Jens Axboe, linux-kernel, Dave Kleikamp
Cc: Zach Brown, Christoph Hellwig, Maxim Patlasov, Andrew Morton,
Alexander Viro, Tejun Heo, Dave Chinner, Ming Lei, linux-api
In-Reply-To: <1439778711-9621-1-git-send-email-ming.lei@canonical.com>
If loop block is mounted via 'mount -o loop', it isn't easy
to pass file descriptor opened as O_DIRECT, so this patch
introduces a new command to support direct IO for this case.
Cc: linux-api@vger.kernel.org
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
drivers/block/loop.c | 19 +++++++++++++++++++
include/uapi/linux/loop.h | 1 +
2 files changed, 20 insertions(+)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 06cad7e..75db3b9 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1228,6 +1228,20 @@ static int loop_set_capacity(struct loop_device *lo, struct block_device *bdev)
return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit);
}
+static int loop_set_dio(struct loop_device *lo, unsigned long arg)
+{
+ int error = -ENXIO;
+ if (lo->lo_state != Lo_bound)
+ goto out;
+
+ __loop_update_dio(lo, !!arg);
+ if (lo->use_dio == !!arg)
+ return 0;
+ error = -EINVAL;
+ out:
+ return error;
+}
+
static int lo_ioctl(struct block_device *bdev, fmode_t mode,
unsigned int cmd, unsigned long arg)
{
@@ -1271,6 +1285,11 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
err = loop_set_capacity(lo, bdev);
break;
+ case LOOP_SET_DIRECT_IO:
+ err = -EPERM;
+ if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
+ err = loop_set_dio(lo, arg);
+ break;
default:
err = lo->ioctl ? lo->ioctl(lo, cmd, arg) : -EINVAL;
}
diff --git a/include/uapi/linux/loop.h b/include/uapi/linux/loop.h
index 949851c..c8125ec 100644
--- a/include/uapi/linux/loop.h
+++ b/include/uapi/linux/loop.h
@@ -87,6 +87,7 @@ struct loop_info64 {
#define LOOP_GET_STATUS64 0x4C05
#define LOOP_CHANGE_FD 0x4C06
#define LOOP_SET_CAPACITY 0x4C07
+#define LOOP_SET_DIRECT_IO 0x4C08
/* /dev/loop-control interface */
#define LOOP_CTL_ADD 0x4C80
--
1.9.1
^ permalink raw reply related
* [PATCH v10 4/6] block: loop: prepare for supporing direct IO
From: Ming Lei @ 2015-08-17 2:31 UTC (permalink / raw)
To: Jens Axboe, linux-kernel, Dave Kleikamp
Cc: Zach Brown, Christoph Hellwig, Maxim Patlasov, Andrew Morton,
Alexander Viro, Tejun Heo, Dave Chinner, Ming Lei, linux-api
In-Reply-To: <1439778711-9621-1-git-send-email-ming.lei@canonical.com>
This patches provides one interface for enabling direct IO
from user space:
- userspace(such as losetup) can pass 'file' which is
opened/fcntl as O_DIRECT
Also __loop_update_dio() is introduced to check if direct I/O
can be used on current loop setting.
The last big change is to introduce LO_FLAGS_DIRECT_IO flag
for userspace to know if direct IO is used to access backing
file.
Cc: linux-api@vger.kernel.org
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
drivers/block/loop.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++
drivers/block/loop.h | 1 +
include/uapi/linux/loop.h | 1 +
3 files changed, 79 insertions(+)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 1875aad..06cad7e 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -164,6 +164,62 @@ static loff_t get_loop_size(struct loop_device *lo, struct file *file)
return get_size(lo->lo_offset, lo->lo_sizelimit, file);
}
+static void __loop_update_dio(struct loop_device *lo, bool dio)
+{
+ struct file *file = lo->lo_backing_file;
+ struct address_space *mapping = file->f_mapping;
+ struct inode *inode = mapping->host;
+ unsigned short sb_bsize = 0;
+ unsigned dio_align = 0;
+ bool use_dio;
+
+ if (inode->i_sb->s_bdev) {
+ sb_bsize = bdev_logical_block_size(inode->i_sb->s_bdev);
+ dio_align = sb_bsize - 1;
+ }
+
+ /*
+ * We support direct I/O only if lo_offset is aligned with the
+ * logical I/O size of backing device, and the logical block
+ * size of loop is bigger than the backing device's and the loop
+ * needn't transform transfer.
+ *
+ * TODO: the above condition may be loosed in the future, and
+ * direct I/O may be switched runtime at that time because most
+ * of requests in sane appplications should be PAGE_SIZE algined
+ */
+ if (dio) {
+ if (queue_logical_block_size(lo->lo_queue) >= sb_bsize &&
+ !(lo->lo_offset & dio_align) &&
+ mapping->a_ops->direct_IO &&
+ !lo->transfer)
+ use_dio = true;
+ else
+ use_dio = false;
+ } else {
+ use_dio = false;
+ }
+
+ if (lo->use_dio == use_dio)
+ return;
+
+ /* flush dirty pages before changing direct IO */
+ vfs_fsync(file, 0);
+
+ /*
+ * The flag of LO_FLAGS_DIRECT_IO is handled similarly with
+ * LO_FLAGS_READ_ONLY, both are set from kernel, and losetup
+ * will get updated by ioctl(LOOP_GET_STATUS)
+ */
+ blk_mq_freeze_queue(lo->lo_queue);
+ lo->use_dio = use_dio;
+ if (use_dio)
+ lo->lo_flags |= LO_FLAGS_DIRECT_IO;
+ else
+ lo->lo_flags &= ~LO_FLAGS_DIRECT_IO;
+ blk_mq_unfreeze_queue(lo->lo_queue);
+}
+
static int
figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
{
@@ -421,6 +477,12 @@ struct switch_request {
struct completion wait;
};
+static inline void loop_update_dio(struct loop_device *lo)
+{
+ __loop_update_dio(lo, io_is_direct(lo->lo_backing_file) |
+ lo->use_dio);
+}
+
/*
* Do the actual switch; called from the BIO completion routine
*/
@@ -441,6 +503,7 @@ static void do_loop_switch(struct loop_device *lo, struct switch_request *p)
mapping->host->i_bdev->bd_block_size : PAGE_SIZE;
lo->old_gfp_mask = mapping_gfp_mask(mapping);
mapping_set_gfp_mask(mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));
+ loop_update_dio(lo);
}
/*
@@ -627,11 +690,19 @@ static ssize_t loop_attr_partscan_show(struct loop_device *lo, char *buf)
return sprintf(buf, "%s\n", partscan ? "1" : "0");
}
+static ssize_t loop_attr_dio_show(struct loop_device *lo, char *buf)
+{
+ int dio = (lo->lo_flags & LO_FLAGS_DIRECT_IO);
+
+ return sprintf(buf, "%s\n", dio ? "1" : "0");
+}
+
LOOP_ATTR_RO(backing_file);
LOOP_ATTR_RO(offset);
LOOP_ATTR_RO(sizelimit);
LOOP_ATTR_RO(autoclear);
LOOP_ATTR_RO(partscan);
+LOOP_ATTR_RO(dio);
static struct attribute *loop_attrs[] = {
&loop_attr_backing_file.attr,
@@ -639,6 +710,7 @@ static struct attribute *loop_attrs[] = {
&loop_attr_sizelimit.attr,
&loop_attr_autoclear.attr,
&loop_attr_partscan.attr,
+ &loop_attr_dio.attr,
NULL,
};
@@ -770,6 +842,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
set_device_ro(bdev, (lo_flags & LO_FLAGS_READ_ONLY) != 0);
+ lo->use_dio = false;
lo->lo_blocksize = lo_blocksize;
lo->lo_device = bdev;
lo->lo_flags = lo_flags;
@@ -783,6 +856,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
if (!(lo_flags & LO_FLAGS_READ_ONLY) && file->f_op->fsync)
blk_queue_flush(lo->lo_queue, REQ_FLUSH);
+ loop_update_dio(lo);
set_capacity(lo->lo_disk, size);
bd_set_size(bdev, size << 9);
loop_sysfs_init(lo);
@@ -1001,6 +1075,9 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
lo->lo_key_owner = uid;
}
+ /* update dio if lo_offset or transfer is changed */
+ __loop_update_dio(lo, lo->use_dio);
+
return 0;
}
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index b6c7d21..d1de221 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -58,6 +58,7 @@ struct loop_device {
struct mutex lo_ctl_mutex;
struct kthread_worker worker;
struct task_struct *worker_task;
+ bool use_dio;
struct request_queue *lo_queue;
struct blk_mq_tag_set tag_set;
diff --git a/include/uapi/linux/loop.h b/include/uapi/linux/loop.h
index e0cecd2..949851c 100644
--- a/include/uapi/linux/loop.h
+++ b/include/uapi/linux/loop.h
@@ -21,6 +21,7 @@ enum {
LO_FLAGS_READ_ONLY = 1,
LO_FLAGS_AUTOCLEAR = 4,
LO_FLAGS_PARTSCAN = 8,
+ LO_FLAGS_DIRECT_IO = 16,
};
#include <asm/posix_types.h> /* for __kernel_old_dev_t */
--
1.9.1
^ permalink raw reply related
* Re: [RFC] Media Controller, the next generation
From: Mauro Carvalho Chehab @ 2015-08-16 13:37 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Hans Verkuil, Linux Media Mailing List, linux-api
In-Reply-To: <1553545.72YdKh14yc@avalon>
Em Sat, 15 Aug 2015 21:25:40 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> Hi Hans,
>
> Thank you for the RFC. I believe it's quite good (at least to my eyes).
> Everything is of course not perfect yet so please see below for some comments.
(added c/c linux-api@vger.kernel.org)
>
> On Monday 03 August 2015 16:07:48 Hans Verkuil wrote:
> > Hi all,
> >
> > During last week's brainstorm meeting in Espoo, Finland, we discussed
> > how to proceed with the MC API. Trying to apply the existing API to DVB
> > devices caused a lot of controversy and this meeting was an attempt to
> > resolve these issues.
> >
> > This RFC is the proposal for the public API (NOT the internal API!) we
> > came up with.
> >
> > The main change is that interfaces (such as devices in /dev) get their own
> > object: struct media_interface. The struct media_entity as is used today
> > will not refer to interfaces anymore.
> >
> > Since interfaces control entities we also want to tell userspace which
> > entities are controlled by which interfaces. We want to keep things simple
> > and avoid having to create a new link structure just for this,
I agree with the Hans proposal. Having a single object to represent graph
links makes easier for both Kernelspace and Userspace to handle them.
> During the meeting we discussed the possibility of creating a
> media_interface_link structure (as drafted in
> http://linuxtv.org/downloads/presentations/mc_ws_2015/media.h). What would be
> the advantage of reusing media_link ?
I would reverse the question: what's the disadvantage?
By using the same type, it should be easier to change userspace programs
to support both interface and data links.
With a single struct, if/when apps need to do graph traversal, they can
use a single algorithm. It also means a single code to allocate and
fetch the structs that come from the Kernel.
> > so instead a struct media_link just provides two object IDs and a flags
> > field. The object IDs refer to pads (for data links) or entity/interface IDs
> > (for associating interfaces with entities).
> >
> > To make this work we need unique pad IDs.
> >
> > We will need to represent connectors as well. The media_entity struct is
> > used for that, but it will be marked as a connector since connectors work
> > slightly different from an entity: if an entity has both input and output
> > pads, then that means that the entity processes the input data in some way
> > before passing it on to the output pads. But for a connector the input and
> > output pads are just hooked up to input and output pins or buses and not
> > to one another.
> >
> > Finally we would like to get all this information atomically in userspace.
> > Atomicity is desired since some of this information well be dynamic.
> > Currently the only dynamic thing is enabling/disabling links, but that is
> > too limiting for devices like FPGAs where the entities can change on the
> > fly as well.
> >
> > Being able to get all the information with a single ioctl will simplify
> > implementing this atomic behavior, and it is also more efficient than
> > calling lots of ENUM ioctls.
> >
> > We decided to keep the existing structs and ioctls and introduce new
> > versions of these structs redesigned to cope with the new insights. The
> > sizes of the reserved fields are suggestions only.
> >
> > We had a discussion about the object IDs. Currently we have only entity IDs,
> > but in the redesign we'll have interface, connector and pad IDs as well.
>
> As connectors are entities I don't think they deserve a special mention here.
Hans proposal is to use a different type for connectors. So, I guess it
makes sense to keep the mention.
> Regarding pad IDs, is there a reason to give pads IDs in the global ID space
> other than reusing media_link ?
I actually have another usage for that ;)
I'll comment that below.
>
> > The idea from the meeting was to use the least significant X bits of the ID
> > to tell the difference of entity, interface and pad types and to use a flag
> > for marking connector entities. After thinking some more I would suggest
> > this scheme instead:
> >
> > #define MEDIA_ID_T_ENTITY 0
> > #define MEDIA_ID_T_CONNECTOR 1
> > #define MEDIA_ID_T_INTERFACE 2
> > #define MEDIA_ID_T_PAD 3
My suggestion is actually that we should have one type for every possible
graph object. So, I would also add a MEDIA_ID_T_LINK, in order to represent
the links.
Btw, I think that we'll end by needing a MEDIA_ID_T_GROUP too, in order
to properly replace the group ID, of course assuming that we keep needing
to have groups of objects.
My idea, if we ever need a MEDIA_ID_T_GROUP is to represent a group as an
object that contains a list of other objects: entities, interfaces, links,
pads, etc.
At the userspace interface, the group will pass just the object IDs for
the objects that belong to such group, plus group name (and maybe flags).
This proposal still need discussions. The most important one is: do we need
groups on userspace? So, I would not cover groups on this RFC yet, but it
would be great if the API we're writing could latter be extended to support
groups.
Internally, at the Kernel, groups make sense, specially when different
drivers are handling the same MC graph.
For example, let's imagine a pipeline with ALSA, V4L, DVB and DRM entities
inside. It makes sense for the ALSA snd-audio-usb driver to group
all ALSA-related objects. The same is true for the DVB core and for
the V4L2 and DRM drivers/core.
So, that's another reason why I think we should have an unique object
ID for the links too: it makes easier to group them.
> >
> > #define MEDIA_ID_TYPE(id) ((id) >> 24)
> > #define MEDIA_ID_VAL(id) ((id) & 0xffffff)
> > #define MEDIA_ID_CREATE(type, id) (((type) << 24) | id)
> >
> > One objection was that the IDs would be unwieldy for users when using e.g.
> > media-ctl since you'll get IDs >= 0x1000000. However, I think that media-ctl
> > can easily either deduce the type (i.e. setting up the data path would
> > always assume type 'pad' or require that the user specifies the type and ID
> > value (i.e. only the lowest 24 bits).
>
> What would be the advantage of that scheme compared to having a type field and
> an id field ?
>
> The proposal makes MEDIA_ID_VAL(id) non-unique, Is there anything really wrong
> with having a single ID space ?
My personal preference when I read Hans proposal were to keep the type separate
from the ID, but, after seeing the usage of the mdev.entity_id, I guess that
this proposal makes things simpler.
However, as Sakari doesn't want big numbers for IDs, in order to improve
human readability, userspace apps may eventually accept two ways to
represent a graph obj that is being controlled:
as a global ID, e. g.something like: "0x010000000000001"
as something like "entity#1", "link#3", ...
The drawback is that we limit the maximum number of types to 8 bits, and
the maximum ID range to 24 bits, but I don't think this is an issue.
> > Since MEDIA_ID_T_ENTITY == 0 this will be backwards compatible with the
> > existing entity IDs.
>
> There has never been any guarantee that entity IDs would be consecutive, so I
> don't think that backward compatibility would be an issue with a single ID
> space.
Well, if userspace implements the same algorithm for graph traversal as
the Kernelspace, e. g. using 1 << entity->id to identify loops, changing
this will break userspace.
So, I guess we're are bound to keep a consecutive range starting from 1
for entities.
> > Note that currently there are no unique pad IDs: adding this requires a
> > single change to struct media_pad_desc where one of the reserved fields is
> > used to export the unique pad ID. Existing apps can ignore this.
> >
> > The new media_entity_desc struct looks like this:
> >
> > struct mc_entity {
> > __u32 id;
> > __u16 num_pads;
> > __u16 reserved[13];
> > };
I would add flags here too. Not needing to read properties for some trivial
things that might be represented via flags sound a good thing to me.
Also, I know that we'll keep num_pads at MC, but I would remove this
from the API, as, the way I'm coding it, the Kernel won't have the
numbe anymore, as my idea is to implement as a list at Kernelspace,
in order to support dynamic changes.
If we ever do such change, that would mean that the kernelspace
would need to count the number of pads just due to the a crap API.
Also, if userspace implements PAD as a list, it won't need it.
So, the "num_pads" on this new API is just an assumption that
either Kernel or userspace (or both) would use a certain data type.
> >
> > I'm going with mc_entity for now. An alternative name might be:
> > mediav2_entity or media_entity_v2 (I never liked the _desc suffix and
> > media_entity can't be used due to clashed with the internal media_entity
> > struct).
I think we should use different namespaces for the internal and external
representation. One alternative would be to rename the internal namespace
(for example, adding "graph_") and keep "media_" for the userspace API.
Anyway, I'm comfortable with whatever consistent namespace proposal.
>
> I'm not too fond of _desc either, mc_ is an interesting alternative, albeit
> maybe slightly too generic.
Agreed.
>
> > A third alternative might be to use mc_entity for the internal data structs
> > and media_entity for the external.
>
> That's interesting too.
No, mc_entity is a bad prefix. Calling something like:
mc_entity_interface
is also confusing.
I would use something like:
media_controller_(entity|intf|pad|link|...)
or
media_graph_(entity|intf|pad|link|...)
or
media_obj_(entity|intf|pad|link|...)
> > As you can see, we didn't have time to discuss the naming for these new
> > structs. But for the purposes of this RFC I'm going with mc_ for now.
> >
> > This struct is much smaller than the original: both name and type will be
> > stored as properties of the entity (properties will be the topic of a
> > separate upcoming RFC). Pretty much everything else has been removed except
> > for num_pads: while even this is not strictly necessary, it was considered
> > to be useful to know for applications that use this API.
I would keep out those num_foo from the defines. The way I see is that
we'll end by removing the need of those counters internally, as, for dynamic
allocation/removal, we'll need to convert the internal data struct to lists.
I guess userspace apps relying on the new API will also do that too.
If we preserve those num_foo and convert internal data struct to lists,
the Kernel will need to artificially count the number of stuff just to satisfy
the userspace interface, on an assumption that userspace would keep using
arrays there instead of lists.
It is a bad design to write an userspace API that assumes that a particular
data struct will be used.
> To clarify the design principle here (and please correct me if my
> understanding doesn't match yours), even though all fields could be exposed as
> properties, the idea was to include core fields deemed of particular interest
> for userspace in mc_entity as a kind of "shortcut".
That doesn't sound the best way to me.
>
> > The new mc_interface struct is defined as follows:
> >
> > #define MEDIA_INTF_T_DVB_FE (MEDIA_ENT_T_DEVNODE + 4)
> > #define MEDIA_INTF_T_DVB_DEMUX (MEDIA_ENT_T_DEVNODE + 5)
> > #define MEDIA_INTF_T_DVB_DVR (MEDIA_ENT_T_DEVNODE + 6)
> > #define MEDIA_INTF_T_DVB_CA (MEDIA_ENT_T_DEVNODE + 7)
> > #define MEDIA_INTF_T_DVB_NET (MEDIA_ENT_T_DEVNODE + 8)
> >
> > // TBC: #define MEDIA_INTF_T_NETIF_DVB (MEDIA_ENT_T_DEVNODE + 9)
> >
> > #define MEDIA_INTF_T_V4L_VIDEO (MEDIA_ENT_T_DEVNODE + 10)
> > #define MEDIA_INTF_T_V4L_VBI (MEDIA_ENT_T_DEVNODE + 11)
> > #define MEDIA_INTF_T_V4L_RADIO (MEDIA_ENT_T_DEVNODE + 12)
> > #define MEDIA_INTF_T_V4L_SUBDEV (MEDIA_ENT_T_DEVNODE + 13)
> > #define MEDIA_INTF_T_V4L_SWRADIO (MEDIA_ENT_T_DEVNODE + 14)
> >
> > #define MEDIA_INTF_T_ALSA_PCM_CAPTURE (MEDIA_ENT_T_DEVNODE + 15)
> > #define MEDIA_INTF_T_ALSA_PCM_PLAYBACK (MEDIA_ENT_T_DEVNODE + 16)
> > #define MEDIA_INTF_T_ALSA_CONTROL (MEDIA_ENT_T_DEVNODE + 17)
> > #define MEDIA_INTF_T_ALSA_COMPRESS (MEDIA_ENT_T_DEVNODE + 18)
> > #define MEDIA_INTF_T_ALSA_RAWMIDI (MEDIA_ENT_T_DEVNODE + 19)
> > #define MEDIA_INTF_T_ALSA_HWDEP (MEDIA_ENT_T_DEVNODE + 20)
>
> I wouldn't reuse MEDIA_ENT_T_DEVNODE in the definition of the new values as
> that macro becomes deprecated. It needs to be kept for backward-compatibility
> of course, but I'd rather define it in terms of the new macros than the other
> way around.
Agreed.
> That's an implementation detail, it doesn't invalidate the above
> list.
>
> > struct mc_interface {
> > __u32 id;
> > __u32 intf_type;
>
> Is there a particular reason to name this field intf_type instead of just type
> ?
>
> > __u32 flags;
> > __u32 num_entity_links;
> > __u32 reserved[8];
> >
> > union {
> > /* Node specifications */
> > struct {
> > __u32 major;
> > __u32 minor;
> > } devnode;
> >
> > // TBC: struct {
> > // __s32 if_index;
> > // } netif;
> >
> > __u32 raw[16];
> > };
> > };
I'm actually in doubt keeping the union here or representing them via
properties. See more below.
> >
> > The type determines which interface it is, and additional details on how to
> > find the interface are in the union. This example also shows how to identify
> > network interfaces if we want to expose the network interfaces such as are
> > created by DVB.
> >
> > The num_entity_links field reports how many entities are controlled by this
> > interface. While not strictly necessary it is useful for applications to
> > know.
> >
> > There is currently only one flag: MEDIA_INTF_FL_DEFAULT (formerly
> > MEDIA_ENT_FL_DEFAULT). This flag tells applications whether this interface
> > is a default interface, i.e. the one apps should use by default. Useful
> > when there are e.g. multiple video interfaces.
>
> Still brainstorming here, would it make sense to have a "usage" field instead
> ? I'm thinking about cases where different device nodes of the same type would
> be considered as default for different use cases. For instance if you have a
> TV capture card that can output two video streams, one for preview purpose in
> YUV and the other one for capture purpose in MPEG, which one would be the
> single default ? The answer will likely vary depending on whether users are
> more interested in watching TV on their screen or recording it.
An "usage" property makes sense to me.
> > The new pad struct looks like this:
> >
> > struct mc_pad {
> > __u32 id;
>
> Is this an ID as in the global pad ID discussed above, or an index local to
> the entity ? I believe an index is still very useful at least inside the
> kernel, and possibly in userspace as well, as pad IDs won't be stable and
> couldn't be used to identify a pad in the context of its entity.
This should be a global ID. We'll need dynamic PADs for some usages on
DVB, so the ID may not be indexed.
Also, identifying a PAD by an index is problematic. One of the problems
I had when creating the V4L2 MC representation for TV devices is that
some entities have multiple PADs but each PAD is used to carry a different
type of signal.
For example, almost all tuners have:
- one output PAD with the TV IF signal;
- one output PAD with the audio IF signal.
Also, all analog TV demodulators have:
- one output PAD for video;
- one output PAD for raw Vertical Blank Interface (VBI) signal.
And a few demods also have a separate PAD for decoded VBI data.
On both cases, the output PADs are not interchangeable, and they're
bound to be linked to different entities or different entity pads.
Right now, as the PAD index is driver-specific, I had to create a
graph traversal function to connect the links inside the drivers,
as the core doesn't know what PADs index has video, audio or VBI.
That's problematic, specially when an independent driver like
snd-audio-usb needs to connect the audio PAD into the entity
that handles audio input samples inside it.
I would very much prefer to have an "usage" field at the PADs,
as this would allow to have core functions to create the links
between the entities and to enable/disable such links at streamon
and streamoff for most devices, avoiding code duplication.
In other words, I would replace the PAD index by something like:
media_pad *find_pad(struct media_entity entity, enum pad_usage usage, int idx);
vbi_pad = find_pad(entity, PAD_TYPE_VBI, 0);
video_pad = find_pad(entity, PAD_TYPE_VIDEO, 0);
audio_pad = find_pad(entity, PAD_TYPE_AUDIO, 0);
or
media_pad *find_pad(struct media_entity entity, char *pad_name);
vbi_pad = find_pad(entity, "vbi");
video_pad = find_pad(entity, "video");
audio_pad = find_pad(entity, "audio");
Such calls are driver-independent and can be used inside ALSA, DVB and
V4L2 core functions, solving the issue on common use cases.
>
> > __u32 entity_id;
> > __u32 flags;
> > __u16 num_links;
> > __u16 reserved[9];
> > };
> >
> > And links look like this:
> >
> > struct mc_link {
> > __u32 source_id;
> > __u32 sink_id;
> > __u32 flags;
> > __u32 reserved[5];
> > };
> >
> > The source/sink_id fields refer to pads (for data links) or interface
> > (source_id) and entity (sink_id) for the interface/entity relationships.
> >
> > While not implemented initially this will also make it possible to model
> > cases where one entity controls another entity. Whether we actually will
> > need this is uncertain, but at least it can be supported should we require
> > it.
> >
> > Finally we will need a way to retrieve all this information. To do this a
> > single struct is created:
> >
> > struct mc_topology {
> > __u32 topology_version;
> > __u32 num_entities;
> > struct mc_entity *entities;
> > __u32 num_interfaces;
> > struct mc_interface *interfaces;
> > __u32 num_pads;
> > struct mc_pad *pads;
> > __u32 num_links;
> > struct mc_link *links;
> > __u32 reserved[64];
> > };
This would work, and I can accept this, but my concern is that we'll have
a hard limit on extending the types of objects, given by the "reserved"
field.
So, an alternative proposal would be to replace it with something like:
struct media_graph_object {
u32 length;
/* Actually, we can merge those two, as proposed by Hans */
u32 type;
u32 id;
void *object_data;
} __attribute__ ((packed));
struct media_graph_topology {
__u32 topology_version;
__u32 num_objects;
struct media_graph_object *objects;
};
And have the structs with (some) properties embed on it, like:
struct media_graph_entity {
struct media_graph_object obj;
u32 flags;
/* ... */
char name[64];
};
Please notice that we don't need to add reserved fields at the structs,
as we're now putting the struct length at the media_graph_object.
So, if we need to, for example, add a new "foo" inside the
media_graph_entity:
struct media_graph_entity {
struct media_graph_object obj;
u32 flags;
/* ... */
char name[64];
u32 foo;
};
There are some advantages of this approach:
- If the size of the entity will change, and obj.length will be bigger.
Userspace will allocate more space to store the object, but will be
backward compatible;
- We can add new object types anytime. If userspace doesn't know the new
type, it should simply discard the object and go to the next one. Again,
backward compatible.
We may eventually add a way for userspace to request only a subset of
the graph elements or to add an ioctl or some other sort of event that
will report topology changes.
> >
> > #define MEDIA_IOC_G_TOPOLOGY _IOWR('|', 0x04, struct mc_topology)
> >
> > Applications can allocate arrays for each type that they want to retrieve
> > (or NULL if they are not interested) and fill in the num_ fields with the
> > size of these arrays. Existing DVB/V4L applications that are typically only
> > interested in the interfaces created by a device will only need the
> > interfaces and can set the others to NULL.
> >
> > The driver will copy the data to the arrays and set the num_ fields to the
> > total number of elements.
> >
> > So by zeroing the struct and calling the ioctl once the num_ fields will be
> > filled in with the number of elements each array requires. The application
> > can allocate the memory and call the ioctl again to get the full topology.
> > The topology_version will be increased whenever the topology changes.
> > Static topologies that cannot change will always return 0 here. Topologies
> > that can change will start with version 1 and never return 0 here.
> >
> > As mentioned before, the property description is absent in this discussion,
> > but the idea is to allow properties for each object type except links (at
> > least for now) and to report all properties as part of the mc_topology
> > ioctl.
> >
> > A separate RFC will be written by Sakari for this.
> >
> > Backwards compatibility notes:
> >
> > - currently only entities are enumerated and that will be unchanged. So
> > neither interfaces nor connectors will be seen by the existing API.
> > - the v4l-subdev interface will still be reported in the union of the old
> > media_entity_desc struct.
> > - currently entities representing DMA engines are reported as type
> > MEDIA_ENT_T_DEVNODE_V4L. This will change: such entities should be reported
> > as having type DMA engine. I think we can make an alias for
> > MEDIA_ENT_T_DEVNODE_V4L called MEDIA_ENT_T_DMA_ENGINE. The major/minor in
> > the dev struct in the union will point to the video device. - These are the
> > only two cases where the dev struct is used, for all other cases it will be
> > 0.
> > - struct media_pad_desc needs to be extended with a unique pad ID field.
> >
> > Regards,
> >
> > Hans
> >
> > PS: I'm on vacation this week, so don't expect quick replies from me :-)
>
^ permalink raw reply
* Re: [PATCH 0/2] Documentation: iio-trig-sysfs: Add missing ABI
From: Jonathan Cameron @ 2015-08-15 15:09 UTC (permalink / raw)
To: Cristina Opriceana
Cc: daniel.baluta-ral2JQCrhuEAvxtiuMwx3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1439378363.git.cristina.opriceana-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 12/08/15 12:26, Cristina Opriceana wrote:
> Add the ABI documentation for the add_trigger and remove_trigger
> attributes, found in iio-trig-sysfs.
>
> Cristina Opriceana (2):
> Documentation: iio-trig-sysfs: Document add_trigger attribute
> Documentation: iio-trig-sysfs: Add remove_trigger attribute ABI
>
> .../ABI/testing/sysfs-bus-iio-trigger-sysfs | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
These two could easily have been one patch given their paired nature,
but I don't care that much.
Applied to the togreg branch of iio.git (fuzz due to a pervious patch in there)
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH RFC 3/3] nvmem: add permission flags in nvmem_config
From: Stefan Wahren @ 2015-08-15 7:21 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: wxt-TNX95d0MmH7DzftRWevZcw, linux-api-u79uwXL29TY76Z2rM5mHXA,
Rob Herring, p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ, Andrew Lunn,
sboyd-sgV2jX0FEOL9JmXXK+q4OQ, arnd-r2nGTMty4D4,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
khilman-DgEjT+Ai2ygdnm+yROfE0A,
maitysanchayan-Re5JQEeQqe8AvxtiuMwx3w,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
mporter-OWPKS81ov/FWk0Htik3J/w,
pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA, Mark Brown, Greg Kroah-Hartman
In-Reply-To: <1439291024-18375-1-git-send-email-srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Hi Srinivas,
> Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> hat am 11. August 2015 um
> 13:03 geschrieben:
>
>
> This patch adds perm variable to nvmem_config structure which will allow
> providers to specify the permissions required for the sysfs binary file.
> This permission is applied on top of root-only access permissions set by
> the core.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> drivers/nvmem/core.c | 1 +
> include/linux/nvmem-provider.h | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 414ed23..7481387 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -312,6 +312,7 @@ struct nvmem_device *nvmem_register(const struct
> nvmem_config *config)
> config->read_only;
> nvmem->bin = bin_attr_template;
> nvmem->bin.attr.mode = nvmem->read_only ? : (S_IRUSR | S_IWUSR);
> + nvmem->bin.attr.mode |= config->perm;
> nvmem->bin.size = nvmem->size;
> device_initialize(&nvmem->dev);
>
> diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
> index 0b68caf..de9a61f 100644
> --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -23,6 +23,7 @@ struct nvmem_config {
> const struct nvmem_cell_info *cells;
> int ncells;
> bool read_only;
> + u16 perm; /* visibility in sysfs */
how about "umode_t mode" or "umode_t sysfs_mode"?
Beside that the whole series looks good to me.
Thanks Stefan
^ permalink raw reply
* Re: [PATCHv4 man-pages 3/3] open.2: describe O_BENEATH flag
From: David Drysdale @ 2015-08-14 15:30 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Michael Kerrisk-manpages,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Alexander Viro, Kees Cook, Eric W. Biederman, Greg Kroah-Hartman,
Meredydd Luff, Will Drewry, Jorge Lucangeli Obes, Ricky Zhou,
Lee Campbell, Julien Tinnes, Mike Depinet, James Morris,
Paolo Bonzini, Paul Moore, Christoph Hellwig, Dave Chinner,
Linux API, linux-arch, LSM List
In-Reply-To: <CALCETrXLhp6-dVq8TS0_vSQBJieP-SrabhoDJE0PZ8KpMz1m0A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Aug 14, 2015 at 3:17 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Fri, Aug 14, 2015 at 2:29 AM, David Drysdale <drysdale-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>> On Fri, Aug 14, 2015 at 6:33 AM, Michael Kerrisk (man-pages)
>> <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On 13 August 2015 at 19:38, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>> On Thu, Aug 13, 2015 at 2:32 AM, David Drysdale <drysdale-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>> Signed-off-by: David Drysdale <drysdale-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>>>
>>>> What's the behavior wrt fcntl(F_GETFL, etc)?
>>>
>>> I would presume that O_BENEATH is one of the so-called "file creation
>>> flags". See this paragraph of the DESCRIPTION:
>>>
>>> In addition, zero or more file creation flags and file status
>>> flags can be bitwise-or'd in flags. The file creation flags are
>>> O_CLOEXEC, O_CREAT, O_DIRECTORY, O_EXCL, O_NOCTTY, O_NOFOLLOW,
>>> O_TMPFILE, O_TRUNC, and O_TTY_INIT. The file status flags are
>>> all of the remaining flags listed below. The distinction between
>>> these two groups of flags is that the file status flags can be
>>> retrieved and (in some cases) modified; see fcntl(2) for details.
>>>
>>> David, presuming this is correct (I can't see how O_BENEATH could be a
>>> "file *status* flag"), your patch should also add O_BENEATH to the
>>> list in that paragraph.
>>
>> Yeah, O_BENEATH makes sense as a file creation flag; I'll add it
>> to that list -- thanks for spotting.
>
> Should there be a test that you can't clear O_BENEATH with F_SETFL?
>
> --Andy
I'll add a test that fcntl(F_SETFL) silently ignores the file creation flags,
including O_BENEATH.
^ permalink raw reply
* Re: [PATCHv4 man-pages 3/3] open.2: describe O_BENEATH flag
From: Andy Lutomirski @ 2015-08-14 14:17 UTC (permalink / raw)
To: David Drysdale
Cc: Michael Kerrisk-manpages, linux-kernel@vger.kernel.org,
Alexander Viro, Kees Cook, Eric W. Biederman, Greg Kroah-Hartman,
Meredydd Luff, Will Drewry, Jorge Lucangeli Obes, Ricky Zhou,
Lee Campbell, Julien Tinnes, Mike Depinet, James Morris,
Paolo Bonzini, Paul Moore, Christoph Hellwig, Dave Chinner,
Linux API, linux-arch, LSM List
In-Reply-To: <CAHse=S9HdriyqKrQRj=x3CpEFftPgVFX7p8B9efu9_mQtEvJSA@mail.gmail.com>
On Fri, Aug 14, 2015 at 2:29 AM, David Drysdale <drysdale@google.com> wrote:
> On Fri, Aug 14, 2015 at 6:33 AM, Michael Kerrisk (man-pages)
> <mtk.manpages@gmail.com> wrote:
>> On 13 August 2015 at 19:38, Andy Lutomirski <luto@amacapital.net> wrote:
>>> On Thu, Aug 13, 2015 at 2:32 AM, David Drysdale <drysdale@google.com> wrote:
>>>> Signed-off-by: David Drysdale <drysdale@google.com>
>>>
>>> What's the behavior wrt fcntl(F_GETFL, etc)?
>>
>> I would presume that O_BENEATH is one of the so-called "file creation
>> flags". See this paragraph of the DESCRIPTION:
>>
>> In addition, zero or more file creation flags and file status
>> flags can be bitwise-or'd in flags. The file creation flags are
>> O_CLOEXEC, O_CREAT, O_DIRECTORY, O_EXCL, O_NOCTTY, O_NOFOLLOW,
>> O_TMPFILE, O_TRUNC, and O_TTY_INIT. The file status flags are
>> all of the remaining flags listed below. The distinction between
>> these two groups of flags is that the file status flags can be
>> retrieved and (in some cases) modified; see fcntl(2) for details.
>>
>> David, presuming this is correct (I can't see how O_BENEATH could be a
>> "file *status* flag"), your patch should also add O_BENEATH to the
>> list in that paragraph.
>
> Yeah, O_BENEATH makes sense as a file creation flag; I'll add it
> to that list -- thanks for spotting.
Should there be a test that you can't clear O_BENEATH with F_SETFL?
--Andy
^ permalink raw reply
* Re: [RFC][PATCH 1/2] usb: gadget: configfs: add MTP function
From: Amit Pundir @ 2015-08-14 9:53 UTC (permalink / raw)
To: Krzysztof Opasiak
Cc: Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Mike Lockwood, Benoit Goby,
Colin Cross, Arve Hjønnevåg, Peter Oh, Greg Hackmann,
Badhri Jagan Sridharan, Android Kernel Team, Jonathan Corbet,
Felipe Balbi, Andrzej Pietrasiewicz, Laurent Pinchart,
Yegor Yefremov, Philippe Reynes, John Stultz, Sumit Semwal
In-Reply-To: <55CD0108.4060303-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
On 14 August 2015 at 02:11, Krzysztof Opasiak <k.opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>
>
> On 08/13/2015 09:57 PM, Greg Kroah-Hartman wrote:
>>
>> On Thu, Aug 13, 2015 at 09:34:46PM +0200, Krzysztof Opasiak wrote:
>>>
>>> Hello,
>>>
>>> On 08/13/2015 09:12 PM, Amit Pundir wrote:
>>>>
>>>> his MTP function is based on years of work originally done in the
>>>> Android kernel tree by:
>>>> Mike Lockwood<lockwood-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>>> Benoit Goby<benoit-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>>> Colin Cross<ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>>> Arve Hjønnevåg<arve-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>>> Peter Oh<poh-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>> Greg Hackmann<ghackmann-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>>> Badhri Jagan Sridharan<Badhri-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>>> I've folded the series up to make it easier to review, and to provide
>>>> a coherent patch description.
>>>>
>>>> Post Gingerbread (Android v2.3), Android dropped USB Mass Storage
>>>> in favor of Media Transfer Protocal (MTP), which is widely used for
>>>> transferring media files to digital music players and similar
>>>> applications. This USB gadget function implements MTP functionalty.
>>>>
>>>> Historically this function has been a part of Android composite
>>>> gadget driver. Android composite driver was Android's solution
>>>> for dynamic gadget function switching prior to the ConfigFS gadget
>>>> being merged. There were failed few attempts in past
>>>> http://marc.info/?l=linux-usb&m=132451695808552 to upstream Android
>>>> composite driver as well. Now this Android MTP gadget function has been
>>>> re-implemented so as to be used as a generic ConfigFS function instead.
>>>>
>>>> Again, many thanks to Mike, Benoit, Colin, Arve, Peter, Greg and Badhri,
>>>> as they are the real authors of this work. However, I've folded their
>>>> patches together and modified it enough that I don't want them to be
>>>> blamed for any mistakes I've made condensing their patches down.
>>>>
>>>> Cc: Mike Lockwood<lockwood-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>>> Cc: Benoit Goby<benoit-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>>> Cc: Colin Cross<ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>>> Cc: Arve Hjønnevåg<arve-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>>> Cc: Peter Oh<poh-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>> Cc: Greg Hackmann<ghackmann-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>>> Cc: Badhri Jagan Sridharan<Badhri-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>>> Cc: Android Kernel Team<kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>>> Cc: Greg Kroah-Hartman<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
>>>> Cc: Jonathan Corbet<corbet-T1hC0tSOHrs@public.gmane.org>
>>>> Cc: Felipe Balbi<balbi-l0cyMroinI0@public.gmane.org>
>>>> Cc: Andrzej Pietrasiewicz<andrzej.p-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>>>> Cc: Laurent Pinchart<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>>>> Cc: Yegor Yefremov<yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>>>> Cc: Philippe Reynes<tremyfr-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> Cc: John Stultz<john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> Cc: Sumit Semwal<sumit.semwal-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> Signed-off-by: Amit Pundir<amit.pundir-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>
>>>
>>> In my humble opinion adding such function to Linux kernel doesn't make
>>> any
>>> sense. By design, MTP is a protocol which requires access to userspace
>>> features esp. file system. It is very important to run MTP daemon with
>>> suitable user and LSM label and many many other issues which should be
>>> handled by userspace access policy.
>>>
>>> Moreover this is not a fully functional USB function but only some
>>> interface
>>> which can be used by mtp-responder (mtp-daemon - call it as you like) to
>>> communicate with host. As we have FunctionFS which allows to implement
>>> any
>>> USB function in as a userspace service. As MTP nature is more related to
>>> userspace I think that porting MTP daemon to use this is a right way to
>>> go.
>>> This should be much more reasonable than adding new function which also
>>> requires daemon for proper working. So why add another interface while we
>>> can use a generic one?
Fairly valid point. I did see MTP mentioned in FunctionFS context in
Documentation/usb/functionfs.txt but I could not find an open
userpsace MTP daemon implementation based on F_FS to try.
>>
>>
>> Isn't there already a userspace MTP daemon that uses the existing
>> functionfs for usb gadgets? I thought I remember seeing that
>> somewhere...
>>
>
> I've found some interesting link[2] which may mean that Sailfish OS guys has
> some mtp implementation with functionfs backend:
>
> <<<<< cite
>
> - /dev/mtp
> mtp functionfs rw,relatime
>
>>>>>> cite
>
> Started digging and got it!
>
> This looks like mtp with ffs backend:
>
> https://github.com/nemomobile/buteo-mtp
Thanks for the pointer. This "buteo-mtp" looks interesting.
Regards,
Amit Pundir
>
> Didn't tested, even didn't try to compile, no guarantee;)
>
> Footnotes:
> 1 -
> http://reviewjolla.blogspot.com/2014/06/techspecs-android-on-jolla-phone.html
>
>
> Best regards,
>
> --
> Krzysztof Opasiak
> Samsung R&D Institute Poland
> Samsung Electronics
^ permalink raw reply
* Re: [PATCHv4 man-pages 3/3] open.2: describe O_BENEATH flag
From: David Drysdale @ 2015-08-14 9:29 UTC (permalink / raw)
To: Michael Kerrisk-manpages
Cc: Andy Lutomirski, linux-kernel@vger.kernel.org, Alexander Viro,
Kees Cook, Eric W. Biederman, Greg Kroah-Hartman, Meredydd Luff,
Will Drewry, Jorge Lucangeli Obes, Ricky Zhou, Lee Campbell,
Julien Tinnes, Mike Depinet, James Morris, Paolo Bonzini,
Paul Moore, Christoph Hellwig, Dave Chinner, Linux API,
linux-arch, LSM List, Linux FS Devel
In-Reply-To: <CAKgNAkj+fZJnzKbHb8UvZg-0kNfB7MQHQYoAz-dVhnkso0Uq2A@mail.gmail.com>
On Fri, Aug 14, 2015 at 6:33 AM, Michael Kerrisk (man-pages)
<mtk.manpages@gmail.com> wrote:
> On 13 August 2015 at 19:38, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Thu, Aug 13, 2015 at 2:32 AM, David Drysdale <drysdale@google.com> wrote:
>>> Signed-off-by: David Drysdale <drysdale@google.com>
>>
>> What's the behavior wrt fcntl(F_GETFL, etc)?
>
> I would presume that O_BENEATH is one of the so-called "file creation
> flags". See this paragraph of the DESCRIPTION:
>
> In addition, zero or more file creation flags and file status
> flags can be bitwise-or'd in flags. The file creation flags are
> O_CLOEXEC, O_CREAT, O_DIRECTORY, O_EXCL, O_NOCTTY, O_NOFOLLOW,
> O_TMPFILE, O_TRUNC, and O_TTY_INIT. The file status flags are
> all of the remaining flags listed below. The distinction between
> these two groups of flags is that the file status flags can be
> retrieved and (in some cases) modified; see fcntl(2) for details.
>
> David, presuming this is correct (I can't see how O_BENEATH could be a
> "file *status* flag"), your patch should also add O_BENEATH to the
> list in that paragraph.
Yeah, O_BENEATH makes sense as a file creation flag; I'll add it
to that list -- thanks for spotting.
> Cheers,
>
> Michael
>
>
>
> --
> Michael Kerrisk
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCHv4 man-pages 3/3] open.2: describe O_BENEATH flag
From: Michael Kerrisk (man-pages) @ 2015-08-14 5:33 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Drysdale, linux-kernel@vger.kernel.org, Alexander Viro,
Kees Cook, Eric W. Biederman, Greg Kroah-Hartman, Meredydd Luff,
Will Drewry, Jorge Lucangeli Obes, Ricky Zhou, Lee Campbell,
Julien Tinnes, Mike Depinet, James Morris, Paolo Bonzini,
Paul Moore, Christoph Hellwig, Dave Chinner, Linux API,
linux-arch, LSM List, Linux FS Devel
In-Reply-To: <CALCETrWusE9CbQ6A1iqgBVjei-_JWL4o4ezS14JLEAbrGJmaNg@mail.gmail.com>
On 13 August 2015 at 19:38, Andy Lutomirski <luto@amacapital.net> wrote:
> On Thu, Aug 13, 2015 at 2:32 AM, David Drysdale <drysdale@google.com> wrote:
>> Signed-off-by: David Drysdale <drysdale@google.com>
>
> What's the behavior wrt fcntl(F_GETFL, etc)?
I would presume that O_BENEATH is one of the so-called "file creation
flags". See this paragraph of the DESCRIPTION:
In addition, zero or more file creation flags and file status
flags can be bitwise-or'd in flags. The file creation flags are
O_CLOEXEC, O_CREAT, O_DIRECTORY, O_EXCL, O_NOCTTY, O_NOFOLLOW,
O_TMPFILE, O_TRUNC, and O_TTY_INIT. The file status flags are
all of the remaining flags listed below. The distinction between
these two groups of flags is that the file status flags can be
retrieved and (in some cases) modified; see fcntl(2) for details.
David, presuming this is correct (I can't see how O_BENEATH could be a
"file *status* flag"), your patch should also add O_BENEATH to the
list in that paragraph.
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCHv4 1/1] Documentation: describe how to add a system call
From: Jonathan Corbet @ 2015-08-13 23:53 UTC (permalink / raw)
To: David Drysdale
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA, Michael Kerrisk, Andrew Morton,
Arnd Bergmann, Shuah Khan, Eric B Munson, Randy Dunlap,
Cyril Hrubis, Josh Triplett, Pavel Machek, Andrea Arcangeli,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Oleg Nesterov,
Linus Torvalds, Greg Kroah-Hartman, Andy Lutomirski, Al Viro,
Rusty Russell, Peter Zijlstra, Vivek Goyal, Alexei Starovoitov,
David Herrmann <dh.he>
In-Reply-To: <1439193644-4773-2-git-send-email-drysdale-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
On Mon, 10 Aug 2015 09:00:44 +0100
David Drysdale <drysdale-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> Add a document describing the process of adding a new system call,
> including the need for a flags argument for future compatibility, and
> covering 32-bit/64-bit concerns (albeit in an x86-centric way).
This seems about ready, so I've gone ahead and applied it to the docs
tree. Many thanks!
jon
^ permalink raw reply
* Re: [RFC][PATCH 2/2] usb: gadget: configfs: notify userspace of usb state changes
From: Felipe Balbi @ 2015-08-13 22:07 UTC (permalink / raw)
To: Krzysztof Opasiak
Cc: Amit Pundir, linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Mike Lockwood, Benoit Goby,
Colin Cross, Arve Hjønnevåg, Peter Oh, Greg Hackmann,
Badhri Jagan Sridharan, Android Kernel Team, Greg Kroah-Hartman,
Jonathan Corbet, Felipe Balbi, Andrzej Pietrasiewicz,
Laurent Pinchart, Yegor Yefremov, Philippe Reynes, John Stultz,
Sumit Semwal
In-Reply-To: <55CCF319.1070604-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4624 bytes --]
Hi,
On Thu, Aug 13, 2015 at 09:42:17PM +0200, Krzysztof Opasiak wrote:
> Hello,
>
> On 08/13/2015 09:12 PM, Amit Pundir wrote:
> >This is more of an RFC than an actual submission. There are few
> >scattered #ifdefs..#endifs here and there which still need to be
> >taken care of before going for actual submission.
> >
> >Currently there is no way with the upstream ConfigFS gadget to
> >communicate state changes (connected, disconnected, configured), at
> >the gadget level. Instead such state changes are handled function by
> >function independently I presume. This is problematic, because some
> >coordination between the functions, across the state changes, may be
> >desired at the userspace level. Thus to address this issue, this
> >patch send uevents to allow userspace to be notified of these usb
> >state changes, allowing userspace to respond and configure the
> >configfs gadget appropriately.
> >
> >This patch is based on an Android patchset originaly authored by
> >Badhri Jagan Sridharan<Badhri-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> to send uevent notifications
> >to Android userpace for USB state changes. I've folded his patches
> >together and modified it enough that I don't want him to be blamed for
> >any mistakes I've made condensing his patches down.
> >
> >This patch introduces USB_CONFIGFS_UEVENT Kconfig to handle userspace
> >notifications of usb state changes, and add setup and disconnect
> >functions to intercept the setup requests from the usb_core. It also
> >creates a sysfs device class entry and a device attribute (state) to
> >read and respond to gadget's current state from userspace. As of now
> >this sysfs device class (/sys/class/android_usb) and gadget device
> >(/sys/class/android_usb/android0) with state attribute
> >(/sys/class/android_usb/android0/state) are strictly tied up to
> >facilitate Android userspace requests. But going forward we may want
> >to bring all function devices (hid, printer etc) under a unified usb
> >gadget device class e.g. /sys/class/usb_gadget/g_{func0,func1} etc..
> >
> >Also I think it make sense to add this state attribute to the configfs
> >usb gadget itself i.e. have something like /config/usb_gadget/g1/state
> >to read USB gadget's current state. Since it is going to be consistent
> >throughout all the functions tied up to that gadget.
> >
> >Again this is just an initial RFC, thoughts and feedback would be
> >greatly appreciated.
> >
> >Cc: Mike Lockwood<lockwood-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> >Cc: Benoit Goby<benoit-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> >Cc: Colin Cross<ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> >Cc: Arve Hjønnevåg<arve-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> >Cc: Peter Oh<poh-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> >Cc: Greg Hackmann<ghackmann-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> >Cc: Badhri Jagan Sridharan<Badhri-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> >Cc: Android Kernel Team<kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> >Cc: Greg Kroah-Hartman<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> >Cc: Jonathan Corbet<corbet-T1hC0tSOHrs@public.gmane.org>
> >Cc: Felipe Balbi<balbi-l0cyMroinI0@public.gmane.org>
> >Cc: Andrzej Pietrasiewicz<andrzej.p-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> >Cc: Laurent Pinchart<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> >Cc: Yegor Yefremov<yegorslists-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> >Cc: Philippe Reynes<tremyfr-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >Cc: John Stultz<john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >Cc: Sumit Semwal<sumit.semwal-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >Signed-off-by: Amit Pundir<amit.pundir-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> Generally I agree that there should be some way of notifying userspace about
yes, and we already have a sysfs file for that. See udc-core.c:
static void usb_gadget_state_work(struct work_struct *work)
{
struct usb_gadget *gadget = work_to_gadget(work);
struct usb_udc *udc = gadget->udc;
if (udc)
sysfs_notify(&udc->dev.kobj, NULL, "state");
}
void usb_gadget_set_state(struct usb_gadget *gadget,
enum usb_device_state state)
{
gadget->state = state;
schedule_work(&gadget->work);
}
EXPORT_SYMBOL_GPL(usb_gadget_set_state);
If it's not working for any UDC, it just means the UDC needs to be
patched and if we're missing any state, it means that either the UDC
can't provide that IRQ, or we need to add more states to that
enumeration (which I find unlikely).
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC][PATCH 1/2] usb: gadget: configfs: add MTP function
From: Krzysztof Opasiak @ 2015-08-13 20:41 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Amit Pundir, linux-usb, linux-kernel, linux-doc, linux-api,
Mike Lockwood, Benoit Goby, Colin Cross, Arve Hjønnevåg,
Peter Oh, Greg Hackmann, Badhri Jagan Sridharan,
Android Kernel Team, Jonathan Corbet, Felipe Balbi,
Andrzej Pietrasiewicz, Laurent Pinchart, Yegor Yefremov,
Philippe Reynes, John Stultz, Sumit Semwal
In-Reply-To: <20150813195748.GB30092@kroah.com>
On 08/13/2015 09:57 PM, Greg Kroah-Hartman wrote:
> On Thu, Aug 13, 2015 at 09:34:46PM +0200, Krzysztof Opasiak wrote:
>> Hello,
>>
>> On 08/13/2015 09:12 PM, Amit Pundir wrote:
>>> his MTP function is based on years of work originally done in the
>>> Android kernel tree by:
>>> Mike Lockwood<lockwood@android.com>
>>> Benoit Goby<benoit@android.com>
>>> Colin Cross<ccross@android.com>
>>> Arve Hjønnevåg<arve@android.com>
>>> Peter Oh<poh@broadcom.com>
>>> Greg Hackmann<ghackmann@google.com>
>>> Badhri Jagan Sridharan<Badhri@google.com>
>>> I've folded the series up to make it easier to review, and to provide
>>> a coherent patch description.
>>>
>>> Post Gingerbread (Android v2.3), Android dropped USB Mass Storage
>>> in favor of Media Transfer Protocal (MTP), which is widely used for
>>> transferring media files to digital music players and similar
>>> applications. This USB gadget function implements MTP functionalty.
>>>
>>> Historically this function has been a part of Android composite
>>> gadget driver. Android composite driver was Android's solution
>>> for dynamic gadget function switching prior to the ConfigFS gadget
>>> being merged. There were failed few attempts in past
>>> http://marc.info/?l=linux-usb&m=132451695808552 to upstream Android
>>> composite driver as well. Now this Android MTP gadget function has been
>>> re-implemented so as to be used as a generic ConfigFS function instead.
>>>
>>> Again, many thanks to Mike, Benoit, Colin, Arve, Peter, Greg and Badhri,
>>> as they are the real authors of this work. However, I've folded their
>>> patches together and modified it enough that I don't want them to be
>>> blamed for any mistakes I've made condensing their patches down.
>>>
>>> Cc: Mike Lockwood<lockwood@android.com>
>>> Cc: Benoit Goby<benoit@android.com>
>>> Cc: Colin Cross<ccross@android.com>
>>> Cc: Arve Hjønnevåg<arve@android.com>
>>> Cc: Peter Oh<poh@broadcom.com>
>>> Cc: Greg Hackmann<ghackmann@google.com>
>>> Cc: Badhri Jagan Sridharan<Badhri@google.com>
>>> Cc: Android Kernel Team<kernel-team@android.com>
>>> Cc: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>>> Cc: Jonathan Corbet<corbet@lwn.net>
>>> Cc: Felipe Balbi<balbi@ti.com>
>>> Cc: Andrzej Pietrasiewicz<andrzej.p@samsung.com>
>>> Cc: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
>>> Cc: Yegor Yefremov<yegorslists@googlemail.com>
>>> Cc: Philippe Reynes<tremyfr@gmail.com>
>>> Cc: John Stultz<john.stultz@linaro.org>
>>> Cc: Sumit Semwal<sumit.semwal@linaro.org>
>>> Signed-off-by: Amit Pundir<amit.pundir@linaro.org>
>>
>> In my humble opinion adding such function to Linux kernel doesn't make any
>> sense. By design, MTP is a protocol which requires access to userspace
>> features esp. file system. It is very important to run MTP daemon with
>> suitable user and LSM label and many many other issues which should be
>> handled by userspace access policy.
>>
>> Moreover this is not a fully functional USB function but only some interface
>> which can be used by mtp-responder (mtp-daemon - call it as you like) to
>> communicate with host. As we have FunctionFS which allows to implement any
>> USB function in as a userspace service. As MTP nature is more related to
>> userspace I think that porting MTP daemon to use this is a right way to go.
>> This should be much more reasonable than adding new function which also
>> requires daemon for proper working. So why add another interface while we
>> can use a generic one?
>
> Isn't there already a userspace MTP daemon that uses the existing
> functionfs for usb gadgets? I thought I remember seeing that
> somewhere...
>
I've found some interesting link[2] which may mean that Sailfish OS guys
has some mtp implementation with functionfs backend:
<<<<< cite
- /dev/mtp
mtp functionfs rw,relatime
>>>>> cite
Started digging and got it!
This looks like mtp with ffs backend:
https://github.com/nemomobile/buteo-mtp
Didn't tested, even didn't try to compile, no guarantee;)
Footnotes:
1 -
http://reviewjolla.blogspot.com/2014/06/techspecs-android-on-jolla-phone.html
Best regards,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox