| Message ID | 20211026145622.v4.1.I9d81c3b44f350707b5373d00524af77c4aae862b@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/2] drm/bridge: parade-ps8640: Enable runtime power management | expand |
Hi, On Tue, Oct 26, 2021 at 2:56 PM Philip Chen <philipchen@chromium.org> wrote: > > Fit ps8640 driver into runtime power management framework: > > First, break _poweron() to 3 parts: (1) turn on power and wait for > ps8640's internal MCU to finish init (2) check panel HPD (which is > proxied by GPIO9) (3) the other configs. As runtime_resume() can be > called before panel is powered, we only add (1) to _resume() and leave > (2)(3) to _pre_enable(). We also add (2) to _aux_transfer() as we want > to ensure panel HPD is asserted before we start AUX CH transactions. > > Second, the original driver has a mysterious delay of 50 ms between (2) > and (3). Since Parade's support can't explain what the delay is for, > and we don't see removing the delay break any boards at hand, remove > the delay to fit into this driver change. > > In addition, rename "powered" to "pre_enabled" and don't check for it > in the pm_runtime calls. The pm_runtime calls are already refcounted > so there's no reason to check there. The other user of "powered", > _get_edid(), only cares if pre_enable() has already been called. > > Lastly, change some existing DRM_...() logging to dev_...() along the > way, since DRM_...() seem to be deprecated in [1]. > > [1] https://patchwork.freedesktop.org/patch/454760/ > > Signed-off-by: Philip Chen <philipchen@chromium.org> > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > --- > In v3, I also added pm_suspend_ignore_children() in the ps8640_probe() > but forgot to mention that in the v3 change log. > > Changes in v4: > - Make ps8640_ensure_hpd() return int (This change was mis-added to > another patch [2] in this patch series: > [2] https://patchwork.kernel.org/project/dri-devel/patch/20211026121058.v3.2.I09899dea340f11feab97d719cb4b62bef3179e4b@changeid/) > > Changes in v3: > - Fix typo/wording in the commit message. > - Add ps8640_aux_transfer_msg() for AUX operation. In > ps8640_aux_transfer(), wrap around ps8640_aux_transfer_msg() > with PM operations and HPD check. > - Document why autosuspend_delay is set to 500ms. > > drivers/gpu/drm/bridge/parade-ps8640.c | 186 +++++++++++++++---------- > 1 file changed, 115 insertions(+), 71 deletions(-) Unfortunately, your patch no longer applies to drm-misc/drm-misc-next. Commit 7abbc26fd667 ("drm/bridge: ps8640: Register and attach our DSI device at probe") landed and that causes a merge conflict. Can you rebase and resend? This will also cause a conflict when Sam's change lands [1] so I guess we can see whose lands first. Let me review that now and maybe you add a Tested-by? If it lands that'll make it easier and you can just rebase on both of them? > + pm_runtime_enable(dev); > + /* > + * In practice, ps8640_aux_transfer_msg() takes ~300ms to complete in > + * the worst case. Set autosuspend_delay to 500ms. > + */ > + pm_runtime_set_autosuspend_delay(dev, 500); To be a little nitpicky, this makes it sound as if the 500 ms needs to be greater than the 300 ms for correctness. That's not _really_ the case. During the whole ps8640_aux_transfer_msg() we're holding a PM Runtime reference (so it won't autosuspend no matter what) and at the end of it we mark that we were busy so the timer won't start ticking until then. Really the 500 ms is because we're quite slow to power up (~300 ms) and so we want to avoid powering down and powering up constantly. We definitely need to avoid a power down / power up when reading the EDID and an EDID read involves _several_ calls in a row to the AUX transfer function. We also expect that after userspace reads the EDID it will call us again "soon" to turn the power on and it's nice to not have to wait the 300 ms again. The 500 ms here is really just a number that's not too short but not too long. I suppose it's roughly related to the 300 ms because that's the delay we're trying to avoid, but otherwise they are unrelated. NOTE: the code will still be _correct_ if we miss the 500 ms window, it'll just be a lot slower. [1] https://lore.kernel.org/r/20211020181901.2114645-2-sam@ravnborg.org -Doug
Hi, On Wed, Oct 27, 2021 at 3:07 PM Doug Anderson <dianders@chromium.org> wrote: > > This will also cause a conflict when Sam's change lands [1] so I guess > we can see whose lands first. Let me review that now and maybe you add > a Tested-by? If it lands that'll make it easier and you can just > rebase on both of them? Ah, whoops! I took a look and it looks like Sam's series needs a spin anyway. ...so I'd say go ahead and rebase yours atop drm-misc-next and your and Sam's patch series can race to see which lands first and which needs to be rebased... If you can add Sam to your CC list, though, that would probably be a wise idea. -Doug
Hi Doug, On Wed, Oct 27, 2021 at 3:08 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Oct 26, 2021 at 2:56 PM Philip Chen <philipchen@chromium.org> wrote: > > > > Fit ps8640 driver into runtime power management framework: > > > > First, break _poweron() to 3 parts: (1) turn on power and wait for > > ps8640's internal MCU to finish init (2) check panel HPD (which is > > proxied by GPIO9) (3) the other configs. As runtime_resume() can be > > called before panel is powered, we only add (1) to _resume() and leave > > (2)(3) to _pre_enable(). We also add (2) to _aux_transfer() as we want > > to ensure panel HPD is asserted before we start AUX CH transactions. > > > > Second, the original driver has a mysterious delay of 50 ms between (2) > > and (3). Since Parade's support can't explain what the delay is for, > > and we don't see removing the delay break any boards at hand, remove > > the delay to fit into this driver change. > > > > In addition, rename "powered" to "pre_enabled" and don't check for it > > in the pm_runtime calls. The pm_runtime calls are already refcounted > > so there's no reason to check there. The other user of "powered", > > _get_edid(), only cares if pre_enable() has already been called. > > > > Lastly, change some existing DRM_...() logging to dev_...() along the > > way, since DRM_...() seem to be deprecated in [1]. > > > > [1] https://patchwork.freedesktop.org/patch/454760/ > > > > Signed-off-by: Philip Chen <philipchen@chromium.org> > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > > --- > > In v3, I also added pm_suspend_ignore_children() in the ps8640_probe() > > but forgot to mention that in the v3 change log. > > > > Changes in v4: > > - Make ps8640_ensure_hpd() return int (This change was mis-added to > > another patch [2] in this patch series: > > [2] https://patchwork.kernel.org/project/dri-devel/patch/20211026121058.v3.2.I09899dea340f11feab97d719cb4b62bef3179e4b@changeid/) > > > > Changes in v3: > > - Fix typo/wording in the commit message. > > - Add ps8640_aux_transfer_msg() for AUX operation. In > > ps8640_aux_transfer(), wrap around ps8640_aux_transfer_msg() > > with PM operations and HPD check. > > - Document why autosuspend_delay is set to 500ms. > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 186 +++++++++++++++---------- > > 1 file changed, 115 insertions(+), 71 deletions(-) > > Unfortunately, your patch no longer applies to drm-misc/drm-misc-next. > Commit 7abbc26fd667 ("drm/bridge: ps8640: Register and attach our DSI > device at probe") landed and that causes a merge conflict. Can you > rebase and resend? Yes, I will rebase and resend. > > This will also cause a conflict when Sam's change lands [1] so I guess > we can see whose lands first. Let me review that now and maybe you add > a Tested-by? If it lands that'll make it easier and you can just > rebase on both of them? As per your latest reply, I'll just rebase atop drm-misc-next for now. > > > > + pm_runtime_enable(dev); > > + /* > > + * In practice, ps8640_aux_transfer_msg() takes ~300ms to complete in > > + * the worst case. Set autosuspend_delay to 500ms. > > + */ > > + pm_runtime_set_autosuspend_delay(dev, 500); > > To be a little nitpicky, this makes it sound as if the 500 ms needs to > be greater than the 300 ms for correctness. That's not _really_ the > case. During the whole ps8640_aux_transfer_msg() we're holding a PM > Runtime reference (so it won't autosuspend no matter what) and at the > end of it we mark that we were busy so the timer won't start ticking > until then. Yeah....sorry, looking again, I agree the comment I added in v3 is pretty misleading. I think autosuspend_delay just needs to be consistently longer than the interval between each aux_transfer call during EDID read. I'll measure the interval and add the number to the comment. > > Really the 500 ms is because we're quite slow to power up (~300 ms) > and so we want to avoid powering down and powering up constantly. We > definitely need to avoid a power down / power up when reading the EDID > and an EDID read involves _several_ calls in a row to the AUX transfer > function. We also expect that after userspace reads the EDID it will > call us again "soon" to turn the power on and it's nice to not have to > wait the 300 ms again. The 500 ms here is really just a number that's > not too short but not too long. I suppose it's roughly related to the > 300 ms because that's the delay we're trying to avoid, but otherwise > they are unrelated. NOTE: the code will still be _correct_ if we miss > the 500 ms window, it'll just be a lot slower. > > [1] https://lore.kernel.org/r/20211020181901.2114645-2-sam@ravnborg.org > > -Doug
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 3aaa90913bf8..cf1f630a3958 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -9,6 +9,7 @@ #include <linux/i2c.h> #include <linux/module.h> #include <linux/of_graph.h> +#include <linux/pm_runtime.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> @@ -100,7 +101,7 @@ struct ps8640 { struct regulator_bulk_data supplies[2]; struct gpio_desc *gpio_reset; struct gpio_desc *gpio_powerdown; - bool powered; + bool pre_enabled; }; static const struct regmap_config ps8640_regmap_config[] = { @@ -148,8 +149,29 @@ static inline struct ps8640 *aux_to_ps8640(struct drm_dp_aux *aux) return container_of(aux, struct ps8640, aux); } -static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, - struct drm_dp_aux_msg *msg) +static int ps8640_ensure_hpd(struct ps8640 *ps_bridge) +{ + struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL]; + struct device *dev = &ps_bridge->page[PAGE2_TOP_CNTL]->dev; + int status; + int ret; + + /* + * Apparently something about the firmware in the chip signals that + * HPD goes high by reporting GPIO9 as high (even though HPD isn't + * actually connected to GPIO9). + */ + ret = regmap_read_poll_timeout(map, PAGE2_GPIO_H, status, + status & PS_GPIO9, 20 * 1000, 200 * 1000); + + if (ret < 0) + dev_warn(dev, "HPD didn't go high: %d\n", ret); + + return ret; +} + +static ssize_t ps8640_aux_transfer_msg(struct drm_dp_aux *aux, + struct drm_dp_aux_msg *msg) { struct ps8640 *ps_bridge = aux_to_ps8640(aux); struct regmap *map = ps_bridge->regmap[PAGE0_DP_CNTL]; @@ -274,38 +296,49 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, return len; } -static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge, - const enum ps8640_vdo_control ctrl) +static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, + struct drm_dp_aux_msg *msg) +{ + struct ps8640 *ps_bridge = aux_to_ps8640(aux); + struct device *dev = &ps_bridge->page[PAGE0_DP_CNTL]->dev; + int ret; + + pm_runtime_get_sync(dev); + ret = ps8640_ensure_hpd(ps_bridge); + if (!ret) + ret = ps8640_aux_transfer_msg(aux, msg); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + + return ret; +} + +static void ps8640_bridge_vdo_control(struct ps8640 *ps_bridge, + const enum ps8640_vdo_control ctrl) { struct regmap *map = ps_bridge->regmap[PAGE3_DSI_CNTL1]; + struct device *dev = &ps_bridge->page[PAGE3_DSI_CNTL1]->dev; u8 vdo_ctrl_buf[] = { VDO_CTL_ADD, ctrl }; int ret; ret = regmap_bulk_write(map, PAGE3_SET_ADD, vdo_ctrl_buf, sizeof(vdo_ctrl_buf)); - if (ret < 0) { - DRM_ERROR("failed to %sable VDO: %d\n", - ctrl == ENABLE ? "en" : "dis", ret); - return ret; - } - - return 0; + if (ret < 0) + dev_err(dev, "failed to %sable VDO: %d\n", + ctrl == ENABLE ? "en" : "dis", ret); } -static void ps8640_bridge_poweron(struct ps8640 *ps_bridge) +static int __maybe_unused ps8640_resume(struct device *dev) { - struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL]; - int ret, status; - - if (ps_bridge->powered) - return; + struct ps8640 *ps_bridge = dev_get_drvdata(dev); + int ret; ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies), ps_bridge->supplies); if (ret < 0) { - DRM_ERROR("cannot enable regulators %d\n", ret); - return; + dev_err(dev, "cannot enable regulators %d\n", ret); + return ret; } gpiod_set_value(ps_bridge->gpio_powerdown, 0); @@ -314,86 +347,78 @@ static void ps8640_bridge_poweron(struct ps8640 *ps_bridge) gpiod_set_value(ps_bridge->gpio_reset, 0); /* - * Wait for the ps8640 embedded MCU to be ready - * First wait 200ms and then check the MCU ready flag every 20ms + * Mystery 200 ms delay for the "MCU to be ready". It's unclear if + * this is truly necessary since the MCU will already signal that + * things are "good to go" by signaling HPD on "gpio 9". See + * ps8640_ensure_hpd(). For now we'll keep this mystery delay just in + * case. */ msleep(200); - ret = regmap_read_poll_timeout(map, PAGE2_GPIO_H, status, - status & PS_GPIO9, 20 * 1000, 200 * 1000); - - if (ret < 0) { - DRM_ERROR("failed read PAGE2_GPIO_H: %d\n", ret); - goto err_regulators_disable; - } - - msleep(50); - - /* - * The Manufacturer Command Set (MCS) is a device dependent interface - * intended for factory programming of the display module default - * parameters. Once the display module is configured, the MCS shall be - * disabled by the manufacturer. Once disabled, all MCS commands are - * ignored by the display interface. - */ - - ret = regmap_update_bits(map, PAGE2_MCS_EN, MCS_EN, 0); - if (ret < 0) { - DRM_ERROR("failed write PAGE2_MCS_EN: %d\n", ret); - goto err_regulators_disable; - } - - /* Switch access edp panel's edid through i2c */ - ret = regmap_write(map, PAGE2_I2C_BYPASS, I2C_BYPASS_EN); - if (ret < 0) { - DRM_ERROR("failed write PAGE2_I2C_BYPASS: %d\n", ret); - goto err_regulators_disable; - } - - ps_bridge->powered = true; - - return; - -err_regulators_disable: - regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies), - ps_bridge->supplies); + return 0; } -static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge) +static int __maybe_unused ps8640_suspend(struct device *dev) { + struct ps8640 *ps_bridge = dev_get_drvdata(dev); int ret; - if (!ps_bridge->powered) - return; - gpiod_set_value(ps_bridge->gpio_reset, 1); gpiod_set_value(ps_bridge->gpio_powerdown, 1); ret = regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies), ps_bridge->supplies); if (ret < 0) - DRM_ERROR("cannot disable regulators %d\n", ret); + dev_err(dev, "cannot disable regulators %d\n", ret); - ps_bridge->powered = false; + return ret; } +static const struct dev_pm_ops ps8640_pm_ops = { + SET_RUNTIME_PM_OPS(ps8640_suspend, ps8640_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) +}; + static void ps8640_pre_enable(struct drm_bridge *bridge) { struct ps8640 *ps_bridge = bridge_to_ps8640(bridge); + struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL]; + struct device *dev = &ps_bridge->page[PAGE0_DP_CNTL]->dev; int ret; - ps8640_bridge_poweron(ps_bridge); + pm_runtime_get_sync(dev); + ps8640_ensure_hpd(ps_bridge); + + /* + * The Manufacturer Command Set (MCS) is a device dependent interface + * intended for factory programming of the display module default + * parameters. Once the display module is configured, the MCS shall be + * disabled by the manufacturer. Once disabled, all MCS commands are + * ignored by the display interface. + */ + + ret = regmap_update_bits(map, PAGE2_MCS_EN, MCS_EN, 0); + if (ret < 0) + dev_warn(dev, "failed write PAGE2_MCS_EN: %d\n", ret); - ret = ps8640_bridge_vdo_control(ps_bridge, ENABLE); + /* Switch access edp panel's edid through i2c */ + ret = regmap_write(map, PAGE2_I2C_BYPASS, I2C_BYPASS_EN); if (ret < 0) - ps8640_bridge_poweroff(ps_bridge); + dev_warn(dev, "failed write PAGE2_MCS_EN: %d\n", ret); + + ps8640_bridge_vdo_control(ps_bridge, ENABLE); + + ps_bridge->pre_enabled = true; } static void ps8640_post_disable(struct drm_bridge *bridge) { struct ps8640 *ps_bridge = bridge_to_ps8640(bridge); + ps_bridge->pre_enabled = false; + ps8640_bridge_vdo_control(ps_bridge, DISABLE); - ps8640_bridge_poweroff(ps_bridge); + pm_runtime_put_sync_suspend(&ps_bridge->page[PAGE0_DP_CNTL]->dev); } static int ps8640_bridge_attach(struct drm_bridge *bridge, @@ -474,7 +499,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, struct drm_connector *connector) { struct ps8640 *ps_bridge = bridge_to_ps8640(bridge); - bool poweroff = !ps_bridge->powered; + bool poweroff = !ps_bridge->pre_enabled; struct edid *edid; /* @@ -512,6 +537,12 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { .pre_enable = ps8640_pre_enable, }; +static void ps8640_runtime_disable(void *data) +{ + pm_runtime_dont_use_autosuspend(data); + pm_runtime_disable(data); +} + static int ps8640_probe(struct i2c_client *client) { struct device *dev = &client->dev; @@ -587,6 +618,18 @@ static int ps8640_probe(struct i2c_client *client) ps_bridge->aux.transfer = ps8640_aux_transfer; drm_dp_aux_init(&ps_bridge->aux); + pm_runtime_enable(dev); + /* + * In practice, ps8640_aux_transfer_msg() takes ~300ms to complete in + * the worst case. Set autosuspend_delay to 500ms. + */ + pm_runtime_set_autosuspend_delay(dev, 500); + pm_runtime_use_autosuspend(dev); + pm_suspend_ignore_children(dev, true); + ret = devm_add_action_or_reset(dev, ps8640_runtime_disable, dev); + if (ret) + return ret; + drm_bridge_add(&ps_bridge->bridge); return 0; @@ -613,6 +656,7 @@ static struct i2c_driver ps8640_driver = { .driver = { .name = "ps8640", .of_match_table = ps8640_match, + .pm = &ps8640_pm_ops, }, }; module_i2c_driver(ps8640_driver);