Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Michael Thayer]

13.06.2017 17:41, Greg Kroah-Hartman wrote:
[...]
> And why is the closed vbox-devel list still on the cc: here, the bounces
> from it are getting annoying.
[...]

Not sure why you are still getting them, but we added you to the
white-list as well.  I would prefer the list to stay on as the subject
is relevant to it.

Regards, and sorry for the annoyance
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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Michael Thayer]

Hello Hans,

14.06.2017 15:30, Hans de Goede wrote:
[Discussion of vboxvideo and vboxguest driver clean-up.]
> As I already mentioned in previous mails on this, for the vboxguest driver
> my plan is to simply do a fork and remove anything related to the
> portability. It currently weighs in at 10+ lines of code which is a bit
> much for what it does I believe I will be able to get a Linux only version
> of it down to a small fraction of that and the result will be a much
> cleaner
> and better driver.
> 
> FWIW I've already stared looking into cleaning up the vboxguest driver and
> my first target is to remove all dependencies on the r0drv code.

Moving off-topic, but please let me know if you set up a git repository
for it as you did for vboxvideo.  Do you have plans (applies to
vboxvideo too) for how to merge changes to our code into the cleaned-up
code?  Of course, I am open to patches or suggestions as to how to
simplify the code in our repository as long as they do not affect other
platforms (vboxguest builds and runs for five different operating system
kernels).

Regards
Michael

> Regards,
> 
> Hans

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

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Michael Thayer]

Hello Sean,

13.06.2017 20:03, Sean Paul wrote:
[Discussion of vboxvideo driver clean-up.]

> First, thank you for your submission.

Thank you for your feedback.

[Discussion of the OS-independent code in the driver submission.]

> 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.

I have two particular concerns there: first if we add new functionality
(which we would do out of tree first) it will need porting over.
Acceleration support is the most likely candidate.  And if someone does
make fixes to that part of the code in the kernel tree they will also
need porting over.  I agree, that concern is probably overblown, and
best addressed by keeping that part of the code as close to our tree as
possible while still meeting kernel standards (hence my question as to
what that would be).

The second concern is not relevant to DRI, but it concerns our other
guest drivers (not the host one with the C++ in it Greg!) which Hans
also expressed interest in putting upstream after seeing how vboxvideo
fares.  The OS-independent part is quite a bit larger and more volatile,
though it has thankfully stablised a lot.  That concern is probably also
overblown, though I do wonder whether upstreaming those driver is the
best solution (that would be Hans's call though).

> 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.

If the OS-independent part gets converted then they would be removed in
the process.  Thank you for the reminder.

[...]

> 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.

Conversion to Atomic would probably have to happen at some time or
another anyway.  I have put that off (out-of-tree) so far because I was
tracking the AST driver as closely as possible as the simplest way of
picking up fixes, and because Dave, who wrote that, knows much more
about drm drivers than me.

[...]

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

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Michael Thayer]

14.06.2017 16:42, Sean Paul wrote:
[Discussion of vboxvideo driver kernel integration and clean-up.]
> Once the code is upstream, it's going to be difficult to motivate developers 
> to
> keep the driver close to your downstream version. If I'm working on feature X
> which touches your driver, I'm not going to figure out how your internal 
> version
> works so that I might ease your pain. I don't mean that to be rude, just
> realistic.

I would not expect many changes to happen to the vboxvideo-specific
parts of the driver - what is currently in osdependent -  at least not
without someone talking to me about it first.  If changes happen to the
other parts I would expect to monitor that myself and integrate them
into our out-of-tree driver at regular intervals.  Up until now I have
been tracking the AST driver in this way - not as diligently as I should
have been.  I would expect this to be made easier, not harder if I
switch to tracking in-kernel vboxvideo.  Perhaps I will be proven wrong,
but I do not expect much to be made in the way of changes that will not
make sense for me to integrate, if only because keeping the difference
as small as possible will make sense for me.

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

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Hans de Goede]

Hi,

On 14-06-17 15:40, Michael Thayer wrote:

Hello Hans,

14.06.2017 15:30, Hans de Goede wrote:
[Discussion of vboxvideo and vboxguest driver clean-up.]

As I already mentioned in previous mails on this, for the vboxguest driver
my plan is to simply do a fork and remove anything related to the
portability. It currently weighs in at 10+ lines of code which is a bit
much for what it does I believe I will be able to get a Linux only version
of it down to a small fraction of that and the result will be a much
cleaner
and better driver.

FWIW I've already stared looking into cleaning up the vboxguest driver and
my first target is to remove all dependencies on the r0drv code.


Moving off-topic, but please let me know if you set up a git repository
for it as you did for vboxvideo.


Will do.


Do you have plans (applies to
vboxvideo too) for how to merge changes to our code into the cleaned-up
code?


The git repo will have an "upstream" branch which will contain the vboxguest
dir from the tarbal created by the src/VBox/Additions/linux/export_modules
script. My plan is to sync that branch regularly by coping over a fresh
copy of that dir, then commit that as a commit on top of the previous
sync and look at the git diff to see what changed. Then where necessary
I will port over the changes to the cleaned up version.

>Of course, I am open to patches or suggestions as to how to

simplify the code in our repository as long as they do not affect other
platforms (vboxguest builds and runs for five different operating system
kernels).

Regards
Michael


Regards,

Hans



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Sean Paul]

On Wed, Jun 14, 2017 at 11:34:10AM +0200, Michael Thayer wrote:
> Hello Sean,
> 
> 13.06.2017 20:03, Sean Paul wrote:
> [Discussion of vboxvideo driver clean-up.]
> 
> > First, thank you for your submission.
> 
> Thank you for your feedback.
> 
> [Discussion of the OS-independent code in the driver submission.]
> 
> > 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.
> 
> I have two particular concerns there: first if we add new functionality
> (which we would do out of tree first) it will need porting over.
> Acceleration support is the most likely candidate.  And if someone does
> make fixes to that part of the code in the kernel tree they will also
> need porting over.  I agree, that concern is probably overblown, and
> best addressed by keeping that part of the code as close to our tree as
> possible while still meeting kernel standards (hence my question as to
> what that would be).

Once the code is upstream, it's going to be difficult to motivate developers to
keep the driver close to your downstream version. If I'm working on feature X
which touches your driver, I'm not going to figure out how your internal version
works so that I might ease your pain. I don't mean that to be rude, just
realistic.

Sean

> 
> The second concern is not relevant to DRI, but it concerns our other
> guest drivers (not the host one with the C++ in it Greg!) which Hans
> also expressed interest in putting upstream after seeing how vboxvideo
> fares.  The OS-independent part is quite a bit larger and more volatile,
> though it has thankfully stablised a lot.  That concern is probably also
> overblown, though I do wonder whether upstreaming those driver is the
> best solution (that would be Hans's call though).
> 
> > 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.
> 
> If the OS-independent part gets converted then they would be removed in
> the process.  Thank you for the reminder.
> 
> [...]
> 
> > 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.
> 
> Conversion to Atomic would probably have to happen at some time or
> another anyway.  I have put that off (out-of-tree) so far because I was
> tracking the AST driver as closely as possible as the simplest way of
> picking up fixes, and because Dave, who wrote that, knows much more
> about drm drivers than me.
> 
> [...]
> 
> 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

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Hans de Goede]

Hi all,

On 14-06-17 11:34, Michael Thayer wrote:

Hello Sean,

13.06.2017 20:03, Sean Paul wrote:
[Discussion of vboxvideo driver clean-up.]


First, thank you for your submission.


Thank you for your feedback.

[Discussion of the OS-independent code in the driver submission.]


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.


I have two particular concerns there: first if we add new functionality
(which we would do out of tree first) it will need porting over.
Acceleration support is the most likely candidate.  And if someone does
make fixes to that part of the code in the kernel tree they will also
need porting over.  I agree, that concern is probably overblown, and
best addressed by keeping that part of the code as close to our tree as
possible while still meeting kernel standards (hence my question as to
what that would be).

The second concern is not relevant to DRI, but it concerns our other
guest drivers (not the host one with the C++ in it Greg!) which Hans
also expressed interest in putting upstream after seeing how vboxvideo
fares.  The OS-independent part is quite a bit larger and more volatile,
though it has thankfully stablised a lot.  That concern is probably also
overblown, though I do wonder whether upstreaming those driver is the
best solution (that would be Hans's call though).


As I already mentioned in previous mails on this, for the vboxguest driver
my plan is to simply do a fork and remove anything related to the
portability. It currently weighs in at 10+ lines of code which is a bit
much for what it does I believe I will be able to get a Linux only version
of it down to a small fraction of that and the result will be a much cleaner
and better driver.

FWIW I've already stared looking into cleaning up the vboxguest driver and
my first target is to remove all dependencies on the r0drv code.

Regards,

Hans
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Michael Thayer]

13.06.2017 15:59, Greg Kroah-Hartman wrote:
> On Tue, Jun 13, 2017 at 03:45:14PM +0200, Michael Thayer wrote:
>> 13.06.2017 14:48, Greg Kroah-Hartman wrote:
>> [Discussion of vboxvideo coding style.]
>>> Once your code is accepted into the main kernel tree, why would you
>>> continue to work in an out-of-tree repo anyway?  That's ripe for
>>> disaster, what's keeping you from just working with the in-tree version?
>>
>> One of our use cases is customers running older operating systems,
>> including legacy Linux distributions.  However those customers would
>> still like the most up-to-date guest drivers possible without updating
>> the kernel.  Updating drivers without updating the kernel is not a
>> scenario of interest to upstream kernel developers, which is why we will
>> continue to maintain the out-of-tree repository (which is actually the
>> VirtualBox repository, where the OS-independent code is shared with
>> drivers for other platforms).  The end result is not unlike what Red Hat
>> does when it does back-ports to its stable kernels.
> 
> When Red Hat backports upstream drivers to older kernels, they do not do
> so in a way that is a different coding style or anything like that.
> They take the existing code, apply some rules and modifications to it,
> and complete the backport.  Some drivers can be done "automagically"
> using some good transformation tools that people have developed.
> 
> In fact, its even easier to do this if the code is upstream.  Just keep
> a local copy of the latest upstream version, with a "linux_backport.h"
> type file to handle the api changes.  Lots of people do that, and I
> myself did it for many years while working on different driver
> subsystems that had to ship in older kernels.
> 
> Here's one example of this type of a file that I helped work on:
>   https://github.com/gregkh/greybus/blob/master/kernel_ver.h
> that covered 5-6 different driver subsystems having to track all kernel
> versions from 3.10 to the latest 4.9 release (at that point in time, the
> code got merged upstream.)
> 
> Feel free to copy the same idea for your code, if it's applicable, other
> drivers do this for their "customers" as well.
> 
> Note, none of this requires "os abstractions" on the upstream code at
> all, nor does it require a different coding style at all, that would
> just hinder development and cause massive problems when trying to
> determine what changed upstream.

The reason we have OS abstraction is that we share code between drivers
for different platforms.  There is always going to be code in a driver -
mainly the actual programming of the hardware - which needs to work the
same way on different platforms, but which is not likely to be shared
with other drivers on the same platform.  I personally think that this
is a sensible thing to do, and a discussion that is worth having, in
this case how to reap the benefits (for Linux too) of sharing among
platforms without imposing significant costs on the Linux kernel; but I
do agree that this driver is too small to be worth having a long
argument about.

Regards
Michael

> This isn't the first time we've done this, some of us have a bit of
> experience in operating system development :)
> 
> thanks,
> 
> 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Michael Thayer]

13.06.2017 14:48, Greg Kroah-Hartman wrote:
[Discussion of vboxvideo coding style.]
> Once your code is accepted into the main kernel tree, why would you
> continue to work in an out-of-tree repo anyway?  That's ripe for
> disaster, what's keeping you from just working with the in-tree version?

One of our use cases is customers running older operating systems,
including legacy Linux distributions.  However those customers would
still like the most up-to-date guest drivers possible without updating
the kernel.  Updating drivers without updating the kernel is not a
scenario of interest to upstream kernel developers, which is why we will
continue to maintain the out-of-tree repository (which is actually the
VirtualBox repository, where the OS-independent code is shared with
drivers for other platforms).  The end result is not unlike what Red Hat
does when it does back-ports to its stable kernels.

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

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Michael Thayer]

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.  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).

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.

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

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Sean Paul]

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 

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Greg Kroah-Hartman]

On Tue, Jun 13, 2017 at 05:00:15PM +0200, Michael Thayer wrote:
> 13.06.2017 15:59, Greg Kroah-Hartman wrote:
> > On Tue, Jun 13, 2017 at 03:45:14PM +0200, Michael Thayer wrote:
> >> 13.06.2017 14:48, Greg Kroah-Hartman wrote:
> >> [Discussion of vboxvideo coding style.]
> >>> Once your code is accepted into the main kernel tree, why would you
> >>> continue to work in an out-of-tree repo anyway?  That's ripe for
> >>> disaster, what's keeping you from just working with the in-tree version?
> >>
> >> One of our use cases is customers running older operating systems,
> >> including legacy Linux distributions.  However those customers would
> >> still like the most up-to-date guest drivers possible without updating
> >> the kernel.  Updating drivers without updating the kernel is not a
> >> scenario of interest to upstream kernel developers, which is why we will
> >> continue to maintain the out-of-tree repository (which is actually the
> >> VirtualBox repository, where the OS-independent code is shared with
> >> drivers for other platforms).  The end result is not unlike what Red Hat
> >> does when it does back-ports to its stable kernels.
> > 
> > When Red Hat backports upstream drivers to older kernels, they do not do
> > so in a way that is a different coding style or anything like that.
> > They take the existing code, apply some rules and modifications to it,
> > and complete the backport.  Some drivers can be done "automagically"
> > using some good transformation tools that people have developed.
> > 
> > In fact, its even easier to do this if the code is upstream.  Just keep
> > a local copy of the latest upstream version, with a "linux_backport.h"
> > type file to handle the api changes.  Lots of people do that, and I
> > myself did it for many years while working on different driver
> > subsystems that had to ship in older kernels.
> > 
> > Here's one example of this type of a file that I helped work on:
> > https://github.com/gregkh/greybus/blob/master/kernel_ver.h
> > that covered 5-6 different driver subsystems having to track all kernel
> > versions from 3.10 to the latest 4.9 release (at that point in time, the
> > code got merged upstream.)
> > 
> > Feel free to copy the same idea for your code, if it's applicable, other
> > drivers do this for their "customers" as well.
> > 
> > Note, none of this requires "os abstractions" on the upstream code at
> > all, nor does it require a different coding style at all, that would
> > just hinder development and cause massive problems when trying to
> > determine what changed upstream.
> 
> The reason we have OS abstraction is that we share code between drivers
> for different platforms.  There is always going to be code in a driver -
> mainly the actual programming of the hardware - which needs to work the
> same way on different platforms, but which is not likely to be shared
> with other drivers on the same platform.  I personally think that this
> is a sensible thing to do, and a discussion that is worth having, in
> this case how to reap the benefits (for Linux too) of sharing among
> platforms without imposing significant costs on the Linux kernel; but I
> do agree that this driver is too small to be worth having a long
> argument about.

Yes, please look at the size, you have 7k lines total for the whole
thing, how many could you possibly be sharing?

7k is really a trivial number of lines for a driver, well under half of
the size of the 8250 UART driver[1], making your driver 1/2 as complex
and difficult to maintain as a serial port driver :)

It's really not that much code at all you are quibbling over here, it
could have all been converted already to the correct format by either
one of us with all of the time we have spent writing emails back and
forth...

And why is the closed vbox-devel list still on the cc: here, the bounces
from it are getting annoying.

thanks,

greg k-h

[1] My standard unit of measurement, I need a short name for it one of
these days as I use it all the time.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Greg Kroah-Hartman]

On Tue, Jun 13, 2017 at 03:45:14PM +0200, Michael Thayer wrote:
> 13.06.2017 14:48, Greg Kroah-Hartman wrote:
> [Discussion of vboxvideo coding style.]
> > Once your code is accepted into the main kernel tree, why would you
> > continue to work in an out-of-tree repo anyway?  That's ripe for
> > disaster, what's keeping you from just working with the in-tree version?
> 
> One of our use cases is customers running older operating systems,
> including legacy Linux distributions.  However those customers would
> still like the most up-to-date guest drivers possible without updating
> the kernel.  Updating drivers without updating the kernel is not a
> scenario of interest to upstream kernel developers, which is why we will
> continue to maintain the out-of-tree repository (which is actually the
> VirtualBox repository, where the OS-independent code is shared with
> drivers for other platforms).  The end result is not unlike what Red Hat
> does when it does back-ports to its stable kernels.

When Red Hat backports upstream drivers to older kernels, they do not do
so in a way that is a different coding style or anything like that.
They take the existing code, apply some rules and modifications to it,
and complete the backport.  Some drivers can be done "automagically"
using some good transformation tools that people have developed.

In fact, its even easier to do this if the code is upstream.  Just keep
a local copy of the latest upstream version, with a "linux_backport.h"
type file to handle the api changes.  Lots of people do that, and I
myself did it for many years while working on different driver
subsystems that had to ship in older kernels.

Here's one example of this type of a file that I helped work on:
https://github.com/gregkh/greybus/blob/master/kernel_ver.h
that covered 5-6 different driver subsystems having to track all kernel
versions from 3.10 to the latest 4.9 release (at that point in time, the
code got merged upstream.)

Feel free to copy the same idea for your code, if it's applicable, other
drivers do this for their "customers" as well.

Note, none of this requires "os abstractions" on the upstream code at
all, nor does it require a different coding style at all, that would
just hinder development and cause massive problems when trying to
determine what changed upstream.

This isn't the first time we've done this, some of us have a bit of
experience in operating system development :)

thanks,

greg k-h
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Greg Kroah-Hartman]

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.  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).

Once your code is accepted into the main kernel tree, why would you
continue to work in an out-of-tree repo anyway?  That's ripe for
disaster, what's keeping you from just working with the in-tree version?

Note, this isn't anything new.  Hundreds of companies and thousands of
developers do this every year with Linux, they don't have out-of-tree
repos with different coding style rules, as consistent rules are there
for good reason (hint, our brains, you want us to fix your bugs, and we
want you to fix ours.)

> > 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.

It was meant that way as you are trying to do something that no other
developer group gets away with in the kernel tree, why is this tiny 8k
line driver so special?  I agree it is special to you, that that's
great, but please be aware of the larger picture here.  You will benefit
greatly from being in the kernel source tree (lines removed, bugs fixed,
api changes happen automatically, etc.)  All we ask is that you work on
the same level play ground as everyone else is.  To ask to use a special
coding style and have an os-independant wrapper is not "level" at all,
it insulates 

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Michael Thayer]

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

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Michael Thayer]

Hello Dave,

12.06.2017 09:27, Dave Airlie wrote:

On 12 June 2017 at 16:56, Hans de Goede  wrote:

This commit adds the vboxvideo drm/kms driver for the virtual graphics
card used in Virtual Box virtual machines to drivers/staging.

Why drivers/staging? This driver is already being patched into the kernel
by several distros, thus it is good to get this driver upstream soon, so
that work on the driver can be easily shared.

At the same time we want to take our time to get this driver properly
cleaned up before submitting it as a normal driver under drivers/gpu/drm,
putting this driver in staging for now allows both.

Note this driver has already been significantly cleaned up, when I started
working on this the files under /usr/src/vboxguest/vboxvideo as installed
by Virtual Box 5.1.18 Guest Additions had a total linecount of 52681
lines. The version in this commit has 7275 lines.

Of those 7275 lines 3980 lines are in an osindependent directory which
contains portable code which is shared with other guest platforms such as
C-structure definitions for the virtual graphics card protocol and helper
functions for using these structures to communicate with the card. Since
these files are shared they are in the standard Virtual Box upstream code
style and not in the kernel coding style. All files outside of the
osindependent directory fully adhere to the kernel coding style.



Just some questions,

Does the hw have acceleration support of some kind? this driver seems
be a modesetting only one, which is fine, just wondering if there are
plans to do more.


The device does support acceleration - what you looked at before 
starting on Virgl.  Currently it is only implemented for X11 and goes 
through the guest device without touching the graphics device at all, 
but it is also possible to pass the stream through the graphics device, 
which is what our Windows drivers do.  However, at this point we are not 
sure whether to implement support for this in the drm driver, or to do 
something new.



We haven't used staging for drm drivers for a short while for a few
reasons, the speed of iterating the kms APIs esp in the atomic area
means nobody remembers to build staging, then you end up with broken
staging, I'd like to wait for Daniel to get back from holidays to
consult on whether we should just put this in non-staging so we don't
lose it down the cracks.

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.


How big a change would atomic support be?  The reason I am asking is 
that our out-of-kernel version of the driver builds on all kernels from 
3.11 to 4.11 using ifdefs, and for obvious reasons - reducing the risk 
of breaking a particular kernel version - we try to minimise the 
differences.  We would continue maintaining our version even with the 
driver upstream, to be able to update the driver without updating the 
whole kernel, which is often desirable in virtual machine use cases. 
But we would also want our version to be as close as possible to the 
most recent kernel version so that we can pull fixes as easily as 
possible: generally the people making those fixes will know the drm 
subsystem much better than we do.


To be clear, I am not trying to argue here, just to get an idea for my 
own planning.


Regards
Michael



Dave.
--

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

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Hans de Goede]

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

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Greg Kroah-Hartman]

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.

Remember, this code needs us, we don't need this code at all :)

good luck!

greg k-h
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Hans de Goede]

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

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Greg Kroah-Hartman]

On Mon, Jun 12, 2017 at 02:46:54PM +0200, Michael Thayer wrote:
> 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.

Sorry, but no, in-kernel code has to follow the in-kernel style guide,
we do this for a very good reason (i.e. so others can fix your bugs.)

Why not just use the kernel style rules for your code as well?  That way
no need to be confused when switching between the two :)

> > > 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?

I don't get any bounces from it complaining, so I doubt it.  If it is,
that's the correct way to manage a mailing list, don't have it bounce
nasty messages back at you...

thanks,

greg k-h
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Dan Carpenter]

It's going to be basically impossible to keep the out of tree in sync
with the staging version because there are so many changes needed
everywhere.

Generally in the kernel we don't care about out of tree stuff.  So the
OS independent stuff is going to get completely re-worked.  It's
mechanical changes but it's a lot.

regards,
dan carpenter

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Greg Kroah-Hartman]

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

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Greg Kroah-Hartman]

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...)

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...

thanks,

greg k-h
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Hans de Goede]

Hi,

On 12-06-17 09:27, Dave Airlie wrote:

On 12 June 2017 at 16:56, Hans de Goede  wrote:

This commit adds the vboxvideo drm/kms driver for the virtual graphics
card used in Virtual Box virtual machines to drivers/staging.

Why drivers/staging? This driver is already being patched into the kernel
by several distros, thus it is good to get this driver upstream soon, so
that work on the driver can be easily shared.

At the same time we want to take our time to get this driver properly
cleaned up before submitting it as a normal driver under drivers/gpu/drm,
putting this driver in staging for now allows both.

Note this driver has already been significantly cleaned up, when I started
working on this the files under /usr/src/vboxguest/vboxvideo as installed
by Virtual Box 5.1.18 Guest Additions had a total linecount of 52681
lines. The version in this commit has 7275 lines.

Of those 7275 lines 3980 lines are in an osindependent directory which
contains portable code which is shared with other guest platforms such as
C-structure definitions for the virtual graphics card protocol and helper
functions for using these structures to communicate with the card. Since
these files are shared they are in the standard Virtual Box upstream code
style and not in the kernel coding style. All files outside of the
osindependent directory fully adhere to the kernel coding style.



Just some questions,

Does the hw have acceleration support of some kind? this driver seems
be a modesetting only one, which is fine, just wondering if there are
plans to do more.


Michael, can you answer this. How does the 3D accel stuff for vbox work?
Is that supported under Linux guests at all  ?


We haven't used staging for drm drivers for a short while for a few
reasons, the speed of iterating the kms APIs esp in the atomic area
means nobody remembers to build staging, then you end up with broken
staging, I'd like to wait for Daniel to get back from holidays to
consult on whether we should just put this in non-staging so we don't
lose it down the cracks.


Ok.


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 ?

Regards,

Hans
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Dave Airlie]

On 12 June 2017 at 16:56, Hans de Goede  wrote:
> This commit adds the vboxvideo drm/kms driver for the virtual graphics
> card used in Virtual Box virtual machines to drivers/staging.
>
> Why drivers/staging? This driver is already being patched into the kernel
> by several distros, thus it is good to get this driver upstream soon, so
> that work on the driver can be easily shared.
>
> At the same time we want to take our time to get this driver properly
> cleaned up before submitting it as a normal driver under drivers/gpu/drm,
> putting this driver in staging for now allows both.
>
> Note this driver has already been significantly cleaned up, when I started
> working on this the files under /usr/src/vboxguest/vboxvideo as installed
> by Virtual Box 5.1.18 Guest Additions had a total linecount of 52681
> lines. The version in this commit has 7275 lines.
>
> Of those 7275 lines 3980 lines are in an osindependent directory which
> contains portable code which is shared with other guest platforms such as
> C-structure definitions for the virtual graphics card protocol and helper
> functions for using these structures to communicate with the card. Since
> these files are shared they are in the standard Virtual Box upstream code
> style and not in the kernel coding style. All files outside of the
> osindependent directory fully adhere to the kernel coding style.
>

Just some questions,

Does the hw have acceleration support of some kind? this driver seems
be a modesetting only one, which is fine, just wondering if there are
plans to do more.

We haven't used staging for drm drivers for a short while for a few
reasons, the speed of iterating the kms APIs esp in the atomic area
means nobody remembers to build staging, then you end up with broken
staging, I'd like to wait for Daniel to get back from holidays to
consult on whether we should just put this in non-staging so we don't
lose it down the cracks.

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.

Dave.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel