| Message ID | 20230421053410.1836241-7-alan.previn.teres.alexis@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pxp: Add MTL PXP Support | expand |
On Thu, 2023-04-20 at 22:34 -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. > > However, some failures can still occur after all the depedencies > are met (such as firmware init flow failure, bios configurations > or SOC fusing not allowing PXP enablement). Those scenarios will > only be known to user space when it attempts creating a PXP context. > > With this change, large delays are only met by user-space processes > that explicitly need to create a PXP context and boot very early. > There is no way to avoid this today. > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Acked-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- alan: Hi Jordan, Tvrtko, Daniel Vetter and Lionel,... how to proceed on this series (which have required Rb/Ack's) in light of the recent discussion on the other series here: https://patchwork.freedesktop.org/patch/532994/?series=115980&rev=8 it sounds like: - Jordan still wants the extension query - Daniel recapped the overview of historical discussions and kernel UAPI guidelines and in summary is okay with the GET_PARAM option. However Daniel cites PXP taking 8 seconds to create a context is broken. - I corrected above assumption -> current timeout is 1 second. 8 seconds is not the time taken to init the context, its the max-possible-time to ensure dependencies like the gsc-proxy-component is loaded so that protected context could be attempted successfully. Else, it would return an error that Mesa could interpret as not supported. Should I: (a) rerev this, drop this patch #6 and work towards merging the rest of the series to allow the get-param vs extensions-query to continue independently? (b) go ahead and merge this series as is with the GET_PARAM? (i need to get the Mesa UMD change tested and PR requested first) alan:snip
On 2023-04-26 11:17:16, Teres Alexis, Alan Previn wrote: > alan: Hi Jordan, Tvrtko, Daniel Vetter and Lionel,... > how to proceed on this series (which have required Rb/Ack's) in light of > the recent discussion on the other series here: > https://patchwork.freedesktop.org/patch/532994/?series=115980&rev=8 > it sounds like: > - Jordan still wants the extension query Yes, I do, but so far it doesn't appear that any kernel devs think it's a reasonable request. As I read through your emails about this pxp situation, it seems like a separate issue. When I encountered the 8s delay, it was on MTL, and apparently some firmware issue meant it was never going to work. So, I thought this was a case of it either being supported, or never being supported. Now I'm seeing from your emails that in some cases it might be supported, but just not ready yet. In that case a status which is directly tied to pxp seems valuable. (But, yuck, how did we get into this situation? :) Can you tell that pxp is in progress, but not ready yet, as a separate state from 'it will never work on this platform'? If so, maybe the status could return something like: 0: It's never going to work 1: It's ready to use 2: It's starting and should work soon I could see an argument for treating that as a case where we could still advertise protected content support, but if we try to use it we might be in for a nasty delay. Maybe Lionel would have a different opinion on whether it would be a good idea to go this route. Regarding the extensions list I was requesting, it might be easiest for the kernel if it just replies with all the extensions it knows about regardless of whether they are usable right now. -Jordan
On Wed, 2023-04-26 at 15:35 -0700, Justen, Jordan L wrote: > On 2023-04-26 11:17:16, Teres Alexis, Alan Previn wrote: alan:snip > > > - Jordan still wants the extension query > Yes, I do, but so far it doesn't appear that any kernel devs think > it's a reasonable request. > > As I read through your emails about this pxp situation, it seems like > a separate issue. When I encountered the 8s delay, it was on MTL, and > apparently some firmware issue meant it was never going to work. So, I > thought this was a case of it either being supported, or never being > supported. alan: this might be because of an older patch version in internal tree - which I'm trying hard to fix to follow latest upstream - but keep getting delays and conflicts - but thats unrelated to this upstream POV > Now I'm seeing from your emails that in some cases it might be > supported, but just not ready yet. In that case a status which is > directly tied to pxp seems valuable. (But, yuck, how did we get into > this situation? :) alan: thanks for the go ahead on this PXP's uniquely different-issue (and i must agree, 'yukcy' situation). How did we get here? we are trying to debug this - its interesting to see the same kernel with the same configs much faster on ADL vs MTL but the MTL case is suffering because the mei-heci-parent driver is getting loaded much later (which IS common to ADL) - this delayed parent driver is causing the delay on the gsc-proxy component MTL. From parent load to gsc-proxy bin+init is relatively fast (~few hundred milisecs). But I believe it seems to only be happenning on select OS stacks (our ChromeOS fork is definitely seeing this). > Can you tell that pxp is in progress, but not ready yet, as a separate > state from 'it will never work on this platform'? If so, maybe the > status could return something like: > > 0: It's never going to work > 1: It's ready to use > 2: It's starting and should work soon > > I could see an argument for treating that as a case where we could > still advertise protected content support, but if we try to use it we > might be in for a nasty delay. > alan: IIRC Lionel seemed okay with any permutation that would allow it to not get blocked. Daniele did ask for something similiar to what u mentioned above but he said that is non-blocking. But since both you AND Daniele have mentioned the same thing, i shall re-rev this and send that change out today. I notice most GET_PARAMS use -ENODEV for "never gonna work" so I will stick with that. but 1 = ready to use and 2 = starting and should work sounds good. so '0' will never be returned - we just look for a positive value (from user space). I will also make a PR for mesa side as soon as i get it tested. thanks for reviewing btw. alan:snip
(fixed email addresses again - why is my Evolution client deteorating??) On Thu, 2023-04-27 at 17:18 +0000, Teres Alexis, Alan Previn wrote: > On Wed, 2023-04-26 at 15:35 -0700, Justen, Jordan L wrote: > > On 2023-04-26 11:17:16, Teres Alexis, Alan Previn wrote: > alan:snip > > Can you tell that pxp is in progress, but not ready yet, as a separate > > state from 'it will never work on this platform'? If so, maybe the > > status could return something like: > > > > 0: It's never going to work > > 1: It's ready to use > > 2: It's starting and should work soon > > > > I could see an argument for treating that as a case where we could > > still advertise protected content support, but if we try to use it we > > might be in for a nasty delay. > > > alan: IIRC Lionel seemed okay with any permutation that would allow it to not > get blocked. Daniele did ask for something similiar to what u mentioned above > but he said that is non-blocking. But since both you AND Daniele have mentioned > the same thing, i shall re-rev this and send that change out today. > I notice most GET_PARAMS use -ENODEV for "never gonna work" so I will stick with that. > but 1 = ready to use and 2 = starting and should work sounds good. so '0' will never > be returned - we just look for a positive value (from user space). I will also > make a PR for mesa side as soon as i get it tested. thanks for reviewing btw. alan: I also realize with these final touch-ups, we can go back to the original pxp-context-creation timeout of 250 milisecs like it was on ADL since the user space component will have this new param to check on (so even farther down from 1 sec on the last couple of revs). Jordan, Lional - i am thinking of creating the PR on MESA side to take advantage of GET_PARAM on both get-caps AND runtime creation (latter will be useful to ensure no unnecesssary delay experienced by Mesa stuck in kernel call - which practically never happenned in ADL AFAIK): 1. MESA PXP get caps: - use GET_PARAM (any positive number shall mean its supported). 2. MESA app-triggered PXP context creation (i.e. if caps was supported): - use GET_PARAM to wait until positive number switches from "2" to "1". - now call context creation. So at this point if it fails, we know its an actual failure. you guys okay with above? (i'll re-rev this kernel series first and wait on your ack or feedback before i create/ test/ submit a PR for Mesa side).
On 27/04/2023 21:19, Teres Alexis, Alan Previn wrote: > (fixed email addresses again - why is my Evolution client deteorating??) > > On Thu, 2023-04-27 at 17:18 +0000, Teres Alexis, Alan Previn wrote: >> On Wed, 2023-04-26 at 15:35 -0700, Justen, Jordan L wrote: >>> On 2023-04-26 11:17:16, Teres Alexis, Alan Previn wrote: >> alan:snip >>> Can you tell that pxp is in progress, but not ready yet, as a separate >>> state from 'it will never work on this platform'? If so, maybe the >>> status could return something like: >>> >>> 0: It's never going to work >>> 1: It's ready to use >>> 2: It's starting and should work soon >>> >>> I could see an argument for treating that as a case where we could >>> still advertise protected content support, but if we try to use it we >>> might be in for a nasty delay. >>> >> alan: IIRC Lionel seemed okay with any permutation that would allow it to not >> get blocked. Daniele did ask for something similiar to what u mentioned above >> but he said that is non-blocking. But since both you AND Daniele have mentioned >> the same thing, i shall re-rev this and send that change out today. >> I notice most GET_PARAMS use -ENODEV for "never gonna work" so I will stick with that. >> but 1 = ready to use and 2 = starting and should work sounds good. so '0' will never >> be returned - we just look for a positive value (from user space). I will also >> make a PR for mesa side as soon as i get it tested. thanks for reviewing btw. > alan: I also realize with these final touch-ups, we can go back to the original > pxp-context-creation timeout of 250 milisecs like it was on ADL since the user > space component will have this new param to check on (so even farther down from > 1 sec on the last couple of revs). > > Jordan, Lional - i am thinking of creating the PR on MESA side to take advantage > of GET_PARAM on both get-caps AND runtime creation (latter will be useful to ensure > no unnecesssary delay experienced by Mesa stuck in kernel call - which practically > never happenned in ADL AFAIK): > > 1. MESA PXP get caps: > - use GET_PARAM (any positive number shall mean its supported). > 2. MESA app-triggered PXP context creation (i.e. if caps was supported): > - use GET_PARAM to wait until positive number switches from "2" to "1". > - now call context creation. So at this point if it fails, we know its > an actual failure. > > you guys okay with above? (i'll re-rev this kernel series first and wait on your > ack or feedback before i create/ test/ submit a PR for Mesa side). > Sounds good. Thanks, -Lionel
diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c index 2238e096c957..9729384f033f 100644 --- a/drivers/gpu/drm/i915/i915_getparam.c +++ b/drivers/gpu/drm/i915/i915_getparam.c @@ -5,6 +5,8 @@ #include "gem/i915_gem_mman.h" #include "gt/intel_engine_user.h" +#include "pxp/intel_pxp.h" + #include "i915_cmd_parser.h" #include "i915_drv.h" #include "i915_getparam.h" @@ -102,6 +104,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, if (value < 0) return value; break; + case I915_PARAM_PXP_STATUS: + value = intel_pxp_is_enabled(i915->pxp) ? 0 : -ENODEV; + break; case I915_PARAM_MMAP_GTT_VERSION: /* Though we've started our numbering from 1, and so class all * earlier versions as 0, in effect their value is undefined as diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index b15dd9cc2ffb..e21991cd6c3d 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -771,6 +771,20 @@ typedef struct drm_i915_irq_wait { */ #define I915_PARAM_OA_TIMESTAMP_FREQUENCY 57 +/* + * Query the status of PXP support in i915. + * + * The query can fail in the following scenarios with the listed error codes: + * -ENODEV = PXP support is not available on the GPU device or in the kernel + * due to missing component drivers or kernel configs. + * If the IOCTL is successful, the returned parameter will be set to one of the + * following values: + * 0 = PXP support maybe available but underlying SOC fusing, BIOS or firmware + * configuration is unknown and a PXP-context-creation would be required + * for final verification of feature availibility. + */ +#define I915_PARAM_PXP_STATUS 58 + /* Must be kept compact -- no holes and well documented */ /**