Re: [PATCH v3 4/4] drm/omap: Move DISPC runtime PM handling to omapdrm

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 7538 bytes --]

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
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel