[Intel-gfx] [PATCH 06/11] drm/i915/psr: Add intel_psr_activate_block_get()/put()

[Rodrigo Vivi]

On Mon, Apr 02, 2018 at 03:11:54PM -0700, Souza, Jose wrote:
> On Mon, 2018-04-02 at 11:20 -0700, Rodrigo Vivi wrote:
> > On Fri, Mar 30, 2018 at 03:23:31PM -0700, José Roberto de Souza
> > wrote:
> > > intel_psr_activate_block_get() should be called when by some reason
> > > PSR should not be activated for some time, it will increment
> > > counter
> > > and it should the decremented by intel_psr_activate_block_put()
> > > when PSR can be activated again.
> > > intel_psr_activate_block_put() will not actually activate PSR,
> > > users
> > > of this function should also call intel_psr_activate().
> > 
> > Ohh cool! you made the counter.
> > probably we will need to change things from mutex to spin locker.
> > But also the blocker functions here could already introduce the
> > function
> > calls to really block and release psr.
> 
> Oh so drop the 'drm/i915/psr: Export intel_psr_activate/exit()' and
> call PSR exit and activate from intel_psr_activate_block_get()/put()?

yeap.

> 
> I dind't understand why you want to change struct mutex lock; to
> spinlock_t?

I'm not sure, but I believe that aux transactions can be called from
atomic areas protected with spin locks... if this assumption is true
than you cannot use any code that can sleep inside these areas, and
mutex can sleep. But in case no aux transaction is getting called
from atomic areas feel free to just ignore me ;)

> 
> > 
> > > 
> > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  |  1 +
> > >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> > >  drivers/gpu/drm/i915/intel_psr.c | 54
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 57 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 99af9169d792..41ebb144594e 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -609,6 +609,7 @@ struct i915_psr {
> > >  	bool has_hw_tracking;
> > >  	bool psr2_enabled;
> > >  	u8 sink_sync_latency;
> > > +	unsigned int activate_block_count;
> > >  
> > >  	void (*enable_source)(struct intel_dp *,
> > >  			      const struct intel_crtc_state *);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 70026b772721..020b96324135 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1893,6 +1893,8 @@ void intel_psr_compute_config(struct intel_dp
> > > *intel_dp,
> > >  			      struct intel_crtc_state
> > > *crtc_state);
> > >  void intel_psr_exit(struct intel_dp *intel_dp, bool wait_idle);
> > >  void intel_psr_activate(struct intel_dp *intel_dp, bool schedule);
> > > +void intel_psr_activate_block_get(struct intel_dp *intel_dp);
> > > +void intel_psr_activate_block_put(struct intel_dp *intel_dp);
> > >  
> > >  /* intel_runtime_pm.c */
> > >  int intel_power_domains_init(struct drm_i915_private *);
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 906a12ea934d..8702dbafb42d 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -558,6 +558,8 @@ static void __intel_psr_activate(struct
> > > intel_dp *intel_dp)
> > >  
> > >  	WARN_ON(dev_priv->psr.active);
> > >  	lockdep_assert_held(&dev_priv->psr.lock);
> > > +	if (dev_priv->psr.activate_block_count)
> > > +		return;
> > >  
> > >  	dev_priv->psr.activate(intel_dp);
> > >  	dev_priv->psr.active = true;
> > > @@ -1188,3 +1190,55 @@ void intel_psr_activate(struct intel_dp
> > > *intel_dp, bool schedule)
> > >  out:
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > >  }
> > > +
> > > +/**
> > > + * intel_psr_activate_block_get - Block further attempts to
> > > activate PSR
> > > + * @intel_dp: DisplayPort that have PSR enabled
> > > + *
> > > + * It have a internal reference count, so each
> > > intel_psr_activate_block_get()
> > > + * should have a intel_psr_activate_block_put() counterpart.
> > > + */
> > > +void intel_psr_activate_block_get(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_digital_port *dig_port =
> > > dp_to_dig_port(intel_dp);
> > > +	struct drm_device *dev = dig_port->base.base.dev;
> > > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > > +
> > > +	if (!CAN_PSR(dev_priv))
> > > +		return;
> > > +
> > > +	mutex_lock(&dev_priv->psr.lock);
> > > +	if (dev_priv->psr.enabled != intel_dp)
> > > +		goto out;
> > > +
> > > +	dev_priv->psr.activate_block_count++;
> > > +out:
> > > +	mutex_unlock(&dev_priv->psr.lock);
> > > +}
> > > +
> > > +
> > > +/**
> > > + * intel_psr_activate_block_put - Unblock further attempts to
> > > activate PSR
> > > + * @intel_dp: DisplayPort that have PSR enabled
> > > + *
> > > + * Decrease the reference counter incremented by
> > > intel_psr_activate_block_get()
> > > + * when zero it allows PSR be activate. To actually activate PSR
> > > when reference
> > > + * counter is zero intel_psr_activate() should be called.
> > > + */
> > > +void intel_psr_activate_block_put(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_digital_port *dig_port =
> > > dp_to_dig_port(intel_dp);
> > > +	struct drm_device *dev = dig_port->base.base.dev;
> > > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > > +
> > > +	if (!CAN_PSR(dev_priv))
> > > +		return;
> > > +
> > > +	mutex_lock(&dev_priv->psr.lock);
> > > +	if (dev_priv->psr.enabled != intel_dp)
> > > +		goto out;
> > > +
> > > +	dev_priv->psr.activate_block_count--;
> > > +out:
> > > +	mutex_unlock(&dev_priv->psr.lock);
> > > +}
> > > -- 
> > > 2.16.3
> > >