Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Douglas Anderson <dianders@chromium.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Sam Ravnborg <sam@ravnborg.org>,
	Linus W <linus.walleij@linaro.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	robdclark@chromium.org, Stephen Boyd <swboyd@chromium.org>,
	Steev Klimaszewski <steev@kali.org>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	linux-arm-msm@vger.kernel.org,
	Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>,
	Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
	Thierry Reding <thierry.reding@gmail.com>,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 12/12] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare
Date: Thu, 15 Apr 2021 03:58:25 +0300
Message-ID: <YHePsQgqOau1V5lD@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20210402152701.v3.12.I9e8bd33b49c496745bfac58ea9ab418bd3b6f5ce@changeid>

Hi Doug,

Thank you for the patch.

On Fri, Apr 02, 2021 at 03:28:46PM -0700, Douglas Anderson wrote:
> Unpreparing and re-preparing a panel can be a really heavy
> operation. Panels datasheets often specify something on the order of
> 500ms as the delay you should insert after turning off the panel
> before turning it on again. In addition, turning on a panel can have
> delays on the order of 100ms - 200ms before the panel will assert HPD
> (AKA "panel ready"). The above means that we should avoid turning a
> panel off if we're going to turn it on again shortly.
> 
> The above becomes a problem when we want to read the EDID of a
> panel. The way that ordering works is that userspace wants to read the
> EDID of the panel _before_ fully enabling it so that it can set the
> initial mode correctly. However, we can't read the EDID until we power
> it up. This leads to code that does this dance (like
> ps8640_bridge_get_edid()):
> 
> 1. When userspace requests EDID / the panel modes (through an ioctl),
>    we power on the panel just enough to read the EDID and then power
>    it off.
> 2. Userspace then turns the panel on.
> 
> There's likely not much time between step #1 and #2 and so we want to
> avoid powering the panel off and on again between those two steps.
> 
> Let's use Runtime PM to help us. We'll move the existing prepare() and
> unprepare() to be runtime resume() and runtime suspend(). Now when we
> want to prepare() or unprepare() we just increment or decrement the
> refcount. We'll default to a 1 second autosuspend delay which seems
> sane given the typical delays we see for panels.
> 
> A few notes:
> - It seems the existing unprepare() and prepare() are defined to be
>   no-ops if called extra times. We'll preserve that behavior.

The prepare and unprepare calls are supposed to be balanced, which
should allow us to drop this check. Do you have a reason to suspect that
it may not be the case ?

> - This is a slight change in the ABI of simple panel. If something was
>   absolutely relying on the unprepare() to happen instantly that
>   simply won't be the case anymore. I'm not aware of anyone relying on
>   that behavior, but if there is someone then we'll need to figure out
>   how to enable (or disable) this new delayed behavior selectively.
> - In order for this to work we now have a hard dependency on
>   "PM". From memory this is a legit thing to assume these days and we
>   don't have to find some fallback to keep working if someone wants to
>   build their system without "PM".

Sounds fine to me.

The code looks good to me. Possibly with the prepared check removed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/panel/Kconfig        |  1 +
>  drivers/gpu/drm/panel/panel-simple.c | 93 +++++++++++++++++++++-------
>  2 files changed, 73 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 4894913936e9..ef87d92cdf49 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -80,6 +80,7 @@ config DRM_PANEL_SIMPLE
>  	tristate "support for simple panels"
>  	depends on OF
>  	depends on BACKLIGHT_CLASS_DEVICE
> +	depends on PM
>  	select VIDEOMODE_HELPERS
>  	help
>  	  DRM panel driver for dumb panels that need at most a regulator and
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index be312b5c04dd..6b22872b3281 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -27,6 +27,7 @@
>  #include <linux/module.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  
>  #include <video/display_timing.h>
> @@ -175,6 +176,8 @@ struct panel_simple {
>  	bool enabled;
>  	bool no_hpd;
>  
> +	bool prepared;
> +
>  	ktime_t prepared_time;
>  	ktime_t unprepared_time;
>  
> @@ -334,19 +337,31 @@ static int panel_simple_disable(struct drm_panel *panel)
>  	return 0;
>  }
>  
> +static int panel_simple_suspend(struct device *dev)
> +{
> +	struct panel_simple *p = dev_get_drvdata(dev);
> +
> +	gpiod_set_value_cansleep(p->enable_gpio, 0);
> +	regulator_disable(p->supply);
> +	p->unprepared_time = ktime_get();
> +
> +	return 0;
> +}
> +
>  static int panel_simple_unprepare(struct drm_panel *panel)
>  {
>  	struct panel_simple *p = to_panel_simple(panel);
> +	int ret;
>  
> -	if (p->prepared_time == 0)
> +	/* Unpreparing when already unprepared is a no-op */
> +	if (!p->prepared)
>  		return 0;
>  
> -	gpiod_set_value_cansleep(p->enable_gpio, 0);
> -
> -	regulator_disable(p->supply);
> -
> -	p->prepared_time = 0;
> -	p->unprepared_time = ktime_get();
> +	pm_runtime_mark_last_busy(panel->dev);
> +	ret = pm_runtime_put_autosuspend(panel->dev);
> +	if (ret < 0)
> +		return ret;
> +	p->prepared = false;
>  
>  	return 0;
>  }
> @@ -376,22 +391,19 @@ static int panel_simple_get_hpd_gpio(struct device *dev,
>  	return 0;
>  }
>  
> -static int panel_simple_prepare_once(struct drm_panel *panel)
> +static int panel_simple_prepare_once(struct panel_simple *p)
>  {
> -	struct panel_simple *p = to_panel_simple(panel);
> +	struct device *dev = p->base.dev;
>  	unsigned int delay;
>  	int err;
>  	int hpd_asserted;
>  	unsigned long hpd_wait_us;
>  
> -	if (p->prepared_time != 0)
> -		return 0;
> -
>  	panel_simple_wait(p->unprepared_time, p->desc->delay.unprepare);
>  
>  	err = regulator_enable(p->supply);
>  	if (err < 0) {
> -		dev_err(panel->dev, "failed to enable supply: %d\n", err);
> +		dev_err(dev, "failed to enable supply: %d\n", err);
>  		return err;
>  	}
>  
> @@ -405,7 +417,7 @@ static int panel_simple_prepare_once(struct drm_panel *panel)
>  
>  	if (p->hpd_gpio) {
>  		if (IS_ERR(p->hpd_gpio)) {
> -			err = panel_simple_get_hpd_gpio(panel->dev, p, false);
> +			err = panel_simple_get_hpd_gpio(dev, p, false);
>  			if (err)
>  				goto error;
>  		}
> @@ -423,7 +435,7 @@ static int panel_simple_prepare_once(struct drm_panel *panel)
>  
>  		if (err) {
>  			if (err != -ETIMEDOUT)
> -				dev_err(panel->dev,
> +				dev_err(dev,
>  					"error waiting for hpd GPIO: %d\n", err);
>  			goto error;
>  		}
> @@ -447,25 +459,46 @@ static int panel_simple_prepare_once(struct drm_panel *panel)
>   */
>  #define MAX_PANEL_PREPARE_TRIES		5
>  
> -static int panel_simple_prepare(struct drm_panel *panel)
> +static int panel_simple_resume(struct device *dev)
>  {
> +	struct panel_simple *p = dev_get_drvdata(dev);
>  	int ret;
>  	int try;
>  
>  	for (try = 0; try < MAX_PANEL_PREPARE_TRIES; try++) {
> -		ret = panel_simple_prepare_once(panel);
> +		ret = panel_simple_prepare_once(p);
>  		if (ret != -ETIMEDOUT)
>  			break;
>  	}
>  
>  	if (ret == -ETIMEDOUT)
> -		dev_err(panel->dev, "Prepare timeout after %d tries\n", try);
> +		dev_err(dev, "Prepare timeout after %d tries\n", try);
>  	else if (try)
> -		dev_warn(panel->dev, "Prepare needed %d retries\n", try);
> +		dev_warn(dev, "Prepare needed %d retries\n", try);
>  
>  	return ret;
>  }
>  
> +static int panel_simple_prepare(struct drm_panel *panel)
> +{
> +	struct panel_simple *p = to_panel_simple(panel);
> +	int ret;
> +
> +	/* Preparing when already prepared is a no-op */
> +	if (p->prepared)
> +		return 0;
> +
> +	ret = pm_runtime_get_sync(panel->dev);
> +	if (ret < 0) {
> +		pm_runtime_put_autosuspend(panel->dev);
> +		return ret;
> +	}
> +
> +	p->prepared = true;
> +
> +	return 0;
> +}
> +
>  static int panel_simple_enable(struct drm_panel *panel)
>  {
>  	struct panel_simple *p = to_panel_simple(panel);
> @@ -748,6 +781,18 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  		break;
>  	}
>  
> +	dev_set_drvdata(dev, panel);
> +
> +	/*
> +	 * We use runtime PM for prepare / unprepare since those power the panel
> +	 * on and off and those can be very slow operations. This is important
> +	 * to optimize powering the panel on briefly to read the EDID before
> +	 * fully enabling the panel.
> +	 */
> +	pm_runtime_enable(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +
>  	drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);
>  
>  	err = drm_panel_of_backlight(&panel->base);
> @@ -756,8 +801,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  
>  	drm_panel_add(&panel->base);
>  
> -	dev_set_drvdata(dev, panel);
> -
>  	return 0;
>  
>  free_ddc:
> @@ -4603,10 +4646,17 @@ static void panel_simple_platform_shutdown(struct platform_device *pdev)
>  	panel_simple_shutdown(&pdev->dev);
>  }
>  
> +static const struct dev_pm_ops panel_simple_pm_ops = {
> +	SET_RUNTIME_PM_OPS(panel_simple_suspend, panel_simple_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
> +};
> +
>  static struct platform_driver panel_simple_platform_driver = {
>  	.driver = {
>  		.name = "panel-simple",
>  		.of_match_table = platform_of_match,
> +		.pm = &panel_simple_pm_ops,
>  	},
>  	.probe = panel_simple_platform_probe,
>  	.remove = panel_simple_platform_remove,
> @@ -4901,6 +4951,7 @@ static struct mipi_dsi_driver panel_simple_dsi_driver = {
>  	.driver = {
>  		.name = "panel-simple-dsi",
>  		.of_match_table = dsi_of_match,
> +		.pm = &panel_simple_pm_ops,
>  	},
>  	.probe = panel_simple_dsi_probe,
>  	.remove = panel_simple_dsi_remove,

-- 
Regards,

Laurent Pinchart

  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-02 22:28 [PATCH v3 00/12] drm: Fix EDID reading on ti-sn65dsi86 Douglas Anderson
2021-04-02 22:28 ` [PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable() Douglas Anderson
2021-04-05  0:49   ` Laurent Pinchart
2021-04-15  1:19     ` Doug Anderson
2021-04-15  1:56       ` Laurent Pinchart
2021-04-15 14:48         ` Doug Anderson
2021-04-02 22:28 ` [PATCH v3 02/12] drm/bridge: ti-sn65dsi86: Simplify refclk handling Douglas Anderson
2021-04-02 22:28 ` [PATCH v3 03/12] drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment Douglas Anderson
2021-04-05  0:50   ` Laurent Pinchart
2021-04-02 22:28 ` [PATCH v3 04/12] drm/bridge: ti-sn65dsi86: Reorder remove() Douglas Anderson
2021-04-05  0:52   ` Laurent Pinchart
2021-04-02 22:28 ` [PATCH v3 05/12] drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable() Douglas Anderson
2021-04-05  0:58   ` Laurent Pinchart
2021-04-02 22:28 ` [PATCH v3 06/12] drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function Douglas Anderson
2021-04-05  0:58   ` Laurent Pinchart
2021-04-02 22:28 ` [PATCH v3 07/12] drm/bridge: ti-sn65dsi86: Remove extra call: drm_connector_update_edid_property() Douglas Anderson
2021-04-05  1:01   ` Laurent Pinchart
2021-04-02 22:28 ` [PATCH v3 08/12] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID Douglas Anderson
2021-04-02 22:28 ` [PATCH v3 09/12] drm/bridge: ti-sn65dsi86: Fail aux transfers right away if not powered Douglas Anderson
2021-04-02 22:28 ` [PATCH v3 10/12] drm/bridge: ti-sn65dsi86: Read the EDID only if refclk was provided Douglas Anderson
2021-04-05  1:04   ` Laurent Pinchart
2021-04-02 22:28 ` [PATCH v3 11/12] drm/bridge: ti-sn65dsi86: Print an error if we fallback to panel modes Douglas Anderson
2021-04-05  1:04   ` Laurent Pinchart
2021-04-02 22:28 ` [PATCH v3 12/12] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare Douglas Anderson
2021-04-15  0:58   ` Laurent Pinchart [this message]
2021-04-15  1:22     ` Doug Anderson
2021-04-15  1:30       ` Laurent Pinchart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YHePsQgqOau1V5lD@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=a.hajda@samsung.com \
    --cc=airlied@linux.ie \
    --cc=bjorn.andersson@linaro.org \
    --cc=daniel@ffwll.ch \
    --cc=dianders@chromium.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jernej.skrabec@siol.net \
    --cc=jonas@kwiboo.se \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=narmstrong@baylibre.com \
    --cc=robdclark@chromium.org \
    --cc=sam@ravnborg.org \
    --cc=stanislav.lisovskiy@intel.com \
    --cc=steev@kali.org \
    --cc=swboyd@chromium.org \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git