On 02/15/2015 11:27 AM, Mauro Carvalho Chehab wrote: > Em Sat, 14 Feb 2015 12:43:30 +0100 > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu: > >> On 02/14/2015 12:00 PM, Mauro Carvalho Chehab wrote: >>> Em Sat, 14 Feb 2015 10:32:21 +0100 >>> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu: >>> >>>> On 02/13/2015 11:57 PM, Mauro Carvalho Chehab wrote: >>>>> This patch series adds basic support for the media controller at the >>>>> DVB core: it creates one media entity per DVB devnode, if the media >>>>> device is passed as an argument to the DVB structures. >>>>> >>>>> The cx231xx driver was modified to pass such argument for DVB NET, >>>>> DVB frontend and DVB demux. >>>>> >>>>> - >>>>> >>>>> version 4: >>>>> >>>>> - Addressed the issues pointed via e-mail >>>> >>>> No, you didn't. Especially with regards to the alsa node definition. I'm >>>> pretty sure you need at least the subdevice information which is now removed. >>> >>> Well, back on Jan, 26 I answered your issues about that at: >>> http://www.spinics.net/lists/linux-media/msg85857.html >>> >>> As you didn't reply back in a reasonable amount of time, I assumed that >>> you're happy with that. >>> >>> In any case, the definitions are still there, as nothing got dropped >>> from the external header. >>> >>> So, when ALSA media controller support will be added at the Kernel, we >>> can decide if it will use major/minor or card/device/subdevice or both. >>> >>> As I said back in Jan, 26, IMO, the best would be to use both: >>> >>> struct media_entity_desc { >>> ... >>> union { >>> struct { >>> u32 major; >>> u32 minor; >>> } dev; >>> /* deprecated fields */ >>> ... >>> } >>> union { >>> struct { >>> u32 card; >>> u32 device; >>> u32 subdevice; >>> } alsa_props; >>> __u8 raw[172]; >>> } >>> } >>> >>> (additional and deprecated fields removed just to simplify its >>> representation above) >>> >>> Even for ALSA, it is a way easier for libmediactl.c to keep using >>> major/minor to get the device node name via both udev/sysfs than >>> using anything else, as I don't think that udev has any method to >>> find the associated name without major,minor information. Ok, there >>> are indirect methods using the ALSA API to get such association, but >>> it is just easier to fill everything at the struct than to add the >>> extra complexity for the media control clients to convert between >>> major/minor into card/device/subdevice. >>> >>> What I'm saying is that the card/device/subdevice really seems to be >>> an extra property for this specific type of devnode, and not a >>> replacement. >>> >>> In any case, I think we should take the decision on how to properly >>> map the ALSA specific bits when we merge ALSA media controller patches, >>> and not before. >>> >>>> I also do *not* like the fact that you posted a v4 and immediately applied >>>> these patches to the master without leaving any time for more discussions. >>>> >>>> These patches change the kernel API and need to go to proper review and need >>>> a bunch of Acks, Laurent's at the very minimum since he's MC maintainer. >>>> >>>> Please revert the whole patch series from master, then we can discuss this >>>> more. >>>> >>>> For the record, for patch 02/25: >>>> >>>> Nacked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> >>>> >>>> I do *not* agree with this API change. >>>> >>>> We can discuss this more on Monday. >>> >>> This hole series is for discussions for a long time (since the beginning of >>> January), without rejection, and its now starting to receive patches from >>> other authors. Keeping it OOT just makes harder to discuss and for people to >>> test. It is time to move on. >> >> No, it isn't. Laurent and myself actually discussed it during FOSDEM, and >> we did have concerns about the API changes. Since he's the MC maintainer I >> assumed he would get back to you, but I think he's been very busy since FOSDEM. >> In fact, I believe he attended the Linaro Connect last week, so it is very >> likely he will not have had time to do anything with what we discussed at >> FOSDEM. > > No, I hadn't any return from that, nor by e-mail or IRC, except for Devin's > emails saying he's ok, provided that we'll also look at the cases with > multiple DVB adapters. I have already some patches covering it, but, in > order to discuss, it is important to merge the work I have already done, > plus the patches from Rafael, as my patches are in the top of his ones. > >> Patches changing the MC API need his Ack at minimum. You can't just post a >> v4 patch series and commit the same day. Not when it changes APIs and especially >> not since there were concerns raised about it in the past. You should have >> pinged Laurent and probably myself and ask for comments on v4. > > My understanding from the last discussions on IRC with Laurent is that, while > he prefers to not get this merged for 3.20, he is ok if I do that. So, I > took the decision of waiting till the merge of the patches for 3.20 to be > merged and adding those patches for 3.21, giving us 2 Kernel cycles to > do a deeper review. > > This is basically the same thing I do with all API changes: try to merge > them before -rc1 (or at early -rc) at our development tree, in order to > let all of us to test and to fix needed things before it is too late. > >>> >>> As I said on IRC, as I opted to merge it for 3.21, we'll have 2 entire >>> Kernel cycles to make it mature before being merged upstream. During that >>> period, we can fix any issues on it. >> >> That's not the way things are supposed to work. You wouldn't merge patches >> from us in a similar situation, and quite rightly. I know, we've tried :-) > > No, we did fix API bits on stuff already merged on our development tree. > More than that, we did changes even after merging them upstream, during > -rc Kernels, including API variable renames. Several of such changes were > even done or requested by you ;) Of course, if we realize we missed something, then we change it. But when these API changes were merged, it had our acks. The point is that a post-and-commit is simply bad form, especially since the post contained new patches not posted before, and especially if it contains API changes. Occasionally we do a post-and-commit, but that's usually reserved for fixing compiler warnings or stupid bugs/regressions that we missed and that need to be addressed urgently. > One such example is changeset 87185c958de9cd4acd8392f00d6161f4e11807ff > wich was changed at v3.14-rc5-379-g87185c958de9. > > For the record, the issue we're suffering with the MC is basically because > we didn't follow the proper procedure when MC got merged. > > The golden rule is that we don't add UAPIs at the Kernel without any > driver using it. However, for MC, we added support for DVB, FB and ALSA > before having any driver using. > > That is the source of the problems we're needing to fix now, as just > patching the old structures could cause media controller userspace > applications to not build, so we're forced to deprecate the earlier > bad design decisions, in favor to something that would actually work. I absolutely agree that we should never have added dvb/fb/alsa support at that time. That was stupid in hindsight. > Thankfully, FB is doing the right thing: using major/minor for devnodes, > but ALSA and DVB designs took a risky path. > > In the case of DVB, there are as just an "ID" is meaningless there, as > a DVB device needs at least an adapter ID plus a "type" ID in order > to uniquely specify a device instance. For example, the DVB demux > devnodes are /dev/dvb/adapter?/demux?, if there's no udev rule renaming > it to something else. > > The big issue of using an adapter_id/type_id, however, and the same applies > to ALSA using card/device/subdevice, is that udev may have rules renaming > the device node to something else. As udev knows nothing about DVB (or > ALSA or whatever), we need to get the device's major/minor, in order to > check what's the name of the devnode he actually created. > > So, as properly pointed inside the media-ctl source code, the right and > direct way to get the path name associated with a device node is to use > its major/minor and ask libudev about what's the path associated with > that given device. > > In any case, for ALSA, we should do the right thing here: remove (actually > deprecate) whatever definition is there, and then re-add it only when we > actually have the patches inside the ALSA subsystem to support the media > controller, plus having the corresponding patches for the media-ctl in order > to support the devnode discovery using both udev and sysfs for their nodes. I actually thought about how alsa should be handled and it is doing the right thing. See my patch that I posted today, partially reverting your patch. > >> Just revert, try to setup an irc session next week, based on that post a v5 >> and you are likely able to merge this within 2 weeks. Everyone agrees with >> *what* you want to do, just not about some of the details. > > I'm in a holiday this week up to Tuesday. Even if I want, I can't revert > it right now. Let's schedule a meeting on Wed via IRC, in order to discuss > it. > > In the mean time, if you have any patches that reflecting the approach > you think it is more appropriate, please feel free to send it to ML > for discussions. I'll try to take a look at my mailbox from time to time > during this holiday. Done. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
