Re: [vbox-dev] [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging
Hi, On 12-06-17 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 :( Nope, I removed all that already, unless you count things like: #define Assert(a) WARN_ON_ONCE(!(a)) Which are compatibility defines to get the osindependent code to build using Linux native constructs, rather then implementing a driver specific Assert function. 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 The driver specific malloc/free stuff is code to prepare an IPC packet, that code *used* to call a custom alloc routine, but that has been replaced with a genalloc.h functions. Maybe the functions need to be renamed as they do a lot more then just alloc mem (they also fill the header of the IPC packet). But really there are no custom malloc / assert / other-generic helper functions left I removed all those. 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. There is no more 43000 lines of OS abstraction layer which used to be there, what is left is osindependent/VBoxVideoIPRT.h which offers the osindepdent code things like Assert() which it needs, and is only 137 lines. Anyways I'm fine with cleaning this up, but I did the split at request of VirtualBox upstream, so I will let Michael weigh in here. Michael, are you ok with going all the way and cleaning up the few files still left under the osindependent dir ? Regards, Hans ___ vbox-dev mailing list vbox-dev@virtualbox.org https://www.virtualbox.org/mailman/listinfo/vbox-dev
Re: [vbox-dev] [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging
Hello Greg, 12.06.2017 13:47, Greg Kroah-Hartman wrote: On Mon, Jun 12, 2017 at 01:44:09PM +0200, 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 idea was that bits which are device-specific and which there would be no interest in sharing with other drivers in the kernel could stay in the VirtualBox coding style. Since most updates to this code are likely to come from us and it is shared with our drivers for other platforms, that would make it easier for the in-kernel driver maintainer to pull fixes from the VirtualBox tree. We tried to remove code from osindependent which would not fit that bill - notably, Hans reworked the driver to use a kernel gen_pool structure where it was previously using a VirtualBox-specific heap class. My understanding is that something similar was considered to be acceptable for the new AMD driver code. Of course, our code is much simpler than AMD's, so it would not be the end of the world if it were converted to kernel style, but I see more disadvantages than advantages to doing it. Only put stuff in staging for a good reason, and that reason can be "I don't know how to clean this stuff up", but I don't think that is the case here... And why are you cc:ing a mailing list that does not accept non-members to post? that's just annoying... Isn't dri-devel members only too? My messages seem to take a while to show up there, which would be explained by waiting for a moderator, though I don't get a warning that the message is waiting for approval. For now we have disabled that on vbox-dev too (I assume that was the list you meant). Regards Michael -- 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 ___ vbox-dev mailing list vbox-dev@virtualbox.org https://www.virtualbox.org/mailman/listinfo/vbox-dev
Re: [vbox-dev] [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging
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. 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... > Only put stuff in staging for a good reason, and that reason can be "I > don't know how to clean this stuff up", but I don't think that is the > case here... My main reason for submitting this for staging for now is that it needs to be moved to the atomic API which is non-trivial and will take a while, but if the drm subsys maintainers are willing to take it as is with the plan to convert it while it is already under drivers/gpu/drm that is even better :) Another reason is that I'm only somewhat familiar with the drm subsys, so this needs a good review which may result in other things which need fixing too. Regards, Hans ___ vbox-dev mailing list vbox-dev@virtualbox.org https://www.virtualbox.org/mailman/listinfo/vbox-dev
