From: Jesse B. <jb...@vi...> - 2008-08-26 18:57:34
Attachments:
drm-kill-irq-install-2.patch
|
The DRM design includes ioctls to allow a userland driver to tell the kernel driver when to register its interrupt handler and on what IRQ. This is a really bad idea for several reasons, and fortunately I don't think any DDX drivers take advantage of the "no, use this IRQ" aspect of the API (and even if they did the kernel driver would have to ignore it). This patch removes the DRM support for those ioctls, making drivers just register their interrupt handlers at load time. The patch is fairly straightforward but there are still caveats, so each driver will need careful review to make sure that userland drivers don't set up additional state required for proper interrupt handling/enabling. It also means drivers have to map registers at load time; the r128 bits in particular looked funky in that regard so extra eyes there would be appreciated. I've only tested this patch so far on i915, where it's still slightly broken; I was planning on fixing it once I've sync'd some more linux-core changes into drm-next (namely the rest of the vblank-rework code). drivers/gpu/drm/drm_ioctl.c | 2 drivers/gpu/drm/drm_irq.c | 82 +++++++---------- drivers/gpu/drm/i810/i810_dma.c | 7 - drivers/gpu/drm/i830/i830_dma.c | 7 - drivers/gpu/drm/i915/i915_dma.c | 48 ++++++++- drivers/gpu/drm/i915/i915_drv.c | 3 drivers/gpu/drm/i915/i915_drv.h | 7 + drivers/gpu/drm/i915/i915_irq.c | 62 ------------ drivers/gpu/drm/mga/mga_dma.c | 37 ++++++- drivers/gpu/drm/mga/mga_drv.c | 3 drivers/gpu/drm/mga/mga_irq.c | 32 ------ drivers/gpu/drm/r128/r128_cce.c | 115 ++++++++++++++--------- drivers/gpu/drm/r128/r128_drv.c | 8 + drivers/gpu/drm/r128/r128_irq.c | 28 ----- drivers/gpu/drm/radeon/radeon_cp.c | 43 +++++--- drivers/gpu/drm/radeon/radeon_drv.c | 3 drivers/gpu/drm/radeon/radeon_drv.h | 9 + drivers/gpu/drm/radeon/radeon_irq.c | 50 ---------- drivers/gpu/drm/via/via_drv.c | 3 drivers/gpu/drm/via/via_drv.h | 46 +++++++++ drivers/gpu/drm/via/via_irq.c | 135 +--------------------------- drivers/gpu/drm/via/via_map.c | 67 +++++++++++++ drivers/gpu/drm/via/via_mm.c | 2 include/drm/drmP.h | 13 +- 24 files changed, 355 insertions(+), 457 deletions(-) Thanks, -- Jesse Barnes, Intel Open Source Technology Center |
From: Stephane M. <mar...@ic...> - 2008-08-26 19:03:30
|
On Tue, Aug 26, 2008 at 8:57 PM, Jesse Barnes <jb...@vi...> wrote: > The DRM design includes ioctls to allow a userland driver to tell the kernel > driver when to register its interrupt handler and on what IRQ. This is a > really bad idea for several reasons, and fortunately I don't think any DDX > drivers take advantage of the "no, use this IRQ" aspect of the API (and even > if they did the kernel driver would have to ignore it). > > This patch removes the DRM support for those ioctls, making drivers just > register their interrupt handlers at load time. The patch is fairly > straightforward but there are still caveats, so each driver will need careful > review to make sure that userland drivers don't set up additional state > required for proper interrupt handling/enabling. It also means drivers have > to map registers at load time; the r128 bits in particular looked funky in > that regard so extra eyes there would be appreciated. > > I've only tested this patch so far on i915, where it's still slightly broken; > I was planning on fixing it once I've sync'd some more linux-core changes > into drm-next (namely the rest of the vblank-rework code). > This patch breaks a couple of drivers. Is this an oversight, or does the "new development model" mean that we break drivers that are not in linux each time ? Stephane |
From: Thomas H. <th...@tu...> - 2008-08-26 19:25:02
|
Jesse Barnes wrote: > The DRM design includes ioctls to allow a userland driver to tell the kernel > driver when to register its interrupt handler and on what IRQ. This is a > really bad idea for several reasons, and fortunately I don't think any DDX > drivers take advantage of the "no, use this IRQ" aspect of the API (and even > if they did the kernel driver would have to ignore it). > > This patch removes the DRM support for those ioctls, making drivers just > register their interrupt handlers at load time. The patch is fairly > straightforward but there are still caveats, so each driver will need careful > review to make sure that userland drivers don't set up additional state > required for proper interrupt handling/enabling. It also means drivers have > to map registers at load time; the r128 bits in particular looked funky in > that regard so extra eyes there would be appreciated. > > I've only tested this patch so far on i915, where it's still slightly broken; > I was planning on fixing it once I've sync'd some more linux-core changes > into drm-next (namely the rest of the vblank-rework code). > Hi, I know this breaks via at least since the device is more or less dead until the X server programs the sequencer, so that would require a bunch of setup code at load time to fix. I agree it's a bad idea to keep the old irq ioctls, but why not get rid of the ioctls only and keep the old irq driver infrastructure and let the drivers call drm_irq_install() or drm_irq_uninstall() where fit, either in driver_load() / driver_unload() or driver_firstopen() / driver_lastclose()? /Thomas |
From: Jesse B. <jb...@vi...> - 2008-08-26 19:36:12
|
On Tuesday, August 26, 2008 12:03 pm Stephane Marchesin wrote: > On Tue, Aug 26, 2008 at 8:57 PM, Jesse Barnes <jb...@vi...> wrote: > > The DRM design includes ioctls to allow a userland driver to tell the > > kernel driver when to register its interrupt handler and on what IRQ. > > This is a really bad idea for several reasons, and fortunately I don't > > think any DDX drivers take advantage of the "no, use this IRQ" aspect of > > the API (and even if they did the kernel driver would have to ignore it). > > > > This patch removes the DRM support for those ioctls, making drivers just > > register their interrupt handlers at load time. The patch is fairly > > straightforward but there are still caveats, so each driver will need > > careful review to make sure that userland drivers don't set up additional > > state required for proper interrupt handling/enabling. It also means > > drivers have to map registers at load time; the r128 bits in particular > > looked funky in that regard so extra eyes there would be appreciated. > > > > I've only tested this patch so far on i915, where it's still slightly > > broken; I was planning on fixing it once I've sync'd some more linux-core > > changes into drm-next (namely the rest of the vblank-rework code). > > This patch breaks a couple of drivers. Is this an oversight, or does > the "new development model" mean that we break drivers that are not in > linux each time ? 1) this is an RFC that hasn't been pushed anywhere yet, so nothing is actually broken by it 2) this patch is against the drm-next Linux tree, so any breakage is limited to the drivers there But I assume you're talking about nouveau? Does it have any funky requirements wrt IRQ registration? Or would it be relatively easy to move its interrupt handler registration and IRQ setup to the load routine? As for the "new development model"... Things are actually worse than I thought. There are some fairly large differences between linux-core and upstream, some of which have been in linux-core for a long time. It's one thing to have an out-of-tree development process but another entirely to let stuff rot for months & years there. It just adds to the already huge set of driver combinations we have to worry about and support... So drm-next is all I care about anymore. I'm trying to sync the last couple of years worth of development to that tree so I can start ignoring linux-core entirely. It's just too disconnected from upstream Linux for me to worry about (note that this doesn't mean I don't care about BSD compat; I want to make sure sharing is still reasonably easy, but if anything I'd like the merges to go from drm-next -> linux-core rather than the other way around for my development). -- Jesse Barnes, Intel Open Source Technology Center |
From: Jesse B. <jb...@vi...> - 2008-08-26 19:50:44
|
On Tuesday, August 26, 2008 12:23 pm Thomas Hellström wrote: > Jesse Barnes wrote: > > The DRM design includes ioctls to allow a userland driver to tell the > > kernel driver when to register its interrupt handler and on what IRQ. > > This is a really bad idea for several reasons, and fortunately I don't > > think any DDX drivers take advantage of the "no, use this IRQ" aspect of > > the API (and even if they did the kernel driver would have to ignore it). > > > > This patch removes the DRM support for those ioctls, making drivers just > > register their interrupt handlers at load time. The patch is fairly > > straightforward but there are still caveats, so each driver will need > > careful review to make sure that userland drivers don't set up additional > > state required for proper interrupt handling/enabling. It also means > > drivers have to map registers at load time; the r128 bits in particular > > looked funky in that regard so extra eyes there would be appreciated. > > > > I've only tested this patch so far on i915, where it's still slightly > > broken; I was planning on fixing it once I've sync'd some more linux-core > > changes into drm-next (namely the rest of the vblank-rework code). > > Hi, > > I know this breaks via at least since the device is more or less dead > until the X server programs the sequencer, so that > would require a bunch of setup code at load time to fix. > > I agree it's a bad idea to keep the old irq ioctls, but why not get rid > of the ioctls only and keep the old irq driver infrastructure and let > the drivers call drm_irq_install() or drm_irq_uninstall() where fit, > either in driver_load() / driver_unload() or driver_firstopen() / > driver_lastclose()? Yeah that's definitely a possibility, thanks for looking at the via bits. Though for kernel mode setting via will need to set that stuff up at load time anyway, right? How hard would it be to port that code? Thanks, -- Jesse Barnes, Intel Open Source Technology Center |
From: Thomas H. <th...@tu...> - 2008-08-27 06:50:16
|
Jesse Barnes wrote: > On Tuesday, August 26, 2008 12:23 pm Thomas Hellström wrote: > >> Jesse Barnes wrote: >> >>> The DRM design includes ioctls to allow a userland driver to tell the >>> kernel driver when to register its interrupt handler and on what IRQ. >>> This is a really bad idea for several reasons, and fortunately I don't >>> think any DDX drivers take advantage of the "no, use this IRQ" aspect of >>> the API (and even if they did the kernel driver would have to ignore it). >>> >>> This patch removes the DRM support for those ioctls, making drivers just >>> register their interrupt handlers at load time. The patch is fairly >>> straightforward but there are still caveats, so each driver will need >>> careful review to make sure that userland drivers don't set up additional >>> state required for proper interrupt handling/enabling. It also means >>> drivers have to map registers at load time; the r128 bits in particular >>> looked funky in that regard so extra eyes there would be appreciated. >>> >>> I've only tested this patch so far on i915, where it's still slightly >>> broken; I was planning on fixing it once I've sync'd some more linux-core >>> changes into drm-next (namely the rest of the vblank-rework code). >>> >> Hi, >> >> I know this breaks via at least since the device is more or less dead >> until the X server programs the sequencer, so that >> would require a bunch of setup code at load time to fix. >> >> I agree it's a bad idea to keep the old irq ioctls, but why not get rid >> of the ioctls only and keep the old irq driver infrastructure and let >> the drivers call drm_irq_install() or drm_irq_uninstall() where fit, >> either in driver_load() / driver_unload() or driver_firstopen() / >> driver_lastclose()? >> > > Yeah that's definitely a possibility, thanks for looking at the via bits. > Though for kernel mode setting via will need to set that stuff up at load > time anyway, right? How hard would it be to port that code? > > Thanks, > It wouldn't be too hard to port VIA I think, but it requires some careful thoughts and close interaction with the 2D driver and thus would be a bit time-consuming. The situation is more complicated by the fact that there are three open-source VIA driver (not counting VIA's own) hanging around. :) /Thomas |
From: Stephane M. <mar...@ic...> - 2008-08-26 20:03:37
|
On Tue, Aug 26, 2008 at 9:36 PM, Jesse Barnes <jb...@vi...> wrote: > On Tuesday, August 26, 2008 12:03 pm Stephane Marchesin wrote: >> On Tue, Aug 26, 2008 at 8:57 PM, Jesse Barnes <jb...@vi...> > wrote: >> > The DRM design includes ioctls to allow a userland driver to tell the >> > kernel driver when to register its interrupt handler and on what IRQ. >> > This is a really bad idea for several reasons, and fortunately I don't >> > think any DDX drivers take advantage of the "no, use this IRQ" aspect of >> > the API (and even if they did the kernel driver would have to ignore it). >> > >> > This patch removes the DRM support for those ioctls, making drivers just >> > register their interrupt handlers at load time. The patch is fairly >> > straightforward but there are still caveats, so each driver will need >> > careful review to make sure that userland drivers don't set up additional >> > state required for proper interrupt handling/enabling. It also means >> > drivers have to map registers at load time; the r128 bits in particular >> > looked funky in that regard so extra eyes there would be appreciated. >> > >> > I've only tested this patch so far on i915, where it's still slightly >> > broken; I was planning on fixing it once I've sync'd some more linux-core >> > changes into drm-next (namely the rest of the vblank-rework code). >> >> This patch breaks a couple of drivers. Is this an oversight, or does >> the "new development model" mean that we break drivers that are not in >> linux each time ? > > 1) this is an RFC that hasn't been pushed anywhere yet, so nothing is actually > broken by it > 2) this patch is against the drm-next Linux tree, so any breakage is limited > to the drivers there > > But I assume you're talking about nouveau? nouveau, mach64, xgi... > Does it have any funky > requirements wrt IRQ registration? Or would it be relatively easy to move > its interrupt handler registration and IRQ setup to the load routine? > It is easy, but I'm realizing things might not be as easy for more intrusive changes. I'm merely pointing out that things don't seem to be wokring out when we have a lot of branches and global changes happen. In-development drivers are not fixed, other operating systems are not fixed, in-development features can conflict and so on... It really seems untractable, so it really seems to me need a single place where all the current DRM action happens and the merge to linux from there. Otherwise the open source graphics world will start to lose drivers and platform support. > As for the "new development model"... Things are actually worse than I > thought. There are some fairly large differences between linux-core and > upstream, some of which have been in linux-core for a long time. It's one > thing to have an out-of-tree development process but another entirely to let > stuff rot for months & years there. It just adds to the already huge set of > driver combinations we have to worry about and support... How is doing merges preventing us from working on a single tree ? It's two completely separate problems. > > So drm-next is all I care about anymore. I'm trying to sync the last couple > of years worth of development to that tree so I can start ignoring linux-core > entirely. It's just too disconnected from upstream Linux for me to worry > about (note that this doesn't mean I don't care about BSD compat; I want to > make sure sharing is still reasonably easy, but if anything I'd like the > merges to go from drm-next -> linux-core rather than the other way around for > my development). Ok, so to restate that clearly you'll break drivers that are not in mainstream linux routinely because you don't really care. Stephane |
From: Jesse B. <jb...@vi...> - 2008-08-26 20:21:57
|
On Tuesday, August 26, 2008 1:03 pm Stephane Marchesin wrote: > > As for the "new development model"... Things are actually worse than I > > thought. There are some fairly large differences between linux-core and > > upstream, some of which have been in linux-core for a long time. It's > > one thing to have an out-of-tree development process but another entirely > > to let stuff rot for months & years there. It just adds to the already > > huge set of driver combinations we have to worry about and support... > > How is doing merges preventing us from working on a single tree ? It's > two completely separate problems. There are several problems, see the earlier thread "Adapt on_each_cpu" where we talked about the current dev process problems. > > So drm-next is all I care about anymore. I'm trying to sync the last > > couple of years worth of development to that tree so I can start ignoring > > linux-core entirely. It's just too disconnected from upstream Linux for > > me to worry about (note that this doesn't mean I don't care about BSD > > compat; I want to make sure sharing is still reasonably easy, but if > > anything I'd like the merges to go from drm-next -> linux-core rather > > than the other way around for my development). > > Ok, so to restate that clearly you'll break drivers that are not in > mainstream linux routinely because you don't really care. I think this is an overreaction to an RFC. If I had pushed this into DRM master, leaving several drivers broken I think there would be real cause for complaint. However, I haven't done that. Nor has this gone into the Linux tree, so things aren't out of sync, so let's keep this in perspective. -- Jesse Barnes, Intel Open Source Technology Center |
From: Stephane M. <mar...@ic...> - 2008-08-26 20:28:46
|
On Tue, Aug 26, 2008 at 10:21 PM, Jesse Barnes <jb...@vi...> wrote: > On Tuesday, August 26, 2008 1:03 pm Stephane Marchesin wrote: >> > As for the "new development model"... Things are actually worse than I >> > thought. There are some fairly large differences between linux-core and >> > upstream, some of which have been in linux-core for a long time. It's >> > one thing to have an out-of-tree development process but another entirely >> > to let stuff rot for months & years there. It just adds to the already >> > huge set of driver combinations we have to worry about and support... >> >> How is doing merges preventing us from working on a single tree ? It's >> two completely separate problems. > > There are several problems, see the earlier thread "Adapt on_each_cpu" where > we talked about the current dev process problems. > I am outlining the fact that you confuse a problem and its solution. Problem: merging stuff upstream takes time to Dave Your solution: have lots of in-development trees and everyone upstreams its stuff which breaks other drivers and platforms A possible solution: everyone upstreams its stuff from a common tree which keeps other drivers and platforms working Maybe there are other solutions, I don't know. In any case, I would like to see a new model being agreed upon by the developers. I have not seen such an agreement yet. Stephane |
From: Jesse B. <jb...@vi...> - 2008-08-26 20:50:47
|
On Tuesday, August 26, 2008 1:28 pm Stephane Marchesin wrote: > On Tue, Aug 26, 2008 at 10:21 PM, Jesse Barnes <jb...@vi...> wrote: > > On Tuesday, August 26, 2008 1:03 pm Stephane Marchesin wrote: > >> > As for the "new development model"... Things are actually worse than I > >> > thought. There are some fairly large differences between linux-core > >> > and upstream, some of which have been in linux-core for a long time. > >> > It's one thing to have an out-of-tree development process but another > >> > entirely to let stuff rot for months & years there. It just adds to > >> > the already huge set of driver combinations we have to worry about and > >> > support... > >> > >> How is doing merges preventing us from working on a single tree ? It's > >> two completely separate problems. > > > > There are several problems, see the earlier thread "Adapt on_each_cpu" > > where we talked about the current dev process problems. > > I am outlining the fact that you confuse a problem and its solution. > > Problem: merging stuff upstream takes time to Dave > Your solution: have lots of in-development trees and everyone > upstreams its stuff which breaks other drivers and platforms > A possible solution: everyone upstreams its stuff from a common tree > which keeps other drivers and platforms working > > Maybe there are other solutions, I don't know. In any case, I would > like to see a new model being agreed upon by the developers. I have > not seen such an agreement yet. I'll worry about this when upstream Linux is relatively up to date again. At that point as new features are developed we can discuss how best to merge them, where to merge them first, etc. That said, GEM is an example of a problematic subsystem wrt merging (like TTM before it). It's a big, new feature, touching lots of code. In order to ever get merged into Linux it has to go through review, get changed/updated, etc. Doing all that work in DRM master then merging into Linux is just wasted effort. Why not do the work against Linux until it becomes acceptable, then merge into DRM master? Jesse |
From: Jesse B. <jb...@vi...> - 2008-08-26 21:21:03
|
On Tuesday, August 26, 2008 1:27 pm Stephane Marchesin wrote: > On Tue, Aug 26, 2008 at 10:21 PM, Jesse Barnes <jb...@vi...> wrote: > > On Tuesday, August 26, 2008 1:03 pm Stephane Marchesin wrote: > >> > As for the "new development model"... Things are actually worse than I > >> > thought. There are some fairly large differences between linux-core > >> > and upstream, some of which have been in linux-core for a long time. > >> > It's one thing to have an out-of-tree development process but another > >> > entirely to let stuff rot for months & years there. It just adds to > >> > the already huge set of driver combinations we have to worry about and > >> > support... > >> > >> How is doing merges preventing us from working on a single tree ? It's > >> two completely separate problems. > > > > There are several problems, see the earlier thread "Adapt on_each_cpu" > > where we talked about the current dev process problems. > > I am outlining the fact that you confuse a problem and its solution. > > Problem: merging stuff upstream takes time to Dave > Your solution: have lots of in-development trees and everyone > upstreams its stuff which breaks other drivers and platforms > A possible solution: everyone upstreams its stuff from a common tree > which keeps other drivers and platforms working Figured I should reply to this too (still trying to cool off after our heated discussion on IRC). I'm definitely *not* proposing a bunch of in-development trees to break other drivers and platforms. I don't want to break other drivers and platforms. Let me state again my issues: 1) drm master is way out of date wrt upstream Linux (and yes this is my fault too). This sucks and I'm trying to fix it by sending Dave easily digestible and tested patches against the tree he'll send to Linus for 2.6.28. 2) our current development process makes it fairly easy to get into situation (1). It's far too easy to push something into drm master and assume Dave will take care of it. It's also easy to push something in that the BSD guys miss, causing them to do extra work next time they merge ("hmm wtf is this new code supposed to do?") 3) dealing with ongoing Linux changes in the drm tree is harder than it has to be (we just continually add more compat code all the time, and things often break with new kernel updates) So really I'm just looking for solutions to (2) and (3). For me, that means developing & testing against a Linux tree first, since that's what goes upstream and that's what I have to support. It doesn't mean I can't push the resulting patches into drm master or work with people to make sure things get ported to BSD, etc. I don't think there are any silver bullets here. No matter what we do I think it's a lot of work. On a more personal note, statements like the below really suck: ... <marcheu> jbarnes: it was working fine before you were here, dude ... <MrCooper> yeah, it was fine before people stopped caring for anything but their little niche ... I've worked hard to make sure changes I make update all drivers in the DRM master tree, and worked with the other OS guys to keep things up to date, making things better for everyone in the process. If I've made anyone's life harder, I'm sorry, that's definitely not my intention. When I said, "So drm-next is all I care about anymore", it was more out of frustration than anything else. It's certainly all I care about *at the moment* since I'm trying to get most of the stuff from linux-core into upstream Linux, but that doesn't mean I'll stop working on drm master on other drivers or OSes. Jesse |
From: Stephane M. <mar...@ic...> - 2008-08-26 22:25:13
|
On Tue, Aug 26, 2008 at 11:17 PM, Jesse Barnes <jb...@vi...> wrote: > On Tuesday, August 26, 2008 1:27 pm Stephane Marchesin wrote: >> >> I am outlining the fact that you confuse a problem and its solution. >> >> Problem: merging stuff upstream takes time to Dave >> Your solution: have lots of in-development trees and everyone >> upstreams its stuff which breaks other drivers and platforms >> A possible solution: everyone upstreams its stuff from a common tree >> which keeps other drivers and platforms working > > Figured I should reply to this too (still trying to cool off after our heated > discussion on IRC). > > I'm definitely *not* proposing a bunch of in-development trees to break other > drivers and platforms. I don't want to break other drivers and platforms. > Let me state again my issues: > 1) drm master is way out of date wrt upstream Linux (and yes this is my > fault too). This sucks and I'm trying to fix it by sending Dave easily > digestible and tested patches against the tree he'll send to Linus for > 2.6.28. It seems to me that this is putting more work on Dave, because in the end he'll be the one merging back from the kernel to linux-core. But then the stuff he merges might have become stale so it's more work. Not to mention the linux-core directory also has things to upstream, so we're talking 2-way merges. > 2) our current development process makes it fairly easy to get into > situation (1). It's far too easy to push something into drm master and > assume Dave will take care of it. It's also easy to push something in > that the BSD guys miss, causing them to do extra work next time they > merge ("hmm wtf is this new code supposed to do?") Sure, to which a possible answer is "upstream your own stuff". Again I suppose there could be more answers, but it's a simple first step. Maybe once the un-upstreamed stuff is cleared, we can move on more easily with other things. > 3) dealing with ongoing Linux changes in the drm tree is harder than it has > to be (we just continually add more compat code all the time, and things > often break with new kernel updates) OTOH with the current scheme developers/testers can keep their old kernel. All the nouveau users do that for example, the backwards compatibility doesn't seem impossible to do and that certainly didn't preven the DRM from working for years. > > So really I'm just looking for solutions to (2) and (3). For me, that means > developing & testing against a Linux tree first, since that's what goes > upstream and that's what I have to support. It doesn't mean I can't push the > resulting patches into drm master or work with people to make sure things get > ported to BSD, etc. I don't think there are any silver bullets here. No > matter what we do I think it's a lot of work. > > On a more personal note, statements like the below really suck: > ... > <marcheu> jbarnes: it was working fine before you were here, dude > ... Oh, I realize this statement could be misinterpreted as "you're responsible for breaking the drm" or some such. I meant "our drm development model was working fine for a long time before you were here, so why change it". We used to support Linux/BSD and under development/un-upstreamed drivers in a single tree. We might lose this and I think that's a mistake. If you ask me we "just" need to change the "who's upstreaming what" bit for now. I don't think there is an inherent problem to sharing the DRM tree. ><MrCooper> yeah, it was fine before people stopped caring for anything but > their little niche > ... The core of the issue is there though. Do we still want to share stuff, or do we want to work in our own separate trees. And if we have separate trees, what's the policy for the shared infrastructure: how are the interfaces designed, how many drivers must implement it before upstreaming it... Stephane |
From: Robert N. <rn...@2h...> - 2008-08-26 23:49:20
|
On Tue, 2008-08-26 at 14:17 -0700, Jesse Barnes wrote: > On Tuesday, August 26, 2008 1:27 pm Stephane Marchesin wrote: > > On Tue, Aug 26, 2008 at 10:21 PM, Jesse Barnes <jb...@vi...> > wrote: > > > On Tuesday, August 26, 2008 1:03 pm Stephane Marchesin wrote: > > >> > As for the "new development model"... Things are actually worse than I > > >> > thought. There are some fairly large differences between linux-core > > >> > and upstream, some of which have been in linux-core for a long time. > > >> > It's one thing to have an out-of-tree development process but another > > >> > entirely to let stuff rot for months & years there. It just adds to > > >> > the already huge set of driver combinations we have to worry about and > > >> > support... > > >> > > >> How is doing merges preventing us from working on a single tree ? It's > > >> two completely separate problems. > > > > > > There are several problems, see the earlier thread "Adapt on_each_cpu" > > > where we talked about the current dev process problems. > > > > I am outlining the fact that you confuse a problem and its solution. > > > > Problem: merging stuff upstream takes time to Dave > > Your solution: have lots of in-development trees and everyone > > upstreams its stuff which breaks other drivers and platforms > > A possible solution: everyone upstreams its stuff from a common tree > > which keeps other drivers and platforms working > > Figured I should reply to this too (still trying to cool off after our heated > discussion on IRC). > > I'm definitely *not* proposing a bunch of in-development trees to break other > drivers and platforms. I don't want to break other drivers and platforms. > Let me state again my issues: > 1) drm master is way out of date wrt upstream Linux (and yes this is my > fault too). This sucks and I'm trying to fix it by sending Dave easily > digestible and tested patches against the tree he'll send to Linus for > 2.6.28. > 2) our current development process makes it fairly easy to get into > situation (1). It's far too easy to push something into drm master and > assume Dave will take care of it. It's also easy to push something in > that the BSD guys miss, causing them to do extra work next time they > merge ("hmm wtf is this new code supposed to do?") > 3) dealing with ongoing Linux changes in the drm tree is harder than it has > to be (we just continually add more compat code all the time, and things > often break with new kernel updates) > > So really I'm just looking for solutions to (2) and (3). For me, that means > developing & testing against a Linux tree first, since that's what goes > upstream and that's what I have to support. It doesn't mean I can't push the > resulting patches into drm master or work with people to make sure things get > ported to BSD, etc. I don't think there are any silver bullets here. No > matter what we do I think it's a lot of work. I'm certainly not saying that, you particularly have always been good about working with and helping me to get stuff going on bsd. I am also really appreciative of the work and resources that you guys puts toward development. I still see the biggest part of your frustration being that stuff hasn't been merged and I just don't see that as a problem with where we develop code. That just seems like a failure to get it (merging upstream) done... Like I said on irc, I am all for trying to make drm.git a happier, friendlier place for everyone. I think Dave had some good ideas for how to try and ease everyones pain... In the end, all I really want is to be able to support the features that userland apps want to use. I realize that I'm a second class citizen here, so I'm willing to accept some compromises as long as it doesn't make things overly difficult for me to continue trying to improve things for FreeBSD. You guys certainly have the right to spend your time and effort in whatever way you choose. As I said before, I think that it's great that you guys do the work that you do. I don't see how that makes it fair to strong-arm everyone else into doing it your way, when it seems that we are far from concensus that it is the right way forward. You guys don't even have concensus among the linux crowd. I know that you would have good intentions of merging changes back into drm.git, but I don't think that would last very long. It doesn't really matter which direction your merging changes into, it's going to involve some quantity of pain... and if you already have it in a linux tree, I just don't see anyone willing to put in the effort to merge it to master. Your problem is that feature X is under development, it isn't stable and ready to go into a released linux kernel, so it sits and starts to bitrot... I can't see how it matters if it sits in linux kernel tree or drm.git, it still has to get stabilized and tested before it can get pushed upstream. Would TTM really have made it upstream faster if the code was developed in a linux? I think your going to have the same problem no matter which development model you take. Features have to get tested and someone has to put in the effort to get them upstream. As far as testing goes... I think a bit of publicity would go a long way. As far as I can see, it's difficult for people to see what is being developed and how it might effect them. If it was clear what features were being worked on and in what branches. I think more people would be willing and capable of testing in development code. In my experience a fair number of people are willing and capable of running the git master branch. I don't think the other branches get a lot of excercise except by the git savvy. I think if someone took the time to properly document the development efforts and tell people how they can test the relevant branches, we would see more testing. That said, I'm probably the worlds worst for doing it... robert. > On a more personal note, statements like the below really suck: > ... > <marcheu> jbarnes: it was working fine before you were here, dude > ... > <MrCooper> yeah, it was fine before people stopped caring for anything but > their little niche > ... > > I've worked hard to make sure changes I make update all drivers in the DRM > master tree, and worked with the other OS guys to keep things up to date, > making things better for everyone in the process. If I've made anyone's life > harder, I'm sorry, that's definitely not my intention. When I said, "So > drm-next is all I care about anymore", it was more out of frustration than > anything else. It's certainly all I care about *at the moment* since I'm > trying to get most of the stuff from linux-core into upstream Linux, but that > doesn't mean I'll stop working on drm master on other drivers or OSes. > > Jesse > > ------------------------------------------------------------------------- > This SF.Net email is sponsored by the Moblin Your Move Developer's challenge > Build the coolest Linux based applications with Moblin SDK & win great prizes > Grand prize is a trip for two to an Open Source event anywhere in the world > http://moblin-contest.org/redirect.php?banner_id=100&url=/ > -- > _______________________________________________ > Dri-devel mailing list > Dri...@li... > https://lists.sourceforge.net/lists/listinfo/dri-devel -- Robert Noland <rn...@2h...> 2Hip Networks |
From: Dave A. <ai...@gm...> - 2008-08-26 23:15:20
|
On Wed, Aug 27, 2008 at 5:23 AM, Thomas Hellström <th...@tu...> wrote: > Jesse Barnes wrote: >> The DRM design includes ioctls to allow a userland driver to tell the kernel >> driver when to register its interrupt handler and on what IRQ. This is a >> really bad idea for several reasons, and fortunately I don't think any DDX >> drivers take advantage of the "no, use this IRQ" aspect of the API (and even >> if they did the kernel driver would have to ignore it). >> >> This patch removes the DRM support for those ioctls, making drivers just >> register their interrupt handlers at load time. The patch is fairly >> straightforward but there are still caveats, so each driver will need careful >> review to make sure that userland drivers don't set up additional state >> required for proper interrupt handling/enabling. It also means drivers have >> to map registers at load time; the r128 bits in particular looked funky in >> that regard so extra eyes there would be appreciated. >> >> I've only tested this patch so far on i915, where it's still slightly broken; >> I was planning on fixing it once I've sync'd some more linux-core changes >> into drm-next (namely the rest of the vblank-rework code). >> > Hi, > > I know this breaks via at least since the device is more or less dead > until the X server programs the sequencer, so that > would require a bunch of setup code at load time to fix. > > I agree it's a bad idea to keep the old irq ioctls, but why not get rid > of the ioctls only and keep the old irq driver infrastructure and let > the drivers call drm_irq_install() or drm_irq_uninstall() where fit, > either in driver_load() / driver_unload() or driver_firstopen() / > driver_lastclose()? > This is the only practical solution that might work, however even doing this may prove to be a regression nightmare. Some userspace drivers write to the IRQ control registers themselves. I really think the practical solution is to look the other way, get modesetting drivers, and just ignore this functionality for drivers that own the card. Otherwise I fear actually getting this code upstream and working would be an outright regression-fest. Dave. |
From: Jesse B. <jb...@vi...> - 2008-08-27 00:01:58
|
On Tuesday, August 26, 2008 4:15 pm Dave Airlie wrote: > On Wed, Aug 27, 2008 at 5:23 AM, Thomas Hellström > >> I've only tested this patch so far on i915, where it's still slightly > >> broken; I was planning on fixing it once I've sync'd some more > >> linux-core changes into drm-next (namely the rest of the vblank-rework > >> code). > > > > Hi, > > > > I know this breaks via at least since the device is more or less dead > > until the X server programs the sequencer, so that > > would require a bunch of setup code at load time to fix. > > > > I agree it's a bad idea to keep the old irq ioctls, but why not get rid > > of the ioctls only and keep the old irq driver infrastructure and let > > the drivers call drm_irq_install() or drm_irq_uninstall() where fit, > > either in driver_load() / driver_unload() or driver_firstopen() / > > driver_lastclose()? > > This is the only practical solution that might work, however even > doing this may prove to be a regression nightmare. > Some userspace drivers write to the IRQ control registers themselves. > > I really think the practical solution is to look the other way, get > modesetting drivers, and just ignore this functionality for > drivers that own the card. > > Otherwise I fear actually getting this code upstream and working would > be an outright regression-fest. Yeah, it would likely get ugly unless we were really careful about each driver; and I don't know enough about how the various 2D sides work to do that kind of audit myself. And there's nothing to stop us from doing this carefully for each driver first, and only then ripping out the core code. I think we could manage this in i915 fairly easily at least, but even then I don't think we'd want to do it until the vblank-rework stuff is upstream (which I'm working on now). Thanks, -- Jesse Barnes, Intel Open Source Technology Center |
From: Robert N. <rn...@2h...> - 2008-08-27 00:13:20
|
On Wed, 2008-08-27 at 09:15 +1000, Dave Airlie wrote: > On Wed, Aug 27, 2008 at 5:23 AM, Thomas Hellström > <th...@tu...> wrote: > > Jesse Barnes wrote: > >> The DRM design includes ioctls to allow a userland driver to tell the kernel > >> driver when to register its interrupt handler and on what IRQ. This is a > >> really bad idea for several reasons, and fortunately I don't think any DDX > >> drivers take advantage of the "no, use this IRQ" aspect of the API (and even > >> if they did the kernel driver would have to ignore it). > >> > >> This patch removes the DRM support for those ioctls, making drivers just > >> register their interrupt handlers at load time. The patch is fairly > >> straightforward but there are still caveats, so each driver will need careful > >> review to make sure that userland drivers don't set up additional state > >> required for proper interrupt handling/enabling. It also means drivers have > >> to map registers at load time; the r128 bits in particular looked funky in > >> that regard so extra eyes there would be appreciated. > >> > >> I've only tested this patch so far on i915, where it's still slightly broken; > >> I was planning on fixing it once I've sync'd some more linux-core changes > >> into drm-next (namely the rest of the vblank-rework code). > >> > > Hi, > > > > I know this breaks via at least since the device is more or less dead > > until the X server programs the sequencer, so that > > would require a bunch of setup code at load time to fix. > > > > I agree it's a bad idea to keep the old irq ioctls, but why not get rid > > of the ioctls only and keep the old irq driver infrastructure and let > > the drivers call drm_irq_install() or drm_irq_uninstall() where fit, > > either in driver_load() / driver_unload() or driver_firstopen() / > > driver_lastclose()? > > > > This is the only practical solution that might work, however even > doing this may prove to be a regression nightmare. > Some userspace drivers write to the IRQ control registers themselves. I kinda prompted this topic to come back to life... I'm less concerned with the IRQ bits than I am with the vblank issue, but they are currently tied together. What I'm seeing right now, it that at least the intel ddx driver calls the irq_uninstall ioctl before calling pre_modeset ioctl during vt switch. The way the code is now, irq_uninstall calls vblank_cleanup and clears all of the structures and resets num_crtcs to 0. When it hits modeset_ioctl it tests for num_crtcs and thinks the vblank_init hasn't been called and bails out and doesn't properly enable vblank over the modeset. When returning from vt switch, it is doing the opposite. It calls modeset_ctl ioctl, num_crtcs is still 0, because vblank_init doesn't get called until the irq_install ioctl gets called. So, the short story is that after a vt switch, vlbanks aren't allowed to be disabled. I had a patch around here somewhere that only moved the vblank_init and cleanup routines to the driver load / unload routines. This put the entire responsibilty on the driver, while not touching the irq install / uninstall routines. IIRC, I actually sent that patch to this list... It will require very minor changes to the linux driver bits, just to call vblank_init/cleanup at the right places. IMO, this is the way to go... I don't want to rely on the ddx driver to DTRT any more than I have to... When I mentioned something about this originally, someone stated that it would be good to just install the handlers at load time, and get rid of pre/post install. I ran into problems when I tried to go down that path, so I just settled to only move the vblank bits... robert. > I really think the practical solution is to look the other way, get > modesetting drivers, and just ignore this functionality for > drivers that own the card. > > Otherwise I fear actually getting this code upstream and working would > be an outright regression-fest. > > Dave. > > ------------------------------------------------------------------------- > This SF.Net email is sponsored by the Moblin Your Move Developer's challenge > Build the coolest Linux based applications with Moblin SDK & win great prizes > Grand prize is a trip for two to an Open Source event anywhere in the world > http://moblin-contest.org/redirect.php?banner_id=100&url=/ > -- > _______________________________________________ > Dri-devel mailing list > Dri...@li... > https://lists.sourceforge.net/lists/listinfo/dri-devel -- Robert Noland <rn...@2h...> 2Hip Networks |
From: Jesse B. <jb...@vi...> - 2008-08-27 01:02:05
|
On Tuesday, August 26, 2008 5:13 pm Robert Noland wrote: > I kinda prompted this topic to come back to life... I'm less concerned > with the IRQ bits than I am with the vblank issue, but they are > currently tied together. > > What I'm seeing right now, it that at least the intel ddx driver calls > the irq_uninstall ioctl before calling pre_modeset ioctl during vt > switch. The way the code is now, irq_uninstall calls vblank_cleanup and > clears all of the structures and resets num_crtcs to 0. When it hits > modeset_ioctl it tests for num_crtcs and thinks the vblank_init hasn't > been called and bails out and doesn't properly enable vblank over the > modeset. When returning from vt switch, it is doing the opposite. It > calls modeset_ctl ioctl, num_crtcs is still 0, because vblank_init > doesn't get called until the irq_install ioctl gets called. So, the > short story is that after a vt switch, vlbanks aren't allowed to be > disabled. > > I had a patch around here somewhere that only moved the vblank_init and > cleanup routines to the driver load / unload routines. This put the > entire responsibilty on the driver, while not touching the irq install / > uninstall routines. IIRC, I actually sent that patch to this list... > It will require very minor changes to the linux driver bits, just to > call vblank_init/cleanup at the right places. IMO, this is the way to > go... I don't want to rely on the ddx driver to DTRT any more than I > have to... > > When I mentioned something about this originally, someone stated that it > would be good to just install the handlers at load time, and get rid of > pre/post install. I ran into problems when I tried to go down that > path, so I just settled to only move the vblank bits... I'll post the upstream vblank-rework code as soon as I have it working well; fixing this particular problem should be fairly easy w/o having to rip out all the IRQ stuff (though I was kinda hoping that would be the solution, Dave is right that it's just a bunch of regressions waiting to happen). -- Jesse Barnes, Intel Open Source Technology Center |
From: Robert N. <rn...@2h...> - 2008-08-27 04:03:37
|
On Tue, 2008-08-26 at 18:02 -0700, Jesse Barnes wrote: > On Tuesday, August 26, 2008 5:13 pm Robert Noland wrote: > > I kinda prompted this topic to come back to life... I'm less concerned > > with the IRQ bits than I am with the vblank issue, but they are > > currently tied together. > > > > What I'm seeing right now, it that at least the intel ddx driver calls > > the irq_uninstall ioctl before calling pre_modeset ioctl during vt > > switch. The way the code is now, irq_uninstall calls vblank_cleanup and > > clears all of the structures and resets num_crtcs to 0. When it hits > > modeset_ioctl it tests for num_crtcs and thinks the vblank_init hasn't > > been called and bails out and doesn't properly enable vblank over the > > modeset. When returning from vt switch, it is doing the opposite. It > > calls modeset_ctl ioctl, num_crtcs is still 0, because vblank_init > > doesn't get called until the irq_install ioctl gets called. So, the > > short story is that after a vt switch, vlbanks aren't allowed to be > > disabled. > > > > I had a patch around here somewhere that only moved the vblank_init and > > cleanup routines to the driver load / unload routines. This put the > > entire responsibilty on the driver, while not touching the irq install / > > uninstall routines. IIRC, I actually sent that patch to this list... > > It will require very minor changes to the linux driver bits, just to > > call vblank_init/cleanup at the right places. IMO, this is the way to > > go... I don't want to rely on the ddx driver to DTRT any more than I > > have to... > > > > When I mentioned something about this originally, someone stated that it > > would be good to just install the handlers at load time, and get rid of > > pre/post install. I ran into problems when I tried to go down that > > path, so I just settled to only move the vblank bits... > > I'll post the upstream vblank-rework code as soon as I have it working well; > fixing this particular problem should be fairly easy w/o having to rip out > all the IRQ stuff (though I was kinda hoping that would be the solution, Dave > is right that it's just a bunch of regressions waiting to happen). The patch that I sent on July 24, does the right thing, but it won't apply cleanly any more... It included the infrastructure changes and fixed intel and radeon on BSD. But, yeah it's not hard to DTRT in this case. I still maintain that if the driver sets up the vblank structures it should be responsible for tearing them down. robert. |
From: Jesse B. <jb...@vi...> - 2008-08-27 04:40:34
|
On Tuesday, August 26, 2008 9:03 pm Robert Noland wrote: > On Tue, 2008-08-26 at 18:02 -0700, Jesse Barnes wrote: > > On Tuesday, August 26, 2008 5:13 pm Robert Noland wrote: > > > I kinda prompted this topic to come back to life... I'm less concerned > > > with the IRQ bits than I am with the vblank issue, but they are > > > currently tied together. > > > > > > What I'm seeing right now, it that at least the intel ddx driver calls > > > the irq_uninstall ioctl before calling pre_modeset ioctl during vt > > > switch. The way the code is now, irq_uninstall calls vblank_cleanup > > > and clears all of the structures and resets num_crtcs to 0. When it > > > hits modeset_ioctl it tests for num_crtcs and thinks the vblank_init > > > hasn't been called and bails out and doesn't properly enable vblank > > > over the modeset. When returning from vt switch, it is doing the > > > opposite. It calls modeset_ctl ioctl, num_crtcs is still 0, because > > > vblank_init doesn't get called until the irq_install ioctl gets called. > > > So, the short story is that after a vt switch, vlbanks aren't allowed > > > to be disabled. > > > > > > I had a patch around here somewhere that only moved the vblank_init and > > > cleanup routines to the driver load / unload routines. This put the > > > entire responsibilty on the driver, while not touching the irq install > > > / uninstall routines. IIRC, I actually sent that patch to this list... > > > It will require very minor changes to the linux driver bits, just to > > > call vblank_init/cleanup at the right places. IMO, this is the way to > > > go... I don't want to rely on the ddx driver to DTRT any more than I > > > have to... > > > > > > When I mentioned something about this originally, someone stated that > > > it would be good to just install the handlers at load time, and get rid > > > of pre/post install. I ran into problems when I tried to go down that > > > path, so I just settled to only move the vblank bits... > > > > I'll post the upstream vblank-rework code as soon as I have it working > > well; fixing this particular problem should be fairly easy w/o having to > > rip out all the IRQ stuff (though I was kinda hoping that would be the > > solution, Dave is right that it's just a bunch of regressions waiting to > > happen). > > The patch that I sent on July 24, does the right thing, but it won't > apply cleanly any more... It included the infrastructure changes and > fixed intel and radeon on BSD. But, yeah it's not hard to DTRT in this > case. I still maintain that if the driver sets up the vblank structures > it should be responsible for tearing them down. Agreed, I'll fix that up. Thanks, -- Jesse Barnes, Intel Open Source Technology Center |