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.
Primary change between v2 and v3 is to stop doing the EDID caching in the core. I also added Andrzej's review tags.
[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c7... [2] https://lore.kernel.org/r/161706912161.3012082.17313817257247946143@swboyd.m...
Changes in v3: - Removed "NOTES" from commit message. - Rebased now that we're not moving EDID caching to the core. - Separating out patch to block AUX channel when not powered. - Added note about boot speed implications. - ("Fail aux transfers right away if not powered") split out for v3.
Changes in v2: - Removed 2nd paragraph in commit message.
Douglas Anderson (12): 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 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/bridge: ti-sn65dsi86: Power things properly for reading the EDID drm/bridge: ti-sn65dsi86: Fail aux transfers right away if not powered 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 | 97 ++++++++++++++++----------- drivers/gpu/drm/drm_bridge.c | 3 + drivers/gpu/drm/panel/Kconfig | 1 + drivers/gpu/drm/panel/panel-simple.c | 93 +++++++++++++++++++------ 4 files changed, 134 insertions(+), 60 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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com ---
(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 Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com
(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;
This looks good as it matches drm_atomic_bridge_chain_disable().
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
I'm curious though, given that the bridge passed to the function should be the one closest to the encoder, does this make a difference ?
} } EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
Hi,
On Sun, Apr 4, 2021 at 5:50 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com
(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;This looks good as it matches drm_atomic_bridge_chain_disable().
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Thanks for your review here and several of the other patches. Can you suggest any plan for getting them landed? It would at least be nice to get the non-controversial ones landed.
I'm curious though, given that the bridge passed to the function should be the one closest to the encoder, does this make a difference ?
Yes, that's how I discovered it originally. Let's see. So if I don't have this patch but have the rest of the series then I get a splat at bootup. This shows that dsi_mgr_bridge_pre_enable() must be "earlier" in the chain than my bridge chip. Here's the splat:
msm_dsi_host_get_phy_clk_req: unable to calc clk rate, -22 ------------[ cut here ]------------ disp_cc_mdss_ahb_clk status stuck at 'off' WARNING: CPU: 7 PID: 404 at drivers/clk/qcom/clk-branch.c:92 clk_branch_toggle+0x194/0x280 Modules linked in: joydev CPU: 7 PID: 404 Comm: frecon Tainted: G B 5.12.0-rc3-lockdep+ #2 Hardware name: Google Lazor (rev1 - 2) with LTE (DT) pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--) pc : clk_branch_toggle+0x194/0x280 lr : clk_branch_toggle+0x190/0x280 ... Call trace: clk_branch_toggle+0x194/0x280 clk_branch2_enable+0x28/0x34 clk_core_enable+0x2f4/0x6b4 clk_enable+0x54/0x74 dsi_phy_enable_resource+0x80/0xd8 msm_dsi_phy_enable+0xa8/0x4a8 enable_phy+0x9c/0xf4 dsi_mgr_bridge_pre_enable+0x23c/0x4b0 drm_bridge_chain_pre_enable+0xac/0xe4 ti_sn_bridge_connector_get_modes+0x134/0x1b8 drm_helper_probe_single_connector_modes+0x49c/0x1358 drm_mode_getconnector+0x460/0xe98 drm_ioctl_kernel+0x144/0x228 drm_ioctl+0x418/0x7cc drm_compat_ioctl+0x1bc/0x230 __arm64_compat_sys_ioctl+0x14c/0x188 el0_svc_common+0x128/0x23c do_el0_svc_compat+0x50/0x60 el0_svc_compat+0x24/0x34 el0_sync_compat_handler+0xc0/0xf0 el0_sync_compat+0x174/0x180
Hi Doug,
On Wed, Apr 14, 2021 at 06:19:13PM -0700, Doug Anderson wrote:
On Sun, Apr 4, 2021 at 5:50 PM Laurent Pinchart wrote:
On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com
(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;This looks good as it matches drm_atomic_bridge_chain_disable().
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Thanks for your review here and several of the other patches. Can you suggest any plan for getting them landed? It would at least be nice to get the non-controversial ones landed.
Do you have commit access to drm-misc ? If not, given your contributions, I think you qualify for it.
I'm curious though, given that the bridge passed to the function should be the one closest to the encoder, does this make a difference ?
Yes, that's how I discovered it originally. Let's see. So if I don't have this patch but have the rest of the series then I get a splat at bootup. This shows that dsi_mgr_bridge_pre_enable() must be "earlier" in the chain than my bridge chip. Here's the splat:
Right, I think it's caused by a later patch in the series calling this function with a different bridge than the one closest to the encoder.
msm_dsi_host_get_phy_clk_req: unable to calc clk rate, -22 ------------[ cut here ]------------ disp_cc_mdss_ahb_clk status stuck at 'off' WARNING: CPU: 7 PID: 404 at drivers/clk/qcom/clk-branch.c:92 clk_branch_toggle+0x194/0x280 Modules linked in: joydev CPU: 7 PID: 404 Comm: frecon Tainted: G B 5.12.0-rc3-lockdep+ #2 Hardware name: Google Lazor (rev1 - 2) with LTE (DT) pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--) pc : clk_branch_toggle+0x194/0x280 lr : clk_branch_toggle+0x190/0x280 ... Call trace: clk_branch_toggle+0x194/0x280 clk_branch2_enable+0x28/0x34 clk_core_enable+0x2f4/0x6b4 clk_enable+0x54/0x74 dsi_phy_enable_resource+0x80/0xd8 msm_dsi_phy_enable+0xa8/0x4a8 enable_phy+0x9c/0xf4 dsi_mgr_bridge_pre_enable+0x23c/0x4b0 drm_bridge_chain_pre_enable+0xac/0xe4 ti_sn_bridge_connector_get_modes+0x134/0x1b8 drm_helper_probe_single_connector_modes+0x49c/0x1358 drm_mode_getconnector+0x460/0xe98 drm_ioctl_kernel+0x144/0x228 drm_ioctl+0x418/0x7cc drm_compat_ioctl+0x1bc/0x230 __arm64_compat_sys_ioctl+0x14c/0x188 el0_svc_common+0x128/0x23c do_el0_svc_compat+0x50/0x60 el0_svc_compat+0x24/0x34 el0_sync_compat_handler+0xc0/0xf0 el0_sync_compat+0x174/0x180
Hi,
On Wed, Apr 14, 2021 at 6:56 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Doug,
On Wed, Apr 14, 2021 at 06:19:13PM -0700, Doug Anderson wrote:
On Sun, Apr 4, 2021 at 5:50 PM Laurent Pinchart wrote:
On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com
(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;This looks good as it matches drm_atomic_bridge_chain_disable().
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Thanks for your review here and several of the other patches. Can you suggest any plan for getting them landed? It would at least be nice to get the non-controversial ones landed.
Do you have commit access to drm-misc ? If not, given your contributions, I think you qualify for it.
No, I don't have access. I searched for how to get it and read through the qualifications and, you're right, I think I do. I've hopefully followed the right flow and created an issue to give me ssh access:
https://gitlab.freedesktop.org/freedesktop/freedesktop/-/issues/348
Is that something you (or someone else on this CC list) approves?
I'm curious though, given that the bridge passed to the function should be the one closest to the encoder, does this make a difference ?
Yes, that's how I discovered it originally. Let's see. So if I don't have this patch but have the rest of the series then I get a splat at bootup. This shows that dsi_mgr_bridge_pre_enable() must be "earlier" in the chain than my bridge chip. Here's the splat:
Right, I think it's caused by a later patch in the series calling this function with a different bridge than the one closest to the encoder.
Yup! I still wanted this patch to be first in the series, though, since it's a bugfix that we'd want to land even if the later patches changed in some way.
-Doug
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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com ---
(no changes since v2)
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)
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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com ---
(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
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:37PM -0700, Douglas Anderson wrote:
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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
(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
Let's make the remove() function strictly the reverse of the probe() function so it's easier to reason about.
This patch was created by code inspection and should move us closer to a proper remove.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Andrzej Hajda a.hajda@samsung.com ---
Changes in v3: - Removed "NOTES" from commit message.
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; }
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:38PM -0700, Douglas Anderson wrote:
Let's make the remove() function strictly the reverse of the probe() function so it's easier to reason about.
This patch was created by code inspection and should move us closer to a proper remove.
Signed-off-by: Douglas Anderson dianders@chromium.org Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Changes in v3:
- Removed "NOTES" from commit message.
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;
}
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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com ---
(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 c006678c9921..e30460002c48 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -452,8 +452,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) @@ -869,6 +867,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);
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:39PM -0700, Douglas Anderson wrote:
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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
(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 c006678c9921..e30460002c48 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -452,8 +452,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) @@ -869,6 +867,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);
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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com ---
(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 e30460002c48..51db30d573c1 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,
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:40PM -0700, Douglas Anderson wrote:
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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
(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 e30460002c48..51db30d573c1 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,
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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com ---
(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 51db30d573c1..6390bc58f29a 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);
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:41PM -0700, Douglas Anderson wrote:
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 Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
This looks like a widespread issue, would you be able to send a patch to address all the other drivers ?
(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 51db30d573c1..6390bc58f29a 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);
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.
ALSO NOTE: Without the future patch ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare") there will be boot speed implications here. Specifically we'll power the panel on to read the EDID, then fully off. Then we'll likely have to wait the minimum time between power off and power on.
Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC") Signed-off-by: Douglas Anderson dianders@chromium.org ---
Changes in v3: - Rebased now that we're not moving EDID caching to the core. - Separating out patch to block AUX channel when not powered. - Added note about boot speed implications.
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 6390bc58f29a..543590801a8e 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -129,6 +129,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 @@ -157,6 +158,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; @@ -270,12 +272,17 @@ 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; + bool was_enabled; int num;
if (!edid) { - pm_runtime_get_sync(pdata->dev); + was_enabled = pdata->pre_enabled; + + if (!was_enabled) + drm_bridge_chain_pre_enable(&pdata->bridge); edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc); - pm_runtime_put(pdata->dev); + if (!was_enabled) + drm_bridge_chain_post_disable(&pdata->bridge); }
if (edid && drm_edid_is_valid(edid)) { @@ -846,12 +853,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);
If the bridge (and panel) haven't been powered on then AUX transfers just won't work. Let's just fail them instantly.
Signed-off-by: Douglas Anderson dianders@chromium.org --- If the patch ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare") is accepted then we could consider actually powering the panel on instead of failing the transfer. However, without that patch the overhead would just be too much since we need to do several AUX transfers for a single EDID read and powering up and down each time would just be too much.
Changes in v3: - ("Fail aux transfers right away if not powered") split out for v3.
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 543590801a8e..a76cac93cb46 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -896,6 +896,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;
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 a76cac93cb46..fb50f9f95b0f 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -275,6 +275,18 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) bool was_enabled; int num;
+ /* + * 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; + if (!edid) { was_enabled = pdata->pre_enabled;
@@ -291,6 +303,7 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) return num; }
+exit: return drm_panel_get_modes(pdata->panel, connector); }
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:44PM -0700, Douglas Anderson wrote:
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
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
(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 a76cac93cb46..fb50f9f95b0f 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -275,6 +275,18 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) bool was_enabled; int num;
- /*
* 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;- if (!edid) { was_enabled = pdata->pre_enabled;
@@ -291,6 +303,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 fb50f9f95b0f..3b61898cf9cb 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -303,6 +303,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); }
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:45PM -0700, Douglas Anderson wrote:
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
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
(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 fb50f9f95b0f..3b61898cf9cb 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -303,6 +303,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,
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()):
- 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.
- 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);@@ -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);
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,@@ -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,
Hi,
On Wed, Apr 14, 2021 at 5:58 PM Laurent Pinchart laurent.pinchart@ideasonboard.com 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()):
- 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.
- 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 laurent.pinchart@ideasonboard.com
Thanks!
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()):
- 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.
- 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 laurent.pinchart@ideasonboard.com
dri-devel@lists.freedesktop.org
-
Doug Anderson -
Douglas Anderson
-
Laurent Pinchart