| Message ID | 20181110111654.4387-5-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | omapdrm: Fix runtime PM issues at module load and unload time | expand |
Hi, On Sat, Nov 10, 2018 at 01:16:54PM +0200, Laurent Pinchart wrote: > The internal encoders (DSI, HDMI4, HDMI5 and VENC) runtime PM handlers > attempt to manage the runtime PM state of the connected DISPC, based on > the rationale that the DISPC providing data to the encoders requires > ensuring that the display is active whenever the encoders are active. > > While the DISPC provides data to the encoders, it doesn't as such > constitute a resource that encoders require in order to be taken out > of suspend, contrary to for instance a functional clock or a power > supply. Encoders registers can be accessed without the DISPC being > active, and while the encoders will not output any video stream without > being fed by the DISPC, the DISPC PM state doesn't influence the > encoders PM state. > > For this reason the DISPC PM state is better managed from the omapdrm > driver, in the CRTC enable and disable operations. This allows the > encoders PM state to be handled separately from the DISPC, and in > particular at times when the DISPC may not be available (for instance at > probe due to the DSS probe being deferred, or at remove time du to the > DISPC being already removed). > > Fixes: edb715dffdee ("drm/omap: dss: dsi: Move initialization code from bind to probe") > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- +1 for writing fixes, that cleanup the code at the same time :) Decoupling DISPC from encoders also helps to fix DSI support. Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com> -- Sebastian > drivers/gpu/drm/omapdrm/dss/dsi.c | 7 ------- > drivers/gpu/drm/omapdrm/dss/hdmi4.c | 27 --------------------------- > drivers/gpu/drm/omapdrm/dss/hdmi5.c | 27 --------------------------- > drivers/gpu/drm/omapdrm/dss/venc.c | 7 ------- > drivers/gpu/drm/omapdrm/omap_crtc.c | 6 ++++++ > 5 files changed, 6 insertions(+), 68 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c > index b9d5ad7e67d8..36123c086d97 100644 > --- a/drivers/gpu/drm/omapdrm/dss/dsi.c > +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c > @@ -5473,19 +5473,12 @@ static int dsi_runtime_suspend(struct device *dev) > /* wait for current handler to finish before turning the DSI off */ > synchronize_irq(dsi->irq); > > - dispc_runtime_put(dsi->dss->dispc); > - > return 0; > } > > static int dsi_runtime_resume(struct device *dev) > { > struct dsi_data *dsi = dev_get_drvdata(dev); > - int r; > - > - r = dispc_runtime_get(dsi->dss->dispc); > - if (r) > - return r; > > dsi->is_enabled = true; > /* ensure the irq handler sees the is_enabled value */ > diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c > index 073fa462930a..aabdda394c9c 100644 > --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c > +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c > @@ -841,32 +841,6 @@ static int hdmi4_remove(struct platform_device *pdev) > return 0; > } > > -static int hdmi_runtime_suspend(struct device *dev) > -{ > - struct omap_hdmi *hdmi = dev_get_drvdata(dev); > - > - dispc_runtime_put(hdmi->dss->dispc); > - > - return 0; > -} > - > -static int hdmi_runtime_resume(struct device *dev) > -{ > - struct omap_hdmi *hdmi = dev_get_drvdata(dev); > - int r; > - > - r = dispc_runtime_get(hdmi->dss->dispc); > - if (r < 0) > - return r; > - > - return 0; > -} > - > -static const struct dev_pm_ops hdmi_pm_ops = { > - .runtime_suspend = hdmi_runtime_suspend, > - .runtime_resume = hdmi_runtime_resume, > -}; > - > static const struct of_device_id hdmi_of_match[] = { > { .compatible = "ti,omap4-hdmi", }, > {}, > @@ -877,7 +851,6 @@ struct platform_driver omapdss_hdmi4hw_driver = { > .remove = hdmi4_remove, > .driver = { > .name = "omapdss_hdmi", > - .pm = &hdmi_pm_ops, > .of_match_table = hdmi_of_match, > .suppress_bind_attrs = true, > }, > diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c b/drivers/gpu/drm/omapdrm/dss/hdmi5.c > index b0e4a7463f8c..9e8556f67a29 100644 > --- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c > +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c > @@ -825,32 +825,6 @@ static int hdmi5_remove(struct platform_device *pdev) > return 0; > } > > -static int hdmi_runtime_suspend(struct device *dev) > -{ > - struct omap_hdmi *hdmi = dev_get_drvdata(dev); > - > - dispc_runtime_put(hdmi->dss->dispc); > - > - return 0; > -} > - > -static int hdmi_runtime_resume(struct device *dev) > -{ > - struct omap_hdmi *hdmi = dev_get_drvdata(dev); > - int r; > - > - r = dispc_runtime_get(hdmi->dss->dispc); > - if (r < 0) > - return r; > - > - return 0; > -} > - > -static const struct dev_pm_ops hdmi_pm_ops = { > - .runtime_suspend = hdmi_runtime_suspend, > - .runtime_resume = hdmi_runtime_resume, > -}; > - > static const struct of_device_id hdmi_of_match[] = { > { .compatible = "ti,omap5-hdmi", }, > { .compatible = "ti,dra7-hdmi", }, > @@ -862,7 +836,6 @@ struct platform_driver omapdss_hdmi5hw_driver = { > .remove = hdmi5_remove, > .driver = { > .name = "omapdss_hdmi5", > - .pm = &hdmi_pm_ops, > .of_match_table = hdmi_of_match, > .suppress_bind_attrs = true, > }, > diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c b/drivers/gpu/drm/omapdrm/dss/venc.c > index ff0b18c8e4ac..b5f52727f8b1 100644 > --- a/drivers/gpu/drm/omapdrm/dss/venc.c > +++ b/drivers/gpu/drm/omapdrm/dss/venc.c > @@ -946,19 +946,12 @@ static int venc_runtime_suspend(struct device *dev) > if (venc->tv_dac_clk) > clk_disable_unprepare(venc->tv_dac_clk); > > - dispc_runtime_put(venc->dss->dispc); > - > return 0; > } > > static int venc_runtime_resume(struct device *dev) > { > struct venc_device *venc = dev_get_drvdata(dev); > - int r; > - > - r = dispc_runtime_get(venc->dss->dispc); > - if (r < 0) > - return r; > > if (venc->tv_dac_clk) > clk_prepare_enable(venc->tv_dac_clk); > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c > index 62928ec0e7db..caffc547ef97 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -350,11 +350,14 @@ static void omap_crtc_arm_event(struct drm_crtc *crtc) > static void omap_crtc_atomic_enable(struct drm_crtc *crtc, > struct drm_crtc_state *old_state) > { > + struct omap_drm_private *priv = crtc->dev->dev_private; > struct omap_crtc *omap_crtc = to_omap_crtc(crtc); > int ret; > > DBG("%s", omap_crtc->name); > > + priv->dispc_ops->runtime_get(priv->dispc); > + > spin_lock_irq(&crtc->dev->event_lock); > drm_crtc_vblank_on(crtc); > ret = drm_crtc_vblank_get(crtc); > @@ -367,6 +370,7 @@ static void omap_crtc_atomic_enable(struct drm_crtc *crtc, > static void omap_crtc_atomic_disable(struct drm_crtc *crtc, > struct drm_crtc_state *old_state) > { > + struct omap_drm_private *priv = crtc->dev->dev_private; > struct omap_crtc *omap_crtc = to_omap_crtc(crtc); > > DBG("%s", omap_crtc->name); > @@ -379,6 +383,8 @@ static void omap_crtc_atomic_disable(struct drm_crtc *crtc, > spin_unlock_irq(&crtc->dev->event_lock); > > drm_crtc_vblank_off(crtc); > + > + priv->dispc_ops->runtime_put(priv->dispc); > } > > static enum drm_mode_status omap_crtc_mode_valid(struct drm_crtc *crtc, > -- > Regards, > > Laurent Pinchart >
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c index b9d5ad7e67d8..36123c086d97 100644 --- a/drivers/gpu/drm/omapdrm/dss/dsi.c +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c @@ -5473,19 +5473,12 @@ static int dsi_runtime_suspend(struct device *dev) /* wait for current handler to finish before turning the DSI off */ synchronize_irq(dsi->irq); - dispc_runtime_put(dsi->dss->dispc); - return 0; } static int dsi_runtime_resume(struct device *dev) { struct dsi_data *dsi = dev_get_drvdata(dev); - int r; - - r = dispc_runtime_get(dsi->dss->dispc); - if (r) - return r; dsi->is_enabled = true; /* ensure the irq handler sees the is_enabled value */ diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c index 073fa462930a..aabdda394c9c 100644 --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c @@ -841,32 +841,6 @@ static int hdmi4_remove(struct platform_device *pdev) return 0; } -static int hdmi_runtime_suspend(struct device *dev) -{ - struct omap_hdmi *hdmi = dev_get_drvdata(dev); - - dispc_runtime_put(hdmi->dss->dispc); - - return 0; -} - -static int hdmi_runtime_resume(struct device *dev) -{ - struct omap_hdmi *hdmi = dev_get_drvdata(dev); - int r; - - r = dispc_runtime_get(hdmi->dss->dispc); - if (r < 0) - return r; - - return 0; -} - -static const struct dev_pm_ops hdmi_pm_ops = { - .runtime_suspend = hdmi_runtime_suspend, - .runtime_resume = hdmi_runtime_resume, -}; - static const struct of_device_id hdmi_of_match[] = { { .compatible = "ti,omap4-hdmi", }, {}, @@ -877,7 +851,6 @@ struct platform_driver omapdss_hdmi4hw_driver = { .remove = hdmi4_remove, .driver = { .name = "omapdss_hdmi", - .pm = &hdmi_pm_ops, .of_match_table = hdmi_of_match, .suppress_bind_attrs = true, }, diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c b/drivers/gpu/drm/omapdrm/dss/hdmi5.c index b0e4a7463f8c..9e8556f67a29 100644 --- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c @@ -825,32 +825,6 @@ static int hdmi5_remove(struct platform_device *pdev) return 0; } -static int hdmi_runtime_suspend(struct device *dev) -{ - struct omap_hdmi *hdmi = dev_get_drvdata(dev); - - dispc_runtime_put(hdmi->dss->dispc); - - return 0; -} - -static int hdmi_runtime_resume(struct device *dev) -{ - struct omap_hdmi *hdmi = dev_get_drvdata(dev); - int r; - - r = dispc_runtime_get(hdmi->dss->dispc); - if (r < 0) - return r; - - return 0; -} - -static const struct dev_pm_ops hdmi_pm_ops = { - .runtime_suspend = hdmi_runtime_suspend, - .runtime_resume = hdmi_runtime_resume, -}; - static const struct of_device_id hdmi_of_match[] = { { .compatible = "ti,omap5-hdmi", }, { .compatible = "ti,dra7-hdmi", }, @@ -862,7 +836,6 @@ struct platform_driver omapdss_hdmi5hw_driver = { .remove = hdmi5_remove, .driver = { .name = "omapdss_hdmi5", - .pm = &hdmi_pm_ops, .of_match_table = hdmi_of_match, .suppress_bind_attrs = true, }, diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c b/drivers/gpu/drm/omapdrm/dss/venc.c index ff0b18c8e4ac..b5f52727f8b1 100644 --- a/drivers/gpu/drm/omapdrm/dss/venc.c +++ b/drivers/gpu/drm/omapdrm/dss/venc.c @@ -946,19 +946,12 @@ static int venc_runtime_suspend(struct device *dev) if (venc->tv_dac_clk) clk_disable_unprepare(venc->tv_dac_clk); - dispc_runtime_put(venc->dss->dispc); - return 0; } static int venc_runtime_resume(struct device *dev) { struct venc_device *venc = dev_get_drvdata(dev); - int r; - - r = dispc_runtime_get(venc->dss->dispc); - if (r < 0) - return r; if (venc->tv_dac_clk) clk_prepare_enable(venc->tv_dac_clk); diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 62928ec0e7db..caffc547ef97 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -350,11 +350,14 @@ static void omap_crtc_arm_event(struct drm_crtc *crtc) static void omap_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state) { + struct omap_drm_private *priv = crtc->dev->dev_private; struct omap_crtc *omap_crtc = to_omap_crtc(crtc); int ret; DBG("%s", omap_crtc->name); + priv->dispc_ops->runtime_get(priv->dispc); + spin_lock_irq(&crtc->dev->event_lock); drm_crtc_vblank_on(crtc); ret = drm_crtc_vblank_get(crtc); @@ -367,6 +370,7 @@ static void omap_crtc_atomic_enable(struct drm_crtc *crtc, static void omap_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_crtc_state *old_state) { + struct omap_drm_private *priv = crtc->dev->dev_private; struct omap_crtc *omap_crtc = to_omap_crtc(crtc); DBG("%s", omap_crtc->name); @@ -379,6 +383,8 @@ static void omap_crtc_atomic_disable(struct drm_crtc *crtc, spin_unlock_irq(&crtc->dev->event_lock); drm_crtc_vblank_off(crtc); + + priv->dispc_ops->runtime_put(priv->dispc); } static enum drm_mode_status omap_crtc_mode_valid(struct drm_crtc *crtc,
The internal encoders (DSI, HDMI4, HDMI5 and VENC) runtime PM handlers attempt to manage the runtime PM state of the connected DISPC, based on the rationale that the DISPC providing data to the encoders requires ensuring that the display is active whenever the encoders are active. While the DISPC provides data to the encoders, it doesn't as such constitute a resource that encoders require in order to be taken out of suspend, contrary to for instance a functional clock or a power supply. Encoders registers can be accessed without the DISPC being active, and while the encoders will not output any video stream without being fed by the DISPC, the DISPC PM state doesn't influence the encoders PM state. For this reason the DISPC PM state is better managed from the omapdrm driver, in the CRTC enable and disable operations. This allows the encoders PM state to be handled separately from the DISPC, and in particular at times when the DISPC may not be available (for instance at probe due to the DSS probe being deferred, or at remove time du to the DISPC being already removed). Fixes: edb715dffdee ("drm/omap: dss: dsi: Move initialization code from bind to probe") Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/gpu/drm/omapdrm/dss/dsi.c | 7 ------- drivers/gpu/drm/omapdrm/dss/hdmi4.c | 27 --------------------------- drivers/gpu/drm/omapdrm/dss/hdmi5.c | 27 --------------------------- drivers/gpu/drm/omapdrm/dss/venc.c | 7 ------- drivers/gpu/drm/omapdrm/omap_crtc.c | 6 ++++++ 5 files changed, 6 insertions(+), 68 deletions(-)