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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
