| Message ID | 20230406174419.471256-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 4/6/2023 10:44 AM, Alan Previn wrote: > 1. UAPI update: > Without actually changing backward compatible behavior, update > i915's drm-uapi comments that describe the possible error values > when creating a context with I915_CONTEXT_PARAM_PROTECTED_CONTENT. > Since the first merge of PXP support on ADL, i915 returns > -ENXIO if a dependency such as firmware or component driver > was yet to be loaded or returns -EIO if the creation attempt > failed when requested by the PXP firmware (specific firmware > error responses are reported in dmesg). > > 2. 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. > > 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 procsses > 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> > --- > drivers/gpu/drm/i915/i915_getparam.c | 5 +++++ > include/uapi/drm/i915_drm.h | 22 ++++++++++++++++++++++ > 2 files changed, 27 insertions(+) > > 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 dba7c5a5b25e..0c1729bd911d 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. Would it be useful to add: 1 = PXP support is available And start returning that after we've successfully created our first session? Not sure if userspace would use this though, since they still need to handle the 0 case anyway. I'm also ok with this patch as-is, as long as you get an ack from the userspace drivers for this interface behavior: Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Daniele > + */ > +#define I915_PARAM_PXP_STATUS 58 > + > /* Must be kept compact -- no holes and well documented */ > > /** > @@ -2096,6 +2110,14 @@ struct drm_i915_gem_context_param { > * > * -ENODEV: feature not available > * -EPERM: trying to mark a recoverable or not bannable context as protected > + * -ENXIO: A dependency such as a component driver or firmware is not yet > + * loaded and user space may attempt again. Depending on the device > + * this error may be reported if the protected context creation is > + * attempted very early from kernel start (numbers vary depending on > + * system and kernel config): > + * - ADL/RPL: up to 3 seconds > + * - MTL: up to 8 seconds > + * -EIO: The firmware did not succeed in creating the protected context. > */ > #define I915_CONTEXT_PARAM_PROTECTED_CONTENT 0xd > /* Must be kept compact -- no holes and well documented */
Hi Lionel, does this patch work for you? On Mon, 2023-04-10 at 10:22 -0700, Ceraolo Spurio, Daniele wrote: > On 4/6/2023 10:44 AM, Alan Previn wrote: alan:snip > > +/* > > + * 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. > > Would it be useful to add: > > 1 = PXP support is available > > And start returning that after we've successfully created our first > session? Not sure if userspace would use this though, since they still > need to handle the 0 case anyway. > I'm also ok with this patch as-is, as long as you get an ack from the > userspace drivers for this interface behavior: > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Daniele alan:snip
On 14/04/2023 18:17, Teres Alexis, Alan Previn wrote: > Hi Lionel, does this patch work for you? Hi, Sorry for the late answer. That looks good : Acked-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Thanks, -Lionel > > On Mon, 2023-04-10 at 10:22 -0700, Ceraolo Spurio, Daniele wrote: >> On 4/6/2023 10:44 AM, Alan Previn wrote: > alan:snip > >>> +/* >>> + * 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. >> Would it be useful to add: >> >> 1 = PXP support is available >> >> And start returning that after we've successfully created our first >> session? Not sure if userspace would use this though, since they still >> need to handle the 0 case anyway. >> I'm also ok with this patch as-is, as long as you get an ack from the >> userspace drivers for this interface behavior: >> >> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> >> Daniele > alan:snip >
On Tue, 2023-04-18 at 21:06 +0300, Landwerlin, Lionel G wrote: > On 14/04/2023 18:17, Teres Alexis, Alan Previn wrote: > > Hi Lionel, does this patch work for you? > > > Hi, Sorry for the late answer. > > That looks good : alan: Great - thanks Lionel! :) I'll help get the MESA side PR out in coming days. > > Acked-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > > Thanks, > > > -Lionel alan:snip
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 dba7c5a5b25e..0c1729bd911d 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 */ /** @@ -2096,6 +2110,14 @@ struct drm_i915_gem_context_param { * * -ENODEV: feature not available * -EPERM: trying to mark a recoverable or not bannable context as protected + * -ENXIO: A dependency such as a component driver or firmware is not yet + * loaded and user space may attempt again. Depending on the device + * this error may be reported if the protected context creation is + * attempted very early from kernel start (numbers vary depending on + * system and kernel config): + * - ADL/RPL: up to 3 seconds + * - MTL: up to 8 seconds + * -EIO: The firmware did not succeed in creating the protected context. */ #define I915_CONTEXT_PARAM_PROTECTED_CONTENT 0xd /* Must be kept compact -- no holes and well documented */
1. UAPI update: Without actually changing backward compatible behavior, update i915's drm-uapi comments that describe the possible error values when creating a context with I915_CONTEXT_PARAM_PROTECTED_CONTENT. Since the first merge of PXP support on ADL, i915 returns -ENXIO if a dependency such as firmware or component driver was yet to be loaded or returns -EIO if the creation attempt failed when requested by the PXP firmware (specific firmware error responses are reported in dmesg). 2. 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. 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 procsses 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> --- drivers/gpu/drm/i915/i915_getparam.c | 5 +++++ include/uapi/drm/i915_drm.h | 22 ++++++++++++++++++++++ 2 files changed, 27 insertions(+)