mchehab: thanks for the new series, I've acked them all now. Looks good!
thanks!
I guess today I'll have the patches with interfaces and interface links tested
BTW, I liked the debugging output, should be very useful.
thanks
yeah, that debug is being really helpful on my tests
Regarding: https://patchwork.linuxtv.org/patch/30673/
it's set to Accepted, but it's not yet committed? Is that correct?
it was committed...
at a separate topic
"mm"
I opted to merge in separate and send a separate pull request
during the merge window
OK, no problem.
thanks for the info.
np
hverkuil: I applied Javier patches removing entity.parent
on my tree
I'll move them together with the other patches soon to the media-controller branch
at the upstream development tree
mchehab: note taken
but count me out
?
ok, I've misread you
I thought you meant you where merging them to the master branch
s/where/were/
fyi, I'm still working on MC + interfaces, but progress is slow due to LPC
what's your plan regarding upstream ?
pinchartl: the plan is to merge the MC stuff after having it done, including userspace API
I like that plan :-)
with all that it is needed, including dynamic support
for the 4.4 merge window
now that's a tight schedule
couldn't we merge a first version without dynamic support as a first step ?
well, I'm counting with Javier to help with some stuff
pinchartl: dynamic support is needed due to alsa
I'm not saying we don't need dynamic support
but doing everything in one go doesn't seem like a good idea to me
it's too big
well, let's see what we get
but we'll have 3 people from Samsung working on it
so, I guess it is doable
it depends if you want the community to agree with the work being done
we just need to speedup the review process
pinchartl: the only ones that are looking into it and commenting are the ones that went to the MC workshop
that's the people I was thinking about :-)
if you don't need my ack and Sakari's ack, then it might be doable
well, you should find some time to review patches ;)
I've worked 16h per day last week
and still found time to work on the patches
I guess you're not scaling anymore... you'll likely need to delegate some tasks to someone else
but you can't expect me to work full time on them
or simplify some tasks
I stopped accepting new work
still, MC isn't something I want to drop
pinchartl: as I tried to say you last week, if you're without time, you should focus on just review the patches and not rewrite them
I'm not happy with your patches
I've explained why
yeah, I understand you're concerned with MC
but the feedback wasn't taken in new versions
so what can I do ?
pinchartl: I think I fulfilled all feedbacks I got
and even prepared a simple set of patches that should not take much time to review...
as they're mostly brain-dead patches...
still you had time to review just one of them
that was a step back :-(
instead of implementing support for interfaces in the simplest way possible
why?
you've dropped interfaces and just modify the core to implement the graph object
implementing what was agreed is not simple
it requires lots of work
this is needed. the new API requires it
let me try again to explain what I'd like to see
1. create struct media_entity
without a graph object
sorry
media_interface
2. use media_interface in struct video_device
3. add the userspace API to enumerate interfaces (G_TOPOLOGY)
4. implement that API in the MC core
5. modify drivers accordingly
that way makes no sense to me, as it would mean huge amounts of rework
I think that's the simplest mergeable self-contained patch set
as we'll need to change the internals every time
I rather prefer to do:
1. add one embedded struct that can be used on all objects
2. convert links to lists
3. implement media_interface and links between entities and interfaces
4. implement drivers support for interface
5. work at userspace API
I'm actually thinking on doing 4 and 5 at the same time...
by using Javier
and, maybe 3, 4, and 5, depending on what Shuah is doing
I can't review 1-3 without full dynamic support
and, finally:
6. dynamic support
well, we stripped dynamic support due to your initial feedback
the original plan were to do dynamic support since the beginning
however, with (2), dynamic support for links will be easy
should I just give up then ?
just a matter of adding the proper lock
dynamic support for PADs will be the hardest one
but I guess dynamic support for entities will also be simple
I'm happy to keep delay dynamic PAD support out of the 4.4 scope
I mean, I'm not including dynamic PAD support as part of the goals for 4.4
I don't think dynamic support is doable for 4.4
but I don't see much problems on doing it for links and entities for 4.4
and 1-3 in your approach are not reviewable (by me at least) withotu dynamic support
pinchartl: come on, the patches are simple enough to be reviewed
so what's your goal ? getting something in 4.4, or doing it right ?
pinchartl: my goal is to have support for the needs required to support an hybrid TV PC customer's card with the Media Controller
for 4.4 - excluding the DVB network interface
and add the DVB network interface for 4.5
and you're going to push that to mainline regardless of whether I ack it or not I suppose
pinchartl: well, I'm working hard to get your ack... even travelled to Finland for that to happen
and I took 3 days off while already working at 150% to help making it happen
it was 6 days off for me ;)
actually 7
it's your main job...
no, my main job is to maintain the subsystem
I'm doing that just because you late-nacked the patches I sent last year
I don't see a way out of this for 4.4 if you want to keep your ordering
well, this is a goal
it could be postponed if are there enough reasons why
if I will be short-circuited anyway, why should I spend time reviewing the code ?
pinchartl: I'm really trying to work with you on that
so am I :-)
If I were to just merge, I wouldn't have disabled the DVB parts before reaching upstream...
but you should allocate some time for yourself to review the patches... reviewing one patch on each 6 months won't work
I want to be I can't allocate 2 weeks right now
for the current patch series, you don't need 2 weeks to review
I can't review the current series
I can't see where it's going
I can't review APIs that have no users
such core changes need to be reviewed as part of the big picture
they don't touch the API
let's put it differently
this 8 patches series
by itself it's useless
it doesn't do anything
I have to go I'll be back in ~10 minutes
well, they cause no harm
(switching rooms)
but they cause no good either
they do, just you are not seeing
they ensure that all objects will have a common embedded struct...
that can be linked into a list...
base class so to say
and used with container_of to go to the main object
they provide a way to track topology changes...
as every object adition/removal calls a function
they provide a single point to add debug functions for object-related events (creation, removal, etc)
and they ensure a single ID
that is unique
tracking topology changes is a need for the API that was agreed at the MC
and so ensuring an unique ID
so, two of the pre-requisites for G_TOPOLOGY are fulfilled there
the addition/removal function is a need for MC graphs that are partially created/removed by different independent drivers, like ALSA and au0828
one needs to be notified by the other about topology changes
back
so, it provides for free a single point for a notification event for topology changes
I'm not seeing it because there's no user :-)
to give you an example
if I sent a patch set
there is
that refactored the DVB core code
buy yourself an au0828 based board and you'll see
telling you that it will be used by a future patch set
without providing it
you wouldn't ack that
even if the refactoring wasn't bad in itself
well, if you want, you may sit on your chair and wait for the entire patch series to be written
but then you'll likely need to allocate 2 weeks for review
exactly
that's why I'm advocating for a different approach
pinchartl: and yes, I did several acks and merge in the past with rework...
that would allow to merge support for interfaces soon
without having the final stuff
actually, this happens all the times...
get for example the V4L control framework...
it took years to get it used by all drivers
I guess the work didn't even finish yet
but it was used by at least one driver to start with
sometimes, we need to get faith that the one doing the work is doing it for good
it wasn't merged without knowing whether it would be usable
well, MC is used by more than one driver
YOUR 8 PATCHES ARE NOT USED
they are
I'm ujsing them right now:
[ 17.675218] au0828: Registered device AU0828 [Hauppauge HVR950Q]
[ 17.676358] usb 1-2.4: media_gobj_init: id 0x02000007 link#7: 'Xceive XC5000' pad#4 ==> 'au8522 7-0047' pad#1
[ 17.676362] usb 1-2.4: media_gobj_init: id 0x02000008 link#8: 'Xceive XC5000' pad#4 ==> 'au8522 7-0047' pad#1
[ 17.676365] usb 1-2.4: media_gobj_init: id 0x02000009 link#9: 'au8522 7-0047' pad#2 ==> 'au0828a video' pad#5
[ 17.676368] usb 1-2.4: media_gobj_init: id 0x0200000a link#10: 'au8522 7-0047' pad#2 ==> 'au0828a video' pad#5
[ 17.676371] usb 1-2.4: media_gobj_init: id 0x0200000b link#11: 'au8522 7-0047' pad#3 ==> 'au0828a vbi' pad#6
[ 17.676374] usb 1-2.4: media_gobj_init: id 0x0200000c link#12: 'au8522 7-0047' pad#3 ==> 'au0828a vbi' pad#6
[ 17.676426] usbcore: registered new interface driver au0828
unique IDs got greated - check
are you trying to drive me mad on purpose ?
debug functions work - check
...
no, I don't want you mad
we've been discussing this for too long. what's the conclusion ?
i guess we need to talk
and find some way for this to work
you prefer a top down approach, while I prefer a botton-up approach
on my experience, bottom-up approach works better when the core needs to be fixed
what can we try that we haven't tried it ?
that's what we did for VB2, control framework and other things
I fail to see why this can't work for MC
vb2 and the control framework were internal to the kernel
so it is the MC restrictions
MC is first a userspace API
we should first remove the MC internal barriers to do its job
and only after that come up with the userspace API
as the API should be stable
we can't remove barriers to do the job if we don't agree on what the job is
we can't really write it without having a precise idea about how the code will be implemented
and the job definition is the userspace API
pinchartl: that's why we spend 3 days in Espoo
we have a good understanding about the API
but before coding it, it is better do to the actual code, and see if it is doable or not...
in order to eventually adjust the API if needed
and to prove that what it was proposed will work or not
there are still lots of details that haven't been agreed on, and I believe they will shape the internal implementation
I think the opposite..
those details will only materialize if we start coding the internals
we fundamentally disagree then
yes
anyway, I guess with 3 or 4 patches more I'll have the interface support done
I can't stop you continuing with your current approach
and then I can start working at the driver changes and userspace API
but in that case I don't see why I should bother at all
as I said, if can't provide review without seeing the final result, then you can wait for everything to be ready... but I guess this is not the best way for doing it
the best way in my opinion would be to have a minimal self-contained patch set to implement media_interface
the media interface is not the big problem
it is just a new object
adding it won't fulfill the new API needs
don't try to solve all problems in one go
that's why I'm keeping this to the end
there are use cases for media_interface without dynamic MC support
first cleanup the house, then add a new floor
if the base is not robust enough and you try to add a new floor, the house will fall
I can't tell whether the base is robust enough without seeing how heavy the first floor will be
your patch set doesn't look bad to me, but it doesn't look good either
pinchartl: we know how heavy it will be as we drew a blueprint during the MC workshop
it's neutral
so, why are you refusing to ack?
because I don't know if it does the job as I don't know what the job will be
well, it does the job that it is proposed there
for example: The first 5 patches on this series ensures that all existing object types (entities, pads and links) will have an unique ID, as agreed at the MC workshop.
The next two patches add two debug functions, that helps with the tests of the MC changes. Both are enabled only if DEBUG or dynamic debug is enabled
we never agreed to give unique IDs to links
and even for pads it was still somehow an open question
it might be that we'll need unique IDs for links, or maybe not
I can't tell now
the API we drew use unique IDs for PADs
so I can't ack an infrastructure that gives unique IDs to links without knowing whether we'll need it
well, what harm it does to have an unique ID for links internally at the Kernel?
what harm would it be to add a new field called color to the graph object without having anything using it ?
none
but it doesn't no good eithe
r
and if it's useless we don't want it
well, if we won't need latter, this can easily be removed
just like we did with that "extra_links" field that we added at MC but never used on years
let's concern about cleaning it up after finishing the job
at least for now, a link ID is very useful, in order to debug the patches
[ 17.676358] usb 1-2.4: media_gobj_init: id 0x02000007 link#7: 'Xceive XC5000' pad#4 ==> 'au8522 7-0047' pad#1
[ 17.676362] usb 1-2.4: media_gobj_init: id 0x02000008 link#8: 'Xceive XC5000' pad#4 ==> 'au8522 7-0047' pad#1
(the above, for example, showing that both link/backlink got created, each one with its own ID)
I don't know what to tell you anymore. it seems we're not speaking the same language
if your problems is just if the link ID, I can get rid of that patch
if your problems is just *with* the link ID, I can get rid of that patch
that was an example. my problem is that I need to see where we're going in order to review the foundation patches
I can of course check for obvious and bike-shedding issues
but whether a core modification is needed or not, and whether it's done in the right way, I can't tell without seeing the big picture
a check for obvious bike-shedding issues will help
I can do that to show my good faith :-)
I'll do it today
if if they're either going on the right direction or not, I guess you have trust that they're going on the right direction. If not core changes done at the wrong way can always be reverted
but it won't be an ack
that's what I do when reviewing lots of patches
I trust that whomever is implementing something needs that for a reason
so, even when I can't clearly see where it will lead, if the patch is correct and won't cause regressions, I tend to apply
I trust that you need it for a reason and that you think it's the best solution, but I might disagree on what the best solution is
pinchartl: getting the best solution (for internal changes) is something that we don't try to do on a single patch series
the way upstream work is to commit often, commit asap
sorry
commit often, commit early
but not "merge asap"
that's one approach...
the best solution is achieved after a few rounds of commit
so, we first do a solution and then we improve it
and only then we push it upstream
pinchartl: actually no... we push things incrementally upstream too
VB2 conversion is taking years to happen
not in early development phases
especially for userspace APIs
that's why I'm leaving the userspace API to the end
to give us enough maturity with the internal code
I can't agree on an implementation without the userspace API
this way, when time comes, it will be easy to review the API
that's just a no-go
I have to go again, end of the session
I won't agree at the userspace API without having the proper internal support
as an untested API is wrong
by definition ;)
when did I ever say that I want the userspace API to be merged without drivers using it ?
drivers can only use the API if the internals are in good shape
back