On Tue, Jun 13, 2017 at 01:50:16PM +0200, Michael Thayer wrote: > 12.06.2017 18:03, Greg Kroah-Hartman wrote: > > On Mon, Jun 12, 2017 at 05:40:21PM +0200, Hans de Goede wrote: > >> Hi, > >> > >> On 12-06-17 13:44, Greg Kroah-Hartman wrote: > >>> On Mon, Jun 12, 2017 at 12:07:41PM +0200, Hans de Goede wrote: > >>>>> The most important thing is for the driver to be atomic if it's KMS > >>>>> only, and it would be good to have someone review that properly. > >>>> > >>>> I believe it does not use the atomic APIs atm, so that would be one > >>>> of the first things to fix then. Another question is if people > >>>> (you and Daniel at least) can live with the non kernel-coding > >>>> style shared files under the osindependent dir ? > >>> > >>> Why not just spend a few days and fix up all of the kernel-style issues > >>> so it can be a "real" driver? It shouldn't take all that long, > >>> especially for someone with Linux kernel experience (hint, hint...) > >> > >> The intention of the stuff below the osindepedent dir is for it to > >> be shared 1:1 between vboxvideo driver implementations for different > >> operating-systems. IIRC during the AMD DAL discussion Daniel indicated > >> that some OS independent code was fine (and would be exempt from coding > >> style rules) as long as it had a reasonable clean interface and was not > >> re-implementing anything we already have in the kernel. > > > > In a quick glance at the code in there, there's lots of reimplementing > > happening :( > > > > Maybe keep the data structures around, but really, you write those once, > > and then that's it, they should never change, so it shouldn't matter > > what format they are in. > > > >> If Daniel's verdict is that this needs to be cleaned up, then sure I > >> can easily do that. As mentioned I already did a lot of cleanup, > >> including moving all the other files to the kernel coding-style and > >> removing about 43000 lines of portability cruft / abstraction layers, > >> what is left under the osindependent directory is just C-structure > >> definitions and a few small plain C helper functions, which VirtualBox > >> upstream would like to keep as is... > > > > wrappers for simple things should not be needed at all, come on, you > > know that. We don't like driver-specific malloc/free and in/out > > functions, or asserts, or other crap like that. This implies that this > > driver is an island in itself and somehow more "important" than the 12+ > > million other lines of code that it lives within. Which isn't true. > > > > Just clean it up, that will make it even smaller, which in the end, is > > what really matters, as that will make it easier to maintain, fix, port > > to new apis, and everything else. > > > > There's a good reason why we don't have "os abstraction" layers in > > drivers in Linux, please don't ignore our history and knowledge here for > > no good reason. > > I would appreciate getting Dave and/or Daniel's feedback here, if they > have time. Given the size of the driver here, it seems like a good candidate for a drm-misc "small driver". So while I'm not named Dave or Daniel, I'll add my 2 cents. First, thank you for your submission. > In particular, they might have ideas about how to further > reduce the abstraction surface. In the end, the greater the difference > between the code in the kernel, the harder it is for people to pull > changes from our code to the kernel, and most of the changes in that > part are likely to come from our side. I don't think that converting > non-Linux-specific code in our tree to kernel style would go down well > in our team, so if the code as is is not acceptable, the next question > is, what would be? If we can come up with something which is alright > for both modulo a sed script that would still make people's lives > somewhat easier (though just the different brace style throws a bit of a > spanner in that). It's probably counterproductive to use AMD as a sterling example of getting a driver upstream. As you've mentioned, their driver is *much* larger than yours and there was still a fair amount of consternation about the os independent bits. I took a quick skim through your driver, and there doesn't seem to be much secret sauce there that will be tough to keep up-to-date across platforms. One other concern I have is that I noticed there are a few functions declared/defined in the osindependent code that are never used outside of it, so we have dead code off the hop. > > Please be clear, I am not trying to dictate to anyone. The code is GPL, > it is fine to include it with whatever modifications you deem > appropriate. Since individual distributions are already doing that, it > will still simplify our life somewhat if it is in the upstream kernel, > in whatever form, rather than in multiple forks. > > > Remember, this code needs us, we don't need this code at all :) > > I assume that that was not meant that way, but that came over as > slightly unfriendly to me. I am assuming here that we are looking for > the best way to do something which will be useful to a lot of people. > IMO, in order to accept the driver in drm, the osindependent code needs to be stripped out and it needs to be converted to atomic. Whether you want to do this out of tree, or in staging is up to you. As Dave mentioned, people often overlook staging when making drm core changes, so you'll likely face the same moving target issues either way. Sean > Regards > Michael > > > good luck! > > > > greg k-h > > > -- > Michael Thayer | VirtualBox engineer > ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt > > ORACLE Deutschland B.V. & Co. KG > Hauptverwaltung: Riesstraße 25, D-80992 München > Registergericht: Amtsgericht München, HRA 95603 > > Komplementärin: ORACLE Deutschland Verwaltung B.V. > Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister > der Handelskammer Midden-Nederland, Nr. 30143697 > Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
