[Intel-gfx] [RFC-v1 02/16] drm/i915/pxp: Enable PXP irq worker and callback stub

Tue Dec 8 00:34:16 UTC 2020

Hi Joonas,

All the modification will be included in rev2. Thanks!

> We should take &gt->pxp as the first parameter and keep a tight scope.
DONE

> Again, we should be taking intel_pxp as parameter to tighten the scope.
DONE

>Instead of writing to register that is indicated to be "reserved" (RSVD), we should properly document the register in i915_reg.h and the comment should not be needed.
DONE, remove RSVD from its name and add document in i915_reg.h

> This INFO message is wrongly placed, either it should say "initializing" and be at the beginning or "initialized" and be at the end. Also see previous patch for more comments.
DONE, 

>_ptr is a tautology, we can already see it's apointer.
DONE, remove the "_ptr"

> We should go from intel_pxp to intel_gt to i915 here, once the struct intel_pxp member is moved inside intel_gt
DONE, yes now pxp is in intel_gt

>The mapping between the callbacks and the hardware events are unclear.
>These all seem like display related events, so we probably need a split between the GT and display PXP code.
>It's hard to review as this only adds stubs and no actual flow. I think teardown interrupt handling should come later in the series after init and other code has been added.
Personally I would still prefer to put this in gt instead of display, because when teardown happen it means the "crypto engine" (This should belong to gt) has loss the key for display.

>This INFO message is wrongly placed, either it should say "initializing"
>and be at the beginning or "initialized" and be at the end. Also see previous patch for more comments.
DONE, change the log as "Protected Xe Path (PXP) protected content support initialized " at the end.

>This enum here feels like a misplaced hunk. We should be adding the enums and structs only when they are used in the patch. Reviewing the logic and looking for dead code is much harder when structs are introduced way earlier than they are used.
>We should be adding the base structs at most and extending them as we add more functionality as we go.
DONE, Thanks for capturing this, this enum is for session management only and I have removed this.

-----Original Message-----
From: Joonas Lahtinen <joonas.lahtinen at linux.intel.com> 
Sent: Monday, December 7, 2020 2:21 AM
To: Huang, Sean Z <sean.z.huang at intel.com>; Intel-gfx at lists.freedesktop.org
Subject: Re: [Intel-gfx] [RFC-v1 02/16] drm/i915/pxp: Enable PXP irq worker and callback stub

Quoting Huang, Sean Z (2020-12-07 02:21:20)
> Create the irq worker that serves as callback handler, those callback 
> stubs should be called while the hardware key teardown occurs.
> 
> Signed-off-by: Huang, Sean Z <sean.z.huang at intel.com>

<SNIP>

> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> @@ -13,6 +13,7 @@
>  #include "intel_gt_irq.h"
>  #include "intel_uncore.h"
>  #include "intel_rps.h"
> +#include "pxp/intel_pxp.h"
>  
>  static void guc_irq_handler(struct intel_guc *guc, u16 iir)  { @@ 
> -106,6 +107,9 @@ gen11_other_irq_handler(struct intel_gt *gt, const u8 instance,
>         if (instance == OTHER_GTPM_INSTANCE)
>                 return gen11_rps_irq_handler(&gt->rps, iir);
>  
> +       if (instance == OTHER_KCR_INSTANCE)
> +               return intel_pxp_irq_handler(gt, iir);

We should take &gt->pxp as the first parameter and keep a tight scope.

> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -6,6 +6,58 @@
>  #include "i915_drv.h"
>  #include "intel_pxp.h"
>  
> +static void intel_pxp_write_irq_mask_reg(struct drm_i915_private 
> +*i915, u32 mask)

Again, we should be taking intel_pxp as parameter to tighten the scope.

> +{
> +       /* crypto mask is in bit31-16 (Engine1 Interrupt Mask) */
> +       intel_uncore_write(&i915->uncore, GEN11_CRYPTO_RSVD_INTR_MASK, 
> +mask << 16);

Instead of writing to register that is indicated to be "reserved"
(RSVD), we should properly document the register in i915_reg.h and the comment should not be needed.

> +static void intel_pxp_irq_work(struct work_struct *work) {
> +       struct intel_pxp *pxp_ptr = container_of(work, 
> +typeof(*pxp_ptr), irq_work);

_ptr is a tautology, we can already see it's apointer.

> +       struct drm_i915_private *i915 = container_of(pxp_ptr, 
> + typeof(*i915), pxp);

We should go from intel_pxp to intel_gt to i915 here, once the struct intel_pxp member is moved inside intel_gt

> +       u32 events = 0;
> +
> +       spin_lock_irq(&i915->gt.irq_lock);
> +       events = fetch_and_zero(&pxp_ptr->current_events);
> +       spin_unlock_irq(&i915->gt.irq_lock);
> +
> +       if (events & PXP_IRQ_VECTOR_DISPLAY_PXP_STATE_TERMINATED ||
> +           events & PXP_IRQ_VECTOR_DISPLAY_APP_TERM_PER_FW_REQ)
> +               intel_pxp_teardown_required_callback(i915);
> +
> +       if (events & PXP_IRQ_VECTOR_PXP_DISP_STATE_RESET_COMPLETE)
> +               intel_pxp_global_terminate_complete_callback(i915);

The mapping between the callbacks and the hardware events are unclear.
These all seem like display related events, so we probably need a split between the GT and display PXP code.

It's hard to review as this only adds stubs and no actual flow. I think teardown interrupt handling should come later in the series after init and other code has been added.

> @@ -17,9 +69,45 @@ int intel_pxp_init(struct drm_i915_private *i915)
>  
>         drm_info(&i915->drm, "i915 PXP is inited with i915=[%p]\n", 
> i915);

This INFO message is wrongly placed, either it should say "initializing"
and be at the beginning or "initialized" and be at the end. Also see previous patch for more comments.

> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -8,18 +8,54 @@
>  
>  #include <drm/drm_file.h>
>  
> +#define PXP_IRQ_VECTOR_DISPLAY_PXP_STATE_TERMINATED BIT(1) #define 
> +PXP_IRQ_VECTOR_DISPLAY_APP_TERM_PER_FW_REQ BIT(2) #define 
> +PXP_IRQ_VECTOR_PXP_DISP_STATE_RESET_COMPLETE BIT(3)
> +
> +enum pxp_sm_session_req {
> +       /* Request KMD to allocate session id and move it to IN INIT */
> +       PXP_SM_REQ_SESSION_ID_INIT = 0x0,
> +       /* Inform KMD that UMD has completed the initialization */
> +       PXP_SM_REQ_SESSION_IN_PLAY,
> +       /* Request KMD to terminate the session */
> +       PXP_SM_REQ_SESSION_TERMINATE
> +};

This enum here feels like a misplaced hunk. We should be adding the enums and structs only when they are used in the patch. Reviewing the logic and looking for dead code is much harder when structs are introduced way earlier than they are used.

We should be adding the base structs at most and extending them as we add more functionality as we go.

Regards, Joonas