| Message ID | 20181112194222.193546-13-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> > > The crtc runtime resume doesn't actually operate on the crtc, but > rather > its encoders. The problem with this is that we need to inspect the crtc > state to get the currently connected encoders. Since runtime resume > isn't guaranteed to be called while holding the modeset locks (although > it sometimes is), this presents a race condition. > > Now that we have ->enabled on the virtual encoders, and a lock to > protect it, just call resume on each encoder and only restore the ones > that are enabled. > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 24 --------------------- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 6 ------ > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 +++++++++------------ > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 ++-- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 6 +++--- > 5 files changed, 15 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index d8f58caf2772..9be24907f8c1 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -841,30 +841,6 @@ static struct drm_crtc_state > *dpu_crtc_duplicate_state(struct drm_crtc *crtc) > return &cstate->base; > } > > -void dpu_crtc_runtime_resume(struct drm_crtc *crtc) > -{ > - struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); > - struct drm_encoder *encoder; > - > - mutex_lock(&dpu_crtc->crtc_lock); > - > - if (!dpu_crtc->enabled) > - goto end; > - > - trace_dpu_crtc_runtime_resume(DRMID(crtc)); > - > - /* restore encoder; crtc will be programmed during commit */ > - drm_for_each_encoder(encoder, crtc->dev) { > - if (encoder->crtc != crtc) > - continue; > - > - dpu_encoder_virt_restore(encoder); > - } I agree the patch provides a cleaner solution. Just for the understanding, won't the race condition be addressed if we acquire the modeset lock here and iterate through crtc->state->encoder_mask? Thanks and Regards, Jeykumar S. > - > -end: > - mutex_unlock(&dpu_crtc->crtc_lock); > -} > - > static void dpu_crtc_disable(struct drm_crtc *crtc) > { > struct dpu_crtc *dpu_crtc; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > index 1dca91d1210f..93d21a61a040 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > @@ -329,10 +329,4 @@ static inline bool dpu_crtc_is_enabled(struct > drm_crtc *crtc) > return crtc ? crtc->enabled : false; > } > > -/** > - * dpu_crtc_runtime_resume - called by the top-level on > pm_runtime_resume > - * @crtc: CRTC to resume > - */ > -void dpu_crtc_runtime_resume(struct drm_crtc *crtc); > - > #endif /* _DPU_CRTC_H_ */ > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 3daa86220d47..d89ac520f7e6 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1089,28 +1089,24 @@ static void > _dpu_encoder_virt_enable_helper(struct > drm_encoder *drm_enc) > _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info); > } > > -void dpu_encoder_virt_restore(struct drm_encoder *drm_enc) > +void dpu_encoder_virt_runtime_resume(struct drm_encoder *drm_enc) > { > - struct dpu_encoder_virt *dpu_enc = NULL; > - int i; > - > - if (!drm_enc) { > - DPU_ERROR("invalid encoder\n"); > - return; > - } > - dpu_enc = to_dpu_encoder_virt(drm_enc); > + struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); > > - for (i = 0; i < dpu_enc->num_phys_encs; i++) { > - struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; > + mutex_lock(&dpu_enc->enc_lock); > > - if (phys && (phys != dpu_enc->cur_master) && > phys->ops.restore) > - phys->ops.restore(phys); > - } > + if (!dpu_enc->enabled) > + goto out; > > + if (dpu_enc->cur_slave && dpu_enc->cur_slave->ops.restore) > + dpu_enc->cur_slave->ops.restore(dpu_enc->cur_slave); > if (dpu_enc->cur_master && dpu_enc->cur_master->ops.restore) > dpu_enc->cur_master->ops.restore(dpu_enc->cur_master); > > _dpu_encoder_virt_enable_helper(drm_enc); > + > +out: > + mutex_unlock(&dpu_enc->enc_lock); > } > > static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > index 9dbf38f446d9..aa4f135218fa 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > @@ -126,10 +126,10 @@ int dpu_encoder_wait_for_event(struct drm_encoder > *drm_encoder, > enum dpu_intf_mode dpu_encoder_get_intf_mode(struct drm_encoder > *encoder); > > /** > - * dpu_encoder_virt_restore - restore the encoder configs > + * dpu_encoder_virt_runtime_resume - pm runtime resume the encoder > configs > * @encoder: encoder pointer > */ > -void dpu_encoder_virt_restore(struct drm_encoder *encoder); > +void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder); > > /** > * dpu_encoder_init - initialize virtual encoder object > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index e42685a1d928..c385eab7e441 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -1132,7 +1132,7 @@ static int __maybe_unused > dpu_runtime_resume(struct > device *dev) > int rc = -1; > struct platform_device *pdev = to_platform_device(dev); > struct dpu_kms *dpu_kms = platform_get_drvdata(pdev); > - struct drm_crtc *crtc; > + struct drm_encoder *encoder; > struct drm_device *ddev; > struct dss_module_power *mp = &dpu_kms->mp; > > @@ -1150,8 +1150,8 @@ static int __maybe_unused > dpu_runtime_resume(struct > device *dev) > > dpu_vbif_init_memtypes(dpu_kms); > > - drm_for_each_crtc(crtc, ddev) > - dpu_crtc_runtime_resume(crtc); > + drm_for_each_encoder(encoder, ddev) > + dpu_encoder_virt_runtime_resume(encoder); > > return rc; > }
On Mon, Nov 12, 2018 at 05:47:58PM -0800, Jeykumar Sankaran wrote: > On 2018-11-12 11:42, Sean Paul wrote: > > From: Sean Paul <seanpaul@chromium.org> > > > > The crtc runtime resume doesn't actually operate on the crtc, but rather > > its encoders. The problem with this is that we need to inspect the crtc > > state to get the currently connected encoders. Since runtime resume > > isn't guaranteed to be called while holding the modeset locks (although > > it sometimes is), this presents a race condition. > > > > Now that we have ->enabled on the virtual encoders, and a lock to > > protect it, just call resume on each encoder and only restore the ones > > that are enabled. > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 24 --------------------- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 6 ------ > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 +++++++++------------ > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 ++-- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 6 +++--- > > 5 files changed, 15 insertions(+), 49 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > index d8f58caf2772..9be24907f8c1 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -841,30 +841,6 @@ static struct drm_crtc_state > > *dpu_crtc_duplicate_state(struct drm_crtc *crtc) > > return &cstate->base; > > } > > > > -void dpu_crtc_runtime_resume(struct drm_crtc *crtc) > > -{ > > - struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); > > - struct drm_encoder *encoder; > > - > > - mutex_lock(&dpu_crtc->crtc_lock); > > - > > - if (!dpu_crtc->enabled) > > - goto end; > > - > > - trace_dpu_crtc_runtime_resume(DRMID(crtc)); > > - > > - /* restore encoder; crtc will be programmed during commit */ > > - drm_for_each_encoder(encoder, crtc->dev) { > > - if (encoder->crtc != crtc) > > - continue; > > - > > - dpu_encoder_virt_restore(encoder); > > - } > I agree the patch provides a cleaner solution. Just for the understanding, > won't the race condition be addressed if we acquire the > modeset lock here and iterate through crtc->state->encoder_mask? You can't do that since we do synchronous pm_runtime_get_sync while holding the modeset locks. So you'll have lock recursion. Unfortunately it's not guaranteed that this will be called while holding modeset locks, so that's why we need to use enc_lock. Sean > > Thanks and Regards, > Jeykumar S. > > - > > -end: > > - mutex_unlock(&dpu_crtc->crtc_lock); > > -} > > - > > static void dpu_crtc_disable(struct drm_crtc *crtc) > > { > > struct dpu_crtc *dpu_crtc; > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > > index 1dca91d1210f..93d21a61a040 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > > @@ -329,10 +329,4 @@ static inline bool dpu_crtc_is_enabled(struct > > drm_crtc *crtc) > > return crtc ? crtc->enabled : false; > > } > > > > -/** > > - * dpu_crtc_runtime_resume - called by the top-level on > > pm_runtime_resume > > - * @crtc: CRTC to resume > > - */ > > -void dpu_crtc_runtime_resume(struct drm_crtc *crtc); > > - > > #endif /* _DPU_CRTC_H_ */ > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > index 3daa86220d47..d89ac520f7e6 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -1089,28 +1089,24 @@ static void > > _dpu_encoder_virt_enable_helper(struct > > drm_encoder *drm_enc) > > _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info); > > } > > > > -void dpu_encoder_virt_restore(struct drm_encoder *drm_enc) > > +void dpu_encoder_virt_runtime_resume(struct drm_encoder *drm_enc) > > { > > - struct dpu_encoder_virt *dpu_enc = NULL; > > - int i; > > - > > - if (!drm_enc) { > > - DPU_ERROR("invalid encoder\n"); > > - return; > > - } > > - dpu_enc = to_dpu_encoder_virt(drm_enc); > > + struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); > > > > - for (i = 0; i < dpu_enc->num_phys_encs; i++) { > > - struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; > > + mutex_lock(&dpu_enc->enc_lock); > > > > - if (phys && (phys != dpu_enc->cur_master) && > > phys->ops.restore) > > - phys->ops.restore(phys); > > - } > > + if (!dpu_enc->enabled) > > + goto out; > > > > + if (dpu_enc->cur_slave && dpu_enc->cur_slave->ops.restore) > > + dpu_enc->cur_slave->ops.restore(dpu_enc->cur_slave); > > if (dpu_enc->cur_master && dpu_enc->cur_master->ops.restore) > > dpu_enc->cur_master->ops.restore(dpu_enc->cur_master); > > > > _dpu_encoder_virt_enable_helper(drm_enc); > > + > > +out: > > + mutex_unlock(&dpu_enc->enc_lock); > > } > > > > static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > index 9dbf38f446d9..aa4f135218fa 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > @@ -126,10 +126,10 @@ int dpu_encoder_wait_for_event(struct drm_encoder > > *drm_encoder, > > enum dpu_intf_mode dpu_encoder_get_intf_mode(struct drm_encoder > > *encoder); > > > > /** > > - * dpu_encoder_virt_restore - restore the encoder configs > > + * dpu_encoder_virt_runtime_resume - pm runtime resume the encoder > > configs > > * @encoder: encoder pointer > > */ > > -void dpu_encoder_virt_restore(struct drm_encoder *encoder); > > +void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder); > > > > /** > > * dpu_encoder_init - initialize virtual encoder object > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > index e42685a1d928..c385eab7e441 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > @@ -1132,7 +1132,7 @@ static int __maybe_unused > > dpu_runtime_resume(struct > > device *dev) > > int rc = -1; > > struct platform_device *pdev = to_platform_device(dev); > > struct dpu_kms *dpu_kms = platform_get_drvdata(pdev); > > - struct drm_crtc *crtc; > > + struct drm_encoder *encoder; > > struct drm_device *ddev; > > struct dss_module_power *mp = &dpu_kms->mp; > > > > @@ -1150,8 +1150,8 @@ static int __maybe_unused > > dpu_runtime_resume(struct > > device *dev) > > > > dpu_vbif_init_memtypes(dpu_kms); > > > > - drm_for_each_crtc(crtc, ddev) > > - dpu_crtc_runtime_resume(crtc); > > + drm_for_each_encoder(encoder, ddev) > > + dpu_encoder_virt_runtime_resume(encoder); > > > > return rc; > > } > > -- > Jeykumar S
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index d8f58caf2772..9be24907f8c1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -841,30 +841,6 @@ static struct drm_crtc_state *dpu_crtc_duplicate_state(struct drm_crtc *crtc) return &cstate->base; } -void dpu_crtc_runtime_resume(struct drm_crtc *crtc) -{ - struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); - struct drm_encoder *encoder; - - mutex_lock(&dpu_crtc->crtc_lock); - - if (!dpu_crtc->enabled) - goto end; - - trace_dpu_crtc_runtime_resume(DRMID(crtc)); - - /* restore encoder; crtc will be programmed during commit */ - drm_for_each_encoder(encoder, crtc->dev) { - if (encoder->crtc != crtc) - continue; - - dpu_encoder_virt_restore(encoder); - } - -end: - mutex_unlock(&dpu_crtc->crtc_lock); -} - static void dpu_crtc_disable(struct drm_crtc *crtc) { struct dpu_crtc *dpu_crtc; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index 1dca91d1210f..93d21a61a040 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -329,10 +329,4 @@ static inline bool dpu_crtc_is_enabled(struct drm_crtc *crtc) return crtc ? crtc->enabled : false; } -/** - * dpu_crtc_runtime_resume - called by the top-level on pm_runtime_resume - * @crtc: CRTC to resume - */ -void dpu_crtc_runtime_resume(struct drm_crtc *crtc); - #endif /* _DPU_CRTC_H_ */ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 3daa86220d47..d89ac520f7e6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1089,28 +1089,24 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc) _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info); } -void dpu_encoder_virt_restore(struct drm_encoder *drm_enc) +void dpu_encoder_virt_runtime_resume(struct drm_encoder *drm_enc) { - struct dpu_encoder_virt *dpu_enc = NULL; - int i; - - if (!drm_enc) { - DPU_ERROR("invalid encoder\n"); - return; - } - dpu_enc = to_dpu_encoder_virt(drm_enc); + struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); - for (i = 0; i < dpu_enc->num_phys_encs; i++) { - struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; + mutex_lock(&dpu_enc->enc_lock); - if (phys && (phys != dpu_enc->cur_master) && phys->ops.restore) - phys->ops.restore(phys); - } + if (!dpu_enc->enabled) + goto out; + if (dpu_enc->cur_slave && dpu_enc->cur_slave->ops.restore) + dpu_enc->cur_slave->ops.restore(dpu_enc->cur_slave); if (dpu_enc->cur_master && dpu_enc->cur_master->ops.restore) dpu_enc->cur_master->ops.restore(dpu_enc->cur_master); _dpu_encoder_virt_enable_helper(drm_enc); + +out: + mutex_unlock(&dpu_enc->enc_lock); } static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 9dbf38f446d9..aa4f135218fa 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -126,10 +126,10 @@ int dpu_encoder_wait_for_event(struct drm_encoder *drm_encoder, enum dpu_intf_mode dpu_encoder_get_intf_mode(struct drm_encoder *encoder); /** - * dpu_encoder_virt_restore - restore the encoder configs + * dpu_encoder_virt_runtime_resume - pm runtime resume the encoder configs * @encoder: encoder pointer */ -void dpu_encoder_virt_restore(struct drm_encoder *encoder); +void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder); /** * dpu_encoder_init - initialize virtual encoder object diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index e42685a1d928..c385eab7e441 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -1132,7 +1132,7 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev) int rc = -1; struct platform_device *pdev = to_platform_device(dev); struct dpu_kms *dpu_kms = platform_get_drvdata(pdev); - struct drm_crtc *crtc; + struct drm_encoder *encoder; struct drm_device *ddev; struct dss_module_power *mp = &dpu_kms->mp; @@ -1150,8 +1150,8 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev) dpu_vbif_init_memtypes(dpu_kms); - drm_for_each_crtc(crtc, ddev) - dpu_crtc_runtime_resume(crtc); + drm_for_each_encoder(encoder, ddev) + dpu_encoder_virt_runtime_resume(encoder); return rc; }