Menu
▾
▴
dri-devel
|
From: Jon S. <jon...@gm...> - 2004-09-28 15:54:40
|
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:ano...@dr...:/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
jon...@gm...
|
|
From: Ian R. <id...@us...> - 2004-09-28 16:56:53
|
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. |
|
From: Jon S. <jon...@gm...> - 2004-09-28 17:28:52
|
On Tue, 28 Sep 2004 09:56:38 -0700, Ian Romanick <id...@us...> 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 jon...@gm... |
|
From: Helge H. <hel...@ai...> - 2004-09-28 19:30:02
|
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 |
|
From: Dave A. <ai...@li...> - 2004-09-28 23:10:22
|
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:ano...@dr...:/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
|
|
From: Jon S. <jon...@gm...> - 2004-09-29 01:27:24
|
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 <ai...@li...> 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 jon...@gm... |
|
From: Dave A. <ai...@li...> - 2004-09-29 02:12:02
|
> > 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 |
|
From: Jon S. <jon...@gm...> - 2004-09-29 05:25:20
|
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 jon...@gm... |
|
From: Christoph H. <hc...@in...> - 2004-09-29 12:38:08
|
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 |
|
From: Keith W. <ke...@tu...> - 2004-09-29 13:29:29
|
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 |
|
From: Christoph H. <hc...@in...> - 2004-09-29 13:31:40
|
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. |
|
From: Dave A. <ai...@gm...> - 2004-09-29 13:41:35
|
> > - 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. |
|
From: Jon S. <jon...@gm...> - 2004-10-01 05:15:59
|
On Wed, 29 Sep 2004 13:37:59 +0100, Christoph Hellwig <hc...@in...> 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 jon...@gm... |
|
From: Alan C. <al...@lx...> - 2004-09-29 13:02:58
|
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. |
|
From: Dave A. <ai...@gm...> - 2004-09-29 13:16:14
|
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 <al...@lx...> 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
> xo...@fr...
> http://freedesktop.org/mailman/listinfo/xorg
>
|
|
From: Keith W. <ke...@tu...> - 2004-09-29 13:35:59
|
Christoph Hellwig wrote: > On Wed, Sep 29, 2004 at 02:29:24PM +0100, Keith Whitwell wrote: >=20 >>Christoph Hellwig wrote: >> >> >>> - drm_flush is a noop. a NULL ->flush does the same thing, just easi= er >>> - dito or ->poll >>> - dito for ->read >> >>Pretty sure you couldn't get away with null for these in 2.4, at least. >=20 >=20 > 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=E6cks i= t > aswell. I tried it, led to crashes (panics, I guess) & the change had to be rever= ted.=20 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: +1= 3 -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=20 nowadays. Keith |
|
From: Keith W. <ke...@tu...> - 2004-09-29 14:12:10
|
Keith Whitwell wrote: > Christoph Hellwig wrote: >=20 >> 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 eas= ier >>>> - dito or ->poll >>>> - dito for ->read >>> >>> >>> Pretty sure you couldn't get away with null for these in 2.4, at leas= t. >> >> >> >> 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 cha= r >> devices don't have ->poll. no ->read is pretty rare but 2.4 ch=E6cks = it >> aswell. >=20 >=20 > I tried it, led to crashes (panics, I guess) & the change had to be=20 > reverted. On reverting the crashes stopped. This was for poll and rea= d: Thinking about it, it may not have been a problem of crashing, but rather= that=20 the behaviour visible from a program attempting to read (or poll) was=20 different with noop versions of these functions to NULL versions, and tha= t was=20 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 proble= m=20 would have arisen, but only the gamma & maybe ffb drivers do anything wit= h it. Keith |
|
From: Christoph H. <hc...@in...> - 2004-09-29 14:16:09
|
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. |
|
From: Keith W. <ke...@tu...> - 2004-09-29 14:27:21
|
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 |
|
From: Keith W. <ke...@tu...> - 2004-09-29 14:39:43
|
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 |
|
From: Jon S. <jon...@gm...> - 2004-09-30 18:10:53
Attachments:
open.patch
|
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 jon...@gm... |
|
From: Keith W. <ke...@tu...> - 2004-09-29 14:25:15
|
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);
> };
>
|
|
From: Donnie B. <spy...@ge...> - 2004-09-29 15:29:54
|
On Wed, 2004-09-29 at 07:25, Keith Whitwell wrote: > The whole context thing in the kernel is pretty much cruft. The gamma mo= dule=20 > used to rely on it, maybe the ffb module if that still exists? It would = be=20 > good to see this disappear. A few Gentoo Sparc folks use ffb, and in some cases it works (http://bugs.gentoo.org/show_bug.cgi?id=3D65348). --=20 Donnie Berkholz Gentoo Linux |
|
From: Eric A. <et...@lc...> - 2004-09-30 00:00:50
|
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 et...@lc...
http://people.freebsd.org/~anholt/ an...@Fr...
|
|
From: Felix <fx...@gm...> - 2004-09-29 21:50:05
|
On Tue, 28 Sep 2004 11:54:35 -0400 Jon Smirl <jon...@gm...> 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). >=20 [snip] > --=20 > Jon Smirl > jon...@gm... | Felix K=FChling <fx...@gm...> http://fxk.de.vu | | PGP Fingerprint: 6A3C 9566 5B30 DDED 73C3 B152 151C 5CC1 D888 E595 | |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X