Re: [PATCH 00/47] GuC submission support
Quoting Matthew Brost (2021-10-26 18:51:17) > On Tue, Oct 26, 2021 at 11:59:35AM +0300, Joonas Lahtinen wrote: > > Quoting Matthew Brost (2021-10-25 18:15:09) > > > On Mon, Oct 25, 2021 at 12:37:02PM +0300, Joonas Lahtinen wrote: > > > > Quoting Matthew Brost (2021-10-22 19:42:19) > > > > > On Fri, Oct 22, 2021 at 12:35:04PM +0300, Joonas Lahtinen wrote: > > > > > > Hi Matt & John, > > > > > > > > > > > > Can you please queue patches with the right Fixes: references to > > > > > > convert > > > > > > all the GuC tracepoints to be protected by the LOW_LEVEL_TRACEPOINTS > > > > > > protection for now. Please do so before next Wednesday so we get it > > > > > > queued in drm-intel-next-fixes. > > > > > > > > > > > > > > > > Don't we already do that? I checked i915_trace.h and every tracepoint > > > > > I > > > > > added (intel_context class, i915_request_guc_submit) is protected by > > > > > LOW_LEVEL_TRACEPOINTS. > > > > > > > > > > The only thing I changed outside of that protection is adding the > > > > > guc_id > > > > > field to existing i915_request class tracepoints. > > > > > > > > It's the first search hit for "guc" inside the i915_trace.h file :) > > > > > > > > > Without the guc_id in > > > > > those tracepoints these are basically useless with GuC submission. We > > > > > could revert that if it is a huge deal but as I said then they are > > > > > useless... > > > > > > > > Let's eliminate it for now and restore the tracepoint exactly as it was. > > > > > > > > > > Don't really agree - let's render tracepoints to be useless? Are > > > tracepoints ABI? I googled this and couldn't really find a definie > > > answer. If tracepoints are ABI, then OK I can revert this change but > > > still this is a poor technical decision (tracepoints should not be ABI). > > > > Thats a very heated discussion in general. But the fact is that if > > tracepoint changes have caused regressions to applications, they have > > been forced to be remain untouched. You are free to raise the discussion > > with Linus/LKML if you feel that should not be the case. So the end > > result is that tracepoints are effectively in limbo, not ABI unless some > > application uses them like ABI. > > > > Feel free to search the intel-gfx/lkml for "tracepoints" keyword and look > > for threads with many replies. It's not that I would not agree, it's more > > that I'm not in the mood for repeating that discussion over and over again > > and always land in the same spot. > > > > So for now, we don't add anything new to tracepoints we can't guarantee > > to always be there untouched. Similarly, we don't guarantee any of them > > to remain stable. So we try to be compatible with the limbo. > > > > I'm long overdue waiting for some stable consumer to step up for the > > tracepoints, so we can then start discussion what would actually be the > > best way of getting that information out for them. In ~5 years that has > > not happened. > > > > > > If there is an immediate need, we should instead have an auxilary > > > > tracepoint > > > > which is enabled only through LOW_LEVEL_TRACEPOINTS and that amends the > > > > information of the basic tracepoint. > > > > > > > > > > Regardless of what I said above, I'll post 2 patches. The 1st just > > > remove the GuC, the 2nd modify the tracepoint to include guc_id if > > > LOW_LEVEL_TRACEPOINTS is defined. > > > > Thanks. Let's get a patch merged which simply drops the guc_id for now > > to unblock things. > > > > For the second, an auxilary tracepoint will be preferred instead of > > mutating the existing one (regardless of the LOW_LEVEL_TRACEPOINTS). > > > > I only noticed a patch that mutates the tracepoints, can you > > double-check sending the first patch? > > Sorry for the double reply - missed this one in the first. > > I changed my plans / mind after I send the original email. I only sent a > patch which includes guc_id when LOW_LEVEL_TRACEPOINTS is enabled. That > is the bear minimum I live with. Without it any time there is a problem > results in hacking the kernel. I can't do that. This is a good > compromise. When it comes to fixing a regression, it should be done with the minimal revert/change with "Fixes:" as suggested originally. Then we can leave the discussion for how to best cover the gap you pointed out, to be resolved in the second patch. There are clearly at least two ways to approach it, either mutate the original tracepoint or add an auxilary tracepoint to amend the information. So we should have a quick discussion between the involved parties which is a better approach. We should not fix the regression in a patch where we also initiate a change in behavior. That'll make bisecting and backporting patches a pain. So even if the patches would be merged back-to-back to the tree at the same time, they should be different patches. And in this case it seems that the latter patch should have some discussion to reach a rough consensus and that should not delay t
Re: [PATCH 00/47] GuC submission support
On Tue, Oct 26, 2021 at 11:59:35AM +0300, Joonas Lahtinen wrote: > Quoting Matthew Brost (2021-10-25 18:15:09) > > On Mon, Oct 25, 2021 at 12:37:02PM +0300, Joonas Lahtinen wrote: > > > Quoting Matthew Brost (2021-10-22 19:42:19) > > > > On Fri, Oct 22, 2021 at 12:35:04PM +0300, Joonas Lahtinen wrote: > > > > > Hi Matt & John, > > > > > > > > > > Can you please queue patches with the right Fixes: references to > > > > > convert > > > > > all the GuC tracepoints to be protected by the LOW_LEVEL_TRACEPOINTS > > > > > protection for now. Please do so before next Wednesday so we get it > > > > > queued in drm-intel-next-fixes. > > > > > > > > > > > > > Don't we already do that? I checked i915_trace.h and every tracepoint I > > > > added (intel_context class, i915_request_guc_submit) is protected by > > > > LOW_LEVEL_TRACEPOINTS. > > > > > > > > The only thing I changed outside of that protection is adding the guc_id > > > > field to existing i915_request class tracepoints. > > > > > > It's the first search hit for "guc" inside the i915_trace.h file :) > > > > > > > Without the guc_id in > > > > those tracepoints these are basically useless with GuC submission. We > > > > could revert that if it is a huge deal but as I said then they are > > > > useless... > > > > > > Let's eliminate it for now and restore the tracepoint exactly as it was. > > > > > > > Don't really agree - let's render tracepoints to be useless? Are > > tracepoints ABI? I googled this and couldn't really find a definie > > answer. If tracepoints are ABI, then OK I can revert this change but > > still this is a poor technical decision (tracepoints should not be ABI). > > Thats a very heated discussion in general. But the fact is that if > tracepoint changes have caused regressions to applications, they have > been forced to be remain untouched. You are free to raise the discussion > with Linus/LKML if you feel that should not be the case. So the end > result is that tracepoints are effectively in limbo, not ABI unless some > application uses them like ABI. > > Feel free to search the intel-gfx/lkml for "tracepoints" keyword and look > for threads with many replies. It's not that I would not agree, it's more > that I'm not in the mood for repeating that discussion over and over again > and always land in the same spot. > > So for now, we don't add anything new to tracepoints we can't guarantee > to always be there untouched. Similarly, we don't guarantee any of them > to remain stable. So we try to be compatible with the limbo. > > I'm long overdue waiting for some stable consumer to step up for the > tracepoints, so we can then start discussion what would actually be the > best way of getting that information out for them. In ~5 years that has > not happened. > > > > If there is an immediate need, we should instead have an auxilary > > > tracepoint > > > which is enabled only through LOW_LEVEL_TRACEPOINTS and that amends the > > > information of the basic tracepoint. > > > > > > > Regardless of what I said above, I'll post 2 patches. The 1st just > > remove the GuC, the 2nd modify the tracepoint to include guc_id if > > LOW_LEVEL_TRACEPOINTS is defined. > > Thanks. Let's get a patch merged which simply drops the guc_id for now > to unblock things. > > For the second, an auxilary tracepoint will be preferred instead of > mutating the existing one (regardless of the LOW_LEVEL_TRACEPOINTS). > > I only noticed a patch that mutates the tracepoints, can you > double-check sending the first patch? Sorry for the double reply - missed this one in the first. I changed my plans / mind after I send the original email. I only sent a patch which includes guc_id when LOW_LEVEL_TRACEPOINTS is enabled. That is the bear minimum I live with. Without it any time there is a problem results in hacking the kernel. I can't do that. This is a good compromise. Matt > > Regards, Joonas > > > > > > For the longer term solution we should align towards the dma fence > > > tracepoints. When those are combined with the OA information, one should > > > be able to get a good understanding of both the software and hardware > > > scheduling decisions. > > > > > > > Not sure about this either. I use these tracepoins to correlate things > > to the GuC log. Between the 2, if you know what you are doing you > > basically can figure out everything that is happening. Fields in the > > trace translate directly to fields in the GuC log. Some of these fields > > are backend specific, not sure how these could be pushed the dma fence > > tracepoints. For what it is worth, without these tracepoints we'd likely > > still have a bunch of bugs in the GuC firmware. I understand these > > points, several other i915 developers do, and several of the GuC > > firmware developers do too. > > > > Matt > > > > > Regards, Joonas > > > > > > > > > > > Matt > > > > > > > > > There's the orthogonal track to discuss what would be the stable set > > > > > of > > > > > trac
Re: [PATCH 00/47] GuC submission support
On Tue, Oct 26, 2021 at 11:59:35AM +0300, Joonas Lahtinen wrote: > Quoting Matthew Brost (2021-10-25 18:15:09) > > On Mon, Oct 25, 2021 at 12:37:02PM +0300, Joonas Lahtinen wrote: > > > Quoting Matthew Brost (2021-10-22 19:42:19) > > > > On Fri, Oct 22, 2021 at 12:35:04PM +0300, Joonas Lahtinen wrote: > > > > > Hi Matt & John, > > > > > > > > > > Can you please queue patches with the right Fixes: references to > > > > > convert > > > > > all the GuC tracepoints to be protected by the LOW_LEVEL_TRACEPOINTS > > > > > protection for now. Please do so before next Wednesday so we get it > > > > > queued in drm-intel-next-fixes. > > > > > > > > > > > > > Don't we already do that? I checked i915_trace.h and every tracepoint I > > > > added (intel_context class, i915_request_guc_submit) is protected by > > > > LOW_LEVEL_TRACEPOINTS. > > > > > > > > The only thing I changed outside of that protection is adding the guc_id > > > > field to existing i915_request class tracepoints. > > > > > > It's the first search hit for "guc" inside the i915_trace.h file :) > > > > > > > Without the guc_id in > > > > those tracepoints these are basically useless with GuC submission. We > > > > could revert that if it is a huge deal but as I said then they are > > > > useless... > > > > > > Let's eliminate it for now and restore the tracepoint exactly as it was. > > > > > > > Don't really agree - let's render tracepoints to be useless? Are > > tracepoints ABI? I googled this and couldn't really find a definie > > answer. If tracepoints are ABI, then OK I can revert this change but > > still this is a poor technical decision (tracepoints should not be ABI). > > Thats a very heated discussion in general. But the fact is that if > tracepoint changes have caused regressions to applications, they have > been forced to be remain untouched. You are free to raise the discussion > with Linus/LKML if you feel that should not be the case. So the end > result is that tracepoints are effectively in limbo, not ABI unless some > application uses them like ABI. > Not trying to start or fight a holy war. If the current rules are don't change tracepoints, we won't. Patch posted, let's stay focused, get an RB, and move on. Matt > Feel free to search the intel-gfx/lkml for "tracepoints" keyword and look > for threads with many replies. It's not that I would not agree, it's more > that I'm not in the mood for repeating that discussion over and over again > and always land in the same spot. > > So for now, we don't add anything new to tracepoints we can't guarantee > to always be there untouched. Similarly, we don't guarantee any of them > to remain stable. So we try to be compatible with the limbo. > > I'm long overdue waiting for some stable consumer to step up for the > tracepoints, so we can then start discussion what would actually be the > best way of getting that information out for them. In ~5 years that has > not happened. > > > > If there is an immediate need, we should instead have an auxilary > > > tracepoint > > > which is enabled only through LOW_LEVEL_TRACEPOINTS and that amends the > > > information of the basic tracepoint. > > > > > > > Regardless of what I said above, I'll post 2 patches. The 1st just > > remove the GuC, the 2nd modify the tracepoint to include guc_id if > > LOW_LEVEL_TRACEPOINTS is defined. > > Thanks. Let's get a patch merged which simply drops the guc_id for now > to unblock things. > > For the second, an auxilary tracepoint will be preferred instead of > mutating the existing one (regardless of the LOW_LEVEL_TRACEPOINTS). > > I only noticed a patch that mutates the tracepoints, can you > double-check sending the first patch? > > Regards, Joonas > > > > > > For the longer term solution we should align towards the dma fence > > > tracepoints. When those are combined with the OA information, one should > > > be able to get a good understanding of both the software and hardware > > > scheduling decisions. > > > > > > > Not sure about this either. I use these tracepoins to correlate things > > to the GuC log. Between the 2, if you know what you are doing you > > basically can figure out everything that is happening. Fields in the > > trace translate directly to fields in the GuC log. Some of these fields > > are backend specific, not sure how these could be pushed the dma fence > > tracepoints. For what it is worth, without these tracepoints we'd likely > > still have a bunch of bugs in the GuC firmware. I understand these > > points, several other i915 developers do, and several of the GuC > > firmware developers do too. > > > > Matt > > > > > Regards, Joonas > > > > > > > > > > > Matt > > > > > > > > > There's the orthogonal track to discuss what would be the stable set > > > > > of > > > > > tracepoints we could expose. However, before that discussion is > > > > > closed, > > > > > let's keep a rather strict line to avoid potential maintenance burned. > > > > > > > > > > We can then
Re: [PATCH 00/47] GuC submission support
Quoting Matthew Brost (2021-10-25 18:15:09) > On Mon, Oct 25, 2021 at 12:37:02PM +0300, Joonas Lahtinen wrote: > > Quoting Matthew Brost (2021-10-22 19:42:19) > > > On Fri, Oct 22, 2021 at 12:35:04PM +0300, Joonas Lahtinen wrote: > > > > Hi Matt & John, > > > > > > > > Can you please queue patches with the right Fixes: references to convert > > > > all the GuC tracepoints to be protected by the LOW_LEVEL_TRACEPOINTS > > > > protection for now. Please do so before next Wednesday so we get it > > > > queued in drm-intel-next-fixes. > > > > > > > > > > Don't we already do that? I checked i915_trace.h and every tracepoint I > > > added (intel_context class, i915_request_guc_submit) is protected by > > > LOW_LEVEL_TRACEPOINTS. > > > > > > The only thing I changed outside of that protection is adding the guc_id > > > field to existing i915_request class tracepoints. > > > > It's the first search hit for "guc" inside the i915_trace.h file :) > > > > > Without the guc_id in > > > those tracepoints these are basically useless with GuC submission. We > > > could revert that if it is a huge deal but as I said then they are > > > useless... > > > > Let's eliminate it for now and restore the tracepoint exactly as it was. > > > > Don't really agree - let's render tracepoints to be useless? Are > tracepoints ABI? I googled this and couldn't really find a definie > answer. If tracepoints are ABI, then OK I can revert this change but > still this is a poor technical decision (tracepoints should not be ABI). Thats a very heated discussion in general. But the fact is that if tracepoint changes have caused regressions to applications, they have been forced to be remain untouched. You are free to raise the discussion with Linus/LKML if you feel that should not be the case. So the end result is that tracepoints are effectively in limbo, not ABI unless some application uses them like ABI. Feel free to search the intel-gfx/lkml for "tracepoints" keyword and look for threads with many replies. It's not that I would not agree, it's more that I'm not in the mood for repeating that discussion over and over again and always land in the same spot. So for now, we don't add anything new to tracepoints we can't guarantee to always be there untouched. Similarly, we don't guarantee any of them to remain stable. So we try to be compatible with the limbo. I'm long overdue waiting for some stable consumer to step up for the tracepoints, so we can then start discussion what would actually be the best way of getting that information out for them. In ~5 years that has not happened. > > If there is an immediate need, we should instead have an auxilary tracepoint > > which is enabled only through LOW_LEVEL_TRACEPOINTS and that amends the > > information of the basic tracepoint. > > > > Regardless of what I said above, I'll post 2 patches. The 1st just > remove the GuC, the 2nd modify the tracepoint to include guc_id if > LOW_LEVEL_TRACEPOINTS is defined. Thanks. Let's get a patch merged which simply drops the guc_id for now to unblock things. For the second, an auxilary tracepoint will be preferred instead of mutating the existing one (regardless of the LOW_LEVEL_TRACEPOINTS). I only noticed a patch that mutates the tracepoints, can you double-check sending the first patch? Regards, Joonas > > > For the longer term solution we should align towards the dma fence > > tracepoints. When those are combined with the OA information, one should > > be able to get a good understanding of both the software and hardware > > scheduling decisions. > > > > Not sure about this either. I use these tracepoins to correlate things > to the GuC log. Between the 2, if you know what you are doing you > basically can figure out everything that is happening. Fields in the > trace translate directly to fields in the GuC log. Some of these fields > are backend specific, not sure how these could be pushed the dma fence > tracepoints. For what it is worth, without these tracepoints we'd likely > still have a bunch of bugs in the GuC firmware. I understand these > points, several other i915 developers do, and several of the GuC > firmware developers do too. > > Matt > > > Regards, Joonas > > > > > > > > Matt > > > > > > > There's the orthogonal track to discuss what would be the stable set of > > > > tracepoints we could expose. However, before that discussion is closed, > > > > let's keep a rather strict line to avoid potential maintenance burned. > > > > > > > > We can then relax in the future as needed. > > > > > > > > Regards, Joonas > > > > > > > > Quoting Matthew Brost (2021-06-24 10:04:29) > > > > > As discussed in [1], [2] we are enabling GuC submission support in the > > > > > i915. This is a subset of the patches in step 5 described in [1], > > > > > basically it is absolute to enable CI with GuC submission on gen11+ > > > > > platforms. > > > > > > > > > > This series itself will likely be broken down into smaller patch sets > > > >
Re: [PATCH 00/47] GuC submission support
On 10/25/2021 02:37, Joonas Lahtinen wrote: Quoting Matthew Brost (2021-10-22 19:42:19) On Fri, Oct 22, 2021 at 12:35:04PM +0300, Joonas Lahtinen wrote: Hi Matt & John, Can you please queue patches with the right Fixes: references to convert all the GuC tracepoints to be protected by the LOW_LEVEL_TRACEPOINTS protection for now. Please do so before next Wednesday so we get it queued in drm-intel-next-fixes. Don't we already do that? I checked i915_trace.h and every tracepoint I added (intel_context class, i915_request_guc_submit) is protected by LOW_LEVEL_TRACEPOINTS. The only thing I changed outside of that protection is adding the guc_id field to existing i915_request class tracepoints. It's the first search hit for "guc" inside the i915_trace.h file :) Without the guc_id in those tracepoints these are basically useless with GuC submission. We could revert that if it is a huge deal but as I said then they are useless... Let's eliminate it for now and restore the tracepoint exactly as it was. For what purpose? Your request above was about not adding new tracepoints outside of a low level CONFIG setting. I can understand that on the grounds of not swamping high level tracing with low level details that are not important to the general developer. But this is not about adding extra tracepoints, this is about making the existing tracepoints usable. With GuC submission, the GuC id is a vital piece of information. Without that, you cannot correlate anything that is happening between i915, GuC and the hardware. Which basically means that for a GuC submission based platform, those tracepoints are useless without this information. And GuC submission is POR for all platforms from ADL-P/DG1 onwards. So by not allowing this update, you are preventing any kind of meaningful debug of any scheduling/execution type issues. Again, if you are wanting to reduce spam in higher level debug then sure, make the entire set of scheduling tracepoints LOW_LEVEL only. But keeping them around in a censored manner is pointless. They are not ABI, they are allowed to change as and when necessary. And now, it is necessary to update them to match the new POR submission model for current and all future platforms. If there is an immediate need, we should instead have an auxilary tracepoint which is enabled only through LOW_LEVEL_TRACEPOINTS and that amends the information of the basic tracepoint. For the longer term solution we should align towards the dma fence tracepoints. When those are combined with the OA information, one should be able to get a good understanding of both the software and hardware scheduling decisions. I don't follow this. OA information does not tell you any details of what the GuC is doing. DRM/DMA generic tracepoints certainly won't tell you any hardware/firmware or even i915 specific information. And that is a much longer term goal than being able to debug current platforms with the current driver. John. Regards, Joonas Matt There's the orthogonal track to discuss what would be the stable set of tracepoints we could expose. However, before that discussion is closed, let's keep a rather strict line to avoid potential maintenance burned. We can then relax in the future as needed. Regards, Joonas Quoting Matthew Brost (2021-06-24 10:04:29) As discussed in [1], [2] we are enabling GuC submission support in the i915. This is a subset of the patches in step 5 described in [1], basically it is absolute to enable CI with GuC submission on gen11+ platforms. This series itself will likely be broken down into smaller patch sets to merge. Likely into CTBs changes, basic submission, virtual engines, and resets. A following series will address the missing patches remaining from [1]. Locally tested on TGL machine and basic tests seem to be passing. Signed-off-by: Matthew Brost [1] https://patchwork.freedesktop.org/series/89844/ [2] https://patchwork.freedesktop.org/series/91417/ Daniele Ceraolo Spurio (1): drm/i915/guc: Unblock GuC submission on Gen11+ John Harrison (10): drm/i915/guc: Module load failure test for CT buffer creation drm/i915: Track 'serial' counts for virtual engines drm/i915/guc: Provide mmio list to be saved/restored on engine reset drm/i915/guc: Don't complain about reset races drm/i915/guc: Enable GuC engine reset drm/i915/guc: Fix for error capture after full GPU reset with GuC drm/i915/guc: Hook GuC scheduling policies up drm/i915/guc: Connect reset modparam updates to GuC policy flags drm/i915/guc: Include scheduling policies in the debugfs state dump drm/i915/guc: Add golden context to GuC ADS Matthew Brost (36): drm/i915/guc: Relax CTB response timeout drm/i915/guc: Improve error message for unsolicited CT response drm/i915/guc: Increase size of CTB buffers drm/i915/guc: Add non blocking CTB send function drm/i915/guc: Add stall timer to non blocking CTB send function drm/i915/guc: Optim
Re: [PATCH 00/47] GuC submission support
On Mon, Oct 25, 2021 at 12:37:02PM +0300, Joonas Lahtinen wrote: > Quoting Matthew Brost (2021-10-22 19:42:19) > > On Fri, Oct 22, 2021 at 12:35:04PM +0300, Joonas Lahtinen wrote: > > > Hi Matt & John, > > > > > > Can you please queue patches with the right Fixes: references to convert > > > all the GuC tracepoints to be protected by the LOW_LEVEL_TRACEPOINTS > > > protection for now. Please do so before next Wednesday so we get it > > > queued in drm-intel-next-fixes. > > > > > > > Don't we already do that? I checked i915_trace.h and every tracepoint I > > added (intel_context class, i915_request_guc_submit) is protected by > > LOW_LEVEL_TRACEPOINTS. > > > > The only thing I changed outside of that protection is adding the guc_id > > field to existing i915_request class tracepoints. > > It's the first search hit for "guc" inside the i915_trace.h file :) > > > Without the guc_id in > > those tracepoints these are basically useless with GuC submission. We > > could revert that if it is a huge deal but as I said then they are > > useless... > > Let's eliminate it for now and restore the tracepoint exactly as it was. > Don't really agree - let's render tracepoints to be useless? Are tracepoints ABI? I googled this and couldn't really find a definie answer. If tracepoints are ABI, then OK I can revert this change but still this is a poor technical decision (tracepoints should not be ABI). > If there is an immediate need, we should instead have an auxilary tracepoint > which is enabled only through LOW_LEVEL_TRACEPOINTS and that amends the > information of the basic tracepoint. > Regardless of what I said above, I'll post 2 patches. The 1st just remove the GuC, the 2nd modify the tracepoint to include guc_id if LOW_LEVEL_TRACEPOINTS is defined. > For the longer term solution we should align towards the dma fence > tracepoints. When those are combined with the OA information, one should > be able to get a good understanding of both the software and hardware > scheduling decisions. > Not sure about this either. I use these tracepoins to correlate things to the GuC log. Between the 2, if you know what you are doing you basically can figure out everything that is happening. Fields in the trace translate directly to fields in the GuC log. Some of these fields are backend specific, not sure how these could be pushed the dma fence tracepoints. For what it is worth, without these tracepoints we'd likely still have a bunch of bugs in the GuC firmware. I understand these points, several other i915 developers do, and several of the GuC firmware developers do too. Matt > Regards, Joonas > > > > > Matt > > > > > There's the orthogonal track to discuss what would be the stable set of > > > tracepoints we could expose. However, before that discussion is closed, > > > let's keep a rather strict line to avoid potential maintenance burned. > > > > > > We can then relax in the future as needed. > > > > > > Regards, Joonas > > > > > > Quoting Matthew Brost (2021-06-24 10:04:29) > > > > As discussed in [1], [2] we are enabling GuC submission support in the > > > > i915. This is a subset of the patches in step 5 described in [1], > > > > basically it is absolute to enable CI with GuC submission on gen11+ > > > > platforms. > > > > > > > > This series itself will likely be broken down into smaller patch sets to > > > > merge. Likely into CTBs changes, basic submission, virtual engines, and > > > > resets. > > > > > > > > A following series will address the missing patches remaining from [1]. > > > > > > > > Locally tested on TGL machine and basic tests seem to be passing. > > > > > > > > Signed-off-by: Matthew Brost > > > > > > > > [1] https://patchwork.freedesktop.org/series/89844/ > > > > [2] https://patchwork.freedesktop.org/series/91417/ > > > > > > > > Daniele Ceraolo Spurio (1): > > > > drm/i915/guc: Unblock GuC submission on Gen11+ > > > > > > > > John Harrison (10): > > > > drm/i915/guc: Module load failure test for CT buffer creation > > > > drm/i915: Track 'serial' counts for virtual engines > > > > drm/i915/guc: Provide mmio list to be saved/restored on engine reset > > > > drm/i915/guc: Don't complain about reset races > > > > drm/i915/guc: Enable GuC engine reset > > > > drm/i915/guc: Fix for error capture after full GPU reset with GuC > > > > drm/i915/guc: Hook GuC scheduling policies up > > > > drm/i915/guc: Connect reset modparam updates to GuC policy flags > > > > drm/i915/guc: Include scheduling policies in the debugfs state dump > > > > drm/i915/guc: Add golden context to GuC ADS > > > > > > > > Matthew Brost (36): > > > > drm/i915/guc: Relax CTB response timeout > > > > drm/i915/guc: Improve error message for unsolicited CT response > > > > drm/i915/guc: Increase size of CTB buffers > > > > drm/i915/guc: Add non blocking CTB send function > > > > drm/i915/guc: Add stall timer to non blocking CTB send function > > > > drm/i915/guc: Optimize CTB write
Re: [PATCH 00/47] GuC submission support
Quoting Matthew Brost (2021-10-22 19:42:19) > On Fri, Oct 22, 2021 at 12:35:04PM +0300, Joonas Lahtinen wrote: > > Hi Matt & John, > > > > Can you please queue patches with the right Fixes: references to convert > > all the GuC tracepoints to be protected by the LOW_LEVEL_TRACEPOINTS > > protection for now. Please do so before next Wednesday so we get it > > queued in drm-intel-next-fixes. > > > > Don't we already do that? I checked i915_trace.h and every tracepoint I > added (intel_context class, i915_request_guc_submit) is protected by > LOW_LEVEL_TRACEPOINTS. > > The only thing I changed outside of that protection is adding the guc_id > field to existing i915_request class tracepoints. It's the first search hit for "guc" inside the i915_trace.h file :) > Without the guc_id in > those tracepoints these are basically useless with GuC submission. We > could revert that if it is a huge deal but as I said then they are > useless... Let's eliminate it for now and restore the tracepoint exactly as it was. If there is an immediate need, we should instead have an auxilary tracepoint which is enabled only through LOW_LEVEL_TRACEPOINTS and that amends the information of the basic tracepoint. For the longer term solution we should align towards the dma fence tracepoints. When those are combined with the OA information, one should be able to get a good understanding of both the software and hardware scheduling decisions. Regards, Joonas > > Matt > > > There's the orthogonal track to discuss what would be the stable set of > > tracepoints we could expose. However, before that discussion is closed, > > let's keep a rather strict line to avoid potential maintenance burned. > > > > We can then relax in the future as needed. > > > > Regards, Joonas > > > > Quoting Matthew Brost (2021-06-24 10:04:29) > > > As discussed in [1], [2] we are enabling GuC submission support in the > > > i915. This is a subset of the patches in step 5 described in [1], > > > basically it is absolute to enable CI with GuC submission on gen11+ > > > platforms. > > > > > > This series itself will likely be broken down into smaller patch sets to > > > merge. Likely into CTBs changes, basic submission, virtual engines, and > > > resets. > > > > > > A following series will address the missing patches remaining from [1]. > > > > > > Locally tested on TGL machine and basic tests seem to be passing. > > > > > > Signed-off-by: Matthew Brost > > > > > > [1] https://patchwork.freedesktop.org/series/89844/ > > > [2] https://patchwork.freedesktop.org/series/91417/ > > > > > > Daniele Ceraolo Spurio (1): > > > drm/i915/guc: Unblock GuC submission on Gen11+ > > > > > > John Harrison (10): > > > drm/i915/guc: Module load failure test for CT buffer creation > > > drm/i915: Track 'serial' counts for virtual engines > > > drm/i915/guc: Provide mmio list to be saved/restored on engine reset > > > drm/i915/guc: Don't complain about reset races > > > drm/i915/guc: Enable GuC engine reset > > > drm/i915/guc: Fix for error capture after full GPU reset with GuC > > > drm/i915/guc: Hook GuC scheduling policies up > > > drm/i915/guc: Connect reset modparam updates to GuC policy flags > > > drm/i915/guc: Include scheduling policies in the debugfs state dump > > > drm/i915/guc: Add golden context to GuC ADS > > > > > > Matthew Brost (36): > > > drm/i915/guc: Relax CTB response timeout > > > drm/i915/guc: Improve error message for unsolicited CT response > > > drm/i915/guc: Increase size of CTB buffers > > > drm/i915/guc: Add non blocking CTB send function > > > drm/i915/guc: Add stall timer to non blocking CTB send function > > > drm/i915/guc: Optimize CTB writes and reads > > > drm/i915/guc: Add new GuC interface defines and structures > > > drm/i915/guc: Remove GuC stage descriptor, add lrc descriptor > > > drm/i915/guc: Add lrc descriptor context lookup array > > > drm/i915/guc: Implement GuC submission tasklet > > > drm/i915/guc: Add bypass tasklet submission path to GuC > > > drm/i915/guc: Implement GuC context operations for new inteface > > > drm/i915/guc: Insert fence on context when deregistering > > > drm/i915/guc: Defer context unpin until scheduling is disabled > > > drm/i915/guc: Disable engine barriers with GuC during unpin > > > drm/i915/guc: Extend deregistration fence to schedule disable > > > drm/i915: Disable preempt busywait when using GuC scheduling > > > drm/i915/guc: Ensure request ordering via completion fences > > > drm/i915/guc: Disable semaphores when using GuC scheduling > > > drm/i915/guc: Ensure G2H response has space in buffer > > > drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC > > > drm/i915/guc: Update GuC debugfs to support new GuC > > > drm/i915/guc: Add several request trace points > > > drm/i915: Add intel_context tracing > > > drm/i915/guc: GuC virtual engines > > > drm/i915: Hold reference to intel_context over life of i915
Re: [PATCH 00/47] GuC submission support
On Fri, Oct 22, 2021 at 12:35:04PM +0300, Joonas Lahtinen wrote: > Hi Matt & John, > > Can you please queue patches with the right Fixes: references to convert > all the GuC tracepoints to be protected by the LOW_LEVEL_TRACEPOINTS > protection for now. Please do so before next Wednesday so we get it > queued in drm-intel-next-fixes. > Don't we already do that? I checked i915_trace.h and every tracepoint I added (intel_context class, i915_request_guc_submit) is protected by LOW_LEVEL_TRACEPOINTS. The only thing I changed outside of that protection is adding the guc_id field to existing i915_request class tracepoints. Without the guc_id in those tracepoints these are basically useless with GuC submission. We could revert that if it is a huge deal but as I said then they are useless... Matt > There's the orthogonal track to discuss what would be the stable set of > tracepoints we could expose. However, before that discussion is closed, > let's keep a rather strict line to avoid potential maintenance burned. > > We can then relax in the future as needed. > > Regards, Joonas > > Quoting Matthew Brost (2021-06-24 10:04:29) > > As discussed in [1], [2] we are enabling GuC submission support in the > > i915. This is a subset of the patches in step 5 described in [1], > > basically it is absolute to enable CI with GuC submission on gen11+ > > platforms. > > > > This series itself will likely be broken down into smaller patch sets to > > merge. Likely into CTBs changes, basic submission, virtual engines, and > > resets. > > > > A following series will address the missing patches remaining from [1]. > > > > Locally tested on TGL machine and basic tests seem to be passing. > > > > Signed-off-by: Matthew Brost > > > > [1] https://patchwork.freedesktop.org/series/89844/ > > [2] https://patchwork.freedesktop.org/series/91417/ > > > > Daniele Ceraolo Spurio (1): > > drm/i915/guc: Unblock GuC submission on Gen11+ > > > > John Harrison (10): > > drm/i915/guc: Module load failure test for CT buffer creation > > drm/i915: Track 'serial' counts for virtual engines > > drm/i915/guc: Provide mmio list to be saved/restored on engine reset > > drm/i915/guc: Don't complain about reset races > > drm/i915/guc: Enable GuC engine reset > > drm/i915/guc: Fix for error capture after full GPU reset with GuC > > drm/i915/guc: Hook GuC scheduling policies up > > drm/i915/guc: Connect reset modparam updates to GuC policy flags > > drm/i915/guc: Include scheduling policies in the debugfs state dump > > drm/i915/guc: Add golden context to GuC ADS > > > > Matthew Brost (36): > > drm/i915/guc: Relax CTB response timeout > > drm/i915/guc: Improve error message for unsolicited CT response > > drm/i915/guc: Increase size of CTB buffers > > drm/i915/guc: Add non blocking CTB send function > > drm/i915/guc: Add stall timer to non blocking CTB send function > > drm/i915/guc: Optimize CTB writes and reads > > drm/i915/guc: Add new GuC interface defines and structures > > drm/i915/guc: Remove GuC stage descriptor, add lrc descriptor > > drm/i915/guc: Add lrc descriptor context lookup array > > drm/i915/guc: Implement GuC submission tasklet > > drm/i915/guc: Add bypass tasklet submission path to GuC > > drm/i915/guc: Implement GuC context operations for new inteface > > drm/i915/guc: Insert fence on context when deregistering > > drm/i915/guc: Defer context unpin until scheduling is disabled > > drm/i915/guc: Disable engine barriers with GuC during unpin > > drm/i915/guc: Extend deregistration fence to schedule disable > > drm/i915: Disable preempt busywait when using GuC scheduling > > drm/i915/guc: Ensure request ordering via completion fences > > drm/i915/guc: Disable semaphores when using GuC scheduling > > drm/i915/guc: Ensure G2H response has space in buffer > > drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC > > drm/i915/guc: Update GuC debugfs to support new GuC > > drm/i915/guc: Add several request trace points > > drm/i915: Add intel_context tracing > > drm/i915/guc: GuC virtual engines > > drm/i915: Hold reference to intel_context over life of i915_request > > drm/i915/guc: Disable bonding extension with GuC submission > > drm/i915/guc: Direct all breadcrumbs for a class to single breadcrumbs > > drm/i915/guc: Reset implementation for new GuC interface > > drm/i915: Reset GPU immediately if submission is disabled > > drm/i915/guc: Add disable interrupts to guc sanitize > > drm/i915/guc: Suspend/resume implementation for new interface > > drm/i915/guc: Handle context reset notification > > drm/i915/guc: Handle engine reset failure notification > > drm/i915/guc: Enable the timer expired interrupt for GuC > > drm/i915/guc: Capture error state on context reset > > > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 30 +- > > drivers/gpu/drm/i915/gem/i915_gem_context.h |1 + > > drivers/gpu/drm/i915/gem/i915_gem_mm
Re: [PATCH 00/47] GuC submission support
Hi Matt & John, Can you please queue patches with the right Fixes: references to convert all the GuC tracepoints to be protected by the LOW_LEVEL_TRACEPOINTS protection for now. Please do so before next Wednesday so we get it queued in drm-intel-next-fixes. There's the orthogonal track to discuss what would be the stable set of tracepoints we could expose. However, before that discussion is closed, let's keep a rather strict line to avoid potential maintenance burned. We can then relax in the future as needed. Regards, Joonas Quoting Matthew Brost (2021-06-24 10:04:29) > As discussed in [1], [2] we are enabling GuC submission support in the > i915. This is a subset of the patches in step 5 described in [1], > basically it is absolute to enable CI with GuC submission on gen11+ > platforms. > > This series itself will likely be broken down into smaller patch sets to > merge. Likely into CTBs changes, basic submission, virtual engines, and > resets. > > A following series will address the missing patches remaining from [1]. > > Locally tested on TGL machine and basic tests seem to be passing. > > Signed-off-by: Matthew Brost > > [1] https://patchwork.freedesktop.org/series/89844/ > [2] https://patchwork.freedesktop.org/series/91417/ > > Daniele Ceraolo Spurio (1): > drm/i915/guc: Unblock GuC submission on Gen11+ > > John Harrison (10): > drm/i915/guc: Module load failure test for CT buffer creation > drm/i915: Track 'serial' counts for virtual engines > drm/i915/guc: Provide mmio list to be saved/restored on engine reset > drm/i915/guc: Don't complain about reset races > drm/i915/guc: Enable GuC engine reset > drm/i915/guc: Fix for error capture after full GPU reset with GuC > drm/i915/guc: Hook GuC scheduling policies up > drm/i915/guc: Connect reset modparam updates to GuC policy flags > drm/i915/guc: Include scheduling policies in the debugfs state dump > drm/i915/guc: Add golden context to GuC ADS > > Matthew Brost (36): > drm/i915/guc: Relax CTB response timeout > drm/i915/guc: Improve error message for unsolicited CT response > drm/i915/guc: Increase size of CTB buffers > drm/i915/guc: Add non blocking CTB send function > drm/i915/guc: Add stall timer to non blocking CTB send function > drm/i915/guc: Optimize CTB writes and reads > drm/i915/guc: Add new GuC interface defines and structures > drm/i915/guc: Remove GuC stage descriptor, add lrc descriptor > drm/i915/guc: Add lrc descriptor context lookup array > drm/i915/guc: Implement GuC submission tasklet > drm/i915/guc: Add bypass tasklet submission path to GuC > drm/i915/guc: Implement GuC context operations for new inteface > drm/i915/guc: Insert fence on context when deregistering > drm/i915/guc: Defer context unpin until scheduling is disabled > drm/i915/guc: Disable engine barriers with GuC during unpin > drm/i915/guc: Extend deregistration fence to schedule disable > drm/i915: Disable preempt busywait when using GuC scheduling > drm/i915/guc: Ensure request ordering via completion fences > drm/i915/guc: Disable semaphores when using GuC scheduling > drm/i915/guc: Ensure G2H response has space in buffer > drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC > drm/i915/guc: Update GuC debugfs to support new GuC > drm/i915/guc: Add several request trace points > drm/i915: Add intel_context tracing > drm/i915/guc: GuC virtual engines > drm/i915: Hold reference to intel_context over life of i915_request > drm/i915/guc: Disable bonding extension with GuC submission > drm/i915/guc: Direct all breadcrumbs for a class to single breadcrumbs > drm/i915/guc: Reset implementation for new GuC interface > drm/i915: Reset GPU immediately if submission is disabled > drm/i915/guc: Add disable interrupts to guc sanitize > drm/i915/guc: Suspend/resume implementation for new interface > drm/i915/guc: Handle context reset notification > drm/i915/guc: Handle engine reset failure notification > drm/i915/guc: Enable the timer expired interrupt for GuC > drm/i915/guc: Capture error state on context reset > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 30 +- > drivers/gpu/drm/i915/gem/i915_gem_context.h |1 + > drivers/gpu/drm/i915/gem/i915_gem_mman.c |3 +- > drivers/gpu/drm/i915/gt/gen8_engine_cs.c |6 +- > drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 41 +- > drivers/gpu/drm/i915/gt/intel_breadcrumbs.h | 14 +- > .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |7 + > drivers/gpu/drm/i915/gt/intel_context.c | 41 +- > drivers/gpu/drm/i915/gt/intel_context.h | 31 +- > drivers/gpu/drm/i915/gt/intel_context_types.h | 49 + > drivers/gpu/drm/i915/gt/intel_engine.h| 72 +- > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 182 +- > .../gpu/drm/i915/gt/intel_engine_heartbeat.c | 71 +- > .../gpu/drm/i915/gt/intel_engine_heartbeat.h |4 + > drivers/gpu/drm/i915/gt/intel_engine_types
[PATCH 00/47] GuC submission support
As discussed in [1], [2] we are enabling GuC submission support in the i915. This is a subset of the patches in step 5 described in [1], basically it is absolute to enable CI with GuC submission on gen11+ platforms. This series itself will likely be broken down into smaller patch sets to merge. Likely into CTBs changes, basic submission, virtual engines, and resets. A following series will address the missing patches remaining from [1]. Locally tested on TGL machine and basic tests seem to be passing. Signed-off-by: Matthew Brost [1] https://patchwork.freedesktop.org/series/89844/ [2] https://patchwork.freedesktop.org/series/91417/ Daniele Ceraolo Spurio (1): drm/i915/guc: Unblock GuC submission on Gen11+ John Harrison (10): drm/i915/guc: Module load failure test for CT buffer creation drm/i915: Track 'serial' counts for virtual engines drm/i915/guc: Provide mmio list to be saved/restored on engine reset drm/i915/guc: Don't complain about reset races drm/i915/guc: Enable GuC engine reset drm/i915/guc: Fix for error capture after full GPU reset with GuC drm/i915/guc: Hook GuC scheduling policies up drm/i915/guc: Connect reset modparam updates to GuC policy flags drm/i915/guc: Include scheduling policies in the debugfs state dump drm/i915/guc: Add golden context to GuC ADS Matthew Brost (36): drm/i915/guc: Relax CTB response timeout drm/i915/guc: Improve error message for unsolicited CT response drm/i915/guc: Increase size of CTB buffers drm/i915/guc: Add non blocking CTB send function drm/i915/guc: Add stall timer to non blocking CTB send function drm/i915/guc: Optimize CTB writes and reads drm/i915/guc: Add new GuC interface defines and structures drm/i915/guc: Remove GuC stage descriptor, add lrc descriptor drm/i915/guc: Add lrc descriptor context lookup array drm/i915/guc: Implement GuC submission tasklet drm/i915/guc: Add bypass tasklet submission path to GuC drm/i915/guc: Implement GuC context operations for new inteface drm/i915/guc: Insert fence on context when deregistering drm/i915/guc: Defer context unpin until scheduling is disabled drm/i915/guc: Disable engine barriers with GuC during unpin drm/i915/guc: Extend deregistration fence to schedule disable drm/i915: Disable preempt busywait when using GuC scheduling drm/i915/guc: Ensure request ordering via completion fences drm/i915/guc: Disable semaphores when using GuC scheduling drm/i915/guc: Ensure G2H response has space in buffer drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC drm/i915/guc: Update GuC debugfs to support new GuC drm/i915/guc: Add several request trace points drm/i915: Add intel_context tracing drm/i915/guc: GuC virtual engines drm/i915: Hold reference to intel_context over life of i915_request drm/i915/guc: Disable bonding extension with GuC submission drm/i915/guc: Direct all breadcrumbs for a class to single breadcrumbs drm/i915/guc: Reset implementation for new GuC interface drm/i915: Reset GPU immediately if submission is disabled drm/i915/guc: Add disable interrupts to guc sanitize drm/i915/guc: Suspend/resume implementation for new interface drm/i915/guc: Handle context reset notification drm/i915/guc: Handle engine reset failure notification drm/i915/guc: Enable the timer expired interrupt for GuC drm/i915/guc: Capture error state on context reset drivers/gpu/drm/i915/gem/i915_gem_context.c | 30 +- drivers/gpu/drm/i915/gem/i915_gem_context.h |1 + drivers/gpu/drm/i915/gem/i915_gem_mman.c |3 +- drivers/gpu/drm/i915/gt/gen8_engine_cs.c |6 +- drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 41 +- drivers/gpu/drm/i915/gt/intel_breadcrumbs.h | 14 +- .../gpu/drm/i915/gt/intel_breadcrumbs_types.h |7 + drivers/gpu/drm/i915/gt/intel_context.c | 41 +- drivers/gpu/drm/i915/gt/intel_context.h | 31 +- drivers/gpu/drm/i915/gt/intel_context_types.h | 49 + drivers/gpu/drm/i915/gt/intel_engine.h| 72 +- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 182 +- .../gpu/drm/i915/gt/intel_engine_heartbeat.c | 71 +- .../gpu/drm/i915/gt/intel_engine_heartbeat.h |4 + drivers/gpu/drm/i915/gt/intel_engine_types.h | 12 +- .../drm/i915/gt/intel_execlists_submission.c | 234 +- .../drm/i915/gt/intel_execlists_submission.h | 11 - drivers/gpu/drm/i915/gt/intel_gt.c| 21 + drivers/gpu/drm/i915/gt/intel_gt.h|2 + drivers/gpu/drm/i915/gt/intel_gt_pm.c |6 +- drivers/gpu/drm/i915/gt/intel_gt_requests.c | 22 +- drivers/gpu/drm/i915/gt/intel_gt_requests.h |9 +- drivers/gpu/drm/i915/gt/intel_lrc_reg.h |1 - drivers/gpu/drm/i915/gt/intel_reset.c | 20 +- .../gpu/drm/i915/gt/intel_ring_submission.c | 28 + drivers/gpu/drm/i915/gt/intel_rps.c |4 + drivers/gpu/drm/i915/gt/intel_workarounds.c | 46 +- .../gpu/drm/i915/gt/intel_workarounds_types.h |1 + driv
