| Message ID | 20181112194222.193546-12-sean@poorly.run (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: dpu: Clean up runtime power handling | expand |
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
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; }