The primary goal of this series is to try to properly fix EDID reading for eDP panels using the ti-sn65dsi86 bridge.
Previously we had a patch that added EDID reading but it turned out not to work at bootup. This caused some extra churn at bootup as we tried (and failed) to read the EDID several times and also ended up forcing us to use the hardcoded mode at boot. With this patch series I believe EDID reading is reliable at boot now and we never use the hardcoded mode.
This series is the logical successor to the 3-part series containing the patch ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if refclk") [1] though only one actual patch is the same between the two.
This series starts out with some general / obvious fixes and moves on to some more specific and maybe controversial ones. I wouldn't object to some of the earlier ones landing if they look ready.
This patch was developed against drm-misc-next on a sc7180-trogdor-lazor device. To get things booting for me, I had to use Stephen's patch [2] to keep from crashing but otherwise all the patches I needed were here.
[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c7... [2] https://lore.kernel.org/r/161706912161.3012082.17313817257247946143@swboyd.m...
Changes in v2: - Removed 2nd paragraph in commit message.
Douglas Anderson (14): drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable() drm/bridge: ti-sn65dsi86: Simplify refclk handling drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment drm/bridge: ti-sn65dsi86: Reorder remove() drm/bridge: ti-sn65dsi86: Move MIPI detach() / unregister() to detach() drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable() drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function drm/bridge: ti-sn65dsi86: Remove extra call: drm_connector_update_edid_property() drm/edid: Use the cached EDID in drm_get_edid() if eDP drm/bridge: ti-sn65dsi86: Stop caching the EDID ourselves drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID drm/bridge: ti-sn65dsi86: Read the EDID only if refclk was provided drm/bridge: ti-sn65dsi86: Print an error if we fallback to panel modes drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 125 ++++++++++++++++---------- drivers/gpu/drm/drm_bridge.c | 3 + drivers/gpu/drm/drm_edid.c | 32 ++++++- drivers/gpu/drm/panel/Kconfig | 1 + drivers/gpu/drm/panel/panel-simple.c | 93 ++++++++++++++----- 5 files changed, 184 insertions(+), 70 deletions(-)
The drm_bridge_chain_pre_enable() is not the proper opposite of drm_bridge_chain_post_disable(). It continues along the chain to _before_ the starting bridge. Let's fix that.
Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list") Signed-off-by: Douglas Anderson dianders@chromium.org ---
(no changes since v1)
drivers/gpu/drm/drm_bridge.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 64f0effb52ac..044acd07c153 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge) list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { if (iter->funcs->pre_enable) iter->funcs->pre_enable(iter); + + if (iter == bridge) + break; } } EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
Hi Douglas,
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
Looking at the bridge chaining code always makes me sick :) but beside this the change looks correct, and follows drm_atomic_bridge_chain_pre_enable.
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Regards Andrzej
The clock framework makes it simple to deal with an optional clock. You can call clk_get_optional() and if the clock isn't specified it'll just return NULL without complaint. It's valid to pass NULL to enable/disable/prepare/unprepare. Let's make use of this to simplify things a tiny bit.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Robert Foss robert.foss@linaro.org Reviewed-by: Bjorn Andersson bjorn.andersson@linaro.org Reviewed-by: Stephen Boyd swboyd@chromium.org Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com ---
Changes in v2: - Removed 2nd paragraph in commit message.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 88df4dd0f39d..96fe8f2c0ea9 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1275,14 +1275,9 @@ static int ti_sn_bridge_probe(struct i2c_client *client, return ret; }
- pdata->refclk = devm_clk_get(pdata->dev, "refclk"); - if (IS_ERR(pdata->refclk)) { - ret = PTR_ERR(pdata->refclk); - if (ret == -EPROBE_DEFER) - return ret; - DRM_DEBUG_KMS("refclk not found\n"); - pdata->refclk = NULL; - } + pdata->refclk = devm_clk_get_optional(pdata->dev, "refclk"); + if (IS_ERR(pdata->refclk)) + return PTR_ERR(pdata->refclk);
ret = ti_sn_bridge_parse_dsi_host(pdata); if (ret)
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Regards Andrzej
A random comment inside a function had "/**" in front of it. That doesn't make sense. Remove.
Signed-off-by: Douglas Anderson dianders@chromium.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 96fe8f2c0ea9..76f43af6735d 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -788,7 +788,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge) /* set dsi clk frequency value */ ti_sn_bridge_set_dsi_rate(pdata);
- /** + /* * The SN65DSI86 only supports ASSR Display Authentication method and * this method is enabled by default. An eDP panel must support this * authentication method. We need to enable this method in the eDP panel
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Regards Andrzej
Let's make the remove() function strictly the reverse of the probe() function so it's easier to reason about.
NOTES: - The MIPI calls probably belong in detach() but will be moved in a separate patch. - The cached EDID freeing isn't actually part of probe but needs to be in remove to avoid orphaning memory until better handling of the EDID happens.
This patch was created by code inspection and should move us closer to a proper remove.
Signed-off-by: Douglas Anderson dianders@chromium.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 76f43af6735d..c006678c9921 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1315,20 +1315,21 @@ static int ti_sn_bridge_remove(struct i2c_client *client) if (!pdata) return -EINVAL;
- kfree(pdata->edid); - ti_sn_debugfs_remove(pdata); - - of_node_put(pdata->host_node); - - pm_runtime_disable(pdata->dev); - if (pdata->dsi) { mipi_dsi_detach(pdata->dsi); mipi_dsi_device_unregister(pdata->dsi); }
+ kfree(pdata->edid); + + ti_sn_debugfs_remove(pdata); + drm_bridge_remove(&pdata->bridge);
+ pm_runtime_disable(pdata->dev); + + of_node_put(pdata->host_node); + return 0; }
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
The mipi is incorrectly handled already - mipi devices are searched after bridge registration - it should be reverse, there is comment in the driver that it is due to some dsi hosts, maybe it would be better to fix it there instead of conserve this bad design.
Looks good.
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Regards Andrzej
return 0; }
The register() / attach() for MIPI happen in the bridge's attach(). That means that the inverse belongs in the bridge's detach().
Signed-off-by: Douglas Anderson dianders@chromium.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index c006678c9921..e8e523b3a16b 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -437,7 +437,15 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
static void ti_sn_bridge_detach(struct drm_bridge *bridge) { - drm_dp_aux_unregister(&bridge_to_ti_sn_bridge(bridge)->aux); + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); + + + if (pdata->dsi) { + mipi_dsi_detach(pdata->dsi); + mipi_dsi_device_unregister(pdata->dsi); + } + + drm_dp_aux_unregister(&pdata->aux); }
static void ti_sn_bridge_disable(struct drm_bridge *bridge) @@ -1315,11 +1323,6 @@ static int ti_sn_bridge_remove(struct i2c_client *client) if (!pdata) return -EINVAL;
- if (pdata->dsi) { - mipi_dsi_detach(pdata->dsi); - mipi_dsi_device_unregister(pdata->dsi); - } - kfree(pdata->edid);
ti_sn_debugfs_remove(pdata);
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
As I commented in previous patch, it would be better to fix mipi/bridge registration order in host and this driver.
Have you considered this?
Regards
Andrzej
Hi,
On Wed, Mar 31, 2021 at 2:53 AM Andrzej Hajda a.hajda@samsung.com wrote:
Fair enough. How about I drop this patch at the moment? My series already has enough stuff in it right now and I don't believe anything in the series depends on this patch.
-Doug
We prepared the panel in pre_enable() so we should unprepare it in post_disable() to match.
This becomes important once we start using pre_enable() and post_disable() to make sure things are powered on (and then off again) when reading the EDID.
Signed-off-by: Douglas Anderson dianders@chromium.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index e8e523b3a16b..50a52af8e39f 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -460,8 +460,6 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge) regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0); /* disable DP PLL */ regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0); - - drm_panel_unprepare(pdata->panel); }
static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata) @@ -877,6 +875,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) { struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+ drm_panel_unprepare(pdata->panel); + clk_disable_unprepare(pdata->refclk);
pm_runtime_put_sync(pdata->dev);
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Regards Andrzej
If we just leave the detect() function as NULL then the upper layers assume we're always connected. There's no reason for a stub.
Signed-off-by: Douglas Anderson dianders@chromium.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12 ------------ 1 file changed, 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 50a52af8e39f..a0a00dd1187c 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -306,20 +306,8 @@ static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = { .mode_valid = ti_sn_bridge_connector_mode_valid, };
-static enum drm_connector_status -ti_sn_bridge_connector_detect(struct drm_connector *connector, bool force) -{ - /** - * TODO: Currently if drm_panel is present, then always - * return the status as connected. Need to add support to detect - * device state for hot pluggable scenarios. - */ - return connector_status_connected; -} - static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = { .fill_modes = drm_helper_probe_single_connector_modes, - .detect = ti_sn_bridge_connector_detect, .destroy = drm_connector_cleanup, .reset = drm_atomic_helper_connector_reset, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Regards Andrzej
As of commit 5186421cbfe2 ("drm: Introduce epoch counter to drm_connector") the drm_get_edid() function calls drm_connector_update_edid_property() for us. There's no reason for us to call it again.
Signed-off-by: Douglas Anderson dianders@chromium.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index a0a00dd1187c..9577ebd58c4c 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -270,7 +270,7 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) { struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector); struct edid *edid = pdata->edid; - int num, ret; + int num;
if (!edid) { pm_runtime_get_sync(pdata->dev); @@ -279,12 +279,9 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) }
if (edid && drm_edid_is_valid(edid)) { - ret = drm_connector_update_edid_property(connector, edid); - if (!ret) { - num = drm_add_edid_modes(connector, edid); - if (num) - return num; - } + num = drm_add_edid_modes(connector, edid); + if (num) + return num; }
return drm_panel_get_modes(pdata->panel, connector);
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Regards Andrzej
Each time we call drm_get_edid() we: 1. Go out to the bus and ask for the EDID. 2. Cache the EDID.
We can improve this to actually use the cached EDID so that if drm_get_edid() is called multiple times then we don't need to go out to the bus over and over again.
In normal DP/HDMI cases reading the EDID over and over again isn't _that_ expensive so, presumably, this wasn't that critical in the past. However for eDP going out to the bus can be expensive. This is because eDP panels might be powered off before the EDID was requested so we need to do power sequencing in addition to the transfer.
In theory we should be able to cache the EDID for all types of displays. There is already code throwing the cache away when we detect that a display was unplugged. However, it can be noted that it's _extra_ safe to cache the EDID for eDP since eDP isn't a hot-pluggable interface. If we get the EDID once then we've got the EDID and we should never need to read it again. For now we'll only use the cache for eDP both because it's more important and extra safe.
Signed-off-by: Douglas Anderson dianders@chromium.org ---
(no changes since v1)
drivers/gpu/drm/drm_edid.c | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index c2bbe7bee7b6..fcbf468d73c9 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2049,15 +2049,39 @@ struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { struct edid *edid; + size_t old_edid_size; + const struct edid *old_edid;
if (connector->force == DRM_FORCE_OFF) return NULL;
- if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter)) - return NULL; + if (connector->connector_type == DRM_MODE_CONNECTOR_eDP && + connector->edid_blob_ptr) { + /* + * eDP devices are non-removable, or at least not something + * that's expected to be hot-pluggable. We can freely use + * the cached EDID. + * + * NOTE: technically we could probably even use the cached + * EDID even for non-eDP because the cached EDID should be + * cleared if we ever notice a display is not connected, but + * we'll use an abundance of caution and only do it for eDP. + * It's more important for eDP anyway because the EDID may not + * always be readable, like when the panel is powered down. + */ + old_edid = (const struct edid *)connector->edid_blob_ptr->data; + old_edid_size = ksize(old_edid); + edid = kmalloc(old_edid_size, GFP_KERNEL); + if (edid) + memcpy(edid, old_edid, old_edid_size); + } else { + if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter)) + return NULL; + + edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); + drm_connector_update_edid_property(connector, edid); + }
- edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); - drm_connector_update_edid_property(connector, edid); return edid; } EXPORT_SYMBOL(drm_get_edid);
On Mon, Mar 29, 2021 at 07:53:40PM -0700, Douglas Anderson wrote:
This is a pretty low level function. Too low level for this caching IMO. So I think better just do it a bit higher up like other drivers.
Hi,
On Tue, Mar 30, 2021 at 7:01 AM Ville Syrjälä ville.syrjala@linux.intel.com wrote:
Fair enough. In the past I'd gotten feedback that I'd been jamming too much stuff in my own driver instead of putting it in the core, but I'm happy to leave the EDID caching in the driver if that's what people prefer. It actually makes a bit of the code in the driver a bit less awkward...
-Doug
Now that we have the patch ("drm/edid: Use the cached EDID in drm_get_edid() if eDP") we no longer need to maintain our own cache. Drop this code.
Signed-off-by: Douglas Anderson dianders@chromium.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 9577ebd58c4c..c0398daaa4a6 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -121,7 +121,6 @@ * @debugfs: Used for managing our debugfs. * @host_node: Remote DSI node. * @dsi: Our MIPI DSI source. - * @edid: Detected EDID of eDP panel. * @refclk: Our reference clock. * @panel: Our panel. * @enable_gpio: The GPIO we toggle to enable the bridge. @@ -147,7 +146,6 @@ struct ti_sn_bridge { struct drm_bridge bridge; struct drm_connector connector; struct dentry *debugfs; - struct edid *edid; struct device_node *host_node; struct mipi_dsi_device *dsi; struct clk *refclk; @@ -269,17 +267,17 @@ connector_to_ti_sn_bridge(struct drm_connector *connector) static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) { struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector); - struct edid *edid = pdata->edid; - int num; + struct edid *edid; + int num = 0;
- if (!edid) { - pm_runtime_get_sync(pdata->dev); - edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc); - pm_runtime_put(pdata->dev); - } + pm_runtime_get_sync(pdata->dev); + edid = drm_get_edid(connector, &pdata->aux.ddc); + pm_runtime_put(pdata->dev);
- if (edid && drm_edid_is_valid(edid)) { - num = drm_add_edid_modes(connector, edid); + if (edid) { + if (drm_edid_is_valid(edid)) + num = drm_add_edid_modes(connector, edid); + kfree(edid); if (num) return num; } @@ -1308,8 +1306,6 @@ static int ti_sn_bridge_remove(struct i2c_client *client) if (!pdata) return -EINVAL;
- kfree(pdata->edid); - ti_sn_debugfs_remove(pdata);
drm_bridge_remove(&pdata->bridge);
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Regards Andrzej
Hi,
On Wed, Mar 31, 2021 at 3:12 AM Andrzej Hajda a.hajda@samsung.com wrote:
Note that unless the advice given to me changes, I'm dropping ${SUBJECT} patch on the next spin and putting the EDID cache back in the bridge driver. See:
https://lore.kernel.org/r/YGMvO3PNDCiBmqov@intel.com/
-Doug
eDP panels won't provide their EDID unless they're powered on. Let's chain a power-on before we read the EDID. This roughly matches what was done in 'parade-ps8640.c'.
NOTE: The old code attempted to call pm_runtime_get_sync() before reading the EDID. While that was enough to power the bridge chip on, it wasn't enough to talk to the panel for two reasons: 1. Since we never ran the bridge chip's pre-enable then we never set the bit to ignore HPD. This meant the bridge chip didn't even _try_ to go out on the bus and communicate with the panel. 2. Even if we fixed things to ignore HPD, the EDID still wouldn't read if the panel wasn't on.
One thing that's a bit odd here is taking advantage of the EDID that the core might have cached for us. See the patch ("drm/edid: Use the cached EDID in drm_get_edid() if eDP"). We manage to get at the cache by: - Instantly failing aux transfers if we're not powered. - If the first read of the EDID fails we try again after powering.
Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC") Signed-off-by: Douglas Anderson dianders@chromium.org --- Depending on what people think of the other patches in this series, some of this could change. - If everyone loves the "runtime PM" in the panel driver then we could, in theory, put the pre-enable chaining straight in the "aux transfer" function. - If everyone hates the EDID cache moving to the core then we can avoid some of the awkward flow of things and keep the EDID cache in the sn65dsi86 driver.
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 +++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index c0398daaa4a6..673c9f1c2d8e 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -128,6 +128,7 @@ * @dp_lanes: Count of dp_lanes we're using. * @ln_assign: Value to program to the LN_ASSIGN register. * @ln_polrs: Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG. + * @pre_enabled: If true then pre_enable() has run. * * @gchip: If we expose our GPIOs, this is used. * @gchip_output: A cache of whether we've set GPIOs to output. This @@ -155,6 +156,7 @@ struct ti_sn_bridge { int dp_lanes; u8 ln_assign; u8 ln_polrs; + bool pre_enabled;
#if defined(CONFIG_OF_GPIO) struct gpio_chip gchip; @@ -268,11 +270,33 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) { struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector); struct edid *edid; + bool was_enabled; int num = 0;
- pm_runtime_get_sync(pdata->dev); + /* + * Try to get the EDID first without anything special. There are + * three things that could happen with this call. + * a) It might just return from its cache. + * b) It might try to initiate an AUX transfer which might work. + * c) It might try to initiate an AUX transfer which might fail because + * we're not powered up. + * + * If we get a failure we'll assume case c) and try again. NOTE: we + * don't want to power up every time because that's slow and we don't + * have visibility into whether the data has already been cached. + */ edid = drm_get_edid(connector, &pdata->aux.ddc); - pm_runtime_put(pdata->dev); + if (!edid) { + was_enabled = pdata->pre_enabled; + + if (!was_enabled) + drm_bridge_chain_pre_enable(&pdata->bridge); + + edid = drm_get_edid(connector, &pdata->aux.ddc); + + if (!was_enabled) + drm_bridge_chain_post_disable(&pdata->bridge); + }
if (edid) { if (drm_edid_is_valid(edid)) @@ -852,12 +876,16 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge) HPD_DISABLE);
drm_panel_prepare(pdata->panel); + + pdata->pre_enabled = true; }
static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) { struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+ pdata->pre_enabled = false; + drm_panel_unprepare(pdata->panel);
clk_disable_unprepare(pdata->refclk); @@ -891,6 +919,13 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, int ret; u8 addr_len[SN_AUX_LENGTH_REG + 1 - SN_AUX_ADDR_19_16_REG];
+ /* + * Things just won't work if the panel isn't powered. Return failure + * right away. + */ + if (!pdata->pre_enabled) + return -EIO; + if (len > SN_AUX_MAX_PAYLOAD_BYTES) return -EINVAL;
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
I wonder if this shouldn't be solved in the core - ie caller of get_modes callback should be responsible for powering up the pipeline, otherwise we need to repeat this stuff in every bridge/panel driver.
Any thoughts?
Regards
Andrzej
Hi,
On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda a.hajda@samsung.com wrote:
Yeah, I did look at this a little bit. Presumably it would only make sense to do it for eDP connections since:
a) The concept of reading an EDID doesn't make sense for things like MIPI.
b) For something with an external connector (DP and HDMI) you don't even know they're inserted unless the EDID is ready to read (these devices are, essentially, always powered).
So I started off trying to do this in the core for eDP, but then it wasn't completely clear how to write this code in a way that was super generic. Specifically:
1. I don't think it's a 100% guarantee that everything is powered on in pre-enable and powered off in post-disable. In this bridge chip it's true, but maybe not every eDP driver? Would you want me to just assume this, or add a flag?
2. It wasn't totally clear to me which state to use for telling if the bridge had already been pre-enabled so I could avoid double-calling it. I could dig more if need be but I spent a bit of time looking and was coming up empty. If you have advice I'd appreciate it, though.
3. It wasn't clear to me if I should be using the atomic version (drm_atomic_bridge_chain_pre_enable) if I put this in the core and how exactly to do this, though I am a self-admitted DRM noob. I can do more digging if need be. Again, advice is appreciated.
4. Since I got feedback that the EDID caching belongs in the driver, not in the core [1] then we might end up powering things up pointlessly since the core wouldn't know if the driver was going to return the cache or not.
Given that this patch isn't too much code and not too complicated (and will be even less complicated if I move the EDID caching back into the driver), maybe we can land it and if we see the pattern repeat a bunch more times then think about moving it to the core?
[1] https://lore.kernel.org/dri-devel/YGMvO3PNDCiBmqov@intel.com/
-Doug
W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
I guess you mean MIPI DSI, and yes I agree, more generally it usually(!) doesn't make sense for any setup with fixed display panel.
On the other hand there are DSI/HDMI or DSI/DP adapters which usually have EDID reading logic.
And the concept makes sense for most connectors accepting external displays: HDMI, DP, MHL, VGA...
Usually there are two elements which are not the same:
1. HotPlug signal/wire.
2. EDID reading logic.
The logic responsible for reading EDID needs to be enabled only for time required for EDID reading :) So it's power state often must be controlled explicitly by the bridge driver. So even if in many cases pre_enable powers on the logic for EDID reading it does not make it the rule, so I must step back from my claim that it is up to caller :)
Ok, pre_enable should power on the chip, but for performing initialization of video transport layer. Assumption it will power on EDID logic is incorrect, so my claim seems wrong, but also this patch looks incorrect :)
In general only device containing EDID logic knows how to power it up.
Since I do not know your particular case I can propose few possible ways to investigate:
- call bridge.next->get_modes - you leave responsibility for powering up to the downstream device.
- ddc driver on i2c request should power up the panel - seems also correct,
Regards
Andrzej
Hi,
On Thu, Apr 1, 2021 at 4:12 AM Andrzej Hajda a.hajda@samsung.com wrote:
Yes, sorry! I'll try to be more clear.
So, actually, IMO the concept doesn't make sense for anything with an external connector. Here's the logic for a handful of connectors:
1. MIPI DSI: there is no EDID so this doesn't make sense.
2. An external connector (HDMI, DP, etc): the display that's plugged in is externally powered so doesn't need us to power it up to read the EDID. By definition, when the HPD signal is asserted then it's OK to read the EDID and we don't even know if a display is plugged in until HPD is asserted. Thus no special power sequencing is needed to read the EDID. (Yes, we need to make sure that the eDP controller itself is powered, but that doesn't seem like it's the core's business).
3. eDP: this is where it matters. This is because:
3a) eDP displays aren't powered all the time. If you just boot up or you blank your screen, likely the display has no power at all.
3b) Because the display has no power, the "HPD" signal doesn't assert. In fact, for eDP the "HPD" signal really should mean "display ready" or "display finished powering up".
3c) Even though we never get a HPD signal, we still simply assume that a display is present because this is an "embedded" device.
So eDP is unique (as far as I know) in that it's a type of display that has an EDID but that we will report "a display is here" before we've powered up the display and before we can read the EDID.
OK, I'll plan to keep it in the bridge chip driver now.
I still believe my patch is correct. Specifically I don't need to make any assumptions about display elements upstream of me (sources of the bridge chip). I only need to make assumptions about the pre-enable of the bridge driver itself and anything downstream of it.
At the moment downstream of this particular bridge chip is always a panel device. Even further, all known downstream devices are "simple-panel". That is known to power up the panel enough to read the EDID in the "prepare" stage.
Sure, someone _could_ add another bridge downstream in some design, but it would be up to that person to either fix that downstream driver to power itself in pre-enable or to add some type of quirk disabling the EDID reading.
The "next" bridge is the panel, so I don't think this works.
Right, so I could put the "drm_bridge_chain_pre_enable(&pdata->bridge)" into the ti_sn_aux_transfer() function. I talked about that a little bit "after the cut" in my post where I said:
The reason for the dependence on "runtime PM" in the panel driver is that we are doing DDC over AUX and it breaks the EDID reading into lots of chunks so if we did the powering up and powering down there it would be crazy slow without the delayed poweroff.
-Doug
Hello again after easter,
I have looked little bit more at sn65* driver and its application to have better background.
I miss only info what panel do you have, how it is enabled/power controlled.
W dniu 01.04.2021 o 16:57, Doug Anderson pisze:
Not true IMO, even if external device is powered on, you must enable EDID-reader logic.
I guess it is not uncommon to have different power states for EDID reading and bridge/panel pre-enablement (especially in embedded world). In fact there are setups where EDID-reader is totally different device than the bridge itself, and these devices should be powered/enabled/operational only for time of EDID reading.
Then drm_panel_get_modes will work then.
OK, it resembles to me DSI-controlled panel - to query/configure panel panel driver asks DSI-host to transfer some bytes to the panel and/or back via DSI-bus.
In case of eDP panels we could do similar thing to read edid - we call drm_panel_get_modes - it calls drm_panel_funcs.get_modes callback and it decides (based on DT) if it should fill modes according to hardcoded info into the driver or to ask the physical panel via DP-controller - this way all the players (the panel, AUX/DDC device) will know what to power-up.
I guess there is missing pieces - there is no DP bus :), I am not sure if there is straight way to access panel's aux/ddc from the panel driver, maybe somehow via drm_connector ???
Of course this only my idea - to be discussed with others.
Regards
Andrzej
-Doug
Hi Andrzej,
On Tue, Apr 06, 2021 at 06:52:07PM +0200, Andrzej Hajda wrote:
Sure, but I think Doug was referring to powering up the device connected to the SN65DSI86 output. When that device (from a DT and DRM bridge point of view) is an external connector, it means that the hardware device is an external HDMI/DP sink, and we have no way to control its power. The SN65DSI86 itself of course needs to be powered.
Not if the panel exposes modes through EDID, in that case it's the responsibility of the device connected to the DDC/AUX port to read the EDID and provide modes. The panel driver won't be able to handle it on its own.
If the SN65DSI86 has to call drm_panel_get_modes(), which will then call back into the SN65DSI86 driver to perform the EDID read, it seems to me that the panel driver shouldn't be involved at all.
DRM bridges have "recently" gained new operations to retrieve EDID, and there's a helper (drm_bridge_connector) that creates a connector for a chain of bridges, delegating connector operations to the appropriate bridge in the chain. This seems a better way forward to me (but I'm biased, as I've authored that code :-)).
Of course this only my idea - to be discussed with others.
Hi,
On Tue, Apr 6, 2021 at 9:52 AM Andrzej Hajda a.hajda@samsung.com wrote:
Hello again after easter,
Sorry, I was out last week and I'm just getting back to this now.
I am personally working on "sc7180-trogdor" boards right now (arch/arm64/boot/dts/qcom/sc7180-trogdor*.dts). However I believe that this patch series also enables proper EDID reading on "sdm850-lenovo-yoga-c630.dts". That board, while also a Qualcomm board, has completely different heritage than the trogdor ones. It's a laptop that ships with Windows and (as far as I know) was designed mostly independently.
On the trogdor boards the bridge has some power rails and an enable line hooked up to it and the bridge itself can work when these rails are turned on. The panel is on a separate power rail and you can't talk to the panel at all until it's powered on and then asserts HPD to us to say it finished its boot sequence.
Right, I'm not saying that no components on the motherboard need to be powered in order to read the EDID. I'm only saying that:
1. For external DP we can't know if the sink is there until HPD is asserted.
2. For external DP we have to provide some power to the sink _before_ the sync can assert HPD.
3. As soon as the sink asserts HPD (subject to debouncing rules) we can immediately read the EDID as far as the sink is concerned
Said another way: we never get into a state with external DP where we want to read an EDID of an unpowered sink because we don't even know that the sink is there until it is powered and we can read the EDID from it.
As far as powering up components on the motherboard goes, I think that's a much simpler problem and I don't think we have to worry about it here, do we? This falls squarely in the purvue of the bridge driver itself or, if the DDC bus is some other i2c bus, it should be handled by the normal methods.
OK, I can see how this could work. At least in simple-panel there is already a "ddc-i2c-bus" property that can be used to give the panel access to the DDC bus. Today, when simple-panel tries to use this, it _doesn't_ power on the panel first. I'm not sure how/why that works. I guess maybe it doesn't actually work (you run with a hardcoded mode the first time?) or all the current users have panels where you can always read the EDID? We could probably co-opt this and, possibly, add a boolean property in the simple-panel struct saying whether a panel needs to be turned on to read its modes?
One issue, though, is that we're going to end up with the chicken-and-egg problem again. The bridge chip provides the DDC bus at "attach" time. The panel won't probe until the DDC bus is there. The bridge chip won't probe (and certainly won't attach) until the panel is there.
OK, so as I was writing this it looks like Laurent responded too... So I think Laurent is saying that keeping the EDID reading in the bridge chip (like I'm doing) is fine. I was debating this a bit with myself this afternoon...
Part of me thought: there's no reason to reorganize the world to solve the chicken-and-egg problem. The bridge chip can read the EDID fine and I think that for this bridge chip it's pretty easy to argue that after pre-enable of the bridge (and anything after it) that it's valid to read the EDID. Yes, you could organize it like you said but I wasn't sure the advantages. It felt like the bridge chip chaining was designed specifically so that the EDID reading could be in the bridge like I was doing...
...but then another part of me was thinking about another patch series that was just sent out:
https://lore.kernel.org/r/1618418390-15055-1-git-send-email-rajeevny@codeaur...
In that patch series we need to figure out how to control the backlight of a panel which requires DDC transactions to happen. It might (?) make sense to have that backlight control in the panel driver (maybe?) and doing so would require exposing the "ddc" bus to the panel driver. Once you start thinking of doing that then Andrzej's proposal maybe makes more sense? I'm definitely looking for guidance here.
I did try to prototype up Andrzej's and I feel like I was close but there was still a bug or two, and my chicken-and-egg solution was a big hack, too. I'll plan to keep poking at it tomorrow unless you say I shouldn't.
-Doug
Though I don't have access to any hardware that uses ti-sn65dsi86 and _doesn't_ provide a "refclk", I believe that we'll have trouble reading the EDID at bootup in that case. Specifically I believe that if there's no "refclk" we need the MIPI source clock to be active before we can successfully read the EDID. My evidence here is that, in testing, I couldn't read the EDID until I turned on the DPPLL in the bridge chip and that the DPPLL needs the input clock to be active.
Since this is hard to support, let's punt trying to read the EDID if there's no "refclk".
I don't believe there are any users of the ti-sn65dsi86 bridge chip that _don't_ use "refclk". The bridge chip is _very_ inflexible in that mode. The only time I've seen that mode used was for some really early prototype hardware that was thrown in the e-waste bin years ago when we realized how inflexible it was.
Even if someone is using the bridge chip without the "refclk" they're in no worse shape than they were before the (fairly recent) commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC").
Signed-off-by: Douglas Anderson dianders@chromium.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 673c9f1c2d8e..92498900c58d 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -273,6 +273,18 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) bool was_enabled; int num = 0;
+ /* + * Don't try to read the EDID if no refclk. In theory it is possible + * to make this work but it's tricky. I believe that we need to get + * our upstream MIPI source to provide a pixel clock before we can + * do AUX transations but we need to be able to read the EDID before + * we've picked a display mode. The bridge is already super limited + * if you try to use it without a refclk so presumably limiting to + * the fixed modes our downstream panel reports is fine. + */ + if (!pdata->refclk) + goto exit; + /* * Try to get the EDID first without anything special. There are * three things that could happen with this call. @@ -306,6 +318,7 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) return num; }
+exit: return drm_panel_get_modes(pdata->panel, connector); }
Now that we can properly read the EDID for modes there should be no reason to fallback to the fixed modes that our downstream panel driver provides us. Let's make that clear by: - Putting an error message in the logs if we fall back. - Putting a comment in saying what's going on.
Signed-off-by: Douglas Anderson dianders@chromium.org ---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 92498900c58d..20c3b13939c2 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -318,6 +318,13 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) return num; }
+ /* + * Ideally this should never happen and we could remove the fallback + * but let's preserve old behavior. + */ + DRM_DEV_ERROR(pdata->dev, + "Failed to read EDID; falling back to panel modes"); + exit: return drm_panel_get_modes(pdata->panel, connector); }
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. - 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".
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,
dri-devel@lists.freedesktop.org