Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On Tue, Nov 07, 2023 at 10:13:37AM +0100, Christian König wrote: > Am 06.11.23 um 17:46 schrieb Danilo Krummrich: > > On Fri, Nov 03, 2023 at 09:15:25AM +0100, Christian König wrote: > > > Am 02.11.23 um 19:03 schrieb Danilo Krummrich: > > > > On Thu, Nov 02, 2023 at 11:07:32AM +0100, Christian König wrote: > > > > > Hi Danilo, > > > > > > > Am 31.10.23 um 16:01 schrieb Danilo Krummrich: > > > > > > On Tue, Oct 31, 2023 at 02:20:50PM +0100, Christian König wrote: > > > > > > > [SNIP] > > > > > > > Yeah, I see the intention here and can perfectly relate to > > > it it's just that > > > > > > > I have prioritize other things. > > > > > > I don't see any work being required from your side for this. > > > > > What I wanted to say is that I understand your intentions and > > > can relate to > > > > > that, but other aspects have higher priority in this discussion. > > > > What aspects would that be? > > > > > > Be defensive. As far as I can see this callback is only nice to have > > > and not > > > functionally mandatory. > > > > I gave you quite some reasons why this is desired. > > Yeah, but I need something to make it necessary. Desired is just not enough > in this case. You can't really say that *and*... > > > And generally speaking, even if something is not functionally mandatory, > > rejecting it *only* because of that isn't a good idea. > > Completely agree. ...agree here. That is entirely contradicting. > But what makes this at least questionable is the > combination of not mandatory and giving drivers the opportunity to mess with > submissions again. We already had this below. I explained why it doesn't and you agreed. > > Especially in the scheduler and dma_fence handling we had tons of patches > which added a nice to have and seemingly harmless features which later > turned into a complete nightmare to maintain. That's pseudo argument, you can simply use this to randomly reject everything. The fact is, that we need to get a single integer check right, I fail to see how this will turn into a nightmare to maintain. > > The takeaway is that we need more big red warning signs in the form of > documentation and try to not change things just because it makes them look > better. Documentation would be great indeed. You might be happy to hear that this patch actually comes with documentation on how the job-flow control of the scheduler works and what users can expect. Once merged, this will be the best documented part of the scheduler... > > > It is better to deal with it in terms of content and consider its pros > > and cons. > > > > > > > > And in such cases I have to weight between complexity by adding > > > something > > > which might go boom and being conservative and using well known and > > > working > > > code paths. > > > > > > > Not breaking other drivers? - The callback is optional, and > > > besides that, as > > > > already mentioned, the callback doesn't do anything that can't > > > already go wrong > > > > with the inital credit value from drm_sched_job_init(). > > > > > > During drm_sched_job_init() time the fence of this job isn't > > > published yet. > > > So when the driver specified something invalid we can easily return > > > an error > > > code and abort. > > > > > > Inside the scheduler we can't do anything like this. E.g. what > > > happens if > > > the driver suddenly returns a value which is to large? We can't > > > reject that. > > > > This is all correct and I recognize that. But again, the callback is > > optional. > > I don't see how drivers would be affected not opting in for this feature. > > I see this actually as cons argument for the change. If many drivers would > use this then the code path would be much better tested. It's a single integer check. Again, I'm pretty sure we can get this right. We can even add a few test cases to validate this code path and the credit handling in general - I'd offer that. > > > > > And for drivers which do, they'd have the same problem (if you actually > > want to > > call it one) doing some driver specific workaround as well. And because > > they'd > > not have the correct hook likely many more. > > > > > > > > > Keeping things simple? - The workaround PowerVR is considering instead > > > > (returning a dma-fence in ->prepare_job()) doesn't seem to > > > contribute to this > > > > goal. > > > > > > I don't see this as a workaround, this is essentially the desired > > > solution. > > > All dependencies of a job should be represented as a dma-fence if > > > possible. > > > > So, you're saying that given we have an inaccurate way of tracking > > credits > > (because you're rejecting the callback making it accurate), the desired > > solution > > would be to use an existing callback with a whole different, more > > complicated > > and hence more error prone approach to overcome that? > > > > > > > > The background is that dma-fences have a very well defined semantics > > > which > > > through a rather
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Am 06.11.23 um 17:46 schrieb Danilo Krummrich: On Fri, Nov 03, 2023 at 09:15:25AM +0100, Christian König wrote: Am 02.11.23 um 19:03 schrieb Danilo Krummrich: > On Thu, Nov 02, 2023 at 11:07:32AM +0100, Christian König wrote: > > Hi Danilo, > > > > Am 31.10.23 um 16:01 schrieb Danilo Krummrich: > > > On Tue, Oct 31, 2023 at 02:20:50PM +0100, Christian König wrote: > > > > [SNIP] > > > > Yeah, I see the intention here and can perfectly relate to it it's just that > > > > I have prioritize other things. > > > I don't see any work being required from your side for this. > > What I wanted to say is that I understand your intentions and can relate to > > that, but other aspects have higher priority in this discussion. > What aspects would that be? Be defensive. As far as I can see this callback is only nice to have and not functionally mandatory. I gave you quite some reasons why this is desired. Yeah, but I need something to make it necessary. Desired is just not enough in this case. And generally speaking, even if something is not functionally mandatory, rejecting it *only* because of that isn't a good idea. Completely agree. But what makes this at least questionable is the combination of not mandatory and giving drivers the opportunity to mess with submissions again. Especially in the scheduler and dma_fence handling we had tons of patches which added a nice to have and seemingly harmless features which later turned into a complete nightmare to maintain. The takeaway is that we need more big red warning signs in the form of documentation and try to not change things just because it makes them look better. It is better to deal with it in terms of content and consider its pros and cons. And in such cases I have to weight between complexity by adding something which might go boom and being conservative and using well known and working code paths. > Not breaking other drivers? - The callback is optional, and besides that, as > already mentioned, the callback doesn't do anything that can't already go wrong > with the inital credit value from drm_sched_job_init(). During drm_sched_job_init() time the fence of this job isn't published yet. So when the driver specified something invalid we can easily return an error code and abort. Inside the scheduler we can't do anything like this. E.g. what happens if the driver suddenly returns a value which is to large? We can't reject that. This is all correct and I recognize that. But again, the callback is optional. I don't see how drivers would be affected not opting in for this feature. I see this actually as cons argument for the change. If many drivers would use this then the code path would be much better tested. And for drivers which do, they'd have the same problem (if you actually want to call it one) doing some driver specific workaround as well. And because they'd not have the correct hook likely many more. > Keeping things simple? - The workaround PowerVR is considering instead > (returning a dma-fence in ->prepare_job()) doesn't seem to contribute to this > goal. I don't see this as a workaround, this is essentially the desired solution. All dependencies of a job should be represented as a dma-fence if possible. So, you're saying that given we have an inaccurate way of tracking credits (because you're rejecting the callback making it accurate), the desired solution would be to use an existing callback with a whole different, more complicated and hence more error prone approach to overcome that? The background is that dma-fences have a very well defined semantics which through a rather elaborated hack is validated to the extend that lockdep can check if drivers behave correctly or not: https://elixir.bootlin.com/linux/latest/source/drivers/dma-buf/dma-fence.c#L194 I fail to see what dma-fences have to do with job-flow control of the ring buffer. You mention it to be a plus that there is a hack to be able to validate dma-fences with lockdep, but you miss the fact that the only reason we'd need this in the first place is that you think it is desired to do job-flow control with dma-fences. Well I consider the hack better than this callback. The hack Daniel came up with allows us to validate driver behavior. In other words even if things seems to work we get a warning if drivers do something they are not supposed to do. With this here you are actually breaking this because now drivers have influence again on when stuff is scheduled. I'm not breaking anything. This flow control mechanism is already in the scheduler, it's just that it's inaccurate because drivers need to calculate every job with the worst case credit amount. I'm just making it accurate, such that it becomes useful for some drivers. Well, what's wrong with assuming the worst? I also do not agree that this breaks anything dma-fence related in general. It doesn't give drivers control of *when*
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On Fri, Nov 03, 2023 at 09:15:25AM +0100, Christian König wrote: Am 02.11.23 um 19:03 schrieb Danilo Krummrich: > On Thu, Nov 02, 2023 at 11:07:32AM +0100, Christian König wrote: > > Hi Danilo, > > > > Am 31.10.23 um 16:01 schrieb Danilo Krummrich: > > > On Tue, Oct 31, 2023 at 02:20:50PM +0100, Christian König wrote: > > > > [SNIP] > > > > Yeah, I see the intention here and can perfectly relate to it it's just that > > > > I have prioritize other things. > > > I don't see any work being required from your side for this. > > What I wanted to say is that I understand your intentions and can relate to > > that, but other aspects have higher priority in this discussion. > What aspects would that be? Be defensive. As far as I can see this callback is only nice to have and not functionally mandatory. I gave you quite some reasons why this is desired. And generally speaking, even if something is not functionally mandatory, rejecting it *only* because of that isn't a good idea. It is better to deal with it in terms of content and consider its pros and cons. And in such cases I have to weight between complexity by adding something which might go boom and being conservative and using well known and working code paths. > Not breaking other drivers? - The callback is optional, and besides that, as > already mentioned, the callback doesn't do anything that can't already go wrong > with the inital credit value from drm_sched_job_init(). During drm_sched_job_init() time the fence of this job isn't published yet. So when the driver specified something invalid we can easily return an error code and abort. Inside the scheduler we can't do anything like this. E.g. what happens if the driver suddenly returns a value which is to large? We can't reject that. This is all correct and I recognize that. But again, the callback is optional. I don't see how drivers would be affected not opting in for this feature. And for drivers which do, they'd have the same problem (if you actually want to call it one) doing some driver specific workaround as well. And because they'd not have the correct hook likely many more. > Keeping things simple? - The workaround PowerVR is considering instead > (returning a dma-fence in ->prepare_job()) doesn't seem to contribute to this > goal. I don't see this as a workaround, this is essentially the desired solution. All dependencies of a job should be represented as a dma-fence if possible. So, you're saying that given we have an inaccurate way of tracking credits (because you're rejecting the callback making it accurate), the desired solution would be to use an existing callback with a whole different, more complicated and hence more error prone approach to overcome that? The background is that dma-fences have a very well defined semantics which through a rather elaborated hack is validated to the extend that lockdep can check if drivers behave correctly or not: https://elixir.bootlin.com/linux/latest/source/drivers/dma-buf/dma-fence.c#L194 I fail to see what dma-fences have to do with job-flow control of the ring buffer. You mention it to be a plus that there is a hack to be able to validate dma-fences with lockdep, but you miss the fact that the only reason we'd need this in the first place is that you think it is desired to do job-flow control with dma-fences. With this here you are actually breaking this because now drivers have influence again on when stuff is scheduled. I'm not breaking anything. This flow control mechanism is already in the scheduler, it's just that it's inaccurate because drivers need to calculate every job with the worst case credit amount. I'm just making it accurate, such that it becomes useful for some drivers. I also do not agree that this breaks anything dma-fence related in general. It doesn't give drivers control of *when* stuff is scheduled. It gives the scheduler an idea of *if* it can schedule a job without overflowing the ring buffer. The influence of when comes through the hardware finishing a previous job and the corresponding dma-fence callback creating the corresponding space in terms of credit capacity of the scheduler. > > > > Adding this callback allows for the driver to influence the job submission > > > > and while this might seems useful now I'm just to much of a burned child to > > > > do stuff like this without having a really good reason for it. > > > It does influence the job submission in the exact same way as the initial credit > > > count set through drm_sched_job_init() does. There is absolutely nothing with > > > this callback a driver couldn't mess up in the exact same way with the initial > > > credit count set through drm_sched_job_init(). Following this logic we'd need to > > > abandon the whole patch. > > Well what I thought you would be doing is to replace the fixed one credit > > per job with N credits per job. > That's exactly what the patch does. Plus, with the help of the >
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On Tue, Oct 31, 2023 at 02:20:50PM +0100, Christian König wrote: > Hi Danilo, > > sorry for splitting up the mail thread. I had to fetch this mail from my > spam folder for some reason. > > Am 30.10.23 um 18:56 schrieb Danilo Krummrich: > > Hi Christian, > > > > [SNIP] > > > > And yes, we can live with the overhead of making jobs > > > > slightly bigger than they actually are, thus potentially delaying their > > > > execution. It's just that I don't quite understand the rational behind > > > > this conservatism, as I don't really see what negative impact this extra > > > > ->update_job_credits() call in the credit checking path has, other than > > > > the slight overhead of an if-check for drivers that don't need it. > > > From experience it showed that we should not make the scheduler more > > > complicated than necessary. And I still think that the ring buffers only > > > need to be filled enough to keep the hardware busy. > > Right, and this callback contributes to exactly that. > > > > I don't really think there is much to worry about in terms of introducing > > more > > complexity. The implementation behind this callback is fairly trivial - it's > > simply called right before we check whether a job fits on the ring, to fetch > > the job's actual size. > > > > I would agree if the implementation of that would be bulky, tricky and > > asking > > for a compromise. But, since it actually is simple and straight forward I > > really > > think that if we implement some kind of dynamic job-flow control it should > > be > > implemented as acurate as possible rather than doing it half-baked. > > Yeah, I see the intention here and can perfectly relate to it it's just that > I have prioritize other things. I don't see any work being required from your side for this. > > Adding this callback allows for the driver to influence the job submission > and while this might seems useful now I'm just to much of a burned child to > do stuff like this without having a really good reason for it. It does influence the job submission in the exact same way as the initial credit count set through drm_sched_job_init() does. There is absolutely nothing with this callback a driver couldn't mess up in the exact same way with the initial credit count set through drm_sched_job_init(). Following this logic we'd need to abandon the whole patch. Hence, I don't really understand why you're so focused on this callback. Especially, since it's an optional one. > > > > If this here has some measurable positive effect then yeah we should > > > probably do it, but as long as it's only nice to have I have some > > > objections > > > to that. > > Can't answer this, since Nouveau doesn't support native fence waits. > > However, I > > guess it depends on how many native fences a job carries. So, if we'd have > > two > > jobs with each of them carrying a lot of native fences, but not a lot of > > actual > > work, I can very well imagine that over-accounting can have a measureable > > impact. > > What I can imagine as well is things like the hardware or firmware is > looking at the fullness of the ring buffer to predict how much pending work > there is. > > > I just wonder if we really want to ask for real measurements given that the > > optimization is rather trivial and cheap and we already have enough evidence > > that it makes sense conceptually. > > Well that's the point I disagree on, this callback isn't trivial. If (for > example) the driver returns a value larger than initially estimated for the > job we can run into an endless loop. I agree it doesn't make sense to increase, but it wouldn't break anything, unless the job size starts exceeding the capacity of the ring. And this case is handled anyway. > > It's just one more thing which can go boom. At bare minimum we should check > that the value is always decreasing. Considering the above I still agree, such a check makes sense - gonna add one. - Danilo > > Christian. > > > > > - Danilo > > > > > Regards, > > > Christian. > > > > > > > Regards, > > > > > > > > Boris
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Hi Danilo, sorry for splitting up the mail thread. I had to fetch this mail from my spam folder for some reason. Am 30.10.23 um 18:56 schrieb Danilo Krummrich: Hi Christian, [SNIP] And yes, we can live with the overhead of making jobs slightly bigger than they actually are, thus potentially delaying their execution. It's just that I don't quite understand the rational behind this conservatism, as I don't really see what negative impact this extra ->update_job_credits() call in the credit checking path has, other than the slight overhead of an if-check for drivers that don't need it. From experience it showed that we should not make the scheduler more complicated than necessary. And I still think that the ring buffers only need to be filled enough to keep the hardware busy. Right, and this callback contributes to exactly that. I don't really think there is much to worry about in terms of introducing more complexity. The implementation behind this callback is fairly trivial - it's simply called right before we check whether a job fits on the ring, to fetch the job's actual size. I would agree if the implementation of that would be bulky, tricky and asking for a compromise. But, since it actually is simple and straight forward I really think that if we implement some kind of dynamic job-flow control it should be implemented as acurate as possible rather than doing it half-baked. Yeah, I see the intention here and can perfectly relate to it it's just that I have prioritize other things. Adding this callback allows for the driver to influence the job submission and while this might seems useful now I'm just to much of a burned child to do stuff like this without having a really good reason for it. If this here has some measurable positive effect then yeah we should probably do it, but as long as it's only nice to have I have some objections to that. Can't answer this, since Nouveau doesn't support native fence waits. However, I guess it depends on how many native fences a job carries. So, if we'd have two jobs with each of them carrying a lot of native fences, but not a lot of actual work, I can very well imagine that over-accounting can have a measureable impact. What I can imagine as well is things like the hardware or firmware is looking at the fullness of the ring buffer to predict how much pending work there is. I just wonder if we really want to ask for real measurements given that the optimization is rather trivial and cheap and we already have enough evidence that it makes sense conceptually. Well that's the point I disagree on, this callback isn't trivial. If (for example) the driver returns a value larger than initially estimated for the job we can run into an endless loop. It's just one more thing which can go boom. At bare minimum we should check that the value is always decreasing. Christian. - Danilo Regards, Christian. Regards, Boris
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Hi Christian, On Mon, Oct 30, 2023 at 08:38:45AM +0100, Christian König wrote: > Hi Boris, > > Am 27.10.23 um 12:17 schrieb Boris Brezillon: > > Hi Christian, > > > > On Fri, 27 Oct 2023 11:06:44 +0200 > > Christian König wrote: > > > > > Am 27.10.23 um 10:22 schrieb Boris Brezillon: > > > > On Fri, 27 Oct 2023 09:44:13 +0200 > > > > Christian König wrote: > > > > > Am 27.10.23 um 09:39 schrieb Boris Brezillon: > > > > > > On Fri, 27 Oct 2023 09:35:01 +0200 > > > > > > Christian König wrote: > > > > > > > Am 27.10.23 um 09:32 schrieb Boris Brezillon: > > > > > > > > On Fri, 27 Oct 2023 09:22:12 +0200 > > > > > > > > Christian König wrote: > > > > > > > > > > + > > > > > > > > > > + /** > > > > > > > > > > +* @update_job_credits: Called once the scheduler is > > > > > > > > > > considering this > > > > > > > > > > +* job for execution. > > > > > > > > > > +* > > > > > > > > > > +* Drivers may use this to update the job's submission > > > > > > > > > > credits, which is > > > > > > > > > > +* useful to e.g. deduct the number of native fences > > > > > > > > > > which have been > > > > > > > > > > +* signaled meanwhile. > > > > > > > > > > +* > > > > > > > > > > +* The callback must either return the new number of > > > > > > > > > > submission credits > > > > > > > > > > +* for the given job, or zero if no update is required. > > > > > > > > > > +* > > > > > > > > > > +* This callback is optional. > > > > > > > > > > +*/ > > > > > > > > > > + u32 (*update_job_credits)(struct drm_sched_job > > > > > > > > > > *sched_job); > > > > > > > > > Why do we need an extra callback for this? > > > > > > > > > > > > > > > > > > Just document that prepare_job() is allowed to reduce the > > > > > > > > > number of > > > > > > > > > credits the job might need. > > > > > > > > ->prepare_job() is called only once if the returned fence is > > > > > > > > NULL, but > > > > > > > > we need this credit-update to happen every time a job is > > > > > > > > considered for > > > > > > > > execution by the scheduler. > > > > > > > But the job is only considered for execution once. How do you see > > > > > > > that > > > > > > > this is called multiple times? > > > > > > Nope, it's not. If drm_sched_can_queue() returns false, the > > > > > > scheduler > > > > > > will go look for another entity that has a job ready for execution, > > > > > > and > > > > > > get back to this entity later, and test drm_sched_can_queue() again. > > > > > > Basically, any time drm_sched_can_queue() is called, the job credits > > > > > > update should happen, so we have an accurate view of how many > > > > > > credits > > > > > > this job needs. > > > > > Well, that is the handling which I already rejected because it creates > > > > > unfairness between processes. When you consider the credits needed > > > > > *before* scheduling jobs with a lower credit count are always > > > > > preferred > > > > > over jobs with a higher credit count. > > > > My bad, it doesn't pick another entity when an entity with a > > > > ready job that doesn't fit the queue is found, it just bails out from > > > > drm_sched_rq_select_entity_rr() and returns NULL (AKA: no ready entity > > > > found). But we still want to update the job credits before checking if > > > > the job fits or not (next time this entity is tested). > > > Why? I only see a few possibility here: > > > > > > 1. You need to wait for submissions to the same scheduler to finish > > > before the one you want to push to the ring. > > > This case can be avoided by trying to cast the dependency fences to > > > drm_sched_fences and looking if they are already scheduled. > > > > > > 2. You need to wait for submissions to a different scheduler instance > > > and in this case you should probably return that as dependency instead. > > It's already described as a dependency, but native dependency waits are > > deferred to the FW (we wait on scheduled to run the job, not finished). > > The thing is, after ->prepare_job() returned NULL (no non-native deps > > remaining), and before ->run_job() gets called, there might be several > > of these native deps that get signaled, and we're still considering > > job->submission_credits as unchanged, when each of these signalled > > fence could have reduced the job credits, potentially allowing it to be > > submitted sooner. > > Ah, ok that at least clears up your intentions here. > > Question is if that is really that important for you? I mean you just seem > to fill up more of the ring buffer. > > > > > > So to me it looks like when prepare_job() is called because it is > > > selected as next job for submission you should already know how many > > > credits it needs. > > You know how many credits it needs when ->prepare_job() is called, but > > if this number is too high, the entity will not be picked, and next > > time it's checked, you'll still consider the job credits at the time > >
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Hi Boris, Am 27.10.23 um 12:17 schrieb Boris Brezillon: Hi Christian, On Fri, 27 Oct 2023 11:06:44 +0200 Christian König wrote: Am 27.10.23 um 10:22 schrieb Boris Brezillon: On Fri, 27 Oct 2023 09:44:13 +0200 Christian König wrote: Am 27.10.23 um 09:39 schrieb Boris Brezillon: On Fri, 27 Oct 2023 09:35:01 +0200 Christian König wrote: Am 27.10.23 um 09:32 schrieb Boris Brezillon: On Fri, 27 Oct 2023 09:22:12 +0200 Christian König wrote: + + /** +* @update_job_credits: Called once the scheduler is considering this +* job for execution. +* +* Drivers may use this to update the job's submission credits, which is +* useful to e.g. deduct the number of native fences which have been +* signaled meanwhile. +* +* The callback must either return the new number of submission credits +* for the given job, or zero if no update is required. +* +* This callback is optional. +*/ + u32 (*update_job_credits)(struct drm_sched_job *sched_job); Why do we need an extra callback for this? Just document that prepare_job() is allowed to reduce the number of credits the job might need. ->prepare_job() is called only once if the returned fence is NULL, but we need this credit-update to happen every time a job is considered for execution by the scheduler. But the job is only considered for execution once. How do you see that this is called multiple times? Nope, it's not. If drm_sched_can_queue() returns false, the scheduler will go look for another entity that has a job ready for execution, and get back to this entity later, and test drm_sched_can_queue() again. Basically, any time drm_sched_can_queue() is called, the job credits update should happen, so we have an accurate view of how many credits this job needs. Well, that is the handling which I already rejected because it creates unfairness between processes. When you consider the credits needed *before* scheduling jobs with a lower credit count are always preferred over jobs with a higher credit count. My bad, it doesn't pick another entity when an entity with a ready job that doesn't fit the queue is found, it just bails out from drm_sched_rq_select_entity_rr() and returns NULL (AKA: no ready entity found). But we still want to update the job credits before checking if the job fits or not (next time this entity is tested). Why? I only see a few possibility here: 1. You need to wait for submissions to the same scheduler to finish before the one you want to push to the ring. This case can be avoided by trying to cast the dependency fences to drm_sched_fences and looking if they are already scheduled. 2. You need to wait for submissions to a different scheduler instance and in this case you should probably return that as dependency instead. It's already described as a dependency, but native dependency waits are deferred to the FW (we wait on scheduled to run the job, not finished). The thing is, after ->prepare_job() returned NULL (no non-native deps remaining), and before ->run_job() gets called, there might be several of these native deps that get signaled, and we're still considering job->submission_credits as unchanged, when each of these signalled fence could have reduced the job credits, potentially allowing it to be submitted sooner. Ah, ok that at least clears up your intentions here. Question is if that is really that important for you? I mean you just seem to fill up more of the ring buffer. So to me it looks like when prepare_job() is called because it is selected as next job for submission you should already know how many credits it needs. You know how many credits it needs when ->prepare_job() is called, but if this number is too high, the entity will not be picked, and next time it's checked, you'll still consider the job credits at the time ->prepare_job() was called, which might differ from the current job credits (signalled native fences might have been signalled in the meantime, and they could be evicted). What you can do is to look at the credits of a job *after* it was picked up for scheduling so that you can scheduler more jobs. Sure, but then you might further delay your job if something made it smaller (ie. native fences got signaled) between ->prepare_job() and drm_sched_can_queue(). And any new drm_sched_can_queue() test would just see the old credits value. Out of curiosity, what are you worried about with this optional ->update_job_credits() call in the drm_sched_can_queue() path? Is the if (sched->update_job_credits) overhead considered too high for drivers that don't need it? Since the dma_fences are also used for resource management the scheduler is vital for correct system operation. We had massively problems because people tried to over-optimize the dma_fence handling which lead to very hard to narrow down memory corruptions. So for every change you come up here you need to
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On 2023-10-27 12:41, Boris Brezillon wrote: > On Fri, 27 Oct 2023 10:32:52 -0400 > Luben Tuikov wrote: > >> On 2023-10-27 04:25, Boris Brezillon wrote: >>> Hi Danilo, >>> >>> On Thu, 26 Oct 2023 18:13:00 +0200 >>> Danilo Krummrich wrote: >>> Currently, job flow control is implemented simply by limiting the number of jobs in flight. Therefore, a scheduler is initialized with a credit limit that corresponds to the number of jobs which can be sent to the hardware. This implies that for each job, drivers need to account for the maximum job size possible in order to not overflow the ring buffer. However, there are drivers, such as Nouveau, where the job size has a rather large range. For such drivers it can easily happen that job submissions not even filling the ring by 1% can block subsequent submissions, which, in the worst case, can lead to the ring run dry. In order to overcome this issue, allow for tracking the actual job size instead of the number of jobs. Therefore, add a field to track a job's credit count, which represents the number of credits a job contributes to the scheduler's credit limit. Signed-off-by: Danilo Krummrich --- Changes in V2: == - fixed up influence on scheduling fairness due to consideration of a job's size - If we reach a ready entity in drm_sched_select_entity() but can't actually queue a job from it due to size limitations, just give up and go to sleep until woken up due to a pending job finishing, rather than continue to try other entities. - added a callback to dynamically update a job's credits (Boris) >>> >>> This callback seems controversial. I'd suggest dropping it, so the >>> patch can be merged. >> >> Sorry, why is it controversial? (I did read back-and-forth above, but it >> wasn't clear >> why it is /controversial/.) > > That's a question for Christian, I guess. I didn't quite get what he > was worried about, other than this hook introducing a new way for > drivers to screw things up by returning funky/invalid credits (which we It's up to the driver--they can test, test, test, and fix their code and so on. We can only do so much and shouldn't be baby-sitting drivers ad nauseam. Drivers can also not define this callback. :-) > can report with WARN_ON()s). But let's be honest, there's probably a > hundred different ways (if not more) drivers can shoot themselves in the > foot with drm_sched already... Yes, that's true. So there's no worries with this hook. > >> >> I believe only drivers are privy to changes in the credit availability as >> their >> firmware and hardware executes new jobs and finishes others, and so this >> "update" >> here is essential--leaving it only to prepare_job() wouldn't quite fulfill >> the vision >> of why the credit mechanism introduced by this patch in the first place. > > I kinda agree with you, even if I wouldn't so pessimistic as to how > useful this patch would be without the ->update_job_credits() hook > (it already makes the situation a lot better for Nouveau and probably > other drivers too). Sure, and that's a good thing. The heart of the dynamic credit scheme this patch is introducing *is* update_job_credits() callback. Without it, it's just about like the current job flow-control scheme we have with varied job weights (credits). Remember, it is an optional callback and driver can choose NOT to define it--simple. :-) So, I'm very excited about this, and see a wide range of applications and tricks drivers can do with the credit scheme (albeit had it been an "int" bwha-ha-ha-ha ]:-> ). Have a good weekend everyone! -- Regards, Luben OpenPGP_0x4C15479431A334AF.asc Description: OpenPGP public key OpenPGP_signature.asc Description: OpenPGP digital signature
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On 2023-10-27 12:31, Boris Brezillon wrote: > On Fri, 27 Oct 2023 16:23:24 +0200 > Danilo Krummrich wrote: > >> On 10/27/23 10:25, Boris Brezillon wrote: >>> Hi Danilo, >>> >>> On Thu, 26 Oct 2023 18:13:00 +0200 >>> Danilo Krummrich wrote: >>> Currently, job flow control is implemented simply by limiting the number of jobs in flight. Therefore, a scheduler is initialized with a credit limit that corresponds to the number of jobs which can be sent to the hardware. This implies that for each job, drivers need to account for the maximum job size possible in order to not overflow the ring buffer. However, there are drivers, such as Nouveau, where the job size has a rather large range. For such drivers it can easily happen that job submissions not even filling the ring by 1% can block subsequent submissions, which, in the worst case, can lead to the ring run dry. In order to overcome this issue, allow for tracking the actual job size instead of the number of jobs. Therefore, add a field to track a job's credit count, which represents the number of credits a job contributes to the scheduler's credit limit. Signed-off-by: Danilo Krummrich --- Changes in V2: == - fixed up influence on scheduling fairness due to consideration of a job's size - If we reach a ready entity in drm_sched_select_entity() but can't actually queue a job from it due to size limitations, just give up and go to sleep until woken up due to a pending job finishing, rather than continue to try other entities. - added a callback to dynamically update a job's credits (Boris) >>> >>> This callback seems controversial. I'd suggest dropping it, so the >>> patch can be merged. >> >> I don't think we should drop it just for the sake of moving forward. If >> there are objections >> we'll discuss them. I've seen good reasons why the drivers you are working >> on require this, >> while, following the discussion, I have *not* seen any reasons to drop it. >> It helps simplifying >> driver code and doesn't introduce any complexity or overhead to existing >> drivers. > > Up to you. I'm just saying, moving one step in the right direction is > better than being stuck, and it's not like adding this callback in a > follow-up patch is super complicated either. If you're confident that > we can get all parties to agree on keeping this hook, fine by me. I'd rather have it in now, as it is really *the vision* of this patch. There's no point in pushing in something half-baked. -- Regards, Luben OpenPGP_0x4C15479431A334AF.asc Description: OpenPGP public key OpenPGP_signature.asc Description: OpenPGP digital signature
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Hi,
On 2023-10-27 12:26, Boris Brezillon wrote:
> On Fri, 27 Oct 2023 16:34:26 +0200
> Danilo Krummrich wrote:
>
>> On 10/27/23 09:17, Boris Brezillon wrote:
>>> Hi Danilo,
>>>
>>> On Thu, 26 Oct 2023 18:13:00 +0200
>>> Danilo Krummrich wrote:
>>>
+
+ /**
+ * @update_job_credits: Called once the scheduler is considering this
+ * job for execution.
+ *
+ * Drivers may use this to update the job's submission credits, which is
+ * useful to e.g. deduct the number of native fences which have been
+ * signaled meanwhile.
+ *
+ * The callback must either return the new number of submission credits
+ * for the given job, or zero if no update is required.
+ *
+ * This callback is optional.
+ */
+ u32 (*update_job_credits)(struct drm_sched_job *sched_job);
>>>
>>> I'm copying my late reply to v2 here so it doesn't get lost:
>>>
>>> I keep thinking it'd be simpler to make this a void function that
>>> updates s_job->submission_credits directly. I also don't see the
>>> problem with doing a sanity check on job->submission_credits. I mean,
>>> if the driver is doing something silly, you can't do much to prevent it
>>> anyway, except warn the user that something wrong has happened. If you
>>> want to
>>>
>>> WARN_ON(job->submission_credits == 0 ||
>>> job->submission_credits > job_old_submission_credits);
>>>
>>> that's fine. But none of this sanity checking has to do with the
>>> function prototype/semantics, and I'm still not comfortable with this 0
>>> => no-change. If there's no change, we should just leave
>>> job->submission_credits unchanged (or return job->submission_credits)
>>> instead of inventing a new special case.
>>
>> If we can avoid letting drivers change fields of generic structures directly
>> without any drawbacks I think we should avoid it. Currently, drivers
>> shouldn't
>> have the need to mess with job->credits directly. The initial value is set
>> through drm_sched_job_init() and is updated through the return value of
>> update_job_credits().
>
> Fair enough. I do agree that keeping internal fields out of driver
> hands is a good thing in general, it's just that it's already
> free-for-all in so many places in drm_sched (like the fact drivers
"Free-for-all" doesn't mean we need to follow suit. We should keep
good programming practices, as this patch strives to.
> iterate the pending list in their stop-queue handling) that I didn't
> really see it as an issue. Note that's there's always the option of
> providing drm_sched_job_{update,get}_credits() helpers, with the update
> helper making sure the new credits value is consistent (smaller or
> equal to the old one, and not zero).
>
>>
>> I'm fine getting rid of the 0 => no-change semantics though. Instead we can
>> just
>> WARN() on 0.
>
> Yeah, I think that's preferable. It's pretty easy to return the old
> value if the driver has a way to detect when nothing changed (with a
> get helper if you don't want drivers to touch the credits field).
>
>> However, if we do that I'd also want to change it for
>> drm_sched_job_init() (where 0 currently defaults to 1) such that we accept
>> 0, but
>> WARN() accordingly.
>
> Sure. You update all drivers anyway, so passing 1 instead of 0 is not a
> big deal, I would say.
At this point in time, we should consider 1 as normal, 0 out of spec and
WARN on it but carry on and (perhaps) reset it to 1. Drivers in the future, may
see a need (i.e. do tricks) to return 0, at which point they'll submit a patch
which
does two things, 1) removes the WARN, 2) removes the reset from 0 to 1, and
explain why they need to return 0 to allow (one more) job, but we're nowhere
near then yet,
so status quo for now.
I don't see how it makes sense to call drm_sched_job_init(credits:0), and I
believe
the code is correct to default to 1 in that case--which defaults to the current
flow control we have, which we want.
>
>>
>> I think it's consequent to either consistently give 0 a different meaning or
>> just
>> accept it but WARN() on it.
>
> Using default as a default value makes sense when you're passing
I suppose you meant "using zero as a default value".
> zero-initialized objects that are later extended with new fields, but
> here you update the function prototype and all the call sites, so we're
> better off considering 0 as an invalid value, IMHO.
Yes, absolutely.
You never want to give 0 a meaning, since as you pointed out, it is zero-ed
memory, and as such, can have any meaning you'd like. So yes: WARN on 0;
1 is good and normal.
Regards,
Luben
OpenPGP_0x4C15479431A334AF.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On Fri, 27 Oct 2023 10:32:52 -0400 Luben Tuikov wrote: > On 2023-10-27 04:25, Boris Brezillon wrote: > > Hi Danilo, > > > > On Thu, 26 Oct 2023 18:13:00 +0200 > > Danilo Krummrich wrote: > > > >> Currently, job flow control is implemented simply by limiting the number > >> of jobs in flight. Therefore, a scheduler is initialized with a credit > >> limit that corresponds to the number of jobs which can be sent to the > >> hardware. > >> > >> This implies that for each job, drivers need to account for the maximum > >> job size possible in order to not overflow the ring buffer. > >> > >> However, there are drivers, such as Nouveau, where the job size has a > >> rather large range. For such drivers it can easily happen that job > >> submissions not even filling the ring by 1% can block subsequent > >> submissions, which, in the worst case, can lead to the ring run dry. > >> > >> In order to overcome this issue, allow for tracking the actual job size > >> instead of the number of jobs. Therefore, add a field to track a job's > >> credit count, which represents the number of credits a job contributes > >> to the scheduler's credit limit. > >> > >> Signed-off-by: Danilo Krummrich > >> --- > >> Changes in V2: > >> == > >> - fixed up influence on scheduling fairness due to consideration of a > >> job's > >> size > >> - If we reach a ready entity in drm_sched_select_entity() but can't > >> actually > >> queue a job from it due to size limitations, just give up and go to > >> sleep > >> until woken up due to a pending job finishing, rather than continue > >> to try > >> other entities. > >> - added a callback to dynamically update a job's credits (Boris) > > > > This callback seems controversial. I'd suggest dropping it, so the > > patch can be merged. > > Sorry, why is it controversial? (I did read back-and-forth above, but it > wasn't clear > why it is /controversial/.) That's a question for Christian, I guess. I didn't quite get what he was worried about, other than this hook introducing a new way for drivers to screw things up by returning funky/invalid credits (which we can report with WARN_ON()s). But let's be honest, there's probably a hundred different ways (if not more) drivers can shoot themselves in the foot with drm_sched already... > > I believe only drivers are privy to changes in the credit availability as > their > firmware and hardware executes new jobs and finishes others, and so this > "update" > here is essential--leaving it only to prepare_job() wouldn't quite fulfill > the vision > of why the credit mechanism introduced by this patch in the first place. I kinda agree with you, even if I wouldn't so pessimistic as to how useful this patch would be without the ->update_job_credits() hook (it already makes the situation a lot better for Nouveau and probably other drivers too).
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On Fri, 27 Oct 2023 16:23:24 +0200 Danilo Krummrich wrote: > On 10/27/23 10:25, Boris Brezillon wrote: > > Hi Danilo, > > > > On Thu, 26 Oct 2023 18:13:00 +0200 > > Danilo Krummrich wrote: > > > >> Currently, job flow control is implemented simply by limiting the number > >> of jobs in flight. Therefore, a scheduler is initialized with a credit > >> limit that corresponds to the number of jobs which can be sent to the > >> hardware. > >> > >> This implies that for each job, drivers need to account for the maximum > >> job size possible in order to not overflow the ring buffer. > >> > >> However, there are drivers, such as Nouveau, where the job size has a > >> rather large range. For such drivers it can easily happen that job > >> submissions not even filling the ring by 1% can block subsequent > >> submissions, which, in the worst case, can lead to the ring run dry. > >> > >> In order to overcome this issue, allow for tracking the actual job size > >> instead of the number of jobs. Therefore, add a field to track a job's > >> credit count, which represents the number of credits a job contributes > >> to the scheduler's credit limit. > >> > >> Signed-off-by: Danilo Krummrich > >> --- > >> Changes in V2: > >> == > >>- fixed up influence on scheduling fairness due to consideration of a > >> job's > >> size > >> - If we reach a ready entity in drm_sched_select_entity() but can't > >> actually > >>queue a job from it due to size limitations, just give up and go to > >> sleep > >>until woken up due to a pending job finishing, rather than continue > >> to try > >>other entities. > >>- added a callback to dynamically update a job's credits (Boris) > > > > This callback seems controversial. I'd suggest dropping it, so the > > patch can be merged. > > I don't think we should drop it just for the sake of moving forward. If there > are objections > we'll discuss them. I've seen good reasons why the drivers you are working on > require this, > while, following the discussion, I have *not* seen any reasons to drop it. It > helps simplifying > driver code and doesn't introduce any complexity or overhead to existing > drivers. Up to you. I'm just saying, moving one step in the right direction is better than being stuck, and it's not like adding this callback in a follow-up patch is super complicated either. If you're confident that we can get all parties to agree on keeping this hook, fine by me.
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On Fri, 27 Oct 2023 16:34:26 +0200
Danilo Krummrich wrote:
> On 10/27/23 09:17, Boris Brezillon wrote:
> > Hi Danilo,
> >
> > On Thu, 26 Oct 2023 18:13:00 +0200
> > Danilo Krummrich wrote:
> >
> >> +
> >> + /**
> >> + * @update_job_credits: Called once the scheduler is considering this
> >> + * job for execution.
> >> + *
> >> + * Drivers may use this to update the job's submission credits, which is
> >> + * useful to e.g. deduct the number of native fences which have been
> >> + * signaled meanwhile.
> >> + *
> >> + * The callback must either return the new number of submission credits
> >> + * for the given job, or zero if no update is required.
> >> + *
> >> + * This callback is optional.
> >> + */
> >> + u32 (*update_job_credits)(struct drm_sched_job *sched_job);
> >
> > I'm copying my late reply to v2 here so it doesn't get lost:
> >
> > I keep thinking it'd be simpler to make this a void function that
> > updates s_job->submission_credits directly. I also don't see the
> > problem with doing a sanity check on job->submission_credits. I mean,
> > if the driver is doing something silly, you can't do much to prevent it
> > anyway, except warn the user that something wrong has happened. If you
> > want to
> >
> > WARN_ON(job->submission_credits == 0 ||
> > job->submission_credits > job_old_submission_credits);
> >
> > that's fine. But none of this sanity checking has to do with the
> > function prototype/semantics, and I'm still not comfortable with this 0
> > => no-change. If there's no change, we should just leave
> > job->submission_credits unchanged (or return job->submission_credits)
> > instead of inventing a new special case.
>
> If we can avoid letting drivers change fields of generic structures directly
> without any drawbacks I think we should avoid it. Currently, drivers shouldn't
> have the need to mess with job->credits directly. The initial value is set
> through drm_sched_job_init() and is updated through the return value of
> update_job_credits().
Fair enough. I do agree that keeping internal fields out of driver
hands is a good thing in general, it's just that it's already
free-for-all in so many places in drm_sched (like the fact drivers
iterate the pending list in their stop-queue handling) that I didn't
really see it as an issue. Note that's there's always the option of
providing drm_sched_job_{update,get}_credits() helpers, with the update
helper making sure the new credits value is consistent (smaller or
equal to the old one, and not zero).
>
> I'm fine getting rid of the 0 => no-change semantics though. Instead we can
> just
> WARN() on 0.
Yeah, I think that's preferable. It's pretty easy to return the old
value if the driver has a way to detect when nothing changed (with a
get helper if you don't want drivers to touch the credits field).
> However, if we do that I'd also want to change it for
> drm_sched_job_init() (where 0 currently defaults to 1) such that we accept 0,
> but
> WARN() accordingly.
Sure. You update all drivers anyway, so passing 1 instead of 0 is not a
big deal, I would say.
>
> I think it's consequent to either consistently give 0 a different meaning or
> just
> accept it but WARN() on it.
Using default as a default value makes sense when you're passing
zero-initialized objects that are later extended with new fields, but
here you update the function prototype and all the call sites, so we're
better off considering 0 as an invalid value, IMHO.
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On 10/27/23 16:59, Luben Tuikov wrote: Hi Danilo, On 2023-10-27 10:45, Danilo Krummrich wrote: Hi Luben, On 10/26/23 23:13, Luben Tuikov wrote: On 2023-10-26 12:13, Danilo Krummrich wrote: Currently, job flow control is implemented simply by limiting the number of jobs in flight. Therefore, a scheduler is initialized with a credit limit that corresponds to the number of jobs which can be sent to the hardware. This implies that for each job, drivers need to account for the maximum job size possible in order to not overflow the ring buffer. However, there are drivers, such as Nouveau, where the job size has a rather large range. For such drivers it can easily happen that job submissions not even filling the ring by 1% can block subsequent submissions, which, in the worst case, can lead to the ring run dry. In order to overcome this issue, allow for tracking the actual job size instead of the number of jobs. Therefore, add a field to track a job's credit count, which represents the number of credits a job contributes to the scheduler's credit limit. Signed-off-by: Danilo Krummrich --- Changes in V2: == - fixed up influence on scheduling fairness due to consideration of a job's size - If we reach a ready entity in drm_sched_select_entity() but can't actually queue a job from it due to size limitations, just give up and go to sleep until woken up due to a pending job finishing, rather than continue to try other entities. - added a callback to dynamically update a job's credits (Boris) - renamed 'units' to 'credits' - fixed commit message and comments as requested by Luben Changes in V3: == - rebased onto V7 of the "DRM scheduler changes for Xe" series by Matt - move up drm_sched_can_queue() instead of adding a forward declaration (Boris) - add a drm_sched_available_credits() helper (Boris) - adjust control flow in drm_sched_rq_select_entity_fifo() to Luben's proposal - re-phrase a few comments and fix a typo (Luben) - change naming of all structures credit fields and function parameters to the following scheme - drm_sched_job::credits - drm_gpu_scheduler::credit_limit - drm_gpu_scheduler::credit_count - drm_sched_init(..., u32 credit_limit, ...) - drm_sched_job_init(..., u32 credits, ...) - add proper documentation for the scheduler's job-flow control mechanism This patch is based on V7 of the "DRM scheduler changes for Xe" series. [1] provides a branch based on drm-misc-next, with the named series and this patch on top of it. [1] https://gitlab.freedesktop.org/nouvelles/kernel/-/commits/sched-credits/ --- Documentation/gpu/drm-mm.rst | 6 + drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +- drivers/gpu/drm/lima/lima_sched.c | 2 +- drivers/gpu/drm/msm/msm_gem_submit.c | 2 +- drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +- .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +- drivers/gpu/drm/scheduler/sched_entity.c | 4 +- drivers/gpu/drm/scheduler/sched_main.c| 142 ++ drivers/gpu/drm/v3d/v3d_gem.c | 2 +- include/drm/gpu_scheduler.h | 33 +++- 12 files changed, 152 insertions(+), 49 deletions(-) diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst index 602010cb6894..acc5901ac840 100644 --- a/Documentation/gpu/drm-mm.rst +++ b/Documentation/gpu/drm-mm.rst @@ -552,6 +552,12 @@ Overview .. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c :doc: Overview +Flow Control + + +.. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c + :doc: Flow Control + Scheduler Function References - diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c index 1f357198533f..62bb7fc7448a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (!entity) return 0; - return drm_sched_job_init(&(*job)->base, entity, owner); + return drm_sched_job_init(&(*job)->base, entity, 1, owner); } int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev, diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 2416c526f9b0..3d0f8d182506 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -535,7 +535,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, ret = drm_sched_job_init(>sched_job, >sched_entity[args->pipe], -
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Hi Danilo, On 2023-10-27 10:45, Danilo Krummrich wrote: > Hi Luben, > > On 10/26/23 23:13, Luben Tuikov wrote: >> On 2023-10-26 12:13, Danilo Krummrich wrote: >>> Currently, job flow control is implemented simply by limiting the number >>> of jobs in flight. Therefore, a scheduler is initialized with a credit >>> limit that corresponds to the number of jobs which can be sent to the >>> hardware. >>> >>> This implies that for each job, drivers need to account for the maximum >>> job size possible in order to not overflow the ring buffer. >>> >>> However, there are drivers, such as Nouveau, where the job size has a >>> rather large range. For such drivers it can easily happen that job >>> submissions not even filling the ring by 1% can block subsequent >>> submissions, which, in the worst case, can lead to the ring run dry. >>> >>> In order to overcome this issue, allow for tracking the actual job size >>> instead of the number of jobs. Therefore, add a field to track a job's >>> credit count, which represents the number of credits a job contributes >>> to the scheduler's credit limit. >>> >>> Signed-off-by: Danilo Krummrich >>> --- >>> Changes in V2: >>> == >>>- fixed up influence on scheduling fairness due to consideration of a >>> job's >>> size >>> - If we reach a ready entity in drm_sched_select_entity() but can't >>> actually >>>queue a job from it due to size limitations, just give up and go to >>> sleep >>>until woken up due to a pending job finishing, rather than continue >>> to try >>>other entities. >>>- added a callback to dynamically update a job's credits (Boris) >>>- renamed 'units' to 'credits' >>>- fixed commit message and comments as requested by Luben >>> >>> Changes in V3: >>> == >>>- rebased onto V7 of the "DRM scheduler changes for Xe" series by Matt >>>- move up drm_sched_can_queue() instead of adding a forward declaration >>> (Boris) >>>- add a drm_sched_available_credits() helper (Boris) >>>- adjust control flow in drm_sched_rq_select_entity_fifo() to Luben's >>> proposal >>>- re-phrase a few comments and fix a typo (Luben) >>>- change naming of all structures credit fields and function parameters >>> to the >>> following scheme >>> - drm_sched_job::credits >>> - drm_gpu_scheduler::credit_limit >>> - drm_gpu_scheduler::credit_count >>> - drm_sched_init(..., u32 credit_limit, ...) >>> - drm_sched_job_init(..., u32 credits, ...) >>>- add proper documentation for the scheduler's job-flow control mechanism >>> >>> This patch is based on V7 of the "DRM scheduler changes for Xe" series. [1] >>> provides a branch based on drm-misc-next, with the named series and this >>> patch >>> on top of it. >>> >>> [1] https://gitlab.freedesktop.org/nouvelles/kernel/-/commits/sched-credits/ >>> --- >>> Documentation/gpu/drm-mm.rst | 6 + >>> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +- >>> drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +- >>> drivers/gpu/drm/lima/lima_sched.c | 2 +- >>> drivers/gpu/drm/msm/msm_gem_submit.c | 2 +- >>> drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +- >>> drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +- >>> .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +- >>> drivers/gpu/drm/scheduler/sched_entity.c | 4 +- >>> drivers/gpu/drm/scheduler/sched_main.c| 142 ++ >>> drivers/gpu/drm/v3d/v3d_gem.c | 2 +- >>> include/drm/gpu_scheduler.h | 33 +++- >>> 12 files changed, 152 insertions(+), 49 deletions(-) >>> >>> diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst >>> index 602010cb6894..acc5901ac840 100644 >>> --- a/Documentation/gpu/drm-mm.rst >>> +++ b/Documentation/gpu/drm-mm.rst >>> @@ -552,6 +552,12 @@ Overview >>> .. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c >>> :doc: Overview >>> >>> +Flow Control >>> + >>> + >>> +.. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c >>> + :doc: Flow Control >>> + >>> Scheduler Function References >>> - >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c >>> index 1f357198533f..62bb7fc7448a 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c >>> @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct >>> amdgpu_vm *vm, >>> if (!entity) >>> return 0; >>> >>> - return drm_sched_job_init(&(*job)->base, entity, owner); >>> + return drm_sched_job_init(&(*job)->base, entity, 1, owner); >>> } >>> >>> int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev, >>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c >>> b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c >>> index
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On 10/27/23 03:03, Luben Tuikov wrote: On 2023-10-26 17:13, Luben Tuikov wrote: On 2023-10-26 12:13, Danilo Krummrich wrote: Currently, job flow control is implemented simply by limiting the number of jobs in flight. Therefore, a scheduler is initialized with a credit limit that corresponds to the number of jobs which can be sent to the hardware. This implies that for each job, drivers need to account for the maximum job size possible in order to not overflow the ring buffer. However, there are drivers, such as Nouveau, where the job size has a rather large range. For such drivers it can easily happen that job submissions not even filling the ring by 1% can block subsequent submissions, which, in the worst case, can lead to the ring run dry. In order to overcome this issue, allow for tracking the actual job size instead of the number of jobs. Therefore, add a field to track a job's credit count, which represents the number of credits a job contributes to the scheduler's credit limit. Signed-off-by: Danilo Krummrich --- Changes in V2: == - fixed up influence on scheduling fairness due to consideration of a job's size - If we reach a ready entity in drm_sched_select_entity() but can't actually queue a job from it due to size limitations, just give up and go to sleep until woken up due to a pending job finishing, rather than continue to try other entities. - added a callback to dynamically update a job's credits (Boris) - renamed 'units' to 'credits' - fixed commit message and comments as requested by Luben Changes in V3: == - rebased onto V7 of the "DRM scheduler changes for Xe" series by Matt - move up drm_sched_can_queue() instead of adding a forward declaration (Boris) - add a drm_sched_available_credits() helper (Boris) - adjust control flow in drm_sched_rq_select_entity_fifo() to Luben's proposal - re-phrase a few comments and fix a typo (Luben) - change naming of all structures credit fields and function parameters to the following scheme - drm_sched_job::credits - drm_gpu_scheduler::credit_limit - drm_gpu_scheduler::credit_count - drm_sched_init(..., u32 credit_limit, ...) - drm_sched_job_init(..., u32 credits, ...) - add proper documentation for the scheduler's job-flow control mechanism This patch is based on V7 of the "DRM scheduler changes for Xe" series. [1] provides a branch based on drm-misc-next, with the named series and this patch on top of it. [1] https://gitlab.freedesktop.org/nouvelles/kernel/-/commits/sched-credits/ --- Documentation/gpu/drm-mm.rst | 6 + drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +- drivers/gpu/drm/lima/lima_sched.c | 2 +- drivers/gpu/drm/msm/msm_gem_submit.c | 2 +- drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +- .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +- drivers/gpu/drm/scheduler/sched_entity.c | 4 +- drivers/gpu/drm/scheduler/sched_main.c| 142 ++ drivers/gpu/drm/v3d/v3d_gem.c | 2 +- include/drm/gpu_scheduler.h | 33 +++- 12 files changed, 152 insertions(+), 49 deletions(-) diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst index 602010cb6894..acc5901ac840 100644 --- a/Documentation/gpu/drm-mm.rst +++ b/Documentation/gpu/drm-mm.rst @@ -552,6 +552,12 @@ Overview .. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c :doc: Overview +Flow Control + + +.. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c + :doc: Flow Control + Scheduler Function References - diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c index 1f357198533f..62bb7fc7448a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (!entity) return 0; - return drm_sched_job_init(&(*job)->base, entity, owner); + return drm_sched_job_init(&(*job)->base, entity, 1, owner); } int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev, diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 2416c526f9b0..3d0f8d182506 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -535,7 +535,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, ret = drm_sched_job_init(>sched_job, >sched_entity[args->pipe], -submit->ctx); +1, submit->ctx); if (ret) goto
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Hi Luben, On 10/26/23 23:13, Luben Tuikov wrote: On 2023-10-26 12:13, Danilo Krummrich wrote: Currently, job flow control is implemented simply by limiting the number of jobs in flight. Therefore, a scheduler is initialized with a credit limit that corresponds to the number of jobs which can be sent to the hardware. This implies that for each job, drivers need to account for the maximum job size possible in order to not overflow the ring buffer. However, there are drivers, such as Nouveau, where the job size has a rather large range. For such drivers it can easily happen that job submissions not even filling the ring by 1% can block subsequent submissions, which, in the worst case, can lead to the ring run dry. In order to overcome this issue, allow for tracking the actual job size instead of the number of jobs. Therefore, add a field to track a job's credit count, which represents the number of credits a job contributes to the scheduler's credit limit. Signed-off-by: Danilo Krummrich --- Changes in V2: == - fixed up influence on scheduling fairness due to consideration of a job's size - If we reach a ready entity in drm_sched_select_entity() but can't actually queue a job from it due to size limitations, just give up and go to sleep until woken up due to a pending job finishing, rather than continue to try other entities. - added a callback to dynamically update a job's credits (Boris) - renamed 'units' to 'credits' - fixed commit message and comments as requested by Luben Changes in V3: == - rebased onto V7 of the "DRM scheduler changes for Xe" series by Matt - move up drm_sched_can_queue() instead of adding a forward declaration (Boris) - add a drm_sched_available_credits() helper (Boris) - adjust control flow in drm_sched_rq_select_entity_fifo() to Luben's proposal - re-phrase a few comments and fix a typo (Luben) - change naming of all structures credit fields and function parameters to the following scheme - drm_sched_job::credits - drm_gpu_scheduler::credit_limit - drm_gpu_scheduler::credit_count - drm_sched_init(..., u32 credit_limit, ...) - drm_sched_job_init(..., u32 credits, ...) - add proper documentation for the scheduler's job-flow control mechanism This patch is based on V7 of the "DRM scheduler changes for Xe" series. [1] provides a branch based on drm-misc-next, with the named series and this patch on top of it. [1] https://gitlab.freedesktop.org/nouvelles/kernel/-/commits/sched-credits/ --- Documentation/gpu/drm-mm.rst | 6 + drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +- drivers/gpu/drm/lima/lima_sched.c | 2 +- drivers/gpu/drm/msm/msm_gem_submit.c | 2 +- drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +- .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +- drivers/gpu/drm/scheduler/sched_entity.c | 4 +- drivers/gpu/drm/scheduler/sched_main.c| 142 ++ drivers/gpu/drm/v3d/v3d_gem.c | 2 +- include/drm/gpu_scheduler.h | 33 +++- 12 files changed, 152 insertions(+), 49 deletions(-) diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst index 602010cb6894..acc5901ac840 100644 --- a/Documentation/gpu/drm-mm.rst +++ b/Documentation/gpu/drm-mm.rst @@ -552,6 +552,12 @@ Overview .. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c :doc: Overview +Flow Control + + +.. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c + :doc: Flow Control + Scheduler Function References - diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c index 1f357198533f..62bb7fc7448a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (!entity) return 0; - return drm_sched_job_init(&(*job)->base, entity, owner); + return drm_sched_job_init(&(*job)->base, entity, 1, owner); } int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev, diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 2416c526f9b0..3d0f8d182506 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -535,7 +535,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, ret = drm_sched_job_init(>sched_job, >sched_entity[args->pipe], -submit->ctx); +1, submit->ctx); if (ret) goto err_submit_put; diff --git
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On 10/27/23 09:17, Boris Brezillon wrote: Hi Danilo, On Thu, 26 Oct 2023 18:13:00 +0200 Danilo Krummrich wrote: + + /** +* @update_job_credits: Called once the scheduler is considering this +* job for execution. +* +* Drivers may use this to update the job's submission credits, which is +* useful to e.g. deduct the number of native fences which have been +* signaled meanwhile. +* +* The callback must either return the new number of submission credits +* for the given job, or zero if no update is required. +* +* This callback is optional. +*/ + u32 (*update_job_credits)(struct drm_sched_job *sched_job); I'm copying my late reply to v2 here so it doesn't get lost: I keep thinking it'd be simpler to make this a void function that updates s_job->submission_credits directly. I also don't see the problem with doing a sanity check on job->submission_credits. I mean, if the driver is doing something silly, you can't do much to prevent it anyway, except warn the user that something wrong has happened. If you want to WARN_ON(job->submission_credits == 0 || job->submission_credits > job_old_submission_credits); that's fine. But none of this sanity checking has to do with the function prototype/semantics, and I'm still not comfortable with this 0 => no-change. If there's no change, we should just leave job->submission_credits unchanged (or return job->submission_credits) instead of inventing a new special case. If we can avoid letting drivers change fields of generic structures directly without any drawbacks I think we should avoid it. Currently, drivers shouldn't have the need to mess with job->credits directly. The initial value is set through drm_sched_job_init() and is updated through the return value of update_job_credits(). I'm fine getting rid of the 0 => no-change semantics though. Instead we can just WARN() on 0. However, if we do that I'd also want to change it for drm_sched_job_init() (where 0 currently defaults to 1) such that we accept 0, but WARN() accordingly. I think it's consequent to either consistently give 0 a different meaning or just accept it but WARN() on it. Regards, Boris
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On 2023-10-27 04:25, Boris Brezillon wrote: > Hi Danilo, > > On Thu, 26 Oct 2023 18:13:00 +0200 > Danilo Krummrich wrote: > >> Currently, job flow control is implemented simply by limiting the number >> of jobs in flight. Therefore, a scheduler is initialized with a credit >> limit that corresponds to the number of jobs which can be sent to the >> hardware. >> >> This implies that for each job, drivers need to account for the maximum >> job size possible in order to not overflow the ring buffer. >> >> However, there are drivers, such as Nouveau, where the job size has a >> rather large range. For such drivers it can easily happen that job >> submissions not even filling the ring by 1% can block subsequent >> submissions, which, in the worst case, can lead to the ring run dry. >> >> In order to overcome this issue, allow for tracking the actual job size >> instead of the number of jobs. Therefore, add a field to track a job's >> credit count, which represents the number of credits a job contributes >> to the scheduler's credit limit. >> >> Signed-off-by: Danilo Krummrich >> --- >> Changes in V2: >> == >> - fixed up influence on scheduling fairness due to consideration of a job's >> size >> - If we reach a ready entity in drm_sched_select_entity() but can't >> actually >> queue a job from it due to size limitations, just give up and go to >> sleep >> until woken up due to a pending job finishing, rather than continue to >> try >> other entities. >> - added a callback to dynamically update a job's credits (Boris) > > This callback seems controversial. I'd suggest dropping it, so the > patch can be merged. Sorry, why is it controversial? (I did read back-and-forth above, but it wasn't clear why it is /controversial/.) I believe only drivers are privy to changes in the credit availability as their firmware and hardware executes new jobs and finishes others, and so this "update" here is essential--leaving it only to prepare_job() wouldn't quite fulfill the vision of why the credit mechanism introduced by this patch in the first place. -- Regards, Luben OpenPGP_0x4C15479431A334AF.asc Description: OpenPGP public key OpenPGP_signature.asc Description: OpenPGP digital signature
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On 10/27/23 10:25, Boris Brezillon wrote: Hi Danilo, On Thu, 26 Oct 2023 18:13:00 +0200 Danilo Krummrich wrote: Currently, job flow control is implemented simply by limiting the number of jobs in flight. Therefore, a scheduler is initialized with a credit limit that corresponds to the number of jobs which can be sent to the hardware. This implies that for each job, drivers need to account for the maximum job size possible in order to not overflow the ring buffer. However, there are drivers, such as Nouveau, where the job size has a rather large range. For such drivers it can easily happen that job submissions not even filling the ring by 1% can block subsequent submissions, which, in the worst case, can lead to the ring run dry. In order to overcome this issue, allow for tracking the actual job size instead of the number of jobs. Therefore, add a field to track a job's credit count, which represents the number of credits a job contributes to the scheduler's credit limit. Signed-off-by: Danilo Krummrich --- Changes in V2: == - fixed up influence on scheduling fairness due to consideration of a job's size - If we reach a ready entity in drm_sched_select_entity() but can't actually queue a job from it due to size limitations, just give up and go to sleep until woken up due to a pending job finishing, rather than continue to try other entities. - added a callback to dynamically update a job's credits (Boris) This callback seems controversial. I'd suggest dropping it, so the patch can be merged. I don't think we should drop it just for the sake of moving forward. If there are objections we'll discuss them. I've seen good reasons why the drivers you are working on require this, while, following the discussion, I have *not* seen any reasons to drop it. It helps simplifying driver code and doesn't introduce any complexity or overhead to existing drivers. Regards, Boris - renamed 'units' to 'credits' - fixed commit message and comments as requested by Luben Changes in V3: == - rebased onto V7 of the "DRM scheduler changes for Xe" series by Matt - move up drm_sched_can_queue() instead of adding a forward declaration (Boris) - add a drm_sched_available_credits() helper (Boris) - adjust control flow in drm_sched_rq_select_entity_fifo() to Luben's proposal - re-phrase a few comments and fix a typo (Luben) - change naming of all structures credit fields and function parameters to the following scheme - drm_sched_job::credits - drm_gpu_scheduler::credit_limit - drm_gpu_scheduler::credit_count - drm_sched_init(..., u32 credit_limit, ...) - drm_sched_job_init(..., u32 credits, ...) - add proper documentation for the scheduler's job-flow control mechanism
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Hi Christian, On Fri, 27 Oct 2023 11:06:44 +0200 Christian König wrote: > Am 27.10.23 um 10:22 schrieb Boris Brezillon: > > On Fri, 27 Oct 2023 09:44:13 +0200 > > Christian König wrote: > > > >> Am 27.10.23 um 09:39 schrieb Boris Brezillon: > >>> On Fri, 27 Oct 2023 09:35:01 +0200 > >>> Christian König wrote: > >>> > Am 27.10.23 um 09:32 schrieb Boris Brezillon: > > On Fri, 27 Oct 2023 09:22:12 +0200 > > Christian König wrote: > > > >>> + > >>> + /** > >>> + * @update_job_credits: Called once the scheduler is > >>> considering this > >>> + * job for execution. > >>> + * > >>> + * Drivers may use this to update the job's submission credits, > >>> which is > >>> + * useful to e.g. deduct the number of native fences which have > >>> been > >>> + * signaled meanwhile. > >>> + * > >>> + * The callback must either return the new number of submission > >>> credits > >>> + * for the given job, or zero if no update is required. > >>> + * > >>> + * This callback is optional. > >>> + */ > >>> + u32 (*update_job_credits)(struct drm_sched_job *sched_job); > >> Why do we need an extra callback for this? > >> > >> Just document that prepare_job() is allowed to reduce the number of > >> credits the job might need. > > ->prepare_job() is called only once if the returned fence is NULL, but > > we need this credit-update to happen every time a job is considered for > > execution by the scheduler. > But the job is only considered for execution once. How do you see that > this is called multiple times? > >>> Nope, it's not. If drm_sched_can_queue() returns false, the scheduler > >>> will go look for another entity that has a job ready for execution, and > >>> get back to this entity later, and test drm_sched_can_queue() again. > >>> Basically, any time drm_sched_can_queue() is called, the job credits > >>> update should happen, so we have an accurate view of how many credits > >>> this job needs. > >> Well, that is the handling which I already rejected because it creates > >> unfairness between processes. When you consider the credits needed > >> *before* scheduling jobs with a lower credit count are always preferred > >> over jobs with a higher credit count. > > My bad, it doesn't pick another entity when an entity with a > > ready job that doesn't fit the queue is found, it just bails out from > > drm_sched_rq_select_entity_rr() and returns NULL (AKA: no ready entity > > found). But we still want to update the job credits before checking if > > the job fits or not (next time this entity is tested). > > Why? I only see a few possibility here: > > 1. You need to wait for submissions to the same scheduler to finish > before the one you want to push to the ring. > This case can be avoided by trying to cast the dependency fences to > drm_sched_fences and looking if they are already scheduled. > > 2. You need to wait for submissions to a different scheduler instance > and in this case you should probably return that as dependency instead. It's already described as a dependency, but native dependency waits are deferred to the FW (we wait on scheduled to run the job, not finished). The thing is, after ->prepare_job() returned NULL (no non-native deps remaining), and before ->run_job() gets called, there might be several of these native deps that get signaled, and we're still considering job->submission_credits as unchanged, when each of these signalled fence could have reduced the job credits, potentially allowing it to be submitted sooner. > > So to me it looks like when prepare_job() is called because it is > selected as next job for submission you should already know how many > credits it needs. You know how many credits it needs when ->prepare_job() is called, but if this number is too high, the entity will not be picked, and next time it's checked, you'll still consider the job credits at the time ->prepare_job() was called, which might differ from the current job credits (signalled native fences might have been signalled in the meantime, and they could be evicted). > > >> What you can do is to look at the credits of a job *after* it was picked > >> up for scheduling so that you can scheduler more jobs. > > Sure, but then you might further delay your job if something made it > > smaller (ie. native fences got signaled) between ->prepare_job() and > > drm_sched_can_queue(). And any new drm_sched_can_queue() test would > > just see the old credits value. > > > > Out of curiosity, what are you worried about with this optional > > ->update_job_credits() call in the drm_sched_can_queue() path? Is the > > if (sched->update_job_credits) overhead considered too high for drivers > > that don't need it? > > Since the dma_fences are also used for resource management the
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Am 27.10.23 um 10:22 schrieb Boris Brezillon: On Fri, 27 Oct 2023 09:44:13 +0200 Christian König wrote: Am 27.10.23 um 09:39 schrieb Boris Brezillon: On Fri, 27 Oct 2023 09:35:01 +0200 Christian König wrote: Am 27.10.23 um 09:32 schrieb Boris Brezillon: On Fri, 27 Oct 2023 09:22:12 +0200 Christian König wrote: + + /** +* @update_job_credits: Called once the scheduler is considering this +* job for execution. +* +* Drivers may use this to update the job's submission credits, which is +* useful to e.g. deduct the number of native fences which have been +* signaled meanwhile. +* +* The callback must either return the new number of submission credits +* for the given job, or zero if no update is required. +* +* This callback is optional. +*/ + u32 (*update_job_credits)(struct drm_sched_job *sched_job); Why do we need an extra callback for this? Just document that prepare_job() is allowed to reduce the number of credits the job might need. ->prepare_job() is called only once if the returned fence is NULL, but we need this credit-update to happen every time a job is considered for execution by the scheduler. But the job is only considered for execution once. How do you see that this is called multiple times? Nope, it's not. If drm_sched_can_queue() returns false, the scheduler will go look for another entity that has a job ready for execution, and get back to this entity later, and test drm_sched_can_queue() again. Basically, any time drm_sched_can_queue() is called, the job credits update should happen, so we have an accurate view of how many credits this job needs. Well, that is the handling which I already rejected because it creates unfairness between processes. When you consider the credits needed *before* scheduling jobs with a lower credit count are always preferred over jobs with a higher credit count. My bad, it doesn't pick another entity when an entity with a ready job that doesn't fit the queue is found, it just bails out from drm_sched_rq_select_entity_rr() and returns NULL (AKA: no ready entity found). But we still want to update the job credits before checking if the job fits or not (next time this entity is tested). Why? I only see a few possibility here: 1. You need to wait for submissions to the same scheduler to finish before the one you want to push to the ring. This case can be avoided by trying to cast the dependency fences to drm_sched_fences and looking if they are already scheduled. 2. You need to wait for submissions to a different scheduler instance and in this case you should probably return that as dependency instead. So to me it looks like when prepare_job() is called because it is selected as next job for submission you should already know how many credits it needs. What you can do is to look at the credits of a job *after* it was picked up for scheduling so that you can scheduler more jobs. Sure, but then you might further delay your job if something made it smaller (ie. native fences got signaled) between ->prepare_job() and drm_sched_can_queue(). And any new drm_sched_can_queue() test would just see the old credits value. Out of curiosity, what are you worried about with this optional ->update_job_credits() call in the drm_sched_can_queue() path? Is the if (sched->update_job_credits) overhead considered too high for drivers that don't need it? Since the dma_fences are also used for resource management the scheduler is vital for correct system operation. We had massively problems because people tried to over-optimize the dma_fence handling which lead to very hard to narrow down memory corruptions. So for every change you come up here you need to have a very very good justification. And just saving a bit size of your ring buffer is certainly not one of them. Regards, Christian.
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Hi Danilo, On Thu, 26 Oct 2023 18:13:00 +0200 Danilo Krummrich wrote: > Currently, job flow control is implemented simply by limiting the number > of jobs in flight. Therefore, a scheduler is initialized with a credit > limit that corresponds to the number of jobs which can be sent to the > hardware. > > This implies that for each job, drivers need to account for the maximum > job size possible in order to not overflow the ring buffer. > > However, there are drivers, such as Nouveau, where the job size has a > rather large range. For such drivers it can easily happen that job > submissions not even filling the ring by 1% can block subsequent > submissions, which, in the worst case, can lead to the ring run dry. > > In order to overcome this issue, allow for tracking the actual job size > instead of the number of jobs. Therefore, add a field to track a job's > credit count, which represents the number of credits a job contributes > to the scheduler's credit limit. > > Signed-off-by: Danilo Krummrich > --- > Changes in V2: > == > - fixed up influence on scheduling fairness due to consideration of a job's > size > - If we reach a ready entity in drm_sched_select_entity() but can't > actually > queue a job from it due to size limitations, just give up and go to > sleep > until woken up due to a pending job finishing, rather than continue to > try > other entities. > - added a callback to dynamically update a job's credits (Boris) This callback seems controversial. I'd suggest dropping it, so the patch can be merged. Regards, Boris > - renamed 'units' to 'credits' > - fixed commit message and comments as requested by Luben > > Changes in V3: > == > - rebased onto V7 of the "DRM scheduler changes for Xe" series by Matt > - move up drm_sched_can_queue() instead of adding a forward declaration > (Boris) > - add a drm_sched_available_credits() helper (Boris) > - adjust control flow in drm_sched_rq_select_entity_fifo() to Luben's > proposal > - re-phrase a few comments and fix a typo (Luben) > - change naming of all structures credit fields and function parameters to > the > following scheme > - drm_sched_job::credits > - drm_gpu_scheduler::credit_limit > - drm_gpu_scheduler::credit_count > - drm_sched_init(..., u32 credit_limit, ...) > - drm_sched_job_init(..., u32 credits, ...) > - add proper documentation for the scheduler's job-flow control mechanism
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On Fri, 27 Oct 2023 09:44:13 +0200 Christian König wrote: > Am 27.10.23 um 09:39 schrieb Boris Brezillon: > > On Fri, 27 Oct 2023 09:35:01 +0200 > > Christian König wrote: > > > >> Am 27.10.23 um 09:32 schrieb Boris Brezillon: > >>> On Fri, 27 Oct 2023 09:22:12 +0200 > >>> Christian König wrote: > >>> > > + > > + /** > > +* @update_job_credits: Called once the scheduler is > > considering this > > +* job for execution. > > +* > > +* Drivers may use this to update the job's submission credits, > > which is > > +* useful to e.g. deduct the number of native fences which have > > been > > +* signaled meanwhile. > > +* > > +* The callback must either return the new number of submission > > credits > > +* for the given job, or zero if no update is required. > > +* > > +* This callback is optional. > > +*/ > > + u32 (*update_job_credits)(struct drm_sched_job *sched_job); > Why do we need an extra callback for this? > > Just document that prepare_job() is allowed to reduce the number of > credits the job might need. > >>> ->prepare_job() is called only once if the returned fence is NULL, but > >>> we need this credit-update to happen every time a job is considered for > >>> execution by the scheduler. > >> But the job is only considered for execution once. How do you see that > >> this is called multiple times? > > Nope, it's not. If drm_sched_can_queue() returns false, the scheduler > > will go look for another entity that has a job ready for execution, and > > get back to this entity later, and test drm_sched_can_queue() again. > > Basically, any time drm_sched_can_queue() is called, the job credits > > update should happen, so we have an accurate view of how many credits > > this job needs. > > Well, that is the handling which I already rejected because it creates > unfairness between processes. When you consider the credits needed > *before* scheduling jobs with a lower credit count are always preferred > over jobs with a higher credit count. My bad, it doesn't pick another entity when an entity with a ready job that doesn't fit the queue is found, it just bails out from drm_sched_rq_select_entity_rr() and returns NULL (AKA: no ready entity found). But we still want to update the job credits before checking if the job fits or not (next time this entity is tested). > What you can do is to look at the credits of a job *after* it was picked > up for scheduling so that you can scheduler more jobs. Sure, but then you might further delay your job if something made it smaller (ie. native fences got signaled) between ->prepare_job() and drm_sched_can_queue(). And any new drm_sched_can_queue() test would just see the old credits value. Out of curiosity, what are you worried about with this optional ->update_job_credits() call in the drm_sched_can_queue() path? Is the if (sched->update_job_credits) overhead considered too high for drivers that don't need it?
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Am 27.10.23 um 09:39 schrieb Boris Brezillon: On Fri, 27 Oct 2023 09:35:01 +0200 Christian König wrote: Am 27.10.23 um 09:32 schrieb Boris Brezillon: On Fri, 27 Oct 2023 09:22:12 +0200 Christian König wrote: + + /** +* @update_job_credits: Called once the scheduler is considering this +* job for execution. +* +* Drivers may use this to update the job's submission credits, which is +* useful to e.g. deduct the number of native fences which have been +* signaled meanwhile. +* +* The callback must either return the new number of submission credits +* for the given job, or zero if no update is required. +* +* This callback is optional. +*/ + u32 (*update_job_credits)(struct drm_sched_job *sched_job); Why do we need an extra callback for this? Just document that prepare_job() is allowed to reduce the number of credits the job might need. ->prepare_job() is called only once if the returned fence is NULL, but we need this credit-update to happen every time a job is considered for execution by the scheduler. But the job is only considered for execution once. How do you see that this is called multiple times? Nope, it's not. If drm_sched_can_queue() returns false, the scheduler will go look for another entity that has a job ready for execution, and get back to this entity later, and test drm_sched_can_queue() again. Basically, any time drm_sched_can_queue() is called, the job credits update should happen, so we have an accurate view of how many credits this job needs. Well, that is the handling which I already rejected because it creates unfairness between processes. When you consider the credits needed *before* scheduling jobs with a lower credit count are always preferred over jobs with a higher credit count. What you can do is to look at the credits of a job *after* it was picked up for scheduling so that you can scheduler more jobs. Regards, Christian.
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On Fri, 27 Oct 2023 09:35:01 +0200 Christian König wrote: > Am 27.10.23 um 09:32 schrieb Boris Brezillon: > > On Fri, 27 Oct 2023 09:22:12 +0200 > > Christian König wrote: > > > >>> + > >>> + /** > >>> + * @update_job_credits: Called once the scheduler is considering this > >>> + * job for execution. > >>> + * > >>> + * Drivers may use this to update the job's submission credits, which is > >>> + * useful to e.g. deduct the number of native fences which have been > >>> + * signaled meanwhile. > >>> + * > >>> + * The callback must either return the new number of submission credits > >>> + * for the given job, or zero if no update is required. > >>> + * > >>> + * This callback is optional. > >>> + */ > >>> + u32 (*update_job_credits)(struct drm_sched_job *sched_job); > >> Why do we need an extra callback for this? > >> > >> Just document that prepare_job() is allowed to reduce the number of > >> credits the job might need. > > ->prepare_job() is called only once if the returned fence is NULL, but > > we need this credit-update to happen every time a job is considered for > > execution by the scheduler. > > But the job is only considered for execution once. How do you see that > this is called multiple times? Nope, it's not. If drm_sched_can_queue() returns false, the scheduler will go look for another entity that has a job ready for execution, and get back to this entity later, and test drm_sched_can_queue() again. Basically, any time drm_sched_can_queue() is called, the job credits update should happen, so we have an accurate view of how many credits this job needs.
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Am 27.10.23 um 09:32 schrieb Boris Brezillon: On Fri, 27 Oct 2023 09:22:12 +0200 Christian König wrote: + + /** +* @update_job_credits: Called once the scheduler is considering this +* job for execution. +* +* Drivers may use this to update the job's submission credits, which is +* useful to e.g. deduct the number of native fences which have been +* signaled meanwhile. +* +* The callback must either return the new number of submission credits +* for the given job, or zero if no update is required. +* +* This callback is optional. +*/ + u32 (*update_job_credits)(struct drm_sched_job *sched_job); Why do we need an extra callback for this? Just document that prepare_job() is allowed to reduce the number of credits the job might need. ->prepare_job() is called only once if the returned fence is NULL, but we need this credit-update to happen every time a job is considered for execution by the scheduler. But the job is only considered for execution once. How do you see that this is called multiple times? Regards, Christian. If you're saying this control-flow should be implemented with a dma_fence that's signaled when enough space is available, I fear Danilo's work won't be that useful to the PowerVR driver, unfortunately.
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On Fri, 27 Oct 2023 09:22:12 +0200 Christian König wrote: > > + > > + /** > > +* @update_job_credits: Called once the scheduler is considering this > > +* job for execution. > > +* > > +* Drivers may use this to update the job's submission credits, which is > > +* useful to e.g. deduct the number of native fences which have been > > +* signaled meanwhile. > > +* > > +* The callback must either return the new number of submission credits > > +* for the given job, or zero if no update is required. > > +* > > +* This callback is optional. > > +*/ > > + u32 (*update_job_credits)(struct drm_sched_job *sched_job); > > Why do we need an extra callback for this? > > Just document that prepare_job() is allowed to reduce the number of > credits the job might need. ->prepare_job() is called only once if the returned fence is NULL, but we need this credit-update to happen every time a job is considered for execution by the scheduler. If you're saying this control-flow should be implemented with a dma_fence that's signaled when enough space is available, I fear Danilo's work won't be that useful to the PowerVR driver, unfortunately.
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Am 26.10.23 um 18:13 schrieb Danilo Krummrich: Currently, job flow control is implemented simply by limiting the number of jobs in flight. Therefore, a scheduler is initialized with a credit limit that corresponds to the number of jobs which can be sent to the hardware. This implies that for each job, drivers need to account for the maximum job size possible in order to not overflow the ring buffer. However, there are drivers, such as Nouveau, where the job size has a rather large range. For such drivers it can easily happen that job submissions not even filling the ring by 1% can block subsequent submissions, which, in the worst case, can lead to the ring run dry. In order to overcome this issue, allow for tracking the actual job size instead of the number of jobs. Therefore, add a field to track a job's credit count, which represents the number of credits a job contributes to the scheduler's credit limit. Signed-off-by: Danilo Krummrich --- Changes in V2: == - fixed up influence on scheduling fairness due to consideration of a job's size - If we reach a ready entity in drm_sched_select_entity() but can't actually queue a job from it due to size limitations, just give up and go to sleep until woken up due to a pending job finishing, rather than continue to try other entities. - added a callback to dynamically update a job's credits (Boris) - renamed 'units' to 'credits' - fixed commit message and comments as requested by Luben Changes in V3: == - rebased onto V7 of the "DRM scheduler changes for Xe" series by Matt - move up drm_sched_can_queue() instead of adding a forward declaration (Boris) - add a drm_sched_available_credits() helper (Boris) - adjust control flow in drm_sched_rq_select_entity_fifo() to Luben's proposal - re-phrase a few comments and fix a typo (Luben) - change naming of all structures credit fields and function parameters to the following scheme - drm_sched_job::credits - drm_gpu_scheduler::credit_limit - drm_gpu_scheduler::credit_count - drm_sched_init(..., u32 credit_limit, ...) - drm_sched_job_init(..., u32 credits, ...) - add proper documentation for the scheduler's job-flow control mechanism This patch is based on V7 of the "DRM scheduler changes for Xe" series. [1] provides a branch based on drm-misc-next, with the named series and this patch on top of it. [1] https://gitlab.freedesktop.org/nouvelles/kernel/-/commits/sched-credits/ --- Documentation/gpu/drm-mm.rst | 6 + drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +- drivers/gpu/drm/lima/lima_sched.c | 2 +- drivers/gpu/drm/msm/msm_gem_submit.c | 2 +- drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +- .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +- drivers/gpu/drm/scheduler/sched_entity.c | 4 +- drivers/gpu/drm/scheduler/sched_main.c| 142 ++ drivers/gpu/drm/v3d/v3d_gem.c | 2 +- include/drm/gpu_scheduler.h | 33 +++- 12 files changed, 152 insertions(+), 49 deletions(-) diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst index 602010cb6894..acc5901ac840 100644 --- a/Documentation/gpu/drm-mm.rst +++ b/Documentation/gpu/drm-mm.rst @@ -552,6 +552,12 @@ Overview .. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c :doc: Overview +Flow Control + + +.. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c + :doc: Flow Control + Scheduler Function References - diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c index 1f357198533f..62bb7fc7448a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (!entity) return 0; - return drm_sched_job_init(&(*job)->base, entity, owner); + return drm_sched_job_init(&(*job)->base, entity, 1, owner); } int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev, diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 2416c526f9b0..3d0f8d182506 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -535,7 +535,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, ret = drm_sched_job_init(>sched_job, >sched_entity[args->pipe], -submit->ctx); +1, submit->ctx); if (ret) goto err_submit_put; diff --git a/drivers/gpu/drm/lima/lima_sched.c
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
Hi Danilo, On Thu, 26 Oct 2023 18:13:00 +0200 Danilo Krummrich wrote: > + > + /** > + * @update_job_credits: Called once the scheduler is considering this > + * job for execution. > + * > + * Drivers may use this to update the job's submission credits, which is > + * useful to e.g. deduct the number of native fences which have been > + * signaled meanwhile. > + * > + * The callback must either return the new number of submission credits > + * for the given job, or zero if no update is required. > + * > + * This callback is optional. > + */ > + u32 (*update_job_credits)(struct drm_sched_job *sched_job); I'm copying my late reply to v2 here so it doesn't get lost: I keep thinking it'd be simpler to make this a void function that updates s_job->submission_credits directly. I also don't see the problem with doing a sanity check on job->submission_credits. I mean, if the driver is doing something silly, you can't do much to prevent it anyway, except warn the user that something wrong has happened. If you want to WARN_ON(job->submission_credits == 0 || job->submission_credits > job_old_submission_credits); that's fine. But none of this sanity checking has to do with the function prototype/semantics, and I'm still not comfortable with this 0 => no-change. If there's no change, we should just leave job->submission_credits unchanged (or return job->submission_credits) instead of inventing a new special case. Regards, Boris
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On 2023-10-26 17:13, Luben Tuikov wrote: > On 2023-10-26 12:13, Danilo Krummrich wrote: >> Currently, job flow control is implemented simply by limiting the number >> of jobs in flight. Therefore, a scheduler is initialized with a credit >> limit that corresponds to the number of jobs which can be sent to the >> hardware. >> >> This implies that for each job, drivers need to account for the maximum >> job size possible in order to not overflow the ring buffer. >> >> However, there are drivers, such as Nouveau, where the job size has a >> rather large range. For such drivers it can easily happen that job >> submissions not even filling the ring by 1% can block subsequent >> submissions, which, in the worst case, can lead to the ring run dry. >> >> In order to overcome this issue, allow for tracking the actual job size >> instead of the number of jobs. Therefore, add a field to track a job's >> credit count, which represents the number of credits a job contributes >> to the scheduler's credit limit. >> >> Signed-off-by: Danilo Krummrich >> --- >> Changes in V2: >> == >> - fixed up influence on scheduling fairness due to consideration of a job's >> size >> - If we reach a ready entity in drm_sched_select_entity() but can't >> actually >> queue a job from it due to size limitations, just give up and go to >> sleep >> until woken up due to a pending job finishing, rather than continue to >> try >> other entities. >> - added a callback to dynamically update a job's credits (Boris) >> - renamed 'units' to 'credits' >> - fixed commit message and comments as requested by Luben >> >> Changes in V3: >> == >> - rebased onto V7 of the "DRM scheduler changes for Xe" series by Matt >> - move up drm_sched_can_queue() instead of adding a forward declaration >> (Boris) >> - add a drm_sched_available_credits() helper (Boris) >> - adjust control flow in drm_sched_rq_select_entity_fifo() to Luben's >> proposal >> - re-phrase a few comments and fix a typo (Luben) >> - change naming of all structures credit fields and function parameters to >> the >> following scheme >> - drm_sched_job::credits >> - drm_gpu_scheduler::credit_limit >> - drm_gpu_scheduler::credit_count >> - drm_sched_init(..., u32 credit_limit, ...) >> - drm_sched_job_init(..., u32 credits, ...) >> - add proper documentation for the scheduler's job-flow control mechanism >> >> This patch is based on V7 of the "DRM scheduler changes for Xe" series. [1] >> provides a branch based on drm-misc-next, with the named series and this >> patch >> on top of it. >> >> [1] https://gitlab.freedesktop.org/nouvelles/kernel/-/commits/sched-credits/ >> --- >> Documentation/gpu/drm-mm.rst | 6 + >> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +- >> drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +- >> drivers/gpu/drm/lima/lima_sched.c | 2 +- >> drivers/gpu/drm/msm/msm_gem_submit.c | 2 +- >> drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +- >> drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +- >> .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +- >> drivers/gpu/drm/scheduler/sched_entity.c | 4 +- >> drivers/gpu/drm/scheduler/sched_main.c| 142 ++ >> drivers/gpu/drm/v3d/v3d_gem.c | 2 +- >> include/drm/gpu_scheduler.h | 33 +++- >> 12 files changed, 152 insertions(+), 49 deletions(-) >> >> diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst >> index 602010cb6894..acc5901ac840 100644 >> --- a/Documentation/gpu/drm-mm.rst >> +++ b/Documentation/gpu/drm-mm.rst >> @@ -552,6 +552,12 @@ Overview >> .. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c >> :doc: Overview >> >> +Flow Control >> + >> + >> +.. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c >> + :doc: Flow Control >> + >> Scheduler Function References >> - >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c >> index 1f357198533f..62bb7fc7448a 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c >> @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct >> amdgpu_vm *vm, >> if (!entity) >> return 0; >> >> -return drm_sched_job_init(&(*job)->base, entity, owner); >> +return drm_sched_job_init(&(*job)->base, entity, 1, owner); >> } >> >> int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev, >> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c >> b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c >> index 2416c526f9b0..3d0f8d182506 100644 >> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c >> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c >> @@ -535,7 +535,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, >> void *data, >> >>
Re: [PATCH drm-misc-next v3] drm/sched: implement dynamic job-flow control
On 2023-10-26 12:13, Danilo Krummrich wrote: > Currently, job flow control is implemented simply by limiting the number > of jobs in flight. Therefore, a scheduler is initialized with a credit > limit that corresponds to the number of jobs which can be sent to the > hardware. > > This implies that for each job, drivers need to account for the maximum > job size possible in order to not overflow the ring buffer. > > However, there are drivers, such as Nouveau, where the job size has a > rather large range. For such drivers it can easily happen that job > submissions not even filling the ring by 1% can block subsequent > submissions, which, in the worst case, can lead to the ring run dry. > > In order to overcome this issue, allow for tracking the actual job size > instead of the number of jobs. Therefore, add a field to track a job's > credit count, which represents the number of credits a job contributes > to the scheduler's credit limit. > > Signed-off-by: Danilo Krummrich > --- > Changes in V2: > == > - fixed up influence on scheduling fairness due to consideration of a job's > size > - If we reach a ready entity in drm_sched_select_entity() but can't > actually > queue a job from it due to size limitations, just give up and go to > sleep > until woken up due to a pending job finishing, rather than continue to > try > other entities. > - added a callback to dynamically update a job's credits (Boris) > - renamed 'units' to 'credits' > - fixed commit message and comments as requested by Luben > > Changes in V3: > == > - rebased onto V7 of the "DRM scheduler changes for Xe" series by Matt > - move up drm_sched_can_queue() instead of adding a forward declaration > (Boris) > - add a drm_sched_available_credits() helper (Boris) > - adjust control flow in drm_sched_rq_select_entity_fifo() to Luben's > proposal > - re-phrase a few comments and fix a typo (Luben) > - change naming of all structures credit fields and function parameters to > the > following scheme > - drm_sched_job::credits > - drm_gpu_scheduler::credit_limit > - drm_gpu_scheduler::credit_count > - drm_sched_init(..., u32 credit_limit, ...) > - drm_sched_job_init(..., u32 credits, ...) > - add proper documentation for the scheduler's job-flow control mechanism > > This patch is based on V7 of the "DRM scheduler changes for Xe" series. [1] > provides a branch based on drm-misc-next, with the named series and this patch > on top of it. > > [1] https://gitlab.freedesktop.org/nouvelles/kernel/-/commits/sched-credits/ > --- > Documentation/gpu/drm-mm.rst | 6 + > drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +- > drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +- > drivers/gpu/drm/lima/lima_sched.c | 2 +- > drivers/gpu/drm/msm/msm_gem_submit.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_sched.c | 2 +- > drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +- > .../gpu/drm/scheduler/gpu_scheduler_trace.h | 2 +- > drivers/gpu/drm/scheduler/sched_entity.c | 4 +- > drivers/gpu/drm/scheduler/sched_main.c| 142 ++ > drivers/gpu/drm/v3d/v3d_gem.c | 2 +- > include/drm/gpu_scheduler.h | 33 +++- > 12 files changed, 152 insertions(+), 49 deletions(-) > > diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst > index 602010cb6894..acc5901ac840 100644 > --- a/Documentation/gpu/drm-mm.rst > +++ b/Documentation/gpu/drm-mm.rst > @@ -552,6 +552,12 @@ Overview > .. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c > :doc: Overview > > +Flow Control > + > + > +.. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c > + :doc: Flow Control > + > Scheduler Function References > - > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c > index 1f357198533f..62bb7fc7448a 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c > @@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct > amdgpu_vm *vm, > if (!entity) > return 0; > > - return drm_sched_job_init(&(*job)->base, entity, owner); > + return drm_sched_job_init(&(*job)->base, entity, 1, owner); > } > > int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev, > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > index 2416c526f9b0..3d0f8d182506 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > @@ -535,7 +535,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void > *data, > > ret = drm_sched_job_init(>sched_job, >>sched_entity[args->pipe], > - submit->ctx);
