[11/12] drm/msm: dpu: Add ->enabled to dpu_encoder_virt

Submitted by Sean Paul on Nov. 12, 2018, 7:42 p.m.

Details

Message ID 20181112194222.193546-12-sean@poorly.run
State New
Headers show
Series "drm/msm: dpu: Clean up runtime power handling" ( rev: 2 1 ) in Freedreno

Not browsing as part of any series.

Commit Message

Sean Paul Nov. 12, 2018, 7:42 p.m.
From: Sean Paul <seanpaul@chromium.org>

Add a bool to dpu_encoder_virt to track whether the encoder is enabled
or not. Repurpose the enc_lock mutex to ensure that it is consistent
with the hw state.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++++++++++++++----
 1 file changed, 22 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 10a0676d1dcf..3daa86220d47 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -132,6 +132,7 @@  enum dpu_enc_rc_states {
  * @base:		drm_encoder base class for registration with DRM
  * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
  * @bus_scaling_client:	Client handle to the bus scaling interface
+ * @enabled:		True if the encoder is active, protected by enc_lock
  * @num_phys_encs:	Actual number of physical encoders contained.
  * @phys_encs:		Container of physical encoders managed.
  * @cur_master:		Pointer to the current master in this mode. Optimization
@@ -148,8 +149,8 @@  enum dpu_enc_rc_states {
  *				all CTL paths
  * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
  * @debugfs_root:		Debug file system root file node
- * @enc_lock:			Lock around physical encoder create/destroy and
-				access.
+ * @enc_lock:			Lock around physical encoder
+ *				create/destroy/enable/disable
  * @frame_busy_mask:		Bitmask tracking which phys_enc we are still
  *				busy processing current command.
  *				Bit0 = phys_encs[0] etc.
@@ -175,6 +176,8 @@  struct dpu_encoder_virt {
 	spinlock_t enc_spinlock;
 	uint32_t bus_scaling_client;
 
+	bool enabled;
+
 	unsigned int num_phys_encs;
 	struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
 	struct dpu_encoder_phys *cur_master;
@@ -1121,6 +1124,8 @@  static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
 		return;
 	}
 	dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+	mutex_lock(&dpu_enc->enc_lock);
 	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
 
 	trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay,
@@ -1137,10 +1142,15 @@  static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
 	if (ret) {
 		DPU_ERROR_ENC(dpu_enc, "dpu resource control failed: %d\n",
 				ret);
-		return;
+		goto out;
 	}
 
 	_dpu_encoder_virt_enable_helper(drm_enc);
+
+	dpu_enc->enabled = true;
+
+out:
+	mutex_unlock(&dpu_enc->enc_lock);
 }
 
 static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
@@ -1162,11 +1172,14 @@  static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
 		return;
 	}
 
-	mode = &drm_enc->crtc->state->adjusted_mode;
-
 	dpu_enc = to_dpu_encoder_virt(drm_enc);
 	DPU_DEBUG_ENC(dpu_enc, "\n");
 
+	mutex_lock(&dpu_enc->enc_lock);
+	dpu_enc->enabled = false;
+
+	mode = &drm_enc->crtc->state->adjusted_mode;
+
 	priv = drm_enc->dev->dev_private;
 	dpu_kms = to_dpu_kms(priv->kms);
 
@@ -1200,6 +1213,8 @@  static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
 	DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
 
 	dpu_rm_release(&dpu_kms->rm, drm_enc);
+
+	mutex_unlock(&dpu_enc->enc_lock);
 }
 
 static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
@@ -2233,6 +2248,8 @@  struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
 
 	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
 
+	dpu_enc->enabled = false;
+
 	return &dpu_enc->base;
 }
 

Comments

On 2018-11-12 11:42, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> Add a bool to dpu_encoder_virt to track whether the encoder is enabled
> or not. Repurpose the enc_lock mutex to ensure that it is consistent
> with the hw state.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++++++++++++++----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 10a0676d1dcf..3daa86220d47 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -132,6 +132,7 @@ enum dpu_enc_rc_states {
>   * @base:		drm_encoder base class for registration with DRM
>   * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
>   * @bus_scaling_client:	Client handle to the bus scaling interface
> + * @enabled:		True if the encoder is active, protected by
> enc_lock
>   * @num_phys_encs:	Actual number of physical encoders contained.
>   * @phys_encs:		Container of physical encoders managed.
>   * @cur_master:		Pointer to the current master in this
> mode. Optimization
> @@ -148,8 +149,8 @@ enum dpu_enc_rc_states {
>   *				all CTL paths
>   * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
>   * @debugfs_root:		Debug file system root file node
> - * @enc_lock:			Lock around physical encoder
> create/destroy and
> -				access.
> + * @enc_lock:			Lock around physical encoder
> + *				create/destroy/enable/disable
>   * @frame_busy_mask:		Bitmask tracking which phys_enc we are
> still
>   *				busy processing current command.
>   *				Bit0 = phys_encs[0] etc.
> @@ -175,6 +176,8 @@ struct dpu_encoder_virt {
>  	spinlock_t enc_spinlock;
>  	uint32_t bus_scaling_client;
> 
> +	bool enabled;
> +
>  	unsigned int num_phys_encs;
>  	struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
>  	struct dpu_encoder_phys *cur_master;
> @@ -1121,6 +1124,8 @@ static void dpu_encoder_virt_enable(struct
> drm_encoder *drm_enc)
>  		return;
>  	}
>  	dpu_enc = to_dpu_encoder_virt(drm_enc);
> +
> +	mutex_lock(&dpu_enc->enc_lock);
>  	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
> 
>  	trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay,
> @@ -1137,10 +1142,15 @@ static void dpu_encoder_virt_enable(struct
> drm_encoder *drm_enc)
>  	if (ret) {
>  		DPU_ERROR_ENC(dpu_enc, "dpu resource control failed:
> %d\n",
>  				ret);
> -		return;
> +		goto out;
>  	}
> 
>  	_dpu_encoder_virt_enable_helper(drm_enc);
> +
> +	dpu_enc->enabled = true;
> +
> +out:
> +	mutex_unlock(&dpu_enc->enc_lock);
>  }
> 
>  static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
> @@ -1162,11 +1172,14 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc)
>  		return;
>  	}
> 
> -	mode = &drm_enc->crtc->state->adjusted_mode;
> -
>  	dpu_enc = to_dpu_encoder_virt(drm_enc);
>  	DPU_DEBUG_ENC(dpu_enc, "\n");
> 
> +	mutex_lock(&dpu_enc->enc_lock);
Where do you expect it to go wrong if enable/disable
is not protected using enc_lock?

Thanks and Regards,
Jeykumar S.
> +	dpu_enc->enabled = false;
> +
> +	mode = &drm_enc->crtc->state->adjusted_mode;
> +
>  	priv = drm_enc->dev->dev_private;
>  	dpu_kms = to_dpu_kms(priv->kms);
> 
> @@ -1200,6 +1213,8 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc)
>  	DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
> 
>  	dpu_rm_release(&dpu_kms->rm, drm_enc);
> +
> +	mutex_unlock(&dpu_enc->enc_lock);
>  }
> 
>  static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg 
> *catalog,
> @@ -2233,6 +2248,8 @@ struct drm_encoder *dpu_encoder_init(struct
> drm_device *dev,
> 
>  	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
> 
> +	dpu_enc->enabled = false;
> +
>  	return &dpu_enc->base;
>  }
On Mon, Nov 12, 2018 at 05:43:17PM -0800, Jeykumar Sankaran wrote:
> On 2018-11-12 11:42, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > Add a bool to dpu_encoder_virt to track whether the encoder is enabled
> > or not. Repurpose the enc_lock mutex to ensure that it is consistent
> > with the hw state.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++++++++++++++----
> >  1 file changed, 22 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 10a0676d1dcf..3daa86220d47 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -132,6 +132,7 @@ enum dpu_enc_rc_states {
> >   * @base:		drm_encoder base class for registration with DRM
> >   * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
> >   * @bus_scaling_client:	Client handle to the bus scaling interface
> > + * @enabled:		True if the encoder is active, protected by
> > enc_lock
> >   * @num_phys_encs:	Actual number of physical encoders contained.
> >   * @phys_encs:		Container of physical encoders managed.
> >   * @cur_master:		Pointer to the current master in this
> > mode. Optimization
> > @@ -148,8 +149,8 @@ enum dpu_enc_rc_states {
> >   *				all CTL paths
> >   * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
> >   * @debugfs_root:		Debug file system root file node
> > - * @enc_lock:			Lock around physical encoder
> > create/destroy and
> > -				access.
> > + * @enc_lock:			Lock around physical encoder
> > + *				create/destroy/enable/disable
> >   * @frame_busy_mask:		Bitmask tracking which phys_enc we are
> > still
> >   *				busy processing current command.
> >   *				Bit0 = phys_encs[0] etc.
> > @@ -175,6 +176,8 @@ struct dpu_encoder_virt {
> >  	spinlock_t enc_spinlock;
> >  	uint32_t bus_scaling_client;
> > 
> > +	bool enabled;
> > +
> >  	unsigned int num_phys_encs;
> >  	struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
> >  	struct dpu_encoder_phys *cur_master;
> > @@ -1121,6 +1124,8 @@ static void dpu_encoder_virt_enable(struct
> > drm_encoder *drm_enc)
> >  		return;
> >  	}
> >  	dpu_enc = to_dpu_encoder_virt(drm_enc);
> > +
> > +	mutex_lock(&dpu_enc->enc_lock);
> >  	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
> > 
> >  	trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay,
> > @@ -1137,10 +1142,15 @@ static void dpu_encoder_virt_enable(struct
> > drm_encoder *drm_enc)
> >  	if (ret) {
> >  		DPU_ERROR_ENC(dpu_enc, "dpu resource control failed:
> > %d\n",
> >  				ret);
> > -		return;
> > +		goto out;
> >  	}
> > 
> >  	_dpu_encoder_virt_enable_helper(drm_enc);
> > +
> > +	dpu_enc->enabled = true;
> > +
> > +out:
> > +	mutex_unlock(&dpu_enc->enc_lock);
> >  }
> > 
> >  static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
> > @@ -1162,11 +1172,14 @@ static void dpu_encoder_virt_disable(struct
> > drm_encoder *drm_enc)
> >  		return;
> >  	}
> > 
> > -	mode = &drm_enc->crtc->state->adjusted_mode;
> > -
> >  	dpu_enc = to_dpu_encoder_virt(drm_enc);
> >  	DPU_DEBUG_ENC(dpu_enc, "\n");
> > 
> > +	mutex_lock(&dpu_enc->enc_lock);
> Where do you expect it to go wrong if enable/disable
> is not protected using enc_lock?

runtime_resume can run concurrently with either enable or disable. We need the
enc_lock to ensure that enable/disable/runtime_resume are all relatively atomic.

Sean


> 
> Thanks and Regards,
> Jeykumar S.
> > +	dpu_enc->enabled = false;
> > +
> > +	mode = &drm_enc->crtc->state->adjusted_mode;
> > +
> >  	priv = drm_enc->dev->dev_private;
> >  	dpu_kms = to_dpu_kms(priv->kms);
> > 
> > @@ -1200,6 +1213,8 @@ static void dpu_encoder_virt_disable(struct
> > drm_encoder *drm_enc)
> >  	DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
> > 
> >  	dpu_rm_release(&dpu_kms->rm, drm_enc);
> > +
> > +	mutex_unlock(&dpu_enc->enc_lock);
> >  }
> > 
> >  static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
> > @@ -2233,6 +2248,8 @@ struct drm_encoder *dpu_encoder_init(struct
> > drm_device *dev,
> > 
> >  	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
> > 
> > +	dpu_enc->enabled = false;
> > +
> >  	return &dpu_enc->base;
> >  }
> 
> -- 
> Jeykumar S