hverkuil: just read that the freescale kernel still uses the intdev API -_-'
Yeah, that's why it's so important to get Steve Longerbeam's work in the kernel to replace that crap.
Freescale doesn't seem to be maintaining that code anymore.
or certainly have no interest in updating it.
well, if it works!
larsc: it doesn't.
hverkuil: it works 'good enough' for them apparently
Not for some of their customers. The work on new drivers isn't being done for no reason.
Besides, the code is really exceptionally bad :-)
and of course can't be upgraded to a newer kernel.
I know, the stuff is absolutely horrible
and all code is application specific
new application same part? new driver!
mchehab: any opinion on the last iteration of the media entity types patch series ?
not yet. I'll look on it likely later today.
mchehab: pinchartl: can someone repost the latest proposal, because I have lost the plot. Too many replies to replies to replies to....
btw, any opinion with regards to the connectors documentation patch: https://patchwork.linuxtv.org/patch/33287/?
hverkuil: proposal for what, media entity types or connectors ?
Both :-)
hverkuil: will send in a few
mchehab: I haven't replied to that patch specifically as I explained in my last reply yesterday night in the mail thread why I think we're not there yet and more work is needed. the patch doesn't solve the problem in my opinion, we still have no way to represent connectors that can be considered stable from a userspace point of view
My problem is that I can't spend as much time on this as I want, so having periodic patch series to refer to helps a lot to see the latest version so I know what we're all referring to.
are you kidding?
hverkuil: "[PATCH v3 0/8] media: Add entity types" sent to the list two days ago. 8 patches, no reply yet, so I don't think it requires a repost only two days later :-)
mchehab: I assume the question is addressed to me, and no, I'm not kidding
OK, so it's about that patch series. Good to know.
hverkuil: sent
mchehab: thanks!
https://patchwork.linuxtv.org/patch/33323/
hverkuil: let me know if you have any comment on that series, it has both your patches and mine. the patches that add device_caps to video_device can go through your tree earlier than the rest of the series as far as I'm concerned
pinchartl: it seems you're. All we need to agree is about the name and the documentation, as I explained you on my last reply
we don't need to address complex connectors yet... jsut composite, rf, and s-video
(and only s-video with s-video signal on it)
it is all 1:1 mapping
your replies were just diverging from the question to be answered
mchehab: thanks for resending that patch. I think something weird happened because I don't think I ever got the original (or it was under a different subject line)
mchehab: I don't agree. my replies tried to solve the problem of how to represent connectors in MC
It has my ack, so I must have seen it...
hverkuil: it was under a different subject line, but you acked it
that's not a problem we can split in small independent pieces
the connector functions, as proposed now, are a little piece of the problem
but they're not standalone
hverkuil: that's the original one: https://patchwork.linuxtv.org/patch/33287/
they're part of a bigger solution
we can't get them right if we don't decide how to represent connectors
we haven't even agreed on whether "conn" means connector or connexion, and haven't defined what connexion means
pinchartl: can't you agree that a S-video, a RF and a Composite connector is a single entity????
as Sakari mentioned, if we want to represent connectors, there's no such think as a s-video, RF or composite connector. we have mini-din4, bnc and rca commonly used for that purpose
this is not how to design a userspace API
pinchartl: what's your proposal?
you don't force it bit by bit hoping that noone will notice
it has to be designed and address the problems being pointed out
that's the normal process
pinchartl: this is what we're trying to do
but that's not what we have done so far
if you have a better proposal, answer with your proposal
it is
if a patch is not ok, one should reply with the corrections to it
why am I supposed to do your work ? :-) if I send a patch and receives a reply pointing out issues, I'm supposed to solve the issues and make a new proposal. I won't ask the reviewer to do my work
I can certainly help
well, then reply to the patch pointing the issues there
I've sent a long reply yesterday, it points out lots of issues
I can send the same mail in reply to the patch too, but I doubt that would help
pinchartl: just like sailus e-mail, you're contradicting yourself on your email... sometimes defending to do a 1:1 map for physical connectors and sometimes saying the opposite
if you can't make up your mind about that, you can't really point to a direction
it's brainstorming, I'm pointing out several options and trying to outline pros and cons of them in order to move forward. it's not a proposal for a full solution
the points you raised are interesting when mapping complex connectors like HDMI and DVI, but hardly for RF, composite, s-video
HDMI and DVI don't seem very complex to me, the most difficult issue is multiplexed analog connectors
also, we all agreed on our connectors meeting on irc that we won't be mapping connectors by their physical type
how can I be expected to ack a proposal for RF, composite and s-video connectors if I don't have a clear picture of how we're going to represent connectors in general, and thus can't see if the proposal fits in it ?
it doesn't make sense to use "mini-din4, bnc and rca commonly used for that purpose"
as you're prpoposing today
it's like sending patches one line at a time, asking people to review the lines individually
they're neither good nor bad, you need to see the full picture in order to review something
well, ITU had this philosophy years ago: they rise questions to be answered, and those questions would go to a workgroup that would have 4 years to discuss...
this is not the Kernel way
the kernel philosophy is to upstream APIs when they're ready
clearly, this one isn't
recap what it was done:
we discussed and agreed about the need of connectors...
ask the rt-prempt developers how long they have been working on it
and sometimes that takes 4 years or more...
patches for them were added last year, passed to a 7 months review period (with is a way larger than the usual kernel way)
and connector support got merged
now, we just need to fine tune and answer to 2 simple questions:
we only started really discussing connectors during that irc meeting a few weeks ago.
1) will we rename the names of the connector entities?
2) how to document them
mchehab: the fact that we're trying to solve the connectors issue only now shows that the patch series that got merged in v4.5-rc1 wasn't ready, sorry
no, the fact is that every time we agree with something, you're against
it is looking that you have something personal against that
I won't apologize for trying to make sure userspace APIs are correctly designed, I feel that's part of my duty as a kernel developer
pinchartl: I'm OK to properly design the API
but we can't take years to discuss every single little bit of it
this is not RT... we're not changing the entire Kernel
MC is a significant API, especially given that we're finally extending it towards ALSA
RT is huge because it touches every single little piece of the Kernel... removing BKL, changing mutex/semaphore code, etc
it's not a driver-specific ioctl for some weird device
and even if it was, it still should be designed properly
no, it isn't... and, without the work I started last year, it was far away to be accepted by ALSA
we've started discussing connectors two weeks ago
and during those discussions it became clear that the problem is more complex than initially thought
https://linuxtv.org/news.php?entry=2015-07-08.mchehab
seek for connectors there
we thus can't expect to create a proper solution in a couple of days
we certainly have mentioned connectors before
sorry, https://linuxtv.org/news.php?entry=2015-11-12.mchehab
but we've only started tackling the problem for real two weeks ago
gah...
https://linuxtv.org/news.php?entry=2015-08-17.mchehab
pinchartl: hdmi connectors are indeed complex, and will require lots of discussions
adding support for it now would be wrong
analog connectors, as they can be multiplexed, seems more complex to me
pinchartl: so far, among the drivers that support MC, no driver exports multiplexed connectors
the only driver that has it is saa7134 (among the ones with MC)...
are you saying that MC should not support multiplexed analog connectors ?
and the code doesn't currently export the "composite over s-video" mode
no. I'm saying we have time to discuss that
no hurry
the case we need to solve is just the ones with 1:1 map
how can you be sure that the F_CONN function you've added to v4.5 will work in the case of multiplexed connectors ?
one S-Video connector <=> one S-video "signal"
well, we should either create a separate entity for this, or add multiple PADs to the entity
that's my point, we don't know what we'll do, so we don't know whether the F_CONN we have today will integrate with that or clash completely
and that's what I want to avoid
pinchartl: are you proposing to map this as non-entities????
do you really think we would add something else?
it's fine developing an API piece by piece, but until we have a complete design we can't tell if it's sound and clean
hverkuil, pinchartl: do you agree or not that a connector is an entity?
it is.
I believe connectors should be represented as entities, yes
and your documentation patch documents F_CONN as connexion instead of connector
But what is a connector entity exactly in the case of more complex situations?
pinchartl: you mean connection, I expect?
I don't think we should use entities to represent connections, at least not before defining what a connection is
hverkuil: yes, sorry, mixing French and English :-)
I was wondering, what happens if we put the F_CONN defines under #ifdef __kernel__?
hverkuil: it will break all PC-customer analog drivers
hmm...
No, because they still compile. It's just userspace tools.
the userspace tools will se an unknown entity, with is already supported
yeah, this should work
I don't want to revert the code, but I feel we are too hurried trying to make the 4.5 deadline.
OK, we can do that
I thought it was simple, but there are too many complex cases coming out of the woodwork suddenly.
hverkuil: seems to often be the case with media devices. they seem simple, but that's about where it stops :-(
people writing SPI eeprom drivers don't know how lucky they are :-)
OK, I'll not apply https://patchwork.linuxtv.org/patch/33323/, and I'll add a patch adding #if __KERNEL__ for the connector definitions
frankly, it's sometimes depressing. You think you're future proof, then the next weird gadget turns up that ruins all your work...
I'll change #33323 to "under review", and let's discuss it later
mchehab: I think that's the best option. We can use the ELC to discuss this more face-to-face.
indeed it is sometimes depressing
hverkuil: I agree. we can never prevent that. I try to do my best to cover at least the common cases
hverkuil: mchehab: we can continue discussing it by e-mail before ELC
works for me
to prepare for the mini-summit
otherwise we would spend the full day on that
yes, MC can take as many hours we have at the summits :)
my availability on IRC will be reduced next week as I'll attend Linaro Connect and be in the UTC+7 time zone, but e-mail will work
and possibly IRC too if we can find a time convenient for everybody
It seems so simple when I designed it 7-8 years ago: some boxes, some links, pads and off you go into the sun!
and burn down and fall to earth :-)
except for Monday, I don't have any thing else scheduled next week
hverkuil: it took one more year developing at at Nokia with pretty much full-time work on it
I am available tomorrow for irc discussion.
tomorrow is OK for me
yet, I think we should keep it documented via e-mail somehow...
I'll fly tomorrow afternoon and I still need to pack, so tomorrow might be problematic
mchehab: agreed
not everybody is on IRC, and people outside might have something to contribute for
I'll have an internet connection in the plan though :-) at least in theory
speaking of contributions, have we lost Sylwester, Tomasz, Kamil and the others ?
I guess they're allocated to some other projects :(
that's a shame
there were several changes on the second half of the last year
indeed
shuah: I am really sorry that I don't seem to be able to find time to review your patches. I wish it was otherwise, but I don't see much improvement in that situation.
I just wanted to mention that, because I feel guilty...
I understand - I wish you have time though, your review is valuable. Can you review the v4l2 specific ones at least
pinchartl: please ping us if you have time tomorrow to discuss
shuah: patch series v3 is the latest, right? With a v4 for patch 22/22?
hverkuil: I have v5 on that with just renaming sound/usb media_* to media_snd*
patcv v5 22/22 i.e
ah yes, I see it.
mchehab: I'll let you know tomorrow morning on IRC
mchehab: I'll have lunch with Sakari tomorrow, and we plan to go through several connector use cases. it's easier face to face with a piece of paper. we'll report on the results
shuah: thanks for the rename
OK, great
I should reply your big connectors e-mail later today, after handling patches
it might be hard to believe (it certainly isn't for me :-)) but I'm not trying to impeded development :-)
pinchartl: No problem - it was something I thought about at some point during the development and kind of spaced out
so good thing you pointed out
pinchartl: as I said, I'm all for improving things and designing a good API... but I don't want to go to the ITU mistake of taking forever to have a solution
the perfect solution would take forever to be implemented...
and by the time we have it, we may not be needing it anymore
so, we need a good solution that works for most cases, and can be adapted to fit corner cases
mchehab: I don't think it will take years. it took a long time as patches have been out without being reviewed, and that's partly my fault, but it should certainly not take a year of full-time development
I hope not. Last year, I won't have as much time as last year to work on it
I mean:
*This* year, I won't have as much time as last year to work on it
sailus: ping
mchehab: Pong.
mchehab: Thanks for applying the PM patches btw.
anytime...
I still don't like patch 1, though ;)
but I guess it won't cause much harm
sailus: I'm pinging you because of the other patch series...
lmml_33181_v2_2_4_media_rearrange_the_fields_in_the_g_topology_ioctl_argument.patch
lmml_33184_v2_1_4_media_sanitise_the_reserved_fields_of_the_g_topology_ioctl_arguments.patch
Yes, there are comments, I remember.
yes
I needed to find more information on this.
what do you mean?
The struct size alignment is not very strictly defined AFAIK.
well, we can add a __packed to the structs, to avoid troubles
I wanted to find a reference to one or more C standards, but couldn't.
that actually makes sense at the APIs
That would be a good idea, yes.
But also to align their size to the size of largest data type used in them.
I'm not fully certain that's true right now, I don't remembert.
s/t\././
I can resend them later today, making sure the size is aligned, and removing reserved fields from the G_TOPOLOGY argument.
And adding __packed.
the packed will make sure that gcc won't try to do some weird alignments
isn't __packed__ discouraged for ioctl structures ?
so, from alignment perspective, it should be safe
sailus, mchehab: I think is better to reorder the struct fields with their sizes in decreasing order
$ git grep pack include/uapi/linux/*|wc -l
1022
but when I proposed that last year it was mentioned that it was not future proof if we later need more fields
javier__: I guess we're talking about patch 1/4
pinchartl: Why would it be?
pinchartl: I'm not sure, some distant memory :-)
mchehab: try grepping for __packed instead :-)
looks like most of them are for structures used with network protocols
err... actuall 345 occurrences
actually
there are two variants...
__attribute__((packed));
and __packed
Packed should be used but solely relying on packed isn't.
one of them is not recommended...
can't remember what ;)
s/t\./t the right way to go./
You still have to properly align the fields for sane ABI requirements.
I guess the recommended way is: __attribute__((packed));
yes, align the fields seem a good idea for me
even if we use packed
pinchartl, sailus: as I said before, we actually don't need to align or pack the structs
I tested using G_TOPOLOGY with both 32 and 64 bits on 64 bits machines (arm64 and x86_64), and everything works fine
even with the structs unaligned
mchehab: I was talking about truct media_v2_topology in general, now I see that Sakari proposed the same for patch 2/4 and you mentioned adding new fields to him as well
mchehab: It may work for you, for now.
If you use the same compiler which uses exactly the same ABI.
javier__: yes, sailus's patch 2/4 is almost identical to the one you sent last year
Using another compiler might give you different results.
sailus: yes, a non-GNU compiler might be doing something different
mchehab: yes, I would prefer not packing the structures
One could also ask why would someone use a different compiler than GCC. :-)
sailus: no, the ABI is defined by the C standard
that's why I agree that it is a good idea to align the structs using the reserved fields
pinchartl: It isn't.
You have different alignment of 64-bit types between x86 and x86-64, for instance.
x86 aligns them to 4 bytes, x86-64 to 8.
that's different architectures, not different compilers :-)
sailus, pinchartl: I'm not 100% sure, but  guess guess this is actually defined by ARM and Intel architectural docs
pinchartl: That's still not the C standard.
sailus: ok, part of it is defined by the architecture ABI too
sailus: it this is defined by the architectural docs, the C compiler should enforce it, or it won't work properly
still, I don't see any issue on doing 64-bit alignment by hand using the reserved fields
or using  __attribute__((aligned(8)));
(although there are only 45 occurrences of aligned attribute at uAPI)
if all fields are naturally aligned to 64-bit, then those will be aligned to 32-bit as well AFAICT
IOW, we just have to make sure that if __u64 and __u32 are mixed, then for each __u64 there should be two consecutives __u32
so the next __u64 is aligned and won't be implicit padding
javier__: on the cases sailus addressed, what we have, instead, is a series of u32, like:
struct media_v2_link {
	__u32 id;
	__u32 source_id;
	__u32 sink_id;
	__u32 flags;
mchehab, pinchartl: This isn't the most relevant reference, but as an example the compiler options may matter (from GCC docs for M68k):
	__u32 reserved[5];
};
       -malign-int
       -mno-align-int
           Control whether GCC aligns "int", "long", "long long", "float",
           "double", and "long double" variables on a 32-bit boundary
           (-malign-int) or a 16-bit boundary (-mno-align-int).  Aligning
           variables on 32-bit boundaries produces code that runs somewhat
           faster on processors with 32-bit busses at the expense of more
           memory.
What I'm saying we should make this as certain as reasonably possible rather than just functional in most cases.
sailus: yes but those are for types whose size may vary between arches
but __u64 and __u32 must always be 64 and 32 bit respectively
well, on RISC, aligning to 32 bits makes things faster
sailus: but yeah, I guess it depends how those types are enforced...
sailus: btw, if one would use those compilation options, I bet it will break the uAPI for the code that handles ioctls...
as it will transform u16 into u32
mchehab: It won't. The size of the types is still the same independently of that option, just the alignment is different.
If you don't use __packed, the struct layout may be affected by that compiler option.
if you have
struct {
_u16 field1, field2;
}
if you force alignment to 32 bits, it will actually do:
struct {
u32 field1, reserved1, field2, reserved2;
ops
u32 field1, field2;
}
breaking the ioctl
(need to be checked, as maybe the types for __u16 does some tricks to fix this)
s/types/typedef/
mchehab: If you use __packed, no. And if you don't use packed, the end result is the same than adding padding in between. It won't affect the types.
it will, as the Kernel is likely not compiled using -malign-int
yes, for sure the Kernel doesn't use malign-int
so, if the Kernel header doesn't use __packed, and someone compiles userspace with malign-int, it will very likely break all ioctls that use 16 bit integers
https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#Common-Type-Attributes
That compiler option only changes the default alignment.
Ah, I only read your last comment now.
I think we agree then.
I'll resend the set, with the comments taken into account, ok?
sailus: the problem is that gcc/glibc defines int16_t as:
typedef short int		int16_t;
and the Kernel:
typedef		__s16		int16_t;
using maling-int will change "short int" to use 32 bits
breaking any ioctl needing it
I doubt that.
there's no __packed or __aligned directive neither at the Kernel or at userspace
for the typedefs
I have no M68k hardware around to test that so we remain ignorant.
so, except if there's some sort of hack internally at gcc, the ioctls will break
neither do I
Qemu, anyone? :-)
well, could work...
but too much trouble for a simple thing...
let's just align to 64 bits like I asked :)
we need reserved fields there anyway
The struct size?
Yes, that's what I intended as well.
patch 2/4 is a different discussion...
javier__ and you are wanting to reorder the fields...
last time we've discussed, as far as I remembered, hverkuil and me were against
can't remember pinchartl's opinion
I think pinchartl suggested it originally, either by e-mail or privately. I don't remember.
pinchartl: Ping?
the problem with reorder is that the API will become quickly messy, as we keep adding more stuff to the struct
(as I described on my answer)
I don't think it'd change much.
reorder could break ioctls - won't it
If we add stuff there, we have to have new structs for compatibility.
no, we don't need
shuah: The argument struct size is different.
shuah: Well, if we add more fields, naturally.
right
We just can't reorder once the API is in a stable kernel.
ioctl's with a variable number or arguments work, provided that you never reorder things at the struct...
it should only grow
Agreed.
right that is what I meant
the problem with the reorder sailus and javier__ are proposing is that we'll have things like:
But it'd be the easiest to manage the compat code if the fields are only added at the end.
u32 num_foo, num_bar;
u64 ptr_foo, ptr_bar;
u32 num_foo2, reserved:
I don't have a strict opinion on that patch, if you don't like it, I'm fine dropping it.
u64 ptr_foo2;
(after adding one extra foo2 pointer)
It only affects the struct layout, there's no functional difference.
ok. Let's drop it then. I guess this will produce a better result as we grow the struct
javier__: are you also OK with that?
I'll resend the set w/o that patch then, in a few hours.
ok.
I already applied the other two patches
(didn't push yet)
Ok.
mchehab: yeah I'm OK with dropping it as well, that's why I didn't push any further after you mentioned that the struct may grow
good
sailus: just pushed the other 2 patches
mchehab: Ack.
I'll only resend the first one then.
OK
larsc: please review this patch: media: adv7180: Add of compatible strings for full family
 http://patchwork.linuxtv.org/patch/33198/
nevermind...
those strings are not documented yet at DT docs
sailus: pong
there's a second version of it, though
http://patchwork.linuxtv.org/patch/33204/
i occassionally get "mceusb 3-1.4:1.0: Error: urb status = -32". i was told thats an EPIPE error. is this something that needs to be addressed in the mceusb driver? im using an HP usb IR receiver.
pinchartl: About dropping "media: Rearrange the fields in the G_TOPOLOGY IOCTL      argument
Are you fine with that?
sailus, pinchartl: I'm assuming that either one of you will be handling your patches for media_ctl at the v4l-utils tree
mchehab: Just out of curiosity... should I have push access there? I presume not?
I'd prefer not, but I won't insist too much. I don't really see a reason why rearranging them would cause problems, it just saves memory
sailus: you should ask for it
mchehab: Can you give me access?
sure
mchehab: Obrigado! :-)
done
de nada
please don't forget to update the status at patchwork after applying the patches
mchehab: Ack.
mchehab: Sent.
thanks
mchehab: do you know that your patch series broke media-ctl ?
I mean the MC rework merged in v4.5-rc1
it broke media-ctl --print-dot