Re: [PATCH v3 12/12] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare
Hi Doug, On Wed, Apr 14, 2021 at 06:22:57PM -0700, Doug Anderson wrote: > On Wed, Apr 14, 2021 at 5:58 PM Laurent Pinchart wrote: > > 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 ? > > No, it was just code inspection. The old code definitely made an > effort to make enable of an already enabled panel a no-op and disable > of an already disabled panel a no-op. This is even before my > (somewhat) recent patch to make things timing based, though I did > touch the code. > > Can I maybe suggest that getting rid of the extra check should be a > separate patch after this one? Then if it breaks someone it's easy to > just revert that one and we can still keep the runtime pm? Sounds good to me. > > > - 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 -- Regards, Laurent Pinchart
Re: [PATCH v3 12/12] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare
Hi, On Wed, Apr 14, 2021 at 5:58 PM Laurent Pinchart wrote: > > 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 ? No, it was just code inspection. The old code definitely made an effort to make enable of an already enabled panel a no-op and disable of an already disabled panel a no-op. This is even before my (somewhat) recent patch to make things timing based, though I did touch the code. Can I maybe suggest that getting rid of the extra check should be a separate patch after this one? Then if it breaks someone it's easy to just revert that one and we can still keep the runtime pm? > > - 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 Thanks!
Re: [PATCH v3 12/12] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare
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 > Signed-off-by: Douglas Anderson > --- > > (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 > #include > #include > +#include > #include > > #include > @@ -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