Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP
> alan:snip > > Assuming that: > > 2 = PXP feature is supported but should be ready soon (pending > initialization of non-i915 system dependencies). > > really means, "it'll be ready soon or there is a bug somewhere", > > Acked-by: Jordan Justen > > If Mesa finds that it can't really rely on that, we may have to decide > on a different approach in how to use that return value. > > Is it possible to hold on for another ~12 hours to see if Lionel wants > to Ack as well? > > -Jordan alan: agreed and thanks. And sure, lets give ourselves till tomorrow afternoon PST.
Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP
On 2023-05-10 15:00:03, Teres Alexis, Alan Previn wrote: > > alan:snip > > > This is why I asked if it was it was "basically certain that in a > > production environment, then it will eventually return 1 meaning it's > > ready". Alan's response was a little ambiguous on this point. > alan: if we get a '2' and never transition to '1' - thats a kernel bug or > firmware / system issue. > > > Arguably a transition from 2 to -ENODEV could be considered a kernel > > bug, but it doesn't sound like Alan would agree. :) Maybe Alan would > > agree to saying it's either a kernel, or system integration bug. > alan: agreed - that would be a kernel bug or a system integration bug. > > I also agreed on the init-flow vs app-usage thoughts Jordan had. > That said MESA has many ways it can use this UAPI and we can discuss > that on the MESA patch. > > > hmmm.. so... ack anyone? [insert big hopeful smiley here] > apologies if I am asking too often. Assuming that: 2 = PXP feature is supported but should be ready soon (pending initialization of non-i915 system dependencies). really means, "it'll be ready soon or there is a bug somewhere", Acked-by: Jordan Justen If Mesa finds that it can't really rely on that, we may have to decide on a different approach in how to use that return value. Is it possible to hold on for another ~12 hours to see if Lionel wants to Ack as well? -Jordan
Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP
alan:snip > This is why I asked if it was it was "basically certain that in a > production environment, then it will eventually return 1 meaning it's > ready". Alan's response was a little ambiguous on this point. alan: if we get a '2' and never transition to '1' - thats a kernel bug or firmware / system issue. > Arguably a transition from 2 to -ENODEV could be considered a kernel > bug, but it doesn't sound like Alan would agree. :) Maybe Alan would > agree to saying it's either a kernel, or system integration bug. alan: agreed - that would be a kernel bug or a system integration bug. I also agreed on the init-flow vs app-usage thoughts Jordan had. That said MESA has many ways it can use this UAPI and we can discuss that on the MESA patch. hmmm.. so... ack anyone? [insert big hopeful smiley here] apologies if I am asking too often.
Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP
...fixing some Cc email addresses I somehow mangled. On 2023-05-10 12:40:07, Souza, Jose wrote: > On Mon, 2023-05-08 at 17:49 +, Teres Alexis, Alan Previn wrote: > > On Fri, 2023-05-05 at 00:39 -0700, Justen, Jordan L wrote: > > > On 2023-05-04 22:30:07, Teres Alexis, Alan Previn wrote: > > > > On Thu, 2023-04-27 at 16:48 -0700, Teres Alexis, Alan Previn wrote: > > > > > Because of the additional firmware, component-driver and > > > > > initialization depedencies required on MTL platform before a > > > > > PXP context can be created, UMD calling for PXP creation as a > > > > > way to get-caps can take a long time. An actual real world > > > > > customer stack has seen this happen in the 4-to-8 second range > > > > > after the kernel starts (which sees MESA's init appear in the > > > > > middle of this range as the compositor comes up). To avoid > > > > > unncessary delays experienced by the UMD for get-caps purposes, > > > > > add a GET_PARAM for I915_PARAM_PXP_SUPPORT. > > > > > > > > > alan:snip. > > > > Progress update on the UMD side - I'm working on patch for PR here: > > > > https://gitlab.freedesktop.org/alan_previn_intel/mesa-alan-previn-features/-/commit/fb9d4fbfbef7dfd3f41df335cd31549fd39ddb57 > > > > but taking time to verify certain code paths > > > > > > Just to confirm, if I915_PARAM_PXP_STATUS returns 2 ("will be ready > > > soon"), then it is basically certain that in a production environment, > > > then it will eventually return 1 meaning it's ready, right? > > alan: yes - but not 100%. non-platform-state-machine could still > > cause unexpected failures for example, [1] if hw was fused in a way > > that doesnt permit PXP or [2] enabling certain BIOS debug knobs > > doesnt allow PXP. However at the moment, there is no way for us > > to know for sure without actually creating a protected context. > > Of course having hw-fusing + bios-config that supports PXP have > > always 100% succeeded for me. > > In my opinion it is problematic mark that we support protected > context but then it fails to create protected context. This is why I asked if it was it was "basically certain that in a production environment, then it will eventually return 1 meaning it's ready". Alan's response was a little ambiguous on this point. But, considering the number of applications that will need this feature is low, combined with an extremely low chance that the kernel will end up going from 2 (will be ready soon) to -ENODEV (will never work), well, it seems worth considering advertising it with no delay even if it later fails if used. Arguably a transition from 2 to -ENODEV could be considered a kernel bug, but it doesn't sound like Alan would agree. :) Maybe Alan would agree to saying it's either a kernel, or system integration bug. I think it'd also be ok if we didn't advertise support if an application starts when the kernel is still in the 2 (will be ready soon) state. But, some environments might prefer to wait, so I think the kernel uapi should stay as Alan has it now so we have the flexibility to potentially accommodate this. (Perhaps with driconf, or a build flag, or an env-var.) -Jordan
Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP
On 2023-05-10 12:40:07, Souza, Jose wrote: > On Mon, 2023-05-08 at 17:49 +, Teres Alexis, Alan Previn wrote: > > On Fri, 2023-05-05 at 00:39 -0700, Justen, Jordan L wrote: > > > On 2023-05-04 22:30:07, Teres Alexis, Alan Previn wrote: > > > > On Thu, 2023-04-27 at 16:48 -0700, Teres Alexis, Alan Previn wrote: > > > > > Because of the additional firmware, component-driver and > > > > > initialization depedencies required on MTL platform before a > > > > > PXP context can be created, UMD calling for PXP creation as a > > > > > way to get-caps can take a long time. An actual real world > > > > > customer stack has seen this happen in the 4-to-8 second range > > > > > after the kernel starts (which sees MESA's init appear in the > > > > > middle of this range as the compositor comes up). To avoid > > > > > unncessary delays experienced by the UMD for get-caps purposes, > > > > > add a GET_PARAM for I915_PARAM_PXP_SUPPORT. > > > > > > > > > alan:snip. > > > > Progress update on the UMD side - I'm working on patch for PR here: > > > > https://gitlab.freedesktop.org/alan_previn_intel/mesa-alan-previn-features/-/commit/fb9d4fbfbef7dfd3f41df335cd31549fd39ddb57 > > > > but taking time to verify certain code paths > > > > > > Just to confirm, if I915_PARAM_PXP_STATUS returns 2 ("will be ready > > > soon"), then it is basically certain that in a production environment, > > > then it will eventually return 1 meaning it's ready, right? > > alan: yes - but not 100%. non-platform-state-machine could still > > cause unexpected failures for example, [1] if hw was fused in a way > > that doesnt permit PXP or [2] enabling certain BIOS debug knobs > > doesnt allow PXP. However at the moment, there is no way for us > > to know for sure without actually creating a protected context. > > Of course having hw-fusing + bios-config that supports PXP have > > always 100% succeeded for me. > > In my opinion it is problematic mark that we support protected > context but then it fails to create protected context. This is why I asked if it was it was "basically certain that in a production environment, then it will eventually return 1 meaning it's ready". Alan's response was a little ambiguous on this point. But, considering the number of applications that will need this feature is low, combined with an extremely low chance that the kernel will end up going from 2 (will be ready soon) to -ENODEV (will never work), well, it seems worth considering advertising it with no delay even if it later fails if used. Arguably a transition from 2 to -ENODEV could be considered a kernel bug, but it doesn't sound like Alan would agree. :) Maybe Alan would agree to saying it's either a kernel, or system integration bug. I think it'd also be ok if we didn't advertise support if an application starts when the kernel is still in the 2 (will be ready soon) state. But, some environments might prefer to wait, so I think the kernel uapi should stay as Alan has it now so we have the flexibility to potentially accommodate this. (Perhaps with driconf, or a build flag, or an env-var.) -Jordan
Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP
> > > > > Because of the additional firmware, component-driver and > > > > > initialization depedencies required on MTL platform before a > > > > > PXP context can be created, UMD calling for PXP creation as a > > > > > way to get-caps can take a long time. An actual real world > > > > > customer stack has seen this happen in the 4-to-8 second range > > > > > after the kernel starts (which sees MESA's init appear in the > > > > > middle of this range as the compositor comes up). To avoid > > > > > unncessary delays experienced by the UMD for get-caps purposes, > > > > > add a GET_PARAM for I915_PARAM_PXP_SUPPORT. > > > > > > > > > alan:snip. > > > > Progress update on the UMD side - I'm working on patch for PR here: > > > > https://gitlab.freedesktop.org/alan_previn_intel/mesa-alan-previn-features/-/commit/fb9d4fbfbef7dfd3f41df335cd31549fd39ddb57 > > > > but taking time to verify certain code paths > > > > > > Just to confirm, if I915_PARAM_PXP_STATUS returns 2 ("will be ready > > > soon"), then it is basically certain that in a production environment, > > > then it will eventually return 1 meaning it's ready, right? > > alan: yes - but not 100%. non-platform-state-machine could still > > cause unexpected failures for example, [1] if hw was fused in a way > > that doesnt permit PXP or [2] enabling certain BIOS debug knobs > > doesnt allow PXP. However at the moment, there is no way for us > > to know for sure without actually creating a protected context. > > Of course having hw-fusing + bios-config that supports PXP have > > always 100% succeeded for me. > > In my opinion it is problematic mark that we support protected context but > then it fails to create protected context. > > It should check the I915_PARAM_PXP_STATUS in > i915_gem_supports_protected_context() return earlier if know that protected > context is not supported. > Then create a protected context so we know that all other calls will succeed. Hi Jose, I think your comment WRT how MESA change coule be implemented. Right now this UAPI will provide all possible information: '-ENODEV'==no-support... '1'==supported-and-read... '2'==supported-but-not-ready. Unfortunately the '2'->'1' transition is not something the kernel driver has control over. As per the earlier review comments in prior revs and weeks with others (Lionel, Jordan, Trvtko, Daniele, etc), depending on certain scenarios (kernel configs with many components + interdependencies, .. OR... a fresh platform-IFWI-update ... OR... a distro that is designed to boot under 1 sec), the "2"->"1" can take up to 8-seconds-from-kernel-start. In almost all our internal CI testing we are only seeing it take 2-seconds-from-kernel-start. That said, perhaps we can discuss the MESA behavior on the MESA patch: do we want to block on init time get-caps (i915_gem_supports_protected_context) or during runtime context-creation (if the user explicitly requests it).. or never block from MESA and report if the PXP context failed because it wasnt ready (so user knows it could retry). <-- this last one was what Jordan wanted and what i posted here on MESA patch: https://gitlab.freedesktop.org/alan_previn_intel/mesa-alan-previn-features/-/commit/8728ab5a40c1a83f65afe0072f45a906ff26f706 However, as it stands today, this UAPI kernel patch has incorporated all the requests from prior review comments and provides as much information as required. Do you see other shortcoming this kernel side UAPI behavior? if not, an ack would be greatly appreciated :) since earlier ack from Lionel was on the prior behavior before Jordan's request for this more comprehensive response-set (-ENODEV vs '1' vs '2').
Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP
On Mon, 2023-05-08 at 17:49 +, Teres Alexis, Alan Previn wrote: > On Fri, 2023-05-05 at 00:39 -0700, Justen, Jordan L wrote: > > On 2023-05-04 22:30:07, Teres Alexis, Alan Previn wrote: > > > On Thu, 2023-04-27 at 16:48 -0700, Teres Alexis, Alan Previn wrote: > > > > Because of the additional firmware, component-driver and > > > > initialization depedencies required on MTL platform before a > > > > PXP context can be created, UMD calling for PXP creation as a > > > > way to get-caps can take a long time. An actual real world > > > > customer stack has seen this happen in the 4-to-8 second range > > > > after the kernel starts (which sees MESA's init appear in the > > > > middle of this range as the compositor comes up). To avoid > > > > unncessary delays experienced by the UMD for get-caps purposes, > > > > add a GET_PARAM for I915_PARAM_PXP_SUPPORT. > > > > > > > alan:snip. > > > Progress update on the UMD side - I'm working on patch for PR here: > > > https://gitlab.freedesktop.org/alan_previn_intel/mesa-alan-previn-features/-/commit/fb9d4fbfbef7dfd3f41df335cd31549fd39ddb57 > > > but taking time to verify certain code paths > > > > Just to confirm, if I915_PARAM_PXP_STATUS returns 2 ("will be ready > > soon"), then it is basically certain that in a production environment, > > then it will eventually return 1 meaning it's ready, right? > alan: yes - but not 100%. non-platform-state-machine could still > cause unexpected failures for example, [1] if hw was fused in a way > that doesnt permit PXP or [2] enabling certain BIOS debug knobs > doesnt allow PXP. However at the moment, there is no way for us > to know for sure without actually creating a protected context. > Of course having hw-fusing + bios-config that supports PXP have > always 100% succeeded for me. In my opinion it is problematic mark that we support protected context but then it fails to create protected context. It should check the I915_PARAM_PXP_STATUS in i915_gem_supports_protected_context() return earlier if know that protected context is not supported. Then create a protected context so we know that all other calls will succeed. > > > > > If this is correct, then I think that the change in > > i915_gem_supports_protected_context() is good, and probably we can > > skip the change in iris_create_hw_context(). > > > > Basically, we're timing out for multiple seconds either way. And, I'm > > hoping that the kernel will eventually get the PXP init done and > > create the context. > > > > I think there's 2 cases of interest here. > > > > First, and most likely, the application running doesn't care about > > protected content. In this case we can quickly advertise the support, > > but there will be no consequence because the application won't use the > > feature. > alan: yes - that was one of the goals of this UAPI. > > > > Second, the application does care about protected content. In this > > case we can quickly advertise the support, but if the feature is used > > too quickly, then the context create might take a long time. > alan: no, that isnt the case now, we started at 8-secs, then 2-secs, > and finally settled on the same timeout as ADL since this new UAPI > will be something that can be polled on to be sure we have readiness > to make the create-context-call. That's why i wanted to add the > polling wait in the actual create call - but not the get caps call > which can be quick (as you said above). > > > > > If I915_PARAM_PXP_STATUS returning 2 has a reasonable chance in a > > production environment of eventually finding out that pxp can't work, > > then perhaps more disussion is needed. I guess the worst case scenario > > is no worse than today though. (Except it is still somewhat better, > > because the common case would not involve protected content being > > used by the application.) > alan: hmmm... i guess it depends on the meaning of resonable: if 50% > of the CI platforms being run have incorrect bios config / hw fusing > does it mean this UAPI is only 50% useful? My opinion is the alternative: > in the case of platform that has correctly configured BIOS + fusing, > is the chance of 2 eventually leading to a failure high? The answer is no. > Hypothetically i have actually never seen this happen (note: this UAPI > is new but i know from past debug of customer issues). There are some very > corner-cases but those get into the weeds of pxp runtime state machine.. > I am sure we don't wanna discuss that rabbit hole right now. > > > > Another option besides from the timeout loop in > > iris_create_hw_context() might be to check I915_PARAM_PXP_STATUS after > > the context create fails to tweak the debug message. > alan: Yeah, that is an option - I'm thinking we can add a DBG that reads > either"PXP context failure expected due not ready" vs > "Unexpected PXP context failure" > > > > > -Jordan >
Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP
On Tue, 2023-05-09 at 13:27 +, Teres Alexis, Alan Previn wrote: > > > alan:snip > > > > [Jordan:] > > > Another option besides from the timeout loop in > > > iris_create_hw_context() might be to check I915_PARAM_PXP_STATUS after > > > the context create fails to tweak the debug message. > > alan: Yeah, that is an option - I'm thinking we can add a DBG that reads > > either "PXP context failure expected due not ready" vs > > "Unexpected PXP context failure" > > Hi Jordan, should i proceed this way? (I can repost the UMD patch accordingly) MESA-patch proposal: https://gitlab.freedesktop.org/alan_previn_intel/mesa-alan-previn-features/-/commit/8728ab5a40c1a83f65afe0072f45a906ff26f706
Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP
> alan:snip > > [Jordan:] > > Another option besides from the timeout loop in > > iris_create_hw_context() might be to check I915_PARAM_PXP_STATUS after > > the context create fails to tweak the debug message. > alan: Yeah, that is an option - I'm thinking we can add a DBG that reads > either "PXP context failure expected due not ready" vs > "Unexpected PXP context failure" Hi Jordan, should i proceed this way? (I can repost the UMD patch accordingly)
Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP
On Fri, 2023-05-05 at 00:39 -0700, Justen, Jordan L wrote: > On 2023-05-04 22:30:07, Teres Alexis, Alan Previn wrote: > > On Thu, 2023-04-27 at 16:48 -0700, Teres Alexis, Alan Previn wrote: > > > Because of the additional firmware, component-driver and > > > initialization depedencies required on MTL platform before a > > > PXP context can be created, UMD calling for PXP creation as a > > > way to get-caps can take a long time. An actual real world > > > customer stack has seen this happen in the 4-to-8 second range > > > after the kernel starts (which sees MESA's init appear in the > > > middle of this range as the compositor comes up). To avoid > > > unncessary delays experienced by the UMD for get-caps purposes, > > > add a GET_PARAM for I915_PARAM_PXP_SUPPORT. > > > > > alan:snip. > > Progress update on the UMD side - I'm working on patch for PR here: > > https://gitlab.freedesktop.org/alan_previn_intel/mesa-alan-previn-features/-/commit/fb9d4fbfbef7dfd3f41df335cd31549fd39ddb57 > > but taking time to verify certain code paths > > Just to confirm, if I915_PARAM_PXP_STATUS returns 2 ("will be ready > soon"), then it is basically certain that in a production environment, > then it will eventually return 1 meaning it's ready, right? alan: yes - but not 100%. non-platform-state-machine could still cause unexpected failures for example, [1] if hw was fused in a way that doesnt permit PXP or [2] enabling certain BIOS debug knobs doesnt allow PXP. However at the moment, there is no way for us to know for sure without actually creating a protected context. Of course having hw-fusing + bios-config that supports PXP have always 100% succeeded for me. > > If this is correct, then I think that the change in > i915_gem_supports_protected_context() is good, and probably we can > skip the change in iris_create_hw_context(). > > Basically, we're timing out for multiple seconds either way. And, I'm > hoping that the kernel will eventually get the PXP init done and > create the context. > > I think there's 2 cases of interest here. > > First, and most likely, the application running doesn't care about > protected content. In this case we can quickly advertise the support, > but there will be no consequence because the application won't use the > feature. alan: yes - that was one of the goals of this UAPI. > > Second, the application does care about protected content. In this > case we can quickly advertise the support, but if the feature is used > too quickly, then the context create might take a long time. alan: no, that isnt the case now, we started at 8-secs, then 2-secs, and finally settled on the same timeout as ADL since this new UAPI will be something that can be polled on to be sure we have readiness to make the create-context-call. That's why i wanted to add the polling wait in the actual create call - but not the get caps call which can be quick (as you said above). > > If I915_PARAM_PXP_STATUS returning 2 has a reasonable chance in a > production environment of eventually finding out that pxp can't work, > then perhaps more disussion is needed. I guess the worst case scenario > is no worse than today though. (Except it is still somewhat better, > because the common case would not involve protected content being > used by the application.) alan: hmmm... i guess it depends on the meaning of resonable: if 50% of the CI platforms being run have incorrect bios config / hw fusing does it mean this UAPI is only 50% useful? My opinion is the alternative: in the case of platform that has correctly configured BIOS + fusing, is the chance of 2 eventually leading to a failure high? The answer is no. Hypothetically i have actually never seen this happen (note: this UAPI is new but i know from past debug of customer issues). There are some very corner-cases but those get into the weeds of pxp runtime state machine.. I am sure we don't wanna discuss that rabbit hole right now. > > Another option besides from the timeout loop in > iris_create_hw_context() might be to check I915_PARAM_PXP_STATUS after > the context create fails to tweak the debug message. alan: Yeah, that is an option - I'm thinking we can add a DBG that reads either"PXP context failure expected due not ready" vs "Unexpected PXP context failure" > > -Jordan
Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP
On 2023-05-04 22:30:07, Teres Alexis, Alan Previn wrote: > On Thu, 2023-04-27 at 16:48 -0700, Teres Alexis, Alan Previn wrote: > > Because of the additional firmware, component-driver and > > initialization depedencies required on MTL platform before a > > PXP context can be created, UMD calling for PXP creation as a > > way to get-caps can take a long time. An actual real world > > customer stack has seen this happen in the 4-to-8 second range > > after the kernel starts (which sees MESA's init appear in the > > middle of this range as the compositor comes up). To avoid > > unncessary delays experienced by the UMD for get-caps purposes, > > add a GET_PARAM for I915_PARAM_PXP_SUPPORT. > > > alan:snip. > Progress update on the UMD side - I'm working on patch for PR here: > https://gitlab.freedesktop.org/alan_previn_intel/mesa-alan-previn-features/-/commit/fb9d4fbfbef7dfd3f41df335cd31549fd39ddb57 > but taking time to verify certain code paths Just to confirm, if I915_PARAM_PXP_STATUS returns 2 ("will be ready soon"), then it is basically certain that in a production environment, then it will eventually return 1 meaning it's ready, right? If this is correct, then I think that the change in i915_gem_supports_protected_context() is good, and probably we can skip the change in iris_create_hw_context(). Basically, we're timing out for multiple seconds either way. And, I'm hoping that the kernel will eventually get the PXP init done and create the context. I think there's 2 cases of interest here. First, and most likely, the application running doesn't care about protected content. In this case we can quickly advertise the support, but there will be no consequence because the application won't use the feature. Second, the application does care about protected content. In this case we can quickly advertise the support, but if the feature is used too quickly, then the context create might take a long time. If I915_PARAM_PXP_STATUS returning 2 has a reasonable chance in a production environment of eventually finding out that pxp can't work, then perhaps more disussion is needed. I guess the worst case scenario is no worse than today though. (Except it is still somewhat better, because the common case would not involve protected content being used by the application.) Another option besides from the timeout loop in iris_create_hw_context() might be to check I915_PARAM_PXP_STATUS after the context create fails to tweak the debug message. -Jordan
Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP
On Thu, 2023-04-27 at 16:48 -0700, Teres Alexis, Alan Previn wrote: > Because of the additional firmware, component-driver and > initialization depedencies required on MTL platform before a > PXP context can be created, UMD calling for PXP creation as a > way to get-caps can take a long time. An actual real world > customer stack has seen this happen in the 4-to-8 second range > after the kernel starts (which sees MESA's init appear in the > middle of this range as the compositor comes up). To avoid > unncessary delays experienced by the UMD for get-caps purposes, > add a GET_PARAM for I915_PARAM_PXP_SUPPORT. > alan:snip. Progress update on the UMD side - I'm working on patch for PR here: https://gitlab.freedesktop.org/alan_previn_intel/mesa-alan-previn-features/-/commit/fb9d4fbfbef7dfd3f41df335cd31549fd39ddb57 but taking time to verify certain code paths