Re: [Intel-gfx] [PXP CLEAN PATCH v06 02/27] drm/i915/pxp: Enable PXP irq worker and callback stub

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 16 Nov 2020, "Souza, Jose" <jose.souza@intel.com> wrote:
> On Fri, 2020-11-13 at 16:36 -0800, Sean Z Huang wrote:
>> From: "Huang, Sean Z" <sean.z.huang@intel.com>
>> 
>> 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@intel.com>
>> ---
>>  drivers/gpu/drm/i915/gt/intel_gt_irq.c |  4 ++
>>  drivers/gpu/drm/i915/i915_reg.h        |  1 +
>>  drivers/gpu/drm/i915/pxp/intel_pxp.c   | 95 ++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/pxp/intel_pxp.h   | 22 ++++++
>>  4 files changed, 122 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
>> index 257063a57101..d64013d0afb5 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
>> +++ 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);
>> +
>>  	WARN_ONCE(1, "unhandled other interrupt instance=0x%x, iir=0x%x\n",
>>  		  instance, iir);
>>  }
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 7ea70b7ffcc6..faf6b06145fa 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7941,6 +7941,7 @@ enum {
>>  /* irq instances for OTHER_CLASS */
>>  #define OTHER_GUC_INSTANCE	0
>>  #define OTHER_GTPM_INSTANCE	1
>> +#define OTHER_KCR_INSTANCE	4
>>  
>> 
>> 
>> 
>>  #define GEN11_INTR_IDENTITY_REG(x)	_MMIO(0x190060 + ((x) * 4))
>>  
>> 
>> 
>> 
>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> index a469c55e3e54..d98bff4a0fde 100644
>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> @@ -6,15 +6,110 @@
>>  #include "i915_drv.h"
>>  #include "intel_pxp.h"
>>  
>> 
>> 
>> 
>> +static void intel_pxp_write_irq_mask_reg(struct drm_i915_private *i915, u32 mask)
>> +{
>> +	WARN_ON(INTEL_GEN(i915) < 11);
>
>
> should be drm_warn_on(), also I don't see any value in this warning.
>
>> +
>> +	/* crypto mask is in bit31-16 (Engine1 Interrupt Mask) */
>> +	intel_uncore_write(&i915->uncore, GEN11_CRYPTO_RSVD_INTR_MASK, mask << 16);
>> +}
>> +
>> +static void intel_pxp_unmask_irq(struct intel_gt *gt)
>> +{
>> +	lockdep_assert_held(&gt->irq_lock);
>> +
>> +	intel_pxp_write_irq_mask_reg(gt->i915, 0);
>> +}
>> +
>> +static void intel_pxp_mask_irq(struct intel_gt *gt, u32 mask)
>> +{
>> +	lockdep_assert_held(&gt->irq_lock);
>> +
>> +	intel_pxp_write_irq_mask_reg(gt->i915, mask);
>> +}
>> +
>> +static int intel_pxp_teardown_required_callback(struct drm_i915_private *i915)
>> +{
>> +	drm_dbg(&i915->drm, "%s was called\n", __func__);
>
> Looks like something used for debug, should not be in the upstream version.
> Saw a lot more of debug like this that should be removed.

Absolutely!

>
>> +
>> +	return 0;
>> +}
>> +
>> +static int intel_pxp_global_terminate_complete_callback(struct drm_i915_private *i915)
>> +{
>> +	drm_dbg(&i915->drm, ">>> %s\n", __func__);
>> +
>> +	return 0;
>> +}
>> +
>> +static void intel_pxp_irq_work(struct work_struct *work)
>> +{
>> +	struct intel_pxp *pxp_ptr = container_of(work, typeof(*pxp_ptr), irq_work);
>> +	struct drm_i915_private *i915 = container_of(pxp_ptr, typeof(*i915), pxp);
>> +	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);
>> +
>> +	drm_dbg(&i915->drm, "%s was called with events=[%d]\n", __func__, events);
>> +
>> +	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);
>> +
>> +	spin_lock_irq(&i915->gt.irq_lock);
>> +	intel_pxp_unmask_irq(&i915->gt);
>> +	spin_unlock_irq(&i915->gt.irq_lock);
>> +}
>> +
>>  int intel_pxp_init(struct drm_i915_private *i915)
>>  {
>>  	int ret;
>>  
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>>  	drm_info(&i915->drm, "i915_pxp_init\n");
>>  
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> +	INIT_WORK(&i915->pxp.irq_work, intel_pxp_irq_work);
>> +
>> +	i915->pxp.handled_irr = (PXP_IRQ_VECTOR_DISPLAY_PXP_STATE_TERMINATED |
>> +				 PXP_IRQ_VECTOR_DISPLAY_APP_TERM_PER_FW_REQ |
>> +				 PXP_IRQ_VECTOR_PXP_DISP_STATE_RESET_COMPLETE);
>> +
>>  	return ret;
>>  }
>>  
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>>  void intel_pxp_uninit(struct drm_i915_private *i915)
>>  {
>>  }
>> +
>> +/**
>> + * intel_pxp_irq_handler - Proxies KCR interrupts to PXP.
>> + * @gt: valid GT instance
>> + * @iir: GT interrupt vector associated with the interrupt
>> + *
>> + * Dispatches each vector element into an IRQ to PXP.
>> + */
>> +void intel_pxp_irq_handler(struct intel_gt *gt, u16 iir)
>> +{
>> +	struct drm_i915_private *i915 = gt->i915;
>> +	const u32 events = iir & i915->pxp.handled_irr;
>> +
>> +	drm_dbg(&i915->drm, "%s was called with iir=[0x%04x]\n", __func__, iir);
>> +
>> +	lockdep_assert_held(&gt->irq_lock);
>> +
>> +	if (unlikely(!events)) {
>> +		drm_dbg(&i915->drm, "%s returned due to iir=[0x%04x]\n", __func__, iir);
>> +		goto end;

Please just return instead of goto.

>> +	}
>> +
>> +	intel_pxp_mask_irq(gt, i915->pxp.handled_irr);
>> +
>> +	i915->pxp.current_events |= events;
>> +	schedule_work(&i915->pxp.irq_work);
>> +end:
>> +	return;

No need for return in the end of void functions.

>> +}
>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
>> index 578f1126bada..620774fc32e2 100644
>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
>> @@ -8,14 +8,36 @@
>>  
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>>  #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
>> +};
>> +
>>  struct pxp_context;
>>  
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>>  struct intel_pxp {
>> +	struct work_struct irq_work;
>> +	u32 handled_irr;
>> +	u32 current_events;
>> +
>>  	struct pxp_context *r0ctx;
>>  };
>>  
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> +struct intel_gt;
>>  struct drm_i915_private;
>>  
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> +void intel_pxp_irq_handler(struct intel_gt *gt, u16 iir);
>> +int i915_pxp_teardown_required_callback(struct drm_i915_private *i915);
>> +int i915_pxp_global_terminate_complete_callback(struct drm_i915_private *i915);
>> +
>>  int intel_pxp_init(struct drm_i915_private *i915);
>>  void intel_pxp_uninit(struct drm_i915_private *i915);
>>  
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx