Re: [Intel-gfx] [PATCH v4 12/17] drm/i915/pxp: start the arb session on demand

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 10, 2021 at 03:44:37PM -0700, Daniele Ceraolo Spurio wrote:
> 
> 
> On 6/2/2021 11:14 AM, Rodrigo Vivi wrote:
> > On Mon, May 24, 2021 at 10:47:58PM -0700, Daniele Ceraolo Spurio wrote:
> > > Now that we can handle destruction and re-creation of the arb session,
> > > we can postpone the start of the session to the first submission that
> > > requires it, to avoid keeping it running with no user.
> > > 
> > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > > ---
> > >   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  8 ++--
> > >   drivers/gpu/drm/i915/pxp/intel_pxp.c          | 37 ++++++++++++-------
> > >   drivers/gpu/drm/i915/pxp/intel_pxp.h          |  4 +-
> > >   drivers/gpu/drm/i915/pxp/intel_pxp_irq.c      |  2 +-
> > >   drivers/gpu/drm/i915/pxp/intel_pxp_session.c  |  6 +--
> > >   drivers/gpu/drm/i915/pxp/intel_pxp_tee.c      | 10 +----
> > >   drivers/gpu/drm/i915/pxp/intel_pxp_types.h    |  3 ++
> > >   7 files changed, 39 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > index a11e9d5767bf..c08e28847064 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > @@ -2948,9 +2948,11 @@ eb_select_engine(struct i915_execbuffer *eb)
> > >   	intel_gt_pm_get(ce->engine->gt);
> > >   	if (i915_gem_context_uses_protected_content(eb->gem_context)) {
> > > -		err = intel_pxp_wait_for_arb_start(&ce->engine->gt->pxp);
> > > -		if (err)
> > > -			goto err;
> > > +		if (!intel_pxp_is_active(&ce->engine->gt->pxp)) {
> > > +			err = intel_pxp_start(&ce->engine->gt->pxp);
> > > +			if (err)
> > > +				goto err;
> > > +		}
> > >   		if (i915_gem_context_invalidated(eb->gem_context)) {
> > >   			err = -EACCES;
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > index f713d3423cea..2291c68fd3a0 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > @@ -77,6 +77,7 @@ void intel_pxp_init(struct intel_pxp *pxp)
> > >   	init_completion(&pxp->termination);
> > >   	complete_all(&pxp->termination);
> > > +	mutex_init(&pxp->arb_mutex);
> > >   	INIT_WORK(&pxp->session_work, intel_pxp_session_work);
> > >   	ret = create_vcs_context(pxp);
> > > @@ -113,7 +114,7 @@ void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp)
> > >   	reinit_completion(&pxp->termination);
> > >   }
> > > -static void intel_pxp_queue_termination(struct intel_pxp *pxp)
> > > +static void pxp_queue_termination(struct intel_pxp *pxp)
> > >   {
> > >   	struct intel_gt *gt = pxp_to_gt(pxp);
> > > @@ -132,31 +133,41 @@ static void intel_pxp_queue_termination(struct intel_pxp *pxp)
> > >    * the arb session is restarted from the irq work when we receive the
> > >    * termination completion interrupt
> > >    */
> > > -int intel_pxp_wait_for_arb_start(struct intel_pxp *pxp)
> > > +int intel_pxp_start(struct intel_pxp *pxp)
> > >   {
> > > +	int ret = 0;
> > > +
> > >   	if (!intel_pxp_is_enabled(pxp))
> > > -		return 0;
> > > +		return -ENODEV;
> > > +
> > > +	mutex_lock(&pxp->arb_mutex);
> > > +
> > > +	if (pxp->arb_is_valid)
> > > +		goto unlock;
> > > +
> > > +	pxp_queue_termination(pxp);
> > >   	if (!wait_for_completion_timeout(&pxp->termination,
> > > -					 msecs_to_jiffies(100)))
> > > -		return -ETIMEDOUT;
> > > +					msecs_to_jiffies(100))) {
> > > +		ret = -ETIMEDOUT;
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	/* make sure the compiler doesn't optimize the double access */
> > > +	barrier();
> > >   	if (!pxp->arb_is_valid)
> > > -		return -EIO;
> > > +		ret = -EIO;
> > > -	return 0;
> > > +unlock:
> > > +	mutex_unlock(&pxp->arb_mutex);
> > > +	return ret;
> > >   }
> > >   void intel_pxp_init_hw(struct intel_pxp *pxp)
> > >   {
> > >   	kcr_pxp_enable(pxp_to_gt(pxp));
> > >   	intel_pxp_irq_enable(pxp);
> > > -
> > > -	/*
> > > -	 * the session could've been attacked while we weren't loaded, so
> > > -	 * handle it as if it was and re-create it.
> > > -	 */
> > > -	intel_pxp_queue_termination(pxp);
> > >   }
> > >   void intel_pxp_fini_hw(struct intel_pxp *pxp)
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > > index 91c1a2056309..1f9871e64096 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > > @@ -32,7 +32,7 @@ void intel_pxp_init_hw(struct intel_pxp *pxp);
> > >   void intel_pxp_fini_hw(struct intel_pxp *pxp);
> > >   void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
> > > -int intel_pxp_wait_for_arb_start(struct intel_pxp *pxp);
> > > +int intel_pxp_start(struct intel_pxp *pxp);
> > >   void intel_pxp_invalidate(struct intel_pxp *pxp);
> > >   #else
> > >   static inline void intel_pxp_init(struct intel_pxp *pxp)
> > > @@ -43,7 +43,7 @@ static inline void intel_pxp_fini(struct intel_pxp *pxp)
> > >   {
> > >   }
> > > -static inline int intel_pxp_wait_for_arb_start(struct intel_pxp *pxp)
> > > +static inline int intel_pxp_start(struct intel_pxp *pxp)
> > >   {
> > >   	return 0;
> > >   }
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > > index 196449243515..a230d0034e50 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > > @@ -31,7 +31,7 @@ void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
> > >   		   GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT)) {
> > >   		/* immediately mark PXP as inactive on termination */
> > >   		intel_pxp_mark_termination_in_progress(pxp);
> > > -		pxp->session_events |= PXP_TERMINATION_REQUEST;
> > > +		pxp->session_events |= PXP_TERMINATION_REQUEST | PXP_INVAL_REQUIRED;
> > >   	}
> > >   	if (iir & GEN12_DISPLAY_STATE_RESET_COMPLETE_INTERRUPT)
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > > index e9fe757e368a..c21620916710 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > > @@ -85,9 +85,6 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
> > >   	/* must mark termination in progress calling this function */
> > >   	GEM_WARN_ON(pxp->arb_is_valid);
> > > -	/* invalidate protected objects */
> > > -	intel_pxp_invalidate(pxp);
> > > -
> > >   	/* terminate the hw sessions */
> > >   	ret = intel_pxp_terminate_session(pxp, ARB_SESSION);
> > >   	if (ret) {
> > > @@ -144,6 +141,9 @@ void intel_pxp_session_work(struct work_struct *work)
> > >   	if (!events)
> > >   		return;
> > > +	if (events & PXP_INVAL_REQUIRED)
> > > +		intel_pxp_invalidate(pxp);
> > > +
> > doesn't this invalidation change deserves a separated patch?
> > I'm not sure if I understood why we need this change...
> 
> Before this patch, we always did a full invalidation and an arb restart

oh, indeed, I had missed that part, sorry

> back-to-back and therefore the invalidation was implicit in the
> PXP_TERMINATION_REQUEST flag. This patch changes this into different cases:
> 
> termination irq: invalidate objects, submit termination and restart the arb
> session
> suspend: invalidate objects
> first pxp submission: submit termination and start the arb session
> 
> Therefore we need to be able to select the invalidation as an independent
> case from the termination.

Thanks for the explanation, it makes sense


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>


> 
> Daniele
> 
> 
> > 
> > >   	if (events & PXP_TERMINATION_REQUEST) {
> > >   		events &= ~PXP_TERMINATION_COMPLETE;
> > >   		pxp_terminate(pxp);
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > index e3d178c44e51..35b3fed4ca2f 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > @@ -63,23 +63,15 @@ static int intel_pxp_tee_io_message(struct intel_pxp *pxp,
> > >   static int i915_pxp_tee_component_bind(struct device *i915_kdev,
> > >   				       struct device *tee_kdev, void *data)
> > >   {
> > > -	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
> > >   	struct intel_pxp *pxp = i915_dev_to_pxp(i915_kdev);
> > > -	int ret;
> > >   	pxp->pxp_component = data;
> > >   	pxp->pxp_component->tee_dev = tee_kdev;
> > >   	/* the component is required to fully start the PXP HW */
> > >   	intel_pxp_init_hw(pxp);
> > > -	ret = intel_pxp_wait_for_arb_start(pxp);
> > > -	if (ret) {
> > > -		drm_err(&i915->drm, "Failed to create arb session during bind\n");
> > > -		intel_pxp_fini_hw(pxp);
> > > -		pxp->pxp_component = NULL;
> > > -	}
> > > -	return ret;
> > > +	return 0;
> > >   }
> > >   static void i915_pxp_tee_component_unbind(struct device *i915_kdev,
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> > > index c059a17cbcfe..b3ae49dd73a8 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> > > @@ -7,6 +7,7 @@
> > >   #define __INTEL_PXP_TYPES_H__
> > >   #include <linux/completion.h>
> > > +#include <linux/mutex.h>
> > >   #include <linux/types.h>
> > >   #include <linux/workqueue.h>
> > > @@ -23,6 +24,7 @@ struct intel_pxp {
> > >   	 * even if the keys are gone, so we can't rely on the HW state of the
> > >   	 * session to know if it's valid and need to track the status in SW.
> > >   	 */
> > > +	struct mutex arb_mutex; /* protects arb session start */
> > >   	bool arb_is_valid;
> > >   	/*
> > > @@ -40,6 +42,7 @@ struct intel_pxp {
> > >   	u32 session_events; /* protected with gt->irq_lock */
> > >   #define PXP_TERMINATION_REQUEST  BIT(0)
> > >   #define PXP_TERMINATION_COMPLETE BIT(1)
> > > +#define PXP_INVAL_REQUIRED       BIT(2)
> > >   };
> > >   #endif /* __INTEL_PXP_TYPES_H__ */
> > > -- 
> > > 2.29.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux