DRM code reorganization

DRM code reorganization

[Jon Smirl]

>4) DRM code reorganization. There were several requests to reorganize
>DRM to more closely follow the Linux kernel guidelines. This
>reorganization should occur before new features are added.

What should be the model for reorganizing DRM? An obvious change is
eliminate the naming macros. 

Another is to change to a personality with helper library model like
fbdev has. All of the common DRM code would go into a library module.
Each card would then have a small device specific module which depends
on the library module for common code. 

ian: what about splitting the current memory management code into a
module that can be swapped for your new version?

Are other structure changes needed?




=====
Jon Smirl
jonsmirl@yahoo.com


		
__________________________________
Do you Yahoo!?
Yahoo! Mail is new and improved - Check it out!
http://promotions.yahoo.com/new_mail

Re: DRM code reorganization

[Ian Romanick]

Jon Smirl wrote:
>>4) DRM code reorganization. There were several requests to reorganize
>>DRM to more closely follow the Linux kernel guidelines. This
>>reorganization should occur before new features are added.
> 
> What should be the model for reorganizing DRM? An obvious change is
> eliminate the naming macros. 
> 
> Another is to change to a personality with helper library model like
> fbdev has. All of the common DRM code would go into a library module.
> Each card would then have a small device specific module which depends
> on the library module for common code. 

This would be *very* non-trivial to do.  Doing the DRM like this has 
come up probably a dozen times (or more) over the last 3 years.  The 
problem is that each driver has different needs based on hardware 
functionality.  We've managed to classify these needs into a few groups, 
and drivers can select which functionality they need via a set of 
defines.  These per-driver defines determine what code gets compiled 
into the different routines (as well as which routines even get built).

Trying to make this into a library would just be a mess.  It would 
either break cases where multiple DRMs are built (or loaded) into the 
kernel or result in a *ton* of unused, nearly duplicate routines being 
built into the library.

If this is something that we really want to pursue, someone needs to dig 
in and figure out:

1. How much / which code can be "trivially" shared?
2. How much / which code can be shared with very little work?
3. How much / which code would we rather get a root-canal than try to 
make shared?

The concern has been that, by making a "generic" library layer, we'd 
actually make the DRM more difficult to maintain.  Nobody has really had 
the time to do the research required to either substantiate or refute 
those claims.  Based on the little experience I have in the DRM, I tend 
to believe them.

> ian: what about splitting the current memory management code into a
> module that can be swapped for your new version?

AFAIK, the only drivers that have any sort of in-kernel memory manager 
are the radeon (only used by the R200 driver) and i830.  That memory 
manager only exists to support an NV_vertex_array_range-like extension 
that some Tungsten customers needed.  I don't think there would be any 
benefit to making that swappable.

Once the new memory manager is in, 80% (or more) of the code will be in 
user-mode anyway.  The code that will be in the kernel should be generic 
enough to be completely sharable (i.e., in a generic DRM library).

> Are other structure changes needed?

Re: DRM code reorganization

[Alan Cox]

On Llu, 2004-08-02 at 19:57, Dave Jones wrote:
>  > The problem is that each driver has different needs based on hardware 
>  > functionality.
> 
> How does this differ from any other subsystem that supports
> cards with features that may not be present in another model ?
> Other subsystems have dealt with this problem without the need
> to introduce horrors like the abstractions in DRM.

The abstractions are one big mistake IMHO. But in context their origin
is easy to understand. The original goal was to support a lot of
platforms and to minimise code writing. The Mesa layer uses this kind of
templating a lot and for the 3D client side code its a real win.

Someone I think took them a stage too far and into a place that it
didn't work out.

The memory manager is a bit more complex, a lot of drivers do have
different needs for memory management and some of it has to be client
side. Its also a really really hot path when doing direct render.

>  > AFAIK, the only drivers that have any sort of in-kernel memory manager 
>  > are the radeon (only used by the R200 driver) and i830.
> 
> ISTR SiS has some memory management code too, though I've not looked
> too closely at that for a long time.

SiS and VIA do as well. Both of them overdo the kernel side work and it
hurts performance however.

Alan

Re: DRM code reorganization

[Keith Whitwell]

>> ian: what about splitting the current memory management code into a
>> module that can be swapped for your new version?
> 
> 
> AFAIK, the only drivers that have any sort of in-kernel memory manager 
> are the radeon (only used by the R200 driver) and i830.  That memory 
> manager only exists to support an NV_vertex_array_range-like extension 
> that some Tungsten customers needed.  I don't think there would be any 
> benefit to making that swappable.
> 
> Once the new memory manager is in, 80% (or more) of the code will be in 
> user-mode anyway.  The code that will be in the kernel should be generic 
> enough to be completely sharable (i.e., in a generic DRM library).

Yes, the future is Ian's manager.  The existing ones are built to be discarded 
when something better comes along.

Keith

Re: DRM code reorganization

[Dave Jones]

On Mon, Aug 02, 2004 at 11:02:43AM -0700, Ian Romanick wrote:

 > This would be *very* non-trivial to do.  Doing the DRM like this has 
 > come up probably a dozen times (or more) over the last 3 years.

Which should ring alarm bells that something might not be quite right.

 > The problem is that each driver has different needs based on hardware 
 > functionality.

How does this differ from any other subsystem that supports
cards with features that may not be present in another model ?
Other subsystems have dealt with this problem without the need
to introduce horrors like the abstractions in DRM.
 
 > We've managed to classify these needs into a few groups, 
 > and drivers can select which functionality they need via a set of 
 > defines.  These per-driver defines determine what code gets compiled 
 > into the different routines (as well as which routines even get built).

And among other horrors, crap like typedef's that magically change their
type completely depending on which file they are #include'd into.
Overuse of typedefs is one thing, but what goes on with stuff like
DRIVER_BUF_PRIV_T is bordering on obscene.

If this kind of abuse wasn't so widespread, abstracting this code
out into shared sections and driver specific parts would be a lot
simpler. Sadly, this is the tip of the iceberg.

 > Trying to make this into a library would just be a mess.

Depends on which direction you're looking at things.
>From the view of many kernel developers, anything would be
better than the trainwreck of wrappers/macros/preprocessor abuse
that's there right now.  I'd put money on a lot more people being
prepared to hack on DRM's kernel code if it were more readable.

To give an example of just how bad some folks view on this code is:
An actual conversation at OLS last week..
"I found it easier to look at the C preprocessor output than the
 actual source code to find the types of the variables I was looking at".

 > If this is something that we really want to pursue, someone needs to dig 
 > in and figure out:
 > 
 > 1. How much / which code can be "trivially" shared?
 > 2. How much / which code can be shared with very little work?
 > 3. How much / which code would we rather get a root-canal than try to 
 > make shared?
 > 
 > The concern has been that, by making a "generic" library layer, we'd 
 > actually make the DRM more difficult to maintain.

I don't maintain upstream DRM, but I have a fair amount of responsibility
regarding the Fedora kernel, and I'll state publically that looking at
bugs in drivers/char/drm is right up there on my list of
"things I'd rather not do after lunch". Maintainability goes much
further than 'the guy that wrote the code can grok it'.

People trying to pick up 3d driver programming on Linux have a huge
hurdle in their place, as that code resembles *NO OTHER* driver code
in the tree.

 > Nobody has really had 
 > the time to do the research required to either substantiate or refute 
 > those claims.  Based on the little experience I have in the DRM, I tend 
 > to believe them.

I spent 2-3 days a few months back attempting to clean up some of the
goo, by peeling away the layers of abstraction.  After the 3rd day,
I'd estimate I wasn't even a fraction of the way through, and I lost
the will to keep fighting.

 > >ian: what about splitting the current memory management code into a
 > >module that can be swapped for your new version?
 > 
 > AFAIK, the only drivers that have any sort of in-kernel memory manager 
 > are the radeon (only used by the R200 driver) and i830.

ISTR SiS has some memory management code too, though I've not looked
too closely at that for a long time.

		Dave

Re: DRM code reorganization

[Ian Romanick]

Dave Jones wrote:

> On Mon, Aug 02, 2004 at 11:02:43AM -0700, Ian Romanick wrote:
> 
>  > This would be *very* non-trivial to do.  Doing the DRM like this has 
>  > come up probably a dozen times (or more) over the last 3 years.
> 
> Which should ring alarm bells that something might not be quite right.

And that it hasn't been done all those times should be a sign of 
*something*. ;)

>  > The problem is that each driver has different needs based on hardware 
>  > functionality.
> 
> How does this differ from any other subsystem that supports
> cards with features that may not be present in another model ?
> Other subsystems have dealt with this problem without the need
> to introduce horrors like the abstractions in DRM.

There are a couple of other things to consider.

1. There is a lot more variability among graphics cards that there is 
among, say, network cards.  Look at the output of 'grep __HAVE_ | grep 
define' on any two <card>.h files to see what I mean.  The output for 
tdfx.h and radeon.h, or mga.h and savage.h is *very* different.  That, 
by itself, makes a huge difference on what code is needed.

2. We have an extra dimension to our matrix.  Most other drivers don't 
need to worry about working on BSD.

3. The ever classic....It seemed like a good idea at the time.

>  > We've managed to classify these needs into a few groups, 
>  > and drivers can select which functionality they need via a set of 
>  > defines.  These per-driver defines determine what code gets compiled 
>  > into the different routines (as well as which routines even get built).
> 
> And among other horrors, crap like typedef's that magically change their
> type completely depending on which file they are #include'd into.
> Overuse of typedefs is one thing, but what goes on with stuff like
> DRIVER_BUF_PRIV_T is bordering on obscene.
> 
> If this kind of abuse wasn't so widespread, abstracting this code
> out into shared sections and driver specific parts would be a lot
> simpler. Sadly, this is the tip of the iceberg.

I think it comes down to the fact that the original DRM developers 
wanted templates.  C doesn't have them, so they did the "next best" thing.

>  > Trying to make this into a library would just be a mess.
> 
> Depends on which direction you're looking at things.
>>From the view of many kernel developers, anything would be
> better than the trainwreck of wrappers/macros/preprocessor abuse
> that's there right now.  I'd put money on a lot more people being
> prepared to hack on DRM's kernel code if it were more readable.
> 
> To give an example of just how bad some folks view on this code is:
> An actual conversation at OLS last week..
> "I found it easier to look at the C preprocessor output than the
>  actual source code to find the types of the variables I was looking at".

That's not surprising.

>  > If this is something that we really want to pursue, someone needs to dig 
>  > in and figure out:
>  > 
>  > 1. How much / which code can be "trivially" shared?
>  > 2. How much / which code can be shared with very little work?
>  > 3. How much / which code would we rather get a root-canal than try to 
>  > make shared?
>  > 
>  > The concern has been that, by making a "generic" library layer, we'd 
>  > actually make the DRM more difficult to maintain.
> 
> I don't maintain upstream DRM, but I have a fair amount of responsibility
> regarding the Fedora kernel, and I'll state publically that looking at
> bugs in drivers/char/drm is right up there on my list of
> "things I'd rather not do after lunch". Maintainability goes much
> further than 'the guy that wrote the code can grok it'.
> 
> People trying to pick up 3d driver programming on Linux have a huge
> hurdle in their place, as that code resembles *NO OTHER* driver code
> in the tree.

In all fairness, what's in the kernel does not constitute the guts of 
the 3D driver.  Few people that do 3D driver development ever need touch 
anything in the kernel.  That's by design.  I've been working on the 
open-source 3D drivers for 3 years, and I've made *maybe* 4 changes to 
the kernel code.

In this case that's a two edged sword.  The kernel parts have gotten to 
be like a container pushed too far to the back of the refridgerator for 
too long...

>  > Nobody has really had 
>  > the time to do the research required to either substantiate or refute 
>  > those claims.  Based on the little experience I have in the DRM, I tend 
>  > to believe them.
> 
> I spent 2-3 days a few months back attempting to clean up some of the
> goo, by peeling away the layers of abstraction.  After the 3rd day,
> I'd estimate I wasn't even a fraction of the way through, and I lost
> the will to keep fighting.

That's the core question.  Everyone *knows* that the current DRM code is 
an ugly mess, but is it worth the effort at this point to fix it?  You 
seem to have come to the same conclusion that everyone else that's 
looked at the problem over the last 2 years has.

Just to reiterate.  I am *NOT* saying that it's not a problem.  Nor am I 
saying that nothing should be done.  What I am saying is that it's a 
really big problem, and fixing it may not be worth the effort required.

I think the only realistic approach is to attack the problem bit by bit, 
rather than en masse.  Gradual elimination of the '#if __HAVE_FOO' and 
gradual datatype refactoring is really the only way to attack it.  The 
problem is just too big and there are just too few interested developers.

Re: DRM code reorganization

[Jon Smirl]

We are really short handed for kernel level DRM developers; most 3D
developers work in user space. The main person that wrote it, Gareth
Hughes, doesn't seem to work on it any more. Right now there are three
to four, non-paid people working part-time on DRM. 

How about you kernel developers working in other areas giving us a hand
with reorganizing the DRM code? You don't need to know anything about
3D you would just be reworking the code without changing how it
functions.

It would seem to me like companies counting on desktop Linux might want
to spend a few dollars on getting a better graphics sub-system.

=====
Jon Smirl
jonsmirl@yahoo.com


		
__________________________________
Do you Yahoo!?
Yahoo! Mail Address AutoComplete - You start. We finish.
http://promotions.yahoo.com/new_mail 

Re: DRM code reorganization

[Dave Jones]

On Mon, Aug 02, 2004 at 01:11:26PM -0700, Ian Romanick wrote:

 > > > This would be *very* non-trivial to do.  Doing the DRM like this has 
 > > > come up probably a dozen times (or more) over the last 3 years.
 > >Which should ring alarm bells that something might not be quite right.
 > And that it hasn't been done all those times should be a sign of 
 > *something*. ;)

heh. I'd attribute it to the fact that it's tedious monotonous work
doing cleanup work like this, as opposed to 'sexy' work, like hacking
on something new.  Personally, I've always found something more important
to be doing.  Maybe I can find some more time to look into it soon.

 > 1. There is a lot more variability among graphics cards that there is 
 > among, say, network cards.  Look at the output of 'grep __HAVE_ | grep 
 > define' on any two <card>.h files to see what I mean.  The output for 
 > tdfx.h and radeon.h, or mga.h and savage.h is *very* different.  That, 
 > by itself, makes a huge difference on what code is needed.

The __HAVE_ stuff is another pet gripe of mine.
In particular, the mish-mash of __HAVE_AGP , __REALLY_HAVE_AGP, __MUST_HAVE_AGP
flags have bugged me for a long time.

 > 2. We have an extra dimension to our matrix.  Most other drivers don't 
 > need to worry about working on BSD.

I'm hesitant to name them, but there are drivers in the tree which
whilst not shining examples of what-to-do, they do a far better
job of running on both OS's. Some with abstraction layers and such,
but with nothing remotely as convoluted as drm IMO.

 > 3. The ever classic....It seemed like a good idea at the time.

Reminds me of someone I know who starts amusing stories with the
sentence.. "So, we had a few beers and...", though after reading
some of that code I suspect something much stronger 8-)

 > >If this kind of abuse wasn't so widespread, abstracting this code
 > >out into shared sections and driver specific parts would be a lot
 > >simpler. Sadly, this is the tip of the iceberg.
 > 
 > I think it comes down to the fact that the original DRM developers 
 > wanted templates.  C doesn't have them, so they did the "next best" thing.

I vaguelly recall the code at one point not looking quite 'so bad',
it just grew and grew into this monster.  I'm sure it was done originally
with the best of intentions, but it seems someone along the line got
a bit carried away.

 > >To give an example of just how bad some folks view on this code is:
 > >An actual conversation at OLS last week..
 > >"I found it easier to look at the C preprocessor output than the
 > > actual source code to find the types of the variables I was looking at".
 > That's not surprising.

Maybe we can introduce that code as a starting point :-)

 > That's by design.  I've been working on the 
 > open-source 3D drivers for 3 years, and I've made *maybe* 4 changes to 
 > the kernel code.

Sure. One thing that the DRM code has actually done right is to move the
bits that need arbitration to the kernel, whilst leaving all the fun
bits in userspace. (well, mostly).

 > >I'd estimate I wasn't even a fraction of the way through, and I lost
 > >the will to keep fighting.
 > 
 > That's the core question.  Everyone *knows* that the current DRM code is 
 > an ugly mess, but is it worth the effort at this point to fix it?  You 
 > seem to have come to the same conclusion that everyone else that's 
 > looked at the problem over the last 2 years has.

I think with a coordinated effort, and with an ACK by the folks who maintain
this code on a day-to-day basis, it's worth at least heading in the right
direction.

 > I think the only realistic approach is to attack the problem bit by bit, 
 > rather than en masse.  Gradual elimination of the '#if __HAVE_FOO' and 
 > gradual datatype refactoring is really the only way to attack it.  The 
 > problem is just too big and there are just too few interested developers.

Sounds realistic.  As I mentioned in my last mail, my 'work on one driver
at a time, ridding it of macros etc' led to nothing mergable, and was only
just at the stage where it was compiling again. I'd warn off anyone trying
to do what I tried. I learned a few things along the way, but the end result
wasn't where any of us want things to be.

		Dave

Re: DRM code reorganization

[Dave Jones]

On Mon, Aug 02, 2004 at 01:42:04PM -0700, Jon Smirl wrote:
 > We are really short handed for kernel level DRM developers; most 3D
 > developers work in user space. The main person that wrote it, Gareth
 > Hughes, doesn't seem to work on it any more. Right now there are three
 > to four, non-paid people working part-time on DRM. 
 > 
 > How about you kernel developers working in other areas giving us a hand
 > with reorganizing the DRM code? You don't need to know anything about
 > 3D you would just be reworking the code without changing how it
 > functions.

Whip me, beat me, make me clean up drivers/char/drm

8-)

Seriously, it's not a fun job at all, so finding volunteers may be
somewhat difficult, but for someone with a high pain threshold, it might
be fun[1], but as Ian mentioned, it depends on the payoff.  If subsequent
DRI tree -> kernel merges back out any cleanup work, it's definitly going
to be a waste of time even trying.

Additionally, assuming some grand cleanup happens. Going back the other way
(keeping stuff in the DRI tree up to date with whatever kernel changes were
 made) is going to prove interesting. In short, I'd not expect other OS's to
work out-of-the-box until someone put in the legwork to make them adapt
to whatever changes were made.

Another possibility of course is that the BSD & Linux kernel side bits
go their seperate ways. How active is the kernel side of the BSD world ?

		Dave

[1] Well, fun perhaps for the same sort of person who enjoyed pulling legs
    off of insects as a child, to see if it still wiggles.

Re: DRM code reorganization

[Michel Dänzer]

On Mon, 2004-08-02 at 22:09 +0100, Dave Jones wrote:
> 
> If subsequent DRI tree -> kernel merges back out any cleanup work, it's 
> definitly going to be a waste of time even trying.

That can easily be avoided by doing the cleanup the right way in the
first place, i.e. in the DRI tree...


-- 
Earthling Michel Dänzer      |     Debian (powerpc), X and DRI developer
Libre software enthusiast    |   http://svcs.affero.net/rm.php?r=daenzer

Re: DRM code reorganization

[Jon Smirl]

--- Dave Jones <davej@redhat.com> wrote:
> Another possibility of course is that the BSD & Linux kernel side
> bits go their separate ways. How active is the kernel side of the 
> BSD world ?

I'll probably get flamed to death for suggesting this, but why not?

What about leaving BSD working with the current code and moving forward
on Linux alone? The thread "OLS and console rearchitecture" describes a
lot of changes to the graphics system. Some of these are much easier to
achieve on Linux than BSD since some the underlying operations are
supported in the Linux kernel and not in the BSD one. After we achieve
a new stable point, maybe the first X on GL release, we back port
everything to BSD.

One problem right now is that there are only one or two BSD developers,
all of the rest are Linux based. Most of the Linux based people have no
clue as to BSD's capabilities. Wouldn't it simply be more efficient to
put BSD on hold for a while and then resynch after everything has stabilized?

=====
Jon Smirl
jonsmirl@yahoo.com


		
__________________________________
Do you Yahoo!?
Read only the mail you want - Yahoo! Mail SpamGuard.
http://promotions.yahoo.com/new_mail 

Re: DRM code reorganization

[Alan Cox]

On Llu, 2004-08-02 at 22:09, Dave Jones wrote:
> Whip me, beat me, make me clean up drivers/char/drm
> 
> 8-)

Im sure that can be arranged by someone.

> Another possibility of course is that the BSD & Linux kernel side bits
> go their seperate ways. How active is the kernel side of the BSD world ?

Eric Anholt works actively on it.

Re: DRM code reorganization

[Alan Cox]

On Maw, 2004-08-03 at 00:48, Dave Airlie wrote:
> I'm going to agree with Ian here, making a generic library layer is going
> to make maintaining  the DRM a full time job as opposed to the 5-6 hrs a

Disagree. If the factoring is done right its probably easier to maintain
and you'll have more people able to maintain it. In addition because its
not "magic" more code scans for bugs and the like will
find stuff properly.

> week I do on it ... also the change to a library needs to be done in
> little steps, removing something from one place and adding it to another,
> a big code-drop from the DRI CVS is not going to be acceptable to anyone,
> however breaking things in the kernel tree is also not going to be...

Its going to have to happen to get video stuff working, to get hotplug
working. The question is when and how we do it

Re: DRM code reorganization

[Dave Airlie]

>
> If this is something that we really want to pursue, someone needs to dig in
> and figure out:
>
> 1. How much / which code can be "trivially" shared?
> 2. How much / which code can be shared with very little work?
> 3. How much / which code would we rather get a root-canal than try to make
> shared?
>
> The concern has been that, by making a "generic" library layer, we'd actually
> make the DRM more difficult to maintain.  Nobody has really had the time to do
> the research required to either substantiate or refute those claims.  Based on
> the little experience I have in the DRM, I tend to believe them.

I'm going to agree with Ian here, making a generic library layer is going
to make maintaining  the DRM a full time job as opposed to the 5-6 hrs a
week I do on it ... also the change to a library needs to be done in
little steps, removing something from one place and adding it to another,
a big code-drop from the DRI CVS is not going to be acceptable to anyone,
however breaking things in the kernel tree is also not going to be...

So if people are proposing a complete DRM re-write, then what I forsee has
to happen is, a new development tree is setup, the code is beaten into shape,
we rename the project something else like ghm (graphics hardware manager -
you heard it here first :-), we import it into the kernel and deprecate
the DRM in-kernel, maybe we can develop it in the kernel tree as
(EXPERIMENTAL), then everyone can give out about it while the work is in
progress as opposed to when we are finished it ...

Dave.

-- 
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
pam_smb / Linux DECstation / Linux VAX / ILUG person

Re: DRM code reorganization

[Ian Romanick]

Dave Jones wrote:

> On Mon, Aug 02, 2004 at 01:11:26PM -0700, Ian Romanick wrote:
> 
>  > > > This would be *very* non-trivial to do.  Doing the DRM like this has 
>  > > > come up probably a dozen times (or more) over the last 3 years.
>  > >Which should ring alarm bells that something might not be quite right.
>  > And that it hasn't been done all those times should be a sign of 
>  > *something*. ;)
> 
> heh. I'd attribute it to the fact that it's tedious monotonous work
> doing cleanup work like this, as opposed to 'sexy' work, like hacking
> on something new.  Personally, I've always found something more important
> to be doing.  Maybe I can find some more time to look into it soon.

If you're like me and most of the other developers, you've already got a 
to-do list a mile long.  For me hitting myself on the head with a hammer 
is pretty low. ;)

>  > 1. There is a lot more variability among graphics cards that there is 
>  > among, say, network cards.  Look at the output of 'grep __HAVE_ | grep 
>  > define' on any two <card>.h files to see what I mean.  The output for 
>  > tdfx.h and radeon.h, or mga.h and savage.h is *very* different.  That, 
>  > by itself, makes a huge difference on what code is needed.
> 
> The __HAVE_ stuff is another pet gripe of mine.
> In particular, the mish-mash of __HAVE_AGP , __REALLY_HAVE_AGP, __MUST_HAVE_AGP
> flags have bugged me for a long time.

The problem is that __REALLY_HAVE_FOO is usually just (__HAVE_FOO && 
CONFIG_FOO) on Linux.  They appear to be derived slightly differently on 
NetBSD and FreeBSD.  'grep __REALLY_HAVE drm_os_*bsd.h | grep define' in 
the bsd directory in the DRM tree.  Since there's just the three 
(__REALLY_HAVE_AGP, __REALLY_HAVE_SG, and __REALLY_HAVE_MTRR), I think 
we can live with them.

It shouldn't be too hard to get rid of __MUST_HAVE_AGP, though.

I think this is the right place to start.  A couple of these look easier 
to get rid of than others.  __HAVE_MTRR and __HAVE_AGP are enabled in 
every driver except ffb.  It should be easy enough to get rid of them. 
It looks like __HAVE_RELEASE, __HAVE_DMA_READY, __HAVE_DMA_FLUSH, 
__HAVE_DMA_QUIESCENT, and __HAVE_MULTIPLE_DMA_QUEUES (which looks broken 
anyway) should also be low-hanging fruit.

If we get that far, I think the next step would be to replace the 
DRIVER_* macros with a table of function pointers that would get passed 
around.  Since I doubt any of those uses are performance critical, that 
should be fine.

Then we can start looking at data structure refactoring.

>  > >If this kind of abuse wasn't so widespread, abstracting this code
>  > >out into shared sections and driver specific parts would be a lot
>  > >simpler. Sadly, this is the tip of the iceberg.
>  > 
>  > I think it comes down to the fact that the original DRM developers 
>  > wanted templates.  C doesn't have them, so they did the "next best" thing.
> 
> I vaguelly recall the code at one point not looking quite 'so bad',
> it just grew and grew into this monster.  I'm sure it was done originally
> with the best of intentions, but it seems someone along the line got
> a bit carried away.

There was a point when a *lot* of the device-dependent code was still in 
the OS-dependent directories.  This is how the i810 and i830 drivers 
still are.  I think as more of the code got moved into the 
OS-independent directory, it got less pleasant to read.

Re: DRM code reorganization

[Dave Airlie]

>
> How does this differ from any other subsystem that supports
> cards with features that may not be present in another model ?
> Other subsystems have dealt with this problem without the need
> to introduce horrors like the abstractions in DRM.

The biggest issue I have with all the other subsystems that do this, is
the re-writing of basic driver code *for every driver*, everyone has a PCI
table, with a routine called my_driver_probe, and my_driver_init and
my_driver_exit and when you start a new driver you have to go all
cut-and-paste on its ass, someone changes and interface, you've got to go
change 20 files...

The DRM doesn't suffer from this, we've made huge changes to the DRM pci
probing code for *all* drivers (it's in the -mm tree if you want to look)
by changing one file, I'd hate to see similiar work being done to the fb
tree, you'd be cut-n-pasting and changing names of functions and it would
be horrible drudge work, the DRM isn't a maintainers paradise but if you
learn how it works it is an awful lot easier to maintain than similiar
subsystems...

> And among other horrors, crap like typedef's that magically change their
> type completely depending on which file they are #include'd into.
> Overuse of typedefs is one thing, but what goes on with stuff like
> DRIVER_BUF_PRIV_T is bordering on obscene.
>

this sort of stuff could probably be cleaned up, I'm not so sure this
re-org everyone is pushing for isn't going to lead to people wishing it
was the old way again :-)

> >From the view of many kernel developers, anything would be
> better than the trainwreck of wrappers/macros/preprocessor abuse
> that's there right now.  I'd put money on a lot more people being
> prepared to hack on DRM's kernel code if it were more readable.
>

If we start putting fb type functionality into it more people will start
coming towards it alright... it would be nice if it was a bit more
homely..

I'm sitting on the fence at the moment, as I'm not going to be able to
contribute the amount of time for a big re-write I'm willing to help out
with small changes and keep track of what patches go where....

Dave.

-- 
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
pam_smb / Linux DECstation / Linux VAX / ILUG person

Re: DRM code reorganization

[Eric Anholt]

On Mon, 2004-08-02 at 11:57, Dave Jones wrote:
> I don't maintain upstream DRM, but I have a fair amount of responsibility
> regarding the Fedora kernel, and I'll state publically that looking at
> bugs in drivers/char/drm is right up there on my list of
> "things I'd rather not do after lunch". Maintainability goes much
> further than 'the guy that wrote the code can grok it'.
> 
> People trying to pick up 3d driver programming on Linux have a huge
> hurdle in their place, as that code resembles *NO OTHER* driver code
> in the tree.

Here, I'd note that I got started in the DRM because I was interested in
3D drivers, but the actual 3D drivers were far bigger hurdles compared
to understanding even some nasty kernel spaghetti.

Of course, this is not to say that we don't have a ton of room for
improvement in the DRM, without even going back to having a generic DRM
library.  One of the things that I think would be nice to see is the
gamma DMA made to be like the rest of the drivers, in which case a lot
of code related to just that driver could be removed.  We could also do
some significant cleanups if we could allow ourselves to remove old
interface mistakes eventually.  This has been made harder than it should
be by the long turnaround times for new monolithic X releases, which is
finally changing now.

>  > >ian: what about splitting the current memory management code into a
>  > >module that can be swapped for your new version?
>  > 
>  > AFAIK, the only drivers that have any sort of in-kernel memory manager 
>  > are the radeon (only used by the R200 driver) and i830.
> 
> ISTR SiS has some memory management code too, though I've not looked
> too closely at that for a long time.

The SiS (and via, which copied it) memory management has way too much
code for what little it has to do.  I've replaced it and made it smaller
in terms of code and binary size, 64-bit clean, and (I think) much more
readable.  I'm going to take a look at committing it once I can drag
myself away from X.Org, at which point someone with via hardware could
do the same for theirs.

-- 
Eric Anholt                                eta@lclark.edu          
http://people.freebsd.org/~anholt/         anholt@FreeBSD.org

Re: DRM code reorganization

[Keith Whitwell]

Ian Romanick wrote:

> I think this is the right place to start.  A couple of these look easier 
> to get rid of than others.  __HAVE_MTRR and __HAVE_AGP are enabled in 
> every driver except ffb.  It should be easy enough to get rid of them. 
> It looks like __HAVE_RELEASE, __HAVE_DMA_READY, __HAVE_DMA_FLUSH, 
> __HAVE_DMA_QUIESCENT, and __HAVE_MULTIPLE_DMA_QUEUES (which looks broken 
> anyway) should also be low-hanging fruit.

We've actually managed to do a fair bit of cleanup already - if you look at 
the gamma driver, there's a lot of stuff in there which used to be shared but 
ifdef'ed out between all the drivers.  The __HAVE_MULTIPLE_DMA_QUEUES macro is 
a remnant of this, but I think you'll break gamma when you try & remove it.

It used to be the case that 50% of the #if hoo-hah was just to try & keep the 
gamma driver working.  I don't know how true this is anymore, though.

> If we get that far, I think the next step would be to replace the 
> DRIVER_* macros with a table of function pointers that would get passed 
> around.  Since I doubt any of those uses are performance critical, that 
> should be fine.
> 
> Then we can start looking at data structure refactoring.
> 
>>  > >If this kind of abuse wasn't so widespread, abstracting this code
>>  > >out into shared sections and driver specific parts would be a lot
>>  > >simpler. Sadly, this is the tip of the iceberg.
>>  >  > I think it comes down to the fact that the original DRM 
>> developers  > wanted templates.  C doesn't have them, so they did the 
>> "next best" thing.
>>
>> I vaguelly recall the code at one point not looking quite 'so bad',
>> it just grew and grew into this monster.  I'm sure it was done originally
>> with the best of intentions, but it seems someone along the line got
>> a bit carried away.
> 
> 
> There was a point when a *lot* of the device-dependent code was still in 
> the OS-dependent directories.  This is how the i810 and i830 drivers 
> still are.  I think as more of the code got moved into the 
> OS-independent directory, it got less pleasant to read.

Not a great deal changed as drivers got moved to shared/ -- things like 
copy_from_user() got replaced by DRM_COPY_FROM_USER(), etc, but that's about 
as far as it went.  The template abstractions haven't really changed a great 
deal with the introduction of freebsd support.  If anything, code has been 
simplified by moving the gamma-specific code out of the shared templates and 
into gamma_* files.

Keith

Re: DRM code reorganization

[Ian Romanick]

Keith Whitwell wrote:

> We've actually managed to do a fair bit of cleanup already - if you look 
> at the gamma driver, there's a lot of stuff in there which used to be 
> shared but ifdef'ed out between all the drivers.  The 
> __HAVE_MULTIPLE_DMA_QUEUES macro is a remnant of this, but I think 
> you'll break gamma when you try & remove it.

It looks like __HAVE_MULTIPLE_DMA_QUEUES is a superset of 
__HAVE_DMA_QUEUE.  My thinking was that the code for those two options 
could be merged.  Does that seem reasonable?

Re: DRM code reorganization

[Keith Whitwell]

Ian Romanick wrote:
> Keith Whitwell wrote:
> 
>> We've actually managed to do a fair bit of cleanup already - if you 
>> look at the gamma driver, there's a lot of stuff in there which used 
>> to be shared but ifdef'ed out between all the drivers.  The 
>> __HAVE_MULTIPLE_DMA_QUEUES macro is a remnant of this, but I think 
>> you'll break gamma when you try & remove it.
> 
> 
> It looks like __HAVE_MULTIPLE_DMA_QUEUES is a superset of 
> __HAVE_DMA_QUEUE.  My thinking was that the code for those two options 
> could be merged.  Does that seem reasonable?

If it looks reasonable in the codebase, I don't know any reason not to.

Keith