Re: [Intel-gfx] [RFC PATCH 00/42] Introduce memory region concept (including device local memory)
Quoting Christian König (2019-02-27 04:17:01) > Am 27.02.19 um 00:04 schrieb Dave Airlie: > >>> At the end of the day, I don't really care that much. I get it, we > >>> all have large projects with scarce resources. I just think a few > >>> years down the road we'll all regret it as a community. > >> AMD and others have also spent years tuning TTM for both UMA and VRAM, > >> but especially VRAM. It comes across a bit daft to complain about the > >> effort to move to TTM, but say nothing about the effort to tune GEM > >> for optimal VRAM performance. Effort that has already been expended > >> that you could take advantage of. > > I'm with Alex here, the patches you have now are just the start, I > > realise you think they are all you'll need, but I expect once Chris > > gets going with real VRAM hardware he'll generate reams for stuff. > > > > People have been tuning and making TTM run on VRAM using GPUs for > > longer than you've been making VRAM using GPUs, there had better be > > good and well thought out reasons for avoiding using it, and so far > > you haven't made that argument to me all. In fact your scheduler > > arguments works against you. If we should have abstracted i915 > > scheduler out and used it because it had more features and > > pre-existed, then i915 should be using TTM since it's already > > abstracted out and has more features. > > > > Like we've pulled other stuff out of TTM like reservation objects, I > > don't think i915 uses those yet either when it clearly could be. Maybe > > if we started by fixing that, moving to TTM would be less of a > > problem. > > Just to make it extra clear: At least I absolutely won't mind if we > decommission TTM further! > > We have optimized TTM as much as we could without a fundamental design > change, but essentially there are a couple of problem we can't fix > without touching all drivers at once. > > For example the layered design where TTM calls back into the driver to > move stuff around or allocate something from a domain really needs to go > away. > > So my suggestion is that we filleting TTM into multiple independent > components which a) can be used to implement the existing TTM interface > and b) implement a clean and encapsulated functionality. > > Those components can then be used by drivers independently of TTM to > implement the necessary MM. This is exactly what I was hoping we could do, too. So I'm glad to hear this suggestion. Incrementally extracting and sharing the components would lead to less disruptions than halting the progress while doing a major rewrite of the driver. As I mentioned in IRC, one good step for both the scheduler and memory management would be to actually map out the feature sets. It is clear that we have confusion about what the feature set of the respective components are (at least I do seem to have about TTM / DRM scheduler). Then when we understand what it is that we actually have, it should be easier to start discussing which are the components that could be reused. I think one good way to take on this would be to look into sharing as much of the test assets as possible. We have plenty of testcases excercising the low-on-memory conditions and scheduling corner cases that we've had to tackle. And I'm sure there are tests for the above mentioned TTM VRAM tuning, too. I'll look into and discuss the reservation objects Dave mentioned, and I'm happy to hear about other suggestions. I'd also like to hear comments about the buddy allocator suggestion. It should help in enabling >4K page support for pretty much any driver. Regards, Joonas > Regards, > Christian. > > > > > Anyways, I expect moving to TTM is a big change for i915, and probably > > more than you are able to bite off at present, but I'm going to be > > watching closely what stuff you add on top of this sort of thing, and > > if it starts getting large and messier as you tune it, I'll have to > > start reconsidering how big a NO I have to use. > > > > Dave. > > ___ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [RFC PATCH 00/42] Introduce memory region concept (including device local memory)
Am 27.02.19 um 00:04 schrieb Dave Airlie: At the end of the day, I don't really care that much. I get it, we all have large projects with scarce resources. I just think a few years down the road we'll all regret it as a community. AMD and others have also spent years tuning TTM for both UMA and VRAM, but especially VRAM. It comes across a bit daft to complain about the effort to move to TTM, but say nothing about the effort to tune GEM for optimal VRAM performance. Effort that has already been expended that you could take advantage of. I'm with Alex here, the patches you have now are just the start, I realise you think they are all you'll need, but I expect once Chris gets going with real VRAM hardware he'll generate reams for stuff. People have been tuning and making TTM run on VRAM using GPUs for longer than you've been making VRAM using GPUs, there had better be good and well thought out reasons for avoiding using it, and so far you haven't made that argument to me all. In fact your scheduler arguments works against you. If we should have abstracted i915 scheduler out and used it because it had more features and pre-existed, then i915 should be using TTM since it's already abstracted out and has more features. Like we've pulled other stuff out of TTM like reservation objects, I don't think i915 uses those yet either when it clearly could be. Maybe if we started by fixing that, moving to TTM would be less of a problem. Just to make it extra clear: At least I absolutely won't mind if we decommission TTM further! We have optimized TTM as much as we could without a fundamental design change, but essentially there are a couple of problem we can't fix without touching all drivers at once. For example the layered design where TTM calls back into the driver to move stuff around or allocate something from a domain really needs to go away. So my suggestion is that we filleting TTM into multiple independent components which a) can be used to implement the existing TTM interface and b) implement a clean and encapsulated functionality. Those components can then be used by drivers independently of TTM to implement the necessary MM. Regards, Christian. Anyways, I expect moving to TTM is a big change for i915, and probably more than you are able to bite off at present, but I'm going to be watching closely what stuff you add on top of this sort of thing, and if it starts getting large and messier as you tune it, I'll have to start reconsidering how big a NO I have to use. Dave. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [RFC PATCH 00/42] Introduce memory region concept (including device local memory)
> > At the end of the day, I don't really care that much. I get it, we > > all have large projects with scarce resources. I just think a few > > years down the road we'll all regret it as a community. > > AMD and others have also spent years tuning TTM for both UMA and VRAM, > but especially VRAM. It comes across a bit daft to complain about the > effort to move to TTM, but say nothing about the effort to tune GEM > for optimal VRAM performance. Effort that has already been expended > that you could take advantage of. I'm with Alex here, the patches you have now are just the start, I realise you think they are all you'll need, but I expect once Chris gets going with real VRAM hardware he'll generate reams for stuff. People have been tuning and making TTM run on VRAM using GPUs for longer than you've been making VRAM using GPUs, there had better be good and well thought out reasons for avoiding using it, and so far you haven't made that argument to me all. In fact your scheduler arguments works against you. If we should have abstracted i915 scheduler out and used it because it had more features and pre-existed, then i915 should be using TTM since it's already abstracted out and has more features. Like we've pulled other stuff out of TTM like reservation objects, I don't think i915 uses those yet either when it clearly could be. Maybe if we started by fixing that, moving to TTM would be less of a problem. Anyways, I expect moving to TTM is a big change for i915, and probably more than you are able to bite off at present, but I'm going to be watching closely what stuff you add on top of this sort of thing, and if it starts getting large and messier as you tune it, I'll have to start reconsidering how big a NO I have to use. Dave. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [RFC PATCH 00/42] Introduce memory region concept (including device local memory)
On Tue, Feb 26, 2019 at 12:20 PM Alex Deucher wrote: > > On Tue, Feb 26, 2019 at 7:17 AM Joonas Lahtinen > wrote: > > > > Quoting Alex Deucher (2019-02-25 21:31:43) > > > On Mon, Feb 25, 2019 at 9:35 PM Joonas Lahtinen > > > wrote: > > > > > > > > Quoting Dave Airlie (2019-02-25 12:24:48) > > > > > On Tue, 19 Feb 2019 at 23:32, Joonas Lahtinen > > > > > wrote: > > > > > > > > > > > > + dri-devel mailing list, especially for the buddy allocator part > > > > > > > > > > > > Quoting Dave Airlie (2019-02-15 02:47:07) > > > > > > > On Fri, 15 Feb 2019 at 00:57, Matthew Auld > > > > > > > wrote: > > > > > > > > > > > > > > > > In preparation for upcoming devices with device local memory, > > > > > > > > introduce the > > > > > > > > concept of different memory regions, and a simple buddy > > > > > > > > allocator to manage > > > > > > > > them. > > > > > > > > > > > > > > This is missing the information on why it's not TTM. > > > > > > > > > > > > > > Nothing against extending i915 gem off into doing stuff we already > > > > > > > have examples off in tree, but before you do that it would be > > > > > > > good to > > > > > > > have a why we can't use TTM discussion in public. > > > > > > > > > > > > Glad that you asked. It's my fault that it was not included in > > > > > > the cover letter. I anticipated the question, but was travelling > > > > > > for a couple of days at the time this was sent. I didn't want > > > > > > to write a hasty explanation and then disappear, leaving others to > > > > > > take the heat. > > > > > > > > > > > > So here goes the less-hasty version: > > > > > > > > > > > > We did an analysis on the effort needed vs benefit gained of using > > > > > > TTM when this was started initially. The conclusion was that we > > > > > > already share the interesting bits of code through core DRM, really. > > > > > > > > > > > > Re-writing the memory handling to TTM would buy us more fine-grained > > > > > > locking. But it's more a trait of rewriting the memory handling with > > > > > > the information we have learned, than rewriting it to use TTM :) > > > > > > > > > > > > And further, we've been getting rid of struct_mutex at a steady > > > > > > phase > > > > > > in the past years, so we have a clear path to the fine-grained > > > > > > locking > > > > > > already in the not-so-distant future. With all this we did not see > > > > > > much gained from converting over, as the code sharing is already > > > > > > substantial. > > > > > > > > > > > > We also wanted to have the buddy allocator instead of a for loop > > > > > > making > > > > > > drm_mm allocations to make sure we can keep the memory fragmentation > > > > > > at bay. The intent is to move the buddy allocator to core DRM, to > > > > > > the > > > > > > benefit of all the drivers, if there is interest from community. It > > > > > > has > > > > > > been written as a strictly separate component with that in mind. > > > > > > > > > > > > And if you take the buddy allocator out of the patch set, the rest > > > > > > is > > > > > > mostly just vfuncing things up to be able to have different backing > > > > > > storages for objects. We took the opportunity to move over to the > > > > > > more > > > > > > valgrind friendly mmap while touching things, but it's something we > > > > > > have been contemplating anyway. And yeah, loads of selftests. > > > > > > > > > > > > That's really all that needed adding, and most of it is internal to > > > > > > i915 and not to do with uAPI. This means porting over an userspace > > > > > > driver doesn't require a substantial rewrite, but adding new a few > > > > > > new IOCTLs to set the preferred backing storage placements. > > > > > > > > > > > > All the previous GEM abstractions keep applying, so we did not see > > > > > > a justification to rewrite the kernel driver and userspace drivers. > > > > > > It would have just to made things look like TTM, when we already > > > > > > have the important parts of the code shared with TTM drivers > > > > > > behind the GEM interfaces which all our drivers sit on top of. > > > > > > > > > > a) you guys should be the community as well, if the buddy allocator is > > > > > useful in the core DRM get out there and try and see if anyone else > > > > > has a use case for it, like the GPU scheduler we have now (can i915 > > > > > use that yet? :-) > > > > > > > > Well, the buddy allocator should be useful for anybody wishing to have > > > > as continuous physical allocations as possible. I have naively assumed > > > > that would be almost everyone. So it would be only a question if others > > > > see the amount of work required to convert over is justified for them. > > > > > > > > For the common DRM scheduler, I think a solid move from the beginning > > > > would have been to factor out the i915 scheduler as it was most advanced > > > > in features :) Now there is a way more trivial common scheduler core > > > > with > > > > no easy path to transition
Re: [Intel-gfx] [RFC PATCH 00/42] Introduce memory region concept (including device local memory)
Am 26.02.19 um 18:20 schrieb Alex Deucher: On Tue, Feb 26, 2019 at 7:17 AM Joonas Lahtinen wrote: Quoting Alex Deucher (2019-02-25 21:31:43) On Mon, Feb 25, 2019 at 9:35 PM Joonas Lahtinen wrote: Quoting Dave Airlie (2019-02-25 12:24:48) On Tue, 19 Feb 2019 at 23:32, Joonas Lahtinen wrote: + dri-devel mailing list, especially for the buddy allocator part Quoting Dave Airlie (2019-02-15 02:47:07) On Fri, 15 Feb 2019 at 00:57, Matthew Auld wrote: In preparation for upcoming devices with device local memory, introduce the concept of different memory regions, and a simple buddy allocator to manage them. This is missing the information on why it's not TTM. Nothing against extending i915 gem off into doing stuff we already have examples off in tree, but before you do that it would be good to have a why we can't use TTM discussion in public. Glad that you asked. It's my fault that it was not included in the cover letter. I anticipated the question, but was travelling for a couple of days at the time this was sent. I didn't want to write a hasty explanation and then disappear, leaving others to take the heat. So here goes the less-hasty version: We did an analysis on the effort needed vs benefit gained of using TTM when this was started initially. The conclusion was that we already share the interesting bits of code through core DRM, really. Re-writing the memory handling to TTM would buy us more fine-grained locking. But it's more a trait of rewriting the memory handling with the information we have learned, than rewriting it to use TTM :) And further, we've been getting rid of struct_mutex at a steady phase in the past years, so we have a clear path to the fine-grained locking already in the not-so-distant future. With all this we did not see much gained from converting over, as the code sharing is already substantial. We also wanted to have the buddy allocator instead of a for loop making drm_mm allocations to make sure we can keep the memory fragmentation at bay. The intent is to move the buddy allocator to core DRM, to the benefit of all the drivers, if there is interest from community. It has been written as a strictly separate component with that in mind. And if you take the buddy allocator out of the patch set, the rest is mostly just vfuncing things up to be able to have different backing storages for objects. We took the opportunity to move over to the more valgrind friendly mmap while touching things, but it's something we have been contemplating anyway. And yeah, loads of selftests. That's really all that needed adding, and most of it is internal to i915 and not to do with uAPI. This means porting over an userspace driver doesn't require a substantial rewrite, but adding new a few new IOCTLs to set the preferred backing storage placements. All the previous GEM abstractions keep applying, so we did not see a justification to rewrite the kernel driver and userspace drivers. It would have just to made things look like TTM, when we already have the important parts of the code shared with TTM drivers behind the GEM interfaces which all our drivers sit on top of. a) you guys should be the community as well, if the buddy allocator is useful in the core DRM get out there and try and see if anyone else has a use case for it, like the GPU scheduler we have now (can i915 use that yet? :-) Well, the buddy allocator should be useful for anybody wishing to have as continuous physical allocations as possible. I have naively assumed that would be almost everyone. So it would be only a question if others see the amount of work required to convert over is justified for them. For the common DRM scheduler, I think a solid move from the beginning would have been to factor out the i915 scheduler as it was most advanced in features :) Now there is a way more trivial common scheduler core with no easy path to transition without a feature regression. Can you elaborate? What features are missing from the drm gpu scheduler? Priority based pre-emption is the big thing coming to mind. But maybe we should not derail the discussion in this thread. The discussion should be in the archives, or we can start a new thread. Probably not worth continuing here, but I think there are probably features that the drm scheduler has that the i915 scheduler does not. For example, engine load balancing. So i wouldn't necessarily say it's more advanced, just different feature sets. We could all be enjoying all of these features if we all worked on the same infrastructure. Actually it is worth continuing. To be honest the DRM scheduler came first and the i915 scheduler should have been pushed back a bit more. Additional to that it does support priority based pre-emption from the very beginning, you guys just didn't cared to take a look :( Regards, Christian. We'd have to rewrite many of the more advanced features for that codebase before we could transition over. It's hard to justify such work, for that
Re: [Intel-gfx] [RFC PATCH 00/42] Introduce memory region concept (including device local memory)
On Tue, Feb 26, 2019 at 7:17 AM Joonas Lahtinen wrote: > > Quoting Alex Deucher (2019-02-25 21:31:43) > > On Mon, Feb 25, 2019 at 9:35 PM Joonas Lahtinen > > wrote: > > > > > > Quoting Dave Airlie (2019-02-25 12:24:48) > > > > On Tue, 19 Feb 2019 at 23:32, Joonas Lahtinen > > > > wrote: > > > > > > > > > > + dri-devel mailing list, especially for the buddy allocator part > > > > > > > > > > Quoting Dave Airlie (2019-02-15 02:47:07) > > > > > > On Fri, 15 Feb 2019 at 00:57, Matthew Auld > > > > > > wrote: > > > > > > > > > > > > > > In preparation for upcoming devices with device local memory, > > > > > > > introduce the > > > > > > > concept of different memory regions, and a simple buddy allocator > > > > > > > to manage > > > > > > > them. > > > > > > > > > > > > This is missing the information on why it's not TTM. > > > > > > > > > > > > Nothing against extending i915 gem off into doing stuff we already > > > > > > have examples off in tree, but before you do that it would be good > > > > > > to > > > > > > have a why we can't use TTM discussion in public. > > > > > > > > > > Glad that you asked. It's my fault that it was not included in > > > > > the cover letter. I anticipated the question, but was travelling > > > > > for a couple of days at the time this was sent. I didn't want > > > > > to write a hasty explanation and then disappear, leaving others to > > > > > take the heat. > > > > > > > > > > So here goes the less-hasty version: > > > > > > > > > > We did an analysis on the effort needed vs benefit gained of using > > > > > TTM when this was started initially. The conclusion was that we > > > > > already share the interesting bits of code through core DRM, really. > > > > > > > > > > Re-writing the memory handling to TTM would buy us more fine-grained > > > > > locking. But it's more a trait of rewriting the memory handling with > > > > > the information we have learned, than rewriting it to use TTM :) > > > > > > > > > > And further, we've been getting rid of struct_mutex at a steady phase > > > > > in the past years, so we have a clear path to the fine-grained locking > > > > > already in the not-so-distant future. With all this we did not see > > > > > much gained from converting over, as the code sharing is already > > > > > substantial. > > > > > > > > > > We also wanted to have the buddy allocator instead of a for loop > > > > > making > > > > > drm_mm allocations to make sure we can keep the memory fragmentation > > > > > at bay. The intent is to move the buddy allocator to core DRM, to the > > > > > benefit of all the drivers, if there is interest from community. It > > > > > has > > > > > been written as a strictly separate component with that in mind. > > > > > > > > > > And if you take the buddy allocator out of the patch set, the rest is > > > > > mostly just vfuncing things up to be able to have different backing > > > > > storages for objects. We took the opportunity to move over to the more > > > > > valgrind friendly mmap while touching things, but it's something we > > > > > have been contemplating anyway. And yeah, loads of selftests. > > > > > > > > > > That's really all that needed adding, and most of it is internal to > > > > > i915 and not to do with uAPI. This means porting over an userspace > > > > > driver doesn't require a substantial rewrite, but adding new a few > > > > > new IOCTLs to set the preferred backing storage placements. > > > > > > > > > > All the previous GEM abstractions keep applying, so we did not see > > > > > a justification to rewrite the kernel driver and userspace drivers. > > > > > It would have just to made things look like TTM, when we already > > > > > have the important parts of the code shared with TTM drivers > > > > > behind the GEM interfaces which all our drivers sit on top of. > > > > > > > > a) you guys should be the community as well, if the buddy allocator is > > > > useful in the core DRM get out there and try and see if anyone else > > > > has a use case for it, like the GPU scheduler we have now (can i915 > > > > use that yet? :-) > > > > > > Well, the buddy allocator should be useful for anybody wishing to have > > > as continuous physical allocations as possible. I have naively assumed > > > that would be almost everyone. So it would be only a question if others > > > see the amount of work required to convert over is justified for them. > > > > > > For the common DRM scheduler, I think a solid move from the beginning > > > would have been to factor out the i915 scheduler as it was most advanced > > > in features :) Now there is a way more trivial common scheduler core with > > > no easy path to transition without a feature regression. > > > > Can you elaborate? What features are missing from the drm gpu scheduler? > > Priority based pre-emption is the big thing coming to mind. But maybe we > should not derail the discussion in this thread. The discussion should > be in the archives, or we can start a new thread.
Re: [Intel-gfx] [RFC PATCH 00/42] Introduce memory region concept (including device local memory)
Quoting Alex Deucher (2019-02-25 21:31:43) > On Mon, Feb 25, 2019 at 9:35 PM Joonas Lahtinen > wrote: > > > > Quoting Dave Airlie (2019-02-25 12:24:48) > > > On Tue, 19 Feb 2019 at 23:32, Joonas Lahtinen > > > wrote: > > > > > > > > + dri-devel mailing list, especially for the buddy allocator part > > > > > > > > Quoting Dave Airlie (2019-02-15 02:47:07) > > > > > On Fri, 15 Feb 2019 at 00:57, Matthew Auld > > > > > wrote: > > > > > > > > > > > > In preparation for upcoming devices with device local memory, > > > > > > introduce the > > > > > > concept of different memory regions, and a simple buddy allocator > > > > > > to manage > > > > > > them. > > > > > > > > > > This is missing the information on why it's not TTM. > > > > > > > > > > Nothing against extending i915 gem off into doing stuff we already > > > > > have examples off in tree, but before you do that it would be good to > > > > > have a why we can't use TTM discussion in public. > > > > > > > > Glad that you asked. It's my fault that it was not included in > > > > the cover letter. I anticipated the question, but was travelling > > > > for a couple of days at the time this was sent. I didn't want > > > > to write a hasty explanation and then disappear, leaving others to > > > > take the heat. > > > > > > > > So here goes the less-hasty version: > > > > > > > > We did an analysis on the effort needed vs benefit gained of using > > > > TTM when this was started initially. The conclusion was that we > > > > already share the interesting bits of code through core DRM, really. > > > > > > > > Re-writing the memory handling to TTM would buy us more fine-grained > > > > locking. But it's more a trait of rewriting the memory handling with > > > > the information we have learned, than rewriting it to use TTM :) > > > > > > > > And further, we've been getting rid of struct_mutex at a steady phase > > > > in the past years, so we have a clear path to the fine-grained locking > > > > already in the not-so-distant future. With all this we did not see > > > > much gained from converting over, as the code sharing is already > > > > substantial. > > > > > > > > We also wanted to have the buddy allocator instead of a for loop making > > > > drm_mm allocations to make sure we can keep the memory fragmentation > > > > at bay. The intent is to move the buddy allocator to core DRM, to the > > > > benefit of all the drivers, if there is interest from community. It has > > > > been written as a strictly separate component with that in mind. > > > > > > > > And if you take the buddy allocator out of the patch set, the rest is > > > > mostly just vfuncing things up to be able to have different backing > > > > storages for objects. We took the opportunity to move over to the more > > > > valgrind friendly mmap while touching things, but it's something we > > > > have been contemplating anyway. And yeah, loads of selftests. > > > > > > > > That's really all that needed adding, and most of it is internal to > > > > i915 and not to do with uAPI. This means porting over an userspace > > > > driver doesn't require a substantial rewrite, but adding new a few > > > > new IOCTLs to set the preferred backing storage placements. > > > > > > > > All the previous GEM abstractions keep applying, so we did not see > > > > a justification to rewrite the kernel driver and userspace drivers. > > > > It would have just to made things look like TTM, when we already > > > > have the important parts of the code shared with TTM drivers > > > > behind the GEM interfaces which all our drivers sit on top of. > > > > > > a) you guys should be the community as well, if the buddy allocator is > > > useful in the core DRM get out there and try and see if anyone else > > > has a use case for it, like the GPU scheduler we have now (can i915 > > > use that yet? :-) > > > > Well, the buddy allocator should be useful for anybody wishing to have > > as continuous physical allocations as possible. I have naively assumed > > that would be almost everyone. So it would be only a question if others > > see the amount of work required to convert over is justified for them. > > > > For the common DRM scheduler, I think a solid move from the beginning > > would have been to factor out the i915 scheduler as it was most advanced > > in features :) Now there is a way more trivial common scheduler core with > > no easy path to transition without a feature regression. > > Can you elaborate? What features are missing from the drm gpu scheduler? Priority based pre-emption is the big thing coming to mind. But maybe we should not derail the discussion in this thread. The discussion should be in the archives, or we can start a new thread. > > We'd have to rewrite many of the more advanced features for that codebase > > before we could transition over. It's hard to justify such work, for > > that it would buy us very little compared to amount of work. > > > > Situation would be different if there was something
Re: [Intel-gfx] [RFC PATCH 00/42] Introduce memory region concept (including device local memory)
So I'm not going to go into technical detail here, which I'll happily leave to Joonas et al, but I think a couple of general points deserve to be addressed. On Tue, 26 Feb 2019, Alex Deucher wrote: > On Mon, Feb 25, 2019 at 9:35 PM Joonas Lahtinen > wrote: >> Adding the suggested smaller amount of code vs. doing a much bigger >> rewrite is something of a straightforward choice with the amount of >> platforms and features in flight, especially when the end result is >> the same. > > Because you will probably never do it. It's almost always easier to > just incrementally add on to existing code. One could argue that GEM > evolved into more or less the exact same thing as TTM anyway so why > not bite the bullet and switch at some point? TTM works fine even for > UMA hardware. Surely we have lots of faults, but being averse to refactoring, reworking, and continuously rewriting parts of the driver is not among them by any stretch. Sometimes it's just for the general maintainability, sometimes to remodel stuff to neatly plug in support for that new piece of hardware, and everything in between. > There is a common misconception in big companies that if you utilize > shared infrastructure it will slow you down or you'll lose control of > your code which is I think what you are referring to. Ultimately, it > does sometimes raise the bar, but in the long term it benefits > everyone and usually it doesn't really add that much overhead. It > sounds like you are just feeding that misconception; you can't > contribute to or take advantage of any shared infrastructure because > that might slow you down. If that is the case, then why does upstream > first even matter? It seems like the only common code you want to > support is common code that you wrote in the first place. Again, on a general note, without actually checking the stats, I like to think we're pretty good citizens wrt actively using and contributing to shared infrastructure, and shared uapi. Especially so for KMS, and even when it really has slowed us down. So while you may have fair points about a specific case, and again I'll let Joonas address the specific case, I'll have to ask you to please not generalize that to the whole driver. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [RFC PATCH 00/42] Introduce memory region concept (including device local memory)
On Mon, Feb 25, 2019 at 9:35 PM Joonas Lahtinen wrote: > > Quoting Dave Airlie (2019-02-25 12:24:48) > > On Tue, 19 Feb 2019 at 23:32, Joonas Lahtinen > > wrote: > > > > > > + dri-devel mailing list, especially for the buddy allocator part > > > > > > Quoting Dave Airlie (2019-02-15 02:47:07) > > > > On Fri, 15 Feb 2019 at 00:57, Matthew Auld > > > > wrote: > > > > > > > > > > In preparation for upcoming devices with device local memory, > > > > > introduce the > > > > > concept of different memory regions, and a simple buddy allocator to > > > > > manage > > > > > them. > > > > > > > > This is missing the information on why it's not TTM. > > > > > > > > Nothing against extending i915 gem off into doing stuff we already > > > > have examples off in tree, but before you do that it would be good to > > > > have a why we can't use TTM discussion in public. > > > > > > Glad that you asked. It's my fault that it was not included in > > > the cover letter. I anticipated the question, but was travelling > > > for a couple of days at the time this was sent. I didn't want > > > to write a hasty explanation and then disappear, leaving others to > > > take the heat. > > > > > > So here goes the less-hasty version: > > > > > > We did an analysis on the effort needed vs benefit gained of using > > > TTM when this was started initially. The conclusion was that we > > > already share the interesting bits of code through core DRM, really. > > > > > > Re-writing the memory handling to TTM would buy us more fine-grained > > > locking. But it's more a trait of rewriting the memory handling with > > > the information we have learned, than rewriting it to use TTM :) > > > > > > And further, we've been getting rid of struct_mutex at a steady phase > > > in the past years, so we have a clear path to the fine-grained locking > > > already in the not-so-distant future. With all this we did not see > > > much gained from converting over, as the code sharing is already > > > substantial. > > > > > > We also wanted to have the buddy allocator instead of a for loop making > > > drm_mm allocations to make sure we can keep the memory fragmentation > > > at bay. The intent is to move the buddy allocator to core DRM, to the > > > benefit of all the drivers, if there is interest from community. It has > > > been written as a strictly separate component with that in mind. > > > > > > And if you take the buddy allocator out of the patch set, the rest is > > > mostly just vfuncing things up to be able to have different backing > > > storages for objects. We took the opportunity to move over to the more > > > valgrind friendly mmap while touching things, but it's something we > > > have been contemplating anyway. And yeah, loads of selftests. > > > > > > That's really all that needed adding, and most of it is internal to > > > i915 and not to do with uAPI. This means porting over an userspace > > > driver doesn't require a substantial rewrite, but adding new a few > > > new IOCTLs to set the preferred backing storage placements. > > > > > > All the previous GEM abstractions keep applying, so we did not see > > > a justification to rewrite the kernel driver and userspace drivers. > > > It would have just to made things look like TTM, when we already > > > have the important parts of the code shared with TTM drivers > > > behind the GEM interfaces which all our drivers sit on top of. > > > > a) you guys should be the community as well, if the buddy allocator is > > useful in the core DRM get out there and try and see if anyone else > > has a use case for it, like the GPU scheduler we have now (can i915 > > use that yet? :-) > > Well, the buddy allocator should be useful for anybody wishing to have > as continuous physical allocations as possible. I have naively assumed > that would be almost everyone. So it would be only a question if others > see the amount of work required to convert over is justified for them. > > For the common DRM scheduler, I think a solid move from the beginning > would have been to factor out the i915 scheduler as it was most advanced > in features :) Now there is a way more trivial common scheduler core with > no easy path to transition without a feature regression. Can you elaborate? What features are missing from the drm gpu scheduler? > > We'd have to rewrite many of the more advanced features for that codebase > before we could transition over. It's hard to justify such work, for > that it would buy us very little compared to amount of work. > > Situation would be different if there was something gained from > switching over. This would be the situation if the more advanced > scheduler was picked as the shared codebase. > > > b) however this last two paragraphs fill me with no confidence that > > you've looked at TTM at all. It sounds like you took comments about > > TTM made 10 years ago, and didn't update them. There should be no > > major reason for a uapi change just because you adopt TTM. TTM hasn't >
Re: [Intel-gfx] [RFC PATCH 00/42] Introduce memory region concept (including device local memory)
Quoting Dave Airlie (2019-02-25 12:24:48) > On Tue, 19 Feb 2019 at 23:32, Joonas Lahtinen > wrote: > > > > + dri-devel mailing list, especially for the buddy allocator part > > > > Quoting Dave Airlie (2019-02-15 02:47:07) > > > On Fri, 15 Feb 2019 at 00:57, Matthew Auld wrote: > > > > > > > > In preparation for upcoming devices with device local memory, introduce > > > > the > > > > concept of different memory regions, and a simple buddy allocator to > > > > manage > > > > them. > > > > > > This is missing the information on why it's not TTM. > > > > > > Nothing against extending i915 gem off into doing stuff we already > > > have examples off in tree, but before you do that it would be good to > > > have a why we can't use TTM discussion in public. > > > > Glad that you asked. It's my fault that it was not included in > > the cover letter. I anticipated the question, but was travelling > > for a couple of days at the time this was sent. I didn't want > > to write a hasty explanation and then disappear, leaving others to > > take the heat. > > > > So here goes the less-hasty version: > > > > We did an analysis on the effort needed vs benefit gained of using > > TTM when this was started initially. The conclusion was that we > > already share the interesting bits of code through core DRM, really. > > > > Re-writing the memory handling to TTM would buy us more fine-grained > > locking. But it's more a trait of rewriting the memory handling with > > the information we have learned, than rewriting it to use TTM :) > > > > And further, we've been getting rid of struct_mutex at a steady phase > > in the past years, so we have a clear path to the fine-grained locking > > already in the not-so-distant future. With all this we did not see > > much gained from converting over, as the code sharing is already > > substantial. > > > > We also wanted to have the buddy allocator instead of a for loop making > > drm_mm allocations to make sure we can keep the memory fragmentation > > at bay. The intent is to move the buddy allocator to core DRM, to the > > benefit of all the drivers, if there is interest from community. It has > > been written as a strictly separate component with that in mind. > > > > And if you take the buddy allocator out of the patch set, the rest is > > mostly just vfuncing things up to be able to have different backing > > storages for objects. We took the opportunity to move over to the more > > valgrind friendly mmap while touching things, but it's something we > > have been contemplating anyway. And yeah, loads of selftests. > > > > That's really all that needed adding, and most of it is internal to > > i915 and not to do with uAPI. This means porting over an userspace > > driver doesn't require a substantial rewrite, but adding new a few > > new IOCTLs to set the preferred backing storage placements. > > > > All the previous GEM abstractions keep applying, so we did not see > > a justification to rewrite the kernel driver and userspace drivers. > > It would have just to made things look like TTM, when we already > > have the important parts of the code shared with TTM drivers > > behind the GEM interfaces which all our drivers sit on top of. > > a) you guys should be the community as well, if the buddy allocator is > useful in the core DRM get out there and try and see if anyone else > has a use case for it, like the GPU scheduler we have now (can i915 > use that yet? :-) Well, the buddy allocator should be useful for anybody wishing to have as continuous physical allocations as possible. I have naively assumed that would be almost everyone. So it would be only a question if others see the amount of work required to convert over is justified for them. For the common DRM scheduler, I think a solid move from the beginning would have been to factor out the i915 scheduler as it was most advanced in features :) Now there is a way more trivial common scheduler core with no easy path to transition without a feature regression. We'd have to rewrite many of the more advanced features for that codebase before we could transition over. It's hard to justify such work, for that it would buy us very little compared to amount of work. Situation would be different if there was something gained from switching over. This would be the situation if the more advanced scheduler was picked as the shared codebase. > b) however this last two paragraphs fill me with no confidence that > you've looked at TTM at all. It sounds like you took comments about > TTM made 10 years ago, and didn't update them. There should be no > major reason for a uapi change just because you adopt TTM. TTM hasn't > ever had a common uapi across drivers upstream, one was proposed > initially > 10 years ago. This is one part my confusion on what the question was for and other part bad wording on my behalf. So an attempt to re-answer: When this effort was started it was obvious that the amount of new code required was low (as you
Re: [Intel-gfx] [RFC PATCH 00/42] Introduce memory region concept (including device local memory)
On Tue, 19 Feb 2019 at 23:32, Joonas Lahtinen wrote: > > + dri-devel mailing list, especially for the buddy allocator part > > Quoting Dave Airlie (2019-02-15 02:47:07) > > On Fri, 15 Feb 2019 at 00:57, Matthew Auld wrote: > > > > > > In preparation for upcoming devices with device local memory, introduce > > > the > > > concept of different memory regions, and a simple buddy allocator to > > > manage > > > them. > > > > This is missing the information on why it's not TTM. > > > > Nothing against extending i915 gem off into doing stuff we already > > have examples off in tree, but before you do that it would be good to > > have a why we can't use TTM discussion in public. > > Glad that you asked. It's my fault that it was not included in > the cover letter. I anticipated the question, but was travelling > for a couple of days at the time this was sent. I didn't want > to write a hasty explanation and then disappear, leaving others to > take the heat. > > So here goes the less-hasty version: > > We did an analysis on the effort needed vs benefit gained of using > TTM when this was started initially. The conclusion was that we > already share the interesting bits of code through core DRM, really. > > Re-writing the memory handling to TTM would buy us more fine-grained > locking. But it's more a trait of rewriting the memory handling with > the information we have learned, than rewriting it to use TTM :) > > And further, we've been getting rid of struct_mutex at a steady phase > in the past years, so we have a clear path to the fine-grained locking > already in the not-so-distant future. With all this we did not see > much gained from converting over, as the code sharing is already > substantial. > > We also wanted to have the buddy allocator instead of a for loop making > drm_mm allocations to make sure we can keep the memory fragmentation > at bay. The intent is to move the buddy allocator to core DRM, to the > benefit of all the drivers, if there is interest from community. It has > been written as a strictly separate component with that in mind. > > And if you take the buddy allocator out of the patch set, the rest is > mostly just vfuncing things up to be able to have different backing > storages for objects. We took the opportunity to move over to the more > valgrind friendly mmap while touching things, but it's something we > have been contemplating anyway. And yeah, loads of selftests. > > That's really all that needed adding, and most of it is internal to > i915 and not to do with uAPI. This means porting over an userspace > driver doesn't require a substantial rewrite, but adding new a few > new IOCTLs to set the preferred backing storage placements. > > All the previous GEM abstractions keep applying, so we did not see > a justification to rewrite the kernel driver and userspace drivers. > It would have just to made things look like TTM, when we already > have the important parts of the code shared with TTM drivers > behind the GEM interfaces which all our drivers sit on top of. a) you guys should be the community as well, if the buddy allocator is useful in the core DRM get out there and try and see if anyone else has a use case for it, like the GPU scheduler we have now (can i915 use that yet? :-) b) however this last two paragraphs fill me with no confidence that you've looked at TTM at all. It sounds like you took comments about TTM made 10 years ago, and didn't update them. There should be no major reason for a uapi change just because you adopt TTM. TTM hasn't ever had a common uapi across drivers upstream, one was proposed initially > 10 years ago. All the current TTM using drivers except vmware use a GEM based API as well. TTM is an internal driver helper for managing pools of RAM. I'm just not sure what rebuilding a chunk of shared code inside the i915 driver is buying you, except a transition path into divergence from all the other discrete RAM drivers. Like the gallium aversion in userspace, having a TTM aversion in kernel space is going to be the wrong path, and I'd rather not have to clean it up in 5 years when you guys eventually realise it. The i915 GEM code get rewritten and refactored quite often and has a bus factor of ickle, if he decided to go elsewhere, you will have a pile of code that nobody gets, I think having a TTM backend would have a better long term effect on your driver maintainability. Dave. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [RFC PATCH 00/42] Introduce memory region concept (including device local memory)
+ dri-devel mailing list, especially for the buddy allocator part Quoting Dave Airlie (2019-02-15 02:47:07) > On Fri, 15 Feb 2019 at 00:57, Matthew Auld wrote: > > > > In preparation for upcoming devices with device local memory, introduce the > > concept of different memory regions, and a simple buddy allocator to manage > > them. > > This is missing the information on why it's not TTM. > > Nothing against extending i915 gem off into doing stuff we already > have examples off in tree, but before you do that it would be good to > have a why we can't use TTM discussion in public. Glad that you asked. It's my fault that it was not included in the cover letter. I anticipated the question, but was travelling for a couple of days at the time this was sent. I didn't want to write a hasty explanation and then disappear, leaving others to take the heat. So here goes the less-hasty version: We did an analysis on the effort needed vs benefit gained of using TTM when this was started initially. The conclusion was that we already share the interesting bits of code through core DRM, really. Re-writing the memory handling to TTM would buy us more fine-grained locking. But it's more a trait of rewriting the memory handling with the information we have learned, than rewriting it to use TTM :) And further, we've been getting rid of struct_mutex at a steady phase in the past years, so we have a clear path to the fine-grained locking already in the not-so-distant future. With all this we did not see much gained from converting over, as the code sharing is already substantial. We also wanted to have the buddy allocator instead of a for loop making drm_mm allocations to make sure we can keep the memory fragmentation at bay. The intent is to move the buddy allocator to core DRM, to the benefit of all the drivers, if there is interest from community. It has been written as a strictly separate component with that in mind. And if you take the buddy allocator out of the patch set, the rest is mostly just vfuncing things up to be able to have different backing storages for objects. We took the opportunity to move over to the more valgrind friendly mmap while touching things, but it's something we have been contemplating anyway. And yeah, loads of selftests. That's really all that needed adding, and most of it is internal to i915 and not to do with uAPI. This means porting over an userspace driver doesn't require a substantial rewrite, but adding new a few new IOCTLs to set the preferred backing storage placements. All the previous GEM abstractions keep applying, so we did not see a justification to rewrite the kernel driver and userspace drivers. It would have just to made things look like TTM, when we already have the important parts of the code shared with TTM drivers behind the GEM interfaces which all our drivers sit on top of. I hope this answered the question to some extent, and feel free to ask more details or provide suggestion to what we could have done differently. Regards, Joonas > > Dave. > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
