The primary goal of this series is to try to properly fix EDID reading
for eDP panels using the ti-sn65dsi86 bridge.
Previously we had a patch that added EDID reading but it turned out
not to work at bootup. This caused some extra churn at bootup as we
tried (and failed) to read the EDID several times and also ended up
forcing us to use the hardcoded mode at boot. With this patch series I
believe EDID reading is reliable at boot now and we never use the
hardcoded mode.
This series is the logical successor to the 3-part series containing
the patch ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only
if refclk") [1] though only one actual patch is the same between the
two.
This series starts out with some general / obvious fixes and moves on
to some more specific and maybe controversial ones. I wouldn't object
to some of the earlier ones landing if they look ready.
This patch was developed agains linuxnext (next-20210416) with
drm-misc-next (as of 20210423) merged in on a sc7180-trogdor-lazor
device. To get things booting for me, I had to use Stephen's patch [2]
to keep from crashing but otherwise all the patches I needed were
here.
Primary change between v2 and v3 is to stop doing the EDID caching in
the core. I also added Andrzej's review tags.
Between v3 and v4 this series grew a whole lot. I changed it so that
the EDID reading is actually driven by the panel driver now as was
suggested by Andrzej. While I still believe that the old approach
wasn't too bad I'm still switching. Why?
The main reason is that I think it's useful in general for the panel
code to have access to the DDC bus and to be able to read the
EDID. This may allow us to more easily have the panel code support
multiple sources of panels--it can read the EDID and possibly adjust
timings based on the model ID. It also allows the panel code (or
perhaps backlight code?) to send DDC commands if they are need for a
particular panel.
At the moment, once the panel is provided the DDC bus then existing
code will assume that it should be in charge of reading the
EDID. While it doesn't have to work that way, it seems sane to build
on what's already there.
In order to expose the DDC bus to the panel, I had to solve a bunch of
chicken-and-egg problems in terms of probe ordering between the bridge
and the panel. I've broken the bridge driver into several sub drivers
to make this happen. At the moment the sub-drivers are just there to
solve the probe problem, but conceivably someone could use them to
break the driver up in the future if need be.
Between v4 and v5, high-level view of changes.
- Some of the early patches landed, so dropped from series.
- New pm_runtime_disable() fix (fixed a patch that already landed).
- Added Bjorn's tags to most patches
- Fixed problems when building as a module.
- Reordered debugfs patch and fixed error handling there.
- Dropped last patch. I'm not convinced it's safe w/out more work.
I apologize in advance for the length of this series. Hopefully you
can see that there are only so many because they're broken up into
nice and reviewable bite-sized-chunks. :-)
*** NOTE ***: my hope is to actually land patches that have been
reviewed sometime mid-to-late next week. Please shout if you think
this is too much of a rush and you know of someone who is planning to
review that needs more time.
[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
[2] https://lore.kernel.org/r/161706912161.3012082.17313817257247946143@swboyd.mtv.corp.google.com/
Changes in v5:
- Missing pm_runtime_disable() patch new for v5.
- Reordered to debugfs change to avoid transient issue
- Don't print debugfs creation errors.
- Handle NULL from debugfs_create_dir() which is documented possible.
- Rebased atop the pm_runtime patch, which got reordered.
- Fix module compile problems (Bjorn + kbuild bot)
- Remove useless MODULE_DEVICE_TABLE (Bjorn).
- Fix module compile problems (Bjorn + kbuild bot)
- Remove useless MODULE_DEVICE_TABLE (Bjorn).
Douglas Anderson (20):
drm/panel: panel-simple: Add missing pm_runtime_disable() calls
drm/bridge: ti-sn65dsi86: Rename the main driver data structure
drm/bridge: ti-sn65dsi86: More renames in prep for sub-devices
drm/bridge: ti-sn65dsi86: Use devm to do our runtime_disable
drm/bridge: ti-sn65dsi86: Clean debugfs code
drm/bridge: ti-sn65dsi86: Add local var for "dev" to simplify probe
drm/bridge: ti-sn65dsi86: Cleanup managing of drvdata
drm/bridge: ti-sn65dsi86: Move all the chip-related init to the start
drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into
sub-drivers
drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code
drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend
drm/bridge: ti-sn65dsi86: Code motion of refclk management functions
drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen w/out
pre-enable
drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev
i2c: i2c-core-of: Fix corner case of finding adapter by node
drm/panel: panel-simple: Remove extra call:
drm_connector_update_edid_property()
drm/panel: panel-simple: Power the panel when reading the EDID
drm/panel: panel-simple: Cache the EDID as long as we retain power
drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC
arm64: dts: qcom: Link the panel to the bridge's DDC bus
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 +
drivers/gpu/drm/bridge/Kconfig | 1 +
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 767 ++++++++++++-------
drivers/gpu/drm/panel/panel-simple.c | 49 +-
drivers/i2c/i2c-core-of.c | 17 +-
5 files changed, 538 insertions(+), 297 deletions(-)
--
2.31.1.498.g6c1eba8ee3d-goog
No functional changes--this just makes the diffstat of a future change
easier to understand.
Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 116 +++++++++++++-------------
1 file changed, 58 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 49b76b2ffe25..db367793cdff 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -192,6 +192,64 @@ static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
regmap_write(pdata->regmap, reg + 1, val >> 8);
}
+static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *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_sn65dsi86 *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 int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
{
struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
@@ -460,64 +518,6 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge)
regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
}
-static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *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_sn65dsi86 *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_sn65dsi86 *pdata)
{
unsigned int bit_rate_mhz, clk_freq_mhz;
--
2.31.1.498.g6c1eba8ee3d-goog
Let's make the bridge use autosuspend with a 500ms delay. This is in
preparation for promoting DP AUX transfers to their own sub-driver so
that we're not constantly powering up and down the device as we
transfer all the chunks.
Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 0bd1a1d1453e..49b76b2ffe25 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -243,7 +243,7 @@ static int status_show(struct seq_file *s, void *data)
seq_printf(s, "[0x%02x] = 0x%08x\n", reg, val);
}
- pm_runtime_put(pdata->dev);
+ pm_runtime_put_autosuspend(pdata->dev);
return 0;
}
@@ -293,7 +293,7 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
if (!edid) {
pm_runtime_get_sync(pdata->dev);
edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
- pm_runtime_put(pdata->dev);
+ pm_runtime_put_autosuspend(pdata->dev);
}
if (edid && drm_edid_is_valid(edid)) {
@@ -419,7 +419,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
/* check if continuous dsi clock is required or not */
pm_runtime_get_sync(pdata->dev);
regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val);
- pm_runtime_put(pdata->dev);
+ pm_runtime_put_autosuspend(pdata->dev);
if (!(val & DPPLL_CLK_SRC_DSICLK))
dsi->mode_flags |= MIPI_DSI_CLOCK_NON_CONTINUOUS;
@@ -1050,7 +1050,7 @@ static int ti_sn_bridge_gpio_get(struct gpio_chip *chip, unsigned int offset)
*/
pm_runtime_get_sync(pdata->dev);
ret = regmap_read(pdata->regmap, SN_GPIO_IO_REG, &val);
- pm_runtime_put(pdata->dev);
+ pm_runtime_put_autosuspend(pdata->dev);
if (ret)
return ret;
@@ -1101,7 +1101,7 @@ static int ti_sn_bridge_gpio_direction_input(struct gpio_chip *chip,
* it off and when it comes back it will have lost all state, but
* that's OK because the default is input and we're now an input.
*/
- pm_runtime_put(pdata->dev);
+ pm_runtime_put_autosuspend(pdata->dev);
return 0;
}
@@ -1127,7 +1127,7 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip,
SN_GPIO_MUX_OUTPUT << shift);
if (ret) {
clear_bit(offset, pdata->gchip_output);
- pm_runtime_put(pdata->dev);
+ pm_runtime_put_autosuspend(pdata->dev);
}
return ret;
@@ -1418,6 +1418,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev);
if (ret)
return ret;
+ pm_runtime_set_autosuspend_delay(pdata->dev, 500);
+ pm_runtime_use_autosuspend(pdata->dev);
ti_sn65dsi86_debugfs_init(pdata);
--
2.31.1.498.g6c1eba8ee3d-goog
Let's reorganize how we init and turn on the reference clock in the
code to allow us to turn it on early (even before pre_enable()) so
that we can read the EDID early. This is handy for eDP because:
- We always assume that a panel is there.
- Once we report that a panel is there we get asked to read the EDID.
- Pre-enable isn't called until we know what pixel clock we want to
use and we're ready to turn everything on. That's _after_ we get
asked to read the EDID.
NOTE: the above only works out OK if we "refclk" is provided. Though I
don't have access to any hardware that uses ti-sn65dsi86 and _doesn't_
provide a "refclk", I believe that we'll have trouble reading the EDID
at bootup in that case. Specifically I believe that if there's no
"refclk" we need the MIPI source clock to be active before we can
successfully read the EDID. My evidence here is that, in testing, I
couldn't read the EDID until I turned on the DPPLL in the bridge chip
and that the DPPLL needs the input clock to be active.
Since this is hard to support, let's punt trying to handle this case
if there's no "refclk". In that case we'll enable comms in
pre_enable() like we always did.
I don't believe there are any users of the ti-sn65dsi86 bridge chip
that _don't_ use "refclk". The bridge chip is _very_ inflexible in
that mode. The only time I've seen that mode used was for some really
early prototype hardware that was thrown in the e-waste bin years ago
when we realized how inflexible it was.
Even if someone is using the bridge chip without the "refclk" they're
in no worse shape than they were before the (fairly recent) commit
58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC").
Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
(no changes since v1)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 129 +++++++++++++++++++-------
1 file changed, 94 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index db367793cdff..9dc3cd8e17df 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -132,6 +132,8 @@
* @dp_lanes: Count of dp_lanes we're using.
* @ln_assign: Value to program to the LN_ASSIGN register.
* @ln_polrs: Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
+ * @comms_enabled: If true then communication over the aux channel is enabled.
+ * @comms_mutex: Protects modification of comms_enabled.
*
* @gchip: If we expose our GPIOs, this is used.
* @gchip_output: A cache of whether we've set GPIOs to output. This
@@ -162,6 +164,8 @@ struct ti_sn65dsi86 {
int dp_lanes;
u8 ln_assign;
u8 ln_polrs;
+ bool comms_enabled;
+ struct mutex comms_mutex;
#if defined(CONFIG_OF_GPIO)
struct gpio_chip gchip;
@@ -250,6 +254,47 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
REFCLK_FREQ(i));
}
+static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
+{
+ mutex_lock(&pdata->comms_mutex);
+
+ /* 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);
+
+ pdata->comms_enabled = true;
+
+ mutex_unlock(&pdata->comms_mutex);
+}
+
+static void ti_sn65dsi86_disable_comms(struct ti_sn65dsi86 *pdata)
+{
+ mutex_lock(&pdata->comms_mutex);
+
+ pdata->comms_enabled = false;
+ clk_disable_unprepare(pdata->refclk);
+
+ mutex_unlock(&pdata->comms_mutex);
+}
+
static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
{
struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
@@ -263,6 +308,16 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
gpiod_set_value(pdata->enable_gpio, 1);
+ /*
+ * If we have a reference clock we can enable communication w/ the
+ * panel (including the aux channel) w/out any need for an input clock
+ * so we can do it in resume which lets us read the EDID before
+ * pre_enable(). Without a reference clock we need the MIPI reference
+ * clock so reading early doesn't work.
+ */
+ if (pdata->refclk)
+ ti_sn65dsi86_enable_comms(pdata);
+
return ret;
}
@@ -271,6 +326,9 @@ static int __maybe_unused ti_sn65dsi86_suspend(struct device *dev)
struct ti_sn65dsi86 *pdata = dev_get_drvdata(dev);
int ret;
+ if (pdata->refclk)
+ ti_sn65dsi86_disable_comms(pdata);
+
gpiod_set_value(pdata->enable_gpio, 0);
ret = regulator_bulk_disable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
@@ -844,27 +902,8 @@ static void ti_sn_bridge_pre_enable(struct drm_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);
+ if (!pdata->refclk)
+ ti_sn65dsi86_enable_comms(pdata);
drm_panel_prepare(pdata->panel);
}
@@ -875,7 +914,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
drm_panel_unprepare(pdata->panel);
- clk_disable_unprepare(pdata->refclk);
+ if (!pdata->refclk)
+ ti_sn65dsi86_disable_comms(pdata);
pm_runtime_put_sync(pdata->dev);
}
@@ -909,6 +949,20 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
if (len > SN_AUX_MAX_PAYLOAD_BYTES)
return -EINVAL;
+ pm_runtime_get_sync(pdata->dev);
+ mutex_lock(&pdata->comms_mutex);
+
+ /*
+ * If someone tries to do a DDC over AUX transaction before pre_enable()
+ * on a device without a dedicated reference clock then we just can't
+ * do it. Fail right away. This prevents non-refclk users from reading
+ * the EDID before enabling the panel but such is life.
+ */
+ if (!pdata->comms_enabled) {
+ ret = -EIO;
+ goto exit;
+ }
+
switch (request) {
case DP_AUX_NATIVE_WRITE:
case DP_AUX_I2C_WRITE:
@@ -919,7 +973,8 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
msg->reply = 0;
break;
default:
- return -EINVAL;
+ ret = -EINVAL;
+ goto exit;
}
BUILD_BUG_ON(sizeof(addr_len) != sizeof(__be32));
@@ -943,11 +998,11 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
ret = regmap_read_poll_timeout(pdata->regmap, SN_AUX_CMD_REG, val,
!(val & AUX_CMD_SEND), 0, 50 * 1000);
if (ret)
- return ret;
+ goto exit;
ret = regmap_read(pdata->regmap, SN_AUX_CMD_STATUS_REG, &val);
if (ret)
- return ret;
+ goto exit;
if (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT) {
/*
@@ -955,13 +1010,14 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
* but it hit a timeout. We ignore defers here because they're
* handled in hardware.
*/
- return -ETIMEDOUT;
+ ret = -ETIMEDOUT;
+ goto exit;
}
if (val & AUX_IRQ_STATUS_AUX_SHORT) {
ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
if (ret)
- return ret;
+ goto exit;
} else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
switch (request) {
case DP_AUX_I2C_WRITE:
@@ -973,18 +1029,19 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
break;
}
- return 0;
+ len = 0;
+ goto exit;
}
- if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE ||
- len == 0)
- return len;
+ if (request != DP_AUX_NATIVE_WRITE && request != DP_AUX_I2C_WRITE && len != 0)
+ ret = regmap_bulk_read(pdata->regmap, SN_AUX_RDATA_REG(0), buf, len);
- ret = regmap_bulk_read(pdata->regmap, SN_AUX_RDATA_REG(0), buf, len);
- if (ret)
- return ret;
+exit:
+ mutex_unlock(&pdata->comms_mutex);
+ pm_runtime_mark_last_busy(pdata->dev);
+ pm_runtime_put_autosuspend(pdata->dev);
- return len;
+ return ret ? ret : len;
}
static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
@@ -1390,6 +1447,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
dev_set_drvdata(dev, pdata);
pdata->dev = dev;
+ mutex_init(&pdata->comms_mutex);
+
pdata->regmap = devm_regmap_init_i2c(client,
&ti_sn65dsi86_regmap_config);
if (IS_ERR(pdata->regmap)) {
--
2.31.1.498.g6c1eba8ee3d-goog
The of_find_i2c_adapter_by_node() could end up failing to find an
adapter in certain conditions. Specifically it's possible that
of_dev_or_parent_node_match() could end up finding an I2C client in
the list and cause bus_find_device() to stop early even though an I2C
adapter was present later in the list.
Let's move the i2c_verify_adapter() into the predicate function to
prevent this. Now we'll properly skip over the I2C client and be able
to find the I2C adapter.
This issue has always been a potential problem if a single device tree
node could represent both an I2C client and an adapter. I believe this
is a sane thing to do if, for instance, an I2C-connected DP bridge
chip is present. The bridge chip is an I2C client but it can also
provide an I2C adapter (DDC tunneled over AUX channel). We don't want
to have to create a sub-node just so a panel can link to it with the
"ddc-i2c-bus" property.
I believe that this problem got worse, however, with commit
e814e688413a ("i2c: of: Try to find an I2C adapter matching the
parent"). Starting at that commit it would be even easier to
accidentally miss finding the adapter.
Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
This commit is sorta just jammed into the middle of my series. It has
no dependencies on the earlier patches in the series and I think it
can land independently in the i2c tree. Later patches in the series
won't work right without this one, but they won't crash. If we can't
find the i2c bus we'll just fall back to the hardcoded panel modes
which, at least today, all panels have.
I'll also note that part of me wonders if we should actually fix this
further to run two passes through everything: first look to see if we
find an exact match and only look at the parent pointer if there is no
match. I don't currently have a need for that and it's a slightly
bigger change, but it seems conceivable that it could affect someone?
(no changes since v1)
drivers/i2c/i2c-core-of.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index 3ed74aa4b44b..de0bf5fce3a2 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -124,6 +124,14 @@ static int of_dev_or_parent_node_match(struct device *dev, const void *data)
return 0;
}
+static int of_i2c_adapter_match(struct device *dev, const void *data)
+{
+ if (!of_dev_or_parent_node_match(dev, data))
+ return 0;
+
+ return !!i2c_verify_adapter(dev);
+}
+
/* must call put_device() when done with returned i2c_client device */
struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
{
@@ -146,18 +154,13 @@ EXPORT_SYMBOL(of_find_i2c_device_by_node);
struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
{
struct device *dev;
- struct i2c_adapter *adapter;
dev = bus_find_device(&i2c_bus_type, NULL, node,
- of_dev_or_parent_node_match);
+ of_i2c_adapter_match);
if (!dev)
return NULL;
- adapter = i2c_verify_adapter(dev);
- if (!adapter)
- put_device(dev);
-
- return adapter;
+ return to_i2c_adapter(dev);
}
EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
--
2.31.1.498.g6c1eba8ee3d-goog
I don't believe that it ever makes sense to read the EDID when a panel
is not powered and the powering on of the panel is the job of
prepare(). Let's make sure that this happens before we try to read the
EDID. We use the pm_runtime functions directly rather than directly
calling the normal prepare() function because the pm_runtime functions
are definitely refcounted whereas it's less clear if the prepare() one
is.
NOTE: I'm not 100% sure how EDID reading was working for folks in the
past, but I can only assume that it was failing on the initial attempt
and then working only later. This patch, presumably, will fix that. If
some panel out there really can read the EDID without powering up and
it's a big advantage to preserve the old behavior we can add a
per-panel flag. It appears that providing the DDC bus to the panel in
the past was somewhat uncommon in any case.
Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 4de33c929a59..a12dfe8b8d90 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -510,12 +510,18 @@ static int panel_simple_get_modes(struct drm_panel *panel,
/* probe EDID if a DDC bus is available */
if (p->ddc) {
- struct edid *edid = drm_get_edid(connector, p->ddc);
+ struct edid *edid;
+ pm_runtime_get_sync(panel->dev);
+
+ edid = drm_get_edid(connector, p->ddc);
if (edid) {
num += drm_add_edid_modes(connector, edid);
kfree(edid);
}
+
+ pm_runtime_mark_last_busy(panel->dev);
+ pm_runtime_put_autosuspend(panel->dev);
}
/* add hard-coded panel modes */
--
2.31.1.498.g6c1eba8ee3d-goog
It doesn't make sense to go out to the bus and read the EDID over and
over again. Let's cache it and throw away the cache when we turn power
off from the panel. Autosuspend means that even if there are several
calls to read the EDID before we officially turn the power on then we
should get good use out of this cache.
Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index a12dfe8b8d90..9be050ab372f 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -189,6 +189,8 @@ struct panel_simple {
struct gpio_desc *enable_gpio;
struct gpio_desc *hpd_gpio;
+ struct edid *edid;
+
struct drm_display_mode override_mode;
enum drm_panel_orientation orientation;
@@ -345,6 +347,9 @@ static int panel_simple_suspend(struct device *dev)
regulator_disable(p->supply);
p->unprepared_time = ktime_get();
+ kfree(p->edid);
+ p->edid = NULL;
+
return 0;
}
@@ -510,15 +515,13 @@ static int panel_simple_get_modes(struct drm_panel *panel,
/* probe EDID if a DDC bus is available */
if (p->ddc) {
- struct edid *edid;
-
pm_runtime_get_sync(panel->dev);
- edid = drm_get_edid(connector, p->ddc);
- if (edid) {
- num += drm_add_edid_modes(connector, edid);
- kfree(edid);
- }
+ if (!p->edid)
+ p->edid = drm_get_edid(connector, p->ddc);
+
+ if (p->edid)
+ num += drm_add_edid_modes(connector, p->edid);
pm_runtime_mark_last_busy(panel->dev);
pm_runtime_put_autosuspend(panel->dev);
--
2.31.1.498.g6c1eba8ee3d-goog
Adding this link allows the panel code to do things like read the
EDID.
Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
(no changes since v1)
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 24d293ef56d7..96e530594509 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -265,6 +265,7 @@ panel: panel {
power-supply = <&pp3300_dx_edp>;
backlight = <&backlight>;
hpd-gpios = <&sn65dsi86_bridge 2 GPIO_ACTIVE_HIGH>;
+ ddc-i2c-bus = <&sn65dsi86_bridge>;
ports {
port {
--
2.31.1.498.g6c1eba8ee3d-goog
As of commit 5186421cbfe2 ("drm: Introduce epoch counter to
drm_connector") the drm_get_edid() function calls
drm_connector_update_edid_property() for us. There's no reason for us
to call it again.
Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
As Laurent pointed out [1] this is actually a pretty common
problem. His suggestion to do this more broadly is a good idea but
this series is probably a bit ambitious already so I would suggest
that be taken up separately.
[1] https://lore.kernel.org/r/[email protected]
(no changes since v1)
drivers/gpu/drm/panel/panel-simple.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index bd208abcbf07..4de33c929a59 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -512,7 +512,6 @@ static int panel_simple_get_modes(struct drm_panel *panel,
if (p->ddc) {
struct edid *edid = drm_get_edid(connector, p->ddc);
- drm_connector_update_edid_property(connector, edid);
if (edid) {
num += drm_add_edid_modes(connector, edid);
kfree(edid);
--
2.31.1.498.g6c1eba8ee3d-goog
On Fri, Apr 23, 2021 at 1:00 PM Douglas Anderson <[email protected]> wrote:
>
> It doesn't make sense to go out to the bus and read the EDID over and
> over again. Let's cache it and throw away the cache when we turn power
> off from the panel. Autosuspend means that even if there are several
> calls to read the EDID before we officially turn the power on then we
> should get good use out of this cache.
>
I think i915 caches the edid once on init and never refreshes it
(assuming no hotplugs). That said, I think it makes sense for a more
conservative approach in panel-simple.
> Signed-off-by: Douglas Anderson <[email protected]>
> Reviewed-by: Bjorn Andersson <[email protected]>
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/panel/panel-simple.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index a12dfe8b8d90..9be050ab372f 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -189,6 +189,8 @@ struct panel_simple {
> struct gpio_desc *enable_gpio;
> struct gpio_desc *hpd_gpio;
>
> + struct edid *edid;
> +
> struct drm_display_mode override_mode;
>
> enum drm_panel_orientation orientation;
> @@ -345,6 +347,9 @@ static int panel_simple_suspend(struct device *dev)
> regulator_disable(p->supply);
> p->unprepared_time = ktime_get();
>
> + kfree(p->edid);
> + p->edid = NULL;
> +
> return 0;
> }
>
> @@ -510,15 +515,13 @@ static int panel_simple_get_modes(struct drm_panel *panel,
>
> /* probe EDID if a DDC bus is available */
> if (p->ddc) {
> - struct edid *edid;
> -
> pm_runtime_get_sync(panel->dev);
>
> - edid = drm_get_edid(connector, p->ddc);
> - if (edid) {
> - num += drm_add_edid_modes(connector, edid);
> - kfree(edid);
> - }
> + if (!p->edid)
> + p->edid = drm_get_edid(connector, p->ddc);
> +
> + if (p->edid)
> + num += drm_add_edid_modes(connector, p->edid);
I suppose this would keep banging on the ddc if drm_get_edid()
continuously returns NULL, but maybe that's desireable (it'll succeed
sometime in the future)? At any rate, this is an improvement on
current behavior so it has my vote.
Reviewed-by: Sean Paul <[email protected]>
>
> pm_runtime_mark_last_busy(panel->dev);
> pm_runtime_put_autosuspend(panel->dev);
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Apr 23, 2021 at 1:00 PM Douglas Anderson <[email protected]> wrote:
>
> As of commit 5186421cbfe2 ("drm: Introduce epoch counter to
> drm_connector") the drm_get_edid() function calls
> drm_connector_update_edid_property() for us. There's no reason for us
> to call it again.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> Reviewed-by: Bjorn Andersson <[email protected]>
Reviewed-by: Sean Paul <[email protected]>
> ---
> As Laurent pointed out [1] this is actually a pretty common
> problem. His suggestion to do this more broadly is a good idea but
> this series is probably a bit ambitious already so I would suggest
> that be taken up separately.
>
> [1] https://lore.kernel.org/r/[email protected]
>
> (no changes since v1)
>
> drivers/gpu/drm/panel/panel-simple.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index bd208abcbf07..4de33c929a59 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -512,7 +512,6 @@ static int panel_simple_get_modes(struct drm_panel *panel,
> if (p->ddc) {
> struct edid *edid = drm_get_edid(connector, p->ddc);
>
> - drm_connector_update_edid_property(connector, edid);
> if (edid) {
> num += drm_add_edid_modes(connector, edid);
> kfree(edid);
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Apr 23, 2021 at 1:00 PM Douglas Anderson <[email protected]> wrote:
>
> I don't believe that it ever makes sense to read the EDID when a panel
> is not powered and the powering on of the panel is the job of
> prepare(). Let's make sure that this happens before we try to read the
> EDID. We use the pm_runtime functions directly rather than directly
> calling the normal prepare() function because the pm_runtime functions
> are definitely refcounted whereas it's less clear if the prepare() one
> is.
>
> NOTE: I'm not 100% sure how EDID reading was working for folks in the
> past, but I can only assume that it was failing on the initial attempt
> and then working only later. This patch, presumably, will fix that. If
> some panel out there really can read the EDID without powering up and
> it's a big advantage to preserve the old behavior we can add a
> per-panel flag. It appears that providing the DDC bus to the panel in
> the past was somewhat uncommon in any case.
>
Maybe some combination of drivers caching the EDID for panels while
they're already powered and overly broad pm runtime references?
At any rate, this makes sense to me,
Reviewed-by: Sean Paul <[email protected]>
> Signed-off-by: Douglas Anderson <[email protected]>
> Reviewed-by: Bjorn Andersson <[email protected]>
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/panel/panel-simple.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 4de33c929a59..a12dfe8b8d90 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -510,12 +510,18 @@ static int panel_simple_get_modes(struct drm_panel *panel,
>
> /* probe EDID if a DDC bus is available */
> if (p->ddc) {
> - struct edid *edid = drm_get_edid(connector, p->ddc);
> + struct edid *edid;
>
> + pm_runtime_get_sync(panel->dev);
> +
> + edid = drm_get_edid(connector, p->ddc);
> if (edid) {
> num += drm_add_edid_modes(connector, edid);
> kfree(edid);
> }
> +
> + pm_runtime_mark_last_busy(panel->dev);
> + pm_runtime_put_autosuspend(panel->dev);
> }
>
> /* add hard-coded panel modes */
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel