[v8,6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP

Commit Message

Alan Previn April 21, 2023, 5:34 a.m. UTC

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 <[e-mail]>
Reviewed-by: Daniele Ceraolo Spurio <[e-mail]>
Acked-by: Lionel Landwerlin <[e-mail]>
---
 drivers/gpu/drm/i915/i915_getparam.c |  5 +++++
 include/uapi/drm/i915_drm.h          | 14 ++++++++++++++
 2 files changed, 19 insertions(+)

Comments

Alan Previn April 26, 2023, 6:17 p.m. UTC | #1

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 <[e-mail]>
> Reviewed-by: Daniele Ceraolo Spurio <[e-mail]>
> Acked-by: Lionel Landwerlin <[e-mail]>
> ---
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

Jordan Justen April 26, 2023, 10:35 p.m. UTC | #2

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

Alan Previn April 27, 2023, 6:19 p.m. UTC | #3

(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).

Lionel Landwerlin April 27, 2023, 6:24 p.m. UTC | #4

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

Patch

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 */
 
 /**