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.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
[2] https://lore.kernel.org/r/161706912161.3012082.17313817257247946143@swboyd.mtv.corp.google.com/
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(-)
--
2.31.0.291.g576ba9dcdaf-goog
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 <[email protected]>
---
(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);
--
2.31.0.291.g576ba9dcdaf-goog
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 <[email protected]>
---
(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;
}
--
2.31.0.291.g576ba9dcdaf-goog
A random comment inside a function had "/**" in front of it. That
doesn't make sense. Remove.
Signed-off-by: Douglas Anderson <[email protected]>
---
(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
--
2.31.0.291.g576ba9dcdaf-goog
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 <[email protected]>
---
(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);
--
2.31.0.291.g576ba9dcdaf-goog
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 <[email protected]>
Reviewed-by: Robert Foss <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
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)
--
2.31.0.291.g576ba9dcdaf-goog
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 <[email protected]>
---
(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);
--
2.31.0.291.g576ba9dcdaf-goog
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 <[email protected]>
---
(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,
--
2.31.0.291.g576ba9dcdaf-goog
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 <[email protected]>
---
(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);
--
2.31.0.291.g576ba9dcdaf-goog
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 <[email protected]>
---
(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);
}
--
2.31.0.291.g576ba9dcdaf-goog
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 <[email protected]>
---
(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,
--
2.31.0.291.g576ba9dcdaf-goog
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 <[email protected]>
---
(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);
--
2.31.0.291.g576ba9dcdaf-goog
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 <[email protected]>
---
(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);
--
2.31.0.291.g576ba9dcdaf-goog
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 <[email protected]>
---
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;
--
2.31.0.291.g576ba9dcdaf-goog
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 <[email protected]>
---
(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);
}
--
2.31.0.291.g576ba9dcdaf-goog
On Mon, Mar 29, 2021 at 07:53:40PM -0700, Douglas Anderson wrote:
> 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 <[email protected]>
> ---
>
> (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);
> + }
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.
>
> - 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);
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrj?l?
Intel
Hi,
On Tue, Mar 30, 2021 at 7:01 AM Ville Syrjälä
<[email protected]> wrote:
>
> > @@ -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);
> > + }
>
> 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.
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
Hi Douglas,
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> 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 <[email protected]>
> ---
>
> (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;
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 <[email protected]>
Regards
Andrzej
> }
> }
> EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> 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 <[email protected]>
> Reviewed-by: Robert Foss <[email protected]>
> Reviewed-by: Bjorn Andersson <[email protected]>
> Reviewed-by: Stephen Boyd <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
Regards
Andrzej
> ---
>
> 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:
> A random comment inside a function had "/**" in front of it. That
> doesn't make sense. Remove.
>
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
Regards
Andrzej
> ---
>
> (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:
> 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 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.
> - 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 <[email protected]>
> ---
>
> (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);
> +
Looks good.
Reviewed-by: Andrzej Hajda <[email protected]>
Regards
Andrzej
> return 0;
> }
>
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> The register() / attach() for MIPI happen in the bridge's
> attach(). That means that the inverse belongs in the bridge's
> detach().
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
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> (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:
> 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 <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
Regards
Andrzej
> ---
>
> (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:
> 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 <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
Regards
Andrzej
> ---
>
> (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:
> 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 <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
Regards
Andrzej
> ---
>
> (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:
> 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 <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
Regards
Andrzej
> ---
>
> (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:
> 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 <[email protected]>
> ---
> 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.
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
>
> (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;
>
Hi,
On Wed, Mar 31, 2021 at 3:12 AM Andrzej Hajda <[email protected]> wrote:
>
>
> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> > 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 <[email protected]>
> Reviewed-by: Andrzej Hajda <[email protected]>
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/[email protected]/
-Doug
Hi,
On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda <[email protected]> wrote:
>
>
> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> > 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 <[email protected]>
> > ---
> > 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.
>
>
> 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?
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/[email protected]/
-Doug
Hi,
On Wed, Mar 31, 2021 at 2:53 AM Andrzej Hajda <[email protected]> wrote:
>
>
> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> > The register() / attach() for MIPI happen in the bridge's
> > attach(). That means that the inverse belongs in the bridge's
> > detach().
>
>
> 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?
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
W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
> Hi,
>
> On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda <[email protected]> wrote:
>>
>> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
>>> 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 <[email protected]>
>>> ---
>>> 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.
>>
>> 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?
> 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.
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...
>
> 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).
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 :)
>
> 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?
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
>
> 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/[email protected]/
>
> -Doug
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://protect2.fireeye.com/v1/url?k=e133dd76-bea8e47a-e1325639-000babff3563-c6b07779450426d5&q=1&e=fd12ab0a-1858-4d09-a3b6-0ff1336fc2ba&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>
Hi,
On Thu, Apr 1, 2021 at 4:12 AM Andrzej Hajda <[email protected]> wrote:
>
>
> W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
> > Hi,
> >
> > On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda <[email protected]> wrote:
> >>
> >> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> >>> 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 <[email protected]>
> >>> ---
> >>> 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.
> >>
> >> 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?
> > 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.
>
> I guess you mean MIPI DSI
Yes, sorry! I'll try to be more clear.
> 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...
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.
> > 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).
>
> 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, I'll plan to keep it in the bridge chip driver now.
> > 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?
>
> 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.
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.
> 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.
The "next" bridge is the panel, so I don't think this works.
> - ddc driver on i2c request should power up the panel - seems also correct,
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:
> - 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.
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:
> Hi,
>
> On Thu, Apr 1, 2021 at 4:12 AM Andrzej Hajda <[email protected]> wrote:
>>
>> W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
>>> Hi,
>>>
>>> On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda <[email protected]> wrote:
>>>> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
>>>>> 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 <[email protected]>
>>>>> ---
>>>>> 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.
>>>> 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?
>>> 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.
>> I guess you mean MIPI DSI
> Yes, sorry! I'll try to be more clear.
>
>
>> 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...
> 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).
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.
>
> 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.
>
>>> 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).
>> 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, I'll plan to keep it in the bridge chip driver now.
>
>
>>> 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?
>> 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.
> 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.
>
>
>> 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.
> The "next" bridge is the panel, so I don't think this works.
Then drm_panel_get_modes will work then.
>
>
>> - ddc driver on i2c request should power up the panel - seems also correct,
> 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:
>
>> - 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.
> 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.
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:
> 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:
> > On Thu, Apr 1, 2021 at 4:12 AM Andrzej Hajda <[email protected]> wrote:
> >> W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
> >>> On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda <[email protected]> wrote:
> >>>> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> >>>>> 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 <[email protected]>
> >>>>> ---
> >>>>> 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.
> >>>>
> >>>> 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?
> >>>
> >>> 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.
> >>
> >> I guess you mean MIPI DSI
> >
> > Yes, sorry! I'll try to be more clear.
> >
> >> 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...
> >
> > 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).
>
> Not true IMO, even if external device is powered on, you must enable
> EDID-reader logic.
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.
> 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.
>
> > 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.
> >
> >>> 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).
> >>
> >> 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, I'll plan to keep it in the bridge chip driver now.
> >
> >>> 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?
> >>
> >> 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.
> >
> > 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.
> >
> >> 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.
> >
> > The "next" bridge is the panel, so I don't think this works.
>
> Then drm_panel_get_modes will work then.
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.
> >> - ddc driver on i2c request should power up the panel - seems also correct,
> >
> > 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:
> >
> >> - 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.
> >
> > 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.
>
> 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 ???
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.
--
Regards,
Laurent Pinchart
Hi,
On Tue, Apr 6, 2021 at 9:52 AM Andrzej Hajda <[email protected]> wrote:
>
> Hello again after easter,
Sorry, I was out last week and I'm just getting back to this now.
> 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.
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.
> W dniu 01.04.2021 o 16:57, Doug Anderson pisze:
> > Hi,
> >
> > On Thu, Apr 1, 2021 at 4:12 AM Andrzej Hajda <[email protected]> wrote:
> >>
> >> W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
> >>> Hi,
> >>>
> >>> On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda <[email protected]> wrote:
> >>>> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> >>>>> 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 <[email protected]>
> >>>>> ---
> >>>>> 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.
> >>>> 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?
> >>> 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.
> >> I guess you mean MIPI DSI
> > Yes, sorry! I'll try to be more clear.
> >
> >
> >> 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...
> > 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).
>
> 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.
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.
> > 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.
> >
> >>> 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).
> >> 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, I'll plan to keep it in the bridge chip driver now.
> >
> >
> >>> 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?
> >> 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.
> > 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.
> >
> >
> >> 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.
> > The "next" bridge is the panel, so I don't think this works.
>
>
> Then drm_panel_get_modes will work then.
>
> >> - ddc driver on i2c request should power up the panel - seems also correct,
> > 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:
> >
> >> - 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.
> > 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.
>
>
> 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.
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/[email protected]/
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