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.
NOTE: this makes things look a tad bit asymmetric now since we check
for NULL before clk_prepare_enable() but not for
clk_disable_unprepare(). This seemed OK to me. We already have to
check for NULL in the enable case anyway so why not avoid the extra
call?
Signed-off-by: Douglas Anderson <[email protected]>
---
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 f27306c51e4d..942019842ff4 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1261,14 +1261,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.30.1.766.gb4fecdf3b7-goog
In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over
DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
EDID from the panel. That commit kinda worked but it had some serious
problems.
The problems all stem from the fact that userspace wants to be able to
read the EDID before it explicitly enables the panel. For eDP panels,
though, we don't actually power the panel up until the pre-enable
stage and the pre-enable call happens right before the enable call
with no way to interject in-between. For eDP panels, you can't read
the EDID until you power the panel. The result was that
ti_sn_bridge_connector_get_modes() was always failing to read the EDID
(falling back to what drm_panel_get_modes() returned) until _after_
the EDID was needed.
To make it concrete, on my system I saw this happen:
1. We'd attach the bridge.
2. Userspace would ask for the EDID (several times). We'd try but fail
to read the EDID over and over again and fall back to the hardcoded
modes.
3. Userspace would decide on a mode based only on the hardcoded modes.
4. Userspace would ask to turn the panel on.
5. Userspace would (eventually) check the modes again (in Chrome OS
this happens on the handoff from the boot splash screen to the
browser). Now we'd read them properly and, if they were different,
userspace would request to change the mode.
The fact that userspace would always end up using the hardcoded modes
at first significantly decreases the benefit of the EDID
reading. Also: if the modes were even a tiny bit different we'd end up
doing a wasteful modeset and at boot.
As a side note: at least early EDID read failures were relatively
fast. Though the old ti_sn_bridge_connector_get_modes() did call
pm_runtime_get_sync() it didn't program the important "HPD_DISABLE"
bit. That meant that all the AUX transfers failed pretty quickly.
In any case, enough about the problem. How are we fixing it? Obviously
we need to power the panel on _before_ reading the EDID, but how? It
turns out that there's really no problem with just doing all the work
of our pre_enable() function right at attach time and reading the EDID
right away. We'll do that. It's not as easy as it sounds, though,
because:
1. Powering the panel up and down is a pretty expensive operation. Not
only do we need to wait for the HPD line which seems to take up to
200 ms on most panels, but also most panels say that once you power
them off you need to wait at least 500 ms before powering them on
again. We really don't want to incur 700 ms of time here.
2. If we happen not to have a fixed "refclk" we've got a
chicken-and-egg problem. We seem to need the clock setup to read
the EDID. Without a fixed "refclk", though, the bridge runs with
the MIPI pixel clock which means you've got to use a hardcoded mode
for the MIPI pixel clock.
We'll solve problem #1 above by leaving the panel powered on for a
little while after we read the EDID. If enough time passes and nobody
turns the panel on then we'll undo our work. NOTE: there are no
functional problems if someone turns the panel on after a long delay,
it just might take a little longer to turn on.
We'll solve problem #2 by simply _always_ using a hardcoded mode (not
reading the EDID) if a "refclk" wasn't provided. While it might be
possible to fudge something together to support this, it's my belief
that nobody is using this mode in real life since it's really
inflexible. I saw it used for some really early prototype hardware
that was thrown in the e-waste bin years ago when we realized how
inflexible it was. In any case, if someone is using this they're in no
worse shape than they were before the (fairly recent) commit
58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC").
NOTE: while this patch feels a bit hackish, I'm not sure there's much
we can do better without a more fundamental DRM API change. After
looking at it a bunch, it also doesn't feel as hacky to me as I first
thought. The things that pre-enable does are well defined and well
understood and there should be no problems with doing them early nor
with doing them before userspace requests anything.
Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC")
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 98 ++++++++++++++++++++++++---
1 file changed, 88 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 491c9c4f32d1..af3fb4657af6 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -16,6 +16,7 @@
#include <linux/pm_runtime.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
+#include <linux/workqueue.h>
#include <asm/unaligned.h>
@@ -130,6 +131,12 @@
* @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_early: If true we did an early pre_enable at attach.
+ * @pre_enable_timeout_work: Delayed work to undo the pre_enable from attach
+ * if a normal pre_enable never came.
+ * @pre_enable_mutex: Lock to synchronize between the pre_enable_timeout_work
+ * and normal mechanisms.
+ *
* @gchip: If we expose our GPIOs, this is used.
* @gchip_output: A cache of whether we've set GPIOs to output. This
* serves double-duty of keeping track of the direction and
@@ -158,6 +165,10 @@ struct ti_sn_bridge {
u8 ln_assign;
u8 ln_polrs;
+ bool pre_enabled_early;
+ struct delayed_work pre_enable_timeout_work;
+ struct mutex pre_enable_mutex;
+
#if defined(CONFIG_OF_GPIO)
struct gpio_chip gchip;
DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
@@ -272,12 +283,6 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
struct edid *edid = pdata->edid;
int num, ret;
- if (!edid) {
- pm_runtime_get_sync(pdata->dev);
- edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
- pm_runtime_put(pdata->dev);
- }
-
if (edid && drm_edid_is_valid(edid)) {
ret = drm_connector_update_edid_property(connector, edid);
if (!ret) {
@@ -412,10 +417,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
pm_runtime_put_sync(pdata->dev);
}
-static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
+static void __ti_sn_bridge_pre_enable(struct ti_sn_bridge *pdata)
{
- struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
-
pm_runtime_get_sync(pdata->dev);
/* configure bridge ref_clk */
@@ -443,6 +446,38 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
drm_panel_prepare(pdata->panel);
}
+static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
+{
+ struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+ mutex_lock(&pdata->pre_enable_mutex);
+ if (pdata->pre_enabled_early)
+ /* Already done! Just mark that normal pre_enable happened */
+ pdata->pre_enabled_early = false;
+ else
+ __ti_sn_bridge_pre_enable(pdata);
+ mutex_unlock(&pdata->pre_enable_mutex);
+}
+
+static void ti_sn_bridge_cancel_early_pre_enable(struct ti_sn_bridge *pdata)
+{
+ mutex_lock(&pdata->pre_enable_mutex);
+ if (pdata->pre_enabled_early) {
+ pdata->pre_enabled_early = false;
+ ti_sn_bridge_post_disable(&pdata->bridge);
+ }
+ mutex_unlock(&pdata->pre_enable_mutex);
+}
+
+static void ti_sn_bridge_pre_enable_timeout(struct work_struct *work)
+{
+ struct delayed_work *dwork = to_delayed_work(work);
+ struct ti_sn_bridge *pdata = container_of(dwork, struct ti_sn_bridge,
+ pre_enable_timeout_work);
+
+ ti_sn_bridge_cancel_early_pre_enable(pdata);
+}
+
static int ti_sn_bridge_attach(struct drm_bridge *bridge,
enum drm_bridge_attach_flags flags)
{
@@ -516,6 +551,34 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
}
pdata->dsi = dsi;
+ /*
+ * If we have a refclk then we can support dynamic EDID.
+ *
+ * A few notes:
+ * - From trial and error it appears that we need our clock setup in
+ * order to read the EDID. If we don't have refclk then we
+ * (presumably) need the MIPI clock on, but turning that on implies
+ * knowing the pixel clock / not needing the EDID. Maybe we could
+ * futz this if necessary, but for now we won't.
+ * - In order to read the EDID we need power on to the bridge and
+ * the panel (and it has to finish booting up / assert HPD). This
+ * is slow so we leave the panel powered when we're done but setup a
+ * timeout so we don't leave it on forever.
+ * - The rest of Linux assumes that it can read the EDID without
+ * (explicitly) enabling the power which is why this somewhat awkward
+ * step is needed.
+ */
+ if (pdata->refclk) {
+ mutex_lock(&pdata->pre_enable_mutex);
+
+ pdata->pre_enabled_early = true;
+ __ti_sn_bridge_pre_enable(pdata);
+ pdata->edid = drm_get_edid(&pdata->connector, &pdata->aux.ddc);
+ schedule_delayed_work(&pdata->pre_enable_timeout_work, 30 * HZ);
+
+ mutex_unlock(&pdata->pre_enable_mutex);
+ }
+
return 0;
err_dsi_attach:
@@ -525,6 +588,17 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
return ret;
}
+static void ti_sn_bridge_detach(struct drm_bridge *bridge)
+{
+ struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+ cancel_delayed_work_sync(&pdata->pre_enable_timeout_work);
+ ti_sn_bridge_cancel_early_pre_enable(pdata);
+
+ kfree(pdata->edid);
+ pdata->edid = NULL;
+}
+
static void ti_sn_bridge_disable(struct drm_bridge *bridge)
{
struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
@@ -863,6 +937,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
.attach = ti_sn_bridge_attach,
+ .detach = ti_sn_bridge_detach,
.pre_enable = ti_sn_bridge_pre_enable,
.enable = ti_sn_bridge_enable,
.disable = ti_sn_bridge_disable,
@@ -1227,6 +1302,10 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
if (!pdata)
return -ENOMEM;
+ mutex_init(&pdata->pre_enable_mutex);
+ INIT_DELAYED_WORK(&pdata->pre_enable_timeout_work,
+ ti_sn_bridge_pre_enable_timeout);
+
pdata->regmap = devm_regmap_init_i2c(client,
&ti_sn_bridge_regmap_config);
if (IS_ERR(pdata->regmap)) {
@@ -1301,7 +1380,6 @@ 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);
--
2.30.1.766.gb4fecdf3b7-goog
This patch is _only_ code motion to prepare for the patch
("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if
refclk") and make it easier to understand.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 196 +++++++++++++-------------
1 file changed, 98 insertions(+), 98 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 942019842ff4..491c9c4f32d1 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -345,6 +345,104 @@ static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
pdata->supplies);
}
+static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
+{
+ u32 bit_rate_khz, clk_freq_khz;
+ struct drm_display_mode *mode =
+ &pdata->bridge.encoder->crtc->state->adjusted_mode;
+
+ bit_rate_khz = mode->clock *
+ mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
+ clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2);
+
+ return clk_freq_khz;
+}
+
+/* clk frequencies supported by bridge in Hz in case derived from REFCLK pin */
+static const u32 ti_sn_bridge_refclk_lut[] = {
+ 12000000,
+ 19200000,
+ 26000000,
+ 27000000,
+ 38400000,
+};
+
+/* clk frequencies supported by bridge in Hz in case derived from DACP/N pin */
+static const u32 ti_sn_bridge_dsiclk_lut[] = {
+ 468000000,
+ 384000000,
+ 416000000,
+ 486000000,
+ 460800000,
+};
+
+static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata)
+{
+ int i;
+ u32 refclk_rate;
+ const u32 *refclk_lut;
+ size_t refclk_lut_size;
+
+ if (pdata->refclk) {
+ refclk_rate = clk_get_rate(pdata->refclk);
+ refclk_lut = ti_sn_bridge_refclk_lut;
+ refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut);
+ clk_prepare_enable(pdata->refclk);
+ } else {
+ refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000;
+ refclk_lut = ti_sn_bridge_dsiclk_lut;
+ refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut);
+ }
+
+ /* for i equals to refclk_lut_size means default frequency */
+ for (i = 0; i < refclk_lut_size; i++)
+ if (refclk_lut[i] == refclk_rate)
+ break;
+
+ regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
+ REFCLK_FREQ(i));
+}
+
+static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
+{
+ struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+ clk_disable_unprepare(pdata->refclk);
+
+ pm_runtime_put_sync(pdata->dev);
+}
+
+static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
+{
+ struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+ pm_runtime_get_sync(pdata->dev);
+
+ /* configure bridge ref_clk */
+ ti_sn_bridge_set_refclk_freq(pdata);
+
+ /*
+ * HPD on this bridge chip is a bit useless. This is an eDP bridge
+ * so the HPD is an internal signal that's only there to signal that
+ * the panel is done powering up. ...but the bridge chip debounces
+ * this signal by between 100 ms and 400 ms (depending on process,
+ * voltage, and temperate--I measured it at about 200 ms). One
+ * particular panel asserted HPD 84 ms after it was powered on meaning
+ * that we saw HPD 284 ms after power on. ...but the same panel said
+ * that instead of looking at HPD you could just hardcode a delay of
+ * 200 ms. We'll assume that the panel driver will have the hardcoded
+ * delay in its prepare and always disable HPD.
+ *
+ * If HPD somehow makes sense on some future panel we'll have to
+ * change this to be conditional on someone specifying that HPD should
+ * be used.
+ */
+ regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
+ HPD_DISABLE);
+
+ drm_panel_prepare(pdata->panel);
+}
+
static int ti_sn_bridge_attach(struct drm_bridge *bridge,
enum drm_bridge_attach_flags flags)
{
@@ -443,64 +541,6 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge)
drm_panel_unprepare(pdata->panel);
}
-static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
-{
- u32 bit_rate_khz, clk_freq_khz;
- struct drm_display_mode *mode =
- &pdata->bridge.encoder->crtc->state->adjusted_mode;
-
- bit_rate_khz = mode->clock *
- mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
- clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2);
-
- return clk_freq_khz;
-}
-
-/* clk frequencies supported by bridge in Hz in case derived from REFCLK pin */
-static const u32 ti_sn_bridge_refclk_lut[] = {
- 12000000,
- 19200000,
- 26000000,
- 27000000,
- 38400000,
-};
-
-/* clk frequencies supported by bridge in Hz in case derived from DACP/N pin */
-static const u32 ti_sn_bridge_dsiclk_lut[] = {
- 468000000,
- 384000000,
- 416000000,
- 486000000,
- 460800000,
-};
-
-static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata)
-{
- int i;
- u32 refclk_rate;
- const u32 *refclk_lut;
- size_t refclk_lut_size;
-
- if (pdata->refclk) {
- refclk_rate = clk_get_rate(pdata->refclk);
- refclk_lut = ti_sn_bridge_refclk_lut;
- refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut);
- clk_prepare_enable(pdata->refclk);
- } else {
- refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000;
- refclk_lut = ti_sn_bridge_dsiclk_lut;
- refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut);
- }
-
- /* for i equals to refclk_lut_size means default frequency */
- for (i = 0; i < refclk_lut_size; i++)
- if (refclk_lut[i] == refclk_rate)
- break;
-
- regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
- REFCLK_FREQ(i));
-}
-
static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
{
unsigned int bit_rate_mhz, clk_freq_mhz;
@@ -821,46 +861,6 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
drm_panel_enable(pdata->panel);
}
-static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
-{
- struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
-
- pm_runtime_get_sync(pdata->dev);
-
- /* configure bridge ref_clk */
- ti_sn_bridge_set_refclk_freq(pdata);
-
- /*
- * HPD on this bridge chip is a bit useless. This is an eDP bridge
- * so the HPD is an internal signal that's only there to signal that
- * the panel is done powering up. ...but the bridge chip debounces
- * this signal by between 100 ms and 400 ms (depending on process,
- * voltage, and temperate--I measured it at about 200 ms). One
- * particular panel asserted HPD 84 ms after it was powered on meaning
- * that we saw HPD 284 ms after power on. ...but the same panel said
- * that instead of looking at HPD you could just hardcode a delay of
- * 200 ms. We'll assume that the panel driver will have the hardcoded
- * delay in its prepare and always disable HPD.
- *
- * If HPD somehow makes sense on some future panel we'll have to
- * change this to be conditional on someone specifying that HPD should
- * be used.
- */
- regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
- HPD_DISABLE);
-
- drm_panel_prepare(pdata->panel);
-}
-
-static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
-{
- struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
-
- clk_disable_unprepare(pdata->refclk);
-
- pm_runtime_put_sync(pdata->dev);
-}
-
static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
.attach = ti_sn_bridge_attach,
.pre_enable = ti_sn_bridge_pre_enable,
--
2.30.1.766.gb4fecdf3b7-goog
Hey Douglas,
Thanks for submitting this cleanup, it looks good to me.
Reviewed-by: Robert Foss <[email protected]>
On Fri, 5 Mar 2021 at 00:53, Douglas Anderson <[email protected]> wrote:
>
> 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.
>
> NOTE: this makes things look a tad bit asymmetric now since we check
> for NULL before clk_prepare_enable() but not for
> clk_disable_unprepare(). This seemed OK to me. We already have to
> check for NULL in the enable case anyway so why not avoid the extra
> call?
I think this is fine. Since the refclk != NULL check in
ti_sn_bridge_set_refclk_freq is in order to determine other behaviour,
the asymmetry is required.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> 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 f27306c51e4d..942019842ff4 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1261,14 +1261,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.30.1.766.gb4fecdf3b7-goog
>
Hey Douglas,
Thanks for splitting this out into its own patch.
Reviewed-by: Robert Foss <[email protected]>
On Fri, 5 Mar 2021 at 00:53, Douglas Anderson <[email protected]> wrote:
>
> This patch is _only_ code motion to prepare for the patch
> ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if
> refclk") and make it easier to understand.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 196 +++++++++++++-------------
> 1 file changed, 98 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 942019842ff4..491c9c4f32d1 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -345,6 +345,104 @@ static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
> pdata->supplies);
> }
>
> +static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
> +{
> + u32 bit_rate_khz, clk_freq_khz;
> + struct drm_display_mode *mode =
> + &pdata->bridge.encoder->crtc->state->adjusted_mode;
> +
> + bit_rate_khz = mode->clock *
> + mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> + clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2);
> +
> + return clk_freq_khz;
> +}
> +
> +/* clk frequencies supported by bridge in Hz in case derived from REFCLK pin */
> +static const u32 ti_sn_bridge_refclk_lut[] = {
> + 12000000,
> + 19200000,
> + 26000000,
> + 27000000,
> + 38400000,
> +};
> +
> +/* clk frequencies supported by bridge in Hz in case derived from DACP/N pin */
> +static const u32 ti_sn_bridge_dsiclk_lut[] = {
> + 468000000,
> + 384000000,
> + 416000000,
> + 486000000,
> + 460800000,
> +};
> +
> +static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata)
> +{
> + int i;
> + u32 refclk_rate;
> + const u32 *refclk_lut;
> + size_t refclk_lut_size;
> +
> + if (pdata->refclk) {
> + refclk_rate = clk_get_rate(pdata->refclk);
> + refclk_lut = ti_sn_bridge_refclk_lut;
> + refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut);
> + clk_prepare_enable(pdata->refclk);
> + } else {
> + refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000;
> + refclk_lut = ti_sn_bridge_dsiclk_lut;
> + refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut);
> + }
> +
> + /* for i equals to refclk_lut_size means default frequency */
> + for (i = 0; i < refclk_lut_size; i++)
> + if (refclk_lut[i] == refclk_rate)
> + break;
> +
> + regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
> + REFCLK_FREQ(i));
> +}
> +
> +static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> + clk_disable_unprepare(pdata->refclk);
> +
> + pm_runtime_put_sync(pdata->dev);
> +}
> +
> +static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> + pm_runtime_get_sync(pdata->dev);
> +
> + /* configure bridge ref_clk */
> + ti_sn_bridge_set_refclk_freq(pdata);
> +
> + /*
> + * HPD on this bridge chip is a bit useless. This is an eDP bridge
> + * so the HPD is an internal signal that's only there to signal that
> + * the panel is done powering up. ...but the bridge chip debounces
> + * this signal by between 100 ms and 400 ms (depending on process,
> + * voltage, and temperate--I measured it at about 200 ms). One
> + * particular panel asserted HPD 84 ms after it was powered on meaning
> + * that we saw HPD 284 ms after power on. ...but the same panel said
> + * that instead of looking at HPD you could just hardcode a delay of
> + * 200 ms. We'll assume that the panel driver will have the hardcoded
> + * delay in its prepare and always disable HPD.
> + *
> + * If HPD somehow makes sense on some future panel we'll have to
> + * change this to be conditional on someone specifying that HPD should
> + * be used.
> + */
> + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> + HPD_DISABLE);
> +
> + drm_panel_prepare(pdata->panel);
> +}
> +
> static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> enum drm_bridge_attach_flags flags)
> {
> @@ -443,64 +541,6 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> drm_panel_unprepare(pdata->panel);
> }
>
> -static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
> -{
> - u32 bit_rate_khz, clk_freq_khz;
> - struct drm_display_mode *mode =
> - &pdata->bridge.encoder->crtc->state->adjusted_mode;
> -
> - bit_rate_khz = mode->clock *
> - mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> - clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2);
> -
> - return clk_freq_khz;
> -}
> -
> -/* clk frequencies supported by bridge in Hz in case derived from REFCLK pin */
> -static const u32 ti_sn_bridge_refclk_lut[] = {
> - 12000000,
> - 19200000,
> - 26000000,
> - 27000000,
> - 38400000,
> -};
> -
> -/* clk frequencies supported by bridge in Hz in case derived from DACP/N pin */
> -static const u32 ti_sn_bridge_dsiclk_lut[] = {
> - 468000000,
> - 384000000,
> - 416000000,
> - 486000000,
> - 460800000,
> -};
> -
> -static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata)
> -{
> - int i;
> - u32 refclk_rate;
> - const u32 *refclk_lut;
> - size_t refclk_lut_size;
> -
> - if (pdata->refclk) {
> - refclk_rate = clk_get_rate(pdata->refclk);
> - refclk_lut = ti_sn_bridge_refclk_lut;
> - refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut);
> - clk_prepare_enable(pdata->refclk);
> - } else {
> - refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000;
> - refclk_lut = ti_sn_bridge_dsiclk_lut;
> - refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut);
> - }
> -
> - /* for i equals to refclk_lut_size means default frequency */
> - for (i = 0; i < refclk_lut_size; i++)
> - if (refclk_lut[i] == refclk_rate)
> - break;
> -
> - regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
> - REFCLK_FREQ(i));
> -}
> -
> static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
> {
> unsigned int bit_rate_mhz, clk_freq_mhz;
> @@ -821,46 +861,6 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> drm_panel_enable(pdata->panel);
> }
>
> -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> -{
> - struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> -
> - pm_runtime_get_sync(pdata->dev);
> -
> - /* configure bridge ref_clk */
> - ti_sn_bridge_set_refclk_freq(pdata);
> -
> - /*
> - * HPD on this bridge chip is a bit useless. This is an eDP bridge
> - * so the HPD is an internal signal that's only there to signal that
> - * the panel is done powering up. ...but the bridge chip debounces
> - * this signal by between 100 ms and 400 ms (depending on process,
> - * voltage, and temperate--I measured it at about 200 ms). One
> - * particular panel asserted HPD 84 ms after it was powered on meaning
> - * that we saw HPD 284 ms after power on. ...but the same panel said
> - * that instead of looking at HPD you could just hardcode a delay of
> - * 200 ms. We'll assume that the panel driver will have the hardcoded
> - * delay in its prepare and always disable HPD.
> - *
> - * If HPD somehow makes sense on some future panel we'll have to
> - * change this to be conditional on someone specifying that HPD should
> - * be used.
> - */
> - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> - HPD_DISABLE);
> -
> - drm_panel_prepare(pdata->panel);
> -}
> -
> -static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> -{
> - struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> -
> - clk_disable_unprepare(pdata->refclk);
> -
> - pm_runtime_put_sync(pdata->dev);
> -}
> -
> static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .attach = ti_sn_bridge_attach,
> .pre_enable = ti_sn_bridge_pre_enable,
> --
> 2.30.1.766.gb4fecdf3b7-goog
>
On Thu 04 Mar 17:52 CST 2021, Douglas Anderson wrote:
> This patch is _only_ code motion to prepare for the patch
> ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if
> refclk") and make it easier to understand.
>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 196 +++++++++++++-------------
> 1 file changed, 98 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 942019842ff4..491c9c4f32d1 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -345,6 +345,104 @@ static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
> pdata->supplies);
> }
>
> +static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
> +{
> + u32 bit_rate_khz, clk_freq_khz;
> + struct drm_display_mode *mode =
> + &pdata->bridge.encoder->crtc->state->adjusted_mode;
> +
> + bit_rate_khz = mode->clock *
> + mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> + clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2);
> +
> + return clk_freq_khz;
> +}
> +
> +/* clk frequencies supported by bridge in Hz in case derived from REFCLK pin */
> +static const u32 ti_sn_bridge_refclk_lut[] = {
> + 12000000,
> + 19200000,
> + 26000000,
> + 27000000,
> + 38400000,
> +};
> +
> +/* clk frequencies supported by bridge in Hz in case derived from DACP/N pin */
> +static const u32 ti_sn_bridge_dsiclk_lut[] = {
> + 468000000,
> + 384000000,
> + 416000000,
> + 486000000,
> + 460800000,
> +};
> +
> +static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata)
> +{
> + int i;
> + u32 refclk_rate;
> + const u32 *refclk_lut;
> + size_t refclk_lut_size;
> +
> + if (pdata->refclk) {
> + refclk_rate = clk_get_rate(pdata->refclk);
> + refclk_lut = ti_sn_bridge_refclk_lut;
> + refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut);
> + clk_prepare_enable(pdata->refclk);
> + } else {
> + refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000;
> + refclk_lut = ti_sn_bridge_dsiclk_lut;
> + refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut);
> + }
> +
> + /* for i equals to refclk_lut_size means default frequency */
> + for (i = 0; i < refclk_lut_size; i++)
> + if (refclk_lut[i] == refclk_rate)
> + break;
> +
> + regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
> + REFCLK_FREQ(i));
> +}
> +
> +static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> + clk_disable_unprepare(pdata->refclk);
> +
> + pm_runtime_put_sync(pdata->dev);
> +}
> +
> +static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> + pm_runtime_get_sync(pdata->dev);
> +
> + /* configure bridge ref_clk */
> + ti_sn_bridge_set_refclk_freq(pdata);
> +
> + /*
> + * HPD on this bridge chip is a bit useless. This is an eDP bridge
> + * so the HPD is an internal signal that's only there to signal that
> + * the panel is done powering up. ...but the bridge chip debounces
> + * this signal by between 100 ms and 400 ms (depending on process,
> + * voltage, and temperate--I measured it at about 200 ms). One
> + * particular panel asserted HPD 84 ms after it was powered on meaning
> + * that we saw HPD 284 ms after power on. ...but the same panel said
> + * that instead of looking at HPD you could just hardcode a delay of
> + * 200 ms. We'll assume that the panel driver will have the hardcoded
> + * delay in its prepare and always disable HPD.
> + *
> + * If HPD somehow makes sense on some future panel we'll have to
> + * change this to be conditional on someone specifying that HPD should
> + * be used.
> + */
> + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> + HPD_DISABLE);
> +
> + drm_panel_prepare(pdata->panel);
> +}
> +
> static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> enum drm_bridge_attach_flags flags)
> {
> @@ -443,64 +541,6 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> drm_panel_unprepare(pdata->panel);
> }
>
> -static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
> -{
> - u32 bit_rate_khz, clk_freq_khz;
> - struct drm_display_mode *mode =
> - &pdata->bridge.encoder->crtc->state->adjusted_mode;
> -
> - bit_rate_khz = mode->clock *
> - mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> - clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2);
> -
> - return clk_freq_khz;
> -}
> -
> -/* clk frequencies supported by bridge in Hz in case derived from REFCLK pin */
> -static const u32 ti_sn_bridge_refclk_lut[] = {
> - 12000000,
> - 19200000,
> - 26000000,
> - 27000000,
> - 38400000,
> -};
> -
> -/* clk frequencies supported by bridge in Hz in case derived from DACP/N pin */
> -static const u32 ti_sn_bridge_dsiclk_lut[] = {
> - 468000000,
> - 384000000,
> - 416000000,
> - 486000000,
> - 460800000,
> -};
> -
> -static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata)
> -{
> - int i;
> - u32 refclk_rate;
> - const u32 *refclk_lut;
> - size_t refclk_lut_size;
> -
> - if (pdata->refclk) {
> - refclk_rate = clk_get_rate(pdata->refclk);
> - refclk_lut = ti_sn_bridge_refclk_lut;
> - refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut);
> - clk_prepare_enable(pdata->refclk);
> - } else {
> - refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000;
> - refclk_lut = ti_sn_bridge_dsiclk_lut;
> - refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut);
> - }
> -
> - /* for i equals to refclk_lut_size means default frequency */
> - for (i = 0; i < refclk_lut_size; i++)
> - if (refclk_lut[i] == refclk_rate)
> - break;
> -
> - regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
> - REFCLK_FREQ(i));
> -}
> -
> static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
> {
> unsigned int bit_rate_mhz, clk_freq_mhz;
> @@ -821,46 +861,6 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> drm_panel_enable(pdata->panel);
> }
>
> -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> -{
> - struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> -
> - pm_runtime_get_sync(pdata->dev);
> -
> - /* configure bridge ref_clk */
> - ti_sn_bridge_set_refclk_freq(pdata);
> -
> - /*
> - * HPD on this bridge chip is a bit useless. This is an eDP bridge
> - * so the HPD is an internal signal that's only there to signal that
> - * the panel is done powering up. ...but the bridge chip debounces
> - * this signal by between 100 ms and 400 ms (depending on process,
> - * voltage, and temperate--I measured it at about 200 ms). One
> - * particular panel asserted HPD 84 ms after it was powered on meaning
> - * that we saw HPD 284 ms after power on. ...but the same panel said
> - * that instead of looking at HPD you could just hardcode a delay of
> - * 200 ms. We'll assume that the panel driver will have the hardcoded
> - * delay in its prepare and always disable HPD.
> - *
> - * If HPD somehow makes sense on some future panel we'll have to
> - * change this to be conditional on someone specifying that HPD should
> - * be used.
> - */
> - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> - HPD_DISABLE);
> -
> - drm_panel_prepare(pdata->panel);
> -}
> -
> -static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> -{
> - struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> -
> - clk_disable_unprepare(pdata->refclk);
> -
> - pm_runtime_put_sync(pdata->dev);
> -}
> -
> static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .attach = ti_sn_bridge_attach,
> .pre_enable = ti_sn_bridge_pre_enable,
> --
> 2.30.1.766.gb4fecdf3b7-goog
>
On Thu 04 Mar 17:51 CST 2021, Douglas Anderson wrote:
> 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.
>
> NOTE: this makes things look a tad bit asymmetric now since we check
> for NULL before clk_prepare_enable() but not for
> clk_disable_unprepare(). This seemed OK to me. We already have to
> check for NULL in the enable case anyway so why not avoid the extra
> call?
>
Reviewed-by: Bjorn Andersson <[email protected]>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> 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 f27306c51e4d..942019842ff4 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1261,14 +1261,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.30.1.766.gb4fecdf3b7-goog
>
On Thu 04 Mar 17:52 CST 2021, Douglas Anderson wrote:
> In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over
> DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
> EDID from the panel. That commit kinda worked but it had some serious
> problems.
>
> The problems all stem from the fact that userspace wants to be able to
> read the EDID before it explicitly enables the panel. For eDP panels,
> though, we don't actually power the panel up until the pre-enable
> stage and the pre-enable call happens right before the enable call
> with no way to interject in-between. For eDP panels, you can't read
> the EDID until you power the panel. The result was that
> ti_sn_bridge_connector_get_modes() was always failing to read the EDID
> (falling back to what drm_panel_get_modes() returned) until _after_
> the EDID was needed.
>
> To make it concrete, on my system I saw this happen:
> 1. We'd attach the bridge.
> 2. Userspace would ask for the EDID (several times). We'd try but fail
> to read the EDID over and over again and fall back to the hardcoded
> modes.
> 3. Userspace would decide on a mode based only on the hardcoded modes.
> 4. Userspace would ask to turn the panel on.
> 5. Userspace would (eventually) check the modes again (in Chrome OS
> this happens on the handoff from the boot splash screen to the
> browser). Now we'd read them properly and, if they were different,
> userspace would request to change the mode.
>
> The fact that userspace would always end up using the hardcoded modes
> at first significantly decreases the benefit of the EDID
> reading. Also: if the modes were even a tiny bit different we'd end up
> doing a wasteful modeset and at boot.
>
> As a side note: at least early EDID read failures were relatively
> fast. Though the old ti_sn_bridge_connector_get_modes() did call
> pm_runtime_get_sync() it didn't program the important "HPD_DISABLE"
> bit. That meant that all the AUX transfers failed pretty quickly.
>
> In any case, enough about the problem. How are we fixing it? Obviously
> we need to power the panel on _before_ reading the EDID, but how? It
> turns out that there's really no problem with just doing all the work
> of our pre_enable() function right at attach time and reading the EDID
> right away. We'll do that. It's not as easy as it sounds, though,
> because:
>
> 1. Powering the panel up and down is a pretty expensive operation. Not
> only do we need to wait for the HPD line which seems to take up to
> 200 ms on most panels, but also most panels say that once you power
> them off you need to wait at least 500 ms before powering them on
> again. We really don't want to incur 700 ms of time here.
>
> 2. If we happen not to have a fixed "refclk" we've got a
> chicken-and-egg problem. We seem to need the clock setup to read
> the EDID. Without a fixed "refclk", though, the bridge runs with
> the MIPI pixel clock which means you've got to use a hardcoded mode
> for the MIPI pixel clock.
>
> We'll solve problem #1 above by leaving the panel powered on for a
> little while after we read the EDID. If enough time passes and nobody
> turns the panel on then we'll undo our work. NOTE: there are no
> functional problems if someone turns the panel on after a long delay,
> it just might take a little longer to turn on.
>
> We'll solve problem #2 by simply _always_ using a hardcoded mode (not
> reading the EDID) if a "refclk" wasn't provided. While it might be
> possible to fudge something together to support this, it's my belief
> that nobody is using this mode in real life since it's really
> inflexible. I saw it used for some really early prototype hardware
> that was thrown in the e-waste bin years ago when we realized how
> inflexible it was. In any case, if someone is using this they're in no
> worse shape than they were before the (fairly recent) commit
> 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC").
>
> NOTE: while this patch feels a bit hackish, I'm not sure there's much
> we can do better without a more fundamental DRM API change. After
> looking at it a bunch, it also doesn't feel as hacky to me as I first
> thought. The things that pre-enable does are well defined and well
> understood and there should be no problems with doing them early nor
> with doing them before userspace requests anything.
>
> Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC")
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> ---
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 98 ++++++++++++++++++++++++---
> 1 file changed, 88 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 491c9c4f32d1..af3fb4657af6 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -16,6 +16,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/workqueue.h>
>
> #include <asm/unaligned.h>
>
> @@ -130,6 +131,12 @@
> * @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_early: If true we did an early pre_enable at attach.
> + * @pre_enable_timeout_work: Delayed work to undo the pre_enable from attach
> + * if a normal pre_enable never came.
> + * @pre_enable_mutex: Lock to synchronize between the pre_enable_timeout_work
> + * and normal mechanisms.
> + *
> * @gchip: If we expose our GPIOs, this is used.
> * @gchip_output: A cache of whether we've set GPIOs to output. This
> * serves double-duty of keeping track of the direction and
> @@ -158,6 +165,10 @@ struct ti_sn_bridge {
> u8 ln_assign;
> u8 ln_polrs;
>
> + bool pre_enabled_early;
> + struct delayed_work pre_enable_timeout_work;
> + struct mutex pre_enable_mutex;
> +
> #if defined(CONFIG_OF_GPIO)
> struct gpio_chip gchip;
> DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
> @@ -272,12 +283,6 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> struct edid *edid = pdata->edid;
> int num, ret;
>
> - if (!edid) {
> - pm_runtime_get_sync(pdata->dev);
> - edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
> - pm_runtime_put(pdata->dev);
> - }
> -
> if (edid && drm_edid_is_valid(edid)) {
> ret = drm_connector_update_edid_property(connector, edid);
> if (!ret) {
> @@ -412,10 +417,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> pm_runtime_put_sync(pdata->dev);
> }
>
> -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +static void __ti_sn_bridge_pre_enable(struct ti_sn_bridge *pdata)
> {
> - struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> -
> pm_runtime_get_sync(pdata->dev);
>
> /* configure bridge ref_clk */
> @@ -443,6 +446,38 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> drm_panel_prepare(pdata->panel);
> }
>
> +static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> + mutex_lock(&pdata->pre_enable_mutex);
> + if (pdata->pre_enabled_early)
> + /* Already done! Just mark that normal pre_enable happened */
> + pdata->pre_enabled_early = false;
> + else
> + __ti_sn_bridge_pre_enable(pdata);
> + mutex_unlock(&pdata->pre_enable_mutex);
> +}
> +
> +static void ti_sn_bridge_cancel_early_pre_enable(struct ti_sn_bridge *pdata)
> +{
> + mutex_lock(&pdata->pre_enable_mutex);
> + if (pdata->pre_enabled_early) {
> + pdata->pre_enabled_early = false;
> + ti_sn_bridge_post_disable(&pdata->bridge);
> + }
> + mutex_unlock(&pdata->pre_enable_mutex);
> +}
> +
> +static void ti_sn_bridge_pre_enable_timeout(struct work_struct *work)
> +{
> + struct delayed_work *dwork = to_delayed_work(work);
> + struct ti_sn_bridge *pdata = container_of(dwork, struct ti_sn_bridge,
> + pre_enable_timeout_work);
> +
> + ti_sn_bridge_cancel_early_pre_enable(pdata);
> +}
> +
> static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> enum drm_bridge_attach_flags flags)
> {
> @@ -516,6 +551,34 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> }
> pdata->dsi = dsi;
>
> + /*
> + * If we have a refclk then we can support dynamic EDID.
> + *
> + * A few notes:
> + * - From trial and error it appears that we need our clock setup in
> + * order to read the EDID. If we don't have refclk then we
> + * (presumably) need the MIPI clock on, but turning that on implies
> + * knowing the pixel clock / not needing the EDID. Maybe we could
> + * futz this if necessary, but for now we won't.
> + * - In order to read the EDID we need power on to the bridge and
> + * the panel (and it has to finish booting up / assert HPD). This
> + * is slow so we leave the panel powered when we're done but setup a
> + * timeout so we don't leave it on forever.
> + * - The rest of Linux assumes that it can read the EDID without
> + * (explicitly) enabling the power which is why this somewhat awkward
> + * step is needed.
> + */
> + if (pdata->refclk) {
> + mutex_lock(&pdata->pre_enable_mutex);
> +
> + pdata->pre_enabled_early = true;
> + __ti_sn_bridge_pre_enable(pdata);
> + pdata->edid = drm_get_edid(&pdata->connector, &pdata->aux.ddc);
> + schedule_delayed_work(&pdata->pre_enable_timeout_work, 30 * HZ);
> +
> + mutex_unlock(&pdata->pre_enable_mutex);
> + }
> +
> return 0;
>
> err_dsi_attach:
> @@ -525,6 +588,17 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> return ret;
> }
>
> +static void ti_sn_bridge_detach(struct drm_bridge *bridge)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> + cancel_delayed_work_sync(&pdata->pre_enable_timeout_work);
> + ti_sn_bridge_cancel_early_pre_enable(pdata);
> +
> + kfree(pdata->edid);
> + pdata->edid = NULL;
> +}
> +
> static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> {
> struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> @@ -863,6 +937,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>
> static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .attach = ti_sn_bridge_attach,
> + .detach = ti_sn_bridge_detach,
> .pre_enable = ti_sn_bridge_pre_enable,
> .enable = ti_sn_bridge_enable,
> .disable = ti_sn_bridge_disable,
> @@ -1227,6 +1302,10 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> if (!pdata)
> return -ENOMEM;
>
> + mutex_init(&pdata->pre_enable_mutex);
> + INIT_DELAYED_WORK(&pdata->pre_enable_timeout_work,
> + ti_sn_bridge_pre_enable_timeout);
> +
> pdata->regmap = devm_regmap_init_i2c(client,
> &ti_sn_bridge_regmap_config);
> if (IS_ERR(pdata->regmap)) {
> @@ -1301,7 +1380,6 @@ 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);
> --
> 2.30.1.766.gb4fecdf3b7-goog
>
Quoting Douglas Anderson (2021-03-04 15:51:59)
> 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.
>
> NOTE: this makes things look a tad bit asymmetric now since we check
> for NULL before clk_prepare_enable() but not for
> clk_disable_unprepare(). This seemed OK to me. We already have to
> check for NULL in the enable case anyway so why not avoid the extra
> call?
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
Reviewed-by: Stephen Boyd <[email protected]>
Quoting Douglas Anderson (2021-03-04 15:52:00)
> This patch is _only_ code motion to prepare for the patch
> ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if
> refclk") and make it easier to understand.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
Reviewed-by: Stephen Boyd <[email protected]>
Hi Douglas,
Thank you for the patch.
On Thu, Mar 04, 2021 at 03:51:59PM -0800, Douglas Anderson wrote:
> 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.
>
> NOTE: this makes things look a tad bit asymmetric now since we check
> for NULL before clk_prepare_enable() but not for
> clk_disable_unprepare(). This seemed OK to me. We already have to
> check for NULL in the enable case anyway so why not avoid the extra
> call?
>
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
>
> 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 f27306c51e4d..942019842ff4 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1261,14 +1261,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)
--
Regards,
Laurent Pinchart
Hi Douglas,
Thank you for the patch.
On Thu, Mar 04, 2021 at 03:52:00PM -0800, Douglas Anderson wrote:
> This patch is _only_ code motion to prepare for the patch
> ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if
> refclk") and make it easier to understand.
s/make/makes/
>
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 196 +++++++++++++-------------
> 1 file changed, 98 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 942019842ff4..491c9c4f32d1 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -345,6 +345,104 @@ static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
> pdata->supplies);
> }
>
> +static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
> +{
> + u32 bit_rate_khz, clk_freq_khz;
> + struct drm_display_mode *mode =
> + &pdata->bridge.encoder->crtc->state->adjusted_mode;
> +
> + bit_rate_khz = mode->clock *
> + mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> + clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2);
> +
> + return clk_freq_khz;
> +}
> +
> +/* clk frequencies supported by bridge in Hz in case derived from REFCLK pin */
> +static const u32 ti_sn_bridge_refclk_lut[] = {
> + 12000000,
> + 19200000,
> + 26000000,
> + 27000000,
> + 38400000,
> +};
> +
> +/* clk frequencies supported by bridge in Hz in case derived from DACP/N pin */
> +static const u32 ti_sn_bridge_dsiclk_lut[] = {
> + 468000000,
> + 384000000,
> + 416000000,
> + 486000000,
> + 460800000,
> +};
> +
> +static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata)
> +{
> + int i;
> + u32 refclk_rate;
> + const u32 *refclk_lut;
> + size_t refclk_lut_size;
> +
> + if (pdata->refclk) {
> + refclk_rate = clk_get_rate(pdata->refclk);
> + refclk_lut = ti_sn_bridge_refclk_lut;
> + refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut);
> + clk_prepare_enable(pdata->refclk);
> + } else {
> + refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000;
> + refclk_lut = ti_sn_bridge_dsiclk_lut;
> + refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut);
> + }
> +
> + /* for i equals to refclk_lut_size means default frequency */
> + for (i = 0; i < refclk_lut_size; i++)
> + if (refclk_lut[i] == refclk_rate)
> + break;
> +
> + regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
> + REFCLK_FREQ(i));
> +}
> +
> +static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> + clk_disable_unprepare(pdata->refclk);
> +
> + pm_runtime_put_sync(pdata->dev);
> +}
> +
> +static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> + pm_runtime_get_sync(pdata->dev);
> +
> + /* configure bridge ref_clk */
> + ti_sn_bridge_set_refclk_freq(pdata);
> +
> + /*
> + * HPD on this bridge chip is a bit useless. This is an eDP bridge
> + * so the HPD is an internal signal that's only there to signal that
> + * the panel is done powering up. ...but the bridge chip debounces
> + * this signal by between 100 ms and 400 ms (depending on process,
> + * voltage, and temperate--I measured it at about 200 ms). One
> + * particular panel asserted HPD 84 ms after it was powered on meaning
> + * that we saw HPD 284 ms after power on. ...but the same panel said
> + * that instead of looking at HPD you could just hardcode a delay of
> + * 200 ms. We'll assume that the panel driver will have the hardcoded
> + * delay in its prepare and always disable HPD.
> + *
> + * If HPD somehow makes sense on some future panel we'll have to
> + * change this to be conditional on someone specifying that HPD should
> + * be used.
> + */
> + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> + HPD_DISABLE);
> +
> + drm_panel_prepare(pdata->panel);
> +}
> +
> static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> enum drm_bridge_attach_flags flags)
> {
> @@ -443,64 +541,6 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> drm_panel_unprepare(pdata->panel);
> }
>
> -static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
> -{
> - u32 bit_rate_khz, clk_freq_khz;
> - struct drm_display_mode *mode =
> - &pdata->bridge.encoder->crtc->state->adjusted_mode;
> -
> - bit_rate_khz = mode->clock *
> - mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> - clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2);
> -
> - return clk_freq_khz;
> -}
> -
> -/* clk frequencies supported by bridge in Hz in case derived from REFCLK pin */
> -static const u32 ti_sn_bridge_refclk_lut[] = {
> - 12000000,
> - 19200000,
> - 26000000,
> - 27000000,
> - 38400000,
> -};
> -
> -/* clk frequencies supported by bridge in Hz in case derived from DACP/N pin */
> -static const u32 ti_sn_bridge_dsiclk_lut[] = {
> - 468000000,
> - 384000000,
> - 416000000,
> - 486000000,
> - 460800000,
> -};
> -
> -static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata)
> -{
> - int i;
> - u32 refclk_rate;
> - const u32 *refclk_lut;
> - size_t refclk_lut_size;
> -
> - if (pdata->refclk) {
> - refclk_rate = clk_get_rate(pdata->refclk);
> - refclk_lut = ti_sn_bridge_refclk_lut;
> - refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut);
> - clk_prepare_enable(pdata->refclk);
> - } else {
> - refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000;
> - refclk_lut = ti_sn_bridge_dsiclk_lut;
> - refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut);
> - }
> -
> - /* for i equals to refclk_lut_size means default frequency */
> - for (i = 0; i < refclk_lut_size; i++)
> - if (refclk_lut[i] == refclk_rate)
> - break;
> -
> - regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
> - REFCLK_FREQ(i));
> -}
> -
> static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
> {
> unsigned int bit_rate_mhz, clk_freq_mhz;
> @@ -821,46 +861,6 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> drm_panel_enable(pdata->panel);
> }
>
> -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> -{
> - struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> -
> - pm_runtime_get_sync(pdata->dev);
> -
> - /* configure bridge ref_clk */
> - ti_sn_bridge_set_refclk_freq(pdata);
> -
> - /*
> - * HPD on this bridge chip is a bit useless. This is an eDP bridge
> - * so the HPD is an internal signal that's only there to signal that
> - * the panel is done powering up. ...but the bridge chip debounces
> - * this signal by between 100 ms and 400 ms (depending on process,
> - * voltage, and temperate--I measured it at about 200 ms). One
> - * particular panel asserted HPD 84 ms after it was powered on meaning
> - * that we saw HPD 284 ms after power on. ...but the same panel said
> - * that instead of looking at HPD you could just hardcode a delay of
> - * 200 ms. We'll assume that the panel driver will have the hardcoded
> - * delay in its prepare and always disable HPD.
> - *
> - * If HPD somehow makes sense on some future panel we'll have to
> - * change this to be conditional on someone specifying that HPD should
> - * be used.
> - */
> - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> - HPD_DISABLE);
> -
> - drm_panel_prepare(pdata->panel);
> -}
> -
> -static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> -{
> - struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> -
> - clk_disable_unprepare(pdata->refclk);
> -
> - pm_runtime_put_sync(pdata->dev);
> -}
> -
> static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .attach = ti_sn_bridge_attach,
> .pre_enable = ti_sn_bridge_pre_enable,
--
Regards,
Laurent Pinchart
Hi Doug,
Thank you for the patch.
On Thu, Mar 04, 2021 at 03:52:01PM -0800, Douglas Anderson wrote:
> In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over
> DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
> EDID from the panel. That commit kinda worked but it had some serious
> problems.
>
> The problems all stem from the fact that userspace wants to be able to
> read the EDID before it explicitly enables the panel. For eDP panels,
> though, we don't actually power the panel up until the pre-enable
> stage and the pre-enable call happens right before the enable call
> with no way to interject in-between. For eDP panels, you can't read
> the EDID until you power the panel. The result was that
> ti_sn_bridge_connector_get_modes() was always failing to read the EDID
> (falling back to what drm_panel_get_modes() returned) until _after_
> the EDID was needed.
>
> To make it concrete, on my system I saw this happen:
> 1. We'd attach the bridge.
> 2. Userspace would ask for the EDID (several times). We'd try but fail
> to read the EDID over and over again and fall back to the hardcoded
> modes.
> 3. Userspace would decide on a mode based only on the hardcoded modes.
> 4. Userspace would ask to turn the panel on.
> 5. Userspace would (eventually) check the modes again (in Chrome OS
> this happens on the handoff from the boot splash screen to the
> browser). Now we'd read them properly and, if they were different,
> userspace would request to change the mode.
>
> The fact that userspace would always end up using the hardcoded modes
> at first significantly decreases the benefit of the EDID
> reading. Also: if the modes were even a tiny bit different we'd end up
> doing a wasteful modeset and at boot.
s/and at/at/ ?
> As a side note: at least early EDID read failures were relatively
> fast. Though the old ti_sn_bridge_connector_get_modes() did call
> pm_runtime_get_sync() it didn't program the important "HPD_DISABLE"
> bit. That meant that all the AUX transfers failed pretty quickly.
>
> In any case, enough about the problem. How are we fixing it? Obviously
> we need to power the panel on _before_ reading the EDID, but how? It
> turns out that there's really no problem with just doing all the work
> of our pre_enable() function right at attach time and reading the EDID
> right away. We'll do that. It's not as easy as it sounds, though,
> because:
>
> 1. Powering the panel up and down is a pretty expensive operation. Not
> only do we need to wait for the HPD line which seems to take up to
> 200 ms on most panels, but also most panels say that once you power
> them off you need to wait at least 500 ms before powering them on
> again. We really don't want to incur 700 ms of time here.
>
> 2. If we happen not to have a fixed "refclk" we've got a
> chicken-and-egg problem. We seem to need the clock setup to read
> the EDID. Without a fixed "refclk", though, the bridge runs with
> the MIPI pixel clock which means you've got to use a hardcoded mode
> for the MIPI pixel clock.
>
> We'll solve problem #1 above by leaving the panel powered on for a
> little while after we read the EDID. If enough time passes and nobody
> turns the panel on then we'll undo our work. NOTE: there are no
> functional problems if someone turns the panel on after a long delay,
> it just might take a little longer to turn on.
>
> We'll solve problem #2 by simply _always_ using a hardcoded mode (not
> reading the EDID) if a "refclk" wasn't provided. While it might be
> possible to fudge something together to support this, it's my belief
> that nobody is using this mode in real life since it's really
> inflexible. I saw it used for some really early prototype hardware
> that was thrown in the e-waste bin years ago when we realized how
> inflexible it was. In any case, if someone is using this they're in no
> worse shape than they were before the (fairly recent) commit
> 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC").
>
> NOTE: while this patch feels a bit hackish, I'm not sure there's much
> we can do better without a more fundamental DRM API change. After
> looking at it a bunch, it also doesn't feel as hacky to me as I first
> thought. The things that pre-enable does are well defined and well
> understood and there should be no problems with doing them early nor
> with doing them before userspace requests anything.
>
> Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC")
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 98 ++++++++++++++++++++++++---
> 1 file changed, 88 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 491c9c4f32d1..af3fb4657af6 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -16,6 +16,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/workqueue.h>
>
> #include <asm/unaligned.h>
>
> @@ -130,6 +131,12 @@
> * @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_early: If true we did an early pre_enable at attach.
> + * @pre_enable_timeout_work: Delayed work to undo the pre_enable from attach
> + * if a normal pre_enable never came.
Could we simplify this by using the runtime PM autosuspend feature ? The
configuration of the bridge would be moved from pre_enable to the PM
runtime resume handler, the clk_disable_unprepare() call moved from
post_disable to the runtime suspend handler, and the work queue replaced
by usage of pm_runtime_put_autosuspend().
> + * @pre_enable_mutex: Lock to synchronize between the pre_enable_timeout_work
> + * and normal mechanisms.
> + *
> * @gchip: If we expose our GPIOs, this is used.
> * @gchip_output: A cache of whether we've set GPIOs to output. This
> * serves double-duty of keeping track of the direction and
> @@ -158,6 +165,10 @@ struct ti_sn_bridge {
> u8 ln_assign;
> u8 ln_polrs;
>
> + bool pre_enabled_early;
> + struct delayed_work pre_enable_timeout_work;
> + struct mutex pre_enable_mutex;
> +
> #if defined(CONFIG_OF_GPIO)
> struct gpio_chip gchip;
> DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
> @@ -272,12 +283,6 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> struct edid *edid = pdata->edid;
> int num, ret;
>
> - if (!edid) {
> - pm_runtime_get_sync(pdata->dev);
> - edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
> - pm_runtime_put(pdata->dev);
> - }
> -
> if (edid && drm_edid_is_valid(edid)) {
> ret = drm_connector_update_edid_property(connector, edid);
> if (!ret) {
> @@ -412,10 +417,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> pm_runtime_put_sync(pdata->dev);
> }
>
> -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +static void __ti_sn_bridge_pre_enable(struct ti_sn_bridge *pdata)
> {
> - struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> -
> pm_runtime_get_sync(pdata->dev);
>
> /* configure bridge ref_clk */
> @@ -443,6 +446,38 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> drm_panel_prepare(pdata->panel);
> }
>
> +static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> + mutex_lock(&pdata->pre_enable_mutex);
> + if (pdata->pre_enabled_early)
> + /* Already done! Just mark that normal pre_enable happened */
> + pdata->pre_enabled_early = false;
> + else
> + __ti_sn_bridge_pre_enable(pdata);
> + mutex_unlock(&pdata->pre_enable_mutex);
> +}
> +
> +static void ti_sn_bridge_cancel_early_pre_enable(struct ti_sn_bridge *pdata)
> +{
> + mutex_lock(&pdata->pre_enable_mutex);
> + if (pdata->pre_enabled_early) {
> + pdata->pre_enabled_early = false;
> + ti_sn_bridge_post_disable(&pdata->bridge);
> + }
> + mutex_unlock(&pdata->pre_enable_mutex);
> +}
> +
> +static void ti_sn_bridge_pre_enable_timeout(struct work_struct *work)
> +{
> + struct delayed_work *dwork = to_delayed_work(work);
> + struct ti_sn_bridge *pdata = container_of(dwork, struct ti_sn_bridge,
> + pre_enable_timeout_work);
> +
> + ti_sn_bridge_cancel_early_pre_enable(pdata);
> +}
> +
> static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> enum drm_bridge_attach_flags flags)
> {
> @@ -516,6 +551,34 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> }
> pdata->dsi = dsi;
>
> + /*
> + * If we have a refclk then we can support dynamic EDID.
> + *
> + * A few notes:
> + * - From trial and error it appears that we need our clock setup in
> + * order to read the EDID. If we don't have refclk then we
> + * (presumably) need the MIPI clock on, but turning that on implies
> + * knowing the pixel clock / not needing the EDID. Maybe we could
> + * futz this if necessary, but for now we won't.
> + * - In order to read the EDID we need power on to the bridge and
> + * the panel (and it has to finish booting up / assert HPD). This
> + * is slow so we leave the panel powered when we're done but setup a
> + * timeout so we don't leave it on forever.
> + * - The rest of Linux assumes that it can read the EDID without
> + * (explicitly) enabling the power which is why this somewhat awkward
> + * step is needed.
> + */
> + if (pdata->refclk) {
> + mutex_lock(&pdata->pre_enable_mutex);
> +
> + pdata->pre_enabled_early = true;
> + __ti_sn_bridge_pre_enable(pdata);
> + pdata->edid = drm_get_edid(&pdata->connector, &pdata->aux.ddc);
> + schedule_delayed_work(&pdata->pre_enable_timeout_work, 30 * HZ);
> +
> + mutex_unlock(&pdata->pre_enable_mutex);
> + }
> +
> return 0;
>
> err_dsi_attach:
> @@ -525,6 +588,17 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> return ret;
> }
>
> +static void ti_sn_bridge_detach(struct drm_bridge *bridge)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> + cancel_delayed_work_sync(&pdata->pre_enable_timeout_work);
> + ti_sn_bridge_cancel_early_pre_enable(pdata);
> +
> + kfree(pdata->edid);
> + pdata->edid = NULL;
> +}
> +
> static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> {
> struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> @@ -863,6 +937,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>
> static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .attach = ti_sn_bridge_attach,
> + .detach = ti_sn_bridge_detach,
> .pre_enable = ti_sn_bridge_pre_enable,
> .enable = ti_sn_bridge_enable,
> .disable = ti_sn_bridge_disable,
> @@ -1227,6 +1302,10 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> if (!pdata)
> return -ENOMEM;
>
> + mutex_init(&pdata->pre_enable_mutex);
> + INIT_DELAYED_WORK(&pdata->pre_enable_timeout_work,
> + ti_sn_bridge_pre_enable_timeout);
> +
> pdata->regmap = devm_regmap_init_i2c(client,
> &ti_sn_bridge_regmap_config);
> if (IS_ERR(pdata->regmap)) {
> @@ -1301,7 +1380,6 @@ 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);
--
Regards,
Laurent Pinchart
Hi,
On Sat, Mar 13, 2021 at 1:17 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Doug,
>
> Thank you for the patch.
>
> On Thu, Mar 04, 2021 at 03:52:01PM -0800, Douglas Anderson wrote:
> > In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over
> > DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
> > EDID from the panel. That commit kinda worked but it had some serious
> > problems.
> >
> > The problems all stem from the fact that userspace wants to be able to
> > read the EDID before it explicitly enables the panel. For eDP panels,
> > though, we don't actually power the panel up until the pre-enable
> > stage and the pre-enable call happens right before the enable call
> > with no way to interject in-between. For eDP panels, you can't read
> > the EDID until you power the panel. The result was that
> > ti_sn_bridge_connector_get_modes() was always failing to read the EDID
> > (falling back to what drm_panel_get_modes() returned) until _after_
> > the EDID was needed.
> >
> > To make it concrete, on my system I saw this happen:
> > 1. We'd attach the bridge.
> > 2. Userspace would ask for the EDID (several times). We'd try but fail
> > to read the EDID over and over again and fall back to the hardcoded
> > modes.
> > 3. Userspace would decide on a mode based only on the hardcoded modes.
> > 4. Userspace would ask to turn the panel on.
> > 5. Userspace would (eventually) check the modes again (in Chrome OS
> > this happens on the handoff from the boot splash screen to the
> > browser). Now we'd read them properly and, if they were different,
> > userspace would request to change the mode.
> >
> > The fact that userspace would always end up using the hardcoded modes
> > at first significantly decreases the benefit of the EDID
> > reading. Also: if the modes were even a tiny bit different we'd end up
> > doing a wasteful modeset and at boot.
>
> s/and at/at/ ?
Sure, I can correct if/when I respin or it can be corrected when landed.
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 491c9c4f32d1..af3fb4657af6 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -16,6 +16,7 @@
> > #include <linux/pm_runtime.h>
> > #include <linux/regmap.h>
> > #include <linux/regulator/consumer.h>
> > +#include <linux/workqueue.h>
> >
> > #include <asm/unaligned.h>
> >
> > @@ -130,6 +131,12 @@
> > * @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_early: If true we did an early pre_enable at attach.
> > + * @pre_enable_timeout_work: Delayed work to undo the pre_enable from attach
> > + * if a normal pre_enable never came.
>
> Could we simplify this by using the runtime PM autosuspend feature ? The
> configuration of the bridge would be moved from pre_enable to the PM
> runtime resume handler, the clk_disable_unprepare() call moved from
> post_disable to the runtime suspend handler, and the work queue replaced
> by usage of pm_runtime_put_autosuspend().
It's an interesting idea but I don't think I can make it work, at
least not in a generic enough way. Specifically we can also use this
bridge chip as a generic GPIO provider in Linux. When someone asks us
to read a GPIO then we have to power the bridge on
(pm_runtime_get_sync()) and when someone asks us to configure a GPIO
as an output then we actually leave the bridge powered until they stop
requesting it as an output. At the moment the only user of this
functionality (that I know of) is for the HPD pin on trogdor boards
(long story about why we don't use the dedicated HPD) but the API
supports using these GPIOs for anything and I've tested that it works.
It wouldn't be great to have to keep the panel on in order to access
the GPIOs.
The other problem is that I think the time scales are different. At
boot time I think we'd want to leave the panel on for tens of seconds
to give userspace a chance to start up and configure the panel. After
userspace starts up I think we'd want autosuspend to be much faster.
This could probably be solved by tweaking the runtime delay in code
but I didn't fully dig because of the above problem.
-Doug
Hi,
On Sat, Mar 13, 2021 at 1:13 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Douglas,
>
> Thank you for the patch.
>
> On Thu, Mar 04, 2021 at 03:52:00PM -0800, Douglas Anderson wrote:
> > This patch is _only_ code motion to prepare for the patch
> > ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if
> > refclk") and make it easier to understand.
>
> s/make/makes/
I was never an expert at grammar, but I think either "make" or "makes"
are fine. Simple version with parenthesis:
Mine:
This patch is <blah> to (prepare for the patch <blah>) and (make it
easier to understand).
Yours:
This patch is <blah> (to prepare for the patch <blah>) and (makes it
easier to understand).
I suppose also valid would be:
This patch is <blah> (to prepare for the patch <blah>) and (to make it
easier to understand).
In any case if/when I spin this patch I'm fine changing it to your
version just because (as I understand) it's equally valid and maybe
looks slightly better?
> > Signed-off-by: Douglas Anderson <[email protected]>
>
> Reviewed-by: Laurent Pinchart <[email protected]>
Thanks for the reviews!
-Doug
Hi Doug,
On Mon, Mar 15, 2021 at 09:31:41AM -0700, Doug Anderson wrote:
> On Sat, Mar 13, 2021 at 1:13 PM Laurent Pinchart wrote:
> > On Thu, Mar 04, 2021 at 03:52:00PM -0800, Douglas Anderson wrote:
> > > This patch is _only_ code motion to prepare for the patch
> > > ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if
> > > refclk") and make it easier to understand.
> >
> > s/make/makes/
>
> I was never an expert at grammar, but I think either "make" or "makes"
> are fine. Simple version with parenthesis:
>
> Mine:
>
> This patch is <blah> to (prepare for the patch <blah>) and (make it
> easier to understand).
>
> Yours:
>
> This patch is <blah> (to prepare for the patch <blah>) and (makes it
> easier to understand).
>
> I suppose also valid would be:
>
> This patch is <blah> (to prepare for the patch <blah>) and (to make it
> easier to understand).
Your absolutely right. Both versions are fine, and your preferred
version is best :-)
> In any case if/when I spin this patch I'm fine changing it to your
> version just because (as I understand) it's equally valid and maybe
> looks slightly better?
>
> > > Signed-off-by: Douglas Anderson <[email protected]>
> >
> > Reviewed-by: Laurent Pinchart <[email protected]>
>
> Thanks for the reviews!
--
Regards,
Laurent Pinchart
Hi Doug,
On Mon, Mar 15, 2021 at 09:25:37AM -0700, Doug Anderson wrote:
> On Sat, Mar 13, 2021 at 1:17 PM Laurent Pinchart wrote:
> > On Thu, Mar 04, 2021 at 03:52:01PM -0800, Douglas Anderson wrote:
> > > In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over
> > > DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
> > > EDID from the panel. That commit kinda worked but it had some serious
> > > problems.
> > >
> > > The problems all stem from the fact that userspace wants to be able to
> > > read the EDID before it explicitly enables the panel. For eDP panels,
> > > though, we don't actually power the panel up until the pre-enable
> > > stage and the pre-enable call happens right before the enable call
> > > with no way to interject in-between. For eDP panels, you can't read
> > > the EDID until you power the panel. The result was that
> > > ti_sn_bridge_connector_get_modes() was always failing to read the EDID
> > > (falling back to what drm_panel_get_modes() returned) until _after_
> > > the EDID was needed.
> > >
> > > To make it concrete, on my system I saw this happen:
> > > 1. We'd attach the bridge.
> > > 2. Userspace would ask for the EDID (several times). We'd try but fail
> > > to read the EDID over and over again and fall back to the hardcoded
> > > modes.
> > > 3. Userspace would decide on a mode based only on the hardcoded modes.
> > > 4. Userspace would ask to turn the panel on.
> > > 5. Userspace would (eventually) check the modes again (in Chrome OS
> > > this happens on the handoff from the boot splash screen to the
> > > browser). Now we'd read them properly and, if they were different,
> > > userspace would request to change the mode.
> > >
> > > The fact that userspace would always end up using the hardcoded modes
> > > at first significantly decreases the benefit of the EDID
> > > reading. Also: if the modes were even a tiny bit different we'd end up
> > > doing a wasteful modeset and at boot.
> >
> > s/and at/at/ ?
>
> Sure, I can correct if/when I respin or it can be corrected when landed.
>
> > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > index 491c9c4f32d1..af3fb4657af6 100644
> > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > @@ -16,6 +16,7 @@
> > > #include <linux/pm_runtime.h>
> > > #include <linux/regmap.h>
> > > #include <linux/regulator/consumer.h>
> > > +#include <linux/workqueue.h>
> > >
> > > #include <asm/unaligned.h>
> > >
> > > @@ -130,6 +131,12 @@
> > > * @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_early: If true we did an early pre_enable at attach.
> > > + * @pre_enable_timeout_work: Delayed work to undo the pre_enable from attach
> > > + * if a normal pre_enable never came.
> >
> > Could we simplify this by using the runtime PM autosuspend feature ? The
> > configuration of the bridge would be moved from pre_enable to the PM
> > runtime resume handler, the clk_disable_unprepare() call moved from
> > post_disable to the runtime suspend handler, and the work queue replaced
> > by usage of pm_runtime_put_autosuspend().
>
> It's an interesting idea but I don't think I can make it work, at
> least not in a generic enough way. Specifically we can also use this
> bridge chip as a generic GPIO provider in Linux. When someone asks us
> to read a GPIO then we have to power the bridge on
> (pm_runtime_get_sync()) and when someone asks us to configure a GPIO
> as an output then we actually leave the bridge powered until they stop
> requesting it as an output. At the moment the only user of this
> functionality (that I know of) is for the HPD pin on trogdor boards
> (long story about why we don't use the dedicated HPD) but the API
> supports using these GPIOs for anything and I've tested that it works.
> It wouldn't be great to have to keep the panel on in order to access
> the GPIOs.
The issue you're trying to fix doesn't seem specific to this bridge, so
handling it in the bridge driver bothers me :-S Is there any way we
could handle this in the DRM core ? I don't want to see similar
implementations duplicated in all HDMI/DP bridges.
> The other problem is that I think the time scales are different. At
> boot time I think we'd want to leave the panel on for tens of seconds
> to give userspace a chance to start up and configure the panel. After
> userspace starts up I think we'd want autosuspend to be much faster.
> This could probably be solved by tweaking the runtime delay in code
> but I didn't fully dig because of the above problem.
--
Regards,
Laurent Pinchart
Hi,
On Tue, Mar 16, 2021 at 2:46 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Doug,
>
> On Mon, Mar 15, 2021 at 09:25:37AM -0700, Doug Anderson wrote:
> > On Sat, Mar 13, 2021 at 1:17 PM Laurent Pinchart wrote:
> > > On Thu, Mar 04, 2021 at 03:52:01PM -0800, Douglas Anderson wrote:
> > > > In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over
> > > > DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
> > > > EDID from the panel. That commit kinda worked but it had some serious
> > > > problems.
> > > >
> > > > The problems all stem from the fact that userspace wants to be able to
> > > > read the EDID before it explicitly enables the panel. For eDP panels,
> > > > though, we don't actually power the panel up until the pre-enable
> > > > stage and the pre-enable call happens right before the enable call
> > > > with no way to interject in-between. For eDP panels, you can't read
> > > > the EDID until you power the panel. The result was that
> > > > ti_sn_bridge_connector_get_modes() was always failing to read the EDID
> > > > (falling back to what drm_panel_get_modes() returned) until _after_
> > > > the EDID was needed.
> > > >
> > > > To make it concrete, on my system I saw this happen:
> > > > 1. We'd attach the bridge.
> > > > 2. Userspace would ask for the EDID (several times). We'd try but fail
> > > > to read the EDID over and over again and fall back to the hardcoded
> > > > modes.
> > > > 3. Userspace would decide on a mode based only on the hardcoded modes.
> > > > 4. Userspace would ask to turn the panel on.
> > > > 5. Userspace would (eventually) check the modes again (in Chrome OS
> > > > this happens on the handoff from the boot splash screen to the
> > > > browser). Now we'd read them properly and, if they were different,
> > > > userspace would request to change the mode.
> > > >
> > > > The fact that userspace would always end up using the hardcoded modes
> > > > at first significantly decreases the benefit of the EDID
> > > > reading. Also: if the modes were even a tiny bit different we'd end up
> > > > doing a wasteful modeset and at boot.
> > >
> > > s/and at/at/ ?
> >
> > Sure, I can correct if/when I respin or it can be corrected when landed.
> >
> > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > index 491c9c4f32d1..af3fb4657af6 100644
> > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > @@ -16,6 +16,7 @@
> > > > #include <linux/pm_runtime.h>
> > > > #include <linux/regmap.h>
> > > > #include <linux/regulator/consumer.h>
> > > > +#include <linux/workqueue.h>
> > > >
> > > > #include <asm/unaligned.h>
> > > >
> > > > @@ -130,6 +131,12 @@
> > > > * @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_early: If true we did an early pre_enable at attach.
> > > > + * @pre_enable_timeout_work: Delayed work to undo the pre_enable from attach
> > > > + * if a normal pre_enable never came.
> > >
> > > Could we simplify this by using the runtime PM autosuspend feature ? The
> > > configuration of the bridge would be moved from pre_enable to the PM
> > > runtime resume handler, the clk_disable_unprepare() call moved from
> > > post_disable to the runtime suspend handler, and the work queue replaced
> > > by usage of pm_runtime_put_autosuspend().
> >
> > It's an interesting idea but I don't think I can make it work, at
> > least not in a generic enough way. Specifically we can also use this
> > bridge chip as a generic GPIO provider in Linux. When someone asks us
> > to read a GPIO then we have to power the bridge on
> > (pm_runtime_get_sync()) and when someone asks us to configure a GPIO
> > as an output then we actually leave the bridge powered until they stop
> > requesting it as an output. At the moment the only user of this
> > functionality (that I know of) is for the HPD pin on trogdor boards
> > (long story about why we don't use the dedicated HPD) but the API
> > supports using these GPIOs for anything and I've tested that it works.
> > It wouldn't be great to have to keep the panel on in order to access
> > the GPIOs.
>
> The issue you're trying to fix doesn't seem specific to this bridge, so
> handling it in the bridge driver bothers me :-S Is there any way we
> could handle this in the DRM core ? I don't want to see similar
> implementations duplicated in all HDMI/DP bridges.
Yes, it is true that this problem could affect other drivers. ...and
in full disclosure I think there are other similar workarounds already
present. I haven't personally worked on those chips, but in
ps8640_bridge_get_edid() there is a somewhat similar workaround to
chain a pre-enable (though maybe it's not quite as optimized?). I'm
told that maybe something had to be handled for anx7625 (in
anx7625_get_edid()?) but that definitely doesn't look at all like it's
doing a pre-enable, so maybe things for that bridge just work
differently.
One thing that makes me hesitant about trying to moving this to the
core is that even in sn65dsi86 there is a case where it won't work. As
I mentioned in the patch I'm not aware of anyone using it in
production, but if someone was using the MIPI clock as input to the
bridge chip instead of a fixed "refclk" then trying to get the EDID
after just "pre-enable" falls over. Said another way: I can say that
with this particular bridge chip, if you're using a fixed refclk, you
can read the EDID after the pre-enable. I don't know if that's always
true with all other bridge chips.
So I guess in summary: I think I could put my code in the core, but I
don't _think_ I can just make it automatically enabled.
* In sn65dsi I'd have to only enable it if we have a fixed refclk.
* Maybe in ps8640 I could just always enable it and replace the
existing code? I'd have to find someone to test.
* In anx7625 things look totally different.
Can you give me any advice on how you'd like me to proceed?
-Doug
Hi,
On Tue, Mar 16, 2021 at 5:44 PM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Tue, Mar 16, 2021 at 2:46 PM Laurent Pinchart
> <[email protected]> wrote:
> >
> > Hi Doug,
> >
> > On Mon, Mar 15, 2021 at 09:25:37AM -0700, Doug Anderson wrote:
> > > On Sat, Mar 13, 2021 at 1:17 PM Laurent Pinchart wrote:
> > > > On Thu, Mar 04, 2021 at 03:52:01PM -0800, Douglas Anderson wrote:
> > > > > In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over
> > > > > DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
> > > > > EDID from the panel. That commit kinda worked but it had some serious
> > > > > problems.
> > > > >
> > > > > The problems all stem from the fact that userspace wants to be able to
> > > > > read the EDID before it explicitly enables the panel. For eDP panels,
> > > > > though, we don't actually power the panel up until the pre-enable
> > > > > stage and the pre-enable call happens right before the enable call
> > > > > with no way to interject in-between. For eDP panels, you can't read
> > > > > the EDID until you power the panel. The result was that
> > > > > ti_sn_bridge_connector_get_modes() was always failing to read the EDID
> > > > > (falling back to what drm_panel_get_modes() returned) until _after_
> > > > > the EDID was needed.
> > > > >
> > > > > To make it concrete, on my system I saw this happen:
> > > > > 1. We'd attach the bridge.
> > > > > 2. Userspace would ask for the EDID (several times). We'd try but fail
> > > > > to read the EDID over and over again and fall back to the hardcoded
> > > > > modes.
> > > > > 3. Userspace would decide on a mode based only on the hardcoded modes.
> > > > > 4. Userspace would ask to turn the panel on.
> > > > > 5. Userspace would (eventually) check the modes again (in Chrome OS
> > > > > this happens on the handoff from the boot splash screen to the
> > > > > browser). Now we'd read them properly and, if they were different,
> > > > > userspace would request to change the mode.
> > > > >
> > > > > The fact that userspace would always end up using the hardcoded modes
> > > > > at first significantly decreases the benefit of the EDID
> > > > > reading. Also: if the modes were even a tiny bit different we'd end up
> > > > > doing a wasteful modeset and at boot.
> > > >
> > > > s/and at/at/ ?
> > >
> > > Sure, I can correct if/when I respin or it can be corrected when landed.
> > >
> > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > > index 491c9c4f32d1..af3fb4657af6 100644
> > > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > > @@ -16,6 +16,7 @@
> > > > > #include <linux/pm_runtime.h>
> > > > > #include <linux/regmap.h>
> > > > > #include <linux/regulator/consumer.h>
> > > > > +#include <linux/workqueue.h>
> > > > >
> > > > > #include <asm/unaligned.h>
> > > > >
> > > > > @@ -130,6 +131,12 @@
> > > > > * @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_early: If true we did an early pre_enable at attach.
> > > > > + * @pre_enable_timeout_work: Delayed work to undo the pre_enable from attach
> > > > > + * if a normal pre_enable never came.
> > > >
> > > > Could we simplify this by using the runtime PM autosuspend feature ? The
> > > > configuration of the bridge would be moved from pre_enable to the PM
> > > > runtime resume handler, the clk_disable_unprepare() call moved from
> > > > post_disable to the runtime suspend handler, and the work queue replaced
> > > > by usage of pm_runtime_put_autosuspend().
> > >
> > > It's an interesting idea but I don't think I can make it work, at
> > > least not in a generic enough way. Specifically we can also use this
> > > bridge chip as a generic GPIO provider in Linux. When someone asks us
> > > to read a GPIO then we have to power the bridge on
> > > (pm_runtime_get_sync()) and when someone asks us to configure a GPIO
> > > as an output then we actually leave the bridge powered until they stop
> > > requesting it as an output. At the moment the only user of this
> > > functionality (that I know of) is for the HPD pin on trogdor boards
> > > (long story about why we don't use the dedicated HPD) but the API
> > > supports using these GPIOs for anything and I've tested that it works.
> > > It wouldn't be great to have to keep the panel on in order to access
> > > the GPIOs.
> >
> > The issue you're trying to fix doesn't seem specific to this bridge, so
> > handling it in the bridge driver bothers me :-S Is there any way we
> > could handle this in the DRM core ? I don't want to see similar
> > implementations duplicated in all HDMI/DP bridges.
>
> Yes, it is true that this problem could affect other drivers. ...and
> in full disclosure I think there are other similar workarounds already
> present. I haven't personally worked on those chips, but in
> ps8640_bridge_get_edid() there is a somewhat similar workaround to
> chain a pre-enable (though maybe it's not quite as optimized?). I'm
> told that maybe something had to be handled for anx7625 (in
> anx7625_get_edid()?) but that definitely doesn't look at all like it's
> doing a pre-enable, so maybe things for that bridge just work
> differently.
>
> One thing that makes me hesitant about trying to moving this to the
> core is that even in sn65dsi86 there is a case where it won't work. As
> I mentioned in the patch I'm not aware of anyone using it in
> production, but if someone was using the MIPI clock as input to the
> bridge chip instead of a fixed "refclk" then trying to get the EDID
> after just "pre-enable" falls over. Said another way: I can say that
> with this particular bridge chip, if you're using a fixed refclk, you
> can read the EDID after the pre-enable. I don't know if that's always
> true with all other bridge chips.
>
> So I guess in summary: I think I could put my code in the core, but I
> don't _think_ I can just make it automatically enabled.
>
> * In sn65dsi I'd have to only enable it if we have a fixed refclk.
>
> * Maybe in ps8640 I could just always enable it and replace the
> existing code? I'd have to find someone to test.
>
> * In anx7625 things look totally different.
>
> Can you give me any advice on how you'd like me to proceed?
OK, I've got something that maybe looks better. You can tell me what
you think [1]. I did manage to use PM Runtime to avoid some of the
complexity and I put that usage in simple-panel. We'll see if I get
yelled at for adding more to simple-panel. ;-P
[1] https://lore.kernel.org/dri-devel/[email protected]/
Hi Doug,
On Mon, Mar 29, 2021 at 07:57:05PM -0700, Doug Anderson wrote:
> On Tue, Mar 16, 2021 at 5:44 PM Doug Anderson wrote:
> > On Tue, Mar 16, 2021 at 2:46 PM Laurent Pinchart wrote:
> > > On Mon, Mar 15, 2021 at 09:25:37AM -0700, Doug Anderson wrote:
> > > > On Sat, Mar 13, 2021 at 1:17 PM Laurent Pinchart wrote:
> > > > > On Thu, Mar 04, 2021 at 03:52:01PM -0800, Douglas Anderson wrote:
> > > > > > In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over
> > > > > > DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
> > > > > > EDID from the panel. That commit kinda worked but it had some serious
> > > > > > problems.
> > > > > >
> > > > > > The problems all stem from the fact that userspace wants to be able to
> > > > > > read the EDID before it explicitly enables the panel. For eDP panels,
> > > > > > though, we don't actually power the panel up until the pre-enable
> > > > > > stage and the pre-enable call happens right before the enable call
> > > > > > with no way to interject in-between. For eDP panels, you can't read
> > > > > > the EDID until you power the panel. The result was that
> > > > > > ti_sn_bridge_connector_get_modes() was always failing to read the EDID
> > > > > > (falling back to what drm_panel_get_modes() returned) until _after_
> > > > > > the EDID was needed.
> > > > > >
> > > > > > To make it concrete, on my system I saw this happen:
> > > > > > 1. We'd attach the bridge.
> > > > > > 2. Userspace would ask for the EDID (several times). We'd try but fail
> > > > > > to read the EDID over and over again and fall back to the hardcoded
> > > > > > modes.
> > > > > > 3. Userspace would decide on a mode based only on the hardcoded modes.
> > > > > > 4. Userspace would ask to turn the panel on.
> > > > > > 5. Userspace would (eventually) check the modes again (in Chrome OS
> > > > > > this happens on the handoff from the boot splash screen to the
> > > > > > browser). Now we'd read them properly and, if they were different,
> > > > > > userspace would request to change the mode.
> > > > > >
> > > > > > The fact that userspace would always end up using the hardcoded modes
> > > > > > at first significantly decreases the benefit of the EDID
> > > > > > reading. Also: if the modes were even a tiny bit different we'd end up
> > > > > > doing a wasteful modeset and at boot.
> > > > >
> > > > > s/and at/at/ ?
> > > >
> > > > Sure, I can correct if/when I respin or it can be corrected when landed.
> > > >
> > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > > > index 491c9c4f32d1..af3fb4657af6 100644
> > > > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > > > @@ -16,6 +16,7 @@
> > > > > > #include <linux/pm_runtime.h>
> > > > > > #include <linux/regmap.h>
> > > > > > #include <linux/regulator/consumer.h>
> > > > > > +#include <linux/workqueue.h>
> > > > > >
> > > > > > #include <asm/unaligned.h>
> > > > > >
> > > > > > @@ -130,6 +131,12 @@
> > > > > > * @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_early: If true we did an early pre_enable at attach.
> > > > > > + * @pre_enable_timeout_work: Delayed work to undo the pre_enable from attach
> > > > > > + * if a normal pre_enable never came.
> > > > >
> > > > > Could we simplify this by using the runtime PM autosuspend feature ? The
> > > > > configuration of the bridge would be moved from pre_enable to the PM
> > > > > runtime resume handler, the clk_disable_unprepare() call moved from
> > > > > post_disable to the runtime suspend handler, and the work queue replaced
> > > > > by usage of pm_runtime_put_autosuspend().
> > > >
> > > > It's an interesting idea but I don't think I can make it work, at
> > > > least not in a generic enough way. Specifically we can also use this
> > > > bridge chip as a generic GPIO provider in Linux. When someone asks us
> > > > to read a GPIO then we have to power the bridge on
> > > > (pm_runtime_get_sync()) and when someone asks us to configure a GPIO
> > > > as an output then we actually leave the bridge powered until they stop
> > > > requesting it as an output. At the moment the only user of this
> > > > functionality (that I know of) is for the HPD pin on trogdor boards
> > > > (long story about why we don't use the dedicated HPD) but the API
> > > > supports using these GPIOs for anything and I've tested that it works.
> > > > It wouldn't be great to have to keep the panel on in order to access
> > > > the GPIOs.
> > >
> > > The issue you're trying to fix doesn't seem specific to this bridge, so
> > > handling it in the bridge driver bothers me :-S Is there any way we
> > > could handle this in the DRM core ? I don't want to see similar
> > > implementations duplicated in all HDMI/DP bridges.
> >
> > Yes, it is true that this problem could affect other drivers. ...and
> > in full disclosure I think there are other similar workarounds already
> > present. I haven't personally worked on those chips, but in
> > ps8640_bridge_get_edid() there is a somewhat similar workaround to
> > chain a pre-enable (though maybe it's not quite as optimized?). I'm
> > told that maybe something had to be handled for anx7625 (in
> > anx7625_get_edid()?) but that definitely doesn't look at all like it's
> > doing a pre-enable, so maybe things for that bridge just work
> > differently.
> >
> > One thing that makes me hesitant about trying to moving this to the
> > core is that even in sn65dsi86 there is a case where it won't work. As
> > I mentioned in the patch I'm not aware of anyone using it in
> > production, but if someone was using the MIPI clock as input to the
> > bridge chip instead of a fixed "refclk" then trying to get the EDID
> > after just "pre-enable" falls over. Said another way: I can say that
> > with this particular bridge chip, if you're using a fixed refclk, you
> > can read the EDID after the pre-enable. I don't know if that's always
> > true with all other bridge chips.
> >
> > So I guess in summary: I think I could put my code in the core, but I
> > don't _think_ I can just make it automatically enabled.
> >
> > * In sn65dsi I'd have to only enable it if we have a fixed refclk.
> >
> > * Maybe in ps8640 I could just always enable it and replace the
> > existing code? I'd have to find someone to test.
> >
> > * In anx7625 things look totally different.
> >
> > Can you give me any advice on how you'd like me to proceed?
>
> OK, I've got something that maybe looks better. You can tell me what
> you think [1]. I did manage to use PM Runtime to avoid some of the
> complexity and I put that usage in simple-panel. We'll see if I get
> yelled at for adding more to simple-panel. ;-P
>
> [1] https://lore.kernel.org/dri-devel/[email protected]/
Nice :-)
I'm unfortunately afraid I'm quite busy these days. Could you ping me in
a few weeks if I haven't reviewed the series ?
--
Regards,
Laurent Pinchart