On Mon, 2023-04-17 at 12:15 -0700, Ceraolo Spurio, Daniele wrote:
> On 4/17/2023 11:21 AM, Teres Alexis, Alan Previn wrote:
> > On Mon, 2023-04-10 at 10:14 -0700, Ceraolo Spurio, Daniele wrote:
> > > > @@ -354,8 +368,14 @@ int intel_pxp_start(struct intel_pxp *pxp)
> > > > if (!intel_pxp_is_enabled(pxp))
> > > > return -ENODEV;
> > > >
> > > > - if (wait_for(pxp_component_bound(pxp), 250))
> > > > - return -ENXIO;
> > > > + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) {
> > > > + /* Use a larger 1 second timeout considering all the
> > > > dependencies. */
> > > > + if
> > > > (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 1000))
> > > > + return -ENXIO;
> > > This needs a comment to explain that we expect userspace to retry later
> > > if we return -ENXIO and therefore the timeout is smaller that what would
> > > be required to guarantee that the init can complete. It also needs an
> > > ack from the userspace drivers for this behavior.
> > >
> > alan: I agree with a requirement to comment this down. However, i believe
> > its better
> > to put this int the UAPI header file comment-documentation since it applies
> > to both
> > current MTL as well as previous ADL cases (this is not new behavior being
> > introduced
> > in MTL but it is fixing of the spec of existing behavior).
> > That said, your feedback is already being addressed by patch #6 but i will
> > ammend
> > patch #6 to add the detail you highlighted WRT ~"timeout is not larger than
> > the completion-guarantee...".
>
> Can you move that comment update for the context getparam from the next
> patch to this one? that way it's all nicely self-contained in this patch.
alan: sounds good - will do
alan:snip
> > > >
> > > > -#define GSC_REPLY_LATENCY_MS 200
> > > > +#define GSC_REPLY_LATENCY_MS 210
> > > why move from 200 to 210? not a problem, I just want to understand why
> > > this is required.
> > >
> > > Daniele
> > alan: so 200 is based on the fw spec - and from real testing i observed the
> > occasional highs touching ~185ms.
> > However, the spec gives this number as the expected max time the firmware
> > is going to take to process the request
> > and post a reply. Therefore it doesn't include the additional overhead for
> > the request creation, emision,
> > submission via guc and the completion pipeline completion indication. All
> > of these contribute additional layers
> > of possible delay. Since arb-session creation and invalidation are not
> > common events,
> > I believe a slightly wider overhead of 10 milisec will not hurt.
>
> Agreed. Can you add a comment? something like "Max FW response time is
> 200ms, to which we add 10ms to account for overhead".
alan: will do and thanks for the conditional rb. will fix these accordingly.
> With those 2 nits addressed:
>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com>
>
> Daniele
