I've checked two new directories into DRM CVS for Linux 2.6 -
linux-core, shared-core. This code implements a new model for DRM
where DRM is split into a core piece and personality modules that
share the core. The major reason for doing this is that it allows me
to remove all of the DRM() macros; something that is causing lot's of
complaints from the Linux kernel people.
cvs -z3 -d:pserver:[email protected]:/cvs/dri co drm
The patch for this is quite large, 500K, and it will mess up the Linux
2.4/BSD DRM builds since it removes the DRM() macro. Instead of doing
this as a patch I made the new CVS directories.
I've checked radeon and r128 and they work. Everything else should
work except ffb, please check the other drivers and let me know. I've
probably made some typos with a edit this large.
ffb should work in principle but since I don't own one or a Sparc
machine so I can't compile the driver. I suspect ffb has compiler
errors but the are easily fixed just by copying similar code from one
of the working modules. Please send patches.
The API between the core and personalities doesn't look to be all that
huge. With work I'd guess that 10% of the entry points could be
eliminated. After another year of development we might be able to
specify a stable core API.
What are everyone's thoughts on this? There is no technical reason it
can't be implemented on Linux 2.4/BSD, although I don't see any reason
to do it for 2.4.
Any ideas on how to generate a unique identifier for the core? It
probably should be regenerated by the Makefile whenever the core
changes. Personalities would have to match the core identifier. This
would stop people from loading binary modules that don't match the
core.
After sometime testing and fixing obvious problems I'll generate a
patch for the 2.6 kernel and lkml.
[root@smirl linux-2.6]# lsmod | more
Module Size Used by
tdfx 2816 0
sis 10176 0
mga 56704 0
i915 16896 0
via 19428 0
savage 3840 0
r128 44672 0
radeon 70272 0
drm 59684 8 tdfx,sis,mga,i915,via,savage,r128,radeon
The core provides these entry points....
[root@smirl linux-2.6]# grep EXPORT_SYMBOL *
drm_bufs.c:EXPORT_SYMBOL(drm_order);
drm_bufs.c:EXPORT_SYMBOL(drm_initmap);
drm_dma.c:EXPORT_SYMBOL(drm_core_reclaim_buffers);
drm_drv.c:EXPORT_SYMBOL(drm_cleanup_pci);
drm_drv.c:EXPORT_SYMBOL(drm_init);
drm_drv.c:EXPORT_SYMBOL(drm_exit);
drm_drv.c:EXPORT_SYMBOL(drm_open);
drm_drv.c:EXPORT_SYMBOL(drm_release);
drm_drv.c:EXPORT_SYMBOL(drm_ioctl);
drm_fops.c:EXPORT_SYMBOL(drm_flush);
drm_fops.c:EXPORT_SYMBOL(drm_fasync);
drm_init.c:EXPORT_SYMBOL(drm_flags);
drm_irq.c:EXPORT_SYMBOL(drm_irq_uninstall);
drm_irq.c:EXPORT_SYMBOL(drm_vbl_send_signals);
drm_memory.c:EXPORT_SYMBOL(drm_calloc);
drm_pci.c:EXPORT_SYMBOL(drm_pci_alloc);
drm_pci.c:EXPORT_SYMBOL(drm_pci_free);
drm_stub.c:EXPORT_SYMBOL(drm_probe);
drm_vm.c:EXPORT_SYMBOL(drm_core_get_map_ofs);
drm_vm.c:EXPORT_SYMBOL(drm_core_get_reg_ofs);
Drivers provide these callbacks......
struct drm_driver_fn {
u32 driver_features;
int dev_priv_size;
int permanent_maps;
drm_ioctl_desc_t *ioctls;
int num_ioctls;
int (*preinit)(struct drm_device *, unsigned long flags);
void (*prerelease)(struct drm_device *, struct file *filp);
void (*pretakedown)(struct drm_device *);
int (*postcleanup)(struct drm_device *);
int (*presetup)(struct drm_device *);
int (*postsetup)(struct drm_device *);
int (*dma_ioctl)( DRM_IOCTL_ARGS );
/* these are opposites at the moment */
int (*open_helper)(struct drm_device *, drm_file_t *);
void (*free_filp_priv)(struct drm_device *, drm_file_t *);
void (*release)(struct drm_device *, struct file *filp);
void (*dma_ready)(struct drm_device *);
int (*dma_quiescent)(struct drm_device *);
int (*context_ctor)(struct drm_device *dev, int context);
int (*context_dtor)(struct drm_device *dev, int context);
int (*kernel_context_switch)(struct drm_device *dev, int old, int new);
int (*kernel_context_switch_unlock)(struct drm_device *dev);
int (*vblank_wait)(struct drm_device *dev, unsigned int *sequence);
/* these have to be filled in */
int (*postinit)(struct drm_device *, unsigned long flags);
irqreturn_t (*irq_handler)( DRM_IRQ_ARGS );
void (*irq_preinstall)(struct drm_device *dev);
void (*irq_postinstall)(struct drm_device *dev);
void (*irq_uninstall)(struct drm_device *dev);
void (*reclaim_buffers)(struct file *filp);
unsigned long (*get_map_ofs)(drm_map_t *map);
unsigned long (*get_reg_ofs)(struct drm_device *dev);
void (*set_version)(struct drm_device *dev, drm_set_version_t *sv);
int (*version)(drm_version_t *version);
};
--
Jon Smirl
[email protected]
Jon Smirl wrote:
I /should/ be able to look at the code changes sometime this week. In
the mean time, here are some initial comments...
> What are everyone's thoughts on this? There is no technical reason it
> can't be implemented on Linux 2.4/BSD, although I don't see any reason
> to do it for 2.4.
I'm of two minds about this. On the one hand, I agree with you. 2.4 is
in maintainence mode, and making a big change like this to the graphics
driver system is outside that scope. On the other hand, if the 2.4 and
the 2.6 code bases diverge too much, bugs fixed on the 2.6 side are less
likely to be fixed on the 2.4 side. Moreover, with more divergence
between the two, each gets less coverage. I could really go either way
on it.
> Any ideas on how to generate a unique identifier for the core? It
> probably should be regenerated by the Makefile whenever the core
> changes. Personalities would have to match the core identifier. This
> would stop people from loading binary modules that don't match the
> core.
I guess we'd want something like 'drm_core_version_<version number>'.
Each layered driver would just have to reference the matching symbol.
One neat thing about doing this is that we could theoretically make core
modules that support multiple versions of the interal API and export a
drm_core_version_* symbol for each.
I have only one problem with this type of setup. If the user has a
version mismatch, it's not trivially obvious why they aren't getting
direct rendering. There are already a number of ways a user can get
"stuck" like this, and this adds another one. It's not a new problem,
but it is one that needs to be addressed.
> After sometime testing and fixing obvious problems I'll generate a
> patch for the 2.6 kernel and lkml.
>
> [root@smirl linux-2.6]# lsmod | more
> Module Size Used by
> tdfx 2816 0
> sis 10176 0
> mga 56704 0
> i915 16896 0
> via 19428 0
> savage 3840 0
> r128 44672 0
> radeon 70272 0
> drm 59684 8 tdfx,sis,mga,i915,via,savage,r128,radeon
Anyone have a PCI card so that we can test actually using more than one
at a time? In the mean time, I think just having them all load at once
and one of them work is good enough.
On Tue, 28 Sep 2004 09:56:38 -0700, Ian Romanick <[email protected]> wrote:
> Anyone have a PCI card so that we can test actually using more than one
> at a time? In the mean time, I think just having them all load at once
> and one of them work is good enough.
It would be best if everyone tested each card individually right now,
both PCI and AGP versions should work.
I think I know of a couple places where it might break if multiple
cards are used simultaneously. All static variables in the core are
suspect and need to be individually checked. There are less than 20 so
it shouldn't take too long. Of course any help with this is
appreciated.
Another thing that isn't written is splitting the module parameters
between the core and personalities. I'll also switch syntax from 2.4
style to 2.6 style. When finished each module with have a debug=1
parameter and the core will also have a cards_limit which defaults to
16.
This version also includes minor number reuse so hotplugging a card
in/out won't exhaust the DRM minors.
--
Jon Smirl
[email protected]
On Tue, Sep 28, 2004 at 09:56:38AM -0700, Ian Romanick wrote:
> Jon Smirl wrote:
>
[...]
> >After sometime testing and fixing obvious problems I'll generate a
> >patch for the 2.6 kernel and lkml.
> >
> >[root@smirl linux-2.6]# lsmod | more
> >Module Size Used by
> >tdfx 2816 0
> >sis 10176 0
> >mga 56704 0
> >i915 16896 0
> >via 19428 0
> >savage 3840 0
> >r128 44672 0
> >radeon 70272 0
> >drm 59684 8 tdfx,sis,mga,i915,via,savage,r128,radeon
>
> Anyone have a PCI card so that we can test actually using more than one
> at a time? In the mean time, I think just having them all load at once
> and one of them work is good enough.
I have a radeon9200SE pci card, and a matrox G550 agp card. I can try
when that 2.6 patch becomes available.
Helge Hafting
As with Ian, I'm trying to grab the time to review this over the next day
or two, I have one or two worries but as I haven't read over the code I'll
wait until I'm ready to dedicate some proper time to it ..
Dave.
On Tue, 28 Sep 2004, Jon Smirl wrote:
> I've checked two new directories into DRM CVS for Linux 2.6 -
> linux-core, shared-core. This code implements a new model for DRM
> where DRM is split into a core piece and personality modules that
> share the core. The major reason for doing this is that it allows me
> to remove all of the DRM() macros; something that is causing lot's of
> complaints from the Linux kernel people.
>
> cvs -z3 -d:pserver:[email protected]:/cvs/dri co drm
>
> The patch for this is quite large, 500K, and it will mess up the Linux
> 2.4/BSD DRM builds since it removes the DRM() macro. Instead of doing
> this as a patch I made the new CVS directories.
>
> I've checked radeon and r128 and they work. Everything else should
> work except ffb, please check the other drivers and let me know. I've
> probably made some typos with a edit this large.
>
> ffb should work in principle but since I don't own one or a Sparc
> machine so I can't compile the driver. I suspect ffb has compiler
> errors but the are easily fixed just by copying similar code from one
> of the working modules. Please send patches.
>
> The API between the core and personalities doesn't look to be all that
> huge. With work I'd guess that 10% of the entry points could be
> eliminated. After another year of development we might be able to
> specify a stable core API.
>
> What are everyone's thoughts on this? There is no technical reason it
> can't be implemented on Linux 2.4/BSD, although I don't see any reason
> to do it for 2.4.
>
> Any ideas on how to generate a unique identifier for the core? It
> probably should be regenerated by the Makefile whenever the core
> changes. Personalities would have to match the core identifier. This
> would stop people from loading binary modules that don't match the
> core.
>
> After sometime testing and fixing obvious problems I'll generate a
> patch for the 2.6 kernel and lkml.
>
> [root@smirl linux-2.6]# lsmod | more
> Module Size Used by
> tdfx 2816 0
> sis 10176 0
> mga 56704 0
> i915 16896 0
> via 19428 0
> savage 3840 0
> r128 44672 0
> radeon 70272 0
> drm 59684 8 tdfx,sis,mga,i915,via,savage,r128,radeon
>
> The core provides these entry points....
>
> [root@smirl linux-2.6]# grep EXPORT_SYMBOL *
> drm_bufs.c:EXPORT_SYMBOL(drm_order);
> drm_bufs.c:EXPORT_SYMBOL(drm_initmap);
> drm_dma.c:EXPORT_SYMBOL(drm_core_reclaim_buffers);
> drm_drv.c:EXPORT_SYMBOL(drm_cleanup_pci);
> drm_drv.c:EXPORT_SYMBOL(drm_init);
> drm_drv.c:EXPORT_SYMBOL(drm_exit);
> drm_drv.c:EXPORT_SYMBOL(drm_open);
> drm_drv.c:EXPORT_SYMBOL(drm_release);
> drm_drv.c:EXPORT_SYMBOL(drm_ioctl);
> drm_fops.c:EXPORT_SYMBOL(drm_flush);
> drm_fops.c:EXPORT_SYMBOL(drm_fasync);
> drm_init.c:EXPORT_SYMBOL(drm_flags);
> drm_irq.c:EXPORT_SYMBOL(drm_irq_uninstall);
> drm_irq.c:EXPORT_SYMBOL(drm_vbl_send_signals);
> drm_memory.c:EXPORT_SYMBOL(drm_calloc);
> drm_pci.c:EXPORT_SYMBOL(drm_pci_alloc);
> drm_pci.c:EXPORT_SYMBOL(drm_pci_free);
> drm_stub.c:EXPORT_SYMBOL(drm_probe);
> drm_vm.c:EXPORT_SYMBOL(drm_core_get_map_ofs);
> drm_vm.c:EXPORT_SYMBOL(drm_core_get_reg_ofs);
>
> Drivers provide these callbacks......
>
> struct drm_driver_fn {
> u32 driver_features;
> int dev_priv_size;
> int permanent_maps;
> drm_ioctl_desc_t *ioctls;
> int num_ioctls;
> int (*preinit)(struct drm_device *, unsigned long flags);
> void (*prerelease)(struct drm_device *, struct file *filp);
> void (*pretakedown)(struct drm_device *);
> int (*postcleanup)(struct drm_device *);
> int (*presetup)(struct drm_device *);
> int (*postsetup)(struct drm_device *);
> int (*dma_ioctl)( DRM_IOCTL_ARGS );
> /* these are opposites at the moment */
> int (*open_helper)(struct drm_device *, drm_file_t *);
> void (*free_filp_priv)(struct drm_device *, drm_file_t *);
>
> void (*release)(struct drm_device *, struct file *filp);
> void (*dma_ready)(struct drm_device *);
> int (*dma_quiescent)(struct drm_device *);
> int (*context_ctor)(struct drm_device *dev, int context);
> int (*context_dtor)(struct drm_device *dev, int context);
> int (*kernel_context_switch)(struct drm_device *dev, int old, int new);
> int (*kernel_context_switch_unlock)(struct drm_device *dev);
> int (*vblank_wait)(struct drm_device *dev, unsigned int *sequence);
> /* these have to be filled in */
> int (*postinit)(struct drm_device *, unsigned long flags);
> irqreturn_t (*irq_handler)( DRM_IRQ_ARGS );
> void (*irq_preinstall)(struct drm_device *dev);
> void (*irq_postinstall)(struct drm_device *dev);
> void (*irq_uninstall)(struct drm_device *dev);
> void (*reclaim_buffers)(struct file *filp);
> unsigned long (*get_map_ofs)(drm_map_t *map);
> unsigned long (*get_reg_ofs)(struct drm_device *dev);
> void (*set_version)(struct drm_device *dev, drm_set_version_t *sv);
> int (*version)(drm_version_t *version);
> };
>
>
--
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
pam_smb / Linux DECstation / Linux VAX / ILUG person
Single card seems to be working currently. I still can't get multiple
cards going yet. Second card tries to get the context without holding
the lock and errors out.
I'll keep working on it tonight and tomorrow. I'm past the obvious bug
stage now and into the ones where I stare at it for five hours before
I figure out what is wrong.
Still need someone with a Sparc machine to fix the ffb compile.
On Wed, 29 Sep 2004 00:10:18 +0100 (IST), Dave Airlie <[email protected]> wrote:
> As with Ian, I'm trying to grab the time to review this over the next day
> or two, I have one or two worries but as I haven't read over the code I'll
> wait until I'm ready to dedicate some proper time to it ..
--
Jon Smirl
[email protected]
>
> Still need someone with a Sparc machine to fix the ffb compile.
I've got a cross compiler installed at home for just this purpose .. I'll
work on it this evening..
Dave.
--
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
pam_smb / Linux DECstation / Linux VAX / ILUG person
I've found the problem with multiple drivers, it's in the mapping
code. When drivers close their handles they are deleting permanent
maps that don't belong to them. I'll fix it tomorrow.
--
Jon Smirl
[email protected]
On Tue, Sep 28, 2004 at 11:54:35AM -0400, Jon Smirl wrote:
> I've checked two new directories into DRM CVS for Linux 2.6 -
> linux-core, shared-core. This code implements a new model for DRM
> where DRM is split into a core piece and personality modules that
> share the core. The major reason for doing this is that it allows me
> to remove all of the DRM() macros; something that is causing lot's of
> complaints from the Linux kernel people.
I gave it a quick look and it looks great so far.
Some ideas that would be nice improvements still (not that some may be
inherited from the old drm code, I just looked at the CVS checkout):
- once we have Alan's idea of the graphics core implemented drm_init()
should go awaw
- drm_probe (and it's call to drm_fill_in_dev) looks a little fishy,
what about doing the full ->probe callback in each driver where it
can do basic hw setup, dealing with pci and calls back into the drm
core for minor number allocation and common structure allocations.
This would get rid of the ->preinit and ->postinit hooks.
- isn't drm_order just a copy of get_order()?
- any chance to use proper kernel-doc comments instead of the bastdized
and hard to read version you have currently?
- the coding style is a little strange, like spurious whitespaces inside
braces, maybe you could run it through scripts/Lindent
- care to use linux/lists.h instead of opencoded lists, e.g. in
dev->file_last/dev->file_first or dev->vmalist
- drm_flush is a noop. a NULL ->flush does the same thing, just easier
- dito or ->poll
- dito for ->read
- why do you use DRM_COPY_FROM_USER_IOCTL in Linux-specific code?
- drm__mem_info should be converted to fs/seq_file.c functions
- dito for functions in drm_proc.c
On Mer, 2004-09-29 at 13:37, Christoph Hellwig wrote:
> - once we have Alan's idea of the graphics core implemented drm_init()
> should go awaw
Last I heard Dave Airlie had that working having fixed my bugs.
I have the basics working alright, my only issue was with getting
proper sysfs with it, I'll dig up my latest version and post it, I've
also got a ported DRM for it,
at the moment it was doing something like
/sys/devices/vga00 /sys/devices/vga01 one for each user of the card
(multi-card would look really ugly) whereas what we probably want is a
directory per card along the lines of
/sys/device/vga0
/vga1
and then links 0->x for each registered driver or maybe even links
like common, dri , fb0 (make the names meaningful as they do have
one...)
Dave.
On Wed, 29 Sep 2004 12:59:55 +0100, Alan Cox <[email protected]> wrote:
> On Mer, 2004-09-29 at 13:37, Christoph Hellwig wrote:
> > - once we have Alan's idea of the graphics core implemented drm_init()
> > should go awaw
>
> Last I heard Dave Airlie had that working having fixed my bugs.
>
>
>
>
> _______________________________________________
> xorg mailing list
> [email protected]
> http://freedesktop.org/mailman/listinfo/xorg
>
Christoph Hellwig wrote:
> - drm_flush is a noop. a NULL ->flush does the same thing, just easier
> - dito or ->poll
> - dito for ->read
Pretty sure you couldn't get away with null for these in 2.4, at least.
Keith
On Wed, Sep 29, 2004 at 02:29:24PM +0100, Keith Whitwell wrote:
> Christoph Hellwig wrote:
>
> > - drm_flush is a noop. a NULL ->flush does the same thing, just easier
> > - dito or ->poll
> > - dito for ->read
>
> Pretty sure you couldn't get away with null for these in 2.4, at least.
Umm, of course you could. There's only a hanfull instance defining a
->flush at all. Similarly all file_ops for regular files and many char
devices don't have ->poll. no ->read is pretty rare but 2.4 ch?cks it
aswell.
Christoph Hellwig wrote:
> On Wed, Sep 29, 2004 at 02:29:24PM +0100, Keith Whitwell wrote:
>
>>Christoph Hellwig wrote:
>>
>>
>>> - drm_flush is a noop. a NULL ->flush does the same thing, just easier
>>> - dito or ->poll
>>> - dito for ->read
>>
>>Pretty sure you couldn't get away with null for these in 2.4, at least.
>
>
> Umm, of course you could. There's only a hanfull instance defining a
> ->flush at all. Similarly all file_ops for regular files and many char
> devices don't have ->poll. no ->read is pretty rare but 2.4 ch?cks it
> aswell.
I tried it, led to crashes (panics, I guess) & the change had to be reverted.
On reverting the crashes stopped. This was for poll and read:
revision 1.12
date: 2003-04-23 23:42:28 +0000; author: keithw; state: Exp; lines: +13 -0
Install dummy/noop read & poll fops unless the driver has replacements.
----------------------------
revision 1.11
date: 2003-04-22 08:06:13 +0000; author: keithw; state: Exp; lines: +0 -94
remove DRM read, poll and write_string
I didn't do any more investigation & the behaviour may well be different
nowadays.
Keith
>
> - once we have Alan's idea of the graphics core implemented drm_init()
> should go awaw
> - drm_probe (and it's call to drm_fill_in_dev) looks a little fishy,
> what about doing the full ->probe callback in each driver where it
> can do basic hw setup, dealing with pci and calls back into the drm
> core for minor number allocation and common structure allocations.
We have mentioned this but 90% of the work done by the drivers would
be common, we might do it the otherway I suppose have a driver probe
that calls a function in the core,
> This would get rid of the ->preinit and ->postinit hooks.
> - isn't drm_order just a copy of get_order()?
> - any chance to use proper kernel-doc comments instead of the bastdized
> and hard to read version you have currently?
I think we have doxygen comments in there at the moment - the Mesa/DRI
documentation is done with doxygen...
> - the coding style is a little strange, like spurious whitespaces inside
> braces, maybe you could run it through scripts/Lindent
there are a fair few of these in there in the kernel, it could
probably do with a Lindent at some stage over the whole thing...
> - care to use linux/lists.h instead of opencoded lists, e.g. in
> dev->file_last/dev->file_first or dev->vmalist
> - drm_flush is a noop. a NULL ->flush does the same thing, just easier
> - dito or ->poll
> - dito for ->read
> - why do you use DRM_COPY_FROM_USER_IOCTL in Linux-specific code?
> - drm__mem_info should be converted to fs/seq_file.c functions
> - dito for functions in drm_proc.c
I think I can apply a lot of these to the current kernel code so I'll
probably just start doing patches up for these sort of issues
separately....
I'll get time to create a bitkeeper tree taking Jons changes ready for
merging to Andrew at least, and maybe Linus for 2.6.10.
Dave.
Keith Whitwell wrote:
> Christoph Hellwig wrote:
>
>> On Wed, Sep 29, 2004 at 02:29:24PM +0100, Keith Whitwell wrote:
>>
>>> Christoph Hellwig wrote:
>>>
>>>
>>>> - drm_flush is a noop. a NULL ->flush does the same thing, just easier
>>>> - dito or ->poll
>>>> - dito for ->read
>>>
>>>
>>> Pretty sure you couldn't get away with null for these in 2.4, at least.
>>
>>
>>
>> Umm, of course you could. There's only a hanfull instance defining a
>> ->flush at all. Similarly all file_ops for regular files and many char
>> devices don't have ->poll. no ->read is pretty rare but 2.4 ch?cks it
>> aswell.
>
>
> I tried it, led to crashes (panics, I guess) & the change had to be
> reverted. On reverting the crashes stopped. This was for poll and read:
Thinking about it, it may not have been a problem of crashing, but rather that
the behaviour visible from a program attempting to read (or poll) was
different with noop versions of these functions to NULL versions, and that was
causing problems. This is 18 months ago, so yes, I'm being vague.
The X server does look at this file descriptor, which is where the problem
would have arisen, but only the gamma & maybe ffb drivers do anything with it.
Keith
On Wed, Sep 29, 2004 at 03:12:03PM +0100, Keith Whitwell wrote:
> Thinking about it, it may not have been a problem of crashing, but rather that
> the behaviour visible from a program attempting to read (or poll) was
> different with noop versions of these functions to NULL versions, and that was
> causing problems. This is 18 months ago, so yes, I'm being vague.
>
> The X server does look at this file descriptor, which is where the problem
> would have arisen, but only the gamma & maybe ffb drivers do anything with it.
Indeed, for read you're returning 0 now instead of the -EINVAL from common
code when no ->read is present. I'd say the current drm behaviour is a bug,
but if X drivers rely on it.
Similar in poll your return 0 now while the generic code return
(POLLIN | POLLOUT | POLLRDNORM | POLLWRNORM) for fds without ->poll, and
again I'd say current drm behaviour could be considered a bug.
for ->flush there's no behaviour change of not supplying it.
Jon Smirl wrote:
>
> Drivers provide these callbacks......
>
> struct drm_driver_fn {
> u32 driver_features;
> int dev_priv_size;
> int permanent_maps;
> drm_ioctl_desc_t *ioctls;
> int num_ioctls;
> int (*preinit)(struct drm_device *, unsigned long flags);
> void (*prerelease)(struct drm_device *, struct file *filp);
> void (*pretakedown)(struct drm_device *);
> int (*postcleanup)(struct drm_device *);
> int (*presetup)(struct drm_device *);
> int (*postsetup)(struct drm_device *);
> int (*dma_ioctl)( DRM_IOCTL_ARGS );
> /* these are opposites at the moment */
> int (*open_helper)(struct drm_device *, drm_file_t *);
> void (*free_filp_priv)(struct drm_device *, drm_file_t *);
> void (*release)(struct drm_device *, struct file *filp);
> void (*dma_ready)(struct drm_device *);
Is this used by any driver?
> int (*dma_quiescent)(struct drm_device *);
> int (*context_ctor)(struct drm_device *dev, int context);
> int (*context_dtor)(struct drm_device *dev, int context);
> int (*kernel_context_switch)(struct drm_device *dev, int old, int new);
> int (*kernel_context_switch_unlock)(struct drm_device *dev);
The whole context thing in the kernel is pretty much cruft. The gamma module
used to rely on it, maybe the ffb module if that still exists? It would be
good to see this disappear.
Though the drivers don't rely on it, I don't know if the server-side code
persists in setting it up regardless, which might make it hard to get rid of.
> int (*vblank_wait)(struct drm_device *dev, unsigned int *sequence);
> /* these have to be filled in */
> int (*postinit)(struct drm_device *, unsigned long flags);
Maybe move this up with the other init/cleanup functions so that they are
visibly grouped? Can the large number of init/cleanup functions be
rationalized in some way?
> irqreturn_t (*irq_handler)( DRM_IRQ_ARGS );
> void (*irq_preinstall)(struct drm_device *dev);
> void (*irq_postinstall)(struct drm_device *dev);
> void (*irq_uninstall)(struct drm_device *dev);
> void (*reclaim_buffers)(struct file *filp);
Maybe rename this to dma_reclaim_buffers() to make clear what code it is
associated with?
> unsigned long (*get_map_ofs)(drm_map_t *map);
> unsigned long (*get_reg_ofs)(struct drm_device *dev);
> void (*set_version)(struct drm_device *dev, drm_set_version_t *sv);
> int (*version)(drm_version_t *version);
> };
>
Keith Whitwell wrote:
> Christoph Hellwig wrote:
>
>> On Wed, Sep 29, 2004 at 03:12:03PM +0100, Keith Whitwell wrote:
>>
>>> Thinking about it, it may not have been a problem of crashing, but
>>> rather that the behaviour visible from a program attempting to read
>>> (or poll) was different with noop versions of these functions to NULL
>>> versions, and that was causing problems. This is 18 months ago, so
>>> yes, I'm being vague.
>>>
>>> The X server does look at this file descriptor, which is where the
>>> problem would have arisen, but only the gamma & maybe ffb drivers do
>>> anything with it.
>>
>>
>>
>> Indeed, for read you're returning 0 now instead of the -EINVAL from
>> common
>> code when no ->read is present. I'd say the current drm behaviour is
>> a bug,
>> but if X drivers rely on it.
>
>
> I'd agree, but it's a widely distributed bug. I guess we can fix it in
> the X server, but even better would be to rip out the code as it's
> fundamentally misguided, based on a wierd idea that the kernel would
> somehow ask the X server to perform a context switch between two
> userspace clients...
The piece of the puzzle you're missing is that the read() function really did
used to do something, and was relied upon.
If you want to go right back to prehistory, the drm was originally designed as
a "core + personality" system, where the core supported a number of different
context switching mechanisms to cover a variety of hardware cases. The gamma
driver exercised one path, but everything since then has been a lot more
simplistic, assuming that the hardware state is lost if another context has
been active.
Hardware often has the capacity to hold multiple active contexts or to perform
fast hardware context save & restore. None of the DRI drivers have attempted
to take advantage of that - optimization continues to focus on the
single-client scenario.
A future X-on-GL world where regular applications are presumably doing direct
rendering will change that assumption...
Keith
Christoph Hellwig wrote:
> On Wed, Sep 29, 2004 at 03:12:03PM +0100, Keith Whitwell wrote:
>
>>Thinking about it, it may not have been a problem of crashing, but rather that
>> the behaviour visible from a program attempting to read (or poll) was
>>different with noop versions of these functions to NULL versions, and that was
>>causing problems. This is 18 months ago, so yes, I'm being vague.
>>
>>The X server does look at this file descriptor, which is where the problem
>>would have arisen, but only the gamma & maybe ffb drivers do anything with it.
>
>
> Indeed, for read you're returning 0 now instead of the -EINVAL from common
> code when no ->read is present. I'd say the current drm behaviour is a bug,
> but if X drivers rely on it.
I'd agree, but it's a widely distributed bug. I guess we can fix it in the X
server, but even better would be to rip out the code as it's fundamentally
misguided, based on a wierd idea that the kernel would somehow ask the X
server to perform a context switch between two userspace clients...
Keith
Around 15 o'clock on Sep 29, Keith Whitwell wrote:
> A future X-on-GL world where regular applications are presumably doing direct
> rendering will change that assumption...
I'm not planning on eliminating the X protocol in this environment, so
unless cairo really takes off and applications start coding to cairo-on-GL
instead of cairo-on-X-on-GL, then we'll have about the same number of
contexts, although the X context will be more rational than it currently
is.
-keith
On Tue, 28 Sep 2004 11:54:35 -0400
Jon Smirl <[email protected]> wrote:
> I've checked two new directories into DRM CVS for Linux 2.6 -
> linux-core, shared-core. This code implements a new model for DRM
> where DRM is split into a core piece and personality modules that
> share the core. The major reason for doing this is that it allows me
> to remove all of the DRM() macros; something that is causing lot's of
> complaints from the Linux kernel people.
A single savage works just fine. This is lsmod output with X running:
Module Size Used by
savage 3520 0
drm 62500 3 savage
Is it normal that the savage module looks unused? I can actually rmmod
the savage module while X is running. After that direct rending fails
with some error message about permissions ... reloading savage didn't
help (of course, because X wouldn't reinitialize it). A bit later the
box locked up. Is this 0 usage count and the ability to rmmod the module
while X is running specific to the savage driver or do other drivers
show the same behaviour?
Some questions about future driver development: So the new linux-core
and shared-core are the place to do new driver development? If this is
correct then it will be for 2.6 kernels only, right? I suppose there
would some back-porting effort involved in getting a future savage
driver to work with 2.4 again (like adding back all the DRM() macros).
>
[snip]
> --
> Jon Smirl
> [email protected]
| Felix K?hling <[email protected]> http://fxk.de.vu |
| PGP Fingerprint: 6A3C 9566 5B30 DDED 73C3 B152 151C 5CC1 D888 E595 |
On Mer, 2004-09-29 at 22:52, Felix Kühling wrote:
> Module Size Used by
> savage 3520 0
> drm 62500 3 savage
>
> Is it normal that the savage module looks unused?
looks like a bug. If the drm layer provides the file_operations for
the device node then the locking done automatically locks the wrong
module. Thats easy to fix with try_module_get() and module_put()
On Wed, 29 Sep 2004 23:52:38 +0200, Felix K?hling <[email protected]> wrote:
> Is it normal that the savage module looks unused? I can actually rmmod
> the savage module while X is running. After that direct rending fails
> with some error message about permissions ... reloading savage didn't
> help (of course, because X wouldn't reinitialize it). A bit later the
> box locked up. Is this 0 usage count and the ability to rmmod the module
> while X is running specific to the savage driver or do other drivers
> show the same behaviour?
This is a bug, open is marking the wrong module in use.
> Some questions about future driver development: So the new linux-core
> and shared-core are the place to do new driver development? If this is
> correct then it will be for 2.6 kernels only, right? I suppose there
> would some back-porting effort involved in getting a future savage
> driver to work with 2.4 again (like adding back all the DRM() macros).
There is no real difference between the code in the linux directory
and linux-core except for the removal of the DRM macros and the
associated restructuring needed to make everything work. When we get
linux-core working without problems, it's not there yet, it could
become the future 2.6 platform if everyone agrees. The impact of the
linux-core changes are minimal on the board specific code.
For 2.4 there is a choice: continue using the linux directory or
backport linux-core to 2.4. I don't know how much effort everyone
wants to put into backporting new driver development to 2.4. There are
several possible choices:
1) leave 2.4 alone and stop working on it, 2.4 stays in the linux directory
2) declare the DRM version in the linux-2.4 the final version and only
submit bug patches via the kernel process.
3) backport linux-core to 2.4 and so that everything will build on
both OS's. Some 2.6 kernel changes are starting to make this a very
cluttered option.
4) Make parallel changes to the 2.4 and 2.6 versions.
5) other combinations of these
The removal of the DRM macros from files in the shared directory means
that things can't be shared again unless 2.4/BSD also move the the
core model. I have no strong opinions on what to do about 2.4. I'll go
along with whatever the crowd picks.
--
Jon Smirl
[email protected]
On Wed, 2004-09-29 at 07:25, Keith Whitwell wrote:
> Jon Smirl wrote:
> >
> > Drivers provide these callbacks......
> >
> > struct drm_driver_fn {
> > u32 driver_features;
> > int dev_priv_size;
> > int permanent_maps;
> > drm_ioctl_desc_t *ioctls;
> > int num_ioctls;
>
> > int (*preinit)(struct drm_device *, unsigned long flags);
> > void (*prerelease)(struct drm_device *, struct file *filp);
> > void (*pretakedown)(struct drm_device *);
> > int (*postcleanup)(struct drm_device *);
> > int (*presetup)(struct drm_device *);
> > int (*postsetup)(struct drm_device *);
> > int (*dma_ioctl)( DRM_IOCTL_ARGS );
> > /* these are opposites at the moment */
> > int (*open_helper)(struct drm_device *, drm_file_t *);
> > void (*free_filp_priv)(struct drm_device *, drm_file_t *);
>
> > void (*release)(struct drm_device *, struct file *filp);
> > void (*dma_ready)(struct drm_device *);
>
> Is this used by any driver?
>
> > int (*dma_quiescent)(struct drm_device *);
>
> > int (*context_ctor)(struct drm_device *dev, int context);
> > int (*context_dtor)(struct drm_device *dev, int context);
> > int (*kernel_context_switch)(struct drm_device *dev, int old, int new);
> > int (*kernel_context_switch_unlock)(struct drm_device *dev);
>
> The whole context thing in the kernel is pretty much cruft. The gamma module
> used to rely on it, maybe the ffb module if that still exists? It would be
> good to see this disappear.
>
> Though the drivers don't rely on it, I don't know if the server-side code
> persists in setting it up regardless, which might make it hard to get rid of.
SiS relies on context ctor/dtor (dtor only, when I'm done) for its
kernel memory manager.
--
Eric Anholt [email protected]
http://people.freebsd.org/~anholt/ [email protected]
Patch removes DRM flush, poll, read functions. It leave the fops entry
null so that the OS default function will be used.
The fops table is converted to be one per driver instead of a global.
This fixes the module open ref count problem. It also simplifies
i810/830 by allow them to directly patch their mmap function into the
fops table.
I spent two days looking for a bug in DRM with multiple drivers under
X. I don't think DRM has problems. I see now that X also fails if two
older DRM drivers are loaded. The first problem seems to be the X's
DRM lock refcount varibale is a static. That won't work for two DRM
drivers.
--
Jon Smirl
[email protected]
On Wed, 29 Sep 2004 13:37:59 +0100, Christoph Hellwig <[email protected]> wrote:
> Some ideas that would be nice improvements still (not that some may be
> inherited from the old drm code, I just looked at the CVS checkout):
>
> - once we have Alan's idea of the graphics core implemented drm_init()
> should go awaw
> - drm_probe (and it's call to drm_fill_in_dev) looks a little fishy,
> what about doing the full ->probe callback in each driver where it
> can do basic hw setup, dealing with pci and calls back into the drm
> core for minor number allocation and common structure allocations.
> This would get rid of the ->preinit and ->postinit hooks.
This has to stay the way it currently is because of the stealth mode code
> - isn't drm_order just a copy of get_order()?
switched to get_order
> - any chance to use proper kernel-doc comments instead of the bastdized
> and hard to read version you have currently?
doc people don't want to switch
> - the coding style is a little strange, like spurious whitespaces inside
> braces, maybe you could run it through scripts/Lindent
ran most of it through Lindent. check out CVS for results.
> - care to use linux/lists.h instead of opencoded lists, e.g. in
> dev->file_last/dev->file_first or dev->vmalist
There are about 20 open coded lists. I started to fix some of these
but the code involved can be touchy and it's already well tested.
It would be better to wait on these until someone is working on
the code involved. I don't want to introduce bugs because I don't
understand the code 100%.
> - drm_flush is a noop. a NULL ->flush does the same thing, just easier
> - dito or ->poll
> - dito for ->read
Changed these to use kernel defaults.
> - why do you use DRM_COPY_FROM_USER_IOCTL in Linux-specific code?
That can probably be fixed. I believe it is because it is hiding a
2.4/2.6 change.
> - drm__mem_info should be converted to fs/seq_file.c functions
> - dito for functions in drm_proc.c
I started to do this to but I didn't want to disrupt know working code. These
may get rewritten for sysfs.
>
--
Jon Smirl
[email protected]